From 912a81c59b6794c5ea91add64aee1f447ab66144 Mon Sep 17 00:00:00 2001 From: Vishal Rana Date: Tue, 18 Aug 2026 12:48:28 -0700 Subject: [PATCH] fix: validate environment names, and read the owner record one way MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects found by review of the merged #71, both created by it. That change made the environment name load-bearing: bootstrap writes it into the host owner record and every later command parses it back through a strict grammar. Nothing validated it. `environments:` was the one name-bearing map in validate.go whose key was never checked against gIdent, which was harmless while the name was a lookup key and nothing else. So `Staging`, `prod_east` and `staging replica` all passed `ob validate`, and all three are rejected by the parser that reads the record back. Bootstrap writes the record and succeeds; every mutation afterwards refuses a record it cannot parse, permanently, with no remedy in the message. A legal config bricked the host. This is the item filed as #54 and downgraded to low earlier the same day, on the grounds that the name no longer reached a shell or a filename. That was true when it was written and stopped being true a few hours later, in this repository, without the issue being revisited. Preflight also disagreed with the engine. It read the first two fields of the record and ignored anything after them, so a three-field record passed preflight and then failed every mutation on the engine's parser. A check that is more permissive than the thing it predicts is worse than no check, so there is now one parser — app.ParseHostOwnerRecord — used by both. The refusal for an unparseable record now names its remedy, matching the empty record beside it: no ob command rewrites a record it cannot read, so the operator has to be told to remove it. Tests cover both, and both fail without the fix. Removing the validator gate: --- FAIL: TestEnvironmentNamesMustSurviveTheOwnerRecord environment name "Staging" was accepted by the loader but cannot round-trip the owner record `just check`, `golangci-lint run ./...` and `go test -race ./...` pass. Closes #54. Co-Authored-By: Claude Opus 5 (1M context) --- internal/app/host_owner_probe.go | 57 +++++++++++++++++++++++++++++- internal/app/preflight.go | 27 ++++++++------ internal/app/preflight_test.go | 48 +++++++++++++++++++++++++ internal/app/validate.go | 10 ++++++ internal/engine/host_owner.go | 24 ++++++------- internal/engine/host_owner_test.go | 14 ++++++-- 6 files changed, 153 insertions(+), 27 deletions(-) diff --git a/internal/app/host_owner_probe.go b/internal/app/host_owner_probe.go index 09878696..6878c26b 100644 --- a/internal/app/host_owner_probe.go +++ b/internal/app/host_owner_probe.go @@ -1,6 +1,9 @@ package app -import "path" +import ( + "path" + "strings" +) // Probe exit codes. They are a contract between these scripts and every caller // that classifies their answer — preflight, the engine's readHostOwner, status, @@ -84,3 +87,55 @@ func HostOwnerProbe(recordPath string) string { // one unreadable file. "if [ ! -r " + p + " ]; then exit 2; fi; cat " + p } + +// HostOwnerRecord is the parsed content of the host owner record: the +// application that claimed the host, and the environment it claimed it for. +// +// The record is a single line, "" or " ". +// The first form predates the environment field; it still identifies the owner, +// so it parses rather than failing, and Environment is empty. +type HostOwnerRecord struct { + Application string + Environment string +} + +// Legacy reports a record written before the environment was recorded. +func (r HostOwnerRecord) Legacy() bool { return r.Environment == "" } + +// String renders the record as it is written to the host. +func (r HostOwnerRecord) String() string { + if r.Legacy() { + return r.Application + } + return r.Application + " " + r.Environment +} + +// ParseHostOwnerRecord reads a host owner record. +// +// It lives here, beside the probe that reads the file, because the engine and +// preflight must agree about it exactly. They did not: preflight used to read +// the first two fields and ignore anything after them, so a three-field record +// passed preflight and then failed every mutation on the parser the engine +// actually uses. Preflight exists to predict the engine's answer, and a check +// more permissive than the thing it predicts is worse than no check. +// +// Both names are held to gIdent, the grammar the loader applies to an +// application and — since environment names are validated too — to an +// environment. Anything else is not a record this tool wrote. +func ParseHostOwnerRecord(record string) (HostOwnerRecord, bool) { + fields := strings.Fields(record) + switch len(fields) { + case 1: + if !gIdent.pattern.MatchString(fields[0]) { + return HostOwnerRecord{}, false + } + return HostOwnerRecord{Application: fields[0]}, true + case 2: + if !gIdent.pattern.MatchString(fields[0]) || !gIdent.pattern.MatchString(fields[1]) { + return HostOwnerRecord{}, false + } + return HostOwnerRecord{Application: fields[0], Environment: fields[1]}, true + default: + return HostOwnerRecord{}, false + } +} diff --git a/internal/app/preflight.go b/internal/app/preflight.go index 71e98943..b8215634 100644 --- a/internal/app/preflight.go +++ b/internal/app/preflight.go @@ -188,23 +188,30 @@ func hostOwnerCheck(ctx context.Context, run Runner, path, application, environm Remedy: "inspect the host state directory; an empty record is not a valid claim", } } - // The record is "" or " ". The first - // form predates the environment field; it still identifies the application, - // so it passes the check it can answer and says what it cannot. - fields := strings.Fields(record) - if fields[0] != application { - return Check{Name: "host owner", Detail: fmt.Sprintf("host is owned by application %s", fields[0]), Remedy: "choose an unowned host; Onebox supports one application owner per host"} - } - if len(fields) < 2 { + // The engine's parser, not a second reading of the same file. Preflight + // exists to predict what the engine will do, so anything it accepts that the + // engine rejects is a green light for a refused deploy. + owner, ok := ParseHostOwnerRecord(record) + if !ok { + return Check{ + Name: "host owner", + Detail: "the host owner record is present but is not a record Onebox wrote", + Remedy: "inspect it on the host; no ob command repairs an unparseable record, so remove it and run ob bootstrap", + } + } + if owner.Application != application { + return Check{Name: "host owner", Detail: fmt.Sprintf("host is owned by application %s", owner.Application), Remedy: "choose an unowned host; Onebox supports one application owner per host"} + } + if owner.Legacy() { return Check{Name: "host owner", OK: true, Detail: application + " (claimed before environments were recorded; ob bootstrap will complete it)"} } - if fields[1] != environment { + if owner.Environment != environment { // Every runtime name is application-scoped, so a second environment on // this host would reuse the first one's containers and volumes rather // than collide with them. Nothing downstream can see the difference. return Check{ Name: "host owner", - Detail: fmt.Sprintf("host is claimed by the %s environment of %s", fields[1], application), + Detail: fmt.Sprintf("host is claimed by the %s environment of %s", owner.Environment, application), Remedy: "choose a host for this environment; two environments on one host would share container and volume names", } } diff --git a/internal/app/preflight_test.go b/internal/app/preflight_test.go index d7c09fdb..021c4bda 100644 --- a/internal/app/preflight_test.go +++ b/internal/app/preflight_test.go @@ -410,3 +410,51 @@ func TestPreflightOwnerProbeCarriesTheEngineGuards(t *testing.T) { } } } + +// Preflight exists to predict what the engine will do, so it must accept +// exactly the records the engine's parser accepts. It used to read the first +// two fields and ignore the rest: a three-field record passed preflight and +// then failed every mutation on the parser the engine actually uses. +func TestHostOwnerRecordParsesTheSameForPreflightAndEngine(t *testing.T) { + for _, tc := range []struct { + record string + ok bool + }{ + {"sample", true}, + {"sample production", true}, + {" sample production ", true}, + {"sample production extra", false}, + {"Sample production", false}, + {"sample Production", false}, + {"sample prod_east", false}, + {"", false}, + } { + if _, ok := ParseHostOwnerRecord(tc.record); ok != tc.ok { + t.Fatalf("ParseHostOwnerRecord(%q) ok = %v, want %v", tc.record, ok, tc.ok) + } + } +} + +// An environment name the loader accepts but the owner-record grammar rejects +// claims a host no later command can operate: bootstrap writes the record, and +// every mutation after it refuses a record it cannot parse. +func TestEnvironmentNamesMustSurviveTheOwnerRecord(t *testing.T) { + for _, name := range []string{"Staging", "prod_east", "staging replica", "-lead", "trail-"} { + src := "api_version: onebox.run/v1\napp: sample\nenvironments:\n \"" + name + + "\": {server: root@h}\nworkloads:\n web: {role: application, image: x:1}\n" + if _, err := LoadBytes([]byte(src), "ob.yml"); err == nil { + t.Fatalf("environment name %q was accepted by the loader but cannot round-trip the owner record", name) + } + } + // And the ones that are legal stay legal. + for _, name := range []string{"production", "staging", "prod-east"} { + src := "api_version: onebox.run/v1\napp: sample\nenvironments:\n " + name + + ": {server: root@h}\nworkloads:\n web: {role: application, image: x:1}\n" + if _, err := LoadBytes([]byte(src), "ob.yml"); err != nil { + t.Fatalf("environment name %q should be accepted: %v", name, err) + } + if _, ok := ParseHostOwnerRecord("sample " + name); !ok { + t.Fatalf("environment name %q is accepted by the loader but not by the owner-record parser", name) + } + } +} diff --git a/internal/app/validate.go b/internal/app/validate.go index d3abab57..a0fd7b4a 100644 --- a/internal/app/validate.go +++ b/internal/app/validate.go @@ -18,6 +18,16 @@ func validateSpec(p *Spec) error { } for _, name := range sortedKeys(p.Environments) { + // The name, not only the block under it. Every other map here checks its + // key, and this one did not — which was harmless while the environment + // was a lookup key and nothing else. It is now written into the host + // owner record and read back through a grammar, so a name this validator + // accepts and that parser rejects claims a host no later command can + // operate: bootstrap writes the record, and every mutation after it + // refuses an owner record it cannot parse. + if err := gIdent.check("environments."+name, name); err != nil { + return err + } if err := validateEnvironment(p.Environments[name], "environments."+name); err != nil { return err } diff --git a/internal/engine/host_owner.go b/internal/engine/host_owner.go index 79ab3bb8..47bcbdb0 100644 --- a/internal/engine/host_owner.go +++ b/internal/engine/host_owner.go @@ -62,21 +62,14 @@ type hostOwner struct { func (o hostOwner) legacy() bool { return o.Environment == "" } func parseHostOwner(record string) (hostOwner, bool) { - fields := strings.Fields(record) - switch len(fields) { - case 1: - if !appNameRe.MatchString(fields[0]) { - return hostOwner{}, false - } - return hostOwner{App: fields[0]}, true - case 2: - if !appNameRe.MatchString(fields[0]) || !appNameRe.MatchString(fields[1]) { - return hostOwner{}, false - } - return hostOwner{App: fields[0], Environment: fields[1]}, true - default: + // One parser, shared with preflight. Two readings of the same file drift, + // and the drift showed: preflight read the first two fields and ignored the + // rest, so a three-field record passed there and failed here. + parsed, ok := app.ParseHostOwnerRecord(record) + if !ok { return hostOwner{}, false } + return hostOwner{App: parsed.Application, Environment: parsed.Environment}, true } func (o hostOwner) record() string { @@ -126,7 +119,10 @@ func (e *Engine) readHostOwner(ctx context.Context) (hostOwner, error) { if record == "" { return hostOwner{}, fmt.Errorf("host owner record %s is present but empty, which no ob command can repair; remove it on the host, then run `ob bootstrap`", path) } - return hostOwner{}, fmt.Errorf("host owner record %s is invalid; inspect it before retrying", path) + // Say what repairs it. An unparseable record refuses every mutation for + // as long as it exists, and no ob command rewrites one it cannot read — + // the same dead end the empty record above has, which does say so. + return hostOwner{}, fmt.Errorf("host owner record %s is not a record Onebox wrote, and no ob command can repair it; remove it on the host, then run `ob bootstrap`", path) } return owner, nil } diff --git a/internal/engine/host_owner_test.go b/internal/engine/host_owner_test.go index 811f6e57..db0b391d 100644 --- a/internal/engine/host_owner_test.go +++ b/internal/engine/host_owner_test.go @@ -116,8 +116,18 @@ func TestHostOwnerInvalidRecordFailsClosed(t *testing.T) { return transport.Result{}, false }} engine := New(testConfig(), testProject(t), fake, Options{Out: &bytes.Buffer{}, Sleep: noSleep}) - if err := engine.RequireHostOwner(context.Background()); err == nil || !strings.Contains(err.Error(), "record") || !strings.Contains(err.Error(), "invalid") { - t.Fatalf("invalid owner record was accepted: %v", err) + // The refusal must name the record and say what repairs it. An unparseable + // record blocks every mutation for as long as it exists, and no ob command + // rewrites one it cannot read, so an operator who is not told to remove it + // has no way forward. + err := engine.RequireHostOwner(context.Background()) + if err == nil { + t.Fatal("invalid owner record was accepted") + } + for _, want := range []string{"record", "remove it", "ob bootstrap"} { + if !strings.Contains(err.Error(), want) { + t.Fatalf("refusal does not mention %q: %v", want, err) + } } }