amadan

carlos / platform Public

mail: a failed tenant disassociate no longer strands teardown (final review, item 4)

Teardown's DisassociateTenantResource error was a hard `return err`,
before DeleteIdentity and before releaseDomainClaim. If the
control-plane role lacks ses:DeleteTenantResourceAssociation (spec
§4.2), AccessDenied is neither AlreadyExists nor NotFound, so it was
never swallowed: every teardown where the app was a domain's last
referrer failed, and the domain's global claim was stranded, denying
that domain to every account including its own owner, until the grant
landed. This bit hardest for domains that predate this release and
were never actually associated (spec §4.2's first prerequisite), where
the disassociate call had nothing real to undo anyway.

Log and continue past a failed disassociate. DeleteIdentity's own
"still associated" refusal stays the hard failure, keeping the real
protection SES itself provides (confirmed against the live API, spec
§4.1) while removing the wedge. Two new tests pin both halves: a
failed disassociate does not stop teardown reaching DeleteIdentity, and
DeleteIdentity still refuses when the identity really is associated.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Paul Campbell pushed by claude 4b38e2dca71c51b0fe472bdf6e6d36d2c12a5ce1 parent 1ab138a
2 files changed, +92 −2
  • internal/mail/walk.go +19 −2
  • internal/mail/walk_lifecycle_test.go +73 −0
diff --git a/internal/mail/walk.go b/internal/mail/walk.go
index b250b831d..60be16b96 100644
--- a/internal/mail/walk.go
+++ b/internal/mail/walk.go
@@ -8,6 +8,7 @@ import (
"crypto/ecdh"
"errors"
"fmt"
+ "log"
"regexp"
"sort"
"strings"
@@ -1281,12 +1282,28 @@ func Teardown(ctx context.Context, s release.Store, p Provisioner, w dnsrec.Writ
}
}
// SES refuses to delete an identity while a tenant association
- // exists, so the association goes first. The account's
+ // exists, so the association is attempted first. The account's
// configuration set is NOT disassociated: it is per account and
// outlives any one app.
+ //
+ // A failed disassociate is logged and NOT a hard error (I2,
+ // review follow-up, reversed from this task's first cut). The
+ // control-plane role needs ses:DeleteTenantResourceAssociation
+ // (spec §4.2) and that grant does not live in this repo; if it
+ // is missing, AccessDenied is neither AlreadyExists nor
+ // NotFound, so a `return err` here would fail EVERY teardown
+ // where this app is the last referrer of a domain, and — worse
+ // — leave releaseDomainClaim and s.Delete below unreached, which
+ // strands the domain's global claim: nobody, including this
+ // domain's own owner, could ever claim it again until the grant
+ // landed. DeleteIdentity is the real protection SES itself
+ // provides (confirmed against the live API, spec §4.1: it
+ // refuses to delete an identity that still has a tenant
+ // association), so it stays a hard failure below and does the
+ // job this disassociate call cannot be trusted to gate.
if st.IdentityARN != "" {
if err := p.DisassociateTenantResource(ctx, TenantName(acct), st.IdentityARN); err != nil {
- return err
+ log.Printf("mail: teardown: disassociating tenant resource for %s/%s: %v", acct, cur.Domain, err)
}
}
if err := p.DeleteIdentity(ctx, region, cur.Domain); err != nil {
diff --git a/internal/mail/walk_lifecycle_test.go b/internal/mail/walk_lifecycle_test.go
index cce966e1f..dca7cb085 100644
--- a/internal/mail/walk_lifecycle_test.go
+++ b/internal/mail/walk_lifecycle_test.go
@@ -5,6 +5,7 @@ import (
"encoding/json"
"errors"
"os"
+ "strings"
"testing"
"time"
@@ -540,6 +541,78 @@ func TestTeardownRemovesTheIdentityWhenTheLastAppGoes(t *testing.T) {
}
}
+// I2 (review follow-up): a hard `return err` from DisassociateTenantResource
+// used to run before DeleteIdentity and before releaseDomainClaim. If the
+// control-plane role lacks ses:DeleteTenantResourceAssociation (spec §4.2),
+// AccessDenied is neither AlreadyExists nor NotFound, so it was never
+// swallowed — and that failed every teardown where the app was the last
+// referrer of a domain, stranding the domain's global claim, even for a
+// domain that predates this release and was never actually associated
+// (spec §4.2's first prerequisite). This test pins the fix: a failed
+// disassociate is logged, not fatal, and Teardown still reaches
+// DeleteIdentity and finishes when nothing is really associated.
+func TestTeardownReachesDeleteIdentityDespiteAFailedDisassociate(t *testing.T) {
+ ctx, s, p, pub, now := enableFixture(t)
+ if _, err := Enable(ctx, s, p, pub, testDeployment(), "bag", "harbourmail", now); err != nil {
+ t.Fatalf("Enable: %v", err)
+ }
+ w := dnsrec.NewMemWriter()
+ if _, _, err := EnsureDomain(ctx, s, p, w, testDeployment(),
+ "bag", "harbourmail", "harbourmail.bag.oncarlos.com", now); err != nil {
+ t.Fatalf("re-publish into the inspectable writer: %v", err)
+ }
+
+ // Stand in for a domain that predates this release: verified, but with
+ // no real tenant association, because nothing before this release ever
+ // created one. Clearing it after EnsureDomain (which did associate it,
+ // same as it would for any domain added today) reaches that state
+ // without a second fixture.
+ ident := p.Identities[identityKey("eu-west-1", "harbourmail.bag.oncarlos.com")]
+ delete(p.TenantResources[TenantName("bag")], ident.ARN)
+
+ p.FailNext["DisassociateTenantResource"] = errors.New("AccessDenied: ses:DeleteTenantResourceAssociation")
+
+ if err := Teardown(ctx, s, p, w, testDeployment(), "bag", "harbourmail"); err != nil {
+ t.Fatalf("Teardown: %v, want the failed disassociate logged and teardown to still finish", err)
+ }
+ if _, ok := p.Identities[identityKey("eu-west-1", "harbourmail.bag.oncarlos.com")]; ok {
+ t.Fatal("teardown left the identity behind after a failed but inconsequential disassociate")
+ }
+}
+
+// The real protection stays: when the identity IS still associated, a
+// failed disassociate must not let DeleteIdentity through anyway — SES
+// itself refuses that delete for the same reason (confirmed against the
+// live API, spec §4.1), and this is what makes that refusal the hard
+// failure instead of the disassociate call.
+func TestTeardownStillRefusesADeleteWhileReallyAssociated(t *testing.T) {
+ ctx, s, p, pub, now := enableFixture(t)
+ if _, err := Enable(ctx, s, p, pub, testDeployment(), "bag", "harbourmail", now); err != nil {
+ t.Fatalf("Enable: %v", err)
+ }
+ w := dnsrec.NewMemWriter()
+ if _, _, err := EnsureDomain(ctx, s, p, w, testDeployment(),
+ "bag", "harbourmail", "harbourmail.bag.oncarlos.com", now); err != nil {
+ t.Fatalf("re-publish into the inspectable writer: %v", err)
+ }
+
+ // Left associated on purpose: EnsureDomain associated it above and
+ // nothing has removed it, so this is the case DeleteIdentity's own
+ // guard exists for.
+ p.FailNext["DisassociateTenantResource"] = errors.New("AccessDenied: ses:DeleteTenantResourceAssociation")
+
+ err := Teardown(ctx, s, p, w, testDeployment(), "bag", "harbourmail")
+ if err == nil {
+ t.Fatal("Teardown succeeded despite the identity still being associated — the real protection is gone")
+ }
+ if !strings.Contains(err.Error(), "still associated") {
+ t.Fatalf("Teardown error = %v, want DeleteIdentity's own \"still associated\" refusal", err)
+ }
+ if _, ok := p.Identities[identityKey("eu-west-1", "harbourmail.bag.oncarlos.com")]; !ok {
+ t.Fatal("the identity was deleted despite still being associated")
+ }
+}
+
// Teardown must delete BOTH objects, not just the current one. Every other
// Teardown fixture builds its record through Enable/Declare, which write
// only senders/app.json, so a fixture like those would pass whether or not