amadan
Merge pull request #355 from carlosframework/trash-route-removal
A trashed app's routes are retired on every box, and static platform hosts are steered by default
7 files changed,
+578
−30
docs/site/cli.md+8 −0internal/console/apiinstances.go+44 −1internal/console/apiinstances_test.go+125 −0internal/host/agent.go+22 −1internal/host/host_test.go+30 −0internal/provision/reconcile.go+101 −21internal/provision/reconcile_test.go+248 −7
diff --git a/docs/site/cli.md b/docs/site/cli.md| index 6749533fe..2e8ee9ec5 100644 |
| --- a/docs/site/cli.md |
| +++ b/docs/site/cli.md |
| @@ -321,6 +321,14 @@ has armed its Route53 steering converger. In v1 only instances on the shared |
| pool can be steered; a pool-scoped route is refused, and the refusal names |
| that as the reason. |
| +A **static site on a platform hostname is steered from the moment it is |
| +created** — you do not have to ask. Static is the one kind every edge can |
| +serve on its own, straight from the channel pointer, so there is no reason |
| +for one edge to answer for all of them. `off` turns it back off if you want |
| +a single answer, and nothing re-applies the default to a host that already |
| +exists. Custom domains are not steered for you, because their DNS is yours: |
| +point them at the steering hostname the app's Settings page names. |
| + |
| ### carlos routes |
| The app's routes on this deployment: where each one sends traffic, which |
diff --git a/internal/console/apiinstances.go b/internal/console/apiinstances.go| index b18c33f07..1eeadc26e 100644 |
| --- a/internal/console/apiinstances.go |
| +++ b/internal/console/apiinstances.go |
| @@ -558,7 +558,13 @@ func (s *Server) handleAPIInstanceCreate(w http.ResponseWriter, r *http.Request) |
| // membership: a label naming another claimed app in the account belongs |
| // to that app. Custom-domain manifests never enter apex space (ValidHost |
| // pinned the suffix above), so this check cannot touch them. |
| - if s.cfg.AppsDomain != "" && m.Domain == acct.Sqid+"."+s.cfg.AppsDomain { |
| + // |
| + // The same question answers a second one further down (the steering |
| + // default), so it is asked once, here, and named: is this manifest |
| + // pinned to the account's platform subdomain, or to a domain of the |
| + // app's own? |
| + platformNamespace := s.cfg.AppsDomain != "" && m.Domain == acct.Sqid+"."+s.cfg.AppsDomain |
| + if platformNamespace { |
| if label := strings.TrimSuffix(host, "."+m.Domain); label != app { |
| claimed, err := s.store.HasAppIn(acct.ID, label) |
| if err != nil { |
| @@ -652,6 +658,43 @@ func (s *Server) handleAPIInstanceCreate(w http.ResponseWriter, r *http.Request) |
| Addrs: req.Addrs, |
| Health: req.Health, |
| } |
| + // A static platform host is HA by default. Static is the one serving |
| + // class every platform box materializes unconditionally |
| + // (provision.everywhereClass): no unit, no process, content read from |
| + // the bucket channel pointer, so a far edge serves it with no home box |
| + // and no backhaul. The content half of that is already true — every |
| + // armed edge holds the site and its account wildcard cert. What was |
| + // missing is DNS: with no opt-in the host resolves to one edge, so |
| + // several healthy boxes hold the site and every visitor is sent to one |
| + // of them. |
| + // |
| + // Scoped to the platform namespace deliberately. internal/steering's |
| + // steeredHosts only ever builds latency records for hosts under the |
| + // apps domain, so defaulting a custom domain's record to "latency" |
| + // would write an opt-in no pass will ever honour (#306's shape); a |
| + // custom domain reaches the edges by its owner's own DNS, and the |
| + // console's attach guidance already tells them to CNAME the steering |
| + // hostname. |
| + // |
| + // Scoped to static deliberately too: a home-boxed route reaches a far |
| + // edge by backhaul, a different trade with a different cost, not one to |
| + // make silently for every app on the deployment. |
| + // |
| + // And it honours v1's pool rail, the same one the PATCH door enforces |
| + // with a 409 (steering needs the shared pool, because a pool's own |
| + // edges are not the deployment's). A default that quietly wrote what |
| + // the update door refuses would be the worse kind of inconsistency: the |
| + // record would carry an opt-in nobody could have asked for through the |
| + // front door, and the converger would skip it anyway. |
| + // |
| + // This is a value at MINT, never a converge: `carlos steering --app x |
| + // --host y off` is the escape hatch, and nothing re-stamps a record |
| + // that already exists. The opt-in is inert on a deployment that has not |
| + // set CARLOS_STEERING_ZONE_ID, so a single-box deployment states the |
| + // intent harmlessly and becomes HA the day it arms a second edge. |
| + if rec.Kind == "static" && platformNamespace && rec.Pool == "" { |
| + rec.Steering = "latency" |
| + } |
| // The shape rails, at the door — kind vocabulary, one upstream shape per |
| // record, addr syntax, and the combinations that would wedge a host (a |
| // static route with an upstream, a hibernating addr-backed one). The |
diff --git a/internal/console/apiinstances_test.go b/internal/console/apiinstances_test.go| index b9a5bd7ff..3edc08607 100644 |
| --- a/internal/console/apiinstances_test.go |
| +++ b/internal/console/apiinstances_test.go |
| @@ -2184,3 +2184,128 @@ func TestAPIInstanceCreateArmedPlacementGate(t *testing.T) { |
| t.Fatalf("create region se (no EIP) = %d %s, want 400", code, out) |
| } |
| } |
| + |
| +// A static site is HA by default. Static is the one serving class every |
| +// platform box materializes unconditionally (provision.everywhereClass): |
| +// no unit, no process, content read from the bucket channel pointer, so a |
| +// far edge serves it with no home box and no backhaul. The content half of |
| +// that was already true and measured — every flagship edge served every |
| +// static host with a valid cert. What was missing was DNS: without an |
| +// opt-in the host resolves to one edge, so four healthy boxes held the site |
| +// and every visitor went to one of them. |
| +// |
| +// The opt-in is inert until a deployment sets CARLOS_STEERING_ZONE_ID, so a |
| +// single-box deployment states the intent harmlessly and becomes HA the day |
| +// it arms a second edge. `carlos steering --app x --host y off` is the |
| +// escape hatch, and nothing here ever converges an existing record. |
| +func TestAPIInstanceCreateStaticSteersByDefault(t *testing.T) { |
| + fx, member := instanceFixture(t, "code-inst-static-steer") |
| + fx.srv.cfg.AppsDomain = "oncarlos.test" |
| + if status, _, out := rawReq(t, http.MethodPut, enableURL(fx, "bab", "titogo"), member, `{}`); status != http.StatusOK { |
| + t.Fatalf("enable = %d %s", status, out) |
| + } |
| + host := "titogo." + fx.sqid + ".oncarlos.test" |
| + |
| + status, _, out := rawReq(t, http.MethodPost, instancesURL(fx, "bab", "titogo"), member, |
| + `{"host":"`+host+`","kind":"static","channel":"edge"}`) |
| + if status != http.StatusCreated { |
| + t.Fatalf("static apex create = %d %s, want 201", status, out) |
| + } |
| + stored, err := provision.GetRecord(context.Background(), fx.rel, fx.sqid, "titogo", host) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if stored.Steering != "latency" { |
| + t.Fatalf("stored Steering = %q, want latency — a static platform host is HA by default", stored.Steering) |
| + } |
| + // Whatever the door writes has to survive ValidateShape, or the record |
| + // it just stored is one no box will ever materialize. |
| + if err := provision.ValidateShape(*stored); err != nil { |
| + t.Fatalf("the defaulted record does not validate: %v", err) |
| + } |
| +} |
| + |
| +// The default is scoped to the platform namespace, and deliberately so. The |
| +// steering converger only ever writes latency records for hosts under the |
| +// apps domain (internal/steering's steeredHosts), so defaulting a custom |
| +// domain's record to "latency" would write an opt-in that does nothing — |
| +// a claim in the record that no pass will honour, which is #306's shape. |
| +// A custom domain reaches the edges by its owner's own DNS, and the console |
| +// already tells them to CNAME the steering hostname. |
| +func TestAPIInstanceCreateStaticCustomDomainDoesNotSteer(t *testing.T) { |
| + fx, member := instanceFixture(t, "code-inst-static-custom") |
| + fx.srv.cfg.AppsDomain = "oncarlos.test" |
| + // The operator-written manifest, pinned to the app's OWN domain rather |
| + // than the account's platform subdomain — the custom-domain shape. |
| + enableInstances(t, fx, "titogo", titoManifest()) |
| + host := "site.go.tito.io" |
| + |
| + status, _, out := rawReq(t, http.MethodPost, instancesURL(fx, "bab", "titogo"), member, |
| + `{"host":"`+host+`","region":"ie","kind":"static","channel":"edge"}`) |
| + if status != http.StatusCreated { |
| + t.Fatalf("custom-domain static create = %d %s, want 201", status, out) |
| + } |
| + stored, err := provision.GetRecord(context.Background(), fx.rel, fx.sqid, "titogo", host) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if stored.Steering != "" { |
| + t.Fatalf("stored Steering = %q, want empty — the converger never steers a host outside the apps domain", stored.Steering) |
| + } |
| +} |
| + |
| +// Only static. A home-boxed instance route reaches a far edge by backhaul, |
| +// which is a different trade with a different cost, and widening this |
| +// default to every kind would make that trade silently for every app on the |
| +// deployment. `carlos steering` remains the door for those, per host. |
| +func TestAPIInstanceCreateNonStaticDoesNotSteer(t *testing.T) { |
| + fx, member := instanceFixture(t, "code-inst-nonstatic-steer") |
| + fx.srv.cfg.AppsDomain = "oncarlos.test" |
| + if status, _, out := rawReq(t, http.MethodPut, enableURL(fx, "bab", "titogo"), member, `{}`); status != http.StatusOK { |
| + t.Fatalf("enable = %d %s", status, out) |
| + } |
| + host := "titogo." + fx.sqid + ".oncarlos.test" |
| + |
| + status, _, out := rawReq(t, http.MethodPost, instancesURL(fx, "bab", "titogo"), member, |
| + `{"host":"`+host+`","channel":"edge"}`) // no kind: the instance default |
| + if status != http.StatusCreated { |
| + t.Fatalf("instance create = %d %s, want 201", status, out) |
| + } |
| + stored, err := provision.GetRecord(context.Background(), fx.rel, fx.sqid, "titogo", host) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if stored.Steering != "" { |
| + t.Fatalf("stored Steering = %q, want empty — only static is steered by default", stored.Steering) |
| + } |
| +} |
| + |
| +// v1's pool rail applies to the default exactly as it applies to the PATCH |
| +// door, which answers 409 for the same request. A default that wrote what |
| +// the update door refuses would put an opt-in in the record that nobody |
| +// could have asked for through the front door — and that the converger |
| +// skips regardless, since a pool's own edges are not the deployment's. |
| +func TestAPIInstanceCreateStaticInAPoolDoesNotSteer(t *testing.T) { |
| + fx, member := instanceFixture(t, "code-inst-static-pooled") |
| + fx.srv.cfg.AppsDomain = "oncarlos.test" |
| + // An operator-written manifest can pin BOTH the account's platform |
| + // subdomain and a pool; the member-facing enable door cannot. |
| + enableInstances(t, fx, "titogo", provision.Manifest{ |
| + Schema: 1, App: "titogo", Domain: fx.sqid + ".oncarlos.test", |
| + Pool: "tito", Channel: "stable", Seed: []string{"seed"}, |
| + }) |
| + host := "titogo." + fx.sqid + ".oncarlos.test" |
| + |
| + status, _, out := rawReq(t, http.MethodPost, instancesURL(fx, "bab", "titogo"), member, |
| + `{"host":"`+host+`","kind":"static","channel":"edge"}`) |
| + if status != http.StatusCreated { |
| + t.Fatalf("pooled static create = %d %s, want 201", status, out) |
| + } |
| + stored, err := provision.GetRecord(context.Background(), fx.rel, fx.sqid, "titogo", host) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if stored.Steering != "" { |
| + t.Fatalf("stored Steering = %q, want empty — steering requires the shared pool", stored.Steering) |
| + } |
| +} |
diff --git a/internal/host/agent.go b/internal/host/agent.go| index c6ead7c4f..0eab657ac 100644 |
| --- a/internal/host/agent.go |
| +++ b/internal/host/agent.go |
| @@ -1680,12 +1680,33 @@ func provisionTickConfig(getenv func(string) string) provision.ReconcileConfig { |
| if dd == "" { |
| dd = "/data" |
| } |
| - return provision.ReconcileConfig{ |
| + cfg := provision.ReconcileConfig{ |
| Region: region, |
| Pools: pools, |
| DataDir: dd, |
| Now: time.Now, |
| } |
| + // The root release public key, same source and same parser as adoptLoop's |
| + // and domainsTickConfig's. It guards ONE decision in this pass: whether a |
| + // trash marker may retire a production-flagged app's routes (#340). |
| + // |
| + // Absence is tolerated and leaves ReleasePub nil, which retires trashed |
| + // routes UNGUARDED — deliberately the same end state domains.Config |
| + // documents and accepts for the same decision (§1c), because two passes |
| + // reaching opposite conclusions about one app is worse than either |
| + // answer. A key SET but unparseable lands in the same place, so it is |
| + // logged here rather than swallowed: it is a persistent misconfiguration, |
| + // and the operator who typo'd it must hear about the typo rather than |
| + // silently losing the guard. |
| + if raw := strings.TrimSpace(getenv("CARLOS_RELEASE_PUBKEY")); raw != "" { |
| + pub, err := release.ParsePublicKey(raw, "CARLOS_RELEASE_PUBKEY") |
| + if err != nil { |
| + log.Printf("agent: instances pass: %v — a trash marker will retire routes without checking the production flag", err) |
| + } else { |
| + cfg.ReleasePub = pub |
| + } |
| + } |
| + return cfg |
| } |
| // resolveHost is the production DNS seam: the default resolver with a hard |
diff --git a/internal/host/host_test.go b/internal/host/host_test.go| index 2dad9eb30..77a958504 100644 |
| --- a/internal/host/host_test.go |
| +++ b/internal/host/host_test.go |
| @@ -1,10 +1,12 @@ |
| package host |
| import ( |
| + "bytes" |
| "context" |
| "crypto/ed25519" |
| "encoding/base64" |
| "encoding/json" |
| + "log" |
| "net" |
| "os" |
| "path/filepath" |
| @@ -808,6 +810,34 @@ func TestProvisionTickConfig(t *testing.T) { |
| if got.Region != "ie" || got.Pools != nil { |
| t.Errorf("Region = %q Pools = %v, want ie / nil", got.Region, got.Pools) |
| } |
| + |
| + // The release pubkey (#340): present and parseable arms the production |
| + // guard on trash-driven route retirement. |
| + if got.ReleasePub != nil { |
| + t.Error("ReleasePub set with CARLOS_RELEASE_PUBKEY unset") |
| + } |
| + _, pubB64, err := release.GenerateKey() |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + got = provisionTickConfig(env(map[string]string{"CARLOS_RELEASE_PUBKEY": pubB64})) |
| + if got.ReleasePub == nil { |
| + t.Error("ReleasePub nil with a valid CARLOS_RELEASE_PUBKEY") |
| + } |
| + |
| + // A key SET but unparseable must not be mistaken for a key that parsed. |
| + // It lands on nil — the same unguarded end state domains.Config accepts |
| + // for the same decision — and says so in the log rather than silently. |
| + var logged bytes.Buffer |
| + log.SetOutput(&logged) |
| + t.Cleanup(func() { log.SetOutput(os.Stderr) }) |
| + got = provisionTickConfig(env(map[string]string{"CARLOS_RELEASE_PUBKEY": "not-a-key"})) |
| + if got.ReleasePub != nil { |
| + t.Error("ReleasePub set from an unparseable CARLOS_RELEASE_PUBKEY") |
| + } |
| + if !strings.Contains(logged.String(), "CARLOS_RELEASE_PUBKEY") { |
| + t.Errorf("an unparseable key was swallowed; log was:\n%s", logged.String()) |
| + } |
| } |
| func TestBootstrapIdempotent(t *testing.T) { |
diff --git a/internal/provision/reconcile.go b/internal/provision/reconcile.go| index f7706bb18..7dcaa45a8 100644 |
| --- a/internal/provision/reconcile.go |
| +++ b/internal/provision/reconcile.go |
| @@ -12,6 +12,7 @@ package provision |
| import ( |
| "context" |
| + "crypto/ed25519" |
| "encoding/json" |
| "errors" |
| "fmt" |
| @@ -48,6 +49,17 @@ type ReconcileConfig struct { |
| // it would mean, because adoption is what grants the removal phase |
| // authority over that row. |
| DryRun bool |
| + // ReleasePub verifies the production flag before a trash marker is |
| + // allowed to retire an app's routes (#340), the same guard and the same |
| + // key domains.Config carries for the same decision. nil leaves the |
| + // retirement UNGUARDED rather than disabling it — domains §1c's posture, |
| + // carried over deliberately so the two passes cannot reach opposite |
| + // conclusions about the same app: a keyless deployment has no way to |
| + // make the guard pass, and holding every trash forever there would break |
| + // hiding entirely. Nothing destructive to the bucket happens on this |
| + // path; the auto-purge that does still lives in domains, still refuses |
| + // without a key. |
| + ReleasePub ed25519.PublicKey |
| } |
| // acctApp identifies one app within one account — the unit both the record |
| @@ -55,6 +67,43 @@ type ReconcileConfig struct { |
| // only within an account, never globally. |
| type acctApp struct{ acct, app string } |
| +// trashState is what one app's trash marker means to this pass. Two fields |
| +// and not one bool, because the two questions have different safe answers: |
| +// see the trash cache in Reconcile for which is which. |
| +type trashState struct { |
| + declineMint bool |
| + removable bool |
| +} |
| + |
| +// productionClear reports whether a trashed app's routes may be retired — |
| +// i.e. whether we can positively see that it is NOT flagged production. |
| +// Dropping every route of a production app is an outage, which is precisely |
| +// what the flag exists to prevent, so an unreadable flag refuses in the same |
| +// direction as an ON one: the state an attacker with delete-but-not-sign can |
| +// manufacture is exactly the unreadable one. |
| +// |
| +// A nil key is the keyless box, which acts on the marker — domains §1c's |
| +// posture, and the comment on ReconcileConfig.ReleasePub says why. |
| +// |
| +// Both refusals log EVERY pass, deliberately: an operator who wrote that |
| +// marker on purpose has to be told why nothing happened, or they will assume |
| +// deletion is broken. |
| +func productionClear(ctx context.Context, s release.Store, pub ed25519.PublicKey, acct, app string) bool { |
| + if pub == nil { |
| + return true |
| + } |
| + flag, err := release.GetProduction(ctx, s, pub, acct, app) |
| + switch { |
| + case err != nil: |
| + log.Printf("provision: cannot read the production flag for %s/%s — refusing to act on the trash marker: %v", acct, app, err) |
| + return false |
| + case flag != nil && flag.Production: |
| + log.Printf("provision: trash marker on production app %s/%s — refusing to act on it; turn production off first", acct, app) |
| + return false |
| + } |
| + return true |
| +} |
| + |
| // ---- serving classes ------------------------------------------------------- |
| // |
| // Which boxes materialize a record. The multi-region ingress design's Part 3 |
| @@ -667,19 +716,32 @@ func Reconcile(ctx context.Context, s release.Store, reg *registry.Registry, cfg |
| } |
| // --- trash cache, per (acct, app), lazily --------------------------------- |
| - // An app in the trash provisions nothing. domains.Reconcile drops a |
| - // trashed app's routes every pass, so minting (or seed-converging) here |
| - // would re-raise them next tick and the two loops would fight forever. |
| - // The record stays in recs, so the removal phase below never retires the |
| - // route either — dropping a trashed app's routes is domains.Reconcile's |
| - // call alone, with its production-flag guard; this pass only declines to |
| - // create. An unreadable or malformed marker declines minting too: not |
| - // minting is the reversible direction, and a bucket writer who could |
| - // plant a marker to stall minting can already retire the instance |
| - // outright by deleting its record. |
| - trashCache := map[acctApp]bool{} |
| + // An app in the trash provisions nothing, and its existing routes are |
| + // RETIRED — both halves are this pass's job (#340). |
| + // |
| + // Retirement used to be domains.Reconcile's alone, on the grounds that it |
| + // held the production-flag guard. That deferral was a live bug: the |
| + // domains pass runs only on the box holding the housekeeper lease |
| + // (agent.go's domainsLoop wraps it in runHousekeeperDuties), and its |
| + // registry write is local, so on a multi-box deployment every OTHER box |
| + // kept a trashed app's route forever and went on serving it from its own |
| + // edge. Single-box deployments were correct by accident. This pass runs |
| + // on every box and already reads the marker, so the trigger belongs here |
| + // — and the guard comes with it (cfg.ReleasePub). Both passes now remove |
| + // and neither mints, so they agree rather than fight. |
| + // |
| + // One marker, two DIFFERENT answers, and conflating them is the trap: |
| + // |
| + // declineMint — true also when the marker is UNREADABLE. Not minting |
| + // is the reversible direction, and a bucket writer who could plant |
| + // garbage here to stall minting can already retire the instance |
| + // outright by deleting its record. |
| + // removable — true ONLY for a marker positively read and present, |
| + // whose app is not flagged production. Removal is not reversible, so |
| + // an unreadable marker (or an unreadable flag) must never reach it. |
| + trashCache := map[acctApp]trashState{} |
| trashSeen := map[acctApp]bool{} |
| - trashedFor := func(acct, app string) bool { |
| + trashFor := func(acct, app string) trashState { |
| k := acctApp{acct, app} |
| if trashSeen[k] { |
| return trashCache[k] |
| @@ -689,13 +751,14 @@ func Reconcile(ctx context.Context, s release.Store, reg *registry.Registry, cfg |
| switch { |
| case err != nil: |
| log.Printf("provision: trash marker for %s/%s unreadable: %v — not minting for this app", acct, app, err) |
| - trashCache[k] = true |
| + trashCache[k] = trashState{declineMint: true} |
| case tomb != nil: |
| log.Printf("provision: %s/%s is in the trash — not minting instance routes for it", acct, app) |
| - trashCache[k] = true |
| + trashCache[k] = trashState{declineMint: true, removable: productionClear(ctx, s, cfg.ReleasePub, acct, app)} |
| } |
| return trashCache[k] |
| } |
| + trashedFor := func(acct, app string) bool { return trashFor(acct, app).declineMint } |
| // --- mint / converge / conflict ------------------------------------------- |
| routes, err := reg.List() |
| @@ -835,13 +898,16 @@ func Reconcile(ctx context.Context, s release.Store, reg *registry.Registry, cfg |
| // Three conditions, each load-bearing: Provisioned (the flag — not the |
| // app, not the channel — grants removal authority; a hand-added route |
| // never has it), scope (this box's own region/pool jurisdiction — a |
| - // foreign box's provisioned route is not this pass's business), and |
| - // absence from the record set this pass just read (the bucket's own |
| - // decision to retire the instance, not a guess this pass made). |
| + // foreign box's provisioned route is not this pass's business), and a |
| + // retirement the BUCKET decided rather than one this pass guessed — |
| + // either the record is gone, or the app carries a trash marker (#340, |
| + // and see the trash cache above for why the marker's two answers are |
| + // not the same answer). |
| // |
| // Scope is asked through the SAME inScope as the record scan above, with |
| // the class read off the row (everywhereRoute) because the record whose |
| - // class it was is exactly what is gone. Both halves must widen together: |
| + // class it was may be exactly what is gone. Both halves must widen |
| + // together: |
| // a far box that mints a static route under the widened mint gate and then |
| // judges it out of scope here would serve a retired site forever. |
| // |
| @@ -858,22 +924,36 @@ func Reconcile(ctx context.Context, s release.Store, reg *registry.Registry, cfg |
| if !rt.Provisioned || !cfg.inScope(rt.Region, rt.Pool, everywhereRoute(rt)) { |
| continue |
| } |
| + // Two triggers, asked in this order because the first is the |
| + // cheap one: the bucket retired the instance (its record is |
| + // gone), or the bucket retired the whole APP (a trash marker, |
| + // while the record stays put). Both name the bucket's own |
| + // decision; neither is a guess this pass made. |
| + // |
| + // The trash lookup needs both halves of the app's identity off |
| + // the row. A Provisioned row always carries them — routeFor |
| + // copies them from the record — so a row missing either is a |
| + // shape this pass did not mint, and not one it will retire. |
| + why := "record gone" |
| if _, ok := recs[rt.Host]; ok { |
| - continue |
| + if rt.Account == "" || rt.App == "" || !trashFor(rt.Account, rt.App).removable { |
| + continue |
| + } |
| + why = "app in the trash" |
| } |
| if protect[rt.Host] { |
| log.Printf("provision: not removing %s: its record exists but could not be read cleanly this pass", rt.Host) |
| continue |
| } |
| if cfg.DryRun { |
| - log.Printf("provision: dry-run: would remove retired %s route %s (record gone)", rt.Kind, rt.Host) |
| + log.Printf("provision: dry-run: would remove retired %s route %s (%s)", rt.Kind, rt.Host, why) |
| continue |
| } |
| if err := reg.Delete(rt.Host); err != nil { |
| log.Printf("provision: remove retired %s route %s: %v", rt.Kind, rt.Host, err) |
| continue |
| } |
| - log.Printf("provision: removed retired %s route %s (record gone)", rt.Kind, rt.Host) |
| + log.Printf("provision: removed retired %s route %s (%s)", rt.Kind, rt.Host, why) |
| } |
| } |
| return nil |
diff --git a/internal/provision/reconcile_test.go b/internal/provision/reconcile_test.go| index dc73f2e16..b23f39392 100644 |
| --- a/internal/provision/reconcile_test.go |
| +++ b/internal/provision/reconcile_test.go |
| @@ -1,11 +1,17 @@ |
| package provision |
| import ( |
| + "bytes" |
| "context" |
| + "crypto/ed25519" |
| "encoding/json" |
| + "errors" |
| "fmt" |
| + "log" |
| + "os" |
| "path/filepath" |
| "reflect" |
| + "strings" |
| "testing" |
| "time" |
| @@ -85,6 +91,27 @@ func putReplace(t *testing.T, s release.Store, key string, body []byte) { |
| } |
| } |
| +// releaseKeys mints a throwaway release signing pair — the same helper |
| +// internal/domains/reconcile_test.go carries, for the same reason: the |
| +// production flag is signed, so a test that exercises the guard needs both |
| +// halves of a real key. |
| +func releaseKeys(t *testing.T) (ed25519.PrivateKey, ed25519.PublicKey) { |
| + t.Helper() |
| + seedB64, pubB64, err := release.GenerateKey() |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + priv, err := release.ParseSigningKey(seedB64, "test") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + pub, err := release.ParsePublicKey(pubB64, "test") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + return priv, pub |
| +} |
| + |
| func mustJSON(t *testing.T, v any) []byte { |
| t.Helper() |
| b, err := json.Marshal(v) |
| @@ -1266,12 +1293,20 @@ func TestReconcileDeclinesTrashedApp(t *testing.T) { |
| mustGet(t, reg, "acme.go.tito.io") |
| } |
| -// Trash: declining to MINT is the whole refusal — a route that already |
| -// exists when the app is trashed is not retired by this pass. Its record is |
| -// still in the bucket (present in recs), so the removal phase's "record |
| -// gone" trigger never fires; dropping a trashed app's routes is |
| -// domains.Reconcile's call alone, behind its production-flag guard. |
| -func TestReconcileLeavesTrashedAppsExistingRoute(t *testing.T) { |
| +// Trash: a route whose app is in the trash is RETIRED, on every box that |
| +// serves it (#340). This is provision's job and not domains.Reconcile's, |
| +// even though domains removes the same routes for the same reason: the |
| +// domains pass runs only on the box holding the housekeeper lease, so on a |
| +// multi-box deployment every other box kept a trashed app's route forever |
| +// and went on serving it — publicly, from its own edge, with the app |
| +// deleted. Single-box deployments were correct by accident. |
| +// |
| +// The record stays in the bucket while an app is trashed, so the removal |
| +// phase's "record gone" trigger never fires for it. The trash marker is the |
| +// second trigger, asked under exactly the same three gates: Provisioned, |
| +// in scope, and nothing failed to read this pass. |
| +func TestReconcileRemovesTrashedAppsRoute(t *testing.T) { |
| + _, pub := releaseKeys(t) |
| s, sqid := setup(t) |
| if err := CreateRecord(ctx, s, acmeRecord(sqid)); err != nil { |
| t.Fatal(err) |
| @@ -1285,13 +1320,219 @@ func TestReconcileLeavesTrashedAppsExistingRoute(t *testing.T) { |
| if err := release.TrashApp(ctx, s, sqid, "tito", "paul@keymail.dev", time.Now()); err != nil { |
| t.Fatal(err) |
| } |
| - cfg := ReconcileConfig{Region: "ie", Pools: []string{"tito"}, DataDir: "/data"} |
| + cfg := ReconcileConfig{Region: "ie", Pools: []string{"tito"}, DataDir: "/data", ReleasePub: pub} |
| + if err := Reconcile(ctx, s, reg, cfg); err != nil { |
| + t.Fatal(err) |
| + } |
| + if rt, err := reg.Get("acme.go.tito.io"); err != nil || rt != nil { |
| + t.Fatalf("a trashed app's route survived: %+v %v", rt, err) |
| + } |
| +} |
| + |
| +// #340's own shape: the FAR box, the everywhere class, a static site. This |
| +// is the case that was live on flagship — three edges publishing a trashed |
| +// app's host in their edges files while the lease holder had already |
| +// dropped it, so even the lease holder served it again through the |
| +// deployment-wide host map. |
| +func TestReconcileRemovesTrashedEverywhereRouteOnAFarBox(t *testing.T) { |
| + _, pub := releaseKeys(t) |
| + s, sqid := setup(t) |
| + rec := titoRecord(sqid, "site") |
| + rec.Kind = "static" |
| + if err := CreateRecord(ctx, s, rec); err != nil { |
| + t.Fatal(err) |
| + } |
| + reg := regWith(t) |
| + cfg := farBoxCfg() |
| + cfg.ReleasePub = pub |
| + if err := Reconcile(ctx, s, reg, cfg); err != nil { |
| + t.Fatal(err) |
| + } |
| + mustGet(t, reg, "site.go.tito.io") |
| + |
| + if err := release.TrashApp(ctx, s, sqid, "tito", "paul@keymail.dev", time.Now()); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := Reconcile(ctx, s, reg, cfg); err != nil { |
| + t.Fatal(err) |
| + } |
| + if rt, err := reg.Get("site.go.tito.io"); err != nil || rt != nil { |
| + t.Errorf("a far box kept serving a trashed app's static route: %+v %v", rt, err) |
| + } |
| +} |
| + |
| +// The two trash questions are NOT the same question. Declining to mint is |
| +// reversible and answers "true" to an unreadable marker on purpose; |
| +// removal is not reversible and must answer only to a marker positively |
| +// read and present. A pass that conflated them would let a corrupt object |
| +// — or a bucket writer with delete-but-not-sign — retire live routes. |
| +func TestReconcileKeepsTrashedAppsRouteWhenTheMarkerIsUnreadable(t *testing.T) { |
| + _, pub := releaseKeys(t) |
| + s, sqid := setup(t) |
| + if err := CreateRecord(ctx, s, acmeRecord(sqid)); err != nil { |
| + t.Fatal(err) |
| + } |
| + reg := regWith(t, registry.Route{ |
| + Host: "acme.go.tito.io", Slug: "acme", Kind: "instance", |
| + Socket: "/run/carlos/acme.go.tito.io.sock", DBPath: "/data/acme.go.tito.io.db", |
| + App: "tito", Account: sqid, Pool: "tito", Region: "ie", Channel: "stable", |
| + Hibernate: true, Provisioned: true, Seed: `["seed"]`, Status: "ready", |
| + }) |
| + if err := release.TrashApp(ctx, s, sqid, "tito", "paul@keymail.dev", time.Now()); err != nil { |
| + t.Fatal(err) |
| + } |
| + putReplace(t, s, release.AppTombstoneKey(sqid, "tito"), []byte("{not json")) |
| + cfg := ReconcileConfig{Region: "ie", Pools: []string{"tito"}, DataDir: "/data", ReleasePub: pub} |
| if err := Reconcile(ctx, s, reg, cfg); err != nil { |
| t.Fatal(err) |
| } |
| mustGet(t, reg, "acme.go.tito.io") |
| } |
| +// The production flag is why this removal lived in domains.Reconcile at |
| +// all: dropping every route of a production app is an outage, which is |
| +// exactly what the flag exists to prevent. Moving the trigger without the |
| +// guard would have been a regression, not a fix. Same refusal, same |
| +// wording, same pass-after-pass loudness as domains §1c. |
| +func TestReconcileNeverRemovesRoutesOfAProductionFlaggedTrashedApp(t *testing.T) { |
| + priv, pub := releaseKeys(t) |
| + s, sqid := setup(t) |
| + if err := CreateRecord(ctx, s, acmeRecord(sqid)); err != nil { |
| + t.Fatal(err) |
| + } |
| + reg := regWith(t, registry.Route{ |
| + Host: "acme.go.tito.io", Slug: "acme", Kind: "instance", |
| + Socket: "/run/carlos/acme.go.tito.io.sock", DBPath: "/data/acme.go.tito.io.db", |
| + App: "tito", Account: sqid, Pool: "tito", Region: "ie", Channel: "stable", |
| + Hibernate: true, Provisioned: true, Seed: `["seed"]`, Status: "ready", |
| + }) |
| + // The flag goes on FIRST: SetProduction refuses a trashed app. |
| + if err := release.SetProduction(ctx, s, priv, sqid, "tito", true, "paul@keymail.dev"); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := release.TrashApp(ctx, s, sqid, "tito", "eve@evil.example", time.Now()); err != nil { |
| + t.Fatal(err) |
| + } |
| + |
| + var logged bytes.Buffer |
| + log.SetOutput(&logged) |
| + t.Cleanup(func() { log.SetOutput(os.Stderr) }) |
| + |
| + cfg := ReconcileConfig{Region: "ie", Pools: []string{"tito"}, DataDir: "/data", ReleasePub: pub} |
| + for pass := 1; pass <= 2; pass++ { |
| + if err := Reconcile(ctx, s, reg, cfg); err != nil { |
| + t.Fatalf("pass %d: %v", pass, err) |
| + } |
| + mustGet(t, reg, "acme.go.tito.io") |
| + } |
| + if !strings.Contains(logged.String(), "trash marker on production app") { |
| + t.Errorf("no refusal logged; log was:\n%s", logged.String()) |
| + } |
| +} |
| + |
| +// flagUnreadableStore breaks exactly one read — the app's production flag — |
| +// the way an attacker with delete-but-not-sign, or a half-broken bucket, |
| +// would. Everything else reads normally, so the trash marker is perfectly |
| +// valid and only the flag is in doubt. |
| +type flagUnreadableStore struct { |
| + release.Store |
| + key string |
| +} |
| + |
| +func (s flagUnreadableStore) Get(ctx context.Context, key string) ([]byte, string, error) { |
| + if key == s.key { |
| + return nil, "", errors.New("bucket refused the read") |
| + } |
| + return s.Store.Get(ctx, key) |
| +} |
| + |
| +// An UNREADABLE flag refuses in the same direction as an ON one, here as |
| +// everywhere else in this platform: the state an attacker can manufacture |
| +// is exactly the unreadable one. |
| +func TestReconcileDoesNotRemoveTrashedRoutesWhenTheProductionFlagIsUnreadable(t *testing.T) { |
| + _, pub := releaseKeys(t) |
| + dir, sqid := setup(t) |
| + if err := CreateRecord(ctx, dir, acmeRecord(sqid)); err != nil { |
| + t.Fatal(err) |
| + } |
| + reg := regWith(t, registry.Route{ |
| + Host: "acme.go.tito.io", Slug: "acme", Kind: "instance", |
| + Socket: "/run/carlos/acme.go.tito.io.sock", DBPath: "/data/acme.go.tito.io.db", |
| + App: "tito", Account: sqid, Pool: "tito", Region: "ie", Channel: "stable", |
| + Hibernate: true, Provisioned: true, Seed: `["seed"]`, Status: "ready", |
| + }) |
| + if err := release.TrashApp(ctx, dir, sqid, "tito", "paul@keymail.dev", time.Now()); err != nil { |
| + t.Fatal(err) |
| + } |
| + s := flagUnreadableStore{Store: dir, key: release.ProductionKey(sqid, "tito")} |
| + |
| + var logged bytes.Buffer |
| + log.SetOutput(&logged) |
| + t.Cleanup(func() { log.SetOutput(os.Stderr) }) |
| + |
| + cfg := ReconcileConfig{Region: "ie", Pools: []string{"tito"}, DataDir: "/data", ReleasePub: pub} |
| + if err := Reconcile(ctx, s, reg, cfg); err != nil { |
| + t.Fatal(err) |
| + } |
| + mustGet(t, reg, "acme.go.tito.io") |
| + if !strings.Contains(logged.String(), "cannot read the production flag") { |
| + t.Errorf("no refusal logged; log was:\n%s", logged.String()) |
| + } |
| +} |
| + |
| +// A KEYLESS box acts on the marker, deliberately — domains §1c's posture, |
| +// carried over verbatim so the two passes cannot disagree about the same |
| +// app. It cannot verify a flag, and the alternative — hold every trash |
| +// forever wherever there is no pubkey — would break hiding entirely on |
| +// legitimately keyless deployments, whose owners have no way to make the |
| +// guard pass. |
| +func TestReconcileRemovesTrashedAppsRouteOnAKeylessBox(t *testing.T) { |
| + s, sqid := setup(t) |
| + if err := CreateRecord(ctx, s, acmeRecord(sqid)); err != nil { |
| + t.Fatal(err) |
| + } |
| + reg := regWith(t, registry.Route{ |
| + Host: "acme.go.tito.io", Slug: "acme", Kind: "instance", |
| + Socket: "/run/carlos/acme.go.tito.io.sock", DBPath: "/data/acme.go.tito.io.db", |
| + App: "tito", Account: sqid, Pool: "tito", Region: "ie", Channel: "stable", |
| + Hibernate: true, Provisioned: true, Seed: `["seed"]`, Status: "ready", |
| + }) |
| + if err := release.TrashApp(ctx, s, sqid, "tito", "paul@keymail.dev", time.Now()); err != nil { |
| + t.Fatal(err) |
| + } |
| + cfg := ReconcileConfig{Region: "ie", Pools: []string{"tito"}, DataDir: "/data"} // no ReleasePub |
| + if err := Reconcile(ctx, s, reg, cfg); err != nil { |
| + t.Fatal(err) |
| + } |
| + if rt, err := reg.Get("acme.go.tito.io"); err != nil || rt != nil { |
| + t.Fatalf("a keyless box kept a trashed app's route: %+v %v", rt, err) |
| + } |
| +} |
| + |
| +// Provisioned is the flag that grants removal authority, and the trash |
| +// trigger does not widen it: a hand-added route this pass never adopted is |
| +// not this pass's to retire, whatever the app's marker says. |
| +func TestReconcileNeverRemovesAHandAddedRouteOfATrashedApp(t *testing.T) { |
| + _, pub := releaseKeys(t) |
| + s, sqid := setup(t) |
| + if err := CreateRecord(ctx, s, acmeRecord(sqid)); err != nil { |
| + t.Fatal(err) |
| + } |
| + reg := regWith(t, registry.Route{ |
| + Host: "hand.go.tito.io", Slug: "hand", Kind: "instance", |
| + App: "tito", Account: sqid, Pool: "tito", Region: "ie", Channel: "stable", |
| + Status: "ready", // no Provisioned |
| + }) |
| + if err := release.TrashApp(ctx, s, sqid, "tito", "paul@keymail.dev", time.Now()); err != nil { |
| + t.Fatal(err) |
| + } |
| + cfg := ReconcileConfig{Region: "ie", Pools: []string{"tito"}, DataDir: "/data", ReleasePub: pub} |
| + if err := Reconcile(ctx, s, reg, cfg); err != nil { |
| + t.Fatal(err) |
| + } |
| + mustGet(t, reg, "hand.go.tito.io") |
| +} |
| + |
| // Trash: an unreadable or malformed marker also declines minting — not |
| // minting is the reversible direction, and a writer who could plant garbage |
| // here to stall minting can already retire the instance outright by |