Add AWSRetry decorator to ec2_vpc_nacl (#67204)

* Add AWSRetry decorator to ec2_vpc_nacl

* Also add a decorator to ec2_vpc_nacl_info to catch things like API rate limit errors.

* add double-removal integration tests to make sure things don't get too slow

* Fixup retry usage for _info

* Simplify changed logic when modifying a NACL

* tweak error message

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

@@ -161,6 +161,7 @@ except ImportError:
     pass  # Handled by AnsibleAWSModule
 
 from ansible.module_utils.aws.core import AnsibleAWSModule
+from ansible.module_utils.ec2 import AWSRetry
 
 # VPC-supported IANA protocol numbers
 # http://www.iana.org/assignments/protocol-numbers/protocol-numbers.xhtml
@@ -344,12 +345,9 @@ def setup_network_acl(client, module):
     else:
         changed = False
         nacl_id = nacl['NetworkAcls'][0]['NetworkAclId']
-        subnet_result = subnets_changed(nacl, client, module)
+        changed |= subnets_changed(nacl, client, module)
-        nacl_result = nacls_changed(nacl, client, module)
+        changed |= nacls_changed(nacl, client, module)
-        tag_result = tags_changed(nacl_id, client, module)
+        changed |= tags_changed(nacl_id, client, module)
-        if subnet_result is True or nacl_result is True or tag_result is True:
-            changed = True
-            return(changed, nacl_id)
         return (changed, nacl_id)
 
 
@@ -380,63 +378,103 @@ def remove_network_acl(client, module):
 
 
 # Boto3 client methods
+@AWSRetry.jittered_backoff()
+def _create_network_acl(client, *args, **kwargs):
+    return client.create_network_acl(*args, **kwargs)
+
+
 def create_network_acl(vpc_id, client, module):
     try:
         if module.check_mode:
             nacl = dict(NetworkAcl=dict(NetworkAclId="nacl-00000000"))
         else:
-            nacl = client.create_network_acl(VpcId=vpc_id)
+            nacl = _create_network_acl(client, VpcId=vpc_id)
     except botocore.exceptions.ClientError as e:
         module.fail_json_aws(e)
     return nacl
 
 
+@AWSRetry.jittered_backoff(catch_extra_error_codes=['InvalidNetworkAclID.NotFound'])
+def _create_network_acl_entry(client, *args, **kwargs):
+    return client.create_network_acl_entry(*args, **kwargs)
+
+
 def create_network_acl_entry(params, client, module):
     try:
         if not module.check_mode:
-            client.create_network_acl_entry(**params)
+            _create_network_acl_entry(client, **params)
     except botocore.exceptions.ClientError as e:
         module.fail_json_aws(e)
 
 
+@AWSRetry.jittered_backoff(catch_extra_error_codes=['InvalidNetworkAclID.NotFound'])
+def _create_tags(client, *args, **kwargs):
+    return client.create_tags(*args, **kwargs)
+
+
 def create_tags(nacl_id, client, module):
     try:
         delete_tags(nacl_id, client, module)
         if not module.check_mode:
-            client.create_tags(Resources=[nacl_id], Tags=load_tags(module))
+            _create_tags(client, Resources=[nacl_id], Tags=load_tags(module))
     except botocore.exceptions.ClientError as e:
         module.fail_json_aws(e)
 
 
+@AWSRetry.jittered_backoff()
+def _delete_network_acl(client, *args, **kwargs):
+    return client.delete_network_acl(*args, **kwargs)
+
+
 def delete_network_acl(nacl_id, client, module):
     try:
         if not module.check_mode:
-            client.delete_network_acl(NetworkAclId=nacl_id)
+            _delete_network_acl(client, NetworkAclId=nacl_id)
     except botocore.exceptions.ClientError as e:
         module.fail_json_aws(e)
 
 
+@AWSRetry.jittered_backoff(catch_extra_error_codes=['InvalidNetworkAclID.NotFound'])
+def _delete_network_acl_entry(client, *args, **kwargs):
+    return client.delete_network_acl_entry(*args, **kwargs)
+
+
 def delete_network_acl_entry(params, client, module):
     try:
         if not module.check_mode:
-            client.delete_network_acl_entry(**params)
+            _delete_network_acl_entry(client, **params)
     except botocore.exceptions.ClientError as e:
         module.fail_json_aws(e)
 
 
+@AWSRetry.jittered_backoff(catch_extra_error_codes=['InvalidNetworkAclID.NotFound'])
+def _delete_tags(client, *args, **kwargs):
+    return client.delete_tags(*args, **kwargs)
+
+
 def delete_tags(nacl_id, client, module):
     try:
         if not module.check_mode:
-            client.delete_tags(Resources=[nacl_id])
+            _delete_tags(client, Resources=[nacl_id])
     except botocore.exceptions.ClientError as e:
         module.fail_json_aws(e)
 
 
+@AWSRetry.jittered_backoff()
+def _describe_network_acls(client, **kwargs):
+    return client.describe_network_acls(**kwargs)
+
+
+@AWSRetry.jittered_backoff(catch_extra_error_codes=['InvalidNetworkAclID.NotFound'])
+def _describe_network_acls_retry_missing(client, **kwargs):
+    return client.describe_network_acls(**kwargs)
+
+
 def describe_acl_associations(subnets, client, module):
     if not subnets:
         return []
     try:
-        results = client.describe_network_acls(Filters=[
+        results = _describe_network_acls_retry_missing(client, Filters=[
             {'Name': 'association.subnet-id', 'Values': subnets}
         ])
     except botocore.exceptions.ClientError as e:
@@ -448,11 +486,11 @@ def describe_acl_associations(subnets, client, module):
 def describe_network_acl(client, module):
     try:
         if module.params.get('nacl_id'):
-            nacl = client.describe_network_acls(Filters=[
+            nacl = _describe_network_acls(client, Filters=[
                 {'Name': 'network-acl-id', 'Values': [module.params.get('nacl_id')]}
             ])
         else:
-            nacl = client.describe_network_acls(Filters=[
+            nacl = _describe_network_acls(client, Filters=[
                 {'Name': 'tag:Name', 'Values': [module.params.get('name')]}
             ])
     except botocore.exceptions.ClientError as e:
@@ -462,14 +500,14 @@ def describe_network_acl(client, module):
 
 def find_acl_by_id(nacl_id, client, module):
     try:
-        return client.describe_network_acls(NetworkAclIds=[nacl_id])
+        return _describe_network_acls_retry_missing(client, NetworkAclIds=[nacl_id])
     except botocore.exceptions.ClientError as e:
         module.fail_json_aws(e)
 
 
 def find_default_vpc_nacl(vpc_id, client, module):
     try:
-        response = client.describe_network_acls(Filters=[
+        response = _describe_network_acls_retry_missing(client, Filters=[
             {'Name': 'vpc-id', 'Values': [vpc_id]}])
     except botocore.exceptions.ClientError as e:
         module.fail_json_aws(e)
@@ -479,7 +517,7 @@ def find_default_vpc_nacl(vpc_id, client, module):
 
 def find_subnet_ids_by_nacl_id(nacl_id, client, module):
     try:
-        results = client.describe_network_acls(Filters=[
+        results = _describe_network_acls_retry_missing(client, Filters=[
             {'Name': 'association.network-acl-id', 'Values': [nacl_id]}
         ])
     except botocore.exceptions.ClientError as e:
@@ -491,6 +529,11 @@ def find_subnet_ids_by_nacl_id(nacl_id, client, module):
         return []
 
 
+@AWSRetry.jittered_backoff(catch_extra_error_codes=['InvalidNetworkAclID.NotFound'])
+def _replace_network_acl_association(client, *args, **kwargs):
+    return client.replace_network_acl_association(*args, **kwargs)
+
+
 def replace_network_acl_association(nacl_id, subnets, client, module):
     params = dict()
     params['NetworkAclId'] = nacl_id
@@ -498,30 +541,45 @@ def replace_network_acl_association(nacl_id, subnets, client, module):
         params['AssociationId'] = association
         try:
             if not module.check_mode:
-                client.replace_network_acl_association(**params)
+                _replace_network_acl_association(client, **params)
         except botocore.exceptions.ClientError as e:
             module.fail_json_aws(e)
 
 
+@AWSRetry.jittered_backoff(catch_extra_error_codes=['InvalidNetworkAclID.NotFound'])
+def _replace_network_acl_entry(client, *args, **kwargs):
+    return client.replace_network_acl_entry(*args, **kwargs)
+
+
 def replace_network_acl_entry(entries, Egress, nacl_id, client, module):
     for entry in entries:
         params = entry
         params['NetworkAclId'] = nacl_id
         try:
             if not module.check_mode:
-                client.replace_network_acl_entry(**params)
+                _replace_network_acl_entry(client, **params)
         except botocore.exceptions.ClientError as e:
             module.fail_json_aws(e)
 
 
+@AWSRetry.jittered_backoff(catch_extra_error_codes=['InvalidNetworkAclID.NotFound'])
+def _replace_network_acl_association(client, *args, **kwargs):
+    return client.replace_network_acl_association(*args, **kwargs)
+
+
 def restore_default_acl_association(params, client, module):
     try:
         if not module.check_mode:
-            client.replace_network_acl_association(**params)
+            _replace_network_acl_association(client, **params)
     except botocore.exceptions.ClientError as e:
         module.fail_json_aws(e)
 
 
+@AWSRetry.jittered_backoff()
+def _describe_subnets(client, *args, **kwargs):
+    return client.describe_subnets(*args, **kwargs)
+
+
 def subnets_to_associate(nacl, client, module):
     params = list(module.params.get('subnets'))
     if not params:
@@ -529,14 +587,14 @@ def subnets_to_associate(nacl, client, module):
     all_found = []
     if any(x.startswith("subnet-") for x in params):
         try:
-            subnets = client.describe_subnets(Filters=[
+            subnets = _describe_subnets(client, Filters=[
                 {'Name': 'subnet-id', 'Values': params}])
             all_found.extend(subnets.get('Subnets', []))
         except botocore.exceptions.ClientError as e:
             module.fail_json_aws(e)
     if len(params) != len(all_found):
         try:
-            subnets = client.describe_subnets(Filters=[
+            subnets = _describe_subnets(client, Filters=[
                 {'Name': 'tag:Name', 'Values': params}])
             all_found.extend(subnets.get('Subnets', []))
         except botocore.exceptions.ClientError as e:

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

@@ -114,7 +114,7 @@ except ImportError:
 
 from ansible.module_utils.aws.core import AnsibleAWSModule
 from ansible.module_utils._text import to_native
-from ansible.module_utils.ec2 import (ansible_dict_to_boto3_filter_list,
+from ansible.module_utils.ec2 import (AWSRetry, ansible_dict_to_boto3_filter_list,
                                       camel_dict_to_snake_dict, boto3_tag_list_to_ansible_dict)
 
 
@@ -132,11 +132,13 @@ def list_ec2_vpc_nacls(connection, module):
         nacl_ids = []
 
     try:
-        nacls = connection.describe_network_acls(NetworkAclIds=nacl_ids, Filters=filters)
+        nacls = connection.describe_network_acls(aws_retry=True, NetworkAclIds=nacl_ids, Filters=filters)
     except ClientError as e:
-        module.fail_json_aws(e, msg="Unable to describe network ACLs {0}: {1}".format(nacl_ids, to_native(e)))
+        if e.response['Error']['Code'] == 'InvalidNetworkAclID.NotFound':
+            module.fail_json(msg='Unable to describe ACL.  NetworkAcl does not exist')
+        module.fail_json_aws(e, msg="Unable to describe network ACLs {0}".format(nacl_ids))
     except BotoCoreError as e:
-        module.fail_json_aws(e, msg="Unable to describe network ACLs {0}: {1}".format(nacl_ids, to_native(e)))
+        module.fail_json_aws(e, msg="Unable to describe network ACLs {0}".format(nacl_ids))
 
     # Turn the boto3 result in to ansible_friendly_snaked_names
     snaked_nacls = []
@@ -211,7 +213,7 @@ def main():
     if module._name == 'ec2_vpc_nacl_facts':
         module.deprecate("The 'ec2_vpc_nacl_facts' module has been renamed to 'ec2_vpc_nacl_info'", version='2.13')
 
-    connection = module.client('ec2')
+    connection = module.client('ec2', retry_decorator=AWSRetry.jittered_backoff())
 
     list_ec2_vpc_nacls(connection, module)
 

test/integration/targets/ec2_vpc_nacl/tasks/subnet_ids.yml

@@ -140,3 +140,35 @@
   assert:
     that:
       - nacl.changed
+
+- name: re-remove the network ACL by name (test idempotency)
+  ec2_vpc_nacl:
+    vpc_id: "{{ vpc_id }}"
+    name: "{{ resource_prefix }}-acl"
+    state: absent
+  register: nacl
+  until: nacl is success
+  ignore_errors: yes
+  retries: 5
+  delay: 5
+
+- name: assert nacl was removed
+  assert:
+    that:
+      - nacl is not changed
+
+- name: re-remove the network ACL by id (test idempotency)
+  ec2_vpc_nacl:
+    vpc_id: "{{ vpc_id }}"
+    nacl_id: "{{ nacl_id }}"
+    state: absent
+  register: nacl
+  until: nacl is success
+  ignore_errors: yes
+  retries: 5
+  delay: 5
+
+- name: assert nacl was removed
+  assert:
+    that:
+      - nacl is not changed