diff --git a/cmd/agent/run.go b/cmd/agent/run.go index 3843a26..0f28ce9 100644 --- a/cmd/agent/run.go +++ b/cmd/agent/run.go @@ -2,7 +2,6 @@ package main import ( "context" - "errors" "flag" "fmt" "log" @@ -15,7 +14,6 @@ import ( "github.com/bigstack-oss/cube-advisor-agent/internal/agent" "github.com/bigstack-oss/cube-advisor-agent/internal/console" "github.com/bigstack-oss/cube-advisor-agent/internal/identity" - "github.com/bigstack-oss/cube-advisor-agent/internal/openstack" "github.com/bigstack-oss/cube-advisor-agent/internal/toolplane" "github.com/bigstack-oss/cube-advisor-agent/pkg/tunnel" "github.com/bigstack-oss/cube-advisor-agent/pkg/tunnelproto" @@ -91,54 +89,24 @@ func runCmd(args []string) int { } opts = append(opts, toolplane.WithProbes(probeRunner)) } - // The cluster's own action level (ADR 0011), read once here so a malformed - // value is one loud line at startup rather than a mystery repeated per - // call. A missing or empty file is not an error and means observe; a word - // that is not a level is an error, and the agent still starts — at observe, - // serving reads — because refusing to run would take diagnosis away from - // the operator at exactly the moment they need it. - level, err := toolplane.ReadLevel(*dir) - if err != nil { - fmt.Fprintf(os.Stderr, "run: %v; serving %s until it is corrected\n", err, level) - } - opts = append(opts, toolplane.WithLevel(level)) - - reg, err := toolplane.New(toolplane.Allowlist, auditor, opts...) + // Every per-cluster setting the operator configured, through the one path + // that reads them (ADR 0016). A setting absent from that list does not + // exist, which is what stops the next one shipping unwired. + reg, states, err := configure(*dir, toolplane.Allowlist, auditor, opts...) if err != nil { fmt.Fprintf(os.Stderr, "run: %v\n", err) return exitFailed } - - // The OpenStack credential, if the operator has written one. Absent is the - // ordinary state and not an error: an agent with no credential serves - // every read it always did and refuses a create with a message naming the - // missing configuration — which is a different refusal from the action - // level's, and says so. - // - // Wired only when present, so opting in is writing a file, and nothing - // about an existing deployment changes until someone does. - switch cred, err := openstack.ReadCredential(*dir); { - case errors.Is(err, openstack.ErrNoCredential): - // Said once, at startup, because "why did it refuse" is a question - // better answered before it is asked. - fmt.Fprintf(os.Stderr, "run: no OpenStack credential; creates will refuse until one is configured\n") - case err != nil: - // Loud, and still starts: a malformed credential must not take - // diagnosis away from the operator at the moment they need it, which - // is the same argument the action level makes. - fmt.Fprintf(os.Stderr, "run: %v; creates will refuse until it is corrected\n", err) - default: - compute, cerr := openstack.NewCompute(cred) - if cerr != nil { - fmt.Fprintf(os.Stderr, "run: %v; creates will refuse until it is corrected\n", cerr) - break - } - reg.ConfigureWriter(toolplane.BackendOpenStackCompute, compute) - fmt.Fprintf(os.Stderr, "run: OpenStack credential loaded for project %s\n", cred.Project) + // One line per setting, whatever happened to it. A broken setting disables + // what it enables and never more: the agent starts anyway, because + // refusing to run would take diagnosis away from the operator at exactly + // the moment they need it. + for _, st := range states { + 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", level, len(reg.Names())) + fmt.Fprintf(os.Stderr, "run: action level %s; serving %d tool(s)\n", reg.Level(), len(reg.Names())) ctx, stop := signal.NotifyContext(context.Background(), syscall.SIGINT, syscall.SIGTERM) defer stop() diff --git a/cmd/agent/settings.go b/cmd/agent/settings.go new file mode 100644 index 0000000..5836818 --- /dev/null +++ b/cmd/agent/settings.go @@ -0,0 +1,136 @@ +package main + +import ( + "errors" + "fmt" + + "github.com/bigstack-oss/cube-advisor-agent/internal/openstack" + "github.com/bigstack-oss/cube-advisor-agent/internal/toolplane" +) + +// A setting is one thing the cluster's operator configures, as a file in the +// agent's own directory (ADR 0016). +// +// The list below is the whole set. configure walks it and nothing else reads +// operator configuration, so a setting absent from the list does not exist. +// That is the point: startup used to be a sequence of hand-written blocks +// where adding one was optional, and two settings that never got their block +// shipped documented, tested and unreachable. +type setting struct { + // name is what an operator sees in the startup line. + name string + // file is the setting's file in the agent's directory, named in messages + // so that "not configured" says which configuration. + file string + // load reads the setting from dir. It always returns a line, and returns + // an opt or an apply when the setting is usable. + load func(dir string) settingState +} + +// settingState is what one setting resolved to. +type settingState struct { + // line is the one startup line this setting owes, whatever happened: its + // value, that it is not configured, or why it was rejected. + line string + // broken marks a setting an operator wrote that this agent could not + // honour, as distinct from one they never wrote. Only the first is worth + // shouting about. + broken bool + // opt applies the setting when the registry is built; apply applies it + // afterwards. A setting uses whichever its target needs. + opt toolplane.Option + apply func(*toolplane.Registry) +} + +// settings is every per-cluster setting this agent reads. Adding one is adding +// an entry here, and there is no second place to forget. +var settings = []setting{actionLevel, openStackCredential} + +// actionLevel is the cluster's own action level (ADR 0011). +// +// Read once at startup so a malformed value is one loud line rather than a +// mystery repeated per call. Absent, empty and whitespace all mean observe and +// are not errors; a word that is not a level is an error, and the level is +// still applied, because ReadLevel answers observe alongside it and serving +// reads beats refusing to start. +var actionLevel = setting{ + name: "action level", + file: toolplane.LevelFileName, + load: func(dir string) settingState { + level, err := toolplane.ReadLevel(dir) + st := settingState{opt: toolplane.WithLevel(level)} + if err != nil { + st.broken = true + st.line = fmt.Sprintf("action level: %v; serving %s until it is corrected", err, level) + return st + } + st.line = fmt.Sprintf("action level: %s", level) + return st + }, +} + +// openStackCredential is the application credential creates are made with. +// +// Absent is the ordinary state of every cluster that has not opted in: it +// refuses creates with a message naming the missing configuration, which is a +// different refusal from the action level's and says so. Wired only when +// present, so opting in is writing a file. +var openStackCredential = setting{ + name: "OpenStack credential", + file: openstack.CredentialFileName, + load: func(dir string) settingState { + cred, err := openstack.ReadCredential(dir) + switch { + case errors.Is(err, openstack.ErrNoCredential): + return settingState{line: "OpenStack credential: not configured; creates will refuse until one is"} + case err != nil: + return settingState{ + broken: true, + line: fmt.Sprintf("OpenStack credential: %v; creates will refuse until it is corrected", err), + } + } + compute, err := openstack.NewCompute(cred) + if err != nil { + return settingState{ + broken: true, + line: fmt.Sprintf("OpenStack credential: %v; creates will refuse until it is corrected", err), + } + } + return settingState{ + line: fmt.Sprintf("OpenStack credential: loaded for project %s", cred.Project), + apply: func(r *toolplane.Registry) { + r.ConfigureWriter(toolplane.BackendOpenStackCompute, compute) + }, + } + }, +} + +// configure builds the tool plane from the operator's configuration in dir. +// +// The one path every setting takes. run calls it and reads no operator +// configuration itself; `config check` will call the same function, because a +// second validator would be this design's own defect one level up. +// +// extra carries options that are not operator configuration — the probe plane, +// which a flag enables. +func configure(dir string, tools []toolplane.Tool, audit toolplane.Auditor, extra ...toolplane.Option) (*toolplane.Registry, []settingState, error) { + states := make([]settingState, len(settings)) + opts := make([]toolplane.Option, 0, len(extra)+len(settings)) + opts = append(opts, extra...) + for i, s := range settings { + states[i] = s.load(dir) + if states[i].opt != nil { + opts = append(opts, states[i].opt) + } + } + reg, err := toolplane.New(tools, audit, opts...) + if err != nil { + return nil, states, err + } + for _, st := range states { + if st.apply != nil { + st.apply(reg) + } + } + return reg, states, nil +} diff --git a/cmd/agent/settings_test.go b/cmd/agent/settings_test.go new file mode 100644 index 0000000..9c81c2e --- /dev/null +++ b/cmd/agent/settings_test.go @@ -0,0 +1,223 @@ +package main + +import ( + "encoding/json" + "io" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/bigstack-oss/cube-advisor-agent/internal/openstack" + "github.com/bigstack-oss/cube-advisor-agent/internal/toolplane" +) + +// These tests call configure — the function run calls — against a temporary +// directory. That is the whole point of them. +// +// ConfigureInstanceProfile and ConfigureCubeCOS shipped documented, exercised +// by tests, and never called: cmd/agent/run.go had no block for either, and +// lab validation on real hardware was the first thing to notice. Every test +// that found them green built its own Registry, so none of them ever asked +// whether anything builds one in production. A test that does not run the +// startup path cannot catch this, however thorough it is about everything else. +// +// Two layers, because exhaustiveness alone is not enough. The first catches a +// setting nothing loads; the second catches a setting loaded into nothing, +// which would pass the first. + +// effect is one declared setting's observable consequence, written here rather +// than derived from settings so that the two must agree with each other. +type effect struct { + name string + file string + content string + mode os.FileMode + // observe reads back the part of the registry this setting controls. + observe func(*toolplane.Registry) string +} + +func effects(t *testing.T) []effect { + t.Helper() + cred, err := json.Marshal(openstack.Credential{ + AuthURL: "https://10.0.0.1:5000/v3", + ID: "an-application-credential", + Secret: "not-a-real-secret", + Project: "advisor-lab", + }) + if err != nil { + t.Fatal(err) + } + return []effect{ + { + name: "action level", + file: toolplane.LevelFileName, + content: "operate\n", + // Readable by anyone, writable by nobody else: the level is a + // policy statement, and ReadLevel refuses a file others may write. + mode: 0o644, + observe: func(r *toolplane.Registry) string { return r.Level().String() }, + }, + { + name: "OpenStack credential", + file: openstack.CredentialFileName, + content: string(cred), + // Owner only: it holds a secret, and ReadCredential refuses + // anything a group or the world may read. + mode: 0o600, + observe: func(r *toolplane.Registry) string { + names := make([]string, 0, 2) + for _, b := range r.Writers() { + names = append(names, b.String()) + } + return strings.Join(names, ",") + }, + }, + } +} + +// A setting configure loads but nothing asserts, or asserts but configure does +// not load, is the failure this catches. Adding a setting to the list without +// an effect case fails here rather than shipping unobserved. +func TestEverySettingIsDeclaredAndAsserted(t *testing.T) { + declared := map[string]string{} + for _, s := range settings { + if _, dup := declared[s.name]; dup { + t.Errorf("setting %q is declared twice", s.name) + } + if s.load == nil { + t.Errorf("setting %q loads nothing", s.name) + } + declared[s.name] = s.file + } + + asserted := map[string]string{} + for _, e := range effects(t) { + asserted[e.name] = e.file + } + + for name, file := range declared { + want, ok := asserted[name] + if !ok { + t.Errorf("configure loads setting %q but no test asserts it has any effect", name) + continue + } + if want != file { + t.Errorf("setting %q reads %q, the test writes %q", name, file, want) + } + } + for name, file := range asserted { + if _, ok := declared[name]; !ok { + t.Errorf("setting %q has an effect test but configure does not load it: an operator writing %s would change nothing", + name, file) + } + } +} + +// Each declared setting must reach the registry through configure. A list +// entry wired to nothing passes the exhaustiveness check above and fails here. +func TestEverySettingReachesTheRegistryThroughConfigure(t *testing.T) { + for _, e := range effects(t) { + t.Run(e.name, func(t *testing.T) { + unset := e.observe(configureIn(t, t.TempDir())) + + dir := t.TempDir() + writeSetting(t, dir, e) + set := e.observe(configureIn(t, dir)) + + if set == unset { + t.Fatalf("writing %s changed nothing: %s reads %q with and without it — the setting is declared but wired to nothing", + e.file, e.name, set) + } + }) + } +} + +// A setting an operator wrote and this agent cannot honour is marked broken, +// and the agent still configures: a malformed credential must not take +// diagnosis away at the moment it is needed. +func TestABrokenSettingIsMarkedAndStillConfigures(t *testing.T) { + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, toolplane.LevelFileName), []byte("operator\n"), 0o644); err != nil { + t.Fatal(err) + } + + reg, states, err := configure(dir, toolplane.Allowlist, discardAuditor()) + if err != nil { + t.Fatalf("configure refused to build a registry over a malformed setting: %v", err) + } + if reg.Level() != toolplane.DefaultLevel { + t.Errorf("Level() = %s over a malformed file, want %s", reg.Level(), toolplane.DefaultLevel) + } + + var broken []string + for _, st := range states { + if st.line == "" { + t.Error("a setting reported no startup line") + } + if st.broken { + broken = append(broken, st.line) + } + } + if len(broken) != 1 { + t.Fatalf("broken settings = %d, want exactly the malformed action level: %v", len(broken), broken) + } + if !strings.Contains(broken[0], "operator") { + t.Errorf("the line should quote what the operator wrote, so they can see their own typo: %q", broken[0]) + } +} + +// Nothing configured is the ordinary state of a cluster that has not opted in: +// every setting still reports, and the agent serves reads. +func TestAnUnconfiguredDirectoryReportsEverySettingAndServesReads(t *testing.T) { + reg, states, err := configure(t.TempDir(), toolplane.Allowlist, discardAuditor()) + if err != nil { + t.Fatal(err) + } + if len(states) != len(settings) { + t.Fatalf("states = %d, want one per declared setting (%d)", len(states), len(settings)) + } + for i, st := range states { + if st.line == "" { + t.Errorf("setting %q reported no startup line", settings[i].name) + } + if st.broken { + t.Errorf("setting %q is broken when nothing is configured: %s", settings[i].name, st.line) + } + } + if reg.Level() != toolplane.DefaultLevel { + t.Errorf("Level() = %s with nothing configured, want %s", reg.Level(), toolplane.DefaultLevel) + } + if len(reg.Names()) == 0 { + t.Error("an unconfigured agent serves no tools; it should still serve its reads") + } +} + +func configureIn(t *testing.T, dir string) *toolplane.Registry { + t.Helper() + reg, _, err := configure(dir, toolplane.Allowlist, discardAuditor()) + if err != nil { + t.Fatalf("configure(%s): %v", dir, err) + } + return reg +} + +func writeSetting(t *testing.T, dir string, e effect) { + t.Helper() + if err := os.WriteFile(filepath.Join(dir, e.file), []byte(e.content), e.mode); err != nil { + t.Fatal(err) + } + // WriteFile's mode is masked by the process umask, and both settings check + // their own mode. Set it explicitly so the test does not depend on one. + if err := os.Chmod(filepath.Join(dir, e.file), e.mode); err != nil { + t.Fatal(err) + } +} + +func discardAuditor() toolplane.Auditor { + return toolplane.NewWriterAuditor(nopWriteCloser{io.Discard}) +} + +type nopWriteCloser struct{ io.Writer } + +func (nopWriteCloser) Close() error { return nil } diff --git a/internal/toolplane/registry.go b/internal/toolplane/registry.go index 72979dd..7141c9f 100644 --- a/internal/toolplane/registry.go +++ b/internal/toolplane/registry.go @@ -230,6 +230,22 @@ func (r *Registry) ConfigureWriter(b Backend, pw Poster) { } } +// Writers reports the backends with an authenticated write client wired, in a +// stable order. +// +// ConfigureWriter's observable counterpart. Without it, a credential on disk +// reaching this registry is only visible by making a call, so nothing could +// assert the wiring without a network — which is how ConfigureInstanceProfile +// and ConfigureCubeCOS shipped documented, tested and never called. +func (r *Registry) Writers() []Backend { + out := make([]Backend, 0, len(r.writers)) + for b := range r.writers { + out = append(out, b) + } + sort.Slice(out, func(i, j int) bool { return out[i] < out[j] }) + return out +} + // writerFor returns the client for a backend, or a refusing default. // // The default is per backend and says what is missing, because "not