amadan
wip: config-set reconciliation in VerifyDomain (pre-merge)
5 files changed,
+167
−2
internal/mail/aws.go+1 −0internal/mail/memprovisioner.go+10 −1internal/mail/provisioner.go+11 −0internal/mail/walk.go+41 −1internal/mail/walk_verify_test.go+104 −0
diff --git a/internal/mail/aws.go b/internal/mail/aws.go| index 3317c917c..f97c7f9e6 100644 |
| --- a/internal/mail/aws.go |
| +++ b/internal/mail/aws.go |
| @@ -326,6 +326,7 @@ func (p *awsProvisioner) IdentityStatus(ctx context.Context, region, domain stri |
| if got.MailFromAttributes != nil { |
| st.MailFrom = string(got.MailFromAttributes.MailFromDomainStatus) |
| } |
| + st.ConfigSet = aws.ToString(got.ConfigurationSetName) |
| return st, nil |
| } |
diff --git a/internal/mail/memprovisioner.go b/internal/mail/memprovisioner.go| index 8ca357f8d..75bfb1795 100644 |
| --- a/internal/mail/memprovisioner.go |
| +++ b/internal/mail/memprovisioner.go |
| @@ -166,7 +166,16 @@ func (p *MemProvisioner) IdentityStatus(_ context.Context, region, domain string |
| // the same input, would not error at all. |
| return Status{DKIM: "NOT_STARTED", MailFrom: "NOT_STARTED", Sending: "NOT_STARTED"}, nil |
| } |
| - return got.Status, nil |
| + // ConfigSet comes off the identity, not off Status, because that is how |
| + // the real backend answers: GetEmailIdentity returns the binding |
| + // alongside the verification axes, and awsProvisioner reads both from |
| + // the one call. Returning got.Status alone would make the fake report an |
| + // empty binding forever, and a reconciliation test could pass against |
| + // that while production saw something else — the exact divergence the |
| + // NOT_STARTED branch above exists to avoid. |
| + st := got.Status |
| + st.ConfigSet = got.ConfigSet |
| + return st, nil |
| } |
| // SetIdentityStatus is a test seam: drive an identity to verified the way |
diff --git a/internal/mail/provisioner.go b/internal/mail/provisioner.go| index 84e8ebce6..30dfaa5ee 100644 |
| --- a/internal/mail/provisioner.go |
| +++ b/internal/mail/provisioner.go |
| @@ -28,6 +28,17 @@ type Status struct { |
| DKIM string |
| MailFrom string |
| Sending string |
| + // ConfigSet is the configuration set SES currently has BOUND to this |
| + // identity, empty for none. It is read here because |
| + // GetEmailIdentity already returns it and this call already happens on |
| + // every verify — which makes drift detection free (issue #272). |
| + // |
| + // The alternative was reconciling on every status read, which would put |
| + // a GetEmailIdentity per domain on a path that today answers from the |
| + // bucket alone, and would make a working read fail whenever AWS is |
| + // unreachable. Reading it where the call is already being made costs |
| + // nothing and catches the same drift a beat later. |
| + ConfigSet string |
| } |
| // Verified reports whether every axis has converged. |
diff --git a/internal/mail/walk.go b/internal/mail/walk.go| index b2b44723d..10d007ee4 100644 |
| --- a/internal/mail/walk.go |
| +++ b/internal/mail/walk.go |
| @@ -294,7 +294,7 @@ func ConfigSetName(acct string) string { return "carlos-" + acct } |
| // The status is recomputed from scratch each call rather than latched, so a |
| // domain that SES later reports FAILED leaves verified, and one that |
| // recovers leaves failed. |
| -func VerifyDomain(ctx context.Context, s release.Store, p Provisioner, |
| +func VerifyDomain(ctx context.Context, s release.Store, p Provisioner, d Deployment, |
| acct, domain, region string, now time.Time) (*DomainRecord, error) { |
| rec, etag, err := GetDomain(ctx, s, acct, domain) |
| @@ -326,9 +326,49 @@ func VerifyDomain(ctx context.Context, s release.Store, p Provisioner, |
| } |
| st.CheckedAt = now |
| + // RECONCILE THE CONFIGURATION SET (#272). Neither the record nor SES is |
| + // the source of truth here — deployment INTENT is: a deployment with an |
| + // SNS topic wants every identity bound to its account's set, and one |
| + // without wants none bound. Both stores are projections of that, and |
| + // either can drift from it. |
| + // |
| + // Done here rather than on every status read because IdentityStatus has |
| + // already asked SES about this identity: the drift check costs nothing |
| + // extra. Reconciling on read would put an API call per domain on a path |
| + // that answers from the bucket alone today, and would turn a working |
| + // read into a failure whenever AWS is unreachable. |
| + // |
| + // The unbind direction is not symmetry for its own sake: a deployment |
| + // that turns event ingest off, or whose set was deleted, leaves |
| + // identities pointing at a destination that no longer exists, and a |
| + // send through a missing configuration set fails outright. |
| + want := "" |
| + if d.SNSTopicARN != "" { |
| + want = ConfigSetName(acct) |
| + } |
| + rebound := false |
| + if got.ConfigSet != want { |
| + if err := p.SetIdentityConfigSet(ctx, region, domain, want); err != nil { |
| + return nil, fmt.Errorf("mail: rebind identity %s in %s: %w", domain, region, err) |
| + } |
| + rebound = true |
| + } |
| + st.ConfigSet = want |
| + |
| if _, err := PutDomain(ctx, s, acct, rec, etag); err != nil { |
| return nil, err |
| } |
| + |
| + // Grant after rebind, for #279's reason seen from the other side: a |
| + // repair that fixes SES and the record but leaves the IAM policy naming |
| + // no configuration set converts silent-no-events into a 554 on every |
| + // send — strictly worse than the drift it repaired. Only on an actual |
| + // rebind, so the ordinary verify stays the cheap read it was. |
| + if rebound { |
| + if err := RefreshPolicies(ctx, s, p, acct, rec.Apps); err != nil { |
| + return nil, err |
| + } |
| + } |
| return rec, nil |
| } |
diff --git a/internal/mail/walk_verify_test.go b/internal/mail/walk_verify_test.go| index df9ec54af..268fdb03a 100644 |
| --- a/internal/mail/walk_verify_test.go |
| +++ b/internal/mail/walk_verify_test.go |
| @@ -127,3 +127,107 @@ func TestVerifyRefusesAnUnknownRegion(t *testing.T) { |
| t.Fatal("verifying a region the domain has no identity in returned success") |
| } |
| } |
| + |
| +// ---- config-set reconciliation (issue #272, ruling 1) ---------------------- |
| + |
| +// TestVerifyDomainRebindsADriftedConfigSet: the record and SES can disagree, |
| +// and when they do the tooling goes quiet about a domain that emits nothing. |
| +// Reconciliation lands HERE because IdentityStatus already asks SES about |
| +// this identity on every verify — the drift check is free, where doing it on |
| +// every status read would put an API call on a path that answers from the |
| +// bucket alone and would fail whenever AWS is unreachable. |
| +func TestVerifyDomainRebindsADriftedConfigSet(t *testing.T) { |
| + ctx, s, p, _, now := enableFixture(t) |
| + d := testDeployment() |
| + |
| + // Something unbinds the set out of band — exactly what an operator |
| + // reaching for the AWS console does. |
| + if err := p.SetIdentityConfigSet(ctx, d.Region, "seapointish.bag.oncarlos.com", ""); err != nil { |
| + t.Fatalf("unbind: %v", err) |
| + } |
| + got, err := p.IdentityStatus(ctx, d.Region, "seapointish.bag.oncarlos.com") |
| + if err != nil || got.ConfigSet != "" { |
| + t.Fatalf("precondition: identity still reports config set %q (err %v)", got.ConfigSet, err) |
| + } |
| + |
| + if _, err := VerifyDomain(ctx, s, p, d, "bag", "seapointish.bag.oncarlos.com", d.Region, now); err != nil { |
| + t.Fatalf("VerifyDomain: %v", err) |
| + } |
| + |
| + got, err = p.IdentityStatus(ctx, d.Region, "seapointish.bag.oncarlos.com") |
| + if err != nil { |
| + t.Fatalf("IdentityStatus: %v", err) |
| + } |
| + if want := ConfigSetName("bag"); got.ConfigSet != want { |
| + t.Errorf("config set after verify = %q, want %q — verify saw the drift and left it", |
| + got.ConfigSet, want) |
| + } |
| +} |
| + |
| +// TestVerifyDomainUnbindsWhenTheDeploymentHasNoTopic is the reverse |
| +// direction, and it matters as much: a deployment that turns event ingest |
| +// OFF must not leave identities bound to a configuration set whose |
| +// destination no longer exists, because a send through a deleted set fails. |
| +func TestVerifyDomainUnbindsWhenTheDeploymentHasNoTopic(t *testing.T) { |
| + ctx, s, p, _, now := enableFixture(t) |
| + withSNS := testDeployment() |
| + noSNS := Deployment{Region: withSNS.Region, PlatformSuffix: withSNS.PlatformSuffix} |
| + |
| + if got, _ := p.IdentityStatus(ctx, withSNS.Region, "seapointish.bag.oncarlos.com"); got.ConfigSet == "" { |
| + t.Fatal("precondition: the fixture never bound a config set") |
| + } |
| + if _, err := VerifyDomain(ctx, s, p, noSNS, "bag", "seapointish.bag.oncarlos.com", noSNS.Region, now); err != nil { |
| + t.Fatalf("VerifyDomain: %v", err) |
| + } |
| + got, err := p.IdentityStatus(ctx, noSNS.Region, "seapointish.bag.oncarlos.com") |
| + if err != nil { |
| + t.Fatalf("IdentityStatus: %v", err) |
| + } |
| + if got.ConfigSet != "" { |
| + t.Errorf("config set = %q, want empty — a deployment with no topic must not leave identities bound", got.ConfigSet) |
| + } |
| +} |
| + |
| +// TestVerifyDomainRefreshesPoliciesWhenItRebinds is the wiring half. A |
| +// repair that fixes SES and the record but not the IAM policy converts |
| +// silent-no-events into a 554 on every send — strictly worse than the drift |
| +// it repaired. Same lesson as #279, arriving through the other door. |
| +func TestVerifyDomainRefreshesPoliciesWhenItRebinds(t *testing.T) { |
| + ctx, s, p, pub, now := enableFixture(t) |
| + d := testDeployment() |
| + if _, err := Enable(ctx, s, p, pub, d, "bag", "seapointish", now); err != nil { |
| + t.Fatalf("Enable: %v", err) |
| + } |
| + // Strip the config-set ARN back out of the policy, as it would be on an |
| + // app enabled before the set existed. |
| + dom, _, err := GetDomain(ctx, s, "bag", "seapointish.bag.oncarlos.com") |
| + if err != nil { |
| + t.Fatalf("GetDomain: %v", err) |
| + } |
| + if _, err := p.EnsureUser(ctx, UserName("bag", "seapointish"), UserPath, |
| + dom.VerifiedARNs(), nil, []string{"seapointish.bag.oncarlos.com"}); err != nil { |
| + t.Fatalf("EnsureUser: %v", err) |
| + } |
| + if err := p.SetIdentityConfigSet(ctx, d.Region, "seapointish.bag.oncarlos.com", ""); err != nil { |
| + t.Fatalf("unbind: %v", err) |
| + } |
| + |
| + if _, err := VerifyDomain(ctx, s, p, d, "bag", "seapointish.bag.oncarlos.com", d.Region, now); err != nil { |
| + t.Fatalf("VerifyDomain: %v", err) |
| + } |
| + |
| + dom, _, err = GetDomain(ctx, s, "bag", "seapointish.bag.oncarlos.com") |
| + if err != nil { |
| + t.Fatalf("GetDomain: %v", err) |
| + } |
| + want := dom.VerifiedConfigSetARNs() |
| + if len(want) == 0 { |
| + t.Fatal("nothing was bound, so this test proves nothing") |
| + } |
| + policy := p.Users[UserName("bag", "seapointish")].Policy |
| + for _, arn := range want { |
| + if !strings.Contains(policy, arn) { |
| + t.Errorf("policy does not grant %s after the rebind — repairing the binding without the policy is a 554\npolicy = %s", arn, policy) |
| + } |
| + } |
| +} |