amadan
console: insert-only fleet create, honest errors, placement inheritance at instance-create
6 files changed,
+374
−48
internal/console/apiinstances.go+23 −1internal/console/apiinstances_test.go+91 −0internal/console/fleets.go+139 −40internal/console/fleets_test.go+4 −4internal/registry/fleets.go+45 −2internal/registry/fleets_test.go+72 −1
diff --git a/internal/console/apiinstances.go b/internal/console/apiinstances.go| index 34ed311ca..222b73073 100644 |
| --- a/internal/console/apiinstances.go |
| +++ b/internal/console/apiinstances.go |
| @@ -240,8 +240,30 @@ func (s *Server) handleAPIInstanceCreate(w http.ResponseWriter, r *http.Request) |
| "invalid host — an instance host is one DNS label followed by ."+m.Domain) |
| return |
| } |
| + // A fleet-placed app (customer-fleets plan, `carlos apps place`) governs |
| + // every one of its instances' Region uniformly — spec's "one flag on the |
| + // app, all its instances follow" (appPlacement, fleets.go) — so 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 |
| + // (no "/"), which is exactly why this has to be a check made BEFORE |
| + // that validation, not alongside it. |
| + reg, err := openRegistry(s.cfg.Registry) |
| + if err != nil { |
| + log.Printf("console: api instances create %s/%s: open registry: %v", acct.Sqid, app, err) |
| + jsonError(w, http.StatusBadGateway, "could not check this app's placement") |
| + return |
| + } |
| + placed, err := appPlacement(reg, acct.Sqid, app) |
| + if err != nil { |
| + log.Printf("console: api instances 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) |
| - 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 runs in — allowed: " + strings.Join(m.Regions, ", ") |
diff --git a/internal/console/apiinstances_test.go b/internal/console/apiinstances_test.go| index a7905133d..49dfe5963 100644 |
| --- a/internal/console/apiinstances_test.go |
| +++ b/internal/console/apiinstances_test.go |
| @@ -23,6 +23,7 @@ import ( |
| "time" |
| "github.com/carlosframework/platform/internal/provision" |
| + "github.com/carlosframework/platform/internal/registry" |
| "github.com/carlosframework/platform/internal/release" |
| ) |
| @@ -352,6 +353,96 @@ func TestAPIInstanceCreateRefusalOrder(t *testing.T) { |
| } |
| } |
| +// TestAPIInstanceCreateInheritsFleetPlacement: a fleet-placed app's |
| +// instances inherit the app's placement (customer-fleets plan, spec's "one |
| +// flag on the app, all its instances follow") — the request body's region |
| +// is ignored entirely, and even a region the manifest's allow-list would |
| +// otherwise refuse succeeds, because it is never consulted once the app is |
| +// placed. |
| +// |
| +// A route must already exist for "titogo" before place is called: |
| +// appPlacement (fleets.go) derives an app's placement from its ALREADY |
| +// STAMPED routes, so an app placed before it has any route/instance at all |
| +// has nothing yet to derive from — a known limitation of the routes-derived |
| +// approach, not exercised by this test (see fleets.go's appPlacement doc |
| +// comment). Seeding one here is the realistic shape: an app already |
| +// running (e.g. via `carlos add`) before being placed onto a fleet. |
| +func TestAPIInstanceCreateInheritsFleetPlacement(t *testing.T) { |
| + fx, token := instanceFixture(t, "code-inst-placed") |
| + enableInstances(t, fx, "titogo", titoManifest()) // 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": "pi", "bucket": "pi-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/pi"}, token); status != http.StatusOK { |
| + t.Fatalf("place = %d %v, want 200", status, body) |
| + } |
| + |
| + u := instancesURL(fx, "bab", "titogo") |
| + // "us" is outside titoManifest's ["ie"] allow-list — refused on an |
| + // unplaced app (TestAPIInstanceCreateUnplacedHonorsBodyRegion below), |
| + // but accepted (and overridden) here. |
| + status, _, out := rawReq(t, http.MethodPost, u, token, `{"host":"acme.go.tito.io","region":"us"}`) |
| + if status != http.StatusCreated { |
| + t.Fatalf("create on a placed app = %d %s, want 201", status, out) |
| + } |
| + var rec provision.Record |
| + if err := json.Unmarshal([]byte(out), &rec); err != nil { |
| + t.Fatalf("create body %s: %v", out, err) |
| + } |
| + if rec.Region != "fleet/pi" { |
| + t.Fatalf("record Region = %q, want the inherited fleet/pi (body region ignored)", rec.Region) |
| + } |
| + |
| + // The stored record agrees. |
| + stored, err := provision.GetRecord(context.Background(), fx.rel, "bab", "titogo", "acme.go.tito.io") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if stored.Region != "fleet/pi" { |
| + t.Fatalf("stored record Region = %q, want fleet/pi", stored.Region) |
| + } |
| +} |
| + |
| +// TestAPIInstanceCreateUnplacedHonorsBodyRegion is the regression guard for |
| +// the inheritance check above: without a placement, the existing contract |
| +// is unchanged — the body's region is validated against the manifest's |
| +// allow-list and stored exactly as before. |
| +func TestAPIInstanceCreateUnplacedHonorsBodyRegion(t *testing.T) { |
| + fx, token := instanceFixture(t, "code-inst-unplaced") |
| + enableInstances(t, fx, "titogo", titoManifest()) |
| + u := instancesURL(fx, "bab", "titogo") |
| + |
| + status, _, out := rawReq(t, http.MethodPost, u, token, `{"host":"acme.go.tito.io","region":"ie"}`) |
| + if status != http.StatusCreated { |
| + t.Fatalf("create = %d %s, want 201", status, out) |
| + } |
| + var rec provision.Record |
| + 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) |
| + } |
| + |
| + // A region outside the manifest's allow-list is still refused, exactly |
| + // as it was before placement inheritance existed. |
| + status, _, out = rawReq(t, http.MethodPost, u, token, `{"host":"beta.go.tito.io","region":"us"}`) |
| + if status != http.StatusBadRequest { |
| + t.Fatalf("create outside the allow-list on an unplaced app = %d %s, want 400", status, out) |
| + } |
| +} |
| + |
| // TestAPIInstanceCreateEmptyRegionAllowList: an empty allow-list means any |
| // shape-valid region, per §3.1 — including the unconstrained empty one. |
| func TestAPIInstanceCreateEmptyRegionAllowList(t *testing.T) { |
diff --git a/internal/console/fleets.go b/internal/console/fleets.go| index 2fa24e4d8..63780423e 100644 |
| --- a/internal/console/fleets.go |
| +++ b/internal/console/fleets.go |
| @@ -9,14 +9,26 @@ |
| // (fleets.name is the registry's primary key, not scoped per account), so a |
| // caller's own valid {account} segment could otherwise be used to probe |
| // which fleet names exist elsewhere. |
| +// |
| +// ONE HONEST EXCEPTION to that closed-probing posture: POST .../new's 409 |
| +// ("that name is taken") is unavoidably a cross-account existence oracle. |
| +// Fleet names are that same flat, global namespace, and CreateFleet's |
| +// insert-only uniqueness check (registry/fleets.go) has to answer "already |
| +// exists" the same way whether the existing owner is you or a stranger — |
| +// there is no way to make "is this name free" account-scoped when the |
| +// thing being asked about is a single shared channel-identity namespace. No |
| +// other door in this file leaks anything through it: the 409 says a name is |
| +// taken, never by whom. |
| package console |
| import ( |
| "context" |
| "crypto/rand" |
| "encoding/hex" |
| + "errors" |
| "log" |
| "net/http" |
| + "regexp" |
| "strings" |
| "time" |
| @@ -108,25 +120,42 @@ func mintFleetToken() (plaintext, hash string, err error) { |
| return plaintext, hashToken(plaintext), nil |
| } |
| -// handleFleetNew answers `carlos fleets create`: mints the account-owned |
| -// fleet record. The data bucket is taken on trust — it must already exist |
| -// (customer tofu, spec §"Surfaces") — this door only records its name. |
| +// bucketNameRe is the S3 bucket-name shape: 3-63 characters, lowercase |
| +// letters/digits/dot/hyphen, starting and ending with a letter or digit. |
| +// (The real S3 rules also forbid a few extra things — adjacent dots, |
| +// IP-address-shaped names — that this simplified check does not chase; |
| +// those still fail cleanly when the customer's tofu tries to use the |
| +// bucket, which is a world away from THIS value's actual exposure: it ends |
| +// up verbatim in a fleet box's env file, on hardware this console does not |
| +// control, so a value that is not even bucket-name-shaped must never reach |
| +// that far only to fail somewhere with no error message at all.) |
| +var bucketNameRe = regexp.MustCompile(`^[a-z0-9][a-z0-9.-]{1,61}[a-z0-9]$`) |
| + |
| +// handleAPIFleetNew answers `carlos fleets create`: mints the account-owned |
| +// fleet record. The data bucket is taken on trust that it already exists |
| +// (customer tofu, spec §"Surfaces") — this door only checks its NAME is |
| +// bucket-shaped and records it. |
| // |
| -// The existence check and the write below are two calls, not one atomic |
| -// insert: UpsertFleet (Task 1) is a plain upsert keyed on name, with no |
| -// insert-only mode to lose a race against — so two concurrent creates of the |
| -// same brand-new name from two different accounts could, in the narrow |
| -// window between them, have the second one silently take over the first's |
| -// row. Accepted for v1 under the same operator-trust posture the whole |
| -// feature ships under (design doc, "Trust boundaries"): fleet creation is a |
| -// deliberate, rare, human-typed act, not a surface under adversarial load. |
| +// CreateFleet (registry/fleets.go) is insert-only — ON CONFLICT DO NOTHING, |
| +// RowsAffected is the atomic "did I win" answer — so there is no |
| +// read-then-write window here for a second account to silently take over a |
| +// name a first caller just created (an earlier revision of this handler did |
| +// a GetFleet pre-check before UpsertFleet, which had exactly that race; |
| +// CreateFleet replaces both calls with the one atomic one). |
| func (s *Server) handleAPIFleetNew(w http.ResponseWriter, r *http.Request) { |
| addr, acct, ok := s.apiAccount(w, r) |
| if !ok { |
| return |
| } |
| - if !s.cli.write.allow(addr, time.Now()) { |
| - jsonError(w, http.StatusTooManyRequests, "too many requests — wait a minute and try again") |
| + // The fleets namespace is flat and global with no delete door (see this |
| + // file's package doc comment) — the same "a claimed name is a permanent |
| + // row, not a pointer the next call overwrites" shape apps' create budget |
| + // exists for, so this spends the SAME scarce per-day budget |
| + // (handleAPICreateApp), not the ordinary per-minute write budget every |
| + // other fleet door here uses. |
| + if !s.cli.create.allow(addr, time.Now()) { |
| + jsonError(w, http.StatusTooManyRequests, |
| + "fleet creation limit reached for today — try again tomorrow") |
| return |
| } |
| var req struct { |
| @@ -143,8 +172,10 @@ func (s *Server) handleAPIFleetNew(w http.ResponseWriter, r *http.Request) { |
| return |
| } |
| bucket := strings.TrimSpace(req.Bucket) |
| - if bucket == "" { |
| - jsonError(w, http.StatusBadRequest, "bucket is required — the data bucket must already exist") |
| + if !bucketNameRe.MatchString(bucket) { |
| + jsonError(w, http.StatusBadRequest, |
| + "invalid bucket name — 3-63 characters of lowercase letters, digits, dot and hyphen, "+ |
| + "starting and ending with a letter or digit (S3 bucket-name shape)") |
| return |
| } |
| reg, err := openRegistry(s.cfg.Registry) |
| @@ -153,27 +184,23 @@ func (s *Server) handleAPIFleetNew(w http.ResponseWriter, r *http.Request) { |
| jsonError(w, http.StatusInternalServerError, "could not reach the fleet registry") |
| return |
| } |
| - if existing, err := reg.GetFleet(name); err != nil { |
| + created, err := reg.CreateFleet(registry.Fleet{Name: name, Account: acct.Sqid, Bucket: bucket}) |
| + if err != nil { |
| log.Printf("console: fleet new %s/%s: %v", acct.Sqid, name, err) |
| - jsonError(w, http.StatusInternalServerError, "could not check the fleet name") |
| - return |
| - } else if existing != nil { |
| - jsonError(w, http.StatusConflict, "that name is taken") |
| + jsonError(w, http.StatusInternalServerError, "could not create the fleet — try again") |
| return |
| } |
| - f := registry.Fleet{Name: name, Account: acct.Sqid, Bucket: bucket, CreatedAt: time.Now().Unix()} |
| - if err := reg.UpsertFleet(f); err != nil { |
| - log.Printf("console: fleet new %s/%s: %v", acct.Sqid, name, err) |
| - jsonError(w, http.StatusInternalServerError, "could not create the fleet — try again") |
| + if !created { |
| + jsonError(w, http.StatusConflict, "that name is taken") |
| return |
| } |
| s.audit(acct, name, addr, "fleet-create", "created fleet "+name+" (cli)") |
| writeJSON(w, http.StatusOK, map[string]any{ |
| - "fleet": fleetView{Name: f.Name, Bucket: f.Bucket, Boxes: []fleetBoxView{}}, |
| + "fleet": fleetView{Name: name, Bucket: bucket, Boxes: []fleetBoxView{}}, |
| }) |
| } |
| -// handleFleetList answers `carlos fleets ls`: every fleet the caller's |
| +// handleAPIFleetList answers `carlos fleets ls`: every fleet the caller's |
| // account owns, each with its boxes' label and status — never a token hash. |
| func (s *Server) handleAPIFleetList(w http.ResponseWriter, r *http.Request) { |
| _, acct, ok := s.apiAccount(w, r) |
| @@ -212,7 +239,7 @@ func (s *Server) handleAPIFleetList(w http.ResponseWriter, r *http.Request) { |
| writeJSON(w, http.StatusOK, map[string]any{"fleets": out}) |
| } |
| -// handleFleetAddBox answers `carlos fleets add-box`: mints a box row and its |
| +// handleAPIFleetAddBox answers `carlos fleets add-box`: mints a box row and its |
| // bearer token. The plaintext appears exactly once, in this response body — |
| // only its sha256 hex reaches the registry (AddFleetBox), and it is never |
| // logged. |
| @@ -243,18 +270,25 @@ func (s *Server) handleAPIFleetAddBox(w http.ResponseWriter, r *http.Request) { |
| jsonError(w, http.StatusInternalServerError, "could not mint a token — try again") |
| return |
| } |
| - // The fleet is already proven to exist (apiFleet), so the only reachable |
| - // failure from here is AddFleetBox's own uniqueness guard: an ACTIVE box |
| - // already registered under this label. |
| + // The fleet is already proven to exist (apiFleet), so AddFleetBox's own |
| + // "no fleet" case is unreachable here — but a tx/busy/disk failure is |
| + // not, and must not be rendered as the ordinary duplicate-box 409: |
| + // errors.Is against the sentinel is what tells the two apart, rather |
| + // than assuming every failure here is the uniqueness guard. |
| if err := reg.AddFleetBox(registry.FleetBox{Fleet: fleet.Name, Label: label, TokenHash: hash, Status: "active"}); err != nil { |
| - jsonError(w, http.StatusConflict, "fleet "+fleet.Name+" already has an active box "+label) |
| + if errors.Is(err, registry.ErrFleetBoxActive) { |
| + jsonError(w, http.StatusConflict, "fleet "+fleet.Name+" already has an active box "+label) |
| + return |
| + } |
| + log.Printf("console: fleet add-box %s/%s/%s: %v", acct.Sqid, fleet.Name, label, err) |
| + jsonError(w, http.StatusInternalServerError, "could not add the box — try again") |
| return |
| } |
| s.audit(acct, fleet.Name, addr, "fleet-add-box", "added box "+label+" to fleet "+fleet.Name+" (cli)") |
| writeJSON(w, http.StatusOK, map[string]any{"label": label, "token": plaintext}) |
| } |
| -// handleFleetRotateBox answers `carlos fleets rotate-token`: a fresh token |
| +// handleAPIFleetRotateBox answers `carlos fleets rotate-token`: a fresh token |
| // for a box that already exists, its hash replacing the stored one. |
| // |
| // ROTATION IS NOT REVOCATION. It refuses the box's NEXT dial-in attempt with |
| @@ -309,7 +343,7 @@ func (s *Server) handleAPIFleetRotateBox(w http.ResponseWriter, r *http.Request) |
| writeJSON(w, http.StatusOK, map[string]any{"token": plaintext}) |
| } |
| -// handleFleetDetachBox answers `carlos fleets detach-box`: flips a box to |
| +// handleAPIFleetDetachBox answers `carlos fleets detach-box`: flips a box to |
| // "detached", which the edge treats as an outright refusal (a dial-in is |
| // refused, and — unlike rotation — an already-connected channel is closed). |
| // This is the hard revocation rotation is not. |
| @@ -346,6 +380,50 @@ func (s *Server) handleAPIFleetDetachBox(w http.ResponseWriter, r *http.Request) |
| writeJSON(w, http.StatusOK, map[string]any{}) |
| } |
| +// appPlacement reports the fleet placement target already stamped on an |
| +// app's routes ("fleet/<name>"), or "" for no placement. handleAPIPlace |
| +// stamps every one of an app's routes with the SAME target in one call, so |
| +// by construction they are uniform once placed; the first fleet-shaped |
| +// Region found among the app's routes is authoritative and this does not |
| +// (and need not) reconcile a route set that predates that invariant — no |
| +// such state is reachable through this API. |
| +// |
| +// The one caller outside this file is handleAPIInstanceCreate |
| +// (apiinstances.go): a fleet-placed app's NEW instances must inherit its |
| +// placement uniformly (spec's "one flag on the app, all its instances |
| +// follow") rather than trusting whatever region a create request's body |
| +// happens to send — see handleAPIPlace's doc comment for why that |
| +// inheritance has to be a separate mechanism from the route/record stamping |
| +// below, not the same one. |
| +// |
| +// KNOWN LIMITATION: an app placed before it has EVER had a route (never |
| +// `carlos add`-ed, never had an instance provisioned and reconciled onto a |
| +// box) has nothing here to derive from — handleAPIPlace's own route walk is |
| +// a no-op when the app owns no routes yet, so this returns "" and that |
| +// app's very first instance would NOT inherit the placement, even though |
| +// `carlos apps place` was just told it succeeded. Reachable in particular |
| +// for `carlos apps create -place fleet/<name>` (spec's CLI surface, a later |
| +// task): the app has zero routes at the instant of placement by |
| +// construction. Not fixed here — doing so needs a placement fact that |
| +// outlives having no routes at all (an app-level record, not a |
| +// derived-from-routes one), which is a bigger change than this task's |
| +// scope; flagged for whichever task wires up `-place`. |
| +func appPlacement(reg *registry.Registry, acctSqid, app string) (string, error) { |
| + routes, err := reg.List() |
| + if err != nil { |
| + return "", err |
| + } |
| + for _, rt := range routes { |
| + if rt.Account != acctSqid || rt.App != app { |
| + continue |
| + } |
| + if _, ok := registry.FleetPlacement(rt.Region); ok { |
| + return rt.Region, nil |
| + } |
| + } |
| + return "", nil |
| +} |
| + |
| // handleAPIPlace answers `carlos apps place`: repoints where an app's routes |
| // (and its already-provisioned instances) should run. target "" clears |
| // placement back to platform-served/unconstrained; any other value must |
| @@ -360,13 +438,34 @@ func (s *Server) handleAPIFleetDetachBox(w http.ResponseWriter, r *http.Request) |
| // already-provisioned instance records' Region field |
| // (provision.SetRecordRegion) — both are read by provision.Reconcile's |
| // filter (rec.Region != cfg.Region) on a fleet box running |
| -// CARLOS_REGION=fleet/<name>. Stamping routes alone would repoint where a |
| -// BRAND NEW instance provisions but leave every instance that already |
| -// exists filed under its old region forever — silently exempt from a |
| -// placement change the caller was just told succeeded. Both writes happen |
| -// here, in this order (routes, then records); a console with no deployment |
| -// bucket configured (cfg.Releases == nil) refuses the whole request rather |
| -// than silently doing the routes-only half. |
| +// CARLOS_REGION=fleet/<name>. Stamping routes here does NOT, by itself, |
| +// repoint where a NEW instance provisions — instance-create validates its |
| +// Region from the REQUEST BODY (handleAPIInstanceCreate, apiinstances.go), |
| +// and provision.ValidRegion's shape (no "/") would refuse a "fleet/<name>" |
| +// value outright if it somehow arrived that way. A new instance inheriting |
| +// placement is a SEPARATE mechanism (appPlacement, below, consulted by |
| +// handleAPIInstanceCreate directly) — spec's "one flag on the app, all its |
| +// instances follow". What THIS handler's record write closes is the gap |
| +// that inheritance cannot: every instance that ALREADY EXISTS at the moment |
| +// placement changes. Inheritance only ever applies going forward, at |
| +// create time, so without the write below an already-provisioned record |
| +// would stay filed under its old region forever — silently exempt from a |
| +// placement change the caller was just told succeeded. |
| +// |
| +// ORDER MATTERS: routes are stamped BEFORE records, never the reverse. |
| +// provision.Reconcile's adoption pass (reconcile.go:265) retires a route |
| +// whose Region no longer matches what it minted the route from; a |
| +// records-first ordering would open a window in which a reconcile pass — |
| +// running concurrently, on either the OLD or the NEW fleet's box — could |
| +// see a record already repointed to the new target while the route still |
| +// names the old one, and read that route as no longer wanted, deleting a |
| +// route that is still live. Stamping routes first means the route is |
| +// always ahead of (or equal to) any record a concurrent reconcile pass |
| +// would compare it against, never behind. |
| +// |
| +// A console with no deployment bucket configured (cfg.Releases == nil) |
| +// refuses the whole request rather than silently doing the routes-only |
| +// half. |
| func (s *Server) handleAPIPlace(w http.ResponseWriter, r *http.Request) { |
| addr, acct, app, ok := s.apiAccountApp(w, r) |
| if !ok { |
diff --git a/internal/console/fleets_test.go b/internal/console/fleets_test.go| index 3e787bbce..8dd9bead6 100644 |
| --- a/internal/console/fleets_test.go |
| +++ b/internal/console/fleets_test.go |
| @@ -92,10 +92,10 @@ func TestFleetCreateListRoundTrip(t *testing.T) { |
| func TestFleetCreateDuplicateName409(t *testing.T) { |
| fx, token := fleetFixture(t, "code-fleet-dup") |
| u := fleetsURL(fx, fx.sqid) + "/new" |
| - if status, body := postJSON(t, u, map[string]string{"name": "acme", "bucket": "b1"}, token); status != http.StatusOK { |
| + if status, body := postJSON(t, u, map[string]string{"name": "acme", "bucket": "bkt-1"}, token); status != http.StatusOK { |
| t.Fatalf("first create = %d %v, want 200", status, body) |
| } |
| - status, body := postJSON(t, u, map[string]string{"name": "acme", "bucket": "b2"}, token) |
| + status, body := postJSON(t, u, map[string]string{"name": "acme", "bucket": "bkt-2"}, token) |
| if status != http.StatusConflict { |
| t.Fatalf("duplicate create = %d %v, want 409", status, body) |
| } |
| @@ -258,11 +258,11 @@ func TestFleetCrossAccountAccess404(t *testing.T) { |
| // 401 with no credential at all. |
| func TestFleetMissingBearerIs401(t *testing.T) { |
| fx, token := fleetFixture(t, "code-fleet-401") |
| - postJSON(t, fleetsURL(fx, fx.sqid)+"/new", map[string]string{"name": "acme", "bucket": "b"}, token) |
| + postJSON(t, fleetsURL(fx, fx.sqid)+"/new", map[string]string{"name": "acme", "bucket": "acme-data"}, token) |
| postJSON(t, fleetsURL(fx, fx.sqid)+"/acme/boxes", map[string]string{"label": "pi-1"}, token) |
| for _, c := range []struct{ method, url, body string }{ |
| - {http.MethodPost, fleetsURL(fx, fx.sqid) + "/new", `{"name":"x","bucket":"b"}`}, |
| + {http.MethodPost, fleetsURL(fx, fx.sqid) + "/new", `{"name":"x","bucket":"x-data"}`}, |
| {http.MethodGet, fleetsURL(fx, fx.sqid), ""}, |
| {http.MethodPost, fleetsURL(fx, fx.sqid) + "/acme/boxes", `{"label":"x"}`}, |
| {http.MethodPost, fleetsURL(fx, fx.sqid) + "/acme/boxes/pi-1/rotate", `{}`}, |
diff --git a/internal/registry/fleets.go b/internal/registry/fleets.go| index 9d2de77b6..ce06a2b8f 100644 |
| --- a/internal/registry/fleets.go |
| +++ b/internal/registry/fleets.go |
| @@ -9,12 +9,22 @@ package registry |
| import ( |
| "database/sql" |
| + "errors" |
| "fmt" |
| "regexp" |
| "strings" |
| "time" |
| ) |
| +// ErrFleetBoxActive is AddFleetBox's signal that the RowsAffected guard |
| +// fired — the (fleet, label) pair already names an ACTIVE box — as opposed |
| +// to any other failure (a bad shape, a missing fleet, a tx/busy/disk error) |
| +// that reaches AddFleetBox's caller. errors.Is against this is how a caller |
| +// tells "this is the ordinary duplicate-box conflict" apart from |
| +// "something is actually broken", which the plain fmt.Errorf this used to |
| +// return could not do without brittle string matching. |
| +var ErrFleetBoxActive = errors.New("registry: fleet already has an active box under that label") |
| + |
| // Fleet is one account-owned collection of remote boxes. |
| type Fleet struct { |
| Name string // ValidTunnelName |
| @@ -121,6 +131,37 @@ ON CONFLICT(name) DO UPDATE SET |
| return err |
| } |
| +// CreateFleet inserts a brand-new fleet, INSERT-owning the "does this name |
| +// already exist" decision the same way AddFleetBox's uniqueness guard owns |
| +// box uniqueness (below): the INSERT's own `ON CONFLICT(name) DO NOTHING` |
| +// and RowsAffected are the atomic check, not a caller's read-then-write — |
| +// so two callers racing the SAME never-before-seen name can never both |
| +// believe they created it, and the loser can never silently steal |
| +// ownership of the winner's row the way a call through UpsertFleet's |
| +// ON CONFLICT DO UPDATE would. created reports which caller won; a losing |
| +// call makes no change to the existing row at all — not even a rev bump. |
| +func (r *Registry) CreateFleet(f Fleet) (created bool, err error) { |
| + if !ValidTunnelName(f.Name) { |
| + return false, fmt.Errorf("invalid fleet name %q", f.Name) |
| + } |
| + if f.CreatedAt == 0 { |
| + f.CreatedAt = time.Now().Unix() |
| + } |
| + res, err := r.sql.Exec(` |
| +INSERT INTO fleets (name, account, bucket, created_at, rev) |
| +VALUES (?, ?, ?, ?, 1) |
| +ON CONFLICT(name) DO NOTHING`, |
| + f.Name, f.Account, f.Bucket, f.CreatedAt) |
| + if err != nil { |
| + return false, err |
| + } |
| + n, err := res.RowsAffected() |
| + if err != nil { |
| + return false, err |
| + } |
| + return n == 1, nil |
| +} |
| + |
| // GetFleet returns the fleet named name, or (nil, nil) if there is none. |
| func (r *Registry) GetFleet(name string) (*Fleet, error) { |
| row := r.sql.QueryRow(`SELECT name, account, bucket, created_at FROM fleets WHERE name = ?`, name) |
| @@ -180,7 +221,9 @@ func (r *Registry) ListFleets() ([]Fleet, error) { |
| // with the incoming token/status/added_at (rev bumped) — the path a |
| // re-provisioned box takes back onto its old name. RowsAffected()==0 means |
| // the guard fired (an active row was left untouched), which is the signal |
| -// this returns as the duplicate-active error. |
| +// this returns wrapping ErrFleetBoxActive — errors.Is against it, not |
| +// string matching, is how a caller tells this ordinary conflict apart from |
| +// an actual failure below. |
| func (r *Registry) AddFleetBox(b FleetBox) error { |
| if !ValidTunnelName(b.Fleet) { |
| return fmt.Errorf("invalid fleet name %q", b.Fleet) |
| @@ -233,7 +276,7 @@ WHERE fleet_boxes.status <> 'active'`, |
| } |
| if n == 0 { |
| tx.Rollback() |
| - return fmt.Errorf("fleet %q already has an active box %q", b.Fleet, b.Label) |
| + return fmt.Errorf("fleet %q already has an active box %q: %w", b.Fleet, b.Label, ErrFleetBoxActive) |
| } |
| return tx.Commit() |
| } |
diff --git a/internal/registry/fleets_test.go b/internal/registry/fleets_test.go| index 3946abb9a..486b972df 100644 |
| --- a/internal/registry/fleets_test.go |
| +++ b/internal/registry/fleets_test.go |
| @@ -1,7 +1,9 @@ |
| package registry |
| import ( |
| + "errors" |
| "strings" |
| + "sync" |
| "testing" |
| ) |
| @@ -48,6 +50,68 @@ func TestGetFleetMissingIsNilNil(t *testing.T) { |
| } |
| } |
| +// TestCreateFleetInsertOnly: the ordinary sequential case — first create |
| +// wins, a second with the same name is refused without touching the |
| +// existing row. |
| +func TestCreateFleetInsertOnly(t *testing.T) { |
| + reg := openTestRegistry(t) |
| + created, err := reg.CreateFleet(Fleet{Name: "acme", Account: "amadan", Bucket: "acme-data"}) |
| + if err != nil || !created { |
| + t.Fatalf("first CreateFleet: created=%v err=%v, want true, nil", created, err) |
| + } |
| + created, err = reg.CreateFleet(Fleet{Name: "acme", Account: "intruder", Bucket: "stolen"}) |
| + if err != nil || created { |
| + t.Fatalf("second CreateFleet (same name): created=%v err=%v, want false, nil", created, err) |
| + } |
| + got, err := reg.GetFleet("acme") |
| + if err != nil || got == nil || got.Account != "amadan" || got.Bucket != "acme-data" { |
| + t.Fatalf("GetFleet after a refused create = %+v, %v, want the FIRST caller's row untouched", got, err) |
| + } |
| +} |
| + |
| +// TestCreateFleetRaceInsertOnly is the property the console's |
| +// handleAPIFleetNew now depends on entirely (no more read-then-write): two |
| +// callers racing the SAME never-before-seen name — from two DIFFERENT |
| +// accounts — must never both win, and the row that survives must belong to |
| +// whichever one did. This is what makes UpsertFleet's ON CONFLICT DO UPDATE |
| +// (which CreateFleet deliberately does not use) unreachable from a genuine |
| +// race here. |
| +func TestCreateFleetRaceInsertOnly(t *testing.T) { |
| + reg := openTestRegistry(t) |
| + var wg sync.WaitGroup |
| + created := make([]bool, 2) |
| + errs := make([]error, 2) |
| + accounts := []string{"aaaa", "bbbb"} |
| + wg.Add(2) |
| + for i := 0; i < 2; i++ { |
| + go func(i int) { |
| + defer wg.Done() |
| + created[i], errs[i] = reg.CreateFleet(Fleet{Name: "acme", Account: accounts[i], Bucket: accounts[i] + "-data"}) |
| + }(i) |
| + } |
| + wg.Wait() |
| + |
| + for i, err := range errs { |
| + if err != nil { |
| + t.Fatalf("CreateFleet[%d]: %v", i, err) |
| + } |
| + } |
| + if created[0] == created[1] { |
| + t.Fatalf("created = %v, %v — want exactly one true", created[0], created[1]) |
| + } |
| + winner := accounts[0] |
| + if created[1] { |
| + winner = accounts[1] |
| + } |
| + got, err := reg.GetFleet("acme") |
| + if err != nil || got == nil { |
| + t.Fatalf("GetFleet after the race: %+v, %v", got, err) |
| + } |
| + if got.Account != winner { |
| + t.Fatalf("surviving row account = %q, want the winner's %q", got.Account, winner) |
| + } |
| +} |
| + |
| // TestFleetBoxRoundTrips pins AddFleetBox/ListFleetBoxes against the exact |
| // FleetBox shape later tasks depend on. |
| func TestFleetBoxRoundTrips(t *testing.T) { |
| @@ -90,9 +154,16 @@ func TestAddFleetBoxRejectsDuplicateActive(t *testing.T) { |
| if err := reg.AddFleetBox(box); err != nil { |
| t.Fatal(err) |
| } |
| - if err := reg.AddFleetBox(box); err == nil { |
| + err := reg.AddFleetBox(box) |
| + if err == nil { |
| t.Fatal("AddFleetBox on a duplicate active (fleet,label): got nil error, want one") |
| } |
| + // Distinguishable from any other failure via errors.Is, not string |
| + // matching — the property console/fleets.go's handler now relies on to |
| + // tell an ordinary conflict apart from a genuine infrastructure error. |
| + if !errors.Is(err, ErrFleetBoxActive) { |
| + t.Fatalf("AddFleetBox on a duplicate active: err = %v, want it to wrap ErrFleetBoxActive", err) |
| + } |
| } |
| // TestAddFleetBoxReactivatesADetachedLabel: the brief only forbids a |