Better handling of absent AWS SES identity notification information. (#36354)

* Better handling of absent AWS SES identity notification information.

Fixes #36065

aws_ses_identity module now handles the cases where information about
the notification setup for the identity isn't returned by the AWS api.

This seems to happen in an edge case, believed to be eventual
consistency on registering new identities. So this case is treated
as if has been no notification setup for the identity yet.

Also fix 2 flake8 warnings in the module, a missing newline and unused
import.

* Increase the Boto Retries on SES APIs to deal with throttling.

This should address the unstable integration test failing due to
parallel runs in shippable hitting AWS throttling.

* Add retries loading SES details for inclusion in successful response.

There seems to be an eventual consistency behaviour with identity
registration. It's possible to still get no identity back after
registration.

This can cause failures in the shippable builds. This should fix that by
creating a retry of retrieving the identity information after
registration.

A similar retry loop has been added to notification attributes to ensure
this doesn't suffer from the same failure.

* Add missing sleep in get_notification_attributes to avoid busy loop.

lib/ansible/modules/cloud/amazon/aws_ses_identity.py

@@ -235,10 +235,13 @@ from ansible.module_utils.basic import AnsibleModule
 from ansible.module_utils.ec2 import camel_dict_to_snake_dict, ec2_argument_spec, get_aws_connection_info, boto3_conn
 from ansible.module_utils.ec2 import HAS_BOTO3
 
+import time
+
 import traceback
 
 try:
-    from botocore.exceptions import BotoCoreError, ClientError, ParamValidationError
+    from botocore.exceptions import BotoCoreError, ClientError
+    from botocore.config import Config
 except ImportError:
     pass  # caught by imported HAS_BOTO3
 
@@ -253,17 +256,54 @@ def call_and_handle_errors(module, method, **kwargs):
         module.fail_json(msg=str(e), exception=traceback.format_exc())
 
 
-def get_verification_attributes(connection, module, identity):
+def get_verification_attributes(connection, module, identity, retries=0, retryDelay=10):
-    response = call_and_handle_errors(module, connection.get_identity_verification_attributes, Identities=[identity])
+    # Unpredictably get_identity_verification_attributes doesn't include the identity even when we've
-    identity_verification = response['VerificationAttributes']
+    # just registered it. Suspect this is an eventual consistency issue on AWS side.
+    # Don't want this complexity exposed users of the module as they'd have to retry to ensure
+    # a consistent return from the module.
+    # To avoid this we have an internal retry that we use only after registering the identity.
+    for attempt in range(0, retries + 1):
+        response = call_and_handle_errors(module, connection.get_identity_verification_attributes, Identities=[identity])
+        identity_verification = response['VerificationAttributes']
+        if identity in identity_verification:
+            break
+        time.sleep(retryDelay)
     if identity not in identity_verification:
         return None
     return identity_verification[identity]
 
 
-def get_identity_notifications(connection, module, identity):
+def get_identity_notifications(connection, module, identity, retries=0, retryDelay=10):
-    response = call_and_handle_errors(module, connection.get_identity_notification_attributes, Identities=[identity])
+    # Unpredictably get_identity_notifications doesn't include the notifications when we've
-    notification_attributes = response['NotificationAttributes']
+    # just registered the identity.
+    # Don't want this complexity exposed users of the module as they'd have to retry to ensure
+    # a consistent return from the module.
+    # To avoid this we have an internal retry that we use only when getting the current notification
+    # status for return.
+    for attempt in range(0, retries + 1):
+        response = call_and_handle_errors(module, connection.get_identity_notification_attributes, Identities=[identity])
+        notification_attributes = response['NotificationAttributes']
+
+        # No clear AWS docs on when this happens, but it appears sometimes identities are not included in
+        # in the notification attributes when the identity is first registered. Suspect that this is caused by
+        # eventual consistency within the AWS services. It's been observed in builds so we need to handle it.
+        #
+        # When this occurs, just return None and we'll assume no identity notification settings have been changed
+        # from the default which is reasonable if this is just eventual consistency on creation.
+        # See: https://github.com/ansible/ansible/issues/36065
+        if identity in notification_attributes:
+            break
+        else:
+            # Paranoia check for coding errors, we only requested one identity, so if we get a different one
+            # something has gone very wrong.
+            if len(notification_attributes) != 0:
+                module.fail_json(
+                    msg='Unexpected identity found in notification attributes, expected {0} but got {1!r}.'.format(
+                        identity,
+                        notification_attributes.keys(),
+                    )
+                )
+        time.sleep(retryDelay)
     if identity not in notification_attributes:
         return None
     return notification_attributes[identity]
@@ -272,9 +312,17 @@ def get_identity_notifications(connection, module, identity):
 def update_notification_topic(connection, module, identity, identity_notifications, notification_type):
     arg_dict = module.params.get(notification_type.lower() + '_notifications')
     topic_key = notification_type + 'Topic'
-    if topic_key in identity_notifications:
+    if identity_notifications is None:
+        # If there is no configuration for notifications cannot be being sent to topics
+        # hence assume None as the current state.
+        current = None
+    elif topic_key in identity_notifications:
         current = identity_notifications[topic_key]
     else:
+        # If there is information on the notifications setup but no information on the
+        # particular notification topic it's pretty safe to assume there's no topic for
+        # this notification. AWS API docs suggest this information will always be
+        # included but best to be defensive
         current = None
 
     if arg_dict is not None and 'topic' in arg_dict:
@@ -297,9 +345,16 @@ def update_notification_topic(connection, module, identity, identity_notificatio
 def update_notification_topic_headers(connection, module, identity, identity_notifications, notification_type):
     arg_dict = module.params.get(notification_type.lower() + '_notifications')
     header_key = 'HeadersIn' + notification_type + 'NotificationsEnabled'
-    if header_key in identity_notifications:
+    if identity_notifications is None:
+        # If there is no configuration for topic notifications, headers cannot be being
+        # forwarded, hence assume false.
+        current = False
+    elif header_key in identity_notifications:
         current = identity_notifications[header_key]
     else:
+        # AWS API doc indicates that the headers in fields are optional. Unfortunately
+        # it's not clear on what this means. But it's a pretty safe assumption that it means
+        # headers are not included since most API consumers would interpret absence as false.
         current = False
 
     if arg_dict is not None and 'include_headers' in arg_dict:
@@ -320,9 +375,17 @@ def update_notification_topic_headers(connection, module, identity, identity_not
 
 
 def update_feedback_forwarding(connection, module, identity, identity_notifications):
-    if 'ForwardingEnabled' in identity_notifications:
+    if identity_notifications is None:
+        # AWS requires feedback forwarding to be enabled unless bounces and complaints
+        # are being handled by SNS topics. So in the absence of identity_notifications
+        # information existing feedback forwarding must be on.
+        current = True
+    elif 'ForwardingEnabled' in identity_notifications:
         current = identity_notifications['ForwardingEnabled']
     else:
+        # If there is information on the notifications setup but no information on the
+        # forwarding state it's pretty safe to assume forwarding is off. AWS API docs
+        # suggest this information will always be included but best to be defensive
         current = False
 
     required = module.params.get('feedback_forwarding')
@@ -349,8 +412,8 @@ def update_identity_notifications(connection, module):
 
     changed |= update_feedback_forwarding(connection, module, identity, identity_notifications)
 
-    if changed:
+    if changed or identity_notifications is None:
-        identity_notifications = get_identity_notifications(connection, module, identity)
+        identity_notifications = get_identity_notifications(connection, module, identity, retries=4)
     return changed, identity_notifications
 
 
@@ -363,15 +426,21 @@ def create_or_update_identity(connection, module, region, account_id):
             call_and_handle_errors(module, connection.verify_email_identity, EmailAddress=identity)
         else:
             call_and_handle_errors(module, connection.verify_domain_identity, Domain=identity)
-        verification_attributes = get_verification_attributes(connection, module, identity)
+        verification_attributes = get_verification_attributes(connection, module, identity, retries=4)
         changed = True
     elif verification_attributes['VerificationStatus'] not in ('Pending', 'Success'):
         module.fail_json(msg="Identity " + identity + " in bad status " + verification_attributes['VerificationStatus'],
                          verification_attributes=camel_dict_to_snake_dict(verification_attributes))
 
+    if verification_attributes is None:
+        module.fail_json(msg='Unable to load identity verification attributes after registering identity.')
+
     notifications_changed, notification_attributes = update_identity_notifications(connection, module)
     changed |= notifications_changed
 
+    if notification_attributes is None:
+        module.fail_json(msg='Unable to load identity notification attributes.')
+
     identity_arn = 'arn:aws:ses:' + region + ':' + account_id + ':identity/' + identity
 
     module.exit_json(
@@ -433,7 +502,15 @@ def main():
 
     region, ec2_url, aws_connect_params = get_aws_connection_info(module, boto3=True)
 
-    connection = boto3_conn(module, conn_type='client', resource='ses', region=region, endpoint=ec2_url, **aws_connect_params)
+    # Allow up to 10 attempts to call the SES APIs before giving up (9 retries).
+    # SES APIs seem to have a much lower throttling threshold than most of the rest of the AWS APIs.
+    # Docs say 1 call per second. This shouldn't actually be a big problem for normal usage, but
+    # the ansible build runs multiple instances of the test in parallel.
+    # As a result there are build failures due to throttling that exceeds boto's default retries.
+    # The back-off is exponential, so upping the retry attempts allows multiple parallel runs
+    # to succeed.
+    boto_core_config = Config(retries={'max_attempts': 9})
+    connection = boto3_conn(module, conn_type='client', resource='ses', region=region, endpoint=ec2_url, config=boto_core_config, **aws_connect_params)
 
     state = module.params.get("state")
 
@@ -444,5 +521,6 @@ def main():
     else:
         destroy_identity(connection, module)
 
+
 if __name__ == '__main__':
     main()