Use Pinpoint by default #2173

sastels · 2024-05-08T12:43:45Z

Summary | Résumé

Use Pinpoint by default to send the non-shortcode SMS using the default pool.

Exceptions are notifications sent from a dedicated long code and notifications sent to American phone numbers. These will still be sent by SNS.

Related Issues | Cartes liées

https://app.zenhub.com/workspaces/notify-planning-core-6411dfb7c95fb80014e0cab0/issues/gh/cds-snc/notification-planning-core/286

Test instructions | Instructions pour tester la modification

To run locally, set your AWS_PINPOINT_SC_POOL_ID, AWS_PINPOINT_DEFAULT_POOL_ID, and AWS_US_TOLL_FREE_NUMBER environment variables to the values in staging

AWS_PINPOINT_SC_POOL_ID = "pool-b20333ce1e4e49309ba1db3bf94a3f57"
AWS_PINPOINT_DEFAULT_POOL_ID = "pool-8885654b47d6466f9aaee14c62494ff2"
AWS_US_TOLL_FREE_NUMBER=+18449521252

set the env var AWS_PINPOINT_SC_TEMPLATE_IDS to a list of some sms template ids (from your local install). Just one is ok

AWS_PINPOINT_SC_TEMPLATE_IDS = 000000-1111-2222-3333-444444444

SMS can be sent with:

Pinpoint: logs will contain AWS Pinpoint request finished in ... and SMS will be sent from the SC pool number or the default pool number as appropriate.
SNS: logs will contain AWS SNS request finished in ...

Tests:

Send an sms using a template in the SC list. It should be sent from the Pinpoint SC pool.
Send an sms using a template not in the list. It should be sent from the Pinpoint default pool.
Send an sms using a service with a dedicated number. If should be sent through SNS (and from your dedicated number)
Send an sms to a US number (ex: 7065550123, a fake number in Georgia). If should be sent through SNS
Set AWS_PINPOINT_SC_POOL_ID or AWS_PINPOINT_DEFAULT_POOL_ID to None, restart both your terminal window and celery, and send to yourself. It should send using SNS.
Send an email to verify that we didn't affect that! 😬

Release Instructions | Instructions pour le déploiement

None.

Note in particular that if the pool_id variables are not sent then we default to just using SNS.

Reviewer checklist | Liste de vérification du réviseur

This PR does not break existing functionality.
This PR does not violate GCNotify's privacy policies.
This PR does not raise new security concerns. Refer to our GC Notify Risk Register document on our Google drive.
This PR does not significantly alter performance.
Additional required documentation resulting of these changes is covered (such as the README, setup instructions, a related ADR or the technical documentation).

⚠ If boxes cannot be checked off before merging the PR, they should be moved to the "Release Instructions" section with appropriate steps required to verify before release. For example, changes to celery code may require tests on staging to verify that performance has not been affected.

Co-authored-by: Jimmy Royer <[email protected]>

sastels · 2024-05-14T19:31:26Z

tests/app/clients/test_aws_pinpoint.py

+from tests.conftest import set_config_values
+
+
+@pytest.mark.serial


There's some sort of race condition in our tests that I couldn't figure out :/ If these two tests are not run serially then something resets the config variables that they change.

It would be a great topic for a code review session. It seems we have a misunderstanding on our some of our test concurrency is setup and fails when they should be successful. We can put our mind to it and try to brainstorm on this.

I gave some more thoughts to this and one thing I wish we could do better is OOP in our code base. We barely have much and this could be our focus on code reviews session. We rely a lot on environment variables which is not a good practice in general, as the code relies on environment rather than configuration of the logic to run. Encapsulating the pool config in an object would make this easier probably to run instead of relying on environments. Do you know what I mean here?

We do pull the env vars out of the env and put into the Config object, but we should make sure that we always read that object and never the env vars directly (which I think we're pretty good at?). I think that there's magic going on behind the scenes though (with flask, for example), where it takes vars directly from the env :/

In this case, though, in the test I made serial the app wasn't using the values of Config that I added in the test setup 😞 Which - how can that happen?

sastels · 2024-05-14T19:41:35Z

app/clients/sms/aws_sns.py

            except Exception as e:
                self.statsd_client.incr("clients.sns.error")
-                raise str(e)


This stuff was redundant and also the raise didn't work! You can't raise a string, just some subclass of Exception.

pylint for the win! I feel bad we had that for that long in the code. The whole error management does look weird in retrospective, thank you for refactoring.

Ha, I didn't find it with pylint but by accidentally triggering the error locally (I was trying to send to a USA number without setting the AWS_US_TOLL_FREE_NUMBER variable)

Okay. I thought it might be pylint because the latter did raise it as a potential problem when I looked at this code locally in vscode.

app/clients/sms/aws_sns.py

jimleroyer · 2024-05-15T21:05:55Z

app/clients/sms/aws_pinpoint.py

+        if template_id is not None and str(template_id) in self.current_app.config["AWS_PINPOINT_SC_TEMPLATE_IDS"]:
+            pool_id = self.current_app.config["AWS_PINPOINT_SC_POOL_ID"]
+        else:
+            pool_id = self.current_app.config["AWS_PINPOINT_DEFAULT_POOL_ID"]


What happens if the pool_id is None? Is a random OriginationIdentity would be used then?

if the OriginationIdentity is None in the boto call then the call returns an error

Invalid type for parameter OriginationIdentity, value: None, type: <class 'NoneType'>, valid types: <class 'str'>

But as you mention below, we catch that case in our code and fall back to SNS.

jimleroyer · 2024-05-15T21:08:22Z

app/config.py

@@ -267,6 +267,7 @@ class Config(object):
    AWS_SES_SECRET_KEY = os.getenv("AWS_SES_SECRET_KEY")
    AWS_PINPOINT_REGION = os.getenv("AWS_PINPOINT_REGION", "us-west-2")
    AWS_PINPOINT_SC_POOL_ID = os.getenv("AWS_PINPOINT_SC_POOL_ID", None)
+    AWS_PINPOINT_DEFAULT_POOL_ID = os.getenv("AWS_PINPOINT_DEFAULT_POOL_ID", None)


I wonder if we really want None here as a fallback default, or rather prevent the server from booting up to prevent a misconfiguration. It depends how OriginationIdentity set to None in the send_text_message call behaves.

Ok reading the code further, it seems that a None value would route the SNS implementation to get used instead. I got this right?

Yeah exactly. A None value will break the Pinpoint send, so right now we route to SNS. In the future we may want to throw an error instead (ie when we insist on Pinooint for everything). Right now it allows production to ignore pinpoint entirely since we don't have it set there yet.

jimleroyer · 2024-05-15T21:14:40Z

app/delivery/send_to_providers.py

+) -> Any:
+    """
+    Get the provider to use for sending the notification.
+    SMS that are being sent with a dedicated number or to a US number should not use Pinpoint.


Thanks for the documentation and typing annotations.

I figured I had added enough new logic to this function that some documentation was useful 😬

jimleroyer · 2024-05-15T21:24:49Z

tests/app/clients/test_aws_pinpoint.py

+    with pytest.raises(ValueError) as excinfo:
+        aws_pinpoint_client.send_sms(to, content, reference)
+
+    assert "No valid numbers found for SMS delivery" in str(excinfo.value)


Should we have a warning alert in TF specifically for this one? 🤔 I guess it might fall into celery errors but these might be generic enough that we don't always check on these?

possibly, though this is a bit of a strange test - testing that if the notification has an invalid phone number in it then we get a log. Agree though that this is a strange thing that we'd like surfaced immediately though. Basically it means that we screwed up the phone number and AWS doesn't recognize it. So, for example, we start sending texts to a new country and our number formatter messes up. Yeah, I'll add a warning for these 👍

tests/app/dao/test_provider_details_dao.py

app/delivery/send_to_providers.py

jimleroyer · 2024-05-15T21:39:05Z

tests/app/delivery/test_send_to_providers.py

+        assert provider.name == "sns"
+
+    @pytest.mark.parametrize("sc_pool_id, default_pool_id", [(None, "default_pool_id"), ("sc_pool_id", None)])
+    def test_should_use_sns_if_pinpoint_not_configured(self, restore_provider_details, notify_api, sc_pool_id, default_pool_id):


jimleroyer · 2024-05-15T21:39:13Z

tests/app/delivery/test_send_to_providers.py

@@ -51,6 +51,53 @@
 from tests.conftest import set_config_values


+class TestProviderToUse:


Nice set of tests!

Co-authored-by: Jimmy Royer <[email protected]>

app/delivery/send_to_providers.py

Co-authored-by: Jimmy Royer <[email protected]>

jimleroyer

LGTM! can't wait to have more of the pool config going forward!

sastels and others added 30 commits April 5, 2024 17:10

rough in pinpoint

9740520

add pool_id env var

064e36d

sending with pinpoint pool

b9d0d3a

use pinpoint pool for specified templates

0fa34c1

format

7e69706

tweak

b3d12e5

Merge branch 'main' into rough-in-pinpoint

fe263c6

add configuration set

d18ce27

add task for processing pinpoint receipts

fceab9f

add mocks for pinpoint receipt task testing

b8ceb91

rough in test_process_pinpoint_receipts_tasks.py

1a6ba9e

make tests pass

6e3d339

add explicit return

ca2da8c

Merge branch 'main' into rough-in-pinpoint

7ec198b

tweak

3db48e8

tweak

d2e8471

Merge branch 'main' into rough-in-pinpoint

c8eb6c5

working now

6c090ce

add new env vars to .env.examples

3f59f27

Update .env.example

646b86d

Co-authored-by: Jimmy Royer <[email protected]>

Update .env.example

bde0f68

Co-authored-by: Jimmy Royer <[email protected]>

Update app/clients/sms/aws_pinpoint.py

12f539f

Co-authored-by: Jimmy Royer <[email protected]>

Update app/config.py

965fbff

Co-authored-by: Jimmy Royer <[email protected]>

Update app/delivery/send_to_providers.py

26d4419

Co-authored-by: Jimmy Royer <[email protected]>

Update app/config.py

5710df2

Co-authored-by: Jimmy Royer <[email protected]>

Merge branch 'main' into rough-in-pinpoint

28524bb

add typing / docstring for determine_pinpoint_status

efa0df9

wip add pattern matching

d1612c2

matchy-matchy

c08cc14

Merge branch 'main' into rough-in-pinpoint

aed401f

sastels commented May 14, 2024

View reviewed changes

sastels added 2 commits May 14, 2024 15:36

nope

fb8f395

again

2a80780

sastels commented May 14, 2024

View reviewed changes

sastels marked this pull request as ready for review May 14, 2024 19:54

sastels requested review from jimleroyer and jzbahrai May 14, 2024 19:54