Bugfix/ec2 instance mod sgs (ansible-collections#22)

Fixes #54174

* Added SG handling for existing instances + some cleanup
* tests(ec2_instance): Tests for SG modifications to existing instances
* tests(ec2_instance): Test simultaneous state and SG changes
* refactor(ec2_instance): Move security out of for loop
* style(ec2_instance): Update fail message to reflect security groups
* Add changelog

Co-authored-by: Andrea Tartaglia <[email protected]>
Co-authored-by: Mark Chappell <[email protected]>

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

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

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

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

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