Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MAIL FROM and DMARC DNS updates #168

Closed
wants to merge 3 commits into from
Closed

Conversation

coilysiren
Copy link
Contributor

@coilysiren coilysiren commented Jan 28, 2025

Changes

  • Fixes the DNS record for the MAIL FROM domain
  • Adds the DNS record for the DMARC domain

Context for reviewers

Work originates from https://nava.slack.com/archives/C03G1SWD9H7/p1738089826660819

Testing

image image image image image

Preview environment for app

♻️ Environment destroyed ♻️

Preview environment for app-rails

♻️ Environment destroyed ♻️

Copy link
Collaborator

@lorenyu lorenyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming testing looks good the code looks good to me. Had one non-blocking idea/suggestion if you think it's a good idea go for it otherwise not required

Comment on lines 30 to 37
resource "aws_route53_record" "dmarc" {
allow_overwrite = true
ttl = "600"
type = "TXT"
zone_id = var.hosted_zone_id
records = ["10 feedback-smtp.${data.aws_region.current.name}.amazonaws.com"]
name = aws_sesv2_email_identity_mail_from_attributes.sender_domain.mail_from_domain
records = ["v=DMARC1; p=none;"]
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking style suggestion. For email I think dns.tf doesn't really say what it's doing, more how it's doing it. What do you think about instead separating into 3 files:

  • mail_from.tf
  • dkim.tf
  • dmarc.tf

mail_from.tf will have the resources:

  • local mail_from variable
  • aws_sesv2_email_identity_mail_from_attributes.sender_domain
  • aws_route53_record.mx_receive
  • aws_route53_record.spf_mail_from

dkim.tf would have:

  • aws_route53_record.dkim

and dmarc.tf would have:

  • aws_route53_record.dmarc

thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While that's probably correct from the point of view of the greater organization of the project, I think it's important to keep the UX in mind here. Splitting up the DNS into 3 files will probably confuse people more often than not. Especially if they don't have strong working knowledge of have email verification works (I certainly don't!)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's reasonable

infra/modules/notifications-email-domain/resources/dns.tf Outdated Show resolved Hide resolved
@lorenyu
Copy link
Collaborator

lorenyu commented Jan 28, 2025

@coilysiren for testing go ahead and apply the changes on the dev environment and look in the console to see if the MAIL FROM shows up as "SUCCESS" rather than pending or failed, and upload a screenshot of that to the PR description

@lorenyu
Copy link
Collaborator

lorenyu commented Jan 28, 2025

@coilysiren oh even better, use the test notifications page and send an email and show that the email was sent from mail.platform-test-dev.navateam.com

Co-authored-by: Loren Yu <loren@navapbc.com>
coilysiren added a commit to navapbc/template-infra that referenced this pull request Jan 28, 2025
## Changes

- Fixes the DNS record for the MAIL FROM domain
- Adds the DNS record for the DMARC domain

## Context for reviewers

Work originates from
https://nava.slack.com/archives/C03G1SWD9H7/p1738089826660819

## Testing

See: navapbc/platform-test#168
@lorenyu
Copy link
Collaborator

lorenyu commented Jan 31, 2025

Should we close this

@coilysiren
Copy link
Contributor Author

Done => navapbc/template-infra#859

@coilysiren coilysiren closed this Jan 31, 2025
@lorenyu lorenyu deleted the kai/mail-from-dmarc branch January 31, 2025 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants