diff --git a/changelogs/fragments/22-ec2_instance-mod-sgs.yml b/changelogs/fragments/22-ec2_instance-mod-sgs.yml new file mode 100644 index 00000000000..c8145d6624c --- /dev/null +++ b/changelogs/fragments/22-ec2_instance-mod-sgs.yml @@ -0,0 +1,2 @@ +bugfixes: +- ec2_instance - fixes issue where security groups were not changed if the instance already existed. https://github.com/ansible-collections/community.aws/pull/22 diff --git a/plugins/modules/ec2_instance.py b/plugins/modules/ec2_instance.py index 0b268a6f05a..07f1189664e 100644 --- a/plugins/modules/ec2_instance.py +++ b/plugins/modules/ec2_instance.py @@ -808,9 +808,9 @@ except ImportError: pass # caught by AnsibleAWSModule -from ansible.module_utils.six import text_type, string_types +from ansible.module_utils.six import string_types from ansible.module_utils.six.moves.urllib import parse as urlparse -from ansible.module_utils._text import to_bytes, to_native +from ansible.module_utils._text import to_native import ansible_collections.amazon.aws.plugins.module_utils.ec2 as ec2_utils from ansible_collections.amazon.aws.plugins.module_utils.ec2 import (AWSRetry, ansible_dict_to_boto3_filter_list, @@ -1337,15 +1337,47 @@ def value_wrapper(v): ] for mapping in param_mappings: - if params.get(mapping.param_key) is not None and mapping.instance_key not in skip: - value = AWSRetry.jittered_backoff()(ec2.describe_instance_attribute)(Attribute=mapping.attribute_name, InstanceId=id_) - if params.get(mapping.param_key) is not None and value[mapping.instance_key]['Value'] != params.get(mapping.param_key): - arguments = dict( - InstanceId=instance['InstanceId'], - # Attribute=mapping.attribute_name, - ) - arguments[mapping.instance_key] = mapping.add_value(params.get(mapping.param_key)) - changes_to_apply.append(arguments) + if params.get(mapping.param_key) is None: + continue + if mapping.instance_key in skip: + continue + + value = AWSRetry.jittered_backoff()(ec2.describe_instance_attribute)(Attribute=mapping.attribute_name, InstanceId=id_) + if value[mapping.instance_key]['Value'] != params.get(mapping.param_key): + arguments = dict( + InstanceId=instance['InstanceId'], + # Attribute=mapping.attribute_name, + ) + arguments[mapping.instance_key] = mapping.add_value(params.get(mapping.param_key)) + changes_to_apply.append(arguments) + + if params.get('security_group') or params.get('security_groups'): + value = AWSRetry.jittered_backoff()(ec2.describe_instance_attribute)(Attribute="groupSet", InstanceId=id_) + # managing security groups + if params.get('vpc_subnet_id'): + subnet_id = params.get('vpc_subnet_id') + else: + default_vpc = get_default_vpc(ec2) + if default_vpc is None: + module.fail_json( + msg="No default subnet could be found - you must include a VPC subnet ID (vpc_subnet_id parameter) to modify security groups.") + else: + sub = get_default_subnet(ec2, default_vpc) + subnet_id = sub['SubnetId'] + + groups = discover_security_groups( + group=params.get('security_group'), + groups=params.get('security_groups'), + subnet_id=subnet_id, + ec2=ec2 + ) + expected_groups = [g['GroupId'] for g in groups] + instance_groups = [g['GroupId'] for g in value['Groups']] + if set(instance_groups) != set(expected_groups): + changes_to_apply.append(dict( + Groups=expected_groups, + InstanceId=instance['InstanceId'] + )) if (params.get('network') or {}).get('source_dest_check') is not None: # network.source_dest_check is nested, so needs to be treated separately diff --git a/tests/integration/targets/ec2_instance/inventory b/tests/integration/targets/ec2_instance/inventory index 44b46ec88f7..09bae76beb1 100644 --- a/tests/integration/targets/ec2_instance/inventory +++ b/tests/integration/targets/ec2_instance/inventory @@ -11,6 +11,7 @@ iam_instance_role termination_protection tags_and_vpc_settings checkmode_tests +security_group [all:vars] ansible_connection=local diff --git a/tests/integration/targets/ec2_instance/roles/ec2_instance/tasks/env_cleanup.yml b/tests/integration/targets/ec2_instance/roles/ec2_instance/tasks/env_cleanup.yml index 1b6c79e0d95..07c7f72bd8e 100644 --- a/tests/integration/targets/ec2_instance/roles/ec2_instance/tasks/env_cleanup.yml +++ b/tests/integration/targets/ec2_instance/roles/ec2_instance/tasks/env_cleanup.yml @@ -33,6 +33,17 @@ ignore_errors: yes retries: 10 +- name: "remove the second security group" + ec2_group: + name: "{{ resource_prefix }}-sg-2" + description: a security group for ansible tests + vpc_id: "{{ testing_vpc.vpc.id }}" + state: absent + register: removed + until: removed is not failed + ignore_errors: yes + retries: 10 + - name: "remove routing rules" ec2_vpc_route_table: state: absent diff --git a/tests/integration/targets/ec2_instance/roles/ec2_instance/tasks/env_setup.yml b/tests/integration/targets/ec2_instance/roles/ec2_instance/tasks/env_setup.yml index 6c76b7bf79f..7c99f807177 100644 --- a/tests/integration/targets/ec2_instance/roles/ec2_instance/tasks/env_setup.yml +++ b/tests/integration/targets/ec2_instance/roles/ec2_instance/tasks/env_setup.yml @@ -77,3 +77,19 @@ to_port: 80 cidr_ip: 0.0.0.0/0 register: sg + + - name: "create secondary security group with the vpc" + ec2_group: + name: "{{ resource_prefix }}-sg-2" + description: a secondary security group for ansible tests + vpc_id: "{{ testing_vpc.vpc.id }}" + rules: + - proto: tcp + from_port: 22 + to_port: 22 + cidr_ip: 0.0.0.0/0 + - proto: tcp + from_port: 80 + to_port: 80 + cidr_ip: 0.0.0.0/0 + register: sg2 diff --git a/tests/integration/targets/ec2_instance/roles/ec2_instance/tasks/security_group.yml b/tests/integration/targets/ec2_instance/roles/ec2_instance/tasks/security_group.yml new file mode 100644 index 00000000000..c0e52a5f386 --- /dev/null +++ b/tests/integration/targets/ec2_instance/roles/ec2_instance/tasks/security_group.yml @@ -0,0 +1,81 @@ +- block: + - name: "New instance with 2 security groups" + ec2_instance: + name: "{{ resource_prefix }}-test-security-groups" + image_id: "{{ ec2_ami_image }}" + vpc_subnet_id: "{{ testing_subnet_a.subnet.id }}" + tags: + TestId: "{{ resource_prefix }}" + instance_type: t2.micro + wait: false + security_groups: + - "{{ sg.group_id }}" + - "{{ sg2.group_id }}" + register: security_groups_test + + - name: "Recreate same instance with 2 security groups ( Idempotency )" + ec2_instance: + name: "{{ resource_prefix }}-test-security-groups" + image_id: "{{ ec2_ami_image }}" + vpc_subnet_id: "{{ testing_subnet_a.subnet.id }}" + tags: + TestId: "{{ resource_prefix }}" + instance_type: t2.micro + wait: false + security_groups: + - "{{ sg.group_id }}" + - "{{ sg2.group_id }}" + register: security_groups_test_idempotency + + - name: "Gather ec2 facts to check SGs have been added" + ec2_instance_info: + filters: + "tag:Name": "{{ resource_prefix }}-test-security-groups" + "instance-state-name": "running" + register: dual_sg_instance_facts + until: dual_sg_instance_facts.instances | length > 0 + retries: 10 + + - name: "Remove secondary security group from instance" + ec2_instance: + name: "{{ resource_prefix }}-test-security-groups" + image_id: "{{ ec2_ami_image }}" + vpc_subnet_id: "{{ testing_subnet_a.subnet.id }}" + tags: + TestId: "{{ resource_prefix }}" + instance_type: t2.micro + security_groups: + - "{{ sg.group_id }}" + register: remove_secondary_security_group + + - name: "Gather ec2 facts to check seconday SG has been removed" + ec2_instance_info: + filters: + "tag:Name": "{{ resource_prefix }}-test-security-groups" + "instance-state-name": "running" + register: single_sg_instance_facts + until: single_sg_instance_facts.instances | length > 0 + retries: 10 + + - name: "Add secondary security group to instance" + ec2_instance: + name: "{{ resource_prefix }}-test-security-groups" + image_id: "{{ ec2_ami_image }}" + vpc_subnet_id: "{{ testing_subnet_a.subnet.id }}" + tags: + TestId: "{{ resource_prefix }}" + instance_type: t2.micro + security_groups: + - "{{ sg.group_id }}" + - "{{ sg2.group_id }}" + register: add_secondary_security_group + + - assert: + that: + - security_groups_test is not failed + - security_groups_test is changed + - security_groups_test_idempotency is not changed + - remove_secondary_security_group is changed + - single_sg_instance_facts.instances.0.security_groups | length == 1 + - dual_sg_instance_facts.instances.0.security_groups | length == 2 + - add_secondary_security_group is changed