amadan
Decide a teardown on the ledger: one destroyer, no grant while it runs
Review round five was the first review of the teardown guard, and found it unsafe four ways (Astra F1, F3, F4; Opus N1 to N4). The decision is now one compare-and-swap on the bucket's ownership ledger. If nobody else holds the bucket, the ledger names the app as destroying it, and from that write until the app's interval is closed every grant of the name is refused. A grant and a teardown write the same object, so a grant cannot land between the decision and the delete, however long emptying the bucket takes. The decision is read again before emptying and before deleting. If somebody else holds it, the app's interval is marked as leaving in the same write, so two sharers torn down together cannot each defer to the other: the second to write finds the first leaving and destroys. A holder is an open interval, or a store record naming the bucket for an app the ledger has no interval for, which is a store provisioned before ledgers existed. An app that leaves with scanning enabled records that the scanner is owed, and whoever destroys the bucket disables it. A teardown records on its own store record that its bucket was deleted, and a re-run never touches a bucket of that name again. A teardown that leaves a bucket to an app of another account revokes its access, keeps its record and returns ErrBucketRetained: the bucket may still hold its objects, and that is for a person to settle. A minting grant is refused while another app holds the name. A grant over the same app's unfinished teardown takes the store back and runs the walk in full. Completeness is dated when both probes had answered, not when the caller read its clock.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
4 files changed,
+737
−171
internal/objstore/objstore.go+12 −0internal/objstore/ownership.go+192 −10internal/objstore/ownership_test.go+350 −109internal/objstore/walk.go+183 −52
diff --git a/internal/objstore/objstore.go b/internal/objstore/objstore.go| index a113d6b4c..b8a1a7063 100644 |
| --- a/internal/objstore/objstore.go |
| +++ b/internal/objstore/objstore.go |
| @@ -140,6 +140,18 @@ type Record struct { |
| Grant *GrantBlock `json:"grant,omitempty"` |
| Provisioned *Provisioned `json:"provisioned,omitempty"` |
| Scanning *ScanBlock `json:"scanning,omitempty"` |
| + // Teardown is what an unfinished teardown has already done, so a |
| + // re-run does not do it again to something that has changed since. |
| + Teardown *TeardownBlock `json:"teardown,omitempty"` |
| +} |
| + |
| +// TeardownBlock records the irreversible steps of a teardown that have |
| +// succeeded. |
| +type TeardownBlock struct { |
| + // BucketDeletedAt: this store's bucket was emptied and deleted. A |
| + // re-run must never empty or delete a bucket of that name again: if |
| + // one exists now, somebody else created it. |
| + BucketDeletedAt time.Time `json:"bucket_deleted_at"` |
| } |
| func Key(acct, app string) string { return release.AppPrefix(acct, app) + "store.json" } |
diff --git a/internal/objstore/ownership.go b/internal/objstore/ownership.go| index 01937f594..00b866a68 100644 |
| --- a/internal/objstore/ownership.go |
| +++ b/internal/objstore/ownership.go |
| @@ -62,8 +62,38 @@ type OwnerInterval struct { |
| App string `json:"app"` |
| From time.Time `json:"from"` |
| Until *time.Time `json:"until,omitempty"` |
| + // Leaving marks an open interval whose app is being torn down and has |
| + // given up its say over the bucket: it decided, in the same ledger |
| + // write that set this, that another app still holds the bucket. The |
| + // interval stays open (and its hours shared) until the app's |
| + // credentials are revoked, but it no longer counts as a holder when |
| + // someone else decides whether the bucket may be destroyed. |
| + Leaving bool `json:"leaving,omitempty"` |
| } |
| +// OwnerRef names an app, and since when something has been true of it. |
| +type OwnerRef struct { |
| + Account string `json:"account_sqid"` |
| + App string `json:"app"` |
| + Since time.Time `json:"since"` |
| +} |
| + |
| +// Refusals a walk gets from the ledger. Each is a state another walk put |
| +// the bucket in, and each clears when that walk finishes. |
| +var ( |
| + // ErrBucketBeingDestroyed: another app's teardown has decided to |
| + // destroy the bucket and has not finished. Nothing may be granted the |
| + // name until it has. |
| + ErrBucketBeingDestroyed = errors.New("objstore: bucket is being destroyed by another store's teardown") |
| + // ErrBucketHeld: a grant that mints its bucket found the name held by |
| + // another app. |
| + ErrBucketHeld = errors.New("objstore: bucket name is held by another store") |
| + // ErrBucketRetained: a teardown revoked its app's access and left the |
| + // bucket, because an app of ANOTHER account holds it and it may still |
| + // contain this app's objects. The record is kept; a person decides. |
| + ErrBucketRetained = errors.New("objstore: bucket retained: another account's store holds it") |
| +) |
| + |
| func (iv OwnerInterval) open() bool { return iv.Until == nil } |
| // overlaps reports whether the interval touches [from, to). |
| @@ -89,6 +119,19 @@ type BucketOwnership struct { |
| // for it. Write-once. |
| CompleteSince *time.Time `json:"complete_since,omitempty"` |
| Intervals []OwnerInterval `json:"intervals"` |
| + // Destroying names the app whose teardown has decided to destroy the |
| + // bucket, from the write that decided it until the write that closes |
| + // that app's interval. It is the lock between a teardown and a grant: |
| + // both decide by compare-and-swap on this one object, so either the |
| + // grant's interval is there when the teardown decides (and the |
| + // teardown leaves the bucket alone), or this marker is there when the |
| + // grant arrives (and the grant is refused). A teardown that died |
| + // leaves it standing, and re-running that teardown clears it. |
| + Destroying *OwnerRef `json:"destroying,omitempty"` |
| + // ScannerOwed records that an app which left the bucket had scanning |
| + // enabled on it and did not disable it, because another app still held |
| + // the bucket. Whoever destroys the bucket owes the scanner's removal. |
| + ScannerOwed bool `json:"scanner_owed,omitempty"` |
| } |
| func validOwnershipTime(t time.Time) error { |
| @@ -251,6 +294,21 @@ func writeBucketOwnership(ctx context.Context, s release.Store, bucket string, c |
| // grant block can be older than the run that probed, and the ledger vouches |
| // for nothing before it looked. |
| func OpenBucketOwner(ctx context.Context, s release.Store, bucket, acct, app string, from, observed time.Time, minted bool) error { |
| + return openBucketOwner(ctx, s, bucket, acct, app, from, observed, minted, false) |
| +} |
| + |
| +// openBucketOwner is OpenBucketOwner with exclusive: the caller is a grant |
| +// that mints this bucket under its own deterministic name, so nobody else |
| +// should hold it. With exclusive set, an open interval naming another app |
| +// refuses the write (ErrBucketHeld): creating the bucket would hand this |
| +// app a bucket that may hold the other's objects, or hand the other's |
| +// surviving key a bucket of this app's. |
| +// |
| +// Whatever exclusive says, a ledger another app's teardown is destroying |
| +// refuses the write (ErrBucketBeingDestroyed). The same app re-granting |
| +// during its own unfinished teardown takes the bucket back: the marker is |
| +// cleared and a Leaving interval becomes an ordinary one. |
| +func openBucketOwner(ctx context.Context, s release.Store, bucket, acct, app string, from, observed time.Time, minted, exclusive bool) error { |
| if !release.ValidAccount(acct) || !release.ValidApp(app) { |
| return fmt.Errorf("objstore: open bucket owner: invalid owner %q/%q", acct, app) |
| } |
| @@ -274,12 +332,36 @@ func OpenBucketOwner(ctx context.Context, s release.Store, bucket, acct, app str |
| b.CompleteSince = &since |
| } |
| } |
| - for _, iv := range b.Intervals { |
| - if iv.open() && iv.Account == acct && iv.App == app { |
| - return nil, errOwnershipUnchanged |
| + changed := false |
| + if d := b.Destroying; d != nil { |
| + if d.Account != acct || d.App != app { |
| + return nil, fmt.Errorf("%w: %s by %s/%s", ErrBucketBeingDestroyed, bucket, d.Account, d.App) |
| } |
| + b.Destroying, changed = nil, true |
| + } |
| + mine := false |
| + for i, iv := range b.Intervals { |
| + if !iv.open() { |
| + continue |
| + } |
| + if iv.Account == acct && iv.App == app { |
| + mine = true |
| + if iv.Leaving { |
| + b.Intervals[i].Leaving, changed = false, true |
| + } |
| + continue |
| + } |
| + if exclusive { |
| + return nil, fmt.Errorf("%w: %s by %s/%s", ErrBucketHeld, bucket, iv.Account, iv.App) |
| + } |
| + } |
| + if !mine { |
| + b.Intervals = append(b.Intervals, OwnerInterval{Account: acct, App: app, From: from}) |
| + changed = true |
| + } |
| + if !changed { |
| + return nil, errOwnershipUnchanged |
| } |
| - b.Intervals = append(b.Intervals, OwnerInterval{Account: acct, App: app, From: from}) |
| return b, nil |
| }) |
| } |
| @@ -310,6 +392,12 @@ func CloseBucketOwner(ctx context.Context, s release.Store, bucket, acct, app st |
| changed = true |
| } |
| } |
| + // The app that was destroying the bucket is done with it: the |
| + // marker goes with its interval, and the scanner it owed is gone |
| + // with the bucket. |
| + if d := b.Destroying; d != nil && d.Account == acct && d.App == app { |
| + b.Destroying, b.ScannerOwed, changed = nil, false, true |
| + } |
| if !changed { |
| return nil, errOwnershipUnchanged |
| } |
| @@ -317,15 +405,109 @@ func CloseBucketOwner(ctx context.Context, s release.Store, bucket, acct, app st |
| }) |
| } |
| -// openOwnerOtherThan returns an open interval naming an app other than |
| -// acct/app, if the ledger has one: someone else currently holds the bucket. |
| -func (b *BucketOwnership) openOwnerOtherThan(acct, app string) (OwnerInterval, bool) { |
| +// beginBucketTeardown decides, in one compare-and-swap on the ledger, what |
| +// acct/app's teardown may do with the bucket, and records the decision |
| +// where every other walk will see it. |
| +// |
| +// destroy is true when no other app holds the bucket. The ledger then |
| +// names this app as destroying it, which refuses every grant of the name |
| +// until this teardown closes its interval. scannerOwed reports that an app |
| +// which left earlier had scanning enabled. |
| +// |
| +// destroy is false when another app holds it: an open interval that is not |
| +// Leaving, or a store record (holders, read by the caller) naming the |
| +// bucket for an app the ledger has no interval for at all, which is a |
| +// store provisioned before ledgers existed. This app's interval is then |
| +// marked Leaving, so that two apps tearing down one shared bucket at the |
| +// same time cannot each defer to the other and leave it with nobody: the |
| +// second to write finds the first Leaving and destroys. If this app had |
| +// scanning enabled, the ledger records that the scanner is owed. other |
| +// names one holder. |
| +// |
| +// A store with no interval of its own (provisioned before ledgers) gets |
| +// one, from its grant, so the decision is recorded. An app whose interval |
| +// is already closed (a re-run after its close) is not reopened. |
| +func beginBucketTeardown(ctx context.Context, s release.Store, bucket, acct, app string, from time.Time, holders []OwnerRef, scanning bool, now time.Time) (destroy, scannerOwed bool, other OwnerRef, err error) { |
| + from, now = from.UTC(), now.UTC() |
| + if err := validOwnershipTime(from); err != nil { |
| + return false, false, OwnerRef{}, fmt.Errorf("objstore: teardown of bucket %s: %v", bucket, err) |
| + } |
| + err = writeBucketOwnership(ctx, s, bucket, func(b *BucketOwnership) (*BucketOwnership, error) { |
| + destroy, scannerOwed, other = false, false, OwnerRef{} |
| + if b == nil { |
| + b = &BucketOwnership{V: 1, Bucket: bucket} |
| + } |
| + mine, known := -1, map[string]bool{} |
| + for i, iv := range b.Intervals { |
| + known[iv.Account+"/"+iv.App] = true |
| + if iv.Account == acct && iv.App == app { |
| + if iv.open() { |
| + mine = i |
| + } |
| + continue |
| + } |
| + if iv.open() && !iv.Leaving && other.Account == "" { |
| + other = OwnerRef{Account: iv.Account, App: iv.App, Since: iv.From} |
| + } |
| + } |
| + if other.Account == "" { |
| + for _, h := range holders { |
| + if !known[h.Account+"/"+h.App] { |
| + other = h |
| + break |
| + } |
| + } |
| + } |
| + if d := b.Destroying; other.Account == "" && d != nil && (d.Account != acct || d.App != app) { |
| + // Another teardown already decided it is the last holder. |
| + other = *d |
| + } |
| + if mine < 0 && !known[acct+"/"+app] { |
| + b.Intervals = append(b.Intervals, OwnerInterval{Account: acct, App: app, From: from}) |
| + mine = len(b.Intervals) - 1 |
| + } |
| + if other.Account != "" { |
| + if mine >= 0 { |
| + b.Intervals[mine].Leaving = true |
| + } |
| + if scanning { |
| + b.ScannerOwed = true |
| + } |
| + if d := b.Destroying; d != nil && d.Account == acct && d.App == app { |
| + b.Destroying = nil |
| + } |
| + return b, nil |
| + } |
| + destroy, scannerOwed = true, b.ScannerOwed |
| + if mine >= 0 { |
| + b.Intervals[mine].Leaving = false |
| + } |
| + if b.Destroying == nil { |
| + b.Destroying = &OwnerRef{Account: acct, App: app, Since: now} |
| + } |
| + return b, nil |
| + }) |
| + return destroy, scannerOwed, other, err |
| +} |
| + |
| +// stillDestroying re-reads the ledger and reports whether acct/app's |
| +// decision to destroy the bucket still stands: its marker is there and no |
| +// other app holds the bucket. A teardown asks immediately before each |
| +// destructive step. |
| +func stillDestroying(ctx context.Context, s release.Store, bucket, acct, app string) (bool, error) { |
| + b, _, err := ReadBucketOwnership(ctx, s, bucket) |
| + if err != nil || b == nil { |
| + return false, err |
| + } |
| + if d := b.Destroying; d == nil || d.Account != acct || d.App != app { |
| + return false, nil |
| + } |
| for _, iv := range b.Intervals { |
| - if iv.open() && (iv.Account != acct || iv.App != app) { |
| - return iv, true |
| + if iv.open() && !iv.Leaving && (iv.Account != acct || iv.App != app) { |
| + return false, nil |
| } |
| } |
| - return OwnerInterval{}, false |
| + return true, nil |
| } |
| // OwnerAt answers who owned the bucket for the whole UTC hour containing |
diff --git a/internal/objstore/ownership_test.go b/internal/objstore/ownership_test.go| index a2ef3507e..88514ae1d 100644 |
| --- a/internal/objstore/ownership_test.go |
| +++ b/internal/objstore/ownership_test.go |
| @@ -576,55 +576,6 @@ func TestReRunOfAnAdoptingGrantIsStillAdopted(t *testing.T) { |
| } |
| } |
| -// A key that survives a failed DeleteUser can reach a bucket of the same |
| -// name created again. The interval must stay open until the credentials are |
| -// gone, so anyone else granted the name shares its hours and nobody is |
| -// billed for them. |
| -func TestTeardownKeepsOwnershipOpenUntilCredentialsAreRevoked(t *testing.T) { |
| - ctx := context.Background() |
| - s := release.NewDirStore(t.TempDir()) |
| - _, pub := testSealKeypair(t) |
| - p := NewMemProvisioner("eu-west-1") |
| - declare(t, s, "bab", "titogo", "TITO_S3") |
| - if _, err := Grant(ctx, s, p, testDeployment(), pub, "bab", "titogo", "op", GrantSourceOperator, "", t0); err != nil { |
| - t.Fatal(err) |
| - } |
| - p.FailNext = map[string]error{"DeleteUser": errors.New("iam had a bad moment")} |
| - if err := Teardown(ctx, s, p, nil, "bab", "titogo"); err == nil { |
| - t.Fatal("the injected DeleteUser failure did not stop the teardown") |
| - } |
| - if _, exists := p.Buckets[ledgerBucket]; exists { |
| - t.Fatal("fixture: the bucket should be gone by the time DeleteUser fails") |
| - } |
| - b := mustLedger(t, s, ledgerBucket) |
| - if b.Intervals[0].Until != nil { |
| - t.Fatal("ownership was closed while the credentials still exist") |
| - } |
| - // Another app adopts the name while those credentials live: the hours |
| - // are shared, and resolve to nobody. |
| - declare(t, s, "bac", "other", "OTHER_S3") |
| - adoptedAt := t0.Add(48 * time.Hour) |
| - if _, err := Grant(ctx, s, p, testDeployment(), pub, "bac", "other", "op", GrantSourceOperator, ledgerBucket, adoptedAt); err != nil { |
| - t.Fatal(err) |
| - } |
| - if acct, _, ok := mustLedger(t, s, ledgerBucket).OwnerAt(adoptedAt.Add(24 * time.Hour)); ok { |
| - t.Fatalf("resolved %q while an earlier grantee's credentials survive", acct) |
| - } |
| - // The re-run revokes the credentials and only then closes the interval. |
| - p.FailNext = nil |
| - old := ownershipNow |
| - closedAt := t0.Add(96 * time.Hour) |
| - ownershipNow = func() time.Time { return closedAt } |
| - defer func() { ownershipNow = old }() |
| - if err := Teardown(ctx, s, p, nil, "bab", "titogo"); err != nil { |
| - t.Fatal(err) |
| - } |
| - b = mustLedger(t, s, ledgerBucket) |
| - if b.Intervals[0].Until == nil || !b.Intervals[0].Until.Equal(closedAt) { |
| - t.Fatalf("interval after the re-run: %+v", b.Intervals[0]) |
| - } |
| -} |
| - |
| // The reviewed scenario in full: two apps were given the same adopted |
| // bucket before ledgers existed. Re-granting only one of them must not make |
| // it the owner of the other's transfer. |
| @@ -708,41 +659,6 @@ func TestGrantClaimsNothingWhileAnotherPrincipalNamesTheBucket(t *testing.T) { |
| } |
| } |
| -// The same failed teardown of a legacy store, with the record it now |
| -// leaves behind and a provisioner that cannot list principals (AWS today). |
| -// The new owner's grant joins a ledger that claims nothing, and while the |
| -// old key lives the two share every hour: nothing resolves. |
| -func TestFailedLegacyTeardownLeavesARecordTheNextGrantInherits(t *testing.T) { |
| - ctx := context.Background() |
| - s := release.NewDirStore(t.TempDir()) |
| - _, pub := testSealKeypair(t) |
| - mem := NewMemProvisioner("eu-west-1") |
| - p := noInventory{mem} |
| - declare(t, s, "bac", "other", "OTHER_S3") |
| - if _, err := Grant(ctx, s, p, testDeployment(), pub, "bac", "other", "op", GrantSourceOperator, ledgerBucket, t0); err != nil { |
| - t.Fatal(err) |
| - } |
| - if err := s.Delete(ctx, BucketOwnershipKey(ledgerBucket)); err != nil { |
| - t.Fatal(err) |
| - } |
| - mem.FailNext = map[string]error{"DeleteUser": errors.New("iam had a bad moment")} |
| - if err := Teardown(ctx, s, p, nil, "bac", "other"); err == nil { |
| - t.Fatal("the injected DeleteUser failure did not stop the teardown") |
| - } |
| - mem.FailNext = nil |
| - declare(t, s, "bab", "titogo", "TITO_S3") |
| - if _, err := Grant(ctx, s, p, testDeployment(), pub, "bab", "titogo", "op", GrantSourceOperator, "", t0.Add(72*time.Hour)); err != nil { |
| - t.Fatal(err) |
| - } |
| - b := mustLedger(t, s, ledgerBucket) |
| - if b.CompleteSince != nil || len(b.Intervals) != 2 { |
| - t.Fatalf("ledger after the reuse: %+v", b) |
| - } |
| - if acct, _, ok := b.OwnerAt(t0.Add(96 * time.Hour)); ok { |
| - t.Fatalf("resolved %q for a bucket a surviving key can reach", acct) |
| - } |
| -} |
| - |
| // A provisioner that can say whether a bucket exists and cannot list |
| // principals claims nothing. This is the AWS provisioner today. The wrapper |
| // keeps the bucket probe, so the only thing missing is the inventory. |
| @@ -843,12 +759,150 @@ func TestObservationHourDoesNotResolveToTheNewOwner(t *testing.T) { |
| } |
| } |
| -// The retried teardown of amadan discussion 191. B adopted the name A |
| -// mints. B's teardown deleted the bucket and failed to delete B's user. A |
| -// was then granted the name, created the bucket again and wrote to it. B's |
| -// teardown is retried: it must finish B (user, interval, record) and leave |
| -// A's bucket and A's objects exactly as they are. |
| -func TestRetriedTeardownLeavesABucketAnotherAppNowHolds(t *testing.T) { |
| +// Probes answer at once in these tests unless one says otherwise. |
| +func init() { grantElapsed = func(time.Time) time.Duration { return 0 } } |
| + |
| +// A teardown closes its ownership interval only after the credentials are |
| +// revoked, and while it is unfinished nothing can be granted the name: the |
| +// ledger says the bucket is being destroyed, and a grant is refused by the |
| +// same compare-and-swap that would have opened its interval. |
| +func TestTeardownHoldsTheNameUntilItHasFinished(t *testing.T) { |
| + ctx := context.Background() |
| + s := release.NewDirStore(t.TempDir()) |
| + _, pub := testSealKeypair(t) |
| + p := NewMemProvisioner("eu-west-1") |
| + declare(t, s, "bab", "titogo", "TITO_S3") |
| + if _, err := Grant(ctx, s, p, testDeployment(), pub, "bab", "titogo", "op", GrantSourceOperator, "", t0); err != nil { |
| + t.Fatal(err) |
| + } |
| + p.FailNext = map[string]error{"DeleteUser": errors.New("iam had a bad moment")} |
| + if err := Teardown(ctx, s, p, nil, "bab", "titogo"); err == nil { |
| + t.Fatal("the injected DeleteUser failure did not stop the teardown") |
| + } |
| + if _, exists := p.Buckets[ledgerBucket]; exists { |
| + t.Fatal("fixture: the bucket should be gone by the time DeleteUser fails") |
| + } |
| + b := mustLedger(t, s, ledgerBucket) |
| + if b.Intervals[0].Until != nil { |
| + t.Fatal("ownership was closed while the credentials still exist") |
| + } |
| + if b.Destroying == nil || b.Destroying.Account != "bab" || b.Destroying.App != "titogo" { |
| + t.Fatalf("the unfinished teardown is not recorded on the ledger: %+v", b.Destroying) |
| + } |
| + if rec, _, _ := Get(ctx, s, "bab", "titogo"); rec == nil || rec.Teardown == nil || rec.Teardown.BucketDeletedAt.IsZero() { |
| + t.Fatalf("the record does not say its bucket was deleted: %+v", rec) |
| + } |
| + // Another app is granted the name, by adoption and by minting: both are |
| + // refused while those credentials live. |
| + declare(t, s, "bac", "other", "OTHER_S3") |
| + adoptedAt := t0.Add(48 * time.Hour) |
| + if _, err := Grant(ctx, s, p, testDeployment(), pub, "bac", "other", "op", GrantSourceOperator, ledgerBucket, adoptedAt); !errors.Is(err, ErrBucketBeingDestroyed) { |
| + t.Fatalf("an adopting grant during an unfinished teardown: %v, want ErrBucketBeingDestroyed", err) |
| + } |
| + if _, exists := p.Buckets[ledgerBucket]; exists { |
| + t.Fatal("the refused grant created the bucket") |
| + } |
| + if _, ok := p.Users[UserName("bac", "other")]; ok { |
| + t.Fatal("the refused grant created a user") |
| + } |
| + // The re-run revokes the credentials and only then closes the interval |
| + // and lifts the marker. It does not touch a bucket again. |
| + p.FailNext = map[string]error{"EmptyBucket": errors.New("must not be called"), "DeleteBucket": errors.New("must not be called")} |
| + old := ownershipNow |
| + closedAt := t0.Add(96 * time.Hour) |
| + ownershipNow = func() time.Time { return closedAt } |
| + defer func() { ownershipNow = old }() |
| + if err := Teardown(ctx, s, p, nil, "bab", "titogo"); err != nil { |
| + t.Fatal(err) |
| + } |
| + p.FailNext = nil |
| + b = mustLedger(t, s, ledgerBucket) |
| + if b.Intervals[0].Until == nil || !b.Intervals[0].Until.Equal(closedAt) || b.Destroying != nil { |
| + t.Fatalf("ledger after the re-run: %+v destroying=%+v", b.Intervals[0], b.Destroying) |
| + } |
| + // Now the name is free. |
| + if _, err := Grant(ctx, s, p, testDeployment(), pub, "bac", "other", "op", GrantSourceOperator, ledgerBucket, closedAt.Add(time.Hour)); err != nil { |
| + t.Fatalf("a grant after the teardown finished: %v", err) |
| + } |
| +} |
| + |
| +// The same app re-granted over its own unfinished teardown takes the store |
| +// back: the marker is lifted, the record forgets the deleted bucket, and a |
| +// later teardown destroys the new one. |
| +func TestGrantOverItsOwnUnfinishedTeardownTakesTheStoreBack(t *testing.T) { |
| + ctx := context.Background() |
| + s := release.NewDirStore(t.TempDir()) |
| + _, pub := testSealKeypair(t) |
| + p := NewMemProvisioner("eu-west-1") |
| + declare(t, s, "bab", "titogo", "TITO_S3") |
| + if _, err := Grant(ctx, s, p, testDeployment(), pub, "bab", "titogo", "op", GrantSourceOperator, "", t0); err != nil { |
| + t.Fatal(err) |
| + } |
| + p.FailNext = map[string]error{"DeleteUser": errors.New("iam had a bad moment")} |
| + if err := Teardown(ctx, s, p, nil, "bab", "titogo"); err == nil { |
| + t.Fatal("the injected DeleteUser failure did not stop the teardown") |
| + } |
| + p.FailNext = nil |
| + if _, err := Grant(ctx, s, p, testDeployment(), pub, "bab", "titogo", "op", GrantSourceOperator, "", t0.Add(time.Hour)); err != nil { |
| + t.Fatalf("re-grant: %v", err) |
| + } |
| + if _, exists := p.Buckets[ledgerBucket]; !exists { |
| + t.Fatal("the re-grant did not create the bucket again") |
| + } |
| + if b := mustLedger(t, s, ledgerBucket); b.Destroying != nil { |
| + t.Fatalf("the marker outlived the re-grant: %+v", b.Destroying) |
| + } |
| + if rec, _, _ := Get(ctx, s, "bab", "titogo"); rec == nil || rec.Teardown != nil { |
| + t.Fatalf("the record still says its bucket was deleted: %+v", rec) |
| + } |
| + p.Buckets[ledgerBucket]["x"] = []byte("new data") |
| + if err := Teardown(ctx, s, p, nil, "bab", "titogo"); err != nil { |
| + t.Fatal(err) |
| + } |
| + if _, exists := p.Buckets[ledgerBucket]; exists { |
| + t.Fatal("the later teardown left the new bucket") |
| + } |
| +} |
| + |
| +// A grant that mints its bucket is refused when another app holds the |
| +// name. Creating the bucket would hand this app whatever the other left in |
| +// it, or hand the other's key this app's objects. |
| +func TestMintingGrantIsRefusedWhileAnotherAppHoldsTheName(t *testing.T) { |
| + ctx := context.Background() |
| + s := release.NewDirStore(t.TempDir()) |
| + _, pub := testSealKeypair(t) |
| + p := NewMemProvisioner("eu-west-1") |
| + declare(t, s, "bac", "other", "OTHER_S3") |
| + if _, err := Grant(ctx, s, p, testDeployment(), pub, "bac", "other", "op", GrantSourceOperator, ledgerBucket, t0); err != nil { |
| + t.Fatal(err) |
| + } |
| + p.Buckets[ledgerBucket]["secret.txt"] = []byte("the adopter's data") |
| + declare(t, s, "bab", "titogo", "TITO_S3") |
| + if _, err := Grant(ctx, s, p, testDeployment(), pub, "bab", "titogo", "op", GrantSourceOperator, "", t0.Add(time.Hour)); !errors.Is(err, ErrBucketHeld) { |
| + t.Fatalf("a minting grant over a held name: %v, want ErrBucketHeld", err) |
| + } |
| + if _, ok := p.Users[UserName("bab", "titogo")]; ok { |
| + t.Fatal("the refused grant created a user with a policy naming another app's bucket") |
| + } |
| + // Two adopters of one bucket are still allowed: that is an operator's |
| + // own sharing. |
| + declare(t, s, "bac", "second", "SECOND_S3") |
| + if _, err := Grant(ctx, s, p, testDeployment(), pub, "bac", "second", "op", GrantSourceOperator, ledgerBucket, t0.Add(2*time.Hour)); err != nil { |
| + t.Fatalf("a second adopter: %v", err) |
| + } |
| +} |
| + |
| +// The retried teardown of amadan discussion 191, in the state an OLDER |
| +// build leaves: B adopted the name A mints, B's teardown deleted the bucket |
| +// and failed at the user, and nothing recorded any of it (no ledger, no |
| +// mark on B's record). A was then granted the name, created the bucket |
| +// again and wrote to it. B's teardown is retried by this build. |
| +// |
| +// It finds A by A's store record. It revokes B's access and leaves A's |
| +// bucket, objects and scanner exactly as they are. A is another account, so |
| +// the teardown does not report itself done: B's record stands until a |
| +// person, or A's own teardown, settles the bucket. |
| +func TestRetriedLegacyTeardownLeavesABucketAnotherAppNowHolds(t *testing.T) { |
| ctx := context.Background() |
| s := release.NewDirStore(t.TempDir()) |
| _, pub := testSealKeypair(t) |
| @@ -862,6 +916,18 @@ func TestRetriedTeardownLeavesABucketAnotherAppNowHolds(t *testing.T) { |
| t.Fatal("the injected DeleteUser failure did not stop the teardown") |
| } |
| p.FailNext = nil |
| + // As the older build left it. |
| + if err := s.Delete(ctx, BucketOwnershipKey(ledgerBucket)); err != nil { |
| + t.Fatal(err) |
| + } |
| + rec, etag, err := Get(ctx, s, "bac", "other") |
| + if err != nil || rec == nil { |
| + t.Fatal(err) |
| + } |
| + rec.Teardown = nil |
| + if _, err := Put(ctx, s, "bac", "other", rec, etag); err != nil { |
| + t.Fatal(err) |
| + } |
| declare(t, s, "bab", "titogo", "TITO_S3") |
| if _, err := Grant(ctx, s, p, testDeployment(), pub, "bab", "titogo", "op", GrantSourceOperator, "", t0.Add(72*time.Hour)); err != nil { |
| t.Fatal(err) |
| @@ -873,8 +939,8 @@ func TestRetriedTeardownLeavesABucketAnotherAppNowHolds(t *testing.T) { |
| } |
| enableScanning(t, s, "bac", "other") |
| - if err := Teardown(ctx, s, p, sc, "bac", "other"); err != nil { |
| - t.Fatalf("the retry: %v", err) |
| + if err := Teardown(ctx, s, p, sc, "bac", "other"); !errors.Is(err, ErrBucketRetained) { |
| + t.Fatalf("the retry: %v, want ErrBucketRetained", err) |
| } |
| if got := string(p.Buckets[ledgerBucket]["invoice.pdf"]); got != "the new owner's data" { |
| t.Fatalf("the retried teardown destroyed the new owner's bucket (object now %q, bucket present %v)", got, p.Buckets[ledgerBucket] != nil) |
| @@ -885,37 +951,45 @@ func TestRetriedTeardownLeavesABucketAnotherAppNowHolds(t *testing.T) { |
| if _, ok := p.Users[UserName("bac", "other")]; ok { |
| t.Fatal("the retried teardown left its own user alive") |
| } |
| - if rec, _, err := Get(ctx, s, "bac", "other"); err != nil || rec != nil { |
| - t.Fatalf("the retried teardown left its record: %+v %v", rec, err) |
| - } |
| if _, ok := p.Users[UserName("bab", "titogo")]; !ok { |
| t.Fatal("the new owner's user was removed") |
| } |
| + if rec, _, _ := Get(ctx, s, "bac", "other"); rec == nil { |
| + t.Fatal("the record was deleted although the teardown is not done") |
| + } |
| b := mustLedger(t, s, ledgerBucket) |
| - open := 0 |
| + if !b.ScannerOwed { |
| + t.Fatal("the ledger does not record that the leaving app's scanner is owed") |
| + } |
| for _, iv := range b.Intervals { |
| - if iv.Until == nil { |
| - open++ |
| - if iv.Account != "bab" || iv.App != "titogo" { |
| - t.Fatalf("an interval is still open for %s/%s", iv.Account, iv.App) |
| - } |
| + if iv.Until == nil && (iv.Account != "bab" || iv.App != "titogo") { |
| + t.Fatalf("an interval is still open for %s/%s", iv.Account, iv.App) |
| } |
| } |
| - if open != 1 { |
| - t.Fatalf("open intervals = %d, want only the new owner's: %+v", open, b.Intervals) |
| - } |
| - // The last holder's teardown takes the bucket with it. |
| + // The new owner's teardown takes the bucket, and the scanner the other |
| + // app owed, with it. |
| if err := Teardown(ctx, s, p, sc, "bab", "titogo"); err != nil { |
| t.Fatal(err) |
| } |
| if _, exists := p.Buckets[ledgerBucket]; exists { |
| t.Fatal("the last holder's teardown left the bucket") |
| } |
| + if sc.IsEnabled(ledgerBucket) { |
| + t.Fatal("the scanner outlived the bucket: the plan is leaked for good") |
| + } |
| + // And now the first app's teardown can finish. |
| + if err := Teardown(ctx, s, p, sc, "bac", "other"); err != nil { |
| + t.Fatalf("the first app's teardown after the bucket was settled: %v", err) |
| + } |
| + if rec, _, _ := Get(ctx, s, "bac", "other"); rec != nil { |
| + t.Fatalf("the first app's record remains: %+v", rec) |
| + } |
| } |
| -// Two apps an operator handed one adopted bucket. Tearing one down revokes |
| -// its credentials and leaves the bucket, and the other app's access, alone. |
| -// Tearing the other down then deletes the bucket. |
| +// Two apps of one account that an operator handed one adopted bucket. |
| +// Tearing one down revokes its credentials, leaves the bucket and the |
| +// other's access alone, and finishes. Tearing the other down deletes the |
| +// bucket, and disables the scanner only the first app's record knew about. |
| func TestTeardownOfOneSharerLeavesASharedBucket(t *testing.T) { |
| ctx := context.Background() |
| s := release.NewDirStore(t.TempDir()) |
| @@ -929,18 +1003,70 @@ func TestTeardownOfOneSharerLeavesASharedBucket(t *testing.T) { |
| } |
| } |
| p.Buckets[shared]["kept"] = []byte("still needed by two") |
| - if err := Teardown(ctx, s, p, nil, "bab", "one"); err != nil { |
| + sc := NewMemScanner() |
| + if err := sc.EnsureEnabled(ctx, shared); err != nil { |
| + t.Fatal(err) |
| + } |
| + enableScanning(t, s, "bab", "one") |
| + if err := Teardown(ctx, s, p, sc, "bab", "one"); err != nil { |
| t.Fatal(err) |
| } |
| if string(p.Buckets[shared]["kept"]) != "still needed by two" { |
| t.Fatal("tearing down one sharer destroyed the shared bucket") |
| } |
| + if !sc.IsEnabled(shared) { |
| + t.Fatal("tearing down one sharer stripped the other's scanner") |
| + } |
| if _, ok := p.Users[UserName("bab", "two")]; !ok { |
| t.Fatal("the other sharer lost its user") |
| } |
| if _, ok := p.Users[UserName("bab", "one")]; ok { |
| t.Fatal("the torn-down sharer kept its user") |
| } |
| + if rec, _, _ := Get(ctx, s, "bab", "one"); rec != nil { |
| + t.Fatal("a same-account detach should finish and delete its record") |
| + } |
| + // The last holder's record has no scanning block. A nil scanner must |
| + // refuse, not leak the plan; a scanner disables it. |
| + if err := Teardown(ctx, s, p, nil, "bab", "two"); err == nil { |
| + t.Fatal("the last holder's teardown ran with no scanner while a scanner is owed") |
| + } |
| + if err := Teardown(ctx, s, p, sc, "bab", "two"); err != nil { |
| + t.Fatal(err) |
| + } |
| + if _, exists := p.Buckets[shared]; exists { |
| + t.Fatal("the last sharer's teardown left the bucket") |
| + } |
| + if sc.IsEnabled(shared) { |
| + t.Fatal("the scanner plan outlived the bucket") |
| + } |
| +} |
| + |
| +// The same two sharers, provisioned before ledgers existed: neither has an |
| +// interval. Tearing one down must still find the other, by its store |
| +// record, and leave the bucket. |
| +func TestTeardownFindsALegacyHolderByItsRecord(t *testing.T) { |
| + ctx := context.Background() |
| + s := release.NewDirStore(t.TempDir()) |
| + _, pub := testSealKeypair(t) |
| + p := NewMemProvisioner("eu-west-1") |
| + const shared = "carlos-store-shared-archive" |
| + for _, app := range []string{"one", "two"} { |
| + declare(t, s, "bab", app, "ARCHIVE_S3") |
| + if _, err := Grant(ctx, s, p, testDeployment(), pub, "bab", app, "op", GrantSourceOperator, shared, t0); err != nil { |
| + t.Fatal(err) |
| + } |
| + } |
| + if err := s.Delete(ctx, BucketOwnershipKey(shared)); err != nil { |
| + t.Fatal(err) |
| + } |
| + p.Buckets[shared]["kept"] = []byte("still needed by two") |
| + if err := Teardown(ctx, s, p, nil, "bab", "one"); err != nil { |
| + t.Fatal(err) |
| + } |
| + if string(p.Buckets[shared]["kept"]) != "still needed by two" { |
| + t.Fatal("a holder with no ledger interval lost its bucket") |
| + } |
| if err := Teardown(ctx, s, p, nil, "bab", "two"); err != nil { |
| t.Fatal(err) |
| } |
| @@ -948,3 +1074,118 @@ func TestTeardownOfOneSharerLeavesASharedBucket(t *testing.T) { |
| t.Fatal("the last sharer's teardown left the bucket") |
| } |
| } |
| + |
| +// Two sharers torn down at the same time. Each decides by compare-and-swap |
| +// on the ledger. The first to write finds the other holding the bucket and |
| +// leaves; the second finds the first leaving and is the last holder. They |
| +// cannot both defer to each other and leave the bucket with nobody. |
| +func TestConcurrentTeardownsOfASharedBucketLeaveExactlyOneDestroyer(t *testing.T) { |
| + ctx := context.Background() |
| + s := release.NewDirStore(t.TempDir()) |
| + now := t0.Add(time.Hour) |
| + for _, app := range []string{"one", "two"} { |
| + if err := OpenBucketOwner(ctx, s, ledgerBucket, "bab", app, t0, t0, false); err != nil { |
| + t.Fatal(err) |
| + } |
| + } |
| + // Both teardowns have read the ledger with both intervals open. Then |
| + // each makes its decision. |
| + destroyOne, _, _, err := beginBucketTeardown(ctx, s, ledgerBucket, "bab", "one", t0, nil, true, now) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + destroyTwo, owed, _, err := beginBucketTeardown(ctx, s, ledgerBucket, "bab", "two", t0, nil, false, now) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if destroyOne || !destroyTwo { |
| + t.Fatalf("decisions: one destroys=%v, two destroys=%v; want exactly the second", destroyOne, destroyTwo) |
| + } |
| + if !owed { |
| + t.Fatal("the destroyer was not told the leaving app's scanner is owed") |
| + } |
| + b := mustLedger(t, s, ledgerBucket) |
| + if b.Destroying == nil || b.Destroying.App != "two" { |
| + t.Fatalf("destroying marker: %+v", b.Destroying) |
| + } |
| + // A decision repeated (a re-run) is the same decision. |
| + if again, _, _, _ := beginBucketTeardown(ctx, s, ledgerBucket, "bab", "one", t0, nil, true, now); again { |
| + t.Fatal("the leaving app's re-run decided to destroy a bucket another app is destroying") |
| + } |
| + if again, _, _, _ := beginBucketTeardown(ctx, s, ledgerBucket, "bab", "two", t0, nil, false, now); !again { |
| + t.Fatal("the destroyer's re-run changed its mind") |
| + } |
| +} |
| + |
| +// A grant and a teardown of one bucket decide on the same ledger object. |
| +// If the grant's interval lands first, the teardown leaves the bucket. If |
| +// the teardown's decision lands first, the grant is refused. There is no |
| +// order in which the grant is given a bucket the teardown then destroys. |
| +func TestGrantAndTeardownCannotBothWin(t *testing.T) { |
| + ctx := context.Background() |
| + now := t0.Add(time.Hour) |
| + // The grant first. |
| + s := release.NewDirStore(t.TempDir()) |
| + if err := OpenBucketOwner(ctx, s, ledgerBucket, "bab", "one", t0, t0, false); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := OpenBucketOwner(ctx, s, ledgerBucket, "bab", "two", now, now, false); err != nil { |
| + t.Fatal(err) |
| + } |
| + if destroy, _, other, err := beginBucketTeardown(ctx, s, ledgerBucket, "bab", "one", t0, nil, false, now); err != nil || destroy || other.App != "two" { |
| + t.Fatalf("teardown after a grant landed: destroy=%v other=%+v err=%v", destroy, other, err) |
| + } |
| + // The teardown first. |
| + s = release.NewDirStore(t.TempDir()) |
| + if err := OpenBucketOwner(ctx, s, ledgerBucket, "bab", "one", t0, t0, false); err != nil { |
| + t.Fatal(err) |
| + } |
| + if destroy, _, _, err := beginBucketTeardown(ctx, s, ledgerBucket, "bab", "one", t0, nil, false, now); err != nil || !destroy { |
| + t.Fatalf("teardown with no other holder: destroy=%v err=%v", destroy, err) |
| + } |
| + if err := OpenBucketOwner(ctx, s, ledgerBucket, "bab", "two", now, now, false); !errors.Is(err, ErrBucketBeingDestroyed) { |
| + t.Fatalf("a grant after the teardown decided: %v, want ErrBucketBeingDestroyed", err) |
| + } |
| + if ok, err := stillDestroying(ctx, s, ledgerBucket, "bab", "one"); err != nil || !ok { |
| + t.Fatalf("the decision no longer stands: %v %v", ok, err) |
| + } |
| + // A hand on the ledger that takes the marker away stops the teardown |
| + // before its next destructive step. |
| + if err := CloseBucketOwner(ctx, s, ledgerBucket, "bab", "one", now); err != nil { |
| + t.Fatal(err) |
| + } |
| + if ok, _ := stillDestroying(ctx, s, ledgerBucket, "bab", "one"); ok { |
| + t.Fatal("a decision whose marker is gone still stands") |
| + } |
| +} |
| + |
| +// The completeness claim is dated when the probes had answered, not when |
| +// the caller read its clock. The caller's clock says 09:59:58; the probes |
| +// answer twelve seconds later, at 10:00:10. A store that held the name |
| +// could have moved bytes at 10:00:01 and been gone by 10:00:05, so the |
| +// 10:00 hour must not resolve to the new owner. |
| +func TestCompletenessIsDatedAfterTheProbesAnswer(t *testing.T) { |
| + ctx := context.Background() |
| + s := release.NewDirStore(t.TempDir()) |
| + _, pub := testSealKeypair(t) |
| + p := NewMemProvisioner("eu-west-1") |
| + prev := grantElapsed |
| + grantElapsed = func(time.Time) time.Duration { return 12 * time.Second } |
| + defer func() { grantElapsed = prev }() |
| + declare(t, s, "bab", "titogo", "TITO_S3") |
| + callerClock := time.Date(2026, 9, 3, 9, 59, 58, 0, time.UTC) |
| + if _, err := Grant(ctx, s, p, testDeployment(), pub, "bab", "titogo", "op", GrantSourceOperator, "", callerClock); err != nil { |
| + t.Fatal(err) |
| + } |
| + b := mustLedger(t, s, ledgerBucket) |
| + want := time.Date(2026, 9, 3, 10, 0, 10, 0, time.UTC) |
| + if b.CompleteSince == nil || !b.CompleteSince.Equal(want) { |
| + t.Fatalf("complete_since = %v, want %v (after the probes)", b.CompleteSince, want) |
| + } |
| + if acct, _, ok := b.OwnerAt(time.Date(2026, 9, 3, 10, 0, 0, 0, time.UTC)); ok { |
| + t.Fatalf("the 10:00 hour resolved to %q although the probes answered ten seconds into it", acct) |
| + } |
| + if _, _, ok := b.OwnerAt(time.Date(2026, 9, 3, 11, 0, 0, 0, time.UTC)); !ok { |
| + t.Fatal("the 11:00 hour did not resolve") |
| + } |
| +} |
diff --git a/internal/objstore/walk.go b/internal/objstore/walk.go| index b133ea732..d6a1a6c9d 100644 |
| --- a/internal/objstore/walk.go |
| +++ b/internal/objstore/walk.go |
| @@ -11,6 +11,7 @@ import ( |
| "fmt" |
| "log" |
| "regexp" |
| + "strings" |
| "time" |
| "github.com/carlosframework/platform/internal/appconfig" |
| @@ -139,6 +140,12 @@ func ValidBucketName(name string) bool { return bucketNameRe.MatchString(name) } |
| // it must not re-choose the bucket. |
| func Grant(ctx context.Context, s release.Store, p Provisioner, d Deployment, |
| sealPub *ecdh.PublicKey, acct, app, actor, source, adoptBucket string, now time.Time) (*Record, error) { |
| + // The caller's clock was read before this walk began. What the walk |
| + // observes, it observes later: the completeness claim below is dated |
| + // by the caller's clock plus the time this walk has run, taken AFTER |
| + // the probes return, never at the instant the caller happened to read |
| + // the clock. |
| + started := time.Now() |
| // 1. resolve + write the grant block |
| rec, etag, err := Get(ctx, s, acct, app) |
| if err != nil { |
| @@ -147,6 +154,18 @@ func Grant(ctx context.Context, s release.Store, p Provisioner, d Deployment, |
| if rec == nil || rec.Declared == nil { |
| return nil, ErrNotDeclared |
| } |
| + // A grant over an unfinished teardown takes the store back. Whatever |
| + // that teardown had already destroyed is created again below, so its |
| + // record of having destroyed it must not outlive this. And the store is |
| + // not healthy whatever its record and delivered variables say: the |
| + // walk runs in full (rebuilt, at the short-circuit below). |
| + rebuilt := rec.Teardown != nil |
| + if rebuilt { |
| + rec.Teardown = nil |
| + if etag, err = Put(ctx, s, acct, app, rec, etag); err != nil { |
| + return nil, err |
| + } |
| + } |
| // The key this walk started from, for the step-6 retry's guard below. "" |
| // is the ordinary case: a first grant, or a re-run of one that died before |
| // it recorded anything. |
| @@ -198,7 +217,12 @@ func Grant(ctx context.Context, s release.Store, p Provisioner, d Deployment, |
| // A provisioner that cannot answer either question, or a probe that |
| // fails, claims nothing. A ledger that already exists is left as it is |
| // whatever this computes. |
| + // |
| + // The claim is dated when the probes had both answered, not when the |
| + // caller read its clock: a store that held the name could have moved |
| + // bytes and vanished between the two. |
| minted := false |
| + observed := now |
| if rec.Grant.Minted && rec.Provisioned == nil { |
| prober, canProbe := p.(BucketProber) |
| inventory, canList := p.(GranteeProber) |
| @@ -206,9 +230,14 @@ func Grant(ctx context.Context, s release.Store, p Provisioner, d Deployment, |
| exists, perr := prober.BucketExists(ctx, bucket) |
| grantees, gerr := inventory.BucketGrantees(ctx, bucket) |
| minted = perr == nil && gerr == nil && !exists && len(grantees) == 0 |
| + observed = now.Add(grantElapsed(started)) |
| } |
| } |
| - if err := OpenBucketOwner(ctx, s, bucket, acct, app, grantOwnershipFrom(rec.Grant, now), now, minted); err != nil { |
| + // A grant that mints its bucket expects to be the only holder of the |
| + // name (exclusive): it is refused when another app holds it. Every |
| + // grant is refused while another app's teardown is destroying the |
| + // bucket. Both are decided by the ledger write itself. |
| + if err := openBucketOwner(ctx, s, bucket, acct, app, grantOwnershipFrom(rec.Grant, now), observed, minted, rec.Grant.Minted); err != nil { |
| return nil, fmt.Errorf("objstore: record ownership of %s: %w", bucket, err) |
| } |
| // 2. the principal and its policy. EnsureUser runs UNCONDITIONALLY, before |
| @@ -244,7 +273,7 @@ func Grant(ctx context.Context, s release.Store, p Provisioner, d Deployment, |
| // deployment has since changed or dropped means it is reaching for the |
| // wrong one, which only a re-walk (which rewrites the set, and unsets |
| // _ENDPOINT when there is no longer an endpoint) can put right. |
| - if rec.Provisioned != nil && rec.Provisioned.Endpoint == d.Endpoint { |
| + if !rebuilt && rec.Provisioned != nil && rec.Provisioned.Endpoint == d.Endpoint { |
| missing, err := EnvDelivered(ctx, s, acct, app, rec.Declared.EnvPrefix, d.Endpoint != "") |
| if err != nil { |
| return nil, err |
| @@ -539,55 +568,51 @@ func Teardown(ctx context.Context, s release.Store, p Provisioner, sc Scanner, a |
| default: |
| bucket = rec.Grant.Bucket |
| } |
| - // The bucket's ownership ledger is read before anything is destroyed, |
| - // for two reasons. |
| + // What this teardown may do with the bucket is decided before anything |
| + // is destroyed, and decided where every other walk will see it. |
| // |
| - // A store provisioned before ledgers existed has none. It is recorded |
| - // now, incomplete: if this teardown deletes the bucket and then fails to |
| - // delete the user, the surviving key still names the bucket, and the |
| - // open interval written here is the only record that it does. Anyone |
| - // granted the name afterwards finds a ledger that claims nothing and |
| - // shares its hours with this app. Only when no ledger exists at all: an |
| - // existing one already says what it knows, and a re-run after the close |
| - // below must not reopen it. |
| + // A bucket ANOTHER app holds is not this teardown's to destroy. That |
| + // happens three ways: two apps an operator handed one adopted bucket; a |
| + // re-run of a teardown that already deleted its bucket and failed at |
| + // the user, after which another app was granted the name and created |
| + // the bucket again; and a store provisioned before ownership ledgers |
| + // existed, which holds the bucket with no interval to show for it. The |
| + // first two are in the ledger. The third is found by reading every |
| + // store record for one that names this bucket. |
| // |
| - // And a bucket ANOTHER app holds is not this teardown's to destroy. The |
| - // ledger shows an open interval for every app granted the name, so it |
| - // catches both ways that happens: two apps an operator handed one |
| - // adopted bucket, and a re-run of a teardown that already deleted its |
| - // bucket and failed at the user, after which another app was granted the |
| - // name and created the bucket again. Emptying and deleting it then would |
| - // destroy that app's data; disabling its scanner would strip that app's |
| - // protection. Both are skipped. This app's credentials are still revoked |
| - // and its interval closed, and when the last holder is torn down, no |
| - // other interval is open and the bucket goes with it. |
| + // The decision is one compare-and-swap on the ledger |
| + // (beginBucketTeardown). If nobody else holds the bucket, the ledger |
| + // names this app as destroying it, and from that write until this app's |
| + // interval is closed every grant of the name is refused: a grant cannot |
| + // land between the decision and the delete, however long emptying the |
| + // bucket takes. If somebody else holds it, this app's interval is |
| + // marked as leaving, the bucket and its scanner are left alone, and |
| + // only this app's credentials are revoked. |
| // |
| - // What this cannot see: a grant of the name that lands between this read |
| - // and the delete below (the walk has no cross-process lock on a bucket |
| - // name), and a holder provisioned before ledgers existed whose interval |
| - // was never written. |
| - // |
| - // What it can leave behind: a scanner plan. If this app enabled scanning |
| - // on a bucket it shared and the last holder's record does not say so, |
| - // that holder's teardown deletes the bucket without disabling the plan, |
| - // and nothing names the plan afterwards. Leaving another app's bucket |
| - // unprotected would be worse, and the case needs an operator to have |
| - // shared an adopted bucket and scanned it under one app only. |
| - sharedWith := "" |
| - if bucket != "" && ValidBucketName(bucket) { |
| - existing, _, lerr := ReadBucketOwnership(ctx, s, bucket) |
| - if lerr != nil { |
| - return fmt.Errorf("objstore: read ownership of %s: %w", bucket, lerr) |
| - } |
| - if existing == nil { |
| - now := ownershipNow() |
| - if err := OpenBucketOwner(ctx, s, bucket, acct, app, grantOwnershipFrom(rec.Grant, now), now, false); err != nil { |
| - return fmt.Errorf("objstore: record ownership of %s: %w", bucket, err) |
| - } |
| - } else if other, ok := existing.openOwnerOtherThan(acct, app); ok { |
| - sharedWith = other.Account + "/" + other.App |
| - log.Printf("objstore: teardown of %s/%s leaves bucket %s and its scanner in place: %s holds it", acct, app, bucket, sharedWith) |
| + // A teardown that already deleted its bucket says so on its own record |
| + // (TeardownBlock) and never touches a bucket of that name again. |
| + destroy, scannerOwed := false, false |
| + bucketDone := rec.Teardown != nil && !rec.Teardown.BucketDeletedAt.IsZero() |
| + var holder OwnerRef |
| + if bucket != "" && ValidBucketName(bucket) && !bucketDone { |
| + holders, herr := otherStoreHolders(ctx, s, bucket, acct, app) |
| + if herr != nil { |
| + return fmt.Errorf("objstore: list the stores holding %s: %w", bucket, herr) |
| + } |
| + now := ownershipNow() |
| + scanning := rec.Scanning != nil && rec.Scanning.Enabled != nil |
| + var derr error |
| + destroy, scannerOwed, holder, derr = beginBucketTeardown(ctx, s, bucket, acct, app, grantOwnershipFrom(rec.Grant, now), holders, scanning, now) |
| + if derr != nil { |
| + return fmt.Errorf("objstore: decide the teardown of %s: %w", bucket, derr) |
| + } |
| + if !destroy { |
| + log.Printf("objstore: teardown of %s/%s leaves bucket %s and its scanner in place: %s/%s holds it", acct, app, bucket, holder.Account, holder.App) |
| } |
| + } else if bucket != "" && !ValidBucketName(bucket) { |
| + // A name Grant would refuse was never granted through this walk and |
| + // has no ledger. It is destroyed as it always was. |
| + destroy = true |
| } |
| // The scanner goes BEFORE the bucket, while the record that names it still |
| // exists. A Malware Protection plan (and its managed EventBridge rule) |
| @@ -596,10 +621,15 @@ func Teardown(ctx context.Context, s release.Store, p Provisioner, sc Scanner, a |
| // it any more — the read-first, delete-last failure this walk's ordering |
| // exists to prevent. |
| // |
| - // A nil scanner over an enabled record REFUSES, for the same reason a nil |
| - // provisioner does above: a teardown that cannot reach the engine must not |
| - // go on to delete the only record of what it should have disabled. |
| - if rec.Scanning != nil && rec.Scanning.Enabled != nil && bucket != "" && sharedWith == "" { |
| + // It is owed when this record has scanning enabled, and when an app that |
| + // left this bucket earlier had (the ledger carries that debt to whoever |
| + // destroys the bucket). |
| + // |
| + // A nil scanner over a scanner that is owed REFUSES, for the same reason |
| + // a nil provisioner does above: a teardown that cannot reach the engine |
| + // must not go on to delete the only record of what it should have |
| + // disabled. |
| + if destroy && bucket != "" && ((rec.Scanning != nil && rec.Scanning.Enabled != nil) || scannerOwed) { |
| if sc == nil { |
| return fmt.Errorf("objstore: %s/%s has scanning enabled and this deployment has no scanner configured (CARLOS_STORE_SCANNER)", acct, app) |
| } |
| @@ -611,16 +641,50 @@ func Teardown(ctx context.Context, s release.Store, p Provisioner, sc Scanner, a |
| // and validates a name before writing the block), so there is nothing |
| // nameable to destroy — but the deterministic user might exist, and the |
| // delivered vars might too, so only the bucket steps are skipped. |
| - if bucket != "" && sharedWith == "" { |
| + if destroy && bucket != "" { |
| + // The decision is checked again before each destructive step. With |
| + // the marker standing no grant can have landed, so a change here is |
| + // an operator's hand on the ledger; the teardown stops and is re-run. |
| + confirm := func(step string) error { |
| + if !ValidBucketName(bucket) { |
| + return nil |
| + } |
| + ok, cerr := stillDestroying(ctx, s, bucket, acct, app) |
| + if cerr != nil { |
| + return fmt.Errorf("objstore: re-read ownership of %s before %s: %w", bucket, step, cerr) |
| + } |
| + if !ok { |
| + return fmt.Errorf("objstore: ownership of %s changed during the teardown of %s/%s; nothing more was destroyed, run it again", bucket, acct, app) |
| + } |
| + return nil |
| + } |
| + if err := confirm("emptying it"); err != nil { |
| + return err |
| + } |
| // Empty before delete: a real backend refuses to delete a bucket with |
| // objects in it, and EmptyBucket is paginated and resumable for exactly |
| // the bucket too big to clear in one call. |
| if err := p.EmptyBucket(ctx, bucket); err != nil { |
| return fmt.Errorf("objstore: empty bucket %s: %w", bucket, err) |
| } |
| + if err := confirm("deleting it"); err != nil { |
| + return err |
| + } |
| if err := p.DeleteBucket(ctx, bucket); err != nil { |
| return fmt.Errorf("objstore: delete bucket %s: %w", bucket, err) |
| } |
| + // Recorded at once: a re-run after a failure below must not empty or |
| + // delete a bucket of this name again. |
| + fresh, fetag, gerr := Get(ctx, s, acct, app) |
| + if gerr != nil { |
| + return fmt.Errorf("objstore: re-read store record for %s/%s: %w", acct, app, gerr) |
| + } |
| + if fresh != nil { |
| + fresh.Teardown = &TeardownBlock{BucketDeletedAt: ownershipNow()} |
| + if _, perr := Put(ctx, s, acct, app, fresh, fetag); perr != nil { |
| + return fmt.Errorf("objstore: record the deleted bucket for %s/%s: %w", acct, app, perr) |
| + } |
| + } |
| } |
| // DeleteUser reaps the keys and the inline policy, so the credential dies |
| // with the estate it was scoped to — no orphan principal keeps a key that |
| @@ -643,6 +707,17 @@ func Teardown(ctx context.Context, s release.Store, p Provisioner, sc Scanner, a |
| return fmt.Errorf("objstore: close ownership of %s: %w", bucket, err) |
| } |
| } |
| + // Left in place for an app of ANOTHER account: the bucket may still hold |
| + // this app's objects, and that account can read them. This app's access |
| + // is revoked and its interval closed, and the walk stops here with the |
| + // record standing, so the teardown is not reported as done and a person |
| + // decides what happens to the data. Run again once the other store is |
| + // gone, it destroys the bucket and finishes. An app of the SAME account |
| + // holding the bucket is the operator's own sharing, and the teardown |
| + // finishes quietly. |
| + if !destroy && !bucketDone && holder.Account != "" && holder.Account != acct { |
| + return fmt.Errorf("%w: %s is held by %s/%s; %s/%s's access is revoked and its record kept", ErrBucketRetained, bucket, holder.Account, holder.App, acct, app) |
| + } |
| // The delivered set goes even though the credentials behind it are already |
| // dead: leaving them would hand the app's next render a bucket name and a |
| // key that authenticate against nothing, which is harder to debug than an |
| @@ -696,6 +771,62 @@ func Teardown(ctx context.Context, s release.Store, p Provisioner, sc Scanner, a |
| return deleteRecordIfUnchanged(ctx, s, acct, app, startKeyID, startBucket) |
| } |
| +// grantElapsed is how long a grant has been running. A seam, so a test can |
| +// make a probe slow. |
| +var grantElapsed = func(started time.Time) time.Duration { return time.Since(started) } |
| + |
| +// otherStoreHolders reads every store record in the deployment and returns |
| +// the apps, other than acct/app, whose record names bucket. A record is |
| +// what says an app was given access: it exists from the grant until the |
| +// teardown has finished, ledger or no ledger. |
| +func otherStoreHolders(ctx context.Context, s release.Store, bucket, acct, app string) ([]OwnerRef, error) { |
| + accounts, err := s.ListDirs(ctx, "apps/") |
| + if err != nil { |
| + return nil, err |
| + } |
| + var out []OwnerRef |
| + for _, adir := range accounts { |
| + a := strings.TrimSuffix(strings.TrimPrefix(adir, "apps/"), "/") |
| + if !release.ValidAccount(a) { |
| + continue |
| + } |
| + apps, err := s.ListDirs(ctx, adir) |
| + if err != nil { |
| + return nil, err |
| + } |
| + for _, pdir := range apps { |
| + name := strings.TrimSuffix(strings.TrimPrefix(pdir, adir), "/") |
| + if !release.ValidApp(name) || (a == acct && name == app) { |
| + continue |
| + } |
| + rec, _, err := Get(ctx, s, a, name) |
| + if err != nil { |
| + return nil, err |
| + } |
| + if rec == nil { |
| + continue |
| + } |
| + held := "" |
| + since := time.Time{} |
| + switch { |
| + case rec.Provisioned != nil: |
| + held = rec.Provisioned.Bucket |
| + case rec.Grant != nil: |
| + held = rec.Grant.Bucket |
| + } |
| + if rec.Grant != nil { |
| + since = rec.Grant.At |
| + } |
| + // A store whose own teardown already deleted its bucket holds |
| + // nothing by that name any more. |
| + if held == bucket && (rec.Teardown == nil || rec.Teardown.BucketDeletedAt.IsZero()) { |
| + out = append(out, OwnerRef{Account: a, App: name, Since: since}) |
| + } |
| + } |
| + } |
| + return out, nil |
| +} |
| + |
| // deleteRecordIfUnchanged is the final step of Teardown: delete the record, |
| // but only if it still describes the estate the walk just destroyed. |
| // |