[cloud] Add retries/backoff to ec2_vpc_subnet module (ansible#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 ansible#31905

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",

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

@@ -87,10 +87,10 @@
 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,76 +113,87 @@ 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:
         return False
 
 
 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:
         return None
 
 
 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,17 +222,16 @@ 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}
 
     try:
         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)
 

test/integration/targets/ec2_vpc_subnet/aliases

@@ -0,0 +1,2 @@
+cloud/aws
+posix/ci/cloud/group1/aws

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'

test/integration/targets/ec2_vpc_subnet/meta/main.yml

@@ -0,0 +1,3 @@
+dependencies:
+  - prepare_tests
+  - setup_ec2

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 }}'