Revive the Pinpoint implementation to send SMS notifications

[jimleroyer]

Description

As a system ops, I need to send a notification through a designated pool of code so that I can isolate codes depending on services and usage, i.e. short code versus dedicated long code or random pool of codes.

As a product manager, I need to send notifications through the short code, So that GCNotify gains more trust, reliability and throughput.

As a policy maker, I need to send notifications through the short code, so that 2fa notifications are only sent through it as agreed upon with the telecoms (assuming our short code stays exclusive for the 2FA usage).

WHY are we building?

• We need a way to isolate codes into their own groups, so that we can isolate usages to these, such as short code for 2FA or provincial dedicated long codes for specific government bodies (provinces).
• Although this does not increase throughput by itself, it leads to a solution that will increase it.
• For BCP purpose, this allows us to rely on more than mechanism to send our SMS (in a light manner; not a different provider).
• Financially, it might allow us to use long codes rather than the short code and better control our finance because we will be able to determine to which group of code to sent to (short code versus random pool for example).

WHAT are we building?

• Reviving the Pinpoint implementation to send notifications. The reason we need this for group of codes is because AWS only supports assigning a group for sending a notification through Pinpoint API and not via the current SNS API that we use.

VALUE created by our solution

• Capacity to isolate group of codes into sending categories (2FA, random pool, provincial bodies, dedicated long codes).

Out of scope

• Red flags such as the possibility of receipts being broken. Let's tackle this in a different card.

Acceptance Criteria

• [x] The Pinpoint implementation is revived.
• [x] The Pinpoint tests are brought back, reviewed, augmented and passing.
• [x] Switch to pinpoint implementation either using a) previous mechanism to pick providers, b) use a feature flag, c) use a canary release with progressive usage percentage over time of Pinpoint, d) switch to Pinpoint based on specific service IDs (for testing purpose at first).
• [ ] ~The AWS Pinpoint implementation is used in production to send all SMS notifications.~

Red flags

🚩 The SMS delivery receipts might come from a different source than the one for SNS, according to past investigation. If this turns out to be true, feel free to put this card to blocked and create a new task to tackle the new source of notifications with proper plumbing.

Additional information

• PR that introduced the feature: Implement AWS pinpoint client
• PR that removed the feature: feat: disable Pinpoint, use only SNS for SMS
• Past Investigation for SMS delivery messages in Canada on AWS which led to dropping Pinpoint

[jimleroyer]

Starting work on this today! 🕺

[sastels]

Clickops'd a pinpoint pool in staging with a new long code. Can send with it with

aws pinpoint-sms-voice-v2 send-text-message --destination-phone-number +<number> --origination-identity pool-090efa2fefab4cb5b10fa12125998266 --message-body "hi2me from pinpoint pool"

Started roughing in some pinpoint code: https://github.com/cds-snc/notification-api/pull/2152

[ben851]

@sastels to look into creating the pinpoint infrastructure with Terraform

[sastels]

Sadly it appears that terraform does not yet support Pinpoint V2 (including dealing with pools)

[sastels]

• now sending with the pinpoint pool for templates specified by the env var AWS_PINPOINT_TEMPLATE_IDS
• defaulting to sns for the rest - note that sns uses all pinpoint numbers, including the one that's in the pool :/
• still need unit tests
• also, not getting delivery receipts. Apparently we have to set up and use a Kinesis stream.

[ben851]

Pinpoint could possibly be implemented with CloudFormation instead.

[sastels]

looks like we can send delivery receipts to SNS or CloudWatch as well

[sastels]

pinpoint-sms-voice-v2 added on 2022-03-31

Looks like it's not supported in terraform nor cloudformation yet.

[sastels]

WIP PR for a bunch of stuff

• create cloudwatch log groups and iam role for pinpoint
• bash script that creates 2 Pinpoint pools and hooks up cloudwatch

[sastels]

All that IAM stuff in the PR actually works and I can send through a pinpoint pool and see it appear in the logs (tested in dev)! And in the logs we even get the actual phone number used to send.

[sastels]

made a log group for pinpoint failures too and have failures redirected there.

[sastels]

Ok! So we have

• terraform PR to add new log groups, iam role, alerts, ecr, lambda function, and a script to set up the pools
• api PR to send using Pinpoint and to add a task for processing delivery receipts
• lambdas PR to add the delivery receipt lambda function

Some parts of some of this have been tested locally or on dev merging any or all of these in should not affect how the system runs, ie the old SNS flow is still there and unchanged. The pinpoint bits will only be used if we set the AWSPINPOINT* variables in api and send using one of the designated templates.

[sastels]

work continues...

[sastels]

ready for review! https://github.com/cds-snc/notification-lambdas/pull/73 https://github.com/cds-snc/notification-terraform/pull/1246

[sastels]

• addressed comments and merged lambdas PR
• addressed comments in terraform PR

[sastels]

reworked terraform PR a bit more, bootstrapped the ecr repo on dev, and got the lambda applying as well. Ready for another look through by Ben.

[jimleroyer]

TF PR is ready for another look for infrastructure review. After that, Steve will play with the API and polish the PR. We will need to test in staging environment afterward with preconfigured pools of codes.

[sastels]

TF merged and looks good in staging. Can use AWS CLI to send an email to the new pool and the delivery gets logged, lambda triggered, and task scheduled.

Next step is finishing the api PR that contains the new pinpoint receipt processing task and logic to determine whether to send via pinpoint or not

[sastels]

api PR looks good, just going to give it another look over then it'll be ready for review.

[sastels]

api pr ready for review https://github.com/cds-snc/notification-api/pull/2152

[jimleroyer]

Jimmy and team to review the PR.

[P0NDER0SA]

Steve has the puck... he. turned it over at the blue line now it's back in progress

[sastels]

Still have a few of Jimmy's comments to address

[ben851]

Steve will get back to this this week to address Jimmy's comments

[sastels]

I think this is ready for Jimmy to take another look! https://github.com/cds-snc/notification-api/pull/2152

[sastels]

Another PR! https://github.com/cds-snc/notification-manifests/pull/2558

• This one creates the Pinpoint pool environment variables.
• We initially set staging and prod to empty variables.
• After testing this we will set the staging pool variables to the pool and template that has already been set up, and we will test.
• Finally we will move the short code to the prod pool and set the prod pool env vars appropriately. This will let us say when the short code definitely will be used.
• The next step is to add the long codes to the long code pool (with variable AWS_PINPOINT_LC_POOL_ID) and use that (through pinpoint) to send if the template is not in AWS_PINPOINT_SC_TEMPLATE_IDS

Update: PR merged into staging.

[sastels]

API PR approved. Will merge tomorrow after release and test if staging still works (ie with empty variables). If yes, the PR is ready for release.

Then will sent the staging variables to the shortcode pool and template and test with templates in and not in the shortcode template list.

[sastels]

Started PR for useing Pinpoint for non-shortcode templates as well https://github.com/cds-snc/notification-api/pull/2173 (WIP)

[sastels]

api PR merged into staging and tested. Nothing's different :tada:

Now will turn on PinPoint in staging and see if all these new tasks and lambdas work together 😬 https://github.com/cds-snc/notification-manifests/pull/2602

Shortcode pool

pinpoint_to_sqs_sms_callbacks callback lambda

[sastels]

alrighty! tested in staging.

• normal tempates go through normally with SNS
• SC template gets an IAM error

  [2024-05-08 19:35:09,029: ERROR/ForkPoolWorker-2] SMS notification delivery for id: 81af7a26-f09d-4c66-9355-7d1584ce4d64 failed
  Traceback (most recent call last):
  File "/app/app/clients/sms/aws_pinpoint.py", line 37, in send_sms
  response = self._client.send_text_message(
  File "/app/.venv/lib/python3.10/site-packages/botocore/client.py", line 565, in _api_call
  return self._make_api_call(operation_name, kwargs)
  File "/app/.venv/lib/python3.10/site-packages/botocore/client.py", line 1021, in _make_api_call
  raise error_class(parsed_response, operation_name)
  botocore.errorfactory.AccessDeniedException: An error occurred (AccessDeniedException) when calling the SendTextMessage operation: User: arn:aws:sts::239043911459:assumed-role/eks-worker-role/i-02248c15cedc52402 is not authorized to perform: sms-voice:SendTextMessage on resource: arn:aws:sms-voice:ca-central-1:239043911459:pool/pool-b20333ce1e4e49309ba1db3bf94a3f57 because no identity-based policy allows the sms-voice:SendTextMessage action

Oddly the SMS was retried 5 minutes later and sent with SNS - should look into this

[sastels]

ok! This PR added the action that was needed https://github.com/cds-snc/notification-terraform/pull/1314

And this time the pinpoint message was sent with pinpoint!

The lambda processed the receipt, but I see in the celery logs

[2024-05-08 20:21:37,476: ERROR/MainProcess] Received unregistered task of type 'process-pinpoint-result'.
The message has been ignored and discarded.

Possibly a name mismatch? Or I forgot to register the task... that sounds more likely.

In any case, progress!

[sastels]

ok, locally when I start celery I see the task listed

...
  . process-job
  . process-pinpoint-result
  . process-ses-result
  . process-sns-result
  . process-virus-scan-error
...

BUT looking at when celery pod celery-sms-send-scalable-77bcd554b8-5jcbf starts up I don't see the pinpoint task listed!

...
  . process-job
  . process-ses-result
  . process-sns-result
  . process-virus-scan-error
...

So! what's the difference between starting locally and in staging?

Both start with sh scripts/run_celery_send_sms.sh

Note that in the celery pod the code is there in process_pinpoint_receipts_tasks.py

If I run that script on an api node in k8s then I get the same results (ie no process-pinpoint-result)

AFAIK this is where we tell celery what the tasks are

    CELERY_IMPORTS = (
        "app.celery.tasks",
        "app.celery.scheduled_tasks",
        "app.celery.reporting_tasks",
        "app.celery.nightly_tasks",
    )

But process-sns-result isn't in there? it's in app.celery.process_sns_receipts_tasks. So how is it registered?

UPDATE: now I can't see the new task locally either.

ok, but at least I can fix it locally then. https://github.com/cds-snc/notification-api/pull/2175

[sastels]

It's working!

but successful pinpoint sends generate two receipts, one to say it's gone to the carrier, and one to say it's gone to the phone. We want to ignore the one to the carrier. https://github.com/cds-snc/notification-api/pull/2176

[sastels]

the ProviderDetails table has a pinpoint provider still there, it's at the end (priority 50) and inactive. Locally I:

• made it active by manually editing database
• in admin, set the priority to 5 (vs 10 for sns) - this means we will use it by default if the env vars are defined

We'd probably want a proper db migration to do this.

[sastels]

ok! this new PR actually works!

• all sms sent with pinpoint
• sc pool used for specified templated, otherwise the default pool.

Have to add a test or two around this still but can see the light at the end of the tunnel!

[sastels]

Added code to fall back to SNS if

• service has a dedicated number
• sending to an American phone number

Also added a bunch of tests for all this. Unfortunately tests are not all passing in CI despite passing locally.

[sastels]

ok, got the tests working and tested again locally. I think this is ready for :eyes: https://github.com/cds-snc/notification-api/pull/2173

[P0NDER0SA]

REady for review -- someone will look at it today. Probably Jimmy

[jimleroyer]

Reviewed, left a few questions and comments but nothing major to change. Looks good overall.

[sastels]

Steve will review today!

[jimleroyer]

Jimmy to review the review of the review.

[jimleroyer]

Jimmy approved the PR, Steve will merge after the release is done this morning to test it a bit in staging.

[sastels]

Code merged into staging, and released to prod. SNS still used by both (as env vars are not set).

[P0NDER0SA]

Scope was reduced on this card, so we need to review/QA this one and determine if it's ready to be closed.

[sastels]

Had to turn Pinpoint off in staging, will turn back on today hopefully and we can then QA

[jimleroyer]

Removed the QA steps: we will test this feature in the "QA flags" cards that turn this feature on.