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

Fix notifications access from service #154

Closed
wants to merge 14 commits into from
Closed

Conversation

lorenyu
Copy link
Collaborator

@lorenyu lorenyu commented Jan 16, 2025

Ticket

Contributes to navapbc/template-infra#792

Changes

Fix ability for ECS service to access Pinpoint:

  • Add VPC endpoints for Pinpoint services
  • Add IAM policy for accessing Pinpoint and SES to service

Also add notifications test endpoint to example app for testing:

  • Add /notifications page where you can enter an email address and get a test notification to that email address
  • Add links to the homepage to all the test pages in the example app

Context for reviewers

I added /notifications endpoint since it will be useful to test changes to notifications infrastructure, but while testing I discovered some access bugs

Testing

Applied changes to dev network to create the two VPC endpoints using

make infra-update-network NETWORK_NAME=dev

Tested locally
image

Tested on the PR environment, you can too! (took a while to get the IAM permissions right)
image

Preview environment

♻️ Environment destroyed ♻️

@lorenyu lorenyu requested a review from coilysiren January 16, 2025 19:30
@@ -18,6 +18,9 @@ locals {

# AWS services used by ECS Exec
var.enable_command_execution ? ["ssmmessages"] : [],

# AWS services used by notifications
var.enable_notifications ? ["pinpoint", "email-smtp"] : [],
Copy link
Contributor

Choose a reason for hiding this comment

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

It's called mobile-targeting in IAM but the VPC endpoints are ["pinpoint", "email-smtp"] ??? bah

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah i put mobiletargeting at first and got an error saying that doesn't exist.

also, the AWS docs don't mention "email-smtp" anywhere, instead they said to search for smtp and then click on that one, so I had to go to the console and search to find out that email-smtp was what they meant.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

image https://docs.aws.amazon.com/ses/latest/dg/send-email-set-up-vpc-endpoints.html

amazing right?

]
Resource = [
var.domain_identity_arn,
"arn:*:ses:*:*:configuration-set/*",
Copy link
Contributor

Choose a reason for hiding this comment

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

Any specific reason why this 2nd on isn't an arn as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

configuration sets don't actually have arns as far as i could tell. couldn't find it on AWS console, and don't see it in the outputs in the terraform aws_sesv2_configuration_set data source or resource, so you'd have to construct it manually.

and in my opinion i don't think it's actually a big deal to allow the use of any configuration set.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wow... yeah no arns mentioned here. That's a new one for me!

Copy link
Contributor

@coilysiren coilysiren left a comment

Choose a reason for hiding this comment

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

non-blocking q

@lorenyu lorenyu changed the title Add notifications test endpoint to example app Fix notifications access from service Jan 16, 2025
@lorenyu
Copy link
Collaborator Author

lorenyu commented Jan 16, 2025

done in navapbc/template-infra#835

@lorenyu lorenyu closed this Jan 16, 2025
@lorenyu lorenyu deleted the lorenyu/notificationstest branch January 16, 2025 20:23
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