amadan
Fold and validate CARLOS_INBOUND instead of trusting a raw compare
4 files changed,
+122
−5
internal/host/agent.go+10 −5internal/host/host_test.go+14 −0internal/provision/reconcile.go+39 −0internal/provision/reconcile_test.go+59 −0
diff --git a/internal/host/agent.go b/internal/host/agent.go| index c0ce683fe..a77c7fb7b 100644 |
| --- a/internal/host/agent.go |
| +++ b/internal/host/agent.go |
| @@ -1549,11 +1549,16 @@ func provisionTickConfig(getenv func(string) string) provision.ReconcileConfig { |
| // CARLOS_INBOUND is the same apps-domain-style inert gate as |
| // CARLOS_REGION/CARLOS_POOLS above: unset (or "any") is today's |
| // behaviour exactly, a legal "no constraint" state, never a "feature not |
| - // configured" one. Read verbatim, not trimmed against a closed |
| - // vocabulary here — provision.Reconcile/ReconcileSidecars are what |
| - // compare it against InboundNone, so this stays a pure passthrough like |
| - // Region above it. |
| - inbound := strings.TrimSpace(getenv("CARLOS_INBOUND")) |
| + // configured" one. Unlike Region above it, this IS folded and validated |
| + // here (provision.NormalizeInbound) rather than left a pure passthrough: |
| + // review fix, Task 10 finding — an un-folded "None"/"NONE" or a typo'd |
| + // value compared exact-equal against InboundNone false, so a box meant |
| + // to be no-inbound would silently mint routed instances it cannot serve. |
| + // NormalizeInbound folds case/whitespace and maps any unrecognised value |
| + // to the restrictive reading (with its own log line), so |
| + // provision.Reconcile/ReconcileSidecars downstream see only the closed |
| + // vocabulary and their exact-equality comparisons stay correct. |
| + inbound := provision.NormalizeInbound(getenv("CARLOS_INBOUND")) |
| return provision.ReconcileConfig{ |
| Region: region, |
| Pools: pools, |
diff --git a/internal/host/host_test.go b/internal/host/host_test.go| index be918e582..d6a7c72c8 100644 |
| --- a/internal/host/host_test.go |
| +++ b/internal/host/host_test.go |
| @@ -805,6 +805,20 @@ func TestProvisionTickConfigReadsInbound(t *testing.T) { |
| if got.Inbound != "" { |
| t.Fatal("an unset CARLOS_INBOUND must be an inert default — today's behaviour exactly") |
| } |
| + |
| + // Review fix (Task 10 finding): provisionTickConfig must fold case and |
| + // whitespace, and must read an unrecognised value as restrictive rather |
| + // than letting it silently fall through as "any". These pin the actual |
| + // production reading path (getenv -> provision.NormalizeInbound), not |
| + // just the pure function in the provision package. |
| + got = provisionTickConfig(env(map[string]string{"CARLOS_INBOUND": " NONE "})) |
| + if got.Inbound != "none" { |
| + t.Errorf("Inbound = %q, want none (case/whitespace folded)", got.Inbound) |
| + } |
| + got = provisionTickConfig(env(map[string]string{"CARLOS_INBOUND": "off"})) |
| + if got.Inbound != "none" { |
| + t.Errorf("Inbound = %q, want none (unrecognised value reads restrictive)", got.Inbound) |
| + } |
| } |
| func TestBootstrapIdempotent(t *testing.T) { |
diff --git a/internal/provision/reconcile.go b/internal/provision/reconcile.go| index 075db51bb..c80580d03 100644 |
| --- a/internal/provision/reconcile.go |
| +++ b/internal/provision/reconcile.go |
| @@ -48,6 +48,11 @@ type ReconcileConfig struct { |
| // "no-inbound" placement request against this box) AND by Reconcile's |
| // mint gate below (refuses ALL routed instance minting outright — an |
| // ordinary instance record carries no placement vocabulary to match). |
| + // |
| + // Callers building this from a raw env reading MUST pass it through |
| + // NormalizeInbound first (host/agent.go's provisionTickConfig does) — |
| + // both gates below compare this field with exact string equality |
| + // against InboundNone, so an un-folded value would defeat them silently. |
| Inbound string |
| // DryRun reports what the pass would do and writes nothing — not to the |
| // registry, not anywhere. The backfill mitigation (design §4.3): a |
| @@ -67,6 +72,40 @@ const ( |
| InboundNone = "none" |
| ) |
| +// NormalizeInbound folds a raw CARLOS_INBOUND reading (whitespace and case) |
| +// and maps it onto the closed vocabulary above. Review fix (Task 10 |
| +// finding): the box-side comparisons this feeds are exact string equality |
| +// against InboundNone, so an un-folded "None"/"NONE"/"off" fell through as |
| +// if the variable were unset — a box meant to be no-inbound would silently |
| +// get handed traffic it structurally cannot serve, the same silent outage |
| +// this task exists to prevent, just triggered from the other direction. |
| +// |
| +// An UNRECOGNISED value (anything other than "", "any" or "none" once |
| +// folded) is treated as InboundNone — restrictive, not inert — and logged |
| +// loudly, naming the offending value. The only way to produce an |
| +// unrecognised value is to have typed one: CARLOS_INBOUND is never set by |
| +// anything but an operator's own hand, and an operator only sets this |
| +// variable at all when they mean to restrict. So the restrictive reading |
| +// matches intent in the overwhelmingly likely case, and it is this repo's |
| +// house style for an unparseable declaration already (a provision manifest |
| +// with schema > 1 is refused outright, never guessed at). The failure mode |
| +// of guessing wrong here is loud and bounded — new mints are refused with a |
| +// log line naming host and reason, nothing already running is evacuated — |
| +// where the opposite default's failure mode is a box quietly accepting |
| +// traffic it cannot serve. |
| +func NormalizeInbound(raw string) string { |
| + v := strings.ToLower(strings.TrimSpace(raw)) |
| + switch v { |
| + case "", InboundAny, InboundNone: |
| + return v |
| + default: |
| + log.Printf("provision: CARLOS_INBOUND=%q is not a recognised value (want \"\", %q or %q); "+ |
| + "treating this box as CARLOS_INBOUND=%s (no-inbound, restrictive) rather than guessing it means unrestricted", |
| + raw, InboundAny, InboundNone, InboundNone) |
| + return InboundNone |
| + } |
| +} |
| + |
| // acctApp identifies one app within one account — the unit both the record |
| // scan and the manifest/seed cache below key on, since app names are unique |
| // only within an account, never globally. |
diff --git a/internal/provision/reconcile_test.go b/internal/provision/reconcile_test.go| index 099aee210..d7c1c8c46 100644 |
| --- a/internal/provision/reconcile_test.go |
| +++ b/internal/provision/reconcile_test.go |
| @@ -1936,6 +1936,65 @@ func TestReconcileNoInboundBoxRefusesRoutedInstances(t *testing.T) { |
| } |
| } |
| +// TestNormalizeInboundRestrictiveVocabulary pins the case/whitespace-folded |
| +// "none" spellings (review fix, Task 10 finding): "none", "NONE" and |
| +// " None " must all normalize to the one restrictive value the mint/ |
| +// placement gates compare against with exact equality, regardless of how an |
| +// operator typed it. |
| +func TestNormalizeInboundRestrictiveVocabulary(t *testing.T) { |
| + for _, raw := range []string{"none", "NONE", " None "} { |
| + if got := NormalizeInbound(raw); got != InboundNone { |
| + t.Errorf("NormalizeInbound(%q) = %q, want %q", raw, got, InboundNone) |
| + } |
| + } |
| +} |
| + |
| +// TestNormalizeInboundInertVocabulary pins the case/whitespace-folded |
| +// "any"/unset spellings: "", "any" and "ANY" must all normalize to a value |
| +// that is NOT InboundNone, and a box configured with any of them must mint a |
| +// routed instance exactly as it always has — the closed vocabulary must |
| +// never make a deliberate "any" spelling behave like an unrecognised one. |
| +func TestNormalizeInboundInertVocabulary(t *testing.T) { |
| + for _, raw := range []string{"", "any", "ANY"} { |
| + got := NormalizeInbound(raw) |
| + if got == InboundNone { |
| + t.Fatalf("NormalizeInbound(%q) = %q, must not be restrictive", raw, got) |
| + } |
| + |
| + s, sqid := setup(t) |
| + if err := CreateRecord(ctx, s, acmeRecord(sqid)); err != nil { |
| + t.Fatal(err) |
| + } |
| + reg := regWith(t) |
| + cfg := ReconcileConfig{Region: "ie", Pools: []string{"tito"}, DataDir: "/data", Inbound: got} |
| + if err := Reconcile(ctx, s, reg, cfg); err != nil { |
| + t.Fatal(err) |
| + } |
| + mustGet(t, reg, "acme.go.tito.io") |
| + } |
| +} |
| + |
| +// TestNormalizeInboundUnrecognisedIsRestrictiveAndLogged pins the review |
| +// fix's ruling directly (Task 10 finding): the only way to produce an |
| +// unrecognised CARLOS_INBOUND value is to have typed one, and an operator |
| +// only sets this variable at all when they mean to restrict — so an |
| +// unrecognised value must normalize to the restrictive reading AND be |
| +// logged loudly, naming the offending value, so a fat-fingered box is |
| +// caught on the next log tail rather than by a misplaced workload. |
| +func TestNormalizeInboundUnrecognisedIsRestrictiveAndLogged(t *testing.T) { |
| + var logged bytes.Buffer |
| + log.SetOutput(&logged) |
| + t.Cleanup(func() { log.SetOutput(os.Stderr) }) |
| + |
| + got := NormalizeInbound("off") |
| + if got != InboundNone { |
| + t.Fatalf("NormalizeInbound(%q) = %q, want restrictive %q", "off", got, InboundNone) |
| + } |
| + if !strings.Contains(logged.String(), "off") { |
| + t.Fatalf("unrecognised value must be logged by name, got: %s", logged.String()) |
| + } |
| +} |
| + |
| // TestReconcileUnsetInboundMintsAsBefore pins the inert-default half: a box |
| // whose CARLOS_INBOUND is unset (the zero value read by provisionTickConfig) |
| // must mint exactly as it always has — the new guard must never fire for |