Stabilize ec2_eip module (#936) (#946)

backport-3: Stabilize ec2_eip module (#936) Backport into stable-3 of #936 with commit id: d0596e3 SUMMARY fixed check_mode issues added integration tests for check_mode / idempotency updated json returned when state = absent for clarity removed json_query references fixes #159 Depends-On: ansible-collections/amazon.aws#672 ISSUE TYPE Feature Pull Request COMPONENT NAME ec2_eip Reviewed-by: Mark Woolley Reviewed-by: Mark Chappell Reviewed-by: Alina Buzachis Reviewed-by: Joseph Torcasso Reviewed-by: Jill R SUMMARY ISSUE TYPE Bugfix Pull Request Docs Pull Request Feature Pull Request New Module Pull Request COMPONENT NAME ADDITIONAL INFORMATION
ansible-collections · Feb 23, 2022 · a54d252 · a54d252
1 parent 9ccdbdf
commit a54d252
Show file tree

Hide file tree

Showing 5 changed files with 1,425 additions and 936 deletions.
diff --git a/changelogs/fragments/936-stabilize-ec2-eip.yml b/changelogs/fragments/936-stabilize-ec2-eip.yml
@@ -0,0 +1,2 @@
+minor_changes:
+  - ec2_eip - refactor module by fixing check_mode and more clear return obj. added integration tests (https://github.com/ansible-collections/community.aws/pull/936)
diff --git a/plugins/modules/ec2_eip.py b/plugins/modules/ec2_eip.py
@@ -27,8 +27,8 @@
   public_ip:
     description:
       - The IP address of a previously allocated EIP.
-      - When I(public_ip=present) and device is specified, the EIP is associated with the device.
-      - When I(public_ip=absent) and device is specified, the EIP is disassociated from the device.
+      - When I(state=present) and device is specified, the EIP is associated with the device.
+      - When I(state=absent) and device is specified, the EIP is disassociated from the device.
     aliases: [ ip ]
     type: str
   state:
@@ -328,7 +328,7 @@ def find_address(ec2, module, public_ip, device_id, is_instance=True):
     except is_boto3_error_code('InvalidAddress.NotFound') as e:
         # If we're releasing and we can't find it, it's already gone...
         if module.params.get('state') == 'absent':
-            module.exit_json(changed=False)
+            module.exit_json(changed=False, disassociated=False, released=False)
         module.fail_json_aws(e, msg="Couldn't obtain list of existing Elastic IP addresses")
 
     addresses = addresses["Addresses"]
@@ -385,6 +385,8 @@ def allocate_address(ec2, module, domain, reuse_existing_ip_allowed, check_mode,
         return allocate_address_from_pool(ec2, module, domain, check_mode, public_ipv4_pool), True
 
     try:
+        if check_mode:
+            return None, True
         result = ec2.allocate_address(Domain=domain, aws_retry=True), True
     except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e:
         module.fail_json_aws(e, msg="Couldn't allocate Elastic IP address")
@@ -493,8 +495,11 @@ def ensure_absent(ec2, module, address, device_id, check_mode, is_instance=True)
 
 
 def allocate_address_from_pool(ec2, module, domain, check_mode, public_ipv4_pool):
-    # type: (EC2Connection, str, bool, str) -> Address
-    """ Overrides boto's allocate_address function to support BYOIP """
+    # type: (EC2Connection, AnsibleAWSModule, str, bool, str) -> Address
+    """ Overrides botocore's allocate_address function to support BYOIP """
+    if check_mode:
+        return None
+
     params = {}
 
     if domain is not None:
@@ -503,9 +508,6 @@ def allocate_address_from_pool(ec2, module, domain, check_mode, public_ipv4_pool
     if public_ipv4_pool is not None:
         params['PublicIpv4Pool'] = public_ipv4_pool
 
-    if check_mode:
-        params['DryRun'] = 'true'
-
     try:
         result = ec2.allocate_address(aws_retry=True, **params)
     except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e:
@@ -606,19 +608,33 @@ def main():
                     reuse_existing_ip_allowed, allow_reassociation,
                     module.check_mode, is_instance=is_instance
                 )
+                if 'allocation_id' not in result:
+                    # Don't check tags on check_mode here - no EIP to pass through
+                    module.exit_json(**result)
             else:
                 if address:
-                    changed = False
+                    result = {
+                        'changed': False,
+                        'public_ip': address['PublicIp'],
+                        'allocation_id': address['AllocationId']
+                    }
                 else:
                     address, changed = allocate_address(
                         ec2, module, domain, reuse_existing_ip_allowed,
                         module.check_mode, tag_dict, public_ipv4_pool
                     )
-                result = {
-                    'changed': changed,
-                    'public_ip': address['PublicIp'],
-                    'allocation_id': address['AllocationId']
-                }
+                    if address:
+                        result = {
+                            'changed': changed,
+                            'public_ip': address['PublicIp'],
+                            'allocation_id': address['AllocationId']
+                        }
+                    else:
+                        # Don't check tags on check_mode here - no EIP to pass through
+                        result = {
+                            'changed': changed
+                        }
+                        module.exit_json(**result)
 
             result['changed'] |= ensure_ec2_tags(
                 ec2, module, result['allocation_id'],
@@ -633,21 +649,21 @@ def main():
                     released = release_address(ec2, module, address, module.check_mode)
                     result = {
                         'changed': True,
-                        'disassociated': disassociated,
-                        'released': released
+                        'disassociated': disassociated['changed'],
+                        'released': released['changed']
                     }
                 else:
                     result = {
                         'changed': disassociated['changed'],
-                        'disassociated': disassociated,
-                        'released': {'changed': False}
+                        'disassociated': disassociated['changed'],
+                        'released': False
                     }
             else:
                 released = release_address(ec2, module, address, module.check_mode)
                 result = {
                     'changed': released['changed'],
-                    'disassociated': {'changed': False},
-                    'released': released
+                    'disassociated': False,
+                    'released': released['changed']
                 }
 
     except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e:

diff --git a/plugins/modules/ec2_eip_info.py b/plugins/modules/ec2_eip_info.py
@@ -44,7 +44,7 @@
   register: my_vm_eips
 
 - ansible.builtin.debug:
-    msg: "{{ my_vm_eips.addresses | json_query(\"[?private_ip_address=='10.0.0.5']\") }}"
+    msg: "{{ my_vm_eips.addresses | selectattr('private_ip_address', 'equalto', '10.0.0.5') }}"
 
 - name: List all EIP addresses for several VMs.
   community.aws.ec2_eip_info:

diff --git a/tests/integration/targets/ec2_eip/aliases b/tests/integration/targets/ec2_eip/aliases
@@ -1,4 +1,5 @@
 # https://github.com/ansible-collections/community.aws/issues/159
-unstable
+# unstable
 
 cloud/aws
+ec2_eip_info