fix incomplete enforce_count() return values (ansible-collections#964)

fix incomplete enforce_count() return values SUMMARY This is the Ansible way. Since a call to enforce_count() requires that a call to find_instances() has already been made, the information required to populate the instances return key is already held by the variable existing_matches. There are zero use cases where it makes sense not to output this information; if I want exactly one such instance and an existing instance matches the filter, I definitely always want to know its details. Fixes ansible-collections#963, ansible-collections#859 ISSUE TYPE Bugfix Pull Request COMPONENT NAME ec2_instance ADDITIONAL INFORMATION See linked issues. Before: { "changed": false, "invocation": { "module_args": { "availability_zone": null, "aws_access_key": null, "aws_ca_bundle": null, "aws_config": null, "aws_secret_key": null, "count": null, "cpu_credit_specification": null, "cpu_options": null, "debug_botocore_endpoint_logs": false, "detailed_monitoring": false, "ebs_optimized": true, "ec2_url": null, "exact_count": 1, "filters": { "tag:Name": "something", "tag:env": "prod" }, "image": null, "image_id": "REDACTED", "instance_ids": [], "instance_initiated_shutdown_behavior": null, "instance_role": "REDACTED", "instance_type": "m5.2xlarge", "key_name": "REDACTED", "launch_template": null, "metadata_options": null, "name": null, "network": { "source_dest_check": true }, "placement_group": null, "profile": null, "purge_tags": false, "region": "us-west-2", "security_group": null, "security_groups": [ "default" ], "security_token": null, "state": "present", "tags": { "Name": "something", "env": "prod", }, "tenancy": null, "termination_protection": null, "tower_callback": null, "user_data": "REDACTED", "validate_certs": true, "volumes": [ { "device_name": "/dev/sda1", "ebs": { "delete_on_termination": true, "encrypted": true, "volume_size": 20, "volume_type": "gp3" } }, { "device_name": "/dev/xvdo", "ebs": { "delete_on_termination": true, "encrypted": true, "volume_size": 20, "volume_type": "gp3" } }, { "device_name": "/dev/xvdp", "ebs": { "delete_on_termination": true, "encrypted": true, "volume_size": 10, "volume_type": "gp3" } }, { "device_name": "/dev/xvdi", "ebs": { "delete_on_termination": true, "encrypted": true, "volume_size": 100, "volume_type": "gp3" } } ], "vpc_subnet_id": "REDACTED", "wait": true, "wait_timeout": 300 } }, "msg": "1 instances already running, nothing to do." } After: { "changed": false, "instance_ids": [ "REDACTED" ], "instances": [ { "ami_launch_index": 0, "architecture": "x86_64", "block_device_mappings": [ { "device_name": "/dev/sda1", "ebs": { "attach_time": "2022-08-10T19:50:24+00:00", "delete_on_termination": true, "status": "attached", "volume_id": "REDACTED" } }, { "device_name": "/dev/xvdo", "ebs": { "attach_time": "2022-08-10T19:50:24+00:00", "delete_on_termination": true, "status": "attached", "volume_id": "REDACTED" } }, { "device_name": "/dev/xvdp", "ebs": { "attach_time": "2022-08-10T19:50:24+00:00", "delete_on_termination": true, "status": "attached", "volume_id": "REDACTED" } }, { "device_name": "/dev/xvdi", "ebs": { "attach_time": "2022-08-10T19:50:24+00:00", "delete_on_termination": true, "status": "attached", "volume_id": "REDACTED" } } ], "capacity_reservation_specification": { "capacity_reservation_preference": "open" }, "client_token": "REDACTED", "cpu_options": { "core_count": 4, "threads_per_core": 2 }, "ebs_optimized": true, "ena_support": true, "enclave_options": { "enabled": false }, "hibernation_options": { "configured": false }, "hypervisor": "xen", "iam_instance_profile": { "arn": "REDACTED", "id": "REDACTED" }, "image_id": "REDACTED", "instance_id": "REDACTED", "instance_type": "m5.2xlarge", "key_name": "REDACTED", "launch_time": "2022-08-10T19:50:23+00:00", "maintenance_options": { "auto_recovery": "default" }, "metadata_options": { "http_endpoint": "enabled", "http_protocol_ipv6": "disabled", "http_put_response_hop_limit": 1, "http_tokens": "optional", "instance_metadata_tags": "disabled", "state": "applied" }, "monitoring": { "state": "disabled" }, "network_interfaces": [ { "attachment": { "attach_time": "2022-08-10T19:50:23+00:00", "attachment_id": "REDACTED", "delete_on_termination": true, "device_index": 0, "network_card_index": 0, "status": "attached" }, "description": "", "groups": [ { "group_id": "REDACTED", "group_name": "REDACTED" } ], "interface_type": "interface", "ipv6_addresses": [], "mac_address": "REDACTED", "network_interface_id": "REDACTED", "owner_id": "REDACTED", "private_dns_name": "REDACTED", "private_ip_address": "REDACTED", "private_ip_addresses": [ { "primary": true, "private_dns_name": "REDACTED", "private_ip_address": "REDACTED" } ], "source_dest_check": true, "status": "in-use", "subnet_id": "REDACTED", "vpc_id": "REDACTED" } ], "placement": { "availability_zone": "us-west-2a", "group_name": "", "tenancy": "default" }, "platform_details": "Linux/UNIX", "private_dns_name": "REDACTED", "private_dns_name_options": { "enable_resource_name_dns_a_record": false, "enable_resource_name_dns_aaaa_record": false, "hostname_type": "ip-name" }, "private_ip_address": "REDACTED", "product_codes": [], "public_dns_name": "", "root_device_name": "/dev/sda1", "root_device_type": "ebs", "security_groups": [ { "group_id": "REDACTED", "group_name": "REDACTED" } ], "source_dest_check": true, "state": { "code": 16, "name": "running" }, "state_transition_reason": "", "subnet_id": "REDACTED", "tags": { "Name": "something", "env": "prod", }, "usage_operation": "RunInstances", "usage_operation_update_time": "2022-08-10T19:50:23+00:00", "virtualization_type": "hvm", "vpc_id": "REDACTED" } ], "invocation": { "module_args": { "availability_zone": null, "aws_access_key": null, "aws_ca_bundle": null, "aws_config": null, "aws_secret_key": null, "count": null, "cpu_credit_specification": null, "cpu_options": null, "debug_botocore_endpoint_logs": false, "detailed_monitoring": false, "ebs_optimized": true, "ec2_url": null, "exact_count": 1, "filters": { "tag:Name": "something", "tag:env": "prod" }, "image": null, "image_id": "REDACTED", "instance_ids": [], "instance_initiated_shutdown_behavior": null, "instance_role": "REDACTED", "instance_type": "m5.2xlarge", "key_name": "REDACTED", "launch_template": null, "metadata_options": null, "name": null, "network": { "source_dest_check": true }, "placement_group": null, "profile": null, "purge_tags": false, "region": "us-west-2", "security_group": null, "security_groups": [ "default" ], "security_token": null, "state": "present", "tags": { "Name": "something", "env": "prod", }, "tenancy": null, "termination_protection": null, "tower_callback": null, "user_data": "REDACTED", "validate_certs": true, "volumes": [ { "device_name": "/dev/sda1", "ebs": { "delete_on_termination": true, "encrypted": true, "volume_size": 20, "volume_type": "gp3" } }, { "device_name": "/dev/xvdo", "ebs": { "delete_on_termination": true, "encrypted": true, "volume_size": 20, "volume_type": "gp3" } }, { "device_name": "/dev/xvdp", "ebs": { "delete_on_termination": true, "encrypted": true, "volume_size": 10, "volume_type": "gp3" } }, { "device_name": "/dev/xvdi", "ebs": { "delete_on_termination": true, "encrypted": true, "volume_size": 100, "volume_type": "gp3" } } ], "vpc_subnet_id": "REDACTED", "wait": true, "wait_timeout": 300 } }, "msg": "1 instances already running, nothing to do." } Reviewed-by: Alina Buzachis Reviewed-by: Mark Chappell
tremble · Feb 24, 2023 · 3c09cda · 3c09cda
1 parent 2ac00de
commit 3c09cda
Show file tree

Hide file tree

Showing 3 changed files with 114 additions and 33 deletions.
diff --git a/changelogs/fragments/964-ec2_instance-return-instances.yml b/changelogs/fragments/964-ec2_instance-return-instances.yml
@@ -0,0 +1,2 @@
+minor_changes:
+- ec2_instance - more consistently return ``instances`` information (https://github.com/ansible-collections/amazon.aws/pull/964).
diff --git a/plugins/modules/ec2_instance.py b/plugins/modules/ec2_instance.py
@@ -6,7 +6,7 @@
 __metaclass__ = type
 
 
-DOCUMENTATION = r'''
+DOCUMENTATION = r"""
 ---
 module: ec2_instance
 version_added: 1.0.0
@@ -378,9 +378,9 @@
   - amazon.aws.ec2
   - amazon.aws.tags
   - amazon.aws.boto3
-'''
+"""
 
-EXAMPLES = '''
+EXAMPLES = r"""
 # Note: These examples do not set authentication details, see the AWS Guide for details.
 
 - name: Terminate every running instance in a region. Use with EXTREME caution.
@@ -534,12 +534,30 @@
     state: present
     tags:
       foo: bar
-'''
+"""
 
-RETURN = '''
+RETURN = r"""
+instance_ids:
+    description: a list of ec2 instance IDs matching the provided specification and filters
+    returned: always
+    type: list
+    sample: ["i-0123456789abcdef0", "i-0123456789abcdef1"]
+    version_added: 5.3.0
+changed_ids:
+    description: a list of the set of ec2 instance IDs changed by the module action
+    returned: when instances that must be present are launched
+    type: list
+    sample: ["i-0123456789abcdef0"]
+    version_added: 5.3.0
+terminated_ids:
+    description: a list of the set of ec2 instance IDs terminated by the module action
+    returned: when instances that must be absent are terminated
+    type: list
+    sample: ["i-0123456789abcdef1"]
+    version_added: 5.3.0
 instances:
     description: a list of ec2 instances
-    returned: when wait == true
+    returned: when wait == true or when matching instances already exist
     type: complex
     contains:
         ami_launch_index:
@@ -942,7 +960,7 @@
             returned: always
             type: dict
             sample: vpc-0011223344
-'''
+"""
 
 from collections import namedtuple
 import time
@@ -1824,7 +1842,9 @@ def enforce_count(existing_matches, module, desired_module_state):
         if current_count == exact_count:
             module.exit_json(
                 changed=False,
-                msg='{0} instances already running, nothing to do.'.format(exact_count)
+                instances=[pretty_instance(i) for i in existing_matches],
+                instance_ids=[i["InstanceId"] for i in existing_matches],
+                msg=f"{exact_count} instances already running, nothing to do.",
             )
 
         elif current_count < exact_count:
@@ -1843,7 +1863,12 @@ def enforce_count(existing_matches, module, desired_module_state):
             all_instance_ids = [x['InstanceId'] for x in existing_matches]
             terminate_ids = all_instance_ids[0:to_terminate]
             if module.check_mode:
-                module.exit_json(changed=True, msg='Would have terminated following instances if not in check mode {0}'.format(terminate_ids))
+                module.exit_json(
+                    changed=True,
+                    terminated_ids=terminate_ids,
+                    instance_ids=all_instance_ids,
+                    msg=f"Would have terminated following instances if not in check mode {terminate_ids}",
+                )
             # terminate instances
             try:
                 client.terminate_instances(aws_retry=True, InstanceIds=terminate_ids)
@@ -1852,10 +1877,14 @@ def enforce_count(existing_matches, module, desired_module_state):
                 pass
             except botocore.exceptions.ClientError as e:  # pylint: disable=duplicate-except
                 module.fail_json(e, msg='Unable to terminate instances')
+            # include data for all matched instances in addition to the list of terminations
+            # allowing for recovery of metadata from the destructive operation
             module.exit_json(
                 changed=True,
                 msg='Successfully terminated instances.',
                 terminated_ids=terminate_ids,
+                instance_ids=all_instance_ids,
+                instances=existing_matches,
             )
 
     except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e:
@@ -1872,11 +1901,21 @@ def ensure_present(existing_matches, desired_module_state):
         instance_spec = build_run_instance_spec(module.params)
         # If check mode is enabled,suspend 'ensure function'.
         if module.check_mode:
-            module.exit_json(
-                changed=True,
-                spec=instance_spec,
-                msg='Would have launched instances if not in check_mode.',
-            )
+            if existing_matches:
+                instance_ids = [x["InstanceId"] for x in existing_matches]
+                module.exit_json(
+                    changed=True,
+                    instance_ids=instance_ids,
+                    instances=existing_matches,
+                    spec=instance_spec,
+                    msg="Would have launched instances if not in check_mode.",
+                )
+            else:
+                module.exit_json(
+                    changed=True,
+                    spec=instance_spec,
+                    msg="Would have launched instances if not in check_mode.",
+                )
         instance_response = run_instances(**instance_spec)
         instances = instance_response['Instances']
         instance_ids = [i['InstanceId'] for i in instances]
@@ -1904,22 +1943,43 @@ def ensure_present(existing_matches, desired_module_state):
                     client.modify_instance_attribute(aws_retry=True, **c)
                 except botocore.exceptions.ClientError as e:
                     module.fail_json_aws(e, msg="Could not apply change {0} to new instance.".format(str(c)))
+        if existing_matches:
+            # If we came from enforce_count, create a second list to distinguish
+            # between existing and new instances when returning the entire cohort
+            all_instance_ids = [x["InstanceId"] for x in existing_matches] + instance_ids
+        if not module.params.get("wait"):
+            if existing_matches:
+                module.exit_json(
+                    changed=True,
+                    changed_ids=instance_ids,
+                    instance_ids=all_instance_ids,
+                    spec=instance_spec,
+                )
+            else:
+                module.exit_json(
+                    changed=True,
+                    instance_ids=instance_ids,
+                    spec=instance_spec,
+                )
+        await_instances(instance_ids, desired_module_state=desired_module_state)
+        instances = find_instances(ids=instance_ids)
 
-        if not module.params.get('wait'):
+        if existing_matches:
+            all_instances = existing_matches + instances
+            module.exit_json(
+                changed=True,
+                changed_ids=instance_ids,
+                instance_ids=all_instance_ids,
+                instances=[pretty_instance(i) for i in all_instances],
+                spec=instance_spec,
+            )
+        else:
             module.exit_json(
                 changed=True,
                 instance_ids=instance_ids,
+                instances=[pretty_instance(i) for i in instances],
                 spec=instance_spec,
             )
-        await_instances(instance_ids, desired_module_state=desired_module_state)
-        instances = find_instances(ids=instance_ids)
-
-        module.exit_json(
-            changed=True,
-            instances=[pretty_instance(i) for i in instances],
-            instance_ids=instance_ids,
-            spec=instance_spec,
-        )
     except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e:
         module.fail_json_aws(e, msg="Failed to create new EC2 instance")
 

diff --git a/tests/integration/targets/ec2_instance_instance_multiple/tasks/main.yml b/tests/integration/targets/ec2_instance_instance_multiple/tasks/main.yml
@@ -164,7 +164,8 @@
       that:
       - create_multiple_instances is not failed
       - create_multiple_instances is not changed
-      - '"instance_ids" not in create_multiple_instances'
+      - '"instance_ids" in create_multiple_instances'
+      - create_multiple_instances.instance_ids | length == 5
       - '"ec2:RunInstances" not in create_multiple_instances.resource_actions'
 
   - name: "Enforce instance count - launch 5 instances (Idempotency)"
@@ -183,7 +184,8 @@
       that:
       - create_multiple_instances is not failed
       - create_multiple_instances is not changed
-      - '"instance_ids" not in create_multiple_instances'
+      - '"instance_ids" in create_multiple_instances'
+      - create_multiple_instances.instance_ids | length == 5
       - '"ec2:RunInstances" not in create_multiple_instances.resource_actions'
 
   - name: "Enforce instance count to 3 - Terminate 2 instances (check_mode)"
@@ -202,7 +204,10 @@
       that:
       - terminate_multiple_instances is not failed
       - terminate_multiple_instances is changed
-      - '"instance_ids" not in terminate_multiple_instances'
+      - '"instance_ids" in terminate_multiple_instances'
+      - terminate_multiple_instances.instance_ids | length == 5
+      - '"terminated_ids" in terminate_multiple_instances'
+      - terminate_multiple_instances.terminated_ids | length == 2
       - '"ec2:RunInstances" not in terminate_multiple_instances.resource_actions'
 
   - name: "Enforce instance count to 3 - Terminate 2 instances"
@@ -221,6 +226,8 @@
       that:
       - terminate_multiple_instances is not failed
       - terminate_multiple_instances is changed
+      - '"instance_ids" in terminate_multiple_instances'
+      - terminate_multiple_instances.instance_ids | length == 5
       - '"terminated_ids" in terminate_multiple_instances'
       - terminate_multiple_instances.terminated_ids | length == 2
 
@@ -240,7 +247,9 @@
       that:
       - terminate_multiple_instances is not failed
       - terminate_multiple_instances is not changed
-      - '"instance_ids" not in terminate_multiple_instances'
+      - '"instance_ids" in terminate_multiple_instances'
+      - terminate_multiple_instances.instance_ids | length == 3
+      - '"terminated_ids" not in terminate_multiple_instances'
       - '"ec2:TerminateInstances" not in terminate_multiple_instances.resource_actions'
 
   - name: "Enforce instance count to 3 - Terminate 2 instances (Idempotency)"
@@ -258,7 +267,9 @@
       that:
       - terminate_multiple_instances is not failed
       - terminate_multiple_instances is not changed
-      - '"instance_ids" not in terminate_multiple_instances'
+      - '"instance_ids" in terminate_multiple_instances'
+      - terminate_multiple_instances.instance_ids | length == 3
+      - '"terminated_ids" not in terminate_multiple_instances'
       - '"ec2:TerminateInstances" not in terminate_multiple_instances.resource_actions'
 
   - name: "Enforce instance count to 6 - Launch 3 more instances (check_mode)"
@@ -278,7 +289,9 @@
       that:
       - create_multiple_instances is not failed
       - create_multiple_instances is changed
-      - '"instance_ids" not in create_multiple_instances'
+      - '"instance_ids" in create_multiple_instances'
+      - create_multiple_instances.instance_ids | length == 3
+      - '"changed_ids" not in create_multiple_instances'
       - '"ec2:RunInstances" not in create_multiple_instances.resource_actions'
 
   - name: "Enforce instance count to 6 - Launch 3 more instances"
@@ -301,7 +314,9 @@
       - create_multiple_instances is not failed
       - create_multiple_instances is changed
       - '"instance_ids" in create_multiple_instances'
-      - create_multiple_instances.instance_ids | length == 3
+      - create_multiple_instances.instance_ids | length == 6
+      - '"changed_ids" in create_multiple_instances'
+      - create_multiple_instances.changed_ids | length == 3
 
   - name: "Enforce instance count to 6 - Launch 3 more instances (check_mode - Idempotency)"
     ec2_instance:
@@ -320,7 +335,9 @@
       that:
       - create_multiple_instances is not failed
       - create_multiple_instances is not changed
-      - '"instance_ids" not in create_multiple_instances'
+      - '"instance_ids" in create_multiple_instances'
+      - create_multiple_instances.instance_ids | length == 6
+      - '"changed_ids" not in create_multiple_instances'
       - '"ec2:RunInstances" not in create_multiple_instances.resource_actions'
 
   - name: "Enforce instance count to 6 - Launch 3 more instances (Idempotency)"
@@ -339,7 +356,9 @@
       that:
       - create_multiple_instances is not failed
       - create_multiple_instances is not changed
-      - '"instance_ids" not in create_multiple_instances'
+      - '"instance_ids" in create_multiple_instances'
+      - create_multiple_instances.instance_ids | length == 6
+      - '"changed_ids" not in create_multiple_instances'
       - '"ec2:RunInstances" not in create_multiple_instances.resource_actions'