From 32f499c802c5b8e842f1e94262874a0c58a92692 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Fri, 9 Apr 2021 11:08:43 +0100 Subject: [PATCH 1/5] Fix setting the CELERYD_PREFETCH_MULTIPLIER variable for broadcasts This was not being set correctly in the manifest for the notify-delivery-worker-broadcasts worker. --- manifest.yml.j2 | 2 ++ 1 file changed, 2 insertions(+) diff --git a/manifest.yml.j2 b/manifest.yml.j2 index 3aba456fe..3acd561d8 100644 --- a/manifest.yml.j2 +++ b/manifest.yml.j2 @@ -65,7 +65,9 @@ 'notify-delivery-worker-retry-tasks': {}, 'notify-delivery-worker-internal': {}, 'notify-delivery-worker-broadcasts': { + 'additional_env_vars': { 'CELERYD_PREFETCH_MULTIPLIER': 1, + } }, 'notify-delivery-worker-receipts': {}, 'notify-delivery-worker-service-callbacks': {'disk_quota': '2G'}, From c3d9aca43a8e666fcc45aba52ff64d83f419eea8 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Fri, 9 Apr 2021 11:25:48 +0100 Subject: [PATCH 2/5] Remove redundant comment We no longer have a noop client --- app/config.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/config.py b/app/config.py index 81bb897e5..5a5a38fc5 100644 --- a/app/config.py +++ b/app/config.py @@ -365,8 +365,6 @@ class Config(object): AWS_REGION = 'eu-west-1' - # CBC Proxy - # if the access keys are empty then noop client is used CBC_PROXY_AWS_ACCESS_KEY_ID = os.environ.get('CBC_PROXY_AWS_ACCESS_KEY_ID', '') CBC_PROXY_AWS_SECRET_ACCESS_KEY = os.environ.get('CBC_PROXY_AWS_SECRET_ACCESS_KEY', '') From 514afeb6f3f52baaa71bb2dfc58dda8ffe66fbc6 Mon Sep 17 00:00:00 2001 From: David McDonald Date: Fri, 9 Apr 2021 11:38:48 +0100 Subject: [PATCH 3/5] Set `CBC_PROXY_ENABLED` per environment, not dynamically Previously we looked at whether an environment was given AWS access keys to decide if the `CBC_PROXY_ENABLED` setting was true. Given that all environments (apart from development) are currently hooked up to our AWS cell broadcast accounts, it doesn't feel too useful to have a dynamic switch when we can just hardcode it. On top of that, this lays the groundwork for having `CBC_PROXY_ENABLED` to be True even if an individual application doesn't have the CBC PROXY aws access keys as in future only the broadcasts worker will have the AWS keys but all the other apps will know that cell broadcasting is indeed turned on for that environment. --- app/config.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/app/config.py b/app/config.py index 5a5a38fc5..82269e82d 100644 --- a/app/config.py +++ b/app/config.py @@ -365,11 +365,10 @@ class Config(object): AWS_REGION = 'eu-west-1' + CBC_PROXY_ENABLED = True CBC_PROXY_AWS_ACCESS_KEY_ID = os.environ.get('CBC_PROXY_AWS_ACCESS_KEY_ID', '') CBC_PROXY_AWS_SECRET_ACCESS_KEY = os.environ.get('CBC_PROXY_AWS_SECRET_ACCESS_KEY', '') - CBC_PROXY_ENABLED = bool(CBC_PROXY_AWS_ACCESS_KEY_ID) - ENABLED_CBCS = {BroadcastProvider.EE, BroadcastProvider.THREE, BroadcastProvider.O2, BroadcastProvider.VODAFONE} # as defined in api db migration 0331_add_broadcast_org.py @@ -420,6 +419,8 @@ class Development(Config): API_RATE_LIMIT_ENABLED = True DVLA_EMAIL_ADDRESSES = ['success@simulator.amazonses.com'] + CBC_PROXY_ENABLED = False + class Test(Development): NOTIFY_EMAIL_DOMAIN = 'test.notify.com' From 295162c81d7ebab46cbb19209a751055a59efd4c Mon Sep 17 00:00:00 2001 From: David McDonald Date: Mon, 12 Apr 2021 15:17:59 +0100 Subject: [PATCH 4/5] Move CBC proxy enable check This change will make our development environments closer to production even if they aren't hooked up to the CBC proxy lambda functions. Now in development, we will create the broadcast event and create tasks for each broadcast provider event. We will still not create actual broadcast provider message rows in the DB and talk to the CBC proxies. This should be helpful in development to catch any issues we introduce to do with sending broadcast messaging. In time we may wish to have some fake CBC proxies in the AWS tools account that we can interact with to make it even more realistic. --- app/celery/broadcast_message_tasks.py | 11 ++++--- .../celery/test_broadcast_message_tasks.py | 29 ++++++++++--------- 2 files changed, 23 insertions(+), 17 deletions(-) diff --git a/app/celery/broadcast_message_tasks.py b/app/celery/broadcast_message_tasks.py index a319027ec..bfde17f42 100644 --- a/app/celery/broadcast_message_tasks.py +++ b/app/celery/broadcast_message_tasks.py @@ -106,10 +106,6 @@ def check_provider_message_should_send(broadcast_event, provider): @notify_celery.task(name="send-broadcast-event") @statsd(namespace="tasks") def send_broadcast_event(broadcast_event_id): - if not current_app.config['CBC_PROXY_ENABLED']: - current_app.logger.info(f'CBC Proxy disabled, not sending broadcast_event {broadcast_event_id}') - return - broadcast_event = dao_get_broadcast_event_by_id(broadcast_event_id) if ( @@ -149,6 +145,13 @@ def send_broadcast_event(broadcast_event_id): @notify_celery.task(bind=True, name="send-broadcast-provider-message", max_retries=None) @statsd(namespace="tasks") def send_broadcast_provider_message(self, broadcast_event_id, provider): + if not current_app.config['CBC_PROXY_ENABLED']: + current_app.logger.info( + "CBC Proxy disabled, not sending broadcast_provider_message for " + f"broadcast_event_id {broadcast_event_id} with provider {provider}" + ) + return + broadcast_event = dao_get_broadcast_event_by_id(broadcast_event_id) check_provider_message_should_send(broadcast_event, provider) diff --git a/tests/app/celery/test_broadcast_message_tasks.py b/tests/app/celery/test_broadcast_message_tasks.py index 9cbf2e6ac..61d5aae03 100644 --- a/tests/app/celery/test_broadcast_message_tasks.py +++ b/tests/app/celery/test_broadcast_message_tasks.py @@ -1,6 +1,6 @@ import uuid from datetime import datetime -from unittest.mock import ANY, call +from unittest.mock import ANY, Mock, call import pytest from celery.exceptions import Retry @@ -98,18 +98,6 @@ def test_send_broadcast_event_does_nothing_if_provider_set_on_service_isnt_enabl assert mock_send_broadcast_provider_message.apply_async.called is False -def test_send_broadcast_event_does_nothing_if_cbc_proxy_disabled(mocker, notify_api): - mock_send_broadcast_provider_message = mocker.patch( - 'app.celery.broadcast_message_tasks.send_broadcast_provider_message', - ) - - event_id = uuid.uuid4() - with set_config(notify_api, 'ENABLED_CBCS', ['ee', 'vodafone']), set_config(notify_api, 'CBC_PROXY_ENABLED', False): - send_broadcast_event(event_id) - - assert mock_send_broadcast_provider_message.apply_async.called is False - - def test_send_broadcast_event_creates_zendesk_p1(mocker, notify_api, sample_broadcast_service): template = create_template(sample_broadcast_service, BROADCAST_TYPE) broadcast_message = create_broadcast_message( @@ -747,3 +735,18 @@ def test_check_provider_message_should_send_raises_if_current_event_already_has_ create_broadcast_provider_message(current_event, provider='ee', status=existing_message_status) check_provider_message_should_send(current_event, 'ee') + + +def test_send_broadcast_provider_message_does_nothing_if_cbc_proxy_disabled(mocker, notify_api, sample_template): + mock_proxy_client_getter = mocker.patch( + 'app.celery.broadcast_message_tasks.cbc_proxy_client', + ) + mock_client = Mock() + mock_proxy_client_getter.get_proxy.return_value = mock_client + + broadcast_message = create_broadcast_message(sample_template) + broadcast_event = create_broadcast_event(broadcast_message, message_type='alert') + with set_config(notify_api, 'ENABLED_CBCS', ['ee', 'vodafone']), set_config(notify_api, 'CBC_PROXY_ENABLED', False): + send_broadcast_provider_message(broadcast_event.id, 'ee') + + assert mock_client.create_and_send_broadcast.called is False From 4437d60dd7da82eba14628c23c5b04dfcf09bbf5 Mon Sep 17 00:00:00 2001 From: David McDonald Date: Mon, 12 Apr 2021 15:27:47 +0100 Subject: [PATCH 5/5] Only give broadcasts worker IAM creds for CBC proxy There is no need to give it to any of the other workers and so the fewer instances that have these creds the better. You can verify this works by running ``` CF_APP=notify-api CF_SPACE=preview make generate-manifest ``` vs ``` CF_APP=notify-delivery-worker-broadcasts CF_SPACE=preview make generate-manifest ``` --- manifest.yml.j2 | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/manifest.yml.j2 b/manifest.yml.j2 index 3acd561d8..59401d31f 100644 --- a/manifest.yml.j2 +++ b/manifest.yml.j2 @@ -67,6 +67,8 @@ 'notify-delivery-worker-broadcasts': { 'additional_env_vars': { 'CELERYD_PREFETCH_MULTIPLIER': 1, + 'CBC_PROXY_AWS_ACCESS_KEY_ID': CBC_PROXY_AWS_ACCESS_KEY_ID, + 'CBC_PROXY_AWS_SECRET_ACCESS_KEY': CBC_PROXY_AWS_SECRET_ACCESS_KEY, } }, 'notify-delivery-worker-receipts': {}, @@ -127,11 +129,6 @@ applications: AWS_ACCESS_KEY_ID: '{{ AWS_ACCESS_KEY_ID }}' AWS_SECRET_ACCESS_KEY: '{{ AWS_SECRET_ACCESS_KEY }}' - {% if CBC_PROXY_AWS_ACCESS_KEY_ID is defined %} - CBC_PROXY_AWS_ACCESS_KEY_ID: '{{ CBC_PROXY_AWS_ACCESS_KEY_ID }}' - CBC_PROXY_AWS_SECRET_ACCESS_KEY: '{{ CBC_PROXY_AWS_SECRET_ACCESS_KEY }}' - {% endif %} - STATSD_HOST: "notify-statsd-exporter-{{ environment }}.apps.internal" ZENDESK_API_KEY: '{{ ZENDESK_API_KEY }}'