fix: validate environment names, and read the owner record one way

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, "<application>" or "<application> <environment>".
+// 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
+	}
+}

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 "<application>" or "<application> <environment>". 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",
 		}
 	}

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

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
 		}

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
 }

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