settings: an operator can check before the agent runs

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 <subcommand>`.
+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 <path>] [-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)
+}

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
+		}
+	}
+}

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 <url> [-token <t> | -token-file <path> | -token-stdin] [-tunnel <host:port>]
   run     [-server <host:port>] [-dir <path>] [-audit <path>]
+  config  check [-dir <path>] [-probes]
   status
   version
 `, version)