-
Notifications
You must be signed in to change notification settings - Fork 3
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
Add ADR for notifications #830
base: main
Are you sure you want to change the base?
Conversation
- **Pros**: | ||
- Simplifies the developer experience by allowing reuse of the custom app domain already being set up. | ||
- **Cons**: | ||
- Requires [BYODKIM (Bring Your Own DKIM)](https://docs.aws.amazon.com/ses/latest/dg/send-email-authentication-dkim-bring-your-own.html), which involves generating public/private key pairs out of band, adding complexity to maintain key pairs securely since the Terraform [tls_private_key](https://registry.terraform.io/providers/hashicorp/tls/latest/docs/resources/private_key) resource is not recommended for production. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add why it requires BYODKIM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In trying to explain this, I think this is actually false. I know we discussed this a while in Slack but I figured rather than have another long discussion about it I'd just try it, and it seems like it works just fine:
I just created a domain identity via the console for the domain subdomain.test.platform-test-dev.navateam.com using easy DKIM:
- **Pros**: | ||
- Leverages Amazon’s Easy DKIM, simplifying setup by automatically managing public/private keys. | ||
- **Cons**: | ||
- In multi-app repositories, applications that share environments (dev, staging, prod) and hosted zones cannot create their own domain email identity since it would create a naming conflict. Therefore, enabling notifications for a second app requires custom work to allow sharing of the notification resources from the first app. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- In multi-app repositories, applications that share environments (dev, staging, prod) and hosted zones cannot create their own domain email identity since it would create a naming conflict. Therefore, enabling notifications for a second app requires custom work to allow sharing of the notification resources from the first app. | |
- In multi-app repositories, whenever there are multiple applications in an environment (dev, staging, prod) or multiple per hosted zone, they cannot each create their own domain email identity since it would create a naming conflict. Therefore, enabling notifications for a second app requires custom work to allow sharing of the notification resources from the first app. |
|
||
### Consideration 2: Where should notifications resources be defined? | ||
|
||
1. **Option 1**: Create resources as a new app/microservice shared by other apps. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In retrospect I might have chosen this instead to get around the "multiple applications" restriction. But IMO that ship has sailed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was actually the original design in the accelerator. I still like it in theory but in practice there were some challenges that I hadn't solved yet in regards to sharing the pool of notification recipients, similar to the challenges with sharing a cognito identity pool.
- Adds operational complexity by requiring notifications to be separately deployed | ||
- Requires notifications to be designed to support multiple tenants (multiple applications across multiple environments) which adds significant complexity within the notifications service. | ||
|
||
2. **Option 2**: Create resources in a new layer (e.g., build repository layer) shared across apps. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one being only cons is funny to me. I agree though.
- **Pros**: | ||
- Simplest to understand as it mirrors the structure of the rest of the infrastructure | ||
- **Cons**: | ||
- Only feasible if domains are not shared across multiple applications i.e. we use the domain of the app for sender email notifications. which was not the chosen option due to other factors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Only feasible if domains are not shared across multiple applications i.e. we use the domain of the app for sender email notifications. which was not the chosen option due to other factors. | |
- Only feasible if domains are not shared across multiple applications i.e. we use the domain of the app for sender email notifications. This was not the chosen option due to other factors. |
? (not sure if my suggestion is correct, grammar is hard sometimes)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the period should be a comma, I'll fix
- **Pros**: | ||
- As simple as Option 3 for projects where only one app needs to send notifications, which is the common case | ||
- **Cons**: | ||
- May require custom work in cases where more than one app needs to send notifications |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which may look like, for example, a notifications microservice 😆
|
||
### Consideration 3: Which layer should contain DKIM and DMARC DNS Records? | ||
|
||
1. **Option 1**: Create records in the network layer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In retrospect I might have chosen this had I thought about it more. Particularly in the case of grants.gov where we don't have any control of what DNS records get published. The DNS being in a different module would make it deploy an application in a way that feels clearly demarcated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm open to exploring this option, but would want to think about whether the pattern would also work for the A records for setting up the service's custom domain. The problem with the A records is you can only create those records after you know the DNS name for the load balancer which is in the service layer, and I'm hesitant (though open minded) to create a dependency on service layer resources in the network layer.
Ticket
n/a
Changes
n/a
Context for reviewers
realize we made some tradeoffs during the design of notifications that are worth documenting
Testing
n/a