From b6774bf0f7853c145421c6fdd9e70f7b5928fc98 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 19 Jul 2021 12:48:18 +0100 Subject: [PATCH 1/7] Generate Vodafone link test sequence nos in proxy Previously the Celery task to trigger a link test had to know about the special case of a sequence number for Vodafone. Since we're about to change the client to perform multiple tests it makes sense to give it the knowledge of how to generate number itself. Note that we have to import the db inline to avoid a circular import, since this module is itself imported by app/__init__.py. Other invocations of the Vodafone client use stored sequence numbers from the DB, which are called "message numbers" in that context. Since the two use cases are very different (even the names are different!), having them in two places shouldn't cause any confusion. --- app/celery/broadcast_message_tasks.py | 10 ++------- app/clients/cbc_proxy.py | 11 ++++++---- .../celery/test_broadcast_message_tasks.py | 22 ++++--------------- tests/app/clients/test_cbc_proxy.py | 9 +++++--- 4 files changed, 19 insertions(+), 33 deletions(-) diff --git a/app/celery/broadcast_message_tasks.py b/app/celery/broadcast_message_tasks.py index fcdd8e89b..07cced707 100644 --- a/app/celery/broadcast_message_tasks.py +++ b/app/celery/broadcast_message_tasks.py @@ -2,9 +2,8 @@ import uuid from datetime import datetime from flask import current_app -from sqlalchemy.schema import Sequence -from app import cbc_proxy_client, db, notify_celery, zendesk_client +from app import cbc_proxy_client, notify_celery, zendesk_client from app.clients.cbc_proxy import CBCProxyRetryableException from app.config import QueueNames from app.dao.broadcast_message_dao import ( @@ -252,11 +251,6 @@ def send_broadcast_provider_message(self, broadcast_event_id, provider): @notify_celery.task(name='trigger-link-test') def trigger_link_test(provider): identifier = str(uuid.uuid4()) - formatted_seq_number = None - if provider == BroadcastProvider.VODAFONE: - sequence = Sequence('broadcast_provider_message_number_seq') - sequential_number = db.session.connection().execute(sequence) - formatted_seq_number = format_sequential_number(sequential_number) message = f"Sending a link test to CBC proxy for provider {provider}. Identifier in payload is {identifier}" current_app.logger.info(message) - cbc_proxy_client.get_proxy(provider).send_link_test(identifier, formatted_seq_number) + cbc_proxy_client.get_proxy(provider).send_link_test(identifier) diff --git a/app/clients/cbc_proxy.py b/app/clients/cbc_proxy.py index 63a69f892..e475ab8fd 100644 --- a/app/clients/cbc_proxy.py +++ b/app/clients/cbc_proxy.py @@ -5,6 +5,7 @@ import boto3 import botocore from flask import current_app from notifications_utils.template import non_gsm_characters +from sqlalchemy.schema import Sequence from app.config import BroadcastProvider from app.utils import DATETIME_FORMAT, format_sequential_number @@ -79,7 +80,6 @@ class CBCProxyClientBase(ABC): def send_link_test( self, identifier, - sequential_number ): pass @@ -158,7 +158,6 @@ class CBCProxyOne2ManyClient(CBCProxyClientBase): def send_link_test( self, identifier, - sequential_number=None, ): """ link test - open up a connection to a specific provider, and send them an xml payload with a of @@ -234,16 +233,20 @@ class CBCProxyVodafone(CBCProxyClientBase): def send_link_test( self, identifier, - sequential_number, ): """ link test - open up a connection to a specific provider, and send them an xml payload with a of test. """ + from app import db + sequence = Sequence('broadcast_provider_message_number_seq') + sequential_number = db.session.connection().execute(sequence) + formatted_seq_number = format_sequential_number(sequential_number) + payload = { 'message_type': 'test', 'identifier': identifier, - 'message_number': sequential_number, + 'message_number': formatted_seq_number, 'message_format': 'ibag' } diff --git a/tests/app/celery/test_broadcast_message_tasks.py b/tests/app/celery/test_broadcast_message_tasks.py index cb6da59fb..3e44fd18a 100644 --- a/tests/app/celery/test_broadcast_message_tasks.py +++ b/tests/app/celery/test_broadcast_message_tasks.py @@ -1,4 +1,3 @@ -import uuid from datetime import datetime from unittest.mock import ANY, Mock, call @@ -561,29 +560,16 @@ def test_send_broadcast_provider_message_delays_retry_exponentially( ['vodafone', 'Vodafone'], ]) def test_trigger_link_tests_invokes_cbc_proxy_client( - mocker, provider, provider_capitalised + mocker, provider, provider_capitalised, client, ): mock_send_link_test = mocker.patch( f'app.clients.cbc_proxy.CBCProxy{provider_capitalised}.send_link_test', ) + mocker.patch('app.celery.broadcast_message_tasks.uuid.uuid4', return_value=123) + trigger_link_test(provider) - - assert mock_send_link_test.called - # the 0th argument of the call to send_link_test - identifier = mock_send_link_test.mock_calls[0][1][0] - - try: - uuid.UUID(identifier) - except BaseException: - pytest.fail(f"{identifier} is not a valid uuid") - - # testing sequential number: - if provider == 'vodafone': - assert type(mock_send_link_test.mock_calls[0][1][1]) is str - assert len(mock_send_link_test.mock_calls[0][1][1]) == 8 - else: - assert not mock_send_link_test.mock_calls[0][1][1] + assert mock_send_link_test.called_once_with('123') @pytest.mark.parametrize('retry_count, expected_delay', [ diff --git a/tests/app/clients/test_cbc_proxy.py b/tests/app/clients/test_cbc_proxy.py index 708fe99f2..c3261a2dd 100644 --- a/tests/app/clients/test_cbc_proxy.py +++ b/tests/app/clients/test_cbc_proxy.py @@ -8,6 +8,7 @@ from unittest.mock import Mock, call import pytest from botocore.exceptions import ClientError as BotoClientError +from app import db from app.clients.cbc_proxy import ( CBCProxyClient, CBCProxyEE, @@ -573,7 +574,6 @@ def test_cbc_proxy_one_2_many_send_link_test_invokes_function(mocker, cbc_proxy_ cbc_proxy.send_link_test( identifier=identifier, - sequential_number='0000007b', ) ld_client_mock.invoke.assert_called_once_with( @@ -595,6 +595,10 @@ def test_cbc_proxy_one_2_many_send_link_test_invokes_function(mocker, cbc_proxy_ def test_cbc_proxy_vodafone_send_link_test_invokes_function(mocker, cbc_proxy_vodafone): identifier = str(uuid.uuid4()) + db.session.connection().execute( + 'ALTER SEQUENCE broadcast_provider_message_number_seq RESTART WITH 1' + ) + ld_client_mock = mocker.patch.object( cbc_proxy_vodafone, '_lambda_client', @@ -607,7 +611,6 @@ def test_cbc_proxy_vodafone_send_link_test_invokes_function(mocker, cbc_proxy_vo cbc_proxy_vodafone.send_link_test( identifier=identifier, - sequential_number='0000007b', ) ld_client_mock.invoke.assert_called_once_with( @@ -622,5 +625,5 @@ def test_cbc_proxy_vodafone_send_link_test_invokes_function(mocker, cbc_proxy_vo assert payload['identifier'] == identifier assert payload['message_type'] == 'test' - assert payload['message_number'] == '0000007b' + assert payload['message_number'] == '00000001' assert payload['message_format'] == 'ibag' From 1e8eda0d15b7a18a1db58ad3fd6b64be5c7eab20 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 19 Jul 2021 14:28:13 +0100 Subject: [PATCH 2/7] Avoid failover when running link tests We want to get the point where we're running link tests for each lambda independently. The tests weren't checking for the failover mechanism for link tests, so we can just remove it. --- app/clients/cbc_proxy.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/clients/cbc_proxy.py b/app/clients/cbc_proxy.py index e475ab8fd..df2e24395 100644 --- a/app/clients/cbc_proxy.py +++ b/app/clients/cbc_proxy.py @@ -169,7 +169,7 @@ class CBCProxyOne2ManyClient(CBCProxyClientBase): 'message_format': 'cap' } - self._invoke_lambda_with_failover(payload=payload) + self._invoke_lambda(lambda_name=self.lambda_name, payload=payload) def create_and_send_broadcast( self, identifier, headline, description, areas, sent, expires, channel, message_number=None @@ -250,7 +250,7 @@ class CBCProxyVodafone(CBCProxyClientBase): 'message_format': 'ibag' } - self._invoke_lambda_with_failover(payload=payload) + self._invoke_lambda(lambda_name=self.lambda_name, payload=payload) def create_and_send_broadcast( self, identifier, message_number, headline, description, areas, sent, expires, channel From 7fb65761f95c74404b69d846222a074bfd14d8a7 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 19 Jul 2021 14:34:35 +0100 Subject: [PATCH 3/7] Always log when a lambda is invoked This replaces the previous single-purpose log about a link test with a more informative and generic one for all invocations. --- app/celery/broadcast_message_tasks.py | 2 -- app/clients/cbc_proxy.py | 4 ++++ 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/app/celery/broadcast_message_tasks.py b/app/celery/broadcast_message_tasks.py index 07cced707..bd5340562 100644 --- a/app/celery/broadcast_message_tasks.py +++ b/app/celery/broadcast_message_tasks.py @@ -251,6 +251,4 @@ def send_broadcast_provider_message(self, broadcast_event_id, provider): @notify_celery.task(name='trigger-link-test') def trigger_link_test(provider): identifier = str(uuid.uuid4()) - message = f"Sending a link test to CBC proxy for provider {provider}. Identifier in payload is {identifier}" - current_app.logger.info(message) cbc_proxy_client.get_proxy(provider).send_link_test(identifier) diff --git a/app/clients/cbc_proxy.py b/app/clients/cbc_proxy.py index df2e24395..3360bf0a9 100644 --- a/app/clients/cbc_proxy.py +++ b/app/clients/cbc_proxy.py @@ -118,6 +118,10 @@ class CBCProxyClientBase(ABC): def _invoke_lambda(self, lambda_name, payload): payload_bytes = bytes(json.dumps(payload), encoding='utf8') try: + current_app.logger.info( + f"Calling lambda {lambda_name} for link test with payload {payload}" + ) + result = self._lambda_client.invoke( FunctionName=lambda_name, InvocationType='RequestResponse', From 08f48379b4dd389f6b9056f386e2423d59ee43f0 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 19 Jul 2021 14:38:27 +0100 Subject: [PATCH 4/7] Move ID generation into link test method Unlike the other IDs which are stored in the DB, this isn't relevant for the Celery task as it invokes a link test. Moving it into the proxy client will also enable us to generate a second ID in the next commits, where we start doing a link test for the failover lambda. --- app/celery/broadcast_message_tasks.py | 4 +-- app/clients/cbc_proxy.py | 27 ++++++++++--------- .../celery/test_broadcast_message_tasks.py | 4 +-- tests/app/clients/test_cbc_proxy.py | 24 +++++++++++------ 4 files changed, 32 insertions(+), 27 deletions(-) diff --git a/app/celery/broadcast_message_tasks.py b/app/celery/broadcast_message_tasks.py index bd5340562..05b5ba7da 100644 --- a/app/celery/broadcast_message_tasks.py +++ b/app/celery/broadcast_message_tasks.py @@ -1,4 +1,3 @@ -import uuid from datetime import datetime from flask import current_app @@ -250,5 +249,4 @@ def send_broadcast_provider_message(self, broadcast_event_id, provider): @notify_celery.task(name='trigger-link-test') def trigger_link_test(provider): - identifier = str(uuid.uuid4()) - cbc_proxy_client.get_proxy(provider).send_link_test(identifier) + cbc_proxy_client.get_proxy(provider).send_link_tests() diff --git a/app/clients/cbc_proxy.py b/app/clients/cbc_proxy.py index 3360bf0a9..b6216effe 100644 --- a/app/clients/cbc_proxy.py +++ b/app/clients/cbc_proxy.py @@ -1,4 +1,5 @@ import json +import uuid from abc import ABC, abstractmethod import boto3 @@ -77,11 +78,11 @@ class CBCProxyClientBase(ABC): def __init__(self, lambda_client): self._lambda_client = lambda_client - def send_link_test( - self, - identifier, - ): - pass + def send_link_tests(self): + self._send_link_test(self.lambda_name) + self._send_link_test(self.failover_lambda_name) + + def _send_link_test(self): pass def create_and_send_broadcast( self, identifier, headline, description, areas, sent, expires, channel, message_number=None @@ -159,9 +160,9 @@ class CBCProxyOne2ManyClient(CBCProxyClientBase): LANGUAGE_ENGLISH = 'en-GB' LANGUAGE_WELSH = 'cy-GB' - def send_link_test( + def _send_link_test( self, - identifier, + lambda_name, ): """ link test - open up a connection to a specific provider, and send them an xml payload with a of @@ -169,11 +170,11 @@ class CBCProxyOne2ManyClient(CBCProxyClientBase): """ payload = { 'message_type': 'test', - 'identifier': identifier, + 'identifier': str(uuid.uuid4()), 'message_format': 'cap' } - self._invoke_lambda(lambda_name=self.lambda_name, payload=payload) + self._invoke_lambda(lambda_name=lambda_name, payload=payload) def create_and_send_broadcast( self, identifier, headline, description, areas, sent, expires, channel, message_number=None @@ -234,9 +235,9 @@ class CBCProxyVodafone(CBCProxyClientBase): LANGUAGE_ENGLISH = 'English' LANGUAGE_WELSH = 'Welsh' - def send_link_test( + def _send_link_test( self, - identifier, + lambda_name, ): """ link test - open up a connection to a specific provider, and send them an xml payload with a of @@ -249,12 +250,12 @@ class CBCProxyVodafone(CBCProxyClientBase): payload = { 'message_type': 'test', - 'identifier': identifier, + 'identifier': str(uuid.uuid4()), 'message_number': formatted_seq_number, 'message_format': 'ibag' } - self._invoke_lambda(lambda_name=self.lambda_name, payload=payload) + self._invoke_lambda(lambda_name=lambda_name, payload=payload) def create_and_send_broadcast( self, identifier, message_number, headline, description, areas, sent, expires, channel diff --git a/tests/app/celery/test_broadcast_message_tasks.py b/tests/app/celery/test_broadcast_message_tasks.py index 3e44fd18a..fce2d2e91 100644 --- a/tests/app/celery/test_broadcast_message_tasks.py +++ b/tests/app/celery/test_broadcast_message_tasks.py @@ -566,10 +566,8 @@ def test_trigger_link_tests_invokes_cbc_proxy_client( f'app.clients.cbc_proxy.CBCProxy{provider_capitalised}.send_link_test', ) - mocker.patch('app.celery.broadcast_message_tasks.uuid.uuid4', return_value=123) - trigger_link_test(provider) - assert mock_send_link_test.called_once_with('123') + assert mock_send_link_test.called_once() @pytest.mark.parametrize('retry_count, expected_delay', [ diff --git a/tests/app/clients/test_cbc_proxy.py b/tests/app/clients/test_cbc_proxy.py index c3261a2dd..f067c47c0 100644 --- a/tests/app/clients/test_cbc_proxy.py +++ b/tests/app/clients/test_cbc_proxy.py @@ -82,6 +82,14 @@ def test_cbc_proxy_lambda_client_has_correct_keys(cbc_proxy_ee): assert secret == 'cbc-proxy-aws-secret-access-key' +def test_cbc_proxy_send_link_tests(mocker, cbc_proxy_ee): + mock_send_link_test = mocker.patch.object(cbc_proxy_ee, '_send_link_test') + cbc_proxy_ee.send_link_tests() + + mock_send_link_test.assert_any_call(cbc_proxy_ee.lambda_name) + mock_send_link_test.assert_any_call(cbc_proxy_ee.failover_lambda_name) + + @pytest.mark.parametrize('description, expected_language', ( ('my-description', 'en-GB'), ('mŷ-description', 'cy-GB'), @@ -560,7 +568,7 @@ def test_cbc_proxy_create_and_send_tries_failover_lambda_on_function_error_and_r def test_cbc_proxy_one_2_many_send_link_test_invokes_function(mocker, cbc_proxy_client, cbc): cbc_proxy = cbc_proxy_client.get_proxy(cbc) - identifier = str(uuid.uuid4()) + mocker.patch('app.clients.cbc_proxy.uuid.uuid4', return_value=123) ld_client_mock = mocker.patch.object( cbc_proxy, @@ -572,8 +580,8 @@ def test_cbc_proxy_one_2_many_send_link_test_invokes_function(mocker, cbc_proxy_ 'StatusCode': 200, } - cbc_proxy.send_link_test( - identifier=identifier, + cbc_proxy._send_link_test( + lambda_name=f'{cbc}-1-proxy' ) ld_client_mock.invoke.assert_called_once_with( @@ -586,14 +594,14 @@ def test_cbc_proxy_one_2_many_send_link_test_invokes_function(mocker, cbc_proxy_ payload_bytes = kwargs['Payload'] payload = json.loads(payload_bytes) - assert payload['identifier'] == identifier + assert payload['identifier'] == '123' assert payload['message_type'] == 'test' assert 'message_number' not in payload assert payload['message_format'] == 'cap' def test_cbc_proxy_vodafone_send_link_test_invokes_function(mocker, cbc_proxy_vodafone): - identifier = str(uuid.uuid4()) + mocker.patch('app.clients.cbc_proxy.uuid.uuid4', return_value=123) db.session.connection().execute( 'ALTER SEQUENCE broadcast_provider_message_number_seq RESTART WITH 1' @@ -609,8 +617,8 @@ def test_cbc_proxy_vodafone_send_link_test_invokes_function(mocker, cbc_proxy_vo 'StatusCode': 200, } - cbc_proxy_vodafone.send_link_test( - identifier=identifier, + cbc_proxy_vodafone._send_link_test( + lambda_name='vodafone-1-proxy' ) ld_client_mock.invoke.assert_called_once_with( @@ -623,7 +631,7 @@ def test_cbc_proxy_vodafone_send_link_test_invokes_function(mocker, cbc_proxy_vo payload_bytes = kwargs['Payload'] payload = json.loads(payload_bytes) - assert payload['identifier'] == identifier + assert payload['identifier'] == '123' assert payload['message_type'] == 'test' assert payload['message_number'] == '00000001' assert payload['message_format'] == 'ibag' From cdc150de1b92eb9a4dc393948ccf4e33fce91ee4 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 19 Jul 2021 15:09:55 +0100 Subject: [PATCH 5/7] Change link test task to trigger both lambda This modifies the previous "(_)send_link_test" method to trigger a link test for a specific lambda. We then call the method with both the primary and failover lambda in new orchestrator method. Since the _invoke_lambda function doesn't raise exceptions if it fails, there's no need to rescue anything in order to ensure the second link test / invocation runs as well. It doesn't testing for this, since it boils to an absence of code to raise any exception. Note that, like the other parent tests, we only check the new method works with a specific proxy client instance. --- app/celery/broadcast_message_tasks.py | 2 +- app/clients/cbc_proxy.py | 7 +++++-- tests/app/clients/test_cbc_proxy.py | 4 ++-- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/app/celery/broadcast_message_tasks.py b/app/celery/broadcast_message_tasks.py index 05b5ba7da..8a5d2ef99 100644 --- a/app/celery/broadcast_message_tasks.py +++ b/app/celery/broadcast_message_tasks.py @@ -249,4 +249,4 @@ def send_broadcast_provider_message(self, broadcast_event_id, provider): @notify_celery.task(name='trigger-link-test') def trigger_link_test(provider): - cbc_proxy_client.get_proxy(provider).send_link_tests() + cbc_proxy_client.get_proxy(provider).send_link_test() diff --git a/app/clients/cbc_proxy.py b/app/clients/cbc_proxy.py index b6216effe..60f30f0ae 100644 --- a/app/clients/cbc_proxy.py +++ b/app/clients/cbc_proxy.py @@ -78,11 +78,14 @@ class CBCProxyClientBase(ABC): def __init__(self, lambda_client): self._lambda_client = lambda_client - def send_link_tests(self): + def send_link_test(self): self._send_link_test(self.lambda_name) self._send_link_test(self.failover_lambda_name) - def _send_link_test(self): pass + def _send_link_test( + self, + lambda_name, + ): pass def create_and_send_broadcast( self, identifier, headline, description, areas, sent, expires, channel, message_number=None diff --git a/tests/app/clients/test_cbc_proxy.py b/tests/app/clients/test_cbc_proxy.py index f067c47c0..83fa994fc 100644 --- a/tests/app/clients/test_cbc_proxy.py +++ b/tests/app/clients/test_cbc_proxy.py @@ -82,9 +82,9 @@ def test_cbc_proxy_lambda_client_has_correct_keys(cbc_proxy_ee): assert secret == 'cbc-proxy-aws-secret-access-key' -def test_cbc_proxy_send_link_tests(mocker, cbc_proxy_ee): +def test_cbc_proxy_send_link_test(mocker, cbc_proxy_ee): mock_send_link_test = mocker.patch.object(cbc_proxy_ee, '_send_link_test') - cbc_proxy_ee.send_link_tests() + cbc_proxy_ee.send_link_test() mock_send_link_test.assert_any_call(cbc_proxy_ee.lambda_name) mock_send_link_test.assert_any_call(cbc_proxy_ee.failover_lambda_name) From d1791c6bfe8ae5125b91cd73aa6c67e6fca22317 Mon Sep 17 00:00:00 2001 From: Richard Baker Date: Tue, 20 Jul 2021 15:35:50 +0100 Subject: [PATCH 6/7] Remove "link test" from cbc-proxy log output The `_invoke_lambda` function is called when sending both link test and real message types. Signed-off-by: Richard Baker --- app/clients/cbc_proxy.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/clients/cbc_proxy.py b/app/clients/cbc_proxy.py index 60f30f0ae..a1a5e24cd 100644 --- a/app/clients/cbc_proxy.py +++ b/app/clients/cbc_proxy.py @@ -123,7 +123,7 @@ class CBCProxyClientBase(ABC): payload_bytes = bytes(json.dumps(payload), encoding='utf8') try: current_app.logger.info( - f"Calling lambda {lambda_name} for link test with payload {payload}" + f"Calling lambda {lambda_name} with payload {payload}" ) result = self._lambda_client.invoke( From d24b45bb672c0bbb04bf5f6bd20b5c530da29d41 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Tue, 20 Jul 2021 16:06:48 +0100 Subject: [PATCH 7/7] Constrain log length for CBC proxy payload This avoids any issues due to large payloads (e.g. with a lot of polygons in the 'areas' field). While we may miss part of the log in such cases, this is more than we get already anyway. --- app/clients/cbc_proxy.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/clients/cbc_proxy.py b/app/clients/cbc_proxy.py index a1a5e24cd..89ec65db3 100644 --- a/app/clients/cbc_proxy.py +++ b/app/clients/cbc_proxy.py @@ -123,7 +123,7 @@ class CBCProxyClientBase(ABC): payload_bytes = bytes(json.dumps(payload), encoding='utf8') try: current_app.logger.info( - f"Calling lambda {lambda_name} with payload {payload}" + f"Calling lambda {lambda_name} with payload {str(payload)[:1000]}" ) result = self._lambda_client.invoke(