From b59e4df06d52d1d7faf49d69cccae6e3f02fbde3 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Fri, 5 May 2023 08:09:15 -0700 Subject: [PATCH] code review feedback --- app/__init__.py | 1 + app/celery/provider_tasks.py | 19 +++++++++++------- app/clients/cloudwatch/__init__.py | 25 ------------------------ app/clients/cloudwatch/aws_cloudwatch.py | 16 +++++---------- app/cloudfoundry_config.py | 9 +++++++++ 5 files changed, 27 insertions(+), 43 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index f39be3c22..81e5c055a 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -98,6 +98,7 @@ def create_app(application): aws_ses_stub_client.init_app( stub_url=application.config['SES_STUB_URL'] ) + aws_cloudwatch_client.init_app(application) # If a stub url is provided for SES, then use the stub client rather than the real SES boto client email_clients = [aws_ses_stub_client] if application.config['SES_STUB_URL'] else [aws_ses_client] notification_provider_clients.init_app( diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index bee645095..1fe30c92c 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -4,8 +4,7 @@ from zoneinfo import ZoneInfo from flask import current_app from sqlalchemy.orm.exc import NoResultFound -from app import notify_celery -from app.clients.cloudwatch.aws_cloudwatch import AwsCloudwatchClient +from app import aws_cloudwatch_client, notify_celery from app.clients.email import EmailClientNonRetryableException from app.clients.email.aws_ses import AwsSesClientThrottlingSendRateException from app.clients.sms import SmsClientResponseException @@ -21,12 +20,18 @@ from app.models import ( ) -@notify_celery.task(bind=True, name="check_sms_delivery_receipt", max_retries=3, default_retry_delay=300) +@notify_celery.task(bind=True, name="check_sms_delivery_receipt", max_retries=48, default_retry_delay=300) def check_sms_delivery_receipt(self, message_id, notification_id): - current_app.logger.warning(f"CHECKING DELIVERY RECEIPT for {message_id} {notification_id}") - cloudwatch_client = AwsCloudwatchClient() - cloudwatch_client.init_app(current_app) - status, provider_response = cloudwatch_client.check_sms(message_id, notification_id) + """ + This is called after deliver_sms to check the status of the message. This uses the same number of + retries and the same delay period as deliver_sms. In addition, this fires five minutes after + deliver_sms initially. So the idea is that most messages will succeed and show up in the logs quickly. + Other message will resolve successfully after a retry or to. A few will fail but it will take up to + 4 hours to know for sure. The call to check_sms will raise an exception if neither a success nor a + failure appears in the cloudwatch logs, so this should keep retrying until the log appears, or until + we run out of retries. + """ + status, provider_response = aws_cloudwatch_client.check_sms(message_id, notification_id) if status == 'success': status = NOTIFICATION_SENT else: diff --git a/app/clients/cloudwatch/__init__.py b/app/clients/cloudwatch/__init__.py index 31c79f889..e69de29bb 100644 --- a/app/clients/cloudwatch/__init__.py +++ b/app/clients/cloudwatch/__init__.py @@ -1,25 +0,0 @@ -from app.clients import Client, ClientException - - -class CloudwatchClientResponseException(ClientException): - """ - Base Exception for SmsClientsResponses - """ - - def __init__(self, message): - self.message = message - - def __str__(self): - return "Message {}".format(self.message) - - -class CloudwatchClient(Client): - """ - Base Cloudwatch client for checking sms. - """ - - def init_app(self, *args, **kwargs): - raise NotImplementedError("TODO Need to implement.") - - def check_sms(self, *args, **kwargs): - raise NotImplementedError("TODO Need to implement.") diff --git a/app/clients/cloudwatch/aws_cloudwatch.py b/app/clients/cloudwatch/aws_cloudwatch.py index 57960c40a..c6098bef5 100644 --- a/app/clients/cloudwatch/aws_cloudwatch.py +++ b/app/clients/cloudwatch/aws_cloudwatch.py @@ -5,11 +5,11 @@ import time from boto3 import client -from app.clients.cloudwatch import CloudwatchClient +from app.clients import Client from app.cloudfoundry_config import cloud_config -class AwsCloudwatchClient(CloudwatchClient): +class AwsCloudwatchClient(Client): """ This client is responsible for retrieving sms delivery receipts from cloudwatch. """ @@ -21,7 +21,7 @@ class AwsCloudwatchClient(CloudwatchClient): aws_access_key_id=cloud_config.sns_access_key, aws_secret_access_key=cloud_config.sns_secret_key ) - super(CloudwatchClient, self).__init__(*args, **kwargs) + super(Client, self).__init__(*args, **kwargs) self.current_app = current_app self._valid_sender_regex = re.compile(r"^\+?\d{5,14}$") @@ -61,14 +61,8 @@ class AwsCloudwatchClient(CloudwatchClient): return all_log_events def check_sms(self, message_id, notification_id): - """ - Go through the cloudwatch logs, filtering by message id. Check the success logs first. If we find - the message id there, we are done. Otherwise check the failure logs. If we don't find the message - in the success or failure logs, raise an exception. This method is called on a five minute delay, - which is presumably enough time for the cloudwatch log to be populated. - """ - # TODO presumably there is a better way to get the aws account number - account_number = os.getenv("SES_DOMAIN_ARN") + # TODO this clumsy approach to getting the account number will be fixed as part of notify-api #258 + account_number = cloud_config.ses_domain_arn account_number = account_number.replace('arn:aws:ses:us-west-2:', '') account_number = account_number.split(":") account_number = account_number[0] diff --git a/app/cloudfoundry_config.py b/app/cloudfoundry_config.py index 7fda0184d..62527c797 100644 --- a/app/cloudfoundry_config.py +++ b/app/cloudfoundry_config.py @@ -39,6 +39,15 @@ class CloudfoundryConfig: domain_arn = getenv('SES_DOMAIN_ARN', 'dev.notify.gov') return domain_arn.split('/')[-1] + # TODO remove this after notifications-api #258 + @property + def ses_domain_arn(self): + try: + domain_arn = self._ses_credentials('domain_arn') + except KeyError: + domain_arn = getenv('SES_DOMAIN_ARN', 'dev.notify.gov') + return domain_arn + @property def ses_region(self): try: