console+release: the smaller holes the same two reviews found
Five, none of them in the queue, and four of them the same shape: a rule that holds everywhere except in the one place nobody looked. THE MIGRATION SEED TREATED A LOST RACE AS AGREEMENT. The create-only write converged two consoles, which is what it is for — but ErrCASConflict was read as "somebody else enabled it" without reading what they actually wrote. An operator running `disable` in the same instant as a migrating console's first tick loses: the console runs a full pass over tenant bytes against the switch that operator had just turned off. It reads the winner now. `disable` REFUSED OVER A RECORD IT COULD NOT PARSE. An unreadable system/antivirus.json already stops every sweep, so nothing was running — but it also left no door that could put the deployment back into a state it can read, and the one verb whose whole job is "stop" was refusing for the very reason it was needed. It repairs now, taking the etag from a raw read so the CAS stays honest; `enable` still refuses, because overwriting is a way out of a bad state and never a way into an enabled one. `store scan status` CLAIMED HEALTH FOR A DISABLED DEPLOYMENT. The reply carried the per-app record and the engine's own state and never mentioned the deployment switch, so an app enabled on a deployment where nothing sweeps read as covered. It reports deployment_enabled, and names the command when the two disagree — the same honesty the engine_error branch beside it already keeps about the wiring half. THE MINT FORM WOULD NOT SAY WHY. The Service-role account-wide refusal landed in the API door's error arms and not the form's, so the browser door — the one that explicitly OFFERS an account-wide credential — answered "could not mint, try again" and sent the owner round the same impossible request. The form now refuses before the passkey trip, as it does for every other unusable field, with the store's check still behind it as the authority. `S3Store.ListPage` TRUNCATED SILENTLY AT limit <= 0. Its own doc and DirStore both say that means no bound; one ListObjectsV2 returns at most a thousand keys and reports IsTruncated, which this ignored. It follows the truncation now, and the bounded path is still exactly one request — that being the entire reason the interface exists. And the deprecated variable is finally logged loudly at boot, as §4.2.1 said it would be: it still works, which is precisely why it needs saying, since the seed only fires while it is set. TestScanPassDoesNothingWhenDisabled passed against a pass with no switch check at all: RunSweep applies its own refusal before it lists anything, so refusingLister was never consulted, and scanPass only logs per-app errors. The seam it asserts on moves up to storeListerFor, which scanOneApp reaches before RunSweep. Both mutations were made and both now fail: conflict assumed enabled -> TestScanPassSeedYieldsToAConcurrentDisable no switch check in the tick -> TestScanPassDoesNothingWhenDisabled
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YJkJkxZZLgW8X8A5tk69Sq
11 files changed,
+373
−11
cmd/console/main.go+10 −0internal/console/apiscan.go+16 −0internal/console/apiscan_test.go+50 −0internal/console/apisystem.go+25 −3internal/console/apisystem_test.go+31 −0internal/console/scanpass.go+22 −4internal/console/scanpass_test.go+69 −1internal/console/svcpage.go+22 −0internal/console/svcpage_test.go+26 −0internal/release/pagelister_test.go+80 −0internal/release/store.go+22 −3
diff --git a/cmd/console/main.go b/cmd/console/main.go| index 2377d95ec..dce737fd5 100644 |
| --- a/cmd/console/main.go |
| +++ b/cmd/console/main.go |
| @@ -762,6 +762,16 @@ func storeScannerFor(d objstore.Deployment) (objstore.Scanner, bool) { |
| if addr == "" { |
| return nil, false |
| } |
| + // LOUDLY, on every boot, as the migration says. The variable still works, |
| + // which is exactly why it needs saying: a deployment can sit on it |
| + // indefinitely and nobody would learn that the seed only fires while it is |
| + // set, or that the switch is now a command rather than an edit to a unit |
| + // file. One line an operator greps for beats a release note nobody reads. |
| + if seed { |
| + log.Printf("console: CARLOS_STORE_SCANNER is DEPRECATED and is being read as the clamd address (%s). "+ |
| + "Set CARLOS_CLAMD_ADDR instead, and use `carlos system antivirus enable|disable` for the switch — "+ |
| + "the first tick seeds system/antivirus.json from this variable, and only while it is still set.", addr) |
| + } |
| // A scanner with no store to scan is a misconfiguration, and one loud |
| // failure at boot beats a pass that silently finds nothing forever. |
| if d.Backend == "" { |
diff --git a/internal/console/apiscan.go b/internal/console/apiscan.go| index a4e34cf54..c08d6f06d 100644 |
| --- a/internal/console/apiscan.go |
| +++ b/internal/console/apiscan.go |
| @@ -97,6 +97,22 @@ func (s *Server) scanReply(ctx context.Context, acct *Account, app string, rec * |
| "coverage": side, |
| "max_signature_age_seconds": int64(s.scanMaxSigAge() / time.Second), |
| } |
| + // THE DEPLOYMENT SWITCH IS PART OF THE ANSWER, not context somebody else |
| + // holds. A per-app record saying `enabled: clamav` while the deployment is |
| + // disabled describes a control that is not running, and this reply is what |
| + // `carlos store scan status` prints — so leaving it out is a new way for |
| + // the status output to lie, in exactly the shape the engine_error branch |
| + // below already refuses to allow for the wiring half. |
| + on, averr := release.AntivirusEnabled(ctx, s.cfg.Releases) |
| + out["deployment_enabled"] = on |
| + switch { |
| + case averr != nil: |
| + log.Printf("console: api scan status %s/%s: antivirus record: %v", acct.Sqid, app, averr) |
| + out["deployment_error"] = "the deployment's scanning state could not be read — no sweep is running" |
| + case !on && rec.Scanning != nil && rec.Scanning.Enabled != nil: |
| + out["deployment_error"] = "object scanning is disabled for this deployment " + |
| + "(`carlos system antivirus enable`) — this store is enabled, but nothing is sweeping it" |
| + } |
| if s.cfg.StoreScanner == nil { |
| out["engine"] = "" |
| // A record can say "enabled" on a deployment whose engine has since |
diff --git a/internal/console/apiscan_test.go b/internal/console/apiscan_test.go| index a28f10b25..d45bca02d 100644 |
| --- a/internal/console/apiscan_test.go |
| +++ b/internal/console/apiscan_test.go |
| @@ -483,3 +483,53 @@ func disableDeploymentScanning(t *testing.T, fx *consoleFixture) { |
| t.Fatal(err) |
| } |
| } |
| + |
| +// A per-app record saying `enabled: clamav` while the deployment switch is off |
| +// describes a control that is not running. `carlos store scan status` prints |
| +// this reply, so omitting the deployment half is a new way for the status |
| +// output to lie — the same lie the engine_error branch already refuses to tell |
| +// about the wiring half. |
| +func TestScanStatusReportsTheDeploymentSwitch(t *testing.T) { |
| + ctx := context.Background() |
| + fx, _, token := scanArmed(t, "code-scan-depstate") |
| + requestScanning(t, fx, token, false) |
| + if s, o := postJSON(t, scanURL(fx, "titogo")+"/grant", map[string]any{}, token); s != http.StatusOK { |
| + t.Fatalf("grant = %d %v", s, o) |
| + } |
| + |
| + status, out := getJSON(t, scanURL(fx, "titogo"), token) |
| + if status != http.StatusOK { |
| + t.Fatalf("status = %d %v", status, out) |
| + } |
| + if on, _ := out["deployment_enabled"].(bool); !on { |
| + t.Fatalf("deployment_enabled = %v with the switch on", out["deployment_enabled"]) |
| + } |
| + if _, bad := out["deployment_error"]; bad { |
| + t.Fatalf("a healthy deployment reported %v", out["deployment_error"]) |
| + } |
| + |
| + // Now turn the deployment off underneath an app whose own record still |
| + // says enabled. Nothing sweeps it, and the status has to say so. |
| + av, etag, err := release.GetAntivirus(ctx, fx.rel) |
| + if err != nil || av == nil { |
| + t.Fatalf("record = %+v, %v", av, err) |
| + } |
| + av.Enabled = false |
| + if _, err := release.PutAntivirus(ctx, fx.rel, av, etag); err != nil { |
| + t.Fatal(err) |
| + } |
| + |
| + _, out = getJSON(t, scanURL(fx, "titogo"), token) |
| + if on, _ := out["deployment_enabled"].(bool); on { |
| + t.Fatal("deployment_enabled stayed true after a disable") |
| + } |
| + msg := str(t, out, "deployment_error") |
| + if !strings.Contains(msg, "carlos system antivirus enable") { |
| + t.Fatalf("deployment_error = %q, want the command that fixes it", msg) |
| + } |
| + // The per-app record is UNCHANGED — the two facts are reported |
| + // separately, never collapsed. |
| + if out["scanning"] == nil { |
| + t.Fatal("the per-app record vanished from the reply") |
| + } |
| +} |
diff --git a/internal/console/apisystem.go b/internal/console/apisystem.go| index 28f738519..7abbe9d3a 100644 |
| --- a/internal/console/apisystem.go |
| +++ b/internal/console/apisystem.go |
| @@ -21,8 +21,10 @@ package console |
| import ( |
| "context" |
| + "errors" |
| "log" |
| "net/http" |
| + "os" |
| "time" |
| "github.com/carlosframework/platform/internal/release" |
| @@ -140,9 +142,29 @@ func (s *Server) handleAPISystemAntivirusSet(w http.ResponseWriter, r *http.Requ |
| av, etag, err := release.GetAntivirus(ctx, s.cfg.Releases) |
| if err != nil { |
| - log.Printf("console: api system antivirus: read: %v", err) |
| - jsonError(w, http.StatusBadGateway, "could not read the scanning state — try again") |
| - return |
| + if *req.Enabled { |
| + log.Printf("console: api system antivirus: read: %v", err) |
| + jsonError(w, http.StatusBadGateway, "could not read the scanning state — try again") |
| + return |
| + } |
| + // DISABLE REPAIRS, and this is the same deadlock argument apiscan.go |
| + // makes for revoke and enforce-off. A record this build cannot parse |
| + // already stops every sweep — GetAntivirus errors, so the tick returns |
| + // early and both doors refuse — but it also leaves the operator with |
| + // no door that can put the deployment back into a state it can read. |
| + // The one verb whose whole job is "stop" must not be the verb that |
| + // refuses for the very reason it is needed. |
| + // |
| + // The CAS is still honest: the etag comes from a raw read, so a |
| + // concurrent writer still wins. Only the CONTENT is being replaced. |
| + _, raw, gerr := s.cfg.Releases.Get(ctx, release.SystemKey(release.AntivirusName)) |
| + if gerr != nil && !errors.Is(gerr, os.ErrNotExist) { |
| + log.Printf("console: api system antivirus: raw read: %v", gerr) |
| + jsonError(w, http.StatusBadGateway, "could not read the scanning state — try again") |
| + return |
| + } |
| + log.Printf("console: api system antivirus: replacing an unreadable record on disable: %v", err) |
| + av, etag = &release.AntivirusSettings{}, raw |
| } |
| if av == nil { |
| av = &release.AntivirusSettings{} |
diff --git a/internal/console/apisystem_test.go b/internal/console/apisystem_test.go| index 6168417c6..b4412fdf7 100644 |
| --- a/internal/console/apisystem_test.go |
| +++ b/internal/console/apisystem_test.go |
| @@ -132,3 +132,34 @@ func TestSystemAntivirusNeedsAnExplicitState(t *testing.T) { |
| t.Fatalf("empty body = %d %v, want 400", status, out) |
| } |
| } |
| + |
| +// A record this build cannot parse already stops every sweep — so `disable` |
| +// refusing over it strands the operator in a state no door can leave. The one |
| +// verb whose whole job is "stop" must not refuse for the reason it is needed, |
| +// which is the same argument apiscan.go makes for revoke and enforce-off. |
| +// |
| +// `enable` still refuses: repairing by overwriting is a way OUT of a bad |
| +// state, never a way into an enabled one. |
| +func TestSystemAntivirusDisableRepairsAnUnreadableRecord(t *testing.T) { |
| + ctx := context.Background() |
| + fx, token := systemArmed(t, "code-sys-repair") |
| + |
| + if _, err := fx.rel.Put(ctx, release.SystemKey(release.AntivirusName), |
| + []byte(`{"schema":99,"enabled":true}`), ""); err != nil { |
| + t.Fatal(err) |
| + } |
| + |
| + if s, o := postJSON(t, fx.ts.URL+systemAntivirusURL, map[string]any{"enabled": true}, token); s != http.StatusBadGateway { |
| + t.Fatalf("enable over an unreadable record = %d %v, want 502 — enabling must never guess", s, o) |
| + } |
| + if s, o := postJSON(t, fx.ts.URL+systemAntivirusURL, map[string]any{"enabled": false}, token); s != http.StatusOK { |
| + t.Fatalf("disable over an unreadable record = %d %v, want 200", s, o) |
| + } |
| + av, _, err := release.GetAntivirus(ctx, fx.rel) |
| + if err != nil || av == nil || av.Enabled { |
| + t.Fatalf("record after the repair = %+v, %v", av, err) |
| + } |
| + if av.By != "alice@keymail.dev" { |
| + t.Fatalf("by = %q, want the operator who repaired it", av.By) |
| + } |
| +} |
diff --git a/internal/console/scanpass.go b/internal/console/scanpass.go| index 4fc90db7a..4c54cc3e8 100644 |
| --- a/internal/console/scanpass.go |
| +++ b/internal/console/scanpass.go |
| @@ -80,14 +80,32 @@ func (s *Server) antivirusEnabled(ctx context.Context) (bool, error) { |
| } |
| // Create-only: two consoles, or a restart loop, converge on one seed, and |
| // an operator who later runs `disable` is never overridden on the next |
| - // restart. A conflict means somebody else got there first, which is |
| - // success. |
| - if _, err := release.PutAntivirus(ctx, s.cfg.Releases, &release.AntivirusSettings{ |
| + // restart. |
| + _, err = release.PutAntivirus(ctx, s.cfg.Releases, &release.AntivirusSettings{ |
| Enabled: true, |
| Engine: "clamav", |
| At: time.Now().UTC(), |
| By: "migration from CARLOS_STORE_SCANNER", |
| - }, ""); err != nil && !errors.Is(err, release.ErrCASConflict) { |
| + }, "") |
| + if errors.Is(err, release.ErrCASConflict) { |
| + // Somebody else got there first — and NOT NECESSARILY WITH A SEED. |
| + // The race that matters is an operator running `disable` in the same |
| + // instant: the read above saw no record, the operator's write landed, |
| + // and answering "enabled" here would run a whole pass over tenant |
| + // bytes against the switch the operator had just turned off. So read |
| + // the winner rather than assuming it agrees with us. |
| + av, _, rerr := release.GetAntivirus(ctx, s.cfg.Releases) |
| + if rerr != nil { |
| + return false, rerr |
| + } |
| + if av == nil { |
| + // Created and gone again between the two calls. Neither on nor |
| + // off: say off and let the next tick decide. |
| + return false, nil |
| + } |
| + return av.Enabled, nil |
| + } |
| + if err != nil { |
| return false, err |
| } |
| return true, nil |
diff --git a/internal/console/scanpass_test.go b/internal/console/scanpass_test.go| index b12e44ad9..a6e651bf9 100644 |
| --- a/internal/console/scanpass_test.go |
| +++ b/internal/console/scanpass_test.go |
| @@ -90,6 +90,22 @@ func withLister(fx *consoleFixture, l objstore.ObjectLister) { |
| } |
| } |
| +// noSweeps fails the test if the pass gets as far as ASKING for a lister. |
| +// |
| +// refusingLister is a weaker assertion than it looks: RunSweep applies its own |
| +// refusals before it lists anything, so a pass that reached RunSweep with the |
| +// switch off would still never call ListPage — and scanPass only logs per-app |
| +// errors, so nothing would surface either. The seam is scanOneApp's last step |
| +// before RunSweep (scanpass.go), which makes it the earliest place a test can |
| +// tell "did not sweep" from "was not stopped". |
| +func noSweeps(t *testing.T, fx *consoleFixture) { |
| + t.Helper() |
| + fx.srv.storeListerFor = func(context.Context, *objstore.Provisioned) (objstore.ObjectLister, error) { |
| + t.Error("the pass went looking for a bucket to sweep") |
| + return &refusingLister{t: t}, nil |
| + } |
| +} |
| + |
| // No scanner configured: the pass does nothing and touches no backend. This is |
| // every deployment that has not opted in, which is all of them by default. |
| func TestScanPassInertWithoutScanner(t *testing.T) { |
| @@ -416,11 +432,63 @@ func TestScanPassDoesNothingWhenDisabled(t *testing.T) { |
| }, ""); err != nil { |
| t.Fatal(err) |
| } |
| - withLister(fx, &refusingLister{t: t}) |
| + noSweeps(t, fx) |
| + |
| + if err := fx.srv.scanPass(ctx); err != nil { |
| + t.Fatal(err) |
| + } |
| +} |
| + |
| +// The seed is create-only, and losing that race is NOT the same as winning it. |
| +// |
| +// An operator running `disable` in the same instant as a migrating console's |
| +// first tick is the case: the console read no record, the operator's write |
| +// landed, and the seed came back ErrCASConflict. Treating the conflict as |
| +// success — "somebody else got there first, so it must be on" — would run a |
| +// whole pass over tenant bytes against the switch that operator had just |
| +// turned off. The winner is read, not assumed. |
| +func TestScanPassSeedYieldsToAConcurrentDisable(t *testing.T) { |
| + ctx := context.Background() |
| + fx := newConsoleFixture(t) |
| + fx.srv.cfg.StoreScanner = objstore.NewMemScanner() |
| + fx.srv.cfg.ScanSeedFromDeprecatedEnv = true |
| + enableStoreRecordOnly(t, fx, fixtureAcct, fixtureApp) |
| + noSweeps(t, fx) |
| + |
| + // The operator's write lands between the console's read and its own |
| + // create-only write. |
| + fx.srv.cfg.Releases = &seedRaceStore{Store: fx.rel, race: func() { |
| + if _, err := release.PutAntivirus(ctx, fx.rel, &release.AntivirusSettings{ |
| + Enabled: false, Engine: "clamav", At: time.Now().UTC(), By: "op@example.com", |
| + }, ""); err != nil { |
| + t.Error(err) |
| + } |
| + }} |
| if err := fx.srv.scanPass(ctx); err != nil { |
| t.Fatal(err) |
| } |
| + av, _, err := release.GetAntivirus(ctx, fx.rel) |
| + if err != nil || av == nil || av.Enabled { |
| + t.Fatalf("the operator's disable was overwritten or ignored: %+v, %v", av, err) |
| + } |
| +} |
| + |
| +// seedRaceStore runs race() once, immediately after the first read of the |
| +// antivirus record — the instant between the seed's read and its write. |
| +type seedRaceStore struct { |
| + release.Store |
| + race func() |
| + seen bool |
| +} |
| + |
| +func (s *seedRaceStore) Get(ctx context.Context, key string) ([]byte, string, error) { |
| + body, etag, err := s.Store.Get(ctx, key) |
| + if !s.seen && key == release.SystemKey(release.AntivirusName) { |
| + s.seen = true |
| + s.race() |
| + } |
| + return body, etag, err |
| } |
| // enableStoreRecordOnly writes the per-app record without touching the |
diff --git a/internal/console/svcpage.go b/internal/console/svcpage.go| index 636efaa67..05e10bfa4 100644 |
| --- a/internal/console/svcpage.go |
| +++ b/internal/console/svcpage.go |
| @@ -281,6 +281,18 @@ func (s *Server) handleServiceCreatePage(w http.ResponseWriter, r *http.Request) |
| // creates an app of that name the credential silently acquires authority |
| // over an app its minter never chose. |
| app := strings.TrimSpace(r.FormValue("app")) |
| + // Said HERE as well as in NewServicePrincipal, and the duplication is |
| + // deliberate rather than the thing §3.4 warns about. The store keeps the |
| + // AUTHORITY — one place, so neither mint door can forget it — and this is |
| + // a validation, refusing before the passkey trip the way every other |
| + // unusable field on this form does. A form that sent an owner through a |
| + // step-up to be told "could not mint, try again" would send them round the |
| + // same impossible loop; the arm below still catches it if this drifts. |
| + if role == svcrole.Service && app == "" { |
| + back("err", "A service credential must name one app. It is held by an app at runtime, "+ |
| + "and an account-wide one would reach every app in this account. Nothing was minted.") |
| + return |
| + } |
| if app != "" { |
| found, err := s.store.HasAppIn(acct.ID, app) |
| if err != nil { |
| @@ -304,6 +316,16 @@ func (s *Server) handleServiceCreatePage(w http.ResponseWriter, r *http.Request) |
| back("err", "This account already has a live credential called “"+name+"”. Nothing was minted.") |
| return |
| } |
| + // The form is the door that OFFERS an account-wide credential, so it |
| + // is the one most likely to meet this refusal — and the one where |
| + // "could not mint, try again" is worst: the owner would retry the |
| + // same impossible request. Same words as the API door, so the two |
| + // cannot describe the same rule differently. |
| + if errors.Is(err, ErrAccountScopedService) { |
| + back("err", "A service credential must name one app. It is held by an app at runtime, "+ |
| + "and an account-wide one would reach every app in this account. Nothing was minted.") |
| + return |
| + } |
| log.Printf("console: service create %s/%s: %v", acct.Sqid, name, err) |
| back("err", "Could not mint the credential — try again.") |
| return |
diff --git a/internal/console/svcpage_test.go b/internal/console/svcpage_test.go| index af1faff85..bc334849d 100644 |
| --- a/internal/console/svcpage_test.go |
| +++ b/internal/console/svcpage_test.go |
| @@ -613,3 +613,29 @@ func TestPaneMintIsAudited(t *testing.T) { |
| t.Fatalf("no service-create event on the app the credential was scoped to: %+v", events) |
| } |
| } |
| + |
| +// The FORM is the door that offers an account-wide credential, so it is the |
| +// one most likely to meet the Service-role refusal — and the one where |
| +// "could not mint, try again" is worst, because the owner would retry the same |
| +// impossible request forever. Enforcement lives in NewServicePrincipal and |
| +// held either way; this is about the sentence the owner reads. |
| +func TestPaneSaysWhyAServiceCredentialNeedsAnApp(t *testing.T) { |
| + fx := newConsoleFixture(t) |
| + c := signInPaul(t, fx) |
| + csrf := sessionToken(t, fx, c) |
| + |
| + status, loc, body := postForm(t, c, fx.u("/services/new"), |
| + url.Values{"csrf": {csrf}, "name": {"scanbot"}, "role": {"service"}, "app": {""}}) |
| + if status != http.StatusSeeOther || loc != fx.path("/settings") { |
| + t.Fatalf("account-wide service mint = %d -> %q, want 303 back to the page:\n%s", status, loc, body) |
| + } |
| + if rows, err := fx.st.ServicePrincipals(OpsAccountID); err != nil || len(rows) != 0 { |
| + t.Fatalf("ServicePrincipals = %d rows %v, want the mint refused", len(rows), err) |
| + } |
| + // The page has to SAY it, not log it: the generic failure would send the |
| + // owner round the same loop. |
| + _, _, page := get(t, c, fx.u("/settings")) |
| + if !strings.Contains(page, "must name one app") { |
| + t.Fatalf("the settings page does not explain the refusal:\n%s", page) |
| + } |
| +} |
diff --git a/internal/release/pagelister_test.go b/internal/release/pagelister_test.go| index 14ed6704b..7adc59d95 100644 |
| --- a/internal/release/pagelister_test.go |
| +++ b/internal/release/pagelister_test.go |
| @@ -117,3 +117,83 @@ func TestS3StorePageListerOmitsEmptyCursor(t *testing.T) { |
| t.Fatal("an empty cursor was sent as start-after=") |
| } |
| } |
| + |
| +// "limit <= 0 means no bound" has to be true of BOTH stores, or a caller |
| +// asking for everything gets everything from DirStore and the first thousand |
| +// keys from S3 — with nothing to say there was more. One ListObjectsV2 is |
| +// capped at a thousand and reports IsTruncated; the unbounded path has to |
| +// follow it. |
| +func TestS3StorePageListerUnboundedFollowsTruncation(t *testing.T) { |
| + var pages int |
| + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
| + pages++ |
| + w.Header().Set("Content-Type", "application/xml") |
| + if pages == 1 { |
| + // A truncated first page, exactly as S3 answers one. |
| + fmt.Fprint(w, `<?xml version="1.0" encoding="UTF-8"?> |
| +<ListBucketResult xmlns="http://s3.amazonaws.com/doc/2006-03-01/"> |
| + <Name>b</Name><KeyCount>2</KeyCount><IsTruncated>true</IsTruncated> |
| + <NextContinuationToken>tok</NextContinuationToken> |
| + <Contents><Key>q/a</Key><Size>1</Size></Contents> |
| + <Contents><Key>q/b</Key><Size>1</Size></Contents> |
| +</ListBucketResult>`) |
| + return |
| + } |
| + if got := r.URL.Query().Get("continuation-token"); got != "tok" { |
| + t.Errorf("second request carried continuation-token %q, want the token the first page gave", got) |
| + } |
| + fmt.Fprint(w, `<?xml version="1.0" encoding="UTF-8"?> |
| +<ListBucketResult xmlns="http://s3.amazonaws.com/doc/2006-03-01/"> |
| + <Name>b</Name><KeyCount>1</KeyCount><IsTruncated>false</IsTruncated> |
| + <Contents><Key>q/c</Key><Size>1</Size></Contents> |
| +</ListBucketResult>`) |
| + })) |
| + defer srv.Close() |
| + |
| + rgwEnv(t, srv.URL) |
| + s, err := NewS3Store(context.Background(), "b") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + keys, err := s.ListPage(context.Background(), "q/", "", 0) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if fmt.Sprint(keys) != fmt.Sprint([]string{"q/a", "q/b", "q/c"}) { |
| + t.Fatalf("unbounded listing = %v, want every key across both pages", keys) |
| + } |
| + if pages != 2 { |
| + t.Fatalf("made %d requests, want the truncation followed", pages) |
| + } |
| +} |
| + |
| +// And the BOUNDED case stays one request, which is the whole reason this |
| +// interface exists: a drain that paged through a flooded app's entire queue |
| +// every tick would be paying for the listing it deliberately declined to act |
| +// on. |
| +func TestS3StorePageListerBoundedStaysOneRequest(t *testing.T) { |
| + var requests int |
| + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
| + requests++ |
| + w.Header().Set("Content-Type", "application/xml") |
| + fmt.Fprint(w, `<?xml version="1.0" encoding="UTF-8"?> |
| +<ListBucketResult xmlns="http://s3.amazonaws.com/doc/2006-03-01/"> |
| + <Name>b</Name><KeyCount>1</KeyCount><IsTruncated>true</IsTruncated> |
| + <NextContinuationToken>tok</NextContinuationToken> |
| + <Contents><Key>q/a</Key><Size>1</Size></Contents> |
| +</ListBucketResult>`) |
| + })) |
| + defer srv.Close() |
| + |
| + rgwEnv(t, srv.URL) |
| + s, err := NewS3Store(context.Background(), "b") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if _, err := s.ListPage(context.Background(), "q/", "", 1); err != nil { |
| + t.Fatal(err) |
| + } |
| + if requests != 1 { |
| + t.Fatalf("a bounded listing made %d requests, want exactly one", requests) |
| + } |
| +} |
diff --git a/internal/release/store.go b/internal/release/store.go| index 8ee6ca5c9..94a6dc175 100644 |
| --- a/internal/release/store.go |
| +++ b/internal/release/store.go |
| @@ -606,10 +606,29 @@ func (s *S3Store) ListPage(ctx context.Context, prefix, startAfter string, limit |
| if startAfter != "" { |
| in.StartAfter = &startAfter |
| } |
| - if limit > 0 { |
| - n := int32(limit) |
| - in.MaxKeys = &n |
| + if limit <= 0 { |
| + // "No bound" has to MEAN no bound. One ListObjectsV2 returns at most |
| + // a thousand keys and reports IsTruncated; a version of this that |
| + // ignored that would answer a caller asking for everything with the |
| + // first page and no hint there was more — and DirStore, which really |
| + // does return everything, would disagree with it silently. The |
| + // bounded case below is deliberately still one request: that is the |
| + // whole point of the interface. |
| + var keys []string |
| + p := awss3.NewListObjectsV2Paginator(s.Client, in) |
| + for p.HasMorePages() { |
| + page, err := p.NextPage(ctx) |
| + if err != nil { |
| + return nil, err |
| + } |
| + for _, o := range page.Contents { |
| + keys = append(keys, aws.ToString(o.Key)) |
| + } |
| + } |
| + return keys, nil |
| } |
| + n := int32(limit) |
| + in.MaxKeys = &n |
| out, err := s.Client.ListObjectsV2(ctx, in) |
| if err != nil { |
| return nil, err |