From 46c4f6311a5149bd7d049c763fd6c2013a100b72 Mon Sep 17 00:00:00 2001 From: Will Thames Date: Fri, 17 Nov 2017 04:09:42 +1000 Subject: [PATCH] [cloud] Add retries/backoff to ec2_vpc_subnet module (#31870) * Allow backoff for describe_subnets Improve exception handling to latest standards * Add integration test suite for ec2_vpc_subnet * Add test for creating subnet without AZ Fix bug identified by test Fixes #31905 --- .../testing_policies/ec2-policy.json | 1 + .../modules/cloud/amazon/ec2_vpc_subnet.py | 97 +++++---- .../targets/ec2_vpc_subnet/aliases | 2 + .../targets/ec2_vpc_subnet/defaults/main.yml | 4 + .../targets/ec2_vpc_subnet/meta/main.yml | 3 + .../targets/ec2_vpc_subnet/tasks/main.yml | 193 ++++++++++++++++++ 6 files changed, 255 insertions(+), 45 deletions(-) create mode 100644 test/integration/targets/ec2_vpc_subnet/aliases create mode 100644 test/integration/targets/ec2_vpc_subnet/defaults/main.yml create mode 100644 test/integration/targets/ec2_vpc_subnet/meta/main.yml create mode 100644 test/integration/targets/ec2_vpc_subnet/tasks/main.yml diff --git a/hacking/aws_config/testing_policies/ec2-policy.json b/hacking/aws_config/testing_policies/ec2-policy.json index c80178323f4..c5623b62371 100644 --- a/hacking/aws_config/testing_policies/ec2-policy.json +++ b/hacking/aws_config/testing_policies/ec2-policy.json @@ -27,6 +27,7 @@ "ec2:DeleteSubnet", "ec2:DeleteTags", "ec2:DeleteVpc", + "ec2:DeleteTags", "ec2:DeregisterImage", "ec2:Describe*", "ec2:DisassociateAddress", diff --git a/lib/ansible/modules/cloud/amazon/ec2_vpc_subnet.py b/lib/ansible/modules/cloud/amazon/ec2_vpc_subnet.py index 6248a2bce32..a09de0eb930 100644 --- a/lib/ansible/modules/cloud/amazon/ec2_vpc_subnet.py +++ b/lib/ansible/modules/cloud/amazon/ec2_vpc_subnet.py @@ -87,10 +87,10 @@ try: except ImportError: pass # caught by imported boto3 -from ansible.module_utils.basic import AnsibleModule +from ansible.module_utils.aws.core import AnsibleAWSModule from ansible.module_utils.ec2 import (ansible_dict_to_boto3_filter_list, ansible_dict_to_boto3_tag_list, ec2_argument_spec, camel_dict_to_snake_dict, get_aws_connection_info, - boto3_conn, boto3_tag_list_to_ansible_dict, HAS_BOTO3) + boto3_conn, boto3_tag_list_to_ansible_dict, AWSRetry) def get_subnet_info(subnet): @@ -113,9 +113,17 @@ def get_subnet_info(subnet): return subnet -def subnet_exists(conn, subnet_id): +@AWSRetry.exponential_backoff() +def describe_subnets_with_backoff(client, **params): + return client.describe_subnets(**params) + + +def subnet_exists(conn, module, subnet_id): filters = ansible_dict_to_boto3_filter_list({'subnet-id': subnet_id}) - subnets = get_subnet_info(conn.describe_subnets(Filters=filters)) + try: + subnets = get_subnet_info(describe_subnets_with_backoff(conn, Filters=filters)) + except (botocore.exceptions.ClientError, botocore.exceptions.BotoCoreError) as e: + module.fail_json_aws(e, msg="Couldn't check if subnet exists") if len(subnets) > 0 and 'state' in subnets[0] and subnets[0]['state'] == "available": return subnets[0] else: @@ -123,58 +131,61 @@ def subnet_exists(conn, subnet_id): def create_subnet(conn, module, vpc_id, cidr, az, check_mode): + if check_mode: + return + params = dict(VpcId=vpc_id, CidrBlock=cidr) + if az: + params['AvailabilityZone'] = az try: - new_subnet = get_subnet_info(conn.create_subnet(VpcId=vpc_id, CidrBlock=cidr, AvailabilityZone=az)) - # Sometimes AWS takes its time to create a subnet and so using - # new subnets's id to do things like create tags results in - # exception. boto doesn't seem to refresh 'state' of the newly - # created subnet, i.e.: it's always 'pending'. - subnet = False - while subnet is False: - subnet = subnet_exists(conn, new_subnet['id']) - time.sleep(0.1) - except botocore.exceptions.ClientError as e: - if e.response['Error']['Code'] == "DryRunOperation": - subnet = None - else: - module.fail_json(msg=e.message, exception=traceback.format_exc(), - **camel_dict_to_snake_dict(e.response)) + new_subnet = get_subnet_info(conn.create_subnet(**params)) + except (botocore.exceptions.ClientError, botocore.exceptions.BotoCoreError) as e: + module.fail_json_aws(e, msg="Couldn't create subnet") + # Sometimes AWS takes its time to create a subnet and so using + # new subnets's id to do things like create tags results in + # exception. boto doesn't seem to refresh 'state' of the newly + # created subnet, i.e.: it's always 'pending'. + subnet = False + while subnet is False: + subnet = subnet_exists(conn, module, new_subnet['id']) + time.sleep(0.1) return subnet def ensure_tags(conn, module, subnet, tags, add_only, check_mode): - try: - cur_tags = subnet['tags'] + cur_tags = subnet['tags'] - to_delete = dict((k, cur_tags[k]) for k in cur_tags if k not in tags) - if to_delete and not add_only and not check_mode: + to_delete = dict((k, cur_tags[k]) for k in cur_tags if k not in tags) + if to_delete and not add_only and not check_mode: + try: conn.delete_tags(Resources=[subnet['id']], Tags=ansible_dict_to_boto3_tag_list(to_delete)) + except (botocore.exceptions.ClientError, botocore.exceptions.BotoCoreError) as e: + module.fail_json_aws(e, msg="Couldn't delete tags") - to_add = dict((k, tags[k]) for k in tags if k not in cur_tags or cur_tags[k] != tags[k]) - if to_add and not check_mode: + to_add = dict((k, tags[k]) for k in tags if k not in cur_tags or cur_tags[k] != tags[k]) + if to_add and not check_mode: + try: conn.create_tags(Resources=[subnet['id']], Tags=ansible_dict_to_boto3_tag_list(to_add)) - - except botocore.exceptions.ClientError as e: - if e.response['Error']['Code'] != "DryRunOperation": - module.fail_json(msg=e.message, exception=traceback.format_exc(), - **camel_dict_to_snake_dict(e.response)) + except (botocore.exceptions.ClientError, botocore.exceptions.BotoCoreError) as e: + module.fail_json_aws(e, msg="Couldn't create tags") def ensure_map_public(conn, module, subnet, map_public, check_mode): if check_mode: return - try: conn.modify_subnet_attribute(SubnetId=subnet['id'], MapPublicIpOnLaunch={'Value': map_public}) - except botocore.exceptions.ClientError as e: - module.fail_json(msg=e.message, exception=traceback.format_exc(), - **camel_dict_to_snake_dict(e.response)) + except (botocore.exceptions.ClientError, botocore.exceptions.BotoCoreError) as e: + module.fail_json_aws(e, msg="Couldn't modify subnet attribute") -def get_matching_subnet(conn, vpc_id, cidr): +def get_matching_subnet(conn, module, vpc_id, cidr): filters = ansible_dict_to_boto3_filter_list({'vpc-id': vpc_id, 'cidr-block': cidr}) - subnets = get_subnet_info(conn.describe_subnets(Filters=filters)) + try: + subnets = get_subnet_info(conn.describe_subnets(Filters=filters)) + except (botocore.exceptions.ClientError, botocore.exceptions.BotoCoreError) as e: + module.fail_json_aws(e, msg="Couldn't get matching subnet") + if len(subnets) > 0: return subnets[0] else: @@ -182,7 +193,7 @@ def get_matching_subnet(conn, vpc_id, cidr): def ensure_subnet_present(conn, module, vpc_id, cidr, az, tags, map_public, check_mode): - subnet = get_matching_subnet(conn, vpc_id, cidr) + subnet = get_matching_subnet(conn, module, vpc_id, cidr) changed = False if subnet is None: if not check_mode: @@ -211,7 +222,7 @@ def ensure_subnet_present(conn, module, vpc_id, cidr, az, tags, map_public, chec def ensure_subnet_absent(conn, module, vpc_id, cidr, check_mode): - subnet = get_matching_subnet(conn, vpc_id, cidr) + subnet = get_matching_subnet(conn, module, vpc_id, cidr) if subnet is None: return {'changed': False} @@ -219,9 +230,8 @@ def ensure_subnet_absent(conn, module, vpc_id, cidr, check_mode): if not check_mode: conn.delete_subnet(SubnetId=subnet['id'], DryRun=check_mode) return {'changed': True} - except botocore.exceptions.ClientError as e: - module.fail_json(msg=e.message, exception=traceback.format_exc(), - **camel_dict_to_snake_dict(e.response)) + except (botocore.exceptions.ClientError, botocore.exceptions.BotoCoreError) as e: + module.fail_json_aws(e, msg="Couldn't delete subnet") def main(): @@ -237,10 +247,7 @@ def main(): ) ) - module = AnsibleModule(argument_spec=argument_spec, supports_check_mode=True) - - if not HAS_BOTO3: - module.fail_json(msg='boto3 and botocore are required for this module') + module = AnsibleAWSModule(argument_spec=argument_spec, supports_check_mode=True) region, ec2_url, aws_connect_params = get_aws_connection_info(module, boto3=True) diff --git a/test/integration/targets/ec2_vpc_subnet/aliases b/test/integration/targets/ec2_vpc_subnet/aliases new file mode 100644 index 00000000000..ebdf4aa5720 --- /dev/null +++ b/test/integration/targets/ec2_vpc_subnet/aliases @@ -0,0 +1,2 @@ +cloud/aws +posix/ci/cloud/group1/aws diff --git a/test/integration/targets/ec2_vpc_subnet/defaults/main.yml b/test/integration/targets/ec2_vpc_subnet/defaults/main.yml new file mode 100644 index 00000000000..9c529aff021 --- /dev/null +++ b/test/integration/targets/ec2_vpc_subnet/defaults/main.yml @@ -0,0 +1,4 @@ +--- +# defaults file for ec2_vpc_subnet +ec2_vpc_subnet_name: '{{resource_prefix}}' +ec2_vpc_subnet_description: 'Created by ansible integration tests' diff --git a/test/integration/targets/ec2_vpc_subnet/meta/main.yml b/test/integration/targets/ec2_vpc_subnet/meta/main.yml new file mode 100644 index 00000000000..1f64f1169a9 --- /dev/null +++ b/test/integration/targets/ec2_vpc_subnet/meta/main.yml @@ -0,0 +1,3 @@ +dependencies: + - prepare_tests + - setup_ec2 diff --git a/test/integration/targets/ec2_vpc_subnet/tasks/main.yml b/test/integration/targets/ec2_vpc_subnet/tasks/main.yml new file mode 100644 index 00000000000..47854f868d4 --- /dev/null +++ b/test/integration/targets/ec2_vpc_subnet/tasks/main.yml @@ -0,0 +1,193 @@ +--- +# A Note about ec2 environment variable name preference: +# - EC2_URL -> AWS_URL +# - EC2_ACCESS_KEY -> AWS_ACCESS_KEY_ID -> AWS_ACCESS_KEY +# - EC2_SECRET_KEY -> AWS_SECRET_ACCESS_KEY -> AWX_SECRET_KEY +# - EC2_REGION -> AWS_REGION +# + +# - include: ../../setup_ec2/tasks/common.yml module_name: ec2_vpc_subnet + +- block: + + # ============================================================ + - name: create a VPC + ec2_vpc_net: + name: "{{ resource_prefix }}-vpc" + state: present + cidr_block: "10.232.232.128/26" + region: '{{ec2_region}}' + aws_access_key: '{{ aws_access_key }}' + aws_secret_key: '{{ aws_secret_key }}' + security_token: '{{ security_token }}' + tags: + Name: "{{ resource_prefix }}-vpc" + Description: "Created by ansible-test" + register: vpc_result + + - name: create subnet (expected changed=true) + ec2_vpc_subnet: + cidr: "10.232.232.128/28" + az: "{{ ec2_region }}a" + vpc_id: "{{ vpc_result.vpc.id }}" + tags: + Name: '{{ec2_vpc_subnet_name}}' + Description: '{{ec2_vpc_subnet_description}}' + region: '{{ec2_region}}' + aws_access_key: '{{ aws_access_key }}' + aws_secret_key: '{{ aws_secret_key }}' + security_token: '{{ security_token }}' + state: present + register: vpc_subnet_create + + - name: assert creation happened (expected changed=true) + assert: + that: + - 'vpc_subnet_create' + - 'vpc_subnet_create.subnet.id.startswith("subnet-")' + - '"Name" in vpc_subnet_create.subnet.tags and vpc_subnet_create.subnet.tags["Name"] == ec2_vpc_subnet_name' + - '"Description" in vpc_subnet_create.subnet.tags and vpc_subnet_create.subnet.tags["Description"] == ec2_vpc_subnet_description' + + - name: recreate subnet (expected changed=false) + ec2_vpc_subnet: + cidr: "10.232.232.128/28" + az: "{{ ec2_region }}a" + vpc_id: "{{ vpc_result.vpc.id }}" + tags: + Name: '{{ec2_vpc_subnet_name}}' + Description: '{{ec2_vpc_subnet_description}}' + region: '{{ec2_region}}' + aws_access_key: '{{ aws_access_key }}' + aws_secret_key: '{{ aws_secret_key }}' + security_token: '{{ security_token }}' + state: present + register: vpc_subnet_recreate + + - name: assert recreation changed nothing (expected changed=true) + assert: + that: + - 'not vpc_subnet_recreate.changed' + - 'vpc_subnet_recreate.subnet.id.startswith("subnet-")' + - '"Name" in vpc_subnet_recreate.subnet.tags and vpc_subnet_recreate.subnet.tags["Name"] == ec2_vpc_subnet_name' + - '"Description" in vpc_subnet_recreate.subnet.tags and vpc_subnet_recreate.subnet.tags["Description"] == ec2_vpc_subnet_description' + + - name: add a tag (expected changed=true) + ec2_vpc_subnet: + cidr: "10.232.232.128/28" + az: "{{ ec2_region }}a" + vpc_id: "{{ vpc_result.vpc.id }}" + tags: + Name: '{{ec2_vpc_subnet_name}}' + Description: '{{ec2_vpc_subnet_description}}' + AnotherTag: SomeValue + region: '{{ec2_region}}' + aws_access_key: '{{ aws_access_key }}' + aws_secret_key: '{{ aws_secret_key }}' + security_token: '{{ security_token }}' + state: present + register: vpc_subnet_add_a_tag + + - name: assert tag addition happened (expected changed=true) + assert: + that: + - 'vpc_subnet_add_a_tag.changed' + - '"Name" in vpc_subnet_add_a_tag.subnet.tags and vpc_subnet_add_a_tag.subnet.tags["Name"] == ec2_vpc_subnet_name' + - '"Description" in vpc_subnet_add_a_tag.subnet.tags and vpc_subnet_add_a_tag.subnet.tags["Description"] == ec2_vpc_subnet_description' + - '"AnotherTag" in vpc_subnet_add_a_tag.subnet.tags and vpc_subnet_add_a_tag.subnet.tags["AnotherTag"] == "SomeValue"' + + # We may want to change this behaviour by adding purge_tags to the module + # and setting it to false by default + - name: remove tags (expected changed=true) + ec2_vpc_subnet: + cidr: "10.232.232.128/28" + az: "{{ ec2_region }}a" + vpc_id: "{{ vpc_result.vpc.id }}" + tags: + AnotherTag: SomeValue + region: '{{ec2_region}}' + aws_access_key: '{{ aws_access_key }}' + aws_secret_key: '{{ aws_secret_key }}' + security_token: '{{ security_token }}' + state: present + register: vpc_subnet_remove_tags + + - name: assert tag removal happened (expected changed=true) + assert: + that: + - 'vpc_subnet_remove_tags.changed' + - '"Name" not in vpc_subnet_remove_tags.subnet.tags' + - '"Description" not in vpc_subnet_remove_tags.subnet.tags' + - '"AnotherTag" in vpc_subnet_remove_tags.subnet.tags and vpc_subnet_remove_tags.subnet.tags["AnotherTag"] == "SomeValue"' + + - name: test state=absent (expected changed=true) + ec2_vpc_subnet: + cidr: "10.232.232.128/28" + vpc_id: "{{ vpc_result.vpc.id }}" + state: absent + region: '{{ec2_region}}' + aws_access_key: '{{ aws_access_key }}' + aws_secret_key: '{{ aws_secret_key }}' + security_token: '{{ security_token }}' + register: result + + - name: assert state=absent (expected changed=true) + assert: + that: + - 'result.changed' + + - name: create subnet without AZ + ec2_vpc_subnet: + cidr: "10.232.232.128/28" + vpc_id: "{{ vpc_result.vpc.id }}" + state: present + region: '{{ec2_region}}' + aws_access_key: '{{ aws_access_key }}' + aws_secret_key: '{{ aws_secret_key }}' + security_token: '{{ security_token }}' + register: subnet_without_az + + - name: check that subnet without AZ works fine + assert: + that: + - 'subnet_without_az.changed' + + - name: remove subnet without AZ + ec2_vpc_subnet: + cidr: "10.232.232.128/28" + vpc_id: "{{ vpc_result.vpc.id }}" + state: absent + region: '{{ec2_region}}' + aws_access_key: '{{ aws_access_key }}' + aws_secret_key: '{{ aws_secret_key }}' + security_token: '{{ security_token }}' + register: result + + - name: assert state=absent (expected changed=true) + assert: + that: + - 'result.changed' + always: + + ################################################ + # TEARDOWN STARTS HERE + ################################################ + + - name: tidy up subnet + ec2_vpc_subnet: + cidr: "10.232.232.128/28" + vpc_id: "{{ vpc_result.vpc.id }}" + state: absent + region: '{{ec2_region}}' + aws_access_key: '{{ aws_access_key }}' + aws_secret_key: '{{ aws_secret_key }}' + security_token: '{{ security_token }}' + + - name: tidy up VPC + ec2_vpc_net: + name: "{{ resource_prefix }}-vpc" + state: absent + cidr_block: "10.232.232.128/26" + region: '{{ec2_region}}' + aws_access_key: '{{ aws_access_key }}' + aws_secret_key: '{{ aws_secret_key }}' + security_token: '{{ security_token }}'