From c36f03b3e1665e28a572233f3ab139e2b652ed5c Mon Sep 17 00:00:00 2001 From: Mark Chappell Date: Sat, 15 Feb 2020 13:42:02 +0100 Subject: [PATCH] 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 --- .../modules/cloud/amazon/ec2_vpc_nacl.py | 104 ++++++++++++++---- .../modules/cloud/amazon/ec2_vpc_nacl_info.py | 12 +- .../targets/ec2_vpc_nacl/tasks/subnet_ids.yml | 32 ++++++ 3 files changed, 120 insertions(+), 28 deletions(-) diff --git a/lib/ansible/modules/cloud/amazon/ec2_vpc_nacl.py b/lib/ansible/modules/cloud/amazon/ec2_vpc_nacl.py index 5de27e1dddf..5e8ea95a04d 100644 --- a/lib/ansible/modules/cloud/amazon/ec2_vpc_nacl.py +++ b/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) - nacl_result = nacls_changed(nacl, client, module) - tag_result = 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) + changed |= subnets_changed(nacl, client, module) + changed |= nacls_changed(nacl, client, module) + changed |= tags_changed(nacl_id, client, module) 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: diff --git a/lib/ansible/modules/cloud/amazon/ec2_vpc_nacl_info.py b/lib/ansible/modules/cloud/amazon/ec2_vpc_nacl_info.py index d95623774c9..c643d23413f 100644 --- a/lib/ansible/modules/cloud/amazon/ec2_vpc_nacl_info.py +++ b/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) diff --git a/test/integration/targets/ec2_vpc_nacl/tasks/subnet_ids.yml b/test/integration/targets/ec2_vpc_nacl/tasks/subnet_ids.yml index cd4b0c55172..de371d629ae 100644 --- a/test/integration/targets/ec2_vpc_nacl/tasks/subnet_ids.yml +++ b/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