Fix ec2_instance eventual consistency when wait: false (#51885)

* Do not return 'instances' when wait is false

* Added integration tests for wait: false

* Added changelog fragment

* Fix test suite to work with ec2_instance

* Additional permissions
* Enforce boto3 version
* Fix broken tests
* Improve error messages

* fix linter issues

changelogs/fragments/51885-ec2_instance-fix-eventual-consistency.yaml

@@ -0,0 +1,2 @@
+bugfixes:
+  - "ec2_instance - Does not return ``instances`` when ``wait: false`` is specified"

hacking/aws_config/testing_policies/compute-policy.json

@@ -42,6 +42,7 @@
                 "ec2:AssociateVpcCidrBlock",
                 "ec2:AssociateSubnetCidrBlock",
                 "ec2:AttachInternetGateway",
+                "ec2:AttachNetworkInterface",
                 "ec2:AttachVpnGateway",
                 "ec2:CreateCustomerGateway",
                 "ec2:CreateDhcpOptions",
@@ -80,6 +81,7 @@
                 "ec2:DisassociateSubnetCidrBlock",
                 "ec2:ImportKeyPair",
                 "ec2:ModifyImageAttribute",
+                "ec2:ModifyInstanceAttribute",
                 "ec2:ModifySubnetAttribute",
                 "ec2:ModifyVpcAttribute",
                 "ec2:RegisterImage",
@@ -102,6 +104,8 @@
                 "ec2:RevokeSecurityGroupEgress",
                 "ec2:RevokeSecurityGroupIngress",
                 "ec2:RunInstances",
+                "ec2:StartInstances",
+                "ec2:StopInstances",
                 "ec2:TerminateInstances",
                 "ec2:UpdateSecurityGroupRuleDescriptionsIngress",
                 "ec2:UpdateSecurityGroupRuleDescriptionsEgress"

hacking/aws_config/testing_policies/container-policy.json

@@ -45,8 +45,6 @@
             "ecs:StopTask",
             "ecs:UpdateService",
             "elasticloadbalancing:Describe*",
-            "iam:AttachRolePolicy",
-            "iam:CreateRole",
             "iam:GetPolicy",
             "iam:GetPolicyVersion",
             "iam:GetRole",

hacking/aws_config/testing_policies/security-policy.json

@@ -26,6 +26,43 @@
             "Effect": "Allow",
             "Sid": "AllowReadOnlyIAMUse"
         },
+        {
+            "Action": [
+                "iam:AttachRolePolicy",
+                "iam:CreateRole",
+                "iam:DeleteRole",
+                "iam:DetachRolePolicy",
+                "iam:PassRole"
+             ],
+            "Resource": "arn:aws:iam::{{ aws_account }}:role/ansible-test-*",
+            "Effect": "Allow",
+            "Sid": "AllowUpdateOfSpecificRoles"
+        },
+        {
+            "Action": [
+                "iam:CreateInstanceProfile",
+                "iam:DeleteInstanceProfile",
+                "iam:AddRoleToInstanceProfile",
+                "iam:RemoveRoleFromInstanceProfile"
+             ],
+            "Resource": "arn:aws:iam::{{ aws_account }}:instance-profile/ansible-test-*",
+            "Effect": "Allow",
+            "Sid": "AllowUpdateOfSpecificInstanceProfiles"
+        },
+        {
+            "Action": [
+                "ec2:ReplaceIamInstanceProfileAssociation"
+             ],
+            "Resource": "*",
+            "Condition": {
+              "ArnEquals": {
+                "ec2:InstanceProfile": "arn:aws:iam::{{ aws_account }}:instance-profile/ansible-test-*"
+              }
+            },
+            "Effect": "Allow",
+            "Sid": "AllowReplacementOfSpecificInstanceProfiles"
+        },
+
         {
             "Sid": "AllowWAFusage",
             "Action": "waf:*",

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

@@ -301,7 +301,7 @@ EXAMPLES = '''
 RETURN = '''
 instances:
     description: a list of ec2 instances
-    returned: always
+    returned: when wait == true
     type: complex
     contains:
         ami_launch_index:
@@ -1335,11 +1335,11 @@ def ensure_instance_state(state, ec2=None):
     if ec2 is None:
         module.client('ec2')
     if state in ('running', 'started'):
-        changed, failed, instances = change_instance_state(filters=module.params.get('filters'), desired_state='RUNNING')
+        changed, failed, instances, failure_reason = change_instance_state(filters=module.params.get('filters'), desired_state='RUNNING')
 
         if failed:
             module.fail_json(
-                msg="Unable to start instances",
+                msg="Unable to start instances: {0}".format(failure_reason),
                 reboot_success=list(changed),
                 reboot_failed=failed)
 
@@ -1351,16 +1351,16 @@ def ensure_instance_state(state, ec2=None):
             instances=[pretty_instance(i) for i in instances],
         )
     elif state in ('restarted', 'rebooted'):
-        changed, failed, instances = change_instance_state(
+        changed, failed, instances, failure_reason = change_instance_state(
             filters=module.params.get('filters'),
             desired_state='STOPPED')
-        changed, failed, instances = change_instance_state(
+        changed, failed, instances, failure_reason = change_instance_state(
             filters=module.params.get('filters'),
             desired_state='RUNNING')
 
         if failed:
             module.fail_json(
-                msg="Unable to restart instances",
+                msg="Unable to restart instances: {0}".format(failure_reason),
                 reboot_success=list(changed),
                 reboot_failed=failed)
 
@@ -1372,13 +1372,13 @@ def ensure_instance_state(state, ec2=None):
             instances=[pretty_instance(i) for i in instances],
         )
     elif state in ('stopped',):
-        changed, failed, instances = change_instance_state(
+        changed, failed, instances, failure_reason = change_instance_state(
             filters=module.params.get('filters'),
             desired_state='STOPPED')
 
         if failed:
             module.fail_json(
-                msg="Unable to stop instances",
+                msg="Unable to stop instances: {0}".format(failure_reason),
                 stop_success=list(changed),
                 stop_failed=failed)
 
@@ -1390,13 +1390,13 @@ def ensure_instance_state(state, ec2=None):
             instances=[pretty_instance(i) for i in instances],
         )
     elif state in ('absent', 'terminated'):
-        terminated, terminate_failed, instances = change_instance_state(
+        terminated, terminate_failed, instances, failure_reason = change_instance_state(
             filters=module.params.get('filters'),
             desired_state='TERMINATED')
 
         if terminate_failed:
             module.fail_json(
-                msg="Unable to terminate instances",
+                msg="Unable to terminate instances: {0}".format(failure_reason),
                 terminate_success=list(terminated),
                 terminate_failed=terminate_failed)
         module.exit_json(
@@ -1418,6 +1418,7 @@ def change_instance_state(filters, desired_state, ec2=None):
     instances = find_instances(ec2, filters=filters)
     to_change = set(i['InstanceId'] for i in instances if i['State']['Name'].upper() != desired_state)
     unchanged = set()
+    failure_reason = ""
 
     for inst in instances:
         try:
@@ -1448,16 +1449,18 @@ def change_instance_state(filters, desired_state, ec2=None):
 
                 resp = ec2.start_instances(InstanceIds=[inst['InstanceId']])
                 [changed.add(i['InstanceId']) for i in resp['StartingInstances']]
-        except (botocore.exceptions.ClientError, botocore.exceptions.BotoCoreError):
-            # we don't care about exceptions here, as we'll fail out if any instances failed to terminate
-            pass
+        except (botocore.exceptions.ClientError, botocore.exceptions.BotoCoreError) as e:
+            try:
+                failure_reason = to_native(e.message)
+            except AttributeError:
+                failure_reason = to_native(e)
 
     if changed:
         await_instances(ids=list(changed) + list(unchanged), state=desired_state)
 
     change_failed = list(to_change - changed)
     instances = find_instances(ec2, ids=list(i['InstanceId'] for i in instances))
-    return changed, change_failed, instances
+    return changed, change_failed, instances, failure_reason
 
 
 def pretty_instance(i):
@@ -1481,7 +1484,9 @@ def determine_iam_role(name_or_arn):
 
 def handle_existing(existing_matches, changed, ec2, state):
     if state in ('running', 'started') and [i for i in existing_matches if i['State']['Name'] != 'running']:
-        ins_changed, failed, instances = change_instance_state(filters=module.params.get('filters'), desired_state='RUNNING')
+        ins_changed, failed, instances, failure_reason = change_instance_state(filters=module.params.get('filters'), desired_state='RUNNING')
+        if failed:
+            module.fail_json(msg="Couldn't start instances: {0}. Failure reason: {1}".format(instances, failure_reason))
         module.exit_json(
             changed=bool(len(ins_changed)) or changed,
             instances=[pretty_instance(i) for i in instances],
@@ -1532,6 +1537,12 @@ def ensure_present(existing_matches, changed, ec2, state):
                 except botocore.exceptions.ClientError as e:
                     module.fail_json_aws(e, msg="Could not apply change {0} to new instance.".format(str(c)))
 
+        if not module.params.get('wait'):
+            module.exit_json(
+                changed=True,
+                instance_ids=instance_ids,
+                spec=instance_spec,
+            )
         await_instances(instance_ids)
         instances = ec2.get_paginator('describe_instances').paginate(
             InstanceIds=instance_ids

test/integration/targets/ec2_instance/playbooks/roles/ec2_instance/tasks/checkmode_tests.yml

@@ -20,7 +20,7 @@
           ebs:
             delete_on_termination: true
         <<: *aws_connection_info
-      register: instance_creation
+      register: basic_instance
 
     - name: Make basic instance(check mode)
       ec2_instance:
@@ -28,7 +28,7 @@
         image_id: "{{ ec2_ami_image[aws_region] }}"
         security_groups: "{{ sg.group_id }}"
         instance_type: t2.micro
-        vpc_subnet_id: "{{ testing_subnet_c.subnet.id }}"
+        vpc_subnet_id: "{{ testing_subnet_b.subnet.id }}"
         volumes:
         - device_name: /dev/sda1
           ebs:

test/integration/targets/ec2_instance/playbooks/roles/ec2_instance/tasks/instance_no_wait.yml

@@ -0,0 +1,64 @@
+- name: set connection information for all tasks
+  set_fact:
+    aws_connection_info: &aws_connection_info
+      aws_access_key: "{{ aws_access_key }}"
+      aws_secret_key: "{{ aws_secret_key }}"
+      security_token: "{{ security_token }}"
+      region: "{{ aws_region }}"
+  no_log: true
+
+- name: New instance and don't wait for it to complete
+  ec2_instance:
+    name: "{{ resource_prefix }}-test-no-wait"
+    image_id: "{{ ec2_ami_image[aws_region] }}"
+    vpc_subnet_id: "{{ testing_subnet_b.subnet.id }}"
+    tags:
+      TestId: "{{ resource_prefix }}"
+    wait: false
+    instance_type: t2.micro
+    <<: *aws_connection_info
+  register: in_test_vpc
+
+- assert:
+    that:
+    - in_test_vpc is not failed
+    - in_test_vpc is changed
+    - in_test_vpc.instances is not defined
+    - in_test_vpc.instance_ids is defined
+    - in_test_vpc.instance_ids | length > 0
+
+- name: New instance and don't wait for it to complete ( check mode )
+  ec2_instance:
+    name: "{{ resource_prefix }}-test-no-wait-checkmode"
+    image_id: "{{ ec2_ami_image[aws_region] }}"
+    vpc_subnet_id: "{{ testing_subnet_b.subnet.id }}"
+    tags:
+      TestId: "{{ resource_prefix }}"
+    wait: false
+    instance_type: t2.micro
+    <<: *aws_connection_info
+  check_mode: yes
+
+- name: Facts for ec2 test instance
+  ec2_instance_facts:
+    filters:
+      "tag:Name": "{{ resource_prefix }}-test-no-wait"
+      "instance-state-name": "running"
+    <<: *aws_connection_info
+  register: real_instance_fact
+  until: real_instance_fact.instances | length > 0
+  retries: 10
+
+- name: Facts for checkmode ec2 test instance
+  ec2_instance_facts:
+    filters:
+      "tag:Name": "{{ resource_prefix }}-test-no-wait-checkmode"
+      "instance-state-name": "running"
+    <<: *aws_connection_info
+  register: checkmode_instance_fact
+
+- name: "Confirm whether the check mode is working normally."
+  assert:
+    that:
+      - "{{ real_instance_fact.instances | length }} > 0"
+      - "{{ checkmode_instance_fact.instances | length }} == 0"

test/integration/targets/ec2_instance/playbooks/roles/ec2_instance/tasks/main.yml

@@ -98,7 +98,7 @@
     - include_tasks: iam_instance_role.yml
     - include_tasks: checkmode_tests.yml
     - include_tasks: ebs_optimized.yml
-
+    - include_tasks: instance_no_wait.yml
 
     # ============================================================
 

test/integration/targets/ec2_instance/playbooks/version_fail.yml

@@ -36,3 +36,11 @@
             that:
               - ec2_instance_cpu_options_creation.failed
               - 'ec2_instance_cpu_options_creation.msg == "cpu_options is only supported with botocore >= 1.10.16"'
+
+      always:
+        - name: cleanup c4.large in case graceful failure was in fact a graceful success
+          ec2_instance:
+            state: absent
+            name: "ansible-test-{{ resource_prefix | regex_search('([0-9]+)$') }}-ec2"
+            <<: *aws_connection_info
+          ignore_errors: yes

test/integration/targets/ec2_instance/runme.sh

@@ -16,7 +16,7 @@ PYTHON=${ANSIBLE_TEST_PYTHON_INTERPRETER:-python}
 export ANSIBLE_ROLES_PATH=../
 virtualenv --system-site-packages --python "${PYTHON}" "${MYTMPDIR}/botocore-less-than-1.10.16"
 source "${MYTMPDIR}/botocore-less-than-1.10.16/bin/activate"
-"${PYTHON}" -m pip install 'botocore<1.10.16' boto3
+"${PYTHON}" -m pip install 'botocore<1.10.16' 'boto3<1.7.16'
 ansible-playbook -i ../../inventory -e @../../integration_config.yml -e @../../cloud-config-aws.yml -v playbooks/version_fail.yml "$@"
 
 # Run full test suite