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