carlos / platform Public

Merge pull request #282 from carlosframework/mail-configset-reconcile

Verify reconciles the configuration set in both directions, closing #272.

Merged with main brought in — including the open-sourcing redaction pass, whose harbourmail fixture the tests now follow. Gate reproduced locally on the merge result: 36 packages, vet and gofmt clean, GOOS=darwin build.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Paul Campbell pushed by paul@keymail.dev 1 month ago 72ed99ec1f0afaaf952d6450d0785e8cb02f858d parent 0d3cf9dparent 2393da6

7 files changed, +187 −16

internal/console/apimail.go +6 −1
internal/mail/aws.go +1 −0
internal/mail/memprovisioner.go +10 −1
internal/mail/provisioner.go +11 −0
internal/mail/walk.go +41 −1
internal/mail/walk_enable_test.go +5 −5
internal/mail/walk_verify_test.go +113 −8

diff --git a/internal/console/apimail.go b/internal/console/apimail.go

index 2904f745d..136f882e7 100644

--- a/internal/console/apimail.go

+++ b/internal/console/apimail.go

@@ -755,7 +755,12 @@ func (s *Server) handleAPIEmailDomainVerify(w http.ResponseWriter, r *http.Reque

return

}

region := s.cfg.MailDeployment.Region

- rec, err := mail.VerifyDomain(ctx, s.cfg.Releases, s.cfg.MailProvisioner, acct.Sqid, domain, region, time.Now())

+ // The deployment goes in because verify RECONCILES the identity's

+ // configuration set against it (#272): the binding SES should have is a

+ // function of whether this deployment has an SNS topic, not of what the

+ // record last recorded.

+ rec, err := mail.VerifyDomain(ctx, s.cfg.Releases, s.cfg.MailProvisioner,

+ s.cfg.MailDeployment, acct.Sqid, domain, region, time.Now())

if err != nil {

log.Printf("console: api email domain verify %s/%s: %v", acct.Sqid, domain, err)

jsonError(w, http.StatusBadGateway, "could not check verification — try again")

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 cf2ada639..d36e58c9d 100644

--- a/internal/mail/walk.go

+++ b/internal/mail/walk.go

@@ -387,7 +387,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)

@@ -419,9 +419,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_enable_test.go b/internal/mail/walk_enable_test.go

index 45561feb5..0abe6a713 100644

--- a/internal/mail/walk_enable_test.go

+++ b/internal/mail/walk_enable_test.go

@@ -39,7 +39,7 @@ func enableFixture(t *testing.T) (context.Context, release.Store, *MemProvisione

}

p.SetIdentityStatus("eu-west-1", "harbourmail.bag.oncarlos.com",

Status{DKIM: "SUCCESS", MailFrom: "SUCCESS", Sending: "SUCCESS"})

- if _, err := VerifyDomain(ctx, s, p, "bag", "harbourmail.bag.oncarlos.com", "eu-west-1", now); err != nil {

+ if _, err := VerifyDomain(ctx, s, p, testDeployment(), "bag", "harbourmail.bag.oncarlos.com", "eu-west-1", now); err != nil {

t.Fatalf("VerifyDomain: %v", err)

}

return ctx, s, p, pub, now

@@ -185,7 +185,7 @@ func TestEnsureDomainRefreshesPoliciesWhenItBindsAConfigSet(t *testing.T) {

}

p.SetIdentityStatus("eu-west-1", "seapointish.bag.oncarlos.com",

Status{DKIM: "SUCCESS", MailFrom: "SUCCESS", Sending: "SUCCESS"})

- if _, err := VerifyDomain(ctx, s, p, "bag", "seapointish.bag.oncarlos.com", "eu-west-1", now); err != nil {

+ if _, err := VerifyDomain(ctx, s, p, testDeployment(), "bag", "seapointish.bag.oncarlos.com", "eu-west-1", now); err != nil {

t.Fatalf("VerifyDomain: %v", err)

}

if _, err := Enable(ctx, s, p, pub, noSNS, "bag", "seapointish", now); err != nil {

@@ -453,7 +453,7 @@ func TestEnableRewritesThePolicyWhenASecondDomainIsAdded(t *testing.T) {

t.Fatalf("EnsureDomain second: %v", err)

}

p.SetIdentityStatus("eu-west-1", second, Status{DKIM: "SUCCESS", MailFrom: "SUCCESS", Sending: "SUCCESS"})

- if _, err := VerifyDomain(ctx, s, p, "bag", second, "eu-west-1", now); err != nil {

+ if _, err := VerifyDomain(ctx, s, p, testDeployment(), "bag", second, "eu-west-1", now); err != nil {

t.Fatalf("VerifyDomain second: %v", err)

}

@@ -487,7 +487,7 @@ func TestEnableRewritesThePolicyWhenADomainStopsVerifying(t *testing.T) {

t.Fatalf("EnsureDomain second: %v", err)

}

p.SetIdentityStatus("eu-west-1", second, Status{DKIM: "SUCCESS", MailFrom: "SUCCESS", Sending: "SUCCESS"})

- if _, err := VerifyDomain(ctx, s, p, "bag", second, "eu-west-1", now); err != nil {

+ if _, err := VerifyDomain(ctx, s, p, testDeployment(), "bag", second, "eu-west-1", now); err != nil {

t.Fatalf("VerifyDomain second: %v", err)

}

@@ -502,7 +502,7 @@ func TestEnableRewritesThePolicyWhenADomainStopsVerifying(t *testing.T) {

// second regresses: SES now reports it FAILED.

p.SetIdentityStatus("eu-west-1", second, Status{DKIM: "FAILED", MailFrom: "FAILED", Sending: "PENDING"})

- if _, err := VerifyDomain(ctx, s, p, "bag", second, "eu-west-1", now); err != nil {

+ if _, err := VerifyDomain(ctx, s, p, testDeployment(), "bag", second, "eu-west-1", now); err != nil {

t.Fatalf("VerifyDomain regression: %v", err)

}

diff --git a/internal/mail/walk_verify_test.go b/internal/mail/walk_verify_test.go

index 5d2092b8a..15a9d7fac 100644

--- a/internal/mail/walk_verify_test.go

+++ b/internal/mail/walk_verify_test.go

@@ -2,6 +2,7 @@ package mail

import (

"context"

+ "strings"

"testing"

"time"

@@ -29,7 +30,7 @@ func TestVerifyRequiresEveryAxis(t *testing.T) {

p.SetIdentityStatus("eu-west-1", "harbourmail.bag.oncarlos.com",

Status{DKIM: "SUCCESS", MailFrom: "PENDING", Sending: "SUCCESS"})

- rec, err := VerifyDomain(ctx, s, p, "bag", "harbourmail.bag.oncarlos.com", "eu-west-1", now)

+ rec, err := VerifyDomain(ctx, s, p, testDeployment(), "bag", "harbourmail.bag.oncarlos.com", "eu-west-1", now)

if err != nil {

t.Fatalf("VerifyDomain: %v", err)

}

@@ -39,7 +40,7 @@ func TestVerifyRequiresEveryAxis(t *testing.T) {

p.SetIdentityStatus("eu-west-1", "harbourmail.bag.oncarlos.com",

Status{DKIM: "SUCCESS", MailFrom: "SUCCESS", Sending: "SUCCESS"})

- rec, err = VerifyDomain(ctx, s, p, "bag", "harbourmail.bag.oncarlos.com", "eu-west-1", now)

+ rec, err = VerifyDomain(ctx, s, p, testDeployment(), "bag", "harbourmail.bag.oncarlos.com", "eu-west-1", now)

if err != nil {

t.Fatalf("VerifyDomain: %v", err)

}

@@ -57,7 +58,7 @@ func TestVerifyRecordsAPermanentFailure(t *testing.T) {

p.SetIdentityStatus("eu-west-1", "harbourmail.bag.oncarlos.com",

Status{DKIM: "FAILED", MailFrom: "PENDING", Sending: "PENDING"})

- rec, err := VerifyDomain(ctx, s, p, "bag", "harbourmail.bag.oncarlos.com", "eu-west-1", now)

+ rec, err := VerifyDomain(ctx, s, p, testDeployment(), "bag", "harbourmail.bag.oncarlos.com", "eu-west-1", now)

if err != nil {

t.Fatalf("VerifyDomain: %v", err)

}

@@ -75,10 +76,10 @@ func TestVerifyCanRecoverFromFailed(t *testing.T) {

p.SetIdentityStatus("eu-west-1", "harbourmail.bag.oncarlos.com",

Status{DKIM: "FAILED", MailFrom: "PENDING", Sending: "PENDING"})

- VerifyDomain(ctx, s, p, "bag", "harbourmail.bag.oncarlos.com", "eu-west-1", now)

+ VerifyDomain(ctx, s, p, testDeployment(), "bag", "harbourmail.bag.oncarlos.com", "eu-west-1", now)

p.SetIdentityStatus("eu-west-1", "harbourmail.bag.oncarlos.com",

Status{DKIM: "SUCCESS", MailFrom: "SUCCESS", Sending: "SUCCESS"})

- rec, err := VerifyDomain(ctx, s, p, "bag", "harbourmail.bag.oncarlos.com", "eu-west-1", now)

+ rec, err := VerifyDomain(ctx, s, p, testDeployment(), "bag", "harbourmail.bag.oncarlos.com", "eu-west-1", now)

if err != nil {

t.Fatalf("VerifyDomain: %v", err)

}

@@ -100,7 +101,7 @@ func TestVerifyLeavesVerifiedWhenSESRegresses(t *testing.T) {

p.SetIdentityStatus("eu-west-1", "harbourmail.bag.oncarlos.com",

Status{DKIM: "SUCCESS", MailFrom: "SUCCESS", Sending: "SUCCESS"})

- rec, err := VerifyDomain(ctx, s, p, "bag", "harbourmail.bag.oncarlos.com", "eu-west-1", now)

+ rec, err := VerifyDomain(ctx, s, p, testDeployment(), "bag", "harbourmail.bag.oncarlos.com", "eu-west-1", now)

if err != nil {

t.Fatalf("VerifyDomain: %v", err)

}

@@ -110,7 +111,7 @@ func TestVerifyLeavesVerifiedWhenSESRegresses(t *testing.T) {

p.SetIdentityStatus("eu-west-1", "harbourmail.bag.oncarlos.com",

Status{DKIM: "FAILED", MailFrom: "SUCCESS", Sending: "SUCCESS"})

- rec, err = VerifyDomain(ctx, s, p, "bag", "harbourmail.bag.oncarlos.com", "eu-west-1", now)

+ rec, err = VerifyDomain(ctx, s, p, testDeployment(), "bag", "harbourmail.bag.oncarlos.com", "eu-west-1", now)

if err != nil {

t.Fatalf("VerifyDomain: %v", err)

}

@@ -123,7 +124,111 @@ func TestVerifyRefusesAnUnknownRegion(t *testing.T) {

ctx := context.Background()

s, p := release.NewDirStore(t.TempDir()), NewMemProvisioner("1")

now := seedDomain(t, s, p)

- if _, err := VerifyDomain(ctx, s, p, "bag", "harbourmail.bag.oncarlos.com", "ap-south-1", now); err == nil {

+ if _, err := VerifyDomain(ctx, s, p, testDeployment(), "bag", "harbourmail.bag.oncarlos.com", "ap-south-1", now); err == nil {

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, "harbourmail.bag.oncarlos.com", ""); err != nil {

+ t.Fatalf("unbind: %v", err)

+ }

+ got, err := p.IdentityStatus(ctx, d.Region, "harbourmail.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", "harbourmail.bag.oncarlos.com", d.Region, now); err != nil {

+ t.Fatalf("VerifyDomain: %v", err)

+ }

+ got, err = p.IdentityStatus(ctx, d.Region, "harbourmail.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, "harbourmail.bag.oncarlos.com"); got.ConfigSet == "" {

+ t.Fatal("precondition: the fixture never bound a config set")

+ }

+ if _, err := VerifyDomain(ctx, s, p, noSNS, "bag", "harbourmail.bag.oncarlos.com", noSNS.Region, now); err != nil {

+ t.Fatalf("VerifyDomain: %v", err)

+ }

+ got, err := p.IdentityStatus(ctx, noSNS.Region, "harbourmail.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", "harbourmail", 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", "harbourmail.bag.oncarlos.com")

+ if err != nil {

+ t.Fatalf("GetDomain: %v", err)

+ }

+ if _, err := p.EnsureUser(ctx, UserName("bag", "harbourmail"), UserPath,

+ dom.VerifiedARNs(), nil, []string{"harbourmail.bag.oncarlos.com"}); err != nil {

+ t.Fatalf("EnsureUser: %v", err)

+ }

+ if err := p.SetIdentityConfigSet(ctx, d.Region, "harbourmail.bag.oncarlos.com", ""); err != nil {

+ t.Fatalf("unbind: %v", err)

+ }

+ if _, err := VerifyDomain(ctx, s, p, d, "bag", "harbourmail.bag.oncarlos.com", d.Region, now); err != nil {

+ t.Fatalf("VerifyDomain: %v", err)

+ }

+ dom, _, err = GetDomain(ctx, s, "bag", "harbourmail.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", "harbourmail")].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)

+ }