Bugfix/ec2 instance mod sgs

plugins/modules/ec2_instance.py

@@ -808,11 +808,12 @@
 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,
+                                                                     ec2_argument_spec,
                                                                      ansible_dict_to_boto3_filter_list,
                                                                      compare_aws_tags,
                                                                      boto3_tag_list_to_ansible_dict,
@@ -1332,20 +1333,48 @@ def value_wrapper(v):
     param_mappings = [
         ParamMapper('ebs_optimized', 'EbsOptimized', 'ebsOptimized', value_wrapper),
         ParamMapper('termination_protection', 'DisableApiTermination', 'disableApiTermination', value_wrapper),
+        ParamMapper('security_groups', 'Groups', 'groupSet', value_wrapper),
+        ParamMapper('security_group', 'Groups', 'groupSet', value_wrapper),
         # user data is an immutable property
         # ParamMapper('user_data', 'UserData', 'userData', value_wrapper),
     ]
 
     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,
+            if mapping.param_key not in ['security_groups', 'security_group']:
+                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)
+            elif bool(params.get(mapping.param_key)) is not False:
+                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:
+                        raise module.fail_json(
+                            msg="No default subnet could be found - you must include a VPC subnet ID (vpc_subnet_id parameter) to create an instance")
+                    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
                 )
-                arguments[mapping.instance_key] = mapping.add_value(params.get(mapping.param_key))
-                changes_to_apply.append(arguments)
+                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