From 2079202160ea2cdf87fb84e6fb72f73aeba0f7db Mon Sep 17 00:00:00 2001 From: David McDonald Date: Wed, 30 Dec 2020 17:03:17 +0000 Subject: [PATCH 1/4] Stop logging email addresses for SES errors We shouldn't be logging PII so we should not log email addresses. We remove the email address and just log the normal exception message. Note, this meant before that you could see the email address and more easily track down the notification ID in the database. Now instead, you will need to search in the DB for notifications that have gone into technical failure at the time of the log message (as we still don't log the notification ID alongside the failure). --- app/clients/email/aws_ses.py | 5 +---- tests/app/clients/test_aws_ses.py | 1 - 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/app/clients/email/aws_ses.py b/app/clients/email/aws_ses.py index 7c765288f..7e5337e77 100644 --- a/app/clients/email/aws_ses.py +++ b/app/clients/email/aws_ses.py @@ -122,10 +122,7 @@ class AwsSesClient(EmailClient): # http://docs.aws.amazon.com/ses/latest/DeveloperGuide/api-error-codes.html if e.response['Error']['Code'] == 'InvalidParameterValue': - raise InvalidEmailError('email: "{}" message: "{}"'.format( - to_addresses[0], - e.response['Error']['Message'] - )) + raise InvalidEmailError(str(e)) elif ( e.response['Error']['Code'] == 'Throttling' and e.response['Error']['Message'] == 'Maximum sending rate exceeded.' diff --git a/tests/app/clients/test_aws_ses.py b/tests/app/clients/test_aws_ses.py index 17e76aed0..bb34fa911 100644 --- a/tests/app/clients/test_aws_ses.py +++ b/tests/app/clients/test_aws_ses.py @@ -113,7 +113,6 @@ def test_send_email_raises_bad_email_as_InvalidEmailError(mocker): ) assert 'some error message from amazon' in str(excinfo.value) - assert 'definitely@invalid_email.com' in str(excinfo.value) def test_send_email_raises_send_rate_throttling_as_AwsSesClientThrottlingSendRateException(mocker): From 2480f91667ea6574fd196d87a025b386f95fd475 Mon Sep 17 00:00:00 2001 From: David McDonald Date: Wed, 30 Dec 2020 17:06:49 +0000 Subject: [PATCH 2/4] Raise better exception on InvalidParameterValue error There are several reasons why we might get an `InvalidParameterValue` from the SES API. One, as correctly identified before in https://github.com/alphagov/notifications-api/pull/713/files is if we allow an email address on our side that SES rejects. However, there are other types of errors that could cause an `InvalidParameterValue`. One example is a `Header too long: 'Subject'` error that we have seen happen in production. This shouldn't raise an `InvalidEmailError` as that is not appropriate. Therefore, we introduce a new exception `EmailClientNonRetryableException`, that represents any exception back from an email client that we can use whenever we get a `InvalidParameterValue` error. Note, I chose `EmailClientNonRetryableException` rather than `SESClientNonRetryableException` as our code needs to catch this exception and it shouldn't be aware of what email client is being used, it just needs to know that it came from one of the email clients (if in time we have more than one). In time, we may wish to extend the approach of having generic `EmailClient` exceptions and `SMSClient` exceptions as this should be the most extendable pattern and a good abstraction. --- app/celery/provider_tasks.py | 4 ++-- app/clients/email/__init__.py | 12 ++++++++++++ app/clients/email/aws_ses.py | 4 ++-- tests/app/celery/test_provider_tasks.py | 9 ++++++--- tests/app/clients/test_aws_ses.py | 6 +++--- 5 files changed, 25 insertions(+), 10 deletions(-) diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index 67a8f10d1..e9eed5222 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -1,10 +1,10 @@ from flask import current_app -from notifications_utils.recipients import InvalidEmailError from notifications_utils.statsd_decorators import statsd from sqlalchemy.orm.exc import NoResultFound from app import notify_celery from app.config import QueueNames +from app.clients.email import EmailClientNonRetryableException from app.clients.email.aws_ses import AwsSesClientThrottlingSendRateException from app.dao import notifications_dao from app.dao.notifications_dao import update_notification_status_by_id @@ -47,7 +47,7 @@ def deliver_email(self, notification_id): if not notification: raise NoResultFound() send_to_providers.send_email_to_provider(notification) - except InvalidEmailError as e: + except EmailClientNonRetryableException as e: current_app.logger.exception(e) update_notification_status_by_id(notification_id, 'technical-failure') except Exception as e: diff --git a/app/clients/email/__init__.py b/app/clients/email/__init__.py index b5ea93941..6db70a8af 100644 --- a/app/clients/email/__init__.py +++ b/app/clients/email/__init__.py @@ -8,6 +8,18 @@ class EmailClientException(ClientException): pass +class EmailClientNonRetryableException(ClientException): + ''' + Represents an error returned from the email client API with a 4xx response code + that should not be retried and should instead be marked as technical failure. + An example of this would be an email address that makes it through our + validation rules but is rejected by SES. There is no point in retrying this type as + it will always fail however many calls to SES. Whereas a throttling error would not + use this exception as it may succeed if we retry + ''' + pass + + class EmailClient(Client): ''' Base Email client for sending emails. diff --git a/app/clients/email/aws_ses.py b/app/clients/email/aws_ses.py index 7e5337e77..361936ec1 100644 --- a/app/clients/email/aws_ses.py +++ b/app/clients/email/aws_ses.py @@ -5,7 +5,7 @@ from time import monotonic from notifications_utils.recipients import InvalidEmailError from app.clients import STATISTICS_DELIVERED, STATISTICS_FAILURE -from app.clients.email import (EmailClientException, EmailClient) +from app.clients.email import EmailClient, EmailClientException, EmailClientNonRetryableException ses_response_map = { 'Permanent': { @@ -122,7 +122,7 @@ class AwsSesClient(EmailClient): # http://docs.aws.amazon.com/ses/latest/DeveloperGuide/api-error-codes.html if e.response['Error']['Code'] == 'InvalidParameterValue': - raise InvalidEmailError(str(e)) + raise EmailClientNonRetryableException(str(e)) elif ( e.response['Error']['Code'] == 'Throttling' and e.response['Error']['Message'] == 'Maximum sending rate exceeded.' diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index 10764e74f..ea30f950a 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -1,11 +1,11 @@ import pytest from botocore.exceptions import ClientError from celery.exceptions import MaxRetriesExceededError -from notifications_utils.recipients import InvalidEmailError import app from app.celery import provider_tasks from app.celery.provider_tasks import deliver_sms, deliver_email +from app.clients.email import EmailClientNonRetryableException from app.clients.email.aws_ses import AwsSesClientException, AwsSesClientThrottlingSendRateException from app.exceptions import NotificationTechnicalFailureException @@ -93,8 +93,11 @@ def test_should_go_into_technical_error_if_exceeds_retries_on_deliver_email_task assert sample_notification.status == 'technical-failure' -def test_should_technical_error_and_not_retry_if_invalid_email(sample_notification, mocker): - mocker.patch('app.delivery.send_to_providers.send_email_to_provider', side_effect=InvalidEmailError('bad email')) +def test_should_technical_error_and_not_retry_if_EmailClientNonRetryableException(sample_notification, mocker): + mocker.patch( + 'app.delivery.send_to_providers.send_email_to_provider', + side_effect=EmailClientNonRetryableException('bad email') + ) mocker.patch('app.celery.provider_tasks.deliver_email.retry') deliver_email(sample_notification.id) diff --git a/tests/app/clients/test_aws_ses.py b/tests/app/clients/test_aws_ses.py index bb34fa911..178b7c63c 100644 --- a/tests/app/clients/test_aws_ses.py +++ b/tests/app/clients/test_aws_ses.py @@ -1,9 +1,9 @@ import botocore import pytest from unittest.mock import Mock, ANY -from notifications_utils.recipients import InvalidEmailError from app import aws_ses_client +from app.clients.email import EmailClientNonRetryableException from app.clients.email.aws_ses import get_aws_responses, AwsSesClientException, AwsSesClientThrottlingSendRateException @@ -91,7 +91,7 @@ def test_send_email_handles_punycode_to_address(notify_api, mocker): ) -def test_send_email_raises_bad_email_as_InvalidEmailError(mocker): +def test_send_email_raises_invalid_parameter_value_error_as_EmailClientNonRetryableException(mocker): boto_mock = mocker.patch.object(aws_ses_client, '_client', create=True) mocker.patch.object(aws_ses_client, 'statsd_client', create=True) error_response = { @@ -104,7 +104,7 @@ def test_send_email_raises_bad_email_as_InvalidEmailError(mocker): boto_mock.send_email.side_effect = botocore.exceptions.ClientError(error_response, 'opname') mocker.patch.object(aws_ses_client, 'statsd_client', create=True) - with pytest.raises(InvalidEmailError) as excinfo: + with pytest.raises(EmailClientNonRetryableException) as excinfo: aws_ses_client.send_email( source=Mock(), to_addresses='definitely@invalid_email.com', From 977554781f043c4ac735fcdd41f50e74017a467f Mon Sep 17 00:00:00 2001 From: David McDonald Date: Wed, 30 Dec 2020 17:28:21 +0000 Subject: [PATCH 3/4] Add better logging message for tech failure So we can easily identify which notification ID failed --- app/celery/provider_tasks.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index e9eed5222..65878f1f6 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -48,7 +48,9 @@ def deliver_email(self, notification_id): raise NoResultFound() send_to_providers.send_email_to_provider(notification) except EmailClientNonRetryableException as e: - current_app.logger.exception(e) + current_app.logger.exception( + f"Email notification {notification_id} failed: {e}" + ) update_notification_status_by_id(notification_id, 'technical-failure') except Exception as e: try: From 56879d0d228d79cf1cc7a303312c743b899ecf52 Mon Sep 17 00:00:00 2001 From: David McDonald Date: Thu, 31 Dec 2020 11:08:09 +0000 Subject: [PATCH 4/4] Make sure error message is logged as part of the exception --- app/clients/email/aws_ses.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/clients/email/aws_ses.py b/app/clients/email/aws_ses.py index 361936ec1..f9d6e8723 100644 --- a/app/clients/email/aws_ses.py +++ b/app/clients/email/aws_ses.py @@ -122,7 +122,7 @@ class AwsSesClient(EmailClient): # http://docs.aws.amazon.com/ses/latest/DeveloperGuide/api-error-codes.html if e.response['Error']['Code'] == 'InvalidParameterValue': - raise EmailClientNonRetryableException(str(e)) + raise EmailClientNonRetryableException(e.response['Error']['Message']) elif ( e.response['Error']['Code'] == 'Throttling' and e.response['Error']['Message'] == 'Maximum sending rate exceeded.'