From 183bc0c088ee3f9fa28cd99776843274bf9f0dbd Mon Sep 17 00:00:00 2001 From: Travis Wu Date: Sat, 12 Sep 2026 12:51:28 +0800 Subject: [PATCH] settings: a cluster says what it creates ConfigureInstanceProfile had no non-test caller. No flag and no file supplied a profile, so every create refused with "this agent has no flavor configured; it cannot create anything until its operator sets one" -- a message naming something the operator could not act on. Lab validation on the 1cc R630 was the first thing to notice, because every test that found the profile green built its own Registry. The profile becomes a setting in the list slice 1 introduced, so it loads through the one path and the two layers that catch an unloaded or unwired setting apply to it. instance-profile.json holds flavor, image and network, with project optional; it is refused if group- or world-writable and not if readable, the action level's rule rather than the credential's -- it holds no secret, but whoever may write it chooses the image, and choosing the image is choosing what code runs. Unknown fields are rejected rather than ignored. The likely typo is "flavour", the spelling this codebase's own prose uses, and ignoring it would leave the field empty and refuse every create with a message about a flavour the operator can see in their file. executorFilled becomes a map from placeholder to the file that supplies it, so an unconfigured create names instance-profile.json rather than leaving an operator to find it. {dc} carries no file until cube-cos-api access becomes a setting, and falls back to the shorter wording rather than inventing a path. Registry.Profile is ConfigureInstanceProfile's observable counterpart, added for the reason Writers was: without it a profile on disk reaching the registry is visible only by creating against a real cloud, which is how this shipped documented, tested and never called. Signed-off-by: Travis Wu --- cmd/agent/settings.go | 33 +++++- cmd/agent/settings_test.go | 51 ++++++++- internal/toolplane/allowlist.go | 6 +- internal/toolplane/profile.go | 100 +++++++++++++++++ internal/toolplane/profile_test.go | 169 +++++++++++++++++++++++++++++ internal/toolplane/registry.go | 10 ++ internal/toolplane/write.go | 34 ++++-- 7 files changed, 386 insertions(+), 17 deletions(-) create mode 100644 internal/toolplane/profile.go create mode 100644 internal/toolplane/profile_test.go diff --git a/cmd/agent/settings.go b/cmd/agent/settings.go index 5836818..10fd0cb 100644 --- a/cmd/agent/settings.go +++ b/cmd/agent/settings.go @@ -44,7 +44,7 @@ type settingState struct { // 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} +var settings = []setting{actionLevel, instanceProfile, openStackCredential} // actionLevel is the cluster's own action level (ADR 0011). // @@ -69,6 +69,37 @@ var actionLevel = setting{ }, } +// instanceProfile is what this cluster creates: flavour, image and network +// (ADR 0016, slice 3). +// +// Absent is the ordinary state and not an error — a cluster that has not opted +// in creates nothing, and says which file would change that. A file the +// operator wrote and this agent cannot honour is broken and not applied, so a +// half-configured profile creates nothing rather than something half-chosen: +// the empty field would otherwise reach the resolver, which refuses anyway, +// but one loud line at startup beats the same refusal discovered per call. +var instanceProfile = setting{ + name: "instance profile", + file: toolplane.ProfileFileName, + load: func(dir string) settingState { + profile, err := toolplane.ReadInstanceProfile(dir) + switch { + case errors.Is(err, toolplane.ErrNoProfile): + return settingState{line: "instance profile: not configured; creates will refuse until one is"} + case err != nil: + return settingState{ + broken: true, + line: fmt.Sprintf("instance profile: %v; creates will refuse until it is corrected", err), + } + } + return settingState{ + line: fmt.Sprintf("instance profile: flavor %s, image %s, network %s", + profile.Flavor, profile.Image, profile.Network), + apply: func(r *toolplane.Registry) { r.ConfigureInstanceProfile(profile) }, + } + }, +} + // openStackCredential is the application credential creates are made with. // // Absent is the ordinary state of every cluster that has not opted in: it diff --git a/cmd/agent/settings_test.go b/cmd/agent/settings_test.go index 9c81c2e..278420a 100644 --- a/cmd/agent/settings_test.go +++ b/cmd/agent/settings_test.go @@ -58,6 +58,18 @@ func effects(t *testing.T) []effect { mode: 0o644, observe: func(r *toolplane.Registry) string { return r.Level().String() }, }, + { + name: "instance profile", + file: toolplane.ProfileFileName, + content: `{"flavor":"m1.large","image":"ubuntu-24.04","network":"tenant-net"}` + "\n", + // The action level's rule, not the credential's: no secret, but + // whoever may write it chooses the image. + mode: 0o644, + observe: func(r *toolplane.Registry) string { + p := r.Profile() + return strings.Join([]string{p.Flavor, p.Image, p.Network}, "/") + }, + }, { name: "OpenStack credential", file: openstack.CredentialFileName, @@ -167,6 +179,41 @@ func TestABrokenSettingIsMarkedAndStillConfigures(t *testing.T) { } } +// A profile naming two of three fields is broken rather than unset, and is not +// applied: a half-configured profile creates nothing rather than something +// half-chosen. The agent still configures and still serves its reads. +func TestAHalfConfiguredProfileIsBrokenAndNotApplied(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, toolplane.ProfileFileName) + if err := os.WriteFile(path, []byte(`{"flavor":"m1.large","image":"ubuntu-24.04"}`), 0o644); err != nil { + t.Fatal(err) + } + if err := os.Chmod(path, 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 half-configured profile: %v", err) + } + if got := reg.Profile(); got.Flavor != "" || got.Image != "" { + t.Errorf("Profile() = %+v over a half-configured file, want nothing applied", got) + } + + var broken []string + for _, st := range states { + if st.broken { + broken = append(broken, st.line) + } + } + if len(broken) != 1 { + t.Fatalf("broken settings = %d, want exactly the profile: %v", len(broken), broken) + } + if !strings.Contains(broken[0], "network") { + t.Errorf("the line should name the missing field, not the file in general: %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) { @@ -207,8 +254,8 @@ func writeSetting(t *testing.T, dir string, e effect) { 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. + // WriteFile's mode is masked by the process umask, and every setting checks + // its 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) } diff --git a/internal/toolplane/allowlist.go b/internal/toolplane/allowlist.go index 8e369ae..77a9882 100644 --- a/internal/toolplane/allowlist.go +++ b/internal/toolplane/allowlist.go @@ -808,19 +808,19 @@ func (t Tool) validateGet() error { if _, declared := t.Params[dcPlaceholder]; declared { return fmt.Errorf("tool %q declares %s; it is executor context, not a parameter", t.Name, dcPlaceholder) } - return t.checkPlaceholders(pathSegments(t.Get), map[string]bool{dcPlaceholder: true}) + return t.checkPlaceholders(pathSegments(t.Get), map[string]string{dcPlaceholder: ""}) } // checkPlaceholders enforces that every placeholder in tokens is either an // exempt executor slot or a declared model parameter, and that every declared // parameter is used with a finite value set. -func (t Tool) checkPlaceholders(tokens []string, exempt map[string]bool) error { +func (t Tool) checkPlaceholders(tokens []string, exempt map[string]string) error { seen := map[string]bool{} for _, tok := range tokens { if !isPlaceholder(tok) { continue } - if exempt[tok] { + if _, ok := exempt[tok]; ok { continue } _, enumerated := t.Params[tok] diff --git a/internal/toolplane/profile.go b/internal/toolplane/profile.go new file mode 100644 index 0000000..f592bf5 --- /dev/null +++ b/internal/toolplane/profile.go @@ -0,0 +1,100 @@ +package toolplane + +import ( + "bytes" + "encoding/json" + "fmt" + "os" + "path/filepath" +) + +// ProfileFileName is the file in the agent's own directory that describes what +// this cluster creates (ADR 0011, ADR 0016). Beside the action level and the +// enrollment identity, for the reason that directory holds all of them: it is +// per-cluster state the customer's operator owns, already exists, and already +// has its modes enforced. +const ProfileFileName = "instance-profile.json" + +// ErrNoProfile is returned when no profile file exists. +// +// Distinguished from a malformed one because they are different operator +// problems: one is configuration not yet done, the other is configuration done +// wrongly. Both refuse a create; only the second is worth shouting about at +// startup. Same split as ErrNoCredential. +var ErrNoProfile = fmt.Errorf("no instance profile is configured") + +// profileFile is the on-disk shape. Separate from InstanceProfile so the JSON +// names are stated once, here, rather than as tags on a type that also travels +// through the resolver. +type profileFile struct { + Flavor string `json:"flavor"` + Image string `json:"image"` + Network string `json:"network"` + Project string `json:"project,omitempty"` +} + +// ReadInstanceProfile loads the instance profile from dir. +// +// Absent is ErrNoProfile and not a failure: an agent with no profile is the +// ordinary state of every cluster that has not opted in, and it refuses creates +// cleanly rather than refusing to start. +// +// A group- or world-writable file is refused, and readability deliberately is +// not. This is the action level's rule rather than the credential's, and the +// asymmetry is the point: the profile holds no secret — flavour, image and +// network are ids of shared cloud resources, and an operator should be able to +// read what their own agent creates without root. But whoever can write it +// chooses the image, and choosing the image is choosing what code runs on the +// instance this agent will be asked to create. Write access is the escalation; +// read access reveals nothing. +// +// Unknown fields are rejected rather than ignored. The obvious typo here is +// "flavour" — the spelling this codebase's own prose uses — and ignoring it +// would leave the field empty, refusing every create with a message about a +// flavour the operator believes they configured. +func ReadInstanceProfile(dir string) (InstanceProfile, error) { + path := filepath.Join(dir, ProfileFileName) + + info, err := os.Stat(path) + if os.IsNotExist(err) { + return InstanceProfile{}, ErrNoProfile + } + if err != nil { + return InstanceProfile{}, fmt.Errorf("toolplane: stat %s: %w", path, err) + } + if perm := info.Mode().Perm(); perm&0o022 != 0 { + return InstanceProfile{}, fmt.Errorf( + "toolplane: %s has mode %04o; it must not be group- or world-writable — anyone who can write it chooses the image this cluster boots", + path, perm) + } + + b, err := os.ReadFile(path) + if err != nil { + return InstanceProfile{}, fmt.Errorf("toolplane: read %s: %w", path, err) + } + + var f profileFile + dec := json.NewDecoder(bytes.NewReader(b)) + dec.DisallowUnknownFields() + if err := dec.Decode(&f); err != nil { + return InstanceProfile{}, fmt.Errorf("toolplane: %s: %v", path, err) + } + + for _, field := range []struct{ name, value string }{ + {"flavor", f.Flavor}, + {"image", f.Image}, + {"network", f.Network}, + } { + if field.value == "" { + return InstanceProfile{}, fmt.Errorf( + "toolplane: %s has no %s; a half-configured profile creates nothing", path, field.name) + } + } + + return InstanceProfile{ + Flavor: f.Flavor, + Image: f.Image, + Network: f.Network, + Project: f.Project, + }, nil +} diff --git a/internal/toolplane/profile_test.go b/internal/toolplane/profile_test.go new file mode 100644 index 0000000..92e32df --- /dev/null +++ b/internal/toolplane/profile_test.go @@ -0,0 +1,169 @@ +package toolplane + +import ( + "errors" + "os" + "path/filepath" + "strings" + "testing" +) + +func writeProfile(t *testing.T, dir, content string, mode os.FileMode) { + t.Helper() + path := filepath.Join(dir, ProfileFileName) + if err := os.WriteFile(path, []byte(content), mode); err != nil { + t.Fatal(err) + } + if err := os.Chmod(path, mode); err != nil { + t.Fatal(err) + } +} + +const goodProfile = `{"flavor":"m1.large","image":"ubuntu-24.04","network":"tenant-net"}` + +// A cluster that has not opted in is not broken. It creates nothing and says +// so, which is a different thing from a profile its operator got wrong. +func TestNoProfileIsNotAnError(t *testing.T) { + _, err := ReadInstanceProfile(t.TempDir()) + if !errors.Is(err, ErrNoProfile) { + t.Fatalf("ReadInstanceProfile with no file = %v, want ErrNoProfile", err) + } +} + +func TestAProfileIsReadWhole(t *testing.T) { + dir := t.TempDir() + writeProfile(t, dir, goodProfile, 0o644) + + p, err := ReadInstanceProfile(dir) + if err != nil { + t.Fatal(err) + } + if p.Flavor != "m1.large" || p.Image != "ubuntu-24.04" || p.Network != "tenant-net" { + t.Fatalf("ReadInstanceProfile = %+v, want the three fields the file names", p) + } +} + +// Each of the three is required, and the message names the one that is missing +// rather than the file in general: an operator reading it should not have to +// diff their own file against the documentation. +func TestAHalfConfiguredProfileIsRefusedNamingTheMissingField(t *testing.T) { + for _, tc := range []struct{ name, content, want string }{ + {"no flavor", `{"image":"i","network":"n"}`, "flavor"}, + {"no image", `{"flavor":"f","network":"n"}`, "image"}, + {"no network", `{"flavor":"f","image":"i"}`, "network"}, + } { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + writeProfile(t, dir, tc.content, 0o644) + + _, err := ReadInstanceProfile(dir) + if err == nil { + t.Fatal("a profile missing a required field was accepted") + } + if !strings.Contains(err.Error(), tc.want) { + t.Errorf("error = %q, want it to name the missing %s", err, tc.want) + } + }) + } +} + +// "flavour" is the spelling this codebase's own prose uses, so it is the typo +// an operator is most likely to make. Ignoring the unknown key would leave +// Flavor empty and refuse every create with a message about a flavour they can +// see in their file. +func TestABritishFlavourIsRejectedRatherThanIgnored(t *testing.T) { + dir := t.TempDir() + writeProfile(t, dir, `{"flavour":"m1.large","image":"i","network":"n"}`, 0o644) + + _, err := ReadInstanceProfile(dir) + if err == nil { + t.Fatal("an unknown field was ignored; the operator would never learn why their flavour was not used") + } + if !strings.Contains(err.Error(), "flavour") { + t.Errorf("error = %q, want it to quote the key the operator wrote", err) + } +} + +// Whoever may write this file chooses the image, and choosing the image is +// choosing what code runs. Readability is deliberately not constrained. +func TestAProfileOthersCanWriteIsRefused(t *testing.T) { + dir := t.TempDir() + writeProfile(t, dir, goodProfile, 0o666) + + if _, err := ReadInstanceProfile(dir); err == nil { + t.Fatal("a world-writable profile was accepted; anyone local could choose the image") + } + + other := t.TempDir() + writeProfile(t, other, goodProfile, 0o644) + if _, err := ReadInstanceProfile(other); err != nil { + t.Fatalf("a world-readable profile was refused: %v — it holds no secret", err) + } +} + +// project is optional: nova takes the project from the credential's scope, so +// the field is a declaration for the operator and the approval statement, not +// a value the request carries. +func TestProjectIsOptional(t *testing.T) { + dir := t.TempDir() + writeProfile(t, dir, goodProfile, 0o644) + p, err := ReadInstanceProfile(dir) + if err != nil { + t.Fatal(err) + } + if p.Project != "" { + t.Errorf("Project = %q with none in the file, want empty", p.Project) + } + + withProject := t.TempDir() + writeProfile(t, withProject, `{"flavor":"f","image":"i","network":"n","project":"advisor-lab"}`, 0o644) + p, err = ReadInstanceProfile(withProject) + if err != nil { + t.Fatal(err) + } + if p.Project != "advisor-lab" { + t.Errorf("Project = %q, want the file's value", p.Project) + } +} + +// Every placeholder the profile supplies must name the profile file, one by +// one. +// +// The end-to-end test below is not enough on its own and this is why: a Post +// tool's body is a map, so the resolver reaches its placeholders in whatever +// order Go iterates, and any one of the three satisfies "the message names the +// file". Breaking a single placeholder's attribution left that test green — +// found by breaking it, not by reading it. This one is per placeholder and +// deterministic. +func TestEveryProfilePlaceholderNamesTheProfileFile(t *testing.T) { + for _, ph := range []string{flavorPlaceholder, imagePlaceholder, networkPlaceholder, projectPlaceholder} { + if got := executorFilled[ph]; got != ProfileFileName { + t.Errorf("executorFilled[%s] = %q, want %s: an unconfigured create would not say where to write it", + ph, got, ProfileFileName) + } + } +} + +// The refusal an operator actually meets when they raise the level and stop. +// ADR 0016's own slice table asks for this: absent refuses a create naming the +// file, because "no flavor configured" leaves them hunting for where to write +// it. +func TestAnUnconfiguredCreateRefusesNamingTheFile(t *testing.T) { + var create Tool + for _, tool := range Allowlist { + if tool.Name == "create_instance" { + create = tool + } + } + if create.Name == "" { + t.Fatal("create_instance is not in the allowlist") + } + + _, _, err := create.resolveWrite(map[string]string{"{name}": "web-03"}, map[string]string{}) + if err == nil { + t.Fatal("a create resolved with no profile configured") + } + if !strings.Contains(err.Error(), ProfileFileName) { + t.Errorf("error = %q, want it to name %s", err, ProfileFileName) + } +} diff --git a/internal/toolplane/registry.go b/internal/toolplane/registry.go index 7141c9f..bee9974 100644 --- a/internal/toolplane/registry.go +++ b/internal/toolplane/registry.go @@ -216,6 +216,16 @@ func (r *Registry) Level() Level { return r.level } // instance gets a clean refusal rather than a surprising default. func (r *Registry) ConfigureInstanceProfile(p InstanceProfile) { r.profile = p } +// Profile reports what this agent creates, ConfigureInstanceProfile's +// observable counterpart. +// +// It exists for the same reason Writers does: without it, a profile on disk +// reaching this registry is visible only by attempting a create against a real +// cloud, so nothing could assert the wiring without a network — which is how +// ConfigureInstanceProfile shipped documented, tested and never called. It +// holds no secret; the credential beside it does, and that one is not exposed. +func (r *Registry) Profile() InstanceProfile { return r.profile } + // ConfigureWriter wires the authenticated write client for one backend. // Separate from ConfigureCubeCOS so an agent can read the management API // without being able to write anywhere: an operator who wires only the reader diff --git a/internal/toolplane/write.go b/internal/toolplane/write.go index 10b9da8..40795b1 100644 --- a/internal/toolplane/write.go +++ b/internal/toolplane/write.go @@ -56,15 +56,23 @@ const ( projectPlaceholder = "{project}" ) -// executorFilled is the set above, as a lookup. A placeholder in it must not -// appear in Params or Free: declaring it as a caller argument is how it would -// stop being executor context. -var executorFilled = map[string]bool{ - dcPlaceholder: true, - flavorPlaceholder: true, - imagePlaceholder: true, - networkPlaceholder: true, - projectPlaceholder: true, +// executorFilled is the set above, as a lookup from placeholder to the +// operator file that supplies it. A placeholder in it must not appear in +// Params or Free: declaring it as a caller argument is how it would stop being +// executor context. +// +// The file name is carried so an unconfigured create can say which file to +// write. "This agent has no flavor configured" is true and leaves an operator +// hunting; naming the file is the difference between a message they can read +// and one they can act on. An empty value means no file supplies it yet — +// {dc} until cube-cos-api access becomes a setting — and the refusal falls +// back to the shorter wording rather than inventing a path. +var executorFilled = map[string]string{ + dcPlaceholder: "", + flavorPlaceholder: ProfileFileName, + imagePlaceholder: ProfileFileName, + networkPlaceholder: ProfileFileName, + projectPlaceholder: ProfileFileName, } // InstanceProfile is the cluster's answer to "created how?" — everything about @@ -207,7 +215,7 @@ func idempotencyKey(tool, path string, body map[string]string) string { // like it chose something it did not. func (t Tool) resolveWrite(args map[string]string, ctxValues map[string]string) (string, map[string]string, error) { for k := range args { - if executorFilled[k] { + if _, filled := executorFilled[k]; filled { return "", nil, fmt.Errorf("argument %s is executor context, not a caller argument", k) } _, enumerated := t.Params[k] @@ -221,9 +229,13 @@ func (t Tool) resolveWrite(args map[string]string, ctxValues map[string]string) if !isPlaceholder(tok) { return tok, nil } - if executorFilled[tok] { + if file, filled := executorFilled[tok]; filled { v := ctxValues[tok] if v == "" { + if file != "" { + return "", fmt.Errorf("this agent has no %s configured in %s; it cannot create anything until its operator sets one", + strings.Trim(tok, "{}"), file) + } return "", fmt.Errorf("this agent has no %s configured; it cannot create anything until its operator sets one", strings.Trim(tok, "{}")) } return v, nil