From c4ebc55a3a7381a03db13cc67009fcb6279faedd Mon Sep 17 00:00:00 2001 From: Travis Wu Date: Sat, 12 Sep 2026 18:31:30 +0800 Subject: [PATCH] settings: an operator can check before the agent runs ADR 0016 slice 2. There are five per-cluster settings now, and the only way to check one was to restart the agent and read its startup lines. `config check` calls configure -- the function run calls -- prints what each setting resolved to, and exits non-zero if any is broken. It is not a parallel validator. A second reader of the settings list is this design's own defect one level up: the one nobody ran would drift from the one that decides. The startup lines are the same states run prints, and the summary line is formatted in one place so a check and the run it predicts cannot describe one registry differently. Absent is not broken. Every setting but the action level is optional, and a cluster that has not opted in is in a state rather than a mistake, so an unconfigured directory exits zero. A broken setting exits 6 rather than 1: correcting a file and reporting a bug want different actions, and an installer that can only see non-zero has to parse messages to tell them apart. A passing check says what it did not check. The line ADR 0016 drew is syntactic at load, semantic at use -- whether a flavour id exists needs the credential and a network call, and a checker that sometimes talks to a cluster is one an operator learns to ignore the first time it fails because the cloud was busy. main's switch moves into dispatch so the tests reach the subcommand the way an operator does. A subcommand tested only through its own function can be deleted from the switch and still pass; removing the case here fails four tests. Signed-off-by: Travis Wu --- cmd/agent/config.go | 146 ++++++++++++++++++++++++++++ cmd/agent/config_test.go | 194 +++++++++++++++++++++++++++++++++++++ cmd/agent/main.go | 29 ++++-- cmd/agent/run.go | 6 +- cmd/agent/settings_test.go | 12 +-- 5 files changed, 369 insertions(+), 18 deletions(-) create mode 100644 cmd/agent/config.go create mode 100644 cmd/agent/config_test.go 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() }