Skip to content

Add notifications-sms module to support SMS notifications#256

Closed
jrpbc wants to merge 16 commits intomainfrom
johan/notifications-sms
Closed

Add notifications-sms module to support SMS notifications#256
jrpbc wants to merge 16 commits intomainfrom
johan/notifications-sms

Conversation

@jrpbc
Copy link
Copy Markdown
Contributor

@jrpbc jrpbc commented Feb 9, 2026

Ticket

Resolves #976

Changes

What was added, updated, or removed in this PR.

- Implements PinpointSMSVoiceV2 service resources
- Supports SMS configuration sets and opt-out lists
- Includes VPC endpoint for secure connectivity
- Provides IAM policies for SMS sending permissions
- Supports delivery receipt logging to CloudWatch

Context for reviewers

Testing instructions, background context, more in-depth details of the implementation, and anything else you'd like to call out or ask reviewers.

Testing

Provide evidence that the code works as expected. Explain what was done for testing and the results of the test plan. Include screenshots, GIF demos, shell commands or output to help show the changes working as expected. ProTip: you can drag and drop or paste images into this textbox.

Waiting for AWS to approve requested phone number +18665684902.

  • Performed basic validation which includes:
    • Go to /sms-notifications -> use AWS provided receiver phone number +14254147755 (This phone number is for successful messages test); validated Cloudwatch logs for message success (TEXT_SUCCESS status)
    • Go to /sms-notifications -> use AWS provided receiver phone number +14254147167 (This phone number is for failure messages test); validated Cloudwatch logs for message failure (TEXT_BLOCKED status)

Preview environment for app-nextjs

♻️ Environment destroyed ♻️

Preview environment for app

♻️ Environment destroyed ♻️

Preview environment for app-flask

♻️ Environment destroyed ♻️

Preview environment for app-rails

♻️ Environment destroyed ♻️

@jrpbc jrpbc requested a review from doshitan February 13, 2026 14:12
Copy link
Copy Markdown
Contributor

@doshitan doshitan left a comment

Choose a reason for hiding this comment

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

This feels a little complex. Can you update the tech spec with tradeoffs/rationale and future considerations around some of the decisions? It looks like it's been updated a little bit, but a lot of context feels missing.

In particular, the "simulator" stuff doesn't feel very useful. Can we drop it?

Comment thread infra/modules/network/resources/variables.tf
Comment thread infra/app/service/notifications.tf Outdated
Comment thread infra/modules/notifications-sms/resources/main.tf
Comment thread infra/modules/notifications-sms/resources/variables.tf Outdated
Comment thread infra/modules/notifications-sms/resources/variables.tf Outdated
Comment thread app/sms_notifications.py Outdated
Comment thread app/app.py Outdated
Comment thread infra/modules/notifications-sms/resources/variables.tf Outdated
Comment thread infra/app/app-config/env-config/variables.tf Outdated
Comment thread infra/modules/notifications-sms/resources/variables.tf Outdated
@jrpbc
Copy link
Copy Markdown
Contributor Author

jrpbc commented Feb 18, 2026

Updated Tech Specs with: All Considerations; Implementation Options considered.

@jrpbc jrpbc requested a review from doshitan February 19, 2026 02:01
# Allow access to the phone pool created by CloudFormation
data.aws_cloudformation_stack.sms_config_set_outputs.outputs["PhonePoolArn"],
# Allow access to the configuration set created by this module
"arn:aws:sms-voice:*:${data.aws_caller_identity.current.account_id}:configuration-set/${var.name}-config-set"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it not possible to get this value from the created resource directly?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Due to the use of CloudFormation (via aws_cloudformation_stack) and the inability to fully control the timing of its execution, I was intermittently encountering the following error:
`Error: Provider produced inconsistent final plan

When expanding the plan for module.notifications_sms[0].aws_cloudformation_stack.sms_config_set to include new values learned so far during apply, provider "registry.terraform.io/hashicorp/aws" produced an invalid new value for .outputs: was known, but now unknown.
`
With the use of Terraform "data" here allows us to better control the timing and sequence of execution for the CloudFormation stack, preventing the provider from re-evaluating stack outputs in an inconsistent state and eliminating the intermittent plan inconsistency error.

Comment thread infra/app/app-config/env-config/variables.tf
Comment thread docs/infra/sms-notifications.md Outdated
Comment thread infra/app/service/notifications.tf Outdated
@jrpbc jrpbc requested a review from doshitan February 24, 2026 05:58
@jrpbc jrpbc force-pushed the johan/notifications-sms branch from b01e304 to 1a68e31 Compare March 12, 2026 19:20
@jrpbc jrpbc force-pushed the johan/notifications-sms branch 5 times, most recently from ae7a043 to e462bbc Compare March 13, 2026 05:34
Copy link
Copy Markdown
Contributor

@doshitan doshitan left a comment

Choose a reason for hiding this comment

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

A few notes mostly around docs and some cleanup. But I think we are good to create the template PR.

<form method="post">
<div>
<label for="phone_number">Receiver Phone Number (E.164 format: +1234567890):</label><br>
<input type="tel" id="phone_number" name="phone_number"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we can make this a select element populated by the rendering endpoint, with only the predefined simulator numbers by default.

(longer term we should probably better lock down the email testing endpoint as well, we don't really want to be a vehicle to spam people)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Restricting this to the simulator numbers here will prevent from testing approved phone numbers. I added an extra message to this form, specifying the phone numbers that can be used for testing if using simulator phone number as originator.

# 1. Creates AWS End User Messaging SMS configuration set
# 2. Sets up IAM permissions for SMS sending
# 3. Configures SMS delivery tracking and opt-out management
enable_sms_notifications = true
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The default in the template PR should be false.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Applied change

Comment thread infra/project-config/aws_services.tf Outdated
Comment thread infra/app/service/notifications.tf Outdated
module.notifications_phone_pool_temp[0].phone_pool_id)
) : null

#SMS environment variables for notifications-sms module
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
#SMS environment variables for notifications-sms module
# SMS environment variables for notifications-sms module

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Applied change

Comment thread infra/modules/notifications-phone-pool/data/main.tf Outdated
Comment thread docs/infra/sms-notifications.md Outdated
- **SIMULATOR**: Test numbers for development (immediate approval, limited recipients)

### Simulator Phone Numbers
AWS Allows to use **simulator** phone numbers as originator to send text. This simulator phone numbers are only allowed to send SMS text to:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
AWS Allows to use **simulator** phone numbers as originator to send text. This simulator phone numbers are only allowed to send SMS text to:
AWS Allows to use **simulator** phone numbers as originator to send text. This simulator phone numbers are only allowed to send SMS text to:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Applied change

Comment thread docs/infra/sms-notifications.md Outdated
sms_number_type = null # Enter SMS Number Type (e.g: 'SHORT_CODE', 'LONG_CODE', 'TOLL_FREE', 'TEN_DLC', 'SIMULATOR') when available; otherwise leave empty to use simulator number type
}
```
*__Note:__* if an AWS End User SMS Registration ID is not provided, a simulator phone number will be automatically provisioned which will allow applications to send SMS test to:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we point to the above Simulator Phone Numbers section rather than duplicating?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Applied change

Comment thread docs/infra/sms-notifications.md Outdated
*__Note:__* This assumes your AWS End User Messaging SMS Registration has been approved and configured in `infra/<APP_NAME>/app-config/env-config/dev.tf|staging.tf|prod.tf`
### 1. Verify Destination Numbers (Sandbox Accounts)

In sandbox environments, you can only send SMS to up to 10 verified phone numbers. Steps to register destination phone numbers:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
In sandbox environments, you can only send SMS to up to 10 verified phone numbers. Steps to register destination phone numbers:
In sandbox environments, you can only send SMS to up to 10 verified phone numbers. Steps to register destination phone numbers:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Applied change

Comment thread docs/infra/sms-notifications.md Outdated

## Requirements

1. **AWS End User Messaging SMS Registration**: All provisioned originating phone numbers require a AWS End User Messaging Registration form to be completed in the AWS End User Messaging Service, and approved by the AWS phone carrier. Without an approved Registration, originating phone numbers cannot be provisioned.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All provisioned originating phone numbers require a AWS End User Messaging Registration form to be completed

Still makes it sound like every phone number will need a registration form completed at first glance. Can we clarify?

Suggested change
1. **AWS End User Messaging SMS Registration**: All provisioned originating phone numbers require a AWS End User Messaging Registration form to be completed in the AWS End User Messaging Service, and approved by the AWS phone carrier. Without an approved Registration, originating phone numbers cannot be provisioned.
1. **AWS End User Messaging SMS Registration**: All provisioned originating phone numbers require a AWS End User Messaging Registration form to be completed in the AWS End User Messaging Service, and approved by the AWS phone carrier. Without an approved Registration, originating phone numbers cannot be provisioned.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Changed the description of the AWS End User Messaging Registration and its purpose and process

Comment thread docs/infra/sms-notifications.md
- Break notifications-sms module into two components:
  * notifications-phone-pool: manages phone pools and phone numbers (CloudFormation)
  * notifications-sms: manages configuration sets and IAM policies (CloudFormation + Terraform)
- Add logic to reuse phone pool on temporary environments or create one f none exists
- Add logic to create phone pool for production environment
- Update documentations
@jrpbc jrpbc force-pushed the johan/notifications-sms branch from e462bbc to 890c33d Compare March 17, 2026 04:28
    - Break notifications-sms module into two components:
      * notifications-phone-pool: manages phone pools and phone numbers (CloudFormation)
      * notifications-sms: manages configuration sets and IAM policies (CloudFormation + Terraform)
    - Add logic to reuse phone pool on temporary environments or create one f none exists
    - Add logic to create phone pool for production environment
    - Update documentations
@jrpbc jrpbc force-pushed the johan/notifications-sms branch from b1aa6cc to 599b06a Compare March 17, 2026 05:36
@jrpbc jrpbc requested a review from doshitan March 17, 2026 05:49
Comment thread docs/infra/sms-notifications.md
Comment thread docs/infra/sms-notifications.md

## Testing Setup

*__Note:__* This assumes your AWS End User Messaging SMS Registration has been approved and configured in `infra/<APP_NAME>/app-config/env-config/dev.tf|staging.tf|prod.tf`
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The specific environments will vary by project, can we just point back up to the "Configure SMS settings" section?

Comment thread docs/infra/sms-notifications.md Outdated
Comment thread docs/infra/sms-notifications.md Outdated
Comment thread docs/infra/sms-notifications.md Outdated
Comment thread docs/infra/sms-notifications.md Outdated
Comment thread infra/modules/notifications-phone-pool/data/find-existing-pools.sh Outdated
Comment thread infra/modules/notifications-phone-pool/data/find-existing-pools.sh Outdated
Copy link
Copy Markdown
Contributor

@doshitan doshitan Mar 17, 2026

Choose a reason for hiding this comment

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

The script looks to only grab the first phone pool in a region? So the name doesn't feel quite right being plural, maybe more find-an-existing-pool.sh?

Co-authored-by: Tanner Doshier <git@doshitan.com>
jrpbc and others added 2 commits March 30, 2026 15:35
Co-authored-by: Tanner Doshier <git@doshitan.com>
@jrpbc jrpbc marked this pull request as ready for review March 30, 2026 19:51
@jrpbc jrpbc requested a review from doshitan March 30, 2026 19:52
@jrpbc jrpbc force-pushed the johan/notifications-sms branch from 79239d7 to 3cd048f Compare April 9, 2026 01:52
@jrpbc jrpbc force-pushed the johan/notifications-sms branch from 3cd048f to a9f8a87 Compare April 9, 2026 01:58
jrpbc added a commit to navapbc/template-infra that referenced this pull request Apr 13, 2026
## Ticket

Resolves #976 

## Changes

Implements two new Terraform modules for SMS notifications via AWS
PinpointSMSVoiceV2:

- notifications-phone-pool: manages phone pools and phone numbers
* Auto-detects and reuses existing phone pools on temporary environments
  * Creates dedicated phone pool for production environments
* Includes GitHub Actions IAM permissions for SMS-Voice and
CloudFormation resources

- notifications-sms: manages configuration sets and IAM policies
  * Implements PinpointSMSVoiceV2 configuration sets and opt-out lists
  * Includes VPC endpoint for secure connectivity
  * Provides IAM policies for SMS sending permissions
  * Supports delivery receipt logging to CloudWatch

Also adds:
- SMS notifications feature documentation
(docs/infra/sms-notifications.md)
- SMS notifications implementation ADR

## Context for reviewers

## Testing

platform-test PR:
navapbc/platform-test#256 (comment)

---------

Co-authored-by: Tanner Doshier <git@doshitan.com>
@doshitan doshitan closed this Apr 16, 2026
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.

Notifications: SMS support

2 participants