From 5c6b16edc3f6cc8a1c9a87e32b24429572e51f6e Mon Sep 17 00:00:00 2001 From: Andrea Tartaglia Date: Wed, 6 Mar 2019 12:46:37 +0000 Subject: [PATCH] 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 --- ...ec2_instance-fix-eventual-consistency.yaml | 2 + .../testing_policies/compute-policy.json | 4 ++ .../testing_policies/container-policy.json | 2 - .../testing_policies/security-policy.json | 37 +++++++++++ .../modules/cloud/amazon/ec2_instance.py | 41 +++++++----- .../ec2_instance/tasks/checkmode_tests.yml | 4 +- .../ec2_instance/tasks/instance_no_wait.yml | 64 +++++++++++++++++++ .../roles/ec2_instance/tasks/main.yml | 2 +- .../ec2_instance/playbooks/version_fail.yml | 8 +++ .../integration/targets/ec2_instance/runme.sh | 2 +- 10 files changed, 145 insertions(+), 21 deletions(-) create mode 100644 changelogs/fragments/51885-ec2_instance-fix-eventual-consistency.yaml create mode 100644 test/integration/targets/ec2_instance/playbooks/roles/ec2_instance/tasks/instance_no_wait.yml diff --git a/changelogs/fragments/51885-ec2_instance-fix-eventual-consistency.yaml b/changelogs/fragments/51885-ec2_instance-fix-eventual-consistency.yaml new file mode 100644 index 00000000000..9bf66bfd754 --- /dev/null +++ b/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" diff --git a/hacking/aws_config/testing_policies/compute-policy.json b/hacking/aws_config/testing_policies/compute-policy.json index 3a18c175529..94b1baae39f 100644 --- a/hacking/aws_config/testing_policies/compute-policy.json +++ b/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" diff --git a/hacking/aws_config/testing_policies/container-policy.json b/hacking/aws_config/testing_policies/container-policy.json index 944559c1595..4bf60a80b23 100644 --- a/hacking/aws_config/testing_policies/container-policy.json +++ b/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", diff --git a/hacking/aws_config/testing_policies/security-policy.json b/hacking/aws_config/testing_policies/security-policy.json index da25b72dc20..b1760a6bec9 100644 --- a/hacking/aws_config/testing_policies/security-policy.json +++ b/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:*", diff --git a/lib/ansible/modules/cloud/amazon/ec2_instance.py b/lib/ansible/modules/cloud/amazon/ec2_instance.py index 81ec8bfb5a2..83ba89e1c29 100644 --- a/lib/ansible/modules/cloud/amazon/ec2_instance.py +++ b/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 diff --git a/test/integration/targets/ec2_instance/playbooks/roles/ec2_instance/tasks/checkmode_tests.yml b/test/integration/targets/ec2_instance/playbooks/roles/ec2_instance/tasks/checkmode_tests.yml index 8cd0e7dfcba..9bf90c50714 100644 --- a/test/integration/targets/ec2_instance/playbooks/roles/ec2_instance/tasks/checkmode_tests.yml +++ b/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: diff --git a/test/integration/targets/ec2_instance/playbooks/roles/ec2_instance/tasks/instance_no_wait.yml b/test/integration/targets/ec2_instance/playbooks/roles/ec2_instance/tasks/instance_no_wait.yml new file mode 100644 index 00000000000..b69b7439a8d --- /dev/null +++ b/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" diff --git a/test/integration/targets/ec2_instance/playbooks/roles/ec2_instance/tasks/main.yml b/test/integration/targets/ec2_instance/playbooks/roles/ec2_instance/tasks/main.yml index 9f05b0976d8..89c287265c9 100644 --- a/test/integration/targets/ec2_instance/playbooks/roles/ec2_instance/tasks/main.yml +++ b/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 # ============================================================ diff --git a/test/integration/targets/ec2_instance/playbooks/version_fail.yml b/test/integration/targets/ec2_instance/playbooks/version_fail.yml index 517164d1971..6f20b645682 100644 --- a/test/integration/targets/ec2_instance/playbooks/version_fail.yml +++ b/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 diff --git a/test/integration/targets/ec2_instance/runme.sh b/test/integration/targets/ec2_instance/runme.sh index 7aeebaaefc5..f347d78d23e 100755 --- a/test/integration/targets/ec2_instance/runme.sh +++ b/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