aws_acm Add additional AWSRetry error codes (#67671)

* Cleanup tests

* Auto-Retry on ResourceNotFound and RequestInProgress exceptions

* Use AnsibleModule options for required_if logic

* changelog

* Remove (now) duplicate RequestInProgressException catching

* Allow a single retry when attempting to fetch the information about a cert directly after deleting it.

There is a small chance that it goes away while we pull the details.

changelogs/fragments/67671-aws_acm-module_defaults.yaml

@@ -0,0 +1,3 @@
+minor_changes:
+- 'aws_acm: Add the module to group/aws for module_defaults.'
+- 'aws_acm: Update automatic retries to stabilize the integration tests.'

lib/ansible/config/module_defaults.yml

@@ -13,6 +13,8 @@ groupings:
   - acme
   acme_inspect:
   - acme
+  aws_acm:
+  - aws
   aws_acm_facts:
   - aws
   aws_acm_info:

lib/ansible/module_utils/aws/acm.py

@@ -51,7 +51,7 @@ class ACMServiceManager(object):
         region, ec2_url, aws_connect_kwargs = get_aws_connection_info(module, boto3=True)
         self.client = module.client('acm')
 
-    @AWSRetry.backoff(tries=5, delay=5, backoff=2.0)
+    @AWSRetry.backoff(tries=5, delay=5, backoff=2.0, catch_extra_error_codes=['RequestInProgressException'])
     def delete_certificate_with_backoff(self, client, arn):
         client.delete_certificate(CertificateArn=arn)
 
@@ -63,7 +63,7 @@ class ACMServiceManager(object):
             module.fail_json_aws(e, msg="Couldn't delete certificate %s" % arn)
         module.debug("Successfully deleted certificate %s" % arn)
 
-    @AWSRetry.backoff(tries=5, delay=5, backoff=2.0)
+    @AWSRetry.backoff(tries=5, delay=5, backoff=2.0, catch_extra_error_codes=['RequestInProgressException'])
     def list_certificates_with_backoff(self, client, statuses=None):
         paginator = client.get_paginator('list_certificates')
         kwargs = dict()
@@ -71,18 +71,18 @@ class ACMServiceManager(object):
             kwargs['CertificateStatuses'] = statuses
         return paginator.paginate(**kwargs).build_full_result()['CertificateSummaryList']
 
-    @AWSRetry.backoff(tries=5, delay=5, backoff=2.0, catch_extra_error_codes=['ResourceNotFoundException'])
+    @AWSRetry.backoff(tries=5, delay=5, backoff=2.0, catch_extra_error_codes=['ResourceNotFoundException', 'RequestInProgressException'])
     def get_certificate_with_backoff(self, client, certificate_arn):
         response = client.get_certificate(CertificateArn=certificate_arn)
         # strip out response metadata
         return {'Certificate': response['Certificate'],
                 'CertificateChain': response['CertificateChain']}
 
-    @AWSRetry.backoff(tries=5, delay=5, backoff=2.0, catch_extra_error_codes=['ResourceNotFoundException'])
+    @AWSRetry.backoff(tries=5, delay=5, backoff=2.0, catch_extra_error_codes=['ResourceNotFoundException', 'RequestInProgressException'])
     def describe_certificate_with_backoff(self, client, certificate_arn):
         return client.describe_certificate(CertificateArn=certificate_arn)['Certificate']
 
-    @AWSRetry.backoff(tries=5, delay=5, backoff=2.0, catch_extra_error_codes=['ResourceNotFoundException'])
+    @AWSRetry.backoff(tries=5, delay=5, backoff=2.0, catch_extra_error_codes=['ResourceNotFoundException', 'RequestInProgressException'])
     def list_certificate_tags_with_backoff(self, client, certificate_arn):
         return client.list_tags_for_certificate(CertificateArn=certificate_arn)['Tags']
 
@@ -118,7 +118,6 @@ class ACMServiceManager(object):
                 try:
                     cert_data.update(self.get_certificate_with_backoff(client, certificate['CertificateArn']))
                 except (BotoCoreError, ClientError, KeyError) as e:
-                    if e.response['Error']['Code'] != "RequestInProgressException":
                     module.fail_json_aws(e, msg="Couldn't obtain certificate data for domain %s" % certificate['DomainName'])
             cert_data = camel_dict_to_snake_dict(cert_data)
             try:
@@ -177,7 +176,7 @@ class ACMServiceManager(object):
 
     # Tags are a normal Ansible style dict
     # {'Key':'Value'}
-    @AWSRetry.backoff(tries=5, delay=5, backoff=2.0)
+    @AWSRetry.backoff(tries=5, delay=5, backoff=2.0, catch_extra_error_codes=['ResourceNotFoundException', 'RequestInProgressException'])
     def tag_certificate_with_backoff(self, client, arn, tags):
         aws_tags = ansible_dict_to_boto3_tag_list(tags)
         client.add_tags_to_certificate(CertificateArn=arn, Tags=aws_tags)

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

@@ -295,21 +295,17 @@ def main():
         private_key=dict(no_log=True),
         state=dict(default='present', choices=['present', 'absent'])
     )
-    module = AnsibleAWSModule(argument_spec=argument_spec, supports_check_mode=True)
+    required_if = [
+        ['state', 'present', ['certificate', 'name_tag', 'private_key']],
+    ]
+    module = AnsibleAWSModule(argument_spec=argument_spec, supports_check_mode=True, required_if=required_if)
     acm = ACMServiceManager(module)
 
     # Check argument requirements
     if module.params['state'] == 'present':
-        if not module.params['certificate']:
-            module.fail_json(msg="Parameter 'certificate' must be specified if 'state' is specified as 'present'")
-        elif module.params['certificate_arn']:
+        if module.params['certificate_arn']:
             module.fail_json(msg="Parameter 'certificate_arn' is only valid if parameter 'state' is specified as 'absent'")
-        elif not module.params['name_tag']:
-            module.fail_json(msg="Parameter 'name_tag' must be specified if parameter 'state' is specified as 'present'")
-        elif not module.params['private_key']:
-            module.fail_json(msg="Parameter 'private_key' must be specified if 'state' is specified as 'present'")
     else:  # absent
-
         # exactly one of these should be specified
         absent_args = ['certificate_arn', 'domain_name', 'name_tag']
         if sum([(module.params[a] is not None) for a in absent_args]) != 1:

test/integration/targets/aws_acm/tasks/full_acm_test.yml

@@ -1,25 +1,20 @@
 - name: AWS ACM integration test
-  block:
-
-  - set_fact:
-      aws_connection_info: &aws_connection_info
+  module_defaults:
+    group/aws:
       aws_region: "{{ aws_region }}"
       aws_access_key: "{{ aws_access_key }}"
       aws_secret_key: "{{ aws_secret_key }}"
-        security_token: "{{ security_token }}"
-      no_log: True
-
+      security_token: "{{ security_token | default(omit) }}"
+  block:
   # just check this task doesn't fail
   # I'm not sure if I can assume there aren't already other certs in this account
   - name: list certs
     aws_acm_info:
-      <<: *aws_connection_info
     register: list_all
     failed_when: list_all.certificates is not defined
 
   - name: ensure absent cert which doesn't exist - first time
     aws_acm:
-      <<: *aws_connection_info
       name_tag: "{{ item.name }}"
       state: absent
     with_items: "{{ local_certs }}"
@@ -28,7 +23,6 @@
   # check we don't fail when deleting nothing
   - name: ensure absent cert which doesn't exist - second time
     aws_acm:
-      <<: *aws_connection_info
       name_tag: "{{ item.name }}"
       state: absent
     with_items: "{{ local_certs }}"
@@ -37,7 +31,6 @@
 
   - name: list cert which shouldn't exist
     aws_acm_info:
-      <<: *aws_connection_info
       tags:
         Name: "{{ item.name }}"
     register: list_tag
@@ -79,7 +72,6 @@
   - name: upload certificates first time
     aws_acm:
       name_tag: "{{ item.name }}"
-      <<: *aws_connection_info
       certificate: "{{ lookup('file', item.cert ) }}"
       private_key: "{{ lookup('file', item.priv_key ) }}"
       state: present
@@ -104,7 +96,6 @@
   - name: fetch data about cert just uploaded, by ARN
     aws_acm_info:
       certificate_arn: "{{ item.certificate.arn }}"
-      <<: *aws_connection_info
     register: fetch_after_up
     with_items: "{{ upload.results }}"
 
@@ -129,7 +120,6 @@
     aws_acm_info:
       tags:
         Name: "{{ original_cert.name }}"
-      <<: *aws_connection_info
     register: fetch_after_up_name
     with_items: "{{ upload.results }}"
     vars:
@@ -157,7 +147,6 @@
   - name: fetch data about cert just uploaded, by domain name
     aws_acm_info:
       domain_name: "{{ original_cert.domain }}"
-      <<: *aws_connection_info
     register: fetch_after_up_domain
     with_items: "{{ upload.results }}"
     vars:
@@ -185,7 +174,6 @@
   - name: upload certificates again, check not changed
     aws_acm:
       name_tag: "{{ item.name }}"
-      <<: *aws_connection_info
       certificate: "{{ lookup('file', item.cert ) }}"
       private_key: "{{ lookup('file', item.priv_key ) }}"
       state: present
@@ -197,7 +185,6 @@
   - name: update first cert with body of the second, first time
     aws_acm:
       state: present
-      <<: *aws_connection_info
       name_tag: "{{ local_certs[0].name }}"
       certificate: "{{ lookup('file', local_certs[1].cert ) }}"
       private_key: "{{ lookup('file', local_certs[1].priv_key ) }}"
@@ -216,7 +203,6 @@
     aws_acm_info:
       tags:
         Name: "{{ local_certs[0].name }}"
-      <<: *aws_connection_info
     register: fetch_after_overwrite
 
   - name: check output of update fetch
@@ -233,7 +219,6 @@
     aws_acm_info:
       tags:
         Name: "{{ local_certs[1].name }}"
-      <<: *aws_connection_info
     register: check_after_overwrite
 
   - name: check other cert unaffected
@@ -249,7 +234,6 @@
   - name: update first cert with body of the second again
     aws_acm:
       state: present
-      <<: *aws_connection_info
       name_tag: "{{ local_certs[0].name }}"
       certificate: "{{ lookup('file', local_certs[1].cert ) }}"
       private_key: "{{ lookup('file', local_certs[1].priv_key ) }}"
@@ -266,7 +250,6 @@
 
   - name: delete certs 1 and 2
     aws_acm:
-      <<: *aws_connection_info
       state: absent
       domain_name: "{{ local_certs[1].domain }}"
     register: delete_both
@@ -281,13 +264,20 @@
 
   - name: fetch info for certs 1 and 2
     aws_acm_info:
-      <<: *aws_connection_info
       tags:
         Name: "{{ local_certs[item].name }}"
     register: check_del_one
     with_items:
       - 0
       - 1
+    # There is the chance that we're running as the deletion is in progress,
+    # this could trigger ResourceNotFoundException allow a single retry to cope
+    # with this.
+    retries: 2
+    until:
+    - check_del_one is not failed
+    - check_del_one.certificates | length == 0
+    delay: 10
 
   - name: check certs 1 and 2 were already deleted
     with_items: "{{ check_del_one.results }}"
@@ -296,7 +286,6 @@
 
   - name: check cert 3 not deleted
     aws_acm_info:
-      <<: *aws_connection_info
       tags:
         Name: "{{ local_certs[2].name }}"
     register: check_del_one_remain
@@ -304,7 +293,6 @@
 
   - name: delete cert 3
     aws_acm:
-      <<: *aws_connection_info
       state: absent
       domain_name: "{{ local_certs[2].domain }}"
     register: delete_third
@@ -319,7 +307,6 @@
 
   - name: check cert 3 was deleted
     aws_acm_info:
-      <<: *aws_connection_info
       tags:
         Name: "{{ local_certs[2].name }}"
     register: check_del_three
@@ -327,7 +314,6 @@
 
   - name: delete cert 3 again
     aws_acm:
-      <<: *aws_connection_info
       state: absent
       domain_name: "{{ local_certs[2].domain }}"
     register: delete_third
@@ -391,7 +377,6 @@
   - name: upload chained cert, first chain, first time
     aws_acm:
       name_tag: "{{ chained_cert.name }}"
-      <<: *aws_connection_info
       certificate: "{{ lookup('file', chained_cert.chains[0].cert ) }}"
       certificate_chain: "{{ chains.results[0].complete_chain | join('\n') }}"
       private_key: "{{ lookup('file', chained_cert.priv_key ) }}"
@@ -401,7 +386,6 @@
 
   - name: fetch chain of cert we just uploaded
     aws_acm_info:
-      <<: *aws_connection_info
       tags:
         Name: "{{ chained_cert.name }}"
     register: check_chain
@@ -419,7 +403,6 @@
   - name: upload chained cert again, check not changed
     aws_acm:
       name_tag: "{{ chained_cert.name }}"
-      <<: *aws_connection_info
       certificate: "{{ lookup('file', chained_cert.chains[0].cert ) }}"
       certificate_chain: "{{ chains.results[0].complete_chain | join('\n') }}"
       private_key: "{{ lookup('file', chained_cert.priv_key ) }}"
@@ -435,7 +418,6 @@
   - name: upload chained cert, different chain
     aws_acm:
       name_tag: "{{ chained_cert.name }}"
-      <<: *aws_connection_info
       certificate: "{{ lookup('file', chained_cert.chains[1].cert ) }}"
       certificate_chain: "{{ chains.results[1].complete_chain | join('\n') }}"
       private_key: "{{ lookup('file', chained_cert.priv_key ) }}"
@@ -450,7 +432,6 @@
 
   - name: fetch info about chain of cert we just updated
     aws_acm_info:
-      <<: *aws_connection_info
       tags:
         Name: "{{ chained_cert.name }}"
     register: check_chain_2
@@ -468,7 +449,6 @@
   - name: delete chained cert
     aws_acm:
       name_tag: "{{ chained_cert.name }}"
-      <<: *aws_connection_info
       state: absent
     register: delete_chain_3
 
@@ -484,7 +464,6 @@
   - name: delete first bunch of certificates
     aws_acm:
       name_tag: "{{ item.name }}"
-      <<: *aws_connection_info
       state: absent
     with_items: "{{ local_certs }}"
     ignore_errors: yes
@@ -493,7 +472,6 @@
     aws_acm:
       state: absent
       name_tag: "{{ chained_cert.name }}"
-      <<: *aws_connection_info
     ignore_errors: yes