amadan

carlos / platform Public

mail: harden the domain walk against the review's six catches

Validate acct and app before ANY backend call (acct was reaching
EnsureTenant unchecked; app was never checked at all, yet gets persisted
into IAM principal names by later walks). Stop recording a ConfigSet name
for a set that was never created when no SNS topic is configured — a
phantom set name would fail every send with "Configuration set does not
exist" instead of degrading gracefully. Document that StatusFailed is
deliberately NOT preserved across a re-run, only StatusVerified is.

Strengthen the test suite to actually pin the behaviour the code claims:
assert a failed publish leaves no persisted record (the previous assertion
passed even with publish-then-record inverted); add a dedicated test that
drives a region to verified and confirms a re-run doesn't knock it back
(the preservation branch was previously untested); assert the second-app
test doesn't just count apps but confirms the identity was not re-created;
add a failAfterN test-only Writer wrapper to exercise a partial publish
(3 written, then a crash), which MemWriter's one-shot FailNext could not
reach.

Both regressions were verified by deliberate mutation before this commit:
inverting the record/publish order failed the new persisted-record
assertion, and deleting the preservation branch failed the new verified-
status test. Both mutations were reverted before committing.
Paul Campbell e84f0c582abf08202d42052f485e4f2df16ab4d3 parent c293ff1
2 files changed, +165 −3
  • internal/mail/walk.go +15 −2
  • internal/mail/walk_domain_test.go +150 −1
diff --git a/internal/mail/walk.go b/internal/mail/walk.go
index 315982eee..f1635a93b 100644
--- a/internal/mail/walk.go
+++ b/internal/mail/walk.go
@@ -72,6 +72,12 @@ func DomainKindFor(d Deployment, domain string) string {
func EnsureDomain(ctx context.Context, s release.Store, p Provisioner, w dnsrec.Writer,
d Deployment, acct, app, domain string, now time.Time) (*DomainRecord, []dnsrec.Record, error) {
+ if !release.ValidAccount(acct) {
+ return nil, nil, fmt.Errorf("mail: invalid account %q", acct)
+ }
+ if !release.ValidApp(app) {
+ return nil, nil, fmt.Errorf("mail: invalid app %q", app)
+ }
if !ValidDomain(domain) {
return nil, nil, fmt.Errorf("mail: invalid domain %q", domain)
}
@@ -100,8 +106,13 @@ func EnsureDomain(ctx context.Context, s release.Store, p Provisioner, w dnsrec.
return nil, nil, fmt.Errorf("mail: ensure identity %s in %s: %w", domain, d.Region, err)
}
- configSet := ConfigSetName(acct)
+ // configSet stays empty when no SNS topic is configured — recording a
+ // name for a set that was never created would leave later sends
+ // failing with "Configuration set does not exist" instead of the
+ // degraded-but-working deployment the Deployment doc comment promises.
+ configSet := ""
if d.SNSTopicARN != "" {
+ configSet = ConfigSetName(acct)
if err := p.EnsureConfigSet(ctx, d.Region, configSet, d.SNSTopicARN); err != nil {
return nil, nil, fmt.Errorf("mail: ensure config set %s: %w", configSet, err)
}
@@ -123,7 +134,9 @@ func EnsureDomain(ctx context.Context, s release.Store, p Provisioner, w dnsrec.
}
// Preserve a status already reached: a verified region must not be
- // knocked back to pending by a re-run.
+ // knocked back to pending by a re-run. StatusFailed is NOT preserved —
+ // a re-run is a fresh attempt, so a previously failed region goes back
+ // to pending rather than staying stuck at failed.
status := StatusPending
if st, ok := rec.Regions[d.Region]; ok && st.Status == StatusVerified {
status = StatusVerified
diff --git a/internal/mail/walk_domain_test.go b/internal/mail/walk_domain_test.go
index 18be12012..013992c94 100644
--- a/internal/mail/walk_domain_test.go
+++ b/internal/mail/walk_domain_test.go
@@ -120,7 +120,10 @@ func TestEnsureDomainAddsASecondAppWithoutRecreating(t *testing.T) {
now := time.Date(2026, 8, 19, 12, 0, 0, 0, time.UTC)
d := testDeployment()
- EnsureDomain(ctx, s, p, w, d, "bag", "seapointish", "shared.com", now)
+ first, _, err := EnsureDomain(ctx, s, p, w, d, "bag", "seapointish", "shared.com", now)
+ if err != nil {
+ t.Fatalf("first app: %v", err)
+ }
rec, _, err := EnsureDomain(ctx, s, p, w, d, "bag", "marketing", "shared.com", now)
if err != nil {
t.Fatalf("second app: %v", err)
@@ -128,6 +131,12 @@ func TestEnsureDomainAddsASecondAppWithoutRecreating(t *testing.T) {
if len(rec.Apps) != 2 {
t.Fatalf("apps = %v, want both", rec.Apps)
}
+ if len(p.Identities) != 1 {
+ t.Fatalf("identities = %d, want 1 — a second app must join, not re-create", len(p.Identities))
+ }
+ if first.Regions["eu-west-1"].IdentityARN != rec.Regions["eu-west-1"].IdentityARN {
+ t.Fatal("identity ARN changed between apps — the identity was re-created")
+ }
}
// A half-failure after the identity exists but before DNS is published must
@@ -142,6 +151,12 @@ func TestEnsureDomainRepairsAHalfFailedPublish(t *testing.T) {
if _, _, err := EnsureDomain(ctx, s, p, w, d, "bag", "seapointish", "seapointish.bag.oncarlos.com", now); err == nil {
t.Fatal("EnsureDomain succeeded despite a DNS write failure")
}
+ // A failed publish must leave NO record — the record must never claim
+ // DNS that was not actually written. This is what pins the ordering:
+ // create-identity, then publish, then record, in that order.
+ if rec, _, err := GetDomain(ctx, s, "bag", "seapointish.bag.oncarlos.com"); err != nil || rec != nil {
+ t.Fatalf("a failed publish left a record claiming DNS that is not there: %+v", rec)
+ }
if _, _, err := EnsureDomain(ctx, s, p, w, d, "bag", "seapointish", "seapointish.bag.oncarlos.com", now); err != nil {
t.Fatalf("re-run did not repair: %v", err)
}
@@ -149,3 +164,137 @@ func TestEnsureDomainRepairsAHalfFailedPublish(t *testing.T) {
t.Fatalf("after repair, published %d records, want 6", len(w.Records))
}
}
+
+// A region already verified must not be knocked back to pending by a
+// re-run — only a freshly-ensured region starts pending. StatusFailed is
+// deliberately NOT preserved this way (a re-run is a fresh attempt), so
+// this test targets verified specifically.
+func TestEnsureDomainPreservesVerifiedStatus(t *testing.T) {
+ ctx := context.Background()
+ s, p, w := release.NewDirStore(t.TempDir()), NewMemProvisioner("1"), dnsrec.NewMemWriter()
+ now := time.Date(2026, 8, 19, 12, 0, 0, 0, time.UTC)
+ d := testDeployment()
+
+ if _, _, err := EnsureDomain(ctx, s, p, w, d, "bag", "seapointish", "seapointish.bag.oncarlos.com", now); err != nil {
+ t.Fatalf("first: %v", err)
+ }
+
+ // Simulate SES having converged: mark the region verified directly on
+ // the persisted record, the way a status-check walk would.
+ got, etag, err := GetDomain(ctx, s, "bag", "seapointish.bag.oncarlos.com")
+ if err != nil {
+ t.Fatalf("GetDomain: %v", err)
+ }
+ got.Regions["eu-west-1"].Status = StatusVerified
+ if _, err := PutDomain(ctx, s, "bag", got, etag); err != nil {
+ t.Fatalf("PutDomain: %v", err)
+ }
+
+ second, _, err := EnsureDomain(ctx, s, p, w, d, "bag", "seapointish", "seapointish.bag.oncarlos.com", now)
+ if err != nil {
+ t.Fatalf("second: %v", err)
+ }
+ if got := second.Regions["eu-west-1"].Status; got != StatusVerified {
+ t.Fatalf("status = %q, want verified — a re-run must not knock back a converged region", got)
+ }
+}
+
+// A deployment with no SNS topic must not record a configuration set name
+// for a set that was never created — a later send against a phantom set
+// fails with "Configuration set does not exist" on every attempt.
+func TestEnsureDomainLeavesConfigSetEmptyWithoutSNS(t *testing.T) {
+ ctx := context.Background()
+ s, p, w := release.NewDirStore(t.TempDir()), NewMemProvisioner("1"), dnsrec.NewMemWriter()
+ now := time.Date(2026, 8, 19, 12, 0, 0, 0, time.UTC)
+ d := testDeployment()
+ d.SNSTopicARN = ""
+
+ rec, _, err := EnsureDomain(ctx, s, p, w, d, "bag", "seapointish", "seapointish.bag.oncarlos.com", now)
+ if err != nil {
+ t.Fatalf("EnsureDomain: %v", err)
+ }
+ if got := rec.Regions["eu-west-1"].ConfigSet; got != "" {
+ t.Fatalf("ConfigSet = %q, want empty — no SNS topic means no config set was created", got)
+ }
+}
+
+// acct reaches EnsureTenant — a real backend call — before anything checks
+// it, unless validation gates the walk before that call. app is persisted
+// into the record and later turned into an IAM principal name by other
+// walks, so it must be validated too.
+func TestEnsureDomainRefusesInvalidAccount(t *testing.T) {
+ ctx := context.Background()
+ s, p, w := release.NewDirStore(t.TempDir()), NewMemProvisioner("1"), dnsrec.NewMemWriter()
+ now := time.Date(2026, 8, 19, 12, 0, 0, 0, time.UTC)
+
+ if _, _, err := EnsureDomain(ctx, s, p, w, testDeployment(), "../evil", "seapointish", "seapointish.bag.oncarlos.com", now); err == nil {
+ t.Fatal("EnsureDomain accepted an invalid account")
+ }
+ if p.Tenants["../evil"] {
+ t.Fatal("an invalid account reached EnsureTenant before validation")
+ }
+}
+
+func TestEnsureDomainRefusesInvalidApp(t *testing.T) {
+ ctx := context.Background()
+ s, p, w := release.NewDirStore(t.TempDir()), NewMemProvisioner("1"), dnsrec.NewMemWriter()
+ now := time.Date(2026, 8, 19, 12, 0, 0, 0, time.UTC)
+
+ if _, _, err := EnsureDomain(ctx, s, p, w, testDeployment(), "bag", "../evil", "seapointish.bag.oncarlos.com", now); err == nil {
+ t.Fatal("EnsureDomain accepted an invalid app")
+ }
+}
+
+// failAfterN wraps a Writer and fails the (n+1)th EnsureRecord, so a
+// PARTIAL publish can be exercised — MemWriter.FailNext is one-shot on
+// the first call, which is the least realistic crash point for this walk
+// (three published, then a crash, is the realistic one). After the one
+// failure it behaves like the wrapped Writer for good, matching how a
+// transient DNS-provider failure would look on a retry.
+type failAfterN struct {
+ dnsrec.Writer
+ n int
+ err error
+ failed bool
+}
+
+func (f *failAfterN) EnsureRecord(ctx context.Context, r dnsrec.Record) error {
+ if !f.failed && f.n <= 0 {
+ f.failed = true
+ return f.err
+ }
+ if !f.failed {
+ f.n--
+ }
+ return f.Writer.EnsureRecord(ctx, r)
+}
+
+// A crash after SOME records are published but before all six is the
+// realistic failure mode this walk must survive. A re-run must converge to
+// the full set with no duplicates, and the half-published attempt must not
+// have persisted a record claiming DNS that is not fully there.
+func TestEnsureDomainRepairsAPartialPublish(t *testing.T) {
+ ctx := context.Background()
+ s, p := release.NewDirStore(t.TempDir()), NewMemProvisioner("1")
+ inner := dnsrec.NewMemWriter()
+ w := &failAfterN{Writer: inner, n: 3, err: errors.New("route53 down mid-publish")}
+ now := time.Date(2026, 8, 19, 12, 0, 0, 0, time.UTC)
+ d := testDeployment()
+
+ if _, _, err := EnsureDomain(ctx, s, p, w, d, "bag", "seapointish", "seapointish.bag.oncarlos.com", now); err == nil {
+ t.Fatal("EnsureDomain succeeded despite a mid-publish failure")
+ }
+ if len(inner.Records) != 3 {
+ t.Fatalf("published %d records before the failure, want 3", len(inner.Records))
+ }
+ if rec, _, err := GetDomain(ctx, s, "bag", "seapointish.bag.oncarlos.com"); err != nil || rec != nil {
+ t.Fatalf("a partial publish left a record claiming DNS that is not fully there: %+v", rec)
+ }
+
+ if _, _, err := EnsureDomain(ctx, s, p, w, d, "bag", "seapointish", "seapointish.bag.oncarlos.com", now); err != nil {
+ t.Fatalf("re-run did not repair: %v", err)
+ }
+ if len(inner.Records) != 6 {
+ t.Fatalf("after repair, published %d records, want 6 with no duplicates", len(inner.Records))
+ }
+}