diff --git a/cmd/agent/config.go b/cmd/agent/config.go new file mode 100644 index 0000000..fa35606 --- /dev/null +++ b/cmd/agent/config.go @@ -0,0 +1,146 @@ +package main + +import ( + "flag" + "fmt" + "io" + "os" + "strings" + + "github.com/bigstack-oss/cube-advisor-agent/internal/identity" + "github.com/bigstack-oss/cube-advisor-agent/internal/toolplane" +) + +// exitConfigBroken says a setting an operator wrote could not be honoured, as +// distinct from exitFailed, which says the check itself could not run. +// +// The two want different actions — correct a file, versus report a bug — and +// an installer that can only see "non-zero" has to parse messages to tell +// them apart, which is how messages become an interface nobody meant to +// define. +const exitConfigBroken = 6 + +// configCmd is `advisor-agent config `. +func configCmd(args []string) int { + if len(args) < 1 { + configUsage() + return exitUsage + } + switch args[0] { + case "check": + return configCheckCmd(args[1:]) + default: + configUsage() + return exitUsage + } +} + +func configUsage() { + fmt.Fprintf(os.Stderr, `cube-advisor-agent config + + check [-dir ] [-probes] report what each setting resolves to +`) +} + +// configCheckCmd answers "is this cluster configured the way I meant?" before +// anything starts (ADR 0016). +// +// It calls configure — the function run calls — and reads no operator +// configuration itself. A second validator would be this design's own defect +// one level up: the settings list would have two readers, and the one nobody +// ran would drift from the one that decides. +// +// It never loads the identity, dials the tunnel, or builds an agent.Server, so +// it answers on a node that has not enrolled and cannot start serving by +// accident. The imports of this file are the short version of that argument, +// and TestConfigCheckStartsNothing asserts them. +func configCheckCmd(args []string) int { + fs := flag.NewFlagSet("config check", flag.ContinueOnError) + dir := fs.String("dir", identity.DefaultDir, + "the agent directory whose settings to check; a staging copy is checked by pointing this at it") + probes := fs.Bool("probes", false, + "check what `run -probes` would serve: the probe plane changes the tool count, not any setting") + if err := fs.Parse(args); err != nil { + return exitUsage + } + + // Mirrors run's own probe option so the tool count answers for the agent + // the operator will actually start. Building a ProbeRunner validates the + // probe list and touches nothing else; the sweeper that does touch scratch + // is started by run, separately, and never here. + var opts []toolplane.Option + if *probes { + runner, err := toolplane.NewProbeRunner(toolplane.Probes, checkAuditor()) + if err != nil { + fmt.Fprintf(os.Stderr, "config check: %v\n", err) + return exitFailed + } + opts = append(opts, toolplane.WithProbes(runner)) + } + + reg, states, err := configure(*dir, toolplane.Allowlist, checkAuditor(), opts...) + if err != nil { + fmt.Fprintf(os.Stderr, "config check: %v\n", err) + return exitFailed + } + + // The same lines run prints at startup, because they are the same states. + // They go to stdout here and to stderr there: in run they are log context + // beside everything else the process says, and here they are the answer. + var broken []string + for i, st := range states { + fmt.Println(st.line) + if st.broken { + broken = append(broken, settings[i].name) + } + } + fmt.Println(summaryLine(reg)) + + if len(broken) > 0 { + fmt.Fprintf(os.Stderr, "config check: %s broken (%s); the agent would start and refuse what they enable\n", + plural(len(broken), "setting is", "settings are"), strings.Join(broken, ", ")) + return exitConfigBroken + } + fmt.Println(uncheckedNotice) + return exitOK +} + +// uncheckedNotice is printed on success because "every setting is fine" is a +// narrower claim than it reads as. +// +// The line ADR 0016 drew is syntactic at load, semantic at use: these files +// parse, their modes are safe and their values are well-formed, but whether a +// flavour id exists on this cloud needs the credential and a network call, and +// a checker that sometimes talks to a cluster is a different tool with +// different failure modes — one an operator learns to ignore the first time it +// fails because the cloud was busy. +const uncheckedNotice = "checked: every file parses, its mode is safe and its values are well-formed. " + + "Not checked: that these ids exist on this cluster, that the credential redeems, " + + "or that the api answers — the first create and the first read are what prove those." + +// checkAuditor is the auditor a check builds its registry with. +// +// A check serves no tool call, so nothing is ever written. A FileAuditor would +// create or open the audit log, which is a change made by a command whose +// whole purpose is to change nothing. +func checkAuditor() toolplane.Auditor { + return toolplane.NewWriterAuditor(discardWriteCloser{io.Discard}) +} + +type discardWriteCloser struct{ io.Writer } + +func (discardWriteCloser) Close() error { return nil } + +// summaryLine is what this agent would serve, for run's startup log and for a +// check's last line. One formatting, so the two cannot describe the same +// registry differently. +func summaryLine(reg *toolplane.Registry) string { + return fmt.Sprintf("action level %s; serving %d tool(s)", reg.Level(), len(reg.Names())) +} + +func plural(n int, one, many string) string { + if n == 1 { + return "1 " + one + } + return fmt.Sprintf("%d %s", n, many) +} diff --git a/cmd/agent/config_test.go b/cmd/agent/config_test.go new file mode 100644 index 0000000..22602ea --- /dev/null +++ b/cmd/agent/config_test.go @@ -0,0 +1,194 @@ +package main + +import ( + "go/parser" + "go/token" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/bigstack-oss/cube-advisor-agent/internal/toolplane" +) + +// These tests reach `config check` through dispatch — main's own switch — +// rather than by calling configCheckCmd directly. +// +// A subcommand tested only through its own function is a subcommand that can +// be deleted from the switch and still pass. That is not hypothetical here: +// this track has twice shipped something correct and unreachable, most +// recently a tunnel frame whose approval flag no test crossed, so the +// mechanism was right and guarded by nothing. + +// A directory where every setting is written and well-formed is the state an +// operator is trying to reach, and it must exit zero. +func TestConfigCheckExitsZeroWhenEverySettingIsValid(t *testing.T) { + dir := t.TempDir() + for _, e := range effects(t) { + writeSetting(t, dir, e) + } + + code, out, _ := configCheck(t, "-dir", dir) + if code != exitOK { + t.Fatalf("exit = %d, want %d for a fully configured directory:\n%s", code, exitOK, out) + } + for _, e := range effects(t) { + if !strings.Contains(out, e.name+":") { + t.Errorf("output does not report setting %q:\n%s", e.name, out) + } + } + if !strings.Contains(out, "Not checked:") { + t.Errorf("a passing check should say what it did not check, or it reads as a stronger claim than it is:\n%s", out) + } +} + +// Absent is the ordinary state of a cluster that has not opted in, and every +// setting but the action level is optional. Nothing configured is not an +// operator error, so it must not exit non-zero. +func TestConfigCheckExitsZeroOnAnUnconfiguredDirectory(t *testing.T) { + code, out, _ := configCheck(t, "-dir", t.TempDir()) + if code != exitOK { + t.Fatalf("exit = %d, want %d: nothing configured is a state, not a mistake:\n%s", code, exitOK, out) + } + if !strings.Contains(out, "not configured") { + t.Errorf("an unconfigured directory should say so per setting:\n%s", out) + } +} + +// A setting the operator wrote and this agent cannot honour is what the +// command exists to find: it exits non-zero and names which one. +func TestConfigCheckReportsABrokenSettingAndExitsNonZero(t *testing.T) { + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, toolplane.LevelFileName), []byte("operator\n"), 0o644); err != nil { + t.Fatal(err) + } + + code, out, errOut := configCheck(t, "-dir", dir) + if code != exitConfigBroken { + t.Fatalf("exit = %d, want %d for a broken setting:\n%s%s", code, exitConfigBroken, out, errOut) + } + if !strings.Contains(errOut, "action level") { + t.Errorf("the failure should name the broken setting, not just fail:\n%s", errOut) + } + if !strings.Contains(out, "operator") { + t.Errorf("the report should quote what the operator wrote, so they can see their own typo:\n%s", out) + } + if strings.Contains(out, "Not checked:") { + t.Error("a failing check must not print the reassurance a passing one does") + } +} + +// The command reports exactly what configure resolved, because it is the same +// states. A second vocabulary describing one registry is this ADR's own defect +// one level up. +func TestConfigCheckReportsTheStatesConfigureReturns(t *testing.T) { + dir := t.TempDir() + for _, e := range effects(t) { + writeSetting(t, dir, e) + } + + _, states, err := configure(dir, toolplane.Allowlist, checkAuditor()) + if err != nil { + t.Fatal(err) + } + _, out, _ := configCheck(t, "-dir", dir) + for _, st := range states { + if !strings.Contains(out, st.line) { + t.Errorf("configure resolved %q and the check did not print it:\n%s", st.line, out) + } + } +} + +// The command must not start the agent: it answers on a node that has not +// enrolled, and cannot begin serving by accident. +// +// The behavioural half is that a directory holding settings but no identity +// still exits zero — run would refuse there. The structural half reads this +// file's imports: dialling needs the tunnel, serving needs agent.Server, and a +// console needs console. This is enforcement rather than a guarantee, since a +// helper in another file could still reach them, but it catches the change +// that would actually be written. +func TestConfigCheckStartsNothing(t *testing.T) { + dir := t.TempDir() + for _, e := range effects(t) { + writeSetting(t, dir, e) + } + if code, out, _ := configCheck(t, "-dir", dir); code != exitOK { + t.Fatalf("exit = %d on an unenrolled node, want %d: a check must not need an identity:\n%s", code, exitOK, out) + } + + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, "config.go", nil, parser.ImportsOnly) + if err != nil { + t.Fatal(err) + } + forbidden := []string{"pkg/tunnel", "internal/agent", "internal/console"} + for _, imp := range f.Imports { + path := strings.Trim(imp.Path.Value, `"`) + for _, bad := range forbidden { + if strings.HasSuffix(path, bad) { + t.Errorf("config.go imports %s: a check that can dial or serve is no longer a check", path) + } + } + } +} + +// An unknown subcommand under config is a usage error, not a silent success. +func TestConfigRejectsAnUnknownSubcommand(t *testing.T) { + if code := dispatch([]string{"config", "sniff"}); code != exitUsage { + t.Errorf("dispatch(config sniff) = %d, want %d", code, exitUsage) + } + if code := dispatch([]string{"config"}); code != exitUsage { + t.Errorf("dispatch(config) = %d, want %d", code, exitUsage) + } +} + +// configCheck runs `config check` the way an operator does — through main's +// switch — and captures what it printed. +func configCheck(t *testing.T, args ...string) (code int, stdout, stderr string) { + t.Helper() + outR, outW, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + errR, errW, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + realOut, realErr := os.Stdout, os.Stderr + os.Stdout, os.Stderr = outW, errW + + done := make(chan struct{}) + var outBuf, errBuf strings.Builder + go func() { + defer close(done) + copyInto(&outBuf, outR) + }() + errDone := make(chan struct{}) + go func() { + defer close(errDone) + copyInto(&errBuf, errR) + }() + + code = dispatch(append([]string{"config", "check"}, args...)) + + os.Stdout, os.Stderr = realOut, realErr + _ = outW.Close() + _ = errW.Close() + <-done + <-errDone + return code, outBuf.String(), errBuf.String() +} + +func copyInto(dst *strings.Builder, r *os.File) { + buf := make([]byte, 4096) + for { + n, err := r.Read(buf) + if n > 0 { + dst.Write(buf[:n]) + } + if err != nil { + return + } + } +} diff --git a/cmd/agent/main.go b/cmd/agent/main.go index 8de821b..709d42e 100644 --- a/cmd/agent/main.go +++ b/cmd/agent/main.go @@ -45,23 +45,35 @@ const ( ) func main() { - if len(os.Args) < 2 { + os.Exit(dispatch(os.Args[1:])) +} + +// dispatch routes one subcommand and returns its exit code. +// +// Separate from main so a test can reach a subcommand the way an operator +// does — through this switch — rather than by calling its function directly. +// A subcommand whose routing nothing asserts is a subcommand that can be +// removed from here and still pass its own tests. +func dispatch(args []string) int { + if len(args) < 1 { usage() - os.Exit(exitUsage) + return exitUsage } - switch os.Args[1] { + switch args[0] { case "enroll": - os.Exit(enrollCmd(os.Args[2:])) + return enrollCmd(args[1:]) case "run": - os.Exit(runCmd(os.Args[2:])) + return runCmd(args[1:]) + case "config": + return configCmd(args[1:]) case "status": - os.Exit(statusCmd(os.Args[2:])) + return statusCmd(args[1:]) case "version", "-version", "--version": fmt.Printf("cube-advisor-agent %s (%s)\n", version, commit) - os.Exit(exitOK) + return exitOK default: usage() - os.Exit(exitUsage) + return exitUsage } } @@ -70,6 +82,7 @@ func usage() { enroll -server [-token | -token-file | -token-stdin] [-tunnel ] run [-server ] [-dir ] [-audit ] + config check [-dir ] [-probes] status version `, version) diff --git a/cmd/agent/run.go b/cmd/agent/run.go index 0f28ce9..6e8dcc2 100644 --- a/cmd/agent/run.go +++ b/cmd/agent/run.go @@ -105,8 +105,10 @@ func runCmd(args []string) int { fmt.Fprintf(os.Stderr, "run: %s\n", st.line) } // Stated at startup, because "what may this assistant do here" is the - // question an operator asks of a log and should not have to infer. - fmt.Fprintf(os.Stderr, "run: action level %s; serving %d tool(s)\n", reg.Level(), len(reg.Names())) + // question an operator asks of a log and should not have to infer. The + // same line `config check` ends with, formatted in one place so a check + // and the run it predicts cannot describe one registry differently. + fmt.Fprintf(os.Stderr, "run: %s\n", summaryLine(reg)) ctx, stop := signal.NotifyContext(context.Background(), syscall.SIGINT, syscall.SIGTERM) defer stop() diff --git a/cmd/agent/settings_test.go b/cmd/agent/settings_test.go index c062351..3e03136 100644 --- a/cmd/agent/settings_test.go +++ b/cmd/agent/settings_test.go @@ -2,7 +2,6 @@ package main import ( "encoding/json" - "io" "os" "path/filepath" "strings" @@ -279,10 +278,7 @@ func writeSetting(t *testing.T, dir string, e effect) { } } -func discardAuditor() toolplane.Auditor { - return toolplane.NewWriterAuditor(nopWriteCloser{io.Discard}) -} - -type nopWriteCloser struct{ io.Writer } - -func (nopWriteCloser) Close() error { return nil } +// discardAuditor is checkAuditor, the one `config check` builds registries +// with: these tests and that command both want a registry and no audit log, so +// they should not disagree about how to get one. +func discardAuditor() toolplane.Auditor { return checkAuditor() }