amadan
Let a fleet-placed app's sidecars inherit its placement
4 files changed,
+196
−14
internal/console/apiinstances.go+11 −1internal/console/apisidecars.go+28 −12internal/console/apisidecars_test.go+140 −0internal/provision/sidecar.go+17 −1
diff --git a/internal/console/apiinstances.go b/internal/console/apiinstances.go| index ab7202f7e..a53753545 100644 |
| --- a/internal/console/apiinstances.go |
| +++ b/internal/console/apiinstances.go |
| @@ -102,6 +102,16 @@ func (s *Server) apiInstancePrincipal(w http.ResponseWriter, r *http.Request) (s |
| // belongs, and "this path is rate limited" is itself the answer that an app |
| // exists here. |
| func (s *Server) instanceGate(w http.ResponseWriter, r *http.Request) (string, *Account, string, bool) { |
| + return s.instanceGateFor(w, r, "instance") |
| +} |
| + |
| +// instanceGateFor is instanceGate with the noun its 429 uses. The BUDGET is |
| +// deliberately shared across every family that gates here — one credential |
| +// doing one kind of work, and one place for the ordering to live — but a |
| +// member creating sidecars should not be told they made too many "instance |
| +// requests" when they hit it. Only the sentence varies; nothing about the |
| +// order, the key or the limiter does. |
| +func (s *Server) instanceGateFor(w http.ResponseWriter, r *http.Request, noun string) (string, *Account, string, bool) { |
| principal, acct, app, ok := s.apiInstancePrincipal(w, r) |
| if !ok { |
| return "", nil, "", false |
| @@ -110,7 +120,7 @@ func (s *Server) instanceGate(w http.ResponseWriter, r *http.Request) (string, * |
| // principal here is frequently an app, which has no address of its own, |
| // and a signup burst is the workload rather than the abuse. |
| if !s.cli.instances.allow(hashToken(bearer(r)), time.Now()) { |
| - jsonError(w, http.StatusTooManyRequests, "too many instance requests — wait a minute and try again") |
| + jsonError(w, http.StatusTooManyRequests, "too many "+noun+" requests — wait a minute and try again") |
| return "", nil, "", false |
| } |
| // A console with no deployment bucket configured can answer none of |
diff --git a/internal/console/apisidecars.go b/internal/console/apisidecars.go| index b8637359d..5556cc67e 100644 |
| --- a/internal/console/apisidecars.go |
| +++ b/internal/console/apisidecars.go |
| @@ -135,7 +135,7 @@ type sidecarListRecord struct { |
| // name-sorted, capped and honest about the cap, each carrying its |
| // best-effort running version. |
| func (s *Server) handleAPISidecarList(w http.ResponseWriter, r *http.Request) { |
| - _, acct, app, ok := s.instanceGate(w, r) |
| + _, acct, app, ok := s.instanceGateFor(w, r, "sidecar") |
| if !ok { |
| return |
| } |
| @@ -159,7 +159,7 @@ func (s *Server) handleAPISidecarList(w http.ResponseWriter, r *http.Request) { |
| // byte-identical 404 — a sidecar's existence is not a thing to probe for |
| // either, and this way "no such sidecar" and "no such app" read alike. |
| func (s *Server) handleAPISidecarGet(w http.ResponseWriter, r *http.Request) { |
| - _, acct, app, ok := s.instanceGate(w, r) |
| + _, acct, app, ok := s.instanceGateFor(w, r, "sidecar") |
| if !ok { |
| return |
| } |
| @@ -205,7 +205,7 @@ func (s *Server) handleAPISidecarGet(w http.ResponseWriter, r *http.Request) { |
| // stating "any" on a record under a no-inbound manifest would place a bot on |
| // boxes the operator meant to keep it off. |
| func (s *Server) handleAPISidecarCreate(w http.ResponseWriter, r *http.Request) { |
| - principal, acct, app, ok := s.instanceGate(w, r) |
| + principal, acct, app, ok := s.instanceGateFor(w, r, "sidecar") |
| if !ok { |
| return |
| } |
| @@ -233,14 +233,30 @@ func (s *Server) handleAPISidecarCreate(w http.ResponseWriter, r *http.Request) |
| "invalid name — a sidecar name is a single DNS label (lowercase a-z, 0-9, hyphen, 1-63 chars)") |
| return |
| } |
| + // A fleet-placed app governs every one of its records' Region uniformly — |
| + // handleAPIInstanceCreate's rule, mirrored here because Eleven, the |
| + // customer this whole family exists for, places its app onto a fleet. When |
| + // a placement exists it OVERRIDES whatever the request body sent, and the |
| + // ordinary region/allow-list validation below never runs at all: a |
| + // "fleet/<name>" value would fail provision.ValidRegion's shape anyway, |
| + // which is exactly why this has to be a check made BEFORE that validation |
| + // rather than alongside it. A placed app's region is not user input. |
| + reg, err := openRegistry(s.cfg.Registry) |
| + if err != nil { |
| + log.Printf("console: api sidecars create %s/%s: open registry: %v", acct.Sqid, app, err) |
| + jsonError(w, http.StatusBadGateway, "could not check this app's placement") |
| + return |
| + } |
| + placed, err := s.appPlacement(reg, acct, app) |
| + if err != nil { |
| + log.Printf("console: api sidecars create %s/%s: check placement: %v", acct.Sqid, app, err) |
| + jsonError(w, http.StatusBadGateway, "could not check this app's placement") |
| + return |
| + } |
| region := strings.TrimSpace(req.Region) |
| - // No fleet-placement override here, unlike handleAPIInstanceCreate. |
| - // appPlacement answers "fleet/<name>", which provision.ValidateSidecarShape |
| - // refuses as a region (ValidRegion has no "/" in its charset) — so |
| - // inheriting it would write a record that fails its own shape check at |
| - // the next door. Where a sidecar runs is the sidecar family's own scope |
| - // axes: the manifest's regions/pool, and placement. |
| - if !provision.ValidRegion(region) || !instanceRegionAllowed(m.Regions, region) { |
| + if placed != "" { |
| + region = placed |
| + } else if !provision.ValidRegion(region) || !instanceRegionAllowed(m.Regions, region) { |
| msg := "invalid region" |
| if len(m.Regions) > 0 { |
| msg = "region " + region + " is not one this app's sidecars run in — allowed: " + strings.Join(m.Regions, ", ") |
| @@ -308,7 +324,7 @@ func (s *Server) handleAPISidecarCreate(w http.ResponseWriter, r *http.Request) |
| // bot is not destroying that member's data. Idempotent: the caller's goal is |
| // "not there", and a second delete has reached it. |
| func (s *Server) handleAPISidecarDelete(w http.ResponseWriter, r *http.Request) { |
| - principal, acct, app, ok := s.instanceGate(w, r) |
| + principal, acct, app, ok := s.instanceGateFor(w, r, "sidecar") |
| if !ok { |
| return |
| } |
| @@ -369,7 +385,7 @@ func (s *Server) handleAPISidecarDelete(w http.ResponseWriter, r *http.Request) |
| // what stands between an app token and an unbounded write: without it, any |
| // name at all would mint a wake object for a sidecar that does not exist. |
| func (s *Server) handleAPISidecarWake(w http.ResponseWriter, r *http.Request) { |
| - principal, acct, app, ok := s.instanceGate(w, r) |
| + principal, acct, app, ok := s.instanceGateFor(w, r, "sidecar") |
| if !ok { |
| return |
| } |
diff --git a/internal/console/apisidecars_test.go b/internal/console/apisidecars_test.go| index 476ebc7ba..fd3d8e8f8 100644 |
| --- a/internal/console/apisidecars_test.go |
| +++ b/internal/console/apisidecars_test.go |
| @@ -19,11 +19,13 @@ import ( |
| "errors" |
| "fmt" |
| "net/http" |
| + "path/filepath" |
| "strings" |
| "testing" |
| "time" |
| "github.com/carlosframework/platform/internal/provision" |
| + "github.com/carlosframework/platform/internal/registry" |
| "github.com/carlosframework/platform/internal/release" |
| "github.com/carlosframework/platform/internal/status" |
| ) |
| @@ -854,3 +856,141 @@ func TestAPISidecarRateLimit(t *testing.T) { |
| t.Fatalf("enable after spending the record budget = %d %s, want 200", status, out) |
| } |
| } |
| + |
| +// TestAPISidecarCreateInheritsFleetPlacement: a fleet-placed app's sidecars |
| +// inherit the app's placement, exactly as its instances do — the request |
| +// body's region is ignored entirely, and even a region the manifest's |
| +// allow-list would otherwise refuse succeeds, because that validation is |
| +// never consulted once the app is placed. This is not a nicety: Eleven, the |
| +// customer this whole family exists for, places its app onto a fleet, and |
| +// "fleet/<name>" can never pass provision.ValidRegion — so without the |
| +// override there is no such thing as a usable Eleven sidecar record. |
| +// |
| +// A route is seeded first for appPlacement's route-derived fallback path, |
| +// the realistic shape of an app already running before it was placed (see |
| +// TestAPIInstanceCreateInheritsFleetPlacement's own doc comment). |
| +func TestAPISidecarCreateInheritsFleetPlacement(t *testing.T) { |
| + fx, token := instanceFixture(t, "code-side-placed") |
| + enableSidecars(t, fx, "titogo", titoSidecarManifest()) // Regions: []string{"ie"} |
| + |
| + reg, err := registry.Open(fx.regPath) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := reg.Upsert(registry.Route{Host: "www.go.tito.io", Kind: "instance", App: "titogo", Account: "bab"}); err != nil { |
| + t.Fatal(err) |
| + } |
| + if status, body := postJSON(t, fx.ts.URL+"/api/cli/fleets/bab/new", |
| + map[string]string{"name": "eleven", "bucket": "eleven-fleet-data"}, token); status != http.StatusOK { |
| + t.Fatalf("create fleet = %d %v, want 200", status, body) |
| + } |
| + if status, body := postJSON(t, fx.ts.URL+"/api/cli/apps/bab/titogo/place", |
| + map[string]string{"target": "fleet/eleven"}, token); status != http.StatusOK { |
| + t.Fatalf("place = %d %v, want 200", status, body) |
| + } |
| + |
| + // "us" is outside titoSidecarManifest's ["ie"] allow-list — refused on an |
| + // unplaced app (TestAPISidecarCreateUnplacedHonorsBodyRegion below), but |
| + // accepted and overridden here, which is the whole point of the ordering. |
| + status, _, out := rawReq(t, http.MethodPost, sidecarsURL(fx, "bab", "titogo"), token, |
| + `{"name":"bot","region":"us"}`) |
| + if status != http.StatusCreated { |
| + t.Fatalf("create on a placed app = %d %s, want 201", status, out) |
| + } |
| + var rec provision.SidecarRecord |
| + if err := json.Unmarshal([]byte(out), &rec); err != nil { |
| + t.Fatalf("create body %s: %v", out, err) |
| + } |
| + if rec.Region != "fleet/eleven" { |
| + t.Fatalf("record Region = %q, want the inherited fleet/eleven (body region ignored)", rec.Region) |
| + } |
| + // The stored record agrees, and — the half that would otherwise make this |
| + // inert — it passes the shape rail the reconcile pass re-checks it at, so |
| + // a fleet-placed sidecar is mintable rather than permanently protected. |
| + stored, err := provision.GetSidecarRecord(context.Background(), fx.rel, "bab", "titogo", "bot") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if stored.Region != "fleet/eleven" { |
| + t.Fatalf("stored record Region = %q, want fleet/eleven", stored.Region) |
| + } |
| + if err := provision.ValidateSidecarShape(*stored); err != nil { |
| + t.Fatalf("the record this door wrote fails its own shape rail: %v", err) |
| + } |
| +} |
| + |
| +// TestAPISidecarCreateUnplacedHonorsBodyRegion is the regression guard for |
| +// the inheritance above: with no placement, the ordinary contract is |
| +// unchanged — the body's region is shape-checked and matched against the |
| +// manifest's allow-list exactly as before. |
| +func TestAPISidecarCreateUnplacedHonorsBodyRegion(t *testing.T) { |
| + fx, token := instanceFixture(t, "code-side-unplaced") |
| + enableSidecars(t, fx, "titogo", titoSidecarManifest()) |
| + u := sidecarsURL(fx, "bab", "titogo") |
| + |
| + status, _, out := rawReq(t, http.MethodPost, u, token, `{"name":"bot","region":"ie"}`) |
| + if status != http.StatusCreated { |
| + t.Fatalf("create = %d %s, want 201", status, out) |
| + } |
| + var rec provision.SidecarRecord |
| + if err := json.Unmarshal([]byte(out), &rec); err != nil { |
| + t.Fatal(err) |
| + } |
| + if rec.Region != "ie" { |
| + t.Fatalf("Region = %q, want the body's ie (no placement in play)", rec.Region) |
| + } |
| + if status, _, out := rawReq(t, http.MethodPost, u, token, |
| + `{"name":"beta","region":"us"}`); status != http.StatusBadRequest { |
| + t.Fatalf("create outside the allow-list on an unplaced app = %d %s, want 400", status, out) |
| + } |
| + // And a fleet target is NOT a way to smuggle one past the allow-list: an |
| + // unplaced app's region is user input, and user input takes ValidRegion. |
| + if status, _, out := rawReq(t, http.MethodPost, u, token, |
| + `{"name":"beta","region":"fleet/eleven"}`); status != http.StatusBadRequest { |
| + t.Fatalf("create naming a fleet region on an UNPLACED app = %d %s, want 400", status, out) |
| + } |
| +} |
| + |
| +// TestAPISidecarCreatePlacementLookupFailure: a registry the console cannot |
| +// open is the instance door's error shape — 502 "could not check this app's |
| +// placement" — and never a record written against an unknown placement. |
| +func TestAPISidecarCreatePlacementLookupFailure(t *testing.T) { |
| + fx, token := instanceFixture(t, "code-side-placefail") |
| + enableSidecars(t, fx, "titogo", titoSidecarManifest()) |
| + fx.srv.cfg.Registry = filepath.Join(t.TempDir(), "no-such-dir", "registry.db") |
| + |
| + status, _, out := rawReq(t, http.MethodPost, sidecarsURL(fx, "bab", "titogo"), token, |
| + `{"name":"bot","region":"ie"}`) |
| + if status != http.StatusBadGateway { |
| + t.Fatalf("create with an unopenable registry = %d %s, want 502", status, out) |
| + } |
| + if !strings.Contains(out, "placement") { |
| + t.Fatalf("refusal = %s, want it to name the check that failed", out) |
| + } |
| + if _, _, err := fx.rel.Get(context.Background(), |
| + provision.SidecarRecordKey("bab", "titogo", "bot")); err == nil { |
| + t.Fatalf("a record was written despite an unknown placement") |
| + } |
| +} |
| + |
| +// TestAPISidecarRateLimitNamesTheVerbFamily: the budget is shared with the |
| +// instance verbs deliberately (one credential, one kind of work), but a |
| +// member who spends it creating sidecars is told about sidecars. |
| +func TestAPISidecarRateLimitNamesTheVerbFamily(t *testing.T) { |
| + fx, token := instanceFixture(t, "code-side-429text") |
| + enableSidecars(t, fx, "titogo", titoSidecarManifest()) |
| + u := sidecarsURL(fx, "bab", "titogo") |
| + for i := 0; i < cliInstanceBurst; i++ { |
| + if status, _, out := rawReq(t, http.MethodPost, u, token, |
| + fmt.Sprintf(`{"name":"bot%d","region":"ie"}`, i)); status != http.StatusCreated { |
| + t.Fatalf("create %d = %d %s", i, status, out) |
| + } |
| + } |
| + status, _, out := rawReq(t, http.MethodPost, u, token, `{"name":"over","region":"ie"}`) |
| + if status != http.StatusTooManyRequests { |
| + t.Fatalf("create past the budget = %d %s, want 429", status, out) |
| + } |
| + if !strings.Contains(out, "sidecar requests") { |
| + t.Fatalf("429 body = %s, want it to name the verb family the caller was using", out) |
| + } |
| +} |
diff --git a/internal/provision/sidecar.go b/internal/provision/sidecar.go| index 96bff9b64..9f54847f8 100644 |
| --- a/internal/provision/sidecar.go |
| +++ b/internal/provision/sidecar.go |
| @@ -19,6 +19,7 @@ import ( |
| "strings" |
| "time" |
| + "github.com/carlosframework/platform/internal/registry" |
| "github.com/carlosframework/platform/internal/release" |
| ) |
| @@ -219,8 +220,23 @@ func ValidateSidecarShape(rec SidecarRecord) error { |
| default: |
| return fmt.Errorf("placement %q: want %q or %q (empty means %q)", rec.Placement, PlacementAny, PlacementNoInbound, PlacementAny) |
| } |
| + // A region is either a region CODE or a fleet placement target — the two |
| + // grammars Route.Region carries, exactly as an instance record's does. |
| + // registry.FleetPlacement is the one parser for the second, never a |
| + // prefix test written out again here. |
| + // |
| + // This rail is not where user input is policed: the console's create door |
| + // validates a REQUESTED region against ValidRegion and the manifest's |
| + // allow-list, and a fleet-placed app never reaches that path at all |
| + // because its region is the app's placement rather than anything the |
| + // caller sent (apisidecars.go). What this rail is for is the other |
| + // direction — the reconcile pass PROTECTS a record whose shape it cannot |
| + // trust, so refusing the placement grammar here would make every |
| + // fleet-placed sidecar permanently unmintable with no error anywhere. |
| if rec.Region != "" && !ValidRegion(rec.Region) { |
| - return fmt.Errorf("region %q: want [a-z0-9-]{0,16}", rec.Region) |
| + if _, ok := registry.FleetPlacement(rec.Region); !ok { |
| + return fmt.Errorf("region %q: want [a-z0-9-]{0,16} or a fleet placement target (fleet/<name>)", rec.Region) |
| + } |
| } |
| if rec.Pool != "" && !ValidPool(rec.Pool) { |
| return fmt.Errorf("pool %q: want [a-z0-9-]{0,16}", rec.Pool) |