From 635d2f2232c2301ec0aa871a74f638d225eaa3d6 Mon Sep 17 00:00:00 2001 From: Travis Wu Date: Sat, 12 Sep 2026 12:18:27 +0800 Subject: [PATCH] settings: every operator setting loads through one path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Startup read operator configuration as a sequence of hand-written blocks, one per setting, and adding one was optional. Two settings got their block — the action level and the OpenStack credential. Two did not: ConfigureInstanceProfile and ConfigureCubeCOS have no non-test caller, so a profile an operator writes reaches nothing and all twenty-one read-catalogue paths answer "not configured". Both features are documented, exercised by tests, and absent from the shipped binary. Lab validation on the 1cc R630 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. configure(dir) walks a declared list and returns the registry; run calls it and reads no operator configuration itself. The two shipped settings move across unchanged — same files, same formats, same modes, same messages, same behaviour on absent, empty, malformed and group-writable input. Per-setting masks stay per-setting: the level is refused if others may write it, the credential if others may read it. The tests call configure, in two layers. One requires the declared list and the effect table to account for each other, catching a setting nothing loads. The other writes each setting's file into a temporary directory and asserts the returned registry changed, catching a setting loaded into nothing — which passes the first. Registry.Writers reports which backends have a write client wired, so a credential reaching the registry is observable without a network. ADR 0016 slice 1. Signed-off-by: Travis Wu --- cmd/agent/run.go | 54 ++------ cmd/agent/settings.go | 136 ++++++++++++++++++++ cmd/agent/settings_test.go | 223 +++++++++++++++++++++++++++++++++ internal/toolplane/registry.go | 16 +++ 4 files changed, 386 insertions(+), 43 deletions(-) create mode 100644 cmd/agent/settings.go create mode 100644 cmd/agent/settings_test.go 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