ec2_group - Add egress_rules and purge_egress_rules aliases

SUMMARY Fix #440 - exception running with --diff and --check when creating a new SG by adding a rule referencing a missing SG. Add egress_rules and purge_egress_rules aliases - while cleaning up the tests I kept writing "egress_rules" rather than "rules_egress". Cleanup and re-enable ec2_group tests Remove "Classic Networking" tests (we don't have this in CI and it'll be gone in August) Fix Group/VPC deletion in tests (cross-referenced rules weren't being deleted, wipe the rules first) ISSUE TYPE Feature Pull Request COMPONENT NAME ec2_group ADDITIONAL INFORMATION fixes: #440 Reviewed-by: Alina Buzachis <None> (cherry picked from commit 922800d)
ansible-collections · Jun 21, 2022 · 28b1bd7 · 28b1bd7
1 parent 024ffde
commit 28b1bd7
Show file tree

Hide file tree

Showing 6 changed files with 80 additions and 158 deletions.
diff --git a/changelogs/fragments/878-ec2_group.yml b/changelogs/fragments/878-ec2_group.yml
@@ -0,0 +1,5 @@
+minor_changes:
+- ec2_group - add ``purge_egress_rules`` as an alias for ``purge_rules_egress`` (https://github.com/ansible-collections/amazon.aws/pull/878).
+- ec2_group - add ``egress_rules`` as an alias for ``rules_egress`` (https://github.com/ansible-collections/amazon.aws/pull/878).
+bugfixes:
+- ec2_group - fix uncaught exception when running with ``--diff`` and ``--check`` to create a new security group (https://github.com/ansible-collections/amazon.aws/issues/440).
diff --git a/plugins/modules/ec2_group.py b/plugins/modules/ec2_group.py
@@ -117,6 +117,7 @@
     required: false
     type: list
     elements: dict
+    aliases: ['egress_rules']
     suboptions:
         cidr_ip:
             type: str
@@ -195,7 +196,7 @@
       - Purge existing rules_egress on security group that are not found in rules_egress.
     required: false
     default: 'true'
-    aliases: []
+    aliases: ['purge_egress_rules']
     type: bool
   tags:
     description:
@@ -1089,16 +1090,28 @@ def get_final_rules(client, module, security_group_rules, specified_rules, purge
                             rule[source_type] = [rule[source_type]]
                         format_rule[rule_key] = [{source_type: target} for target in rule[source_type]]
             if rule.get('group_id') or rule.get('group_name'):
-                rule_sg = camel_dict_to_snake_dict(group_exists(client, module, module.params['vpc_id'], rule.get('group_id'), rule.get('group_name'))[0])
-                format_rule['user_id_group_pairs'] = [{
-                    'description': rule_sg.get('description', rule_sg.get('group_desc')),
-                    'group_id': rule_sg.get('group_id', rule.get('group_id')),
-                    'group_name': rule_sg.get('group_name', rule.get('group_name')),
-                    'peering_status': rule_sg.get('peering_status'),
-                    'user_id': rule_sg.get('user_id', get_account_id(security_group, module)),
-                    'vpc_id': rule_sg.get('vpc_id', module.params['vpc_id']),
-                    'vpc_peering_connection_id': rule_sg.get('vpc_peering_connection_id')
-                }]
+                rule_sg = group_exists(client, module, module.params['vpc_id'], rule.get('group_id'), rule.get('group_name'))[0]
+                if rule_sg is None:
+                    # --diff during --check
+                    format_rule['user_id_group_pairs'] = [{
+                        'group_id': rule.get('group_id'),
+                        'group_name': rule.get('group_name'),
+                        'peering_status': None,
+                        'user_id': get_account_id(security_group, module),
+                        'vpc_id': module.params['vpc_id'],
+                        'vpc_peering_connection_id': None
+                    }]
+                else:
+                    rule_sg = camel_dict_to_snake_dict(rule_sg)
+                    format_rule['user_id_group_pairs'] = [{
+                        'description': rule_sg.get('description', rule_sg.get('group_desc')),
+                        'group_id': rule_sg.get('group_id', rule.get('group_id')),
+                        'group_name': rule_sg.get('group_name', rule.get('group_name')),
+                        'peering_status': rule_sg.get('peering_status'),
+                        'user_id': rule_sg.get('user_id', get_account_id(security_group, module)),
+                        'vpc_id': rule_sg.get('vpc_id', module.params['vpc_id']),
+                        'vpc_peering_connection_id': rule_sg.get('vpc_peering_connection_id')
+                    }]
                 for k, v in list(format_rule['user_id_group_pairs'][0].items()):
                     if v is None:
                         format_rule['user_id_group_pairs'][0].pop(k)
@@ -1168,7 +1181,7 @@ def get_ip_permissions_sort_key(rule):
         return rule.get('prefix_list_ids')[0]['prefix_list_id']
     elif rule.get('user_id_group_pairs'):
         rule.get('user_id_group_pairs').sort(key=get_rule_sort_key)
-        return rule.get('user_id_group_pairs')[0]['group_id']
+        return rule.get('user_id_group_pairs')[0].get('group_id', '')
     return None
 
 
@@ -1179,10 +1192,10 @@ def main():
         description=dict(),
         vpc_id=dict(),
         rules=dict(type='list', elements='dict'),
-        rules_egress=dict(type='list', elements='dict'),
+        rules_egress=dict(type='list', elements='dict', aliases=['egress_rules']),
         state=dict(default='present', type='str', choices=['present', 'absent']),
         purge_rules=dict(default=True, required=False, type='bool'),
-        purge_rules_egress=dict(default=True, required=False, type='bool'),
+        purge_rules_egress=dict(default=True, required=False, type='bool', aliases=['purge_egress_rules']),
         tags=dict(required=False, type='dict', aliases=['resource_tags']),
         purge_tags=dict(default=True, required=False, type='bool')
     )

diff --git a/tests/integration/targets/ec2_group/aliases b/tests/integration/targets/ec2_group/aliases
@@ -1,9 +1,6 @@
-# reason: broken
-# Tests frequently failing
-# https://github.com/ansible-collections/amazon.aws/issues/440
-disabled
-
+# duration: 15
 slow
 
 cloud/aws
+
 ec2_group_info
diff --git a/tests/integration/targets/ec2_group/tasks/ec2_classic.yml b/tests/integration/targets/ec2_group/tasks/ec2_classic.yml
diff --git a/tests/integration/targets/ec2_group/tasks/main.yml b/tests/integration/targets/ec2_group/tasks/main.yml
@@ -1,35 +1,15 @@
 ---
 - set_fact:
-    aws_security_token: '{{ security_token | default("") }}'
+    # lookup plugins don't have access to module_defaults
+    connection_args:
+        region: "{{ aws_region }}"
+        aws_access_key: "{{ aws_access_key }}"
+        aws_secret_key: "{{ aws_secret_key }}"
+        aws_security_token: "{{ security_token | default(omit) }}"
   no_log: True
 
 # ============================================================
-# EC2 Classic tests can only be run on a pre-2013 AWS account with supported-platforms=EC2
-# Ansible CI does NOT have classic EC2 support; these tests are provided as-is for the
-# community and can be run if you have access to a classic account.  To check if your account
-# has support for EC2 Classic you can use the `amazon.aws.aws_account_attribute` plugin.
-
-- name: determine if this is an EC2 Classic account
-  set_fact:
-    has_ec2_classic: "{{ lookup('amazon.aws.aws_account_attribute',
-                                attribute='has-ec2-classic',
-                                region=aws_region,
-                                aws_access_key=aws_access_key,
-                                aws_secret_key=aws_secret_key,
-                                aws_security_token=aws_security_token,
-                                wantlist=True) }}"
-
-# ============================================================
-- name: Run EC2 Classic accounts if account type is EC2
-  include: ./ec2_classic.yml
-  when: has_ec2_classic
-
-# ============================================================
-# Other tests depend on attribute='default-vpc', ie no vpc_id is set. This is
-# incompatible with EC2 classic accounts, so these tests can only be run in a
-# VPC-type account. See "Q. I really want a default VPC for my existing EC2
-# account. Is that possible?" in https://aws.amazon.com/vpc/faqs/#Default_VPCs
-- name: Run all other tests if account type is VPC
+- name: Run all tests
   module_defaults:
       group/aws:
         aws_access_key: "{{ aws_access_key }}"
@@ -39,12 +19,7 @@
   block:
     - name: determine if there is a default VPC
       set_fact:
-        defaultvpc: "{{ lookup('amazon.aws.aws_account_attribute',
-                              attribute='default-vpc',
-                              region=aws_region,
-                              aws_access_key=aws_access_key,
-                              aws_secret_key=aws_secret_key,
-                              aws_security_token=aws_security_token) }}"
+        defaultvpc: "{{ lookup('amazon.aws.aws_account_attribute', attribute='default-vpc', **connection_args) }}"
       register: default_vpc
 
     - name: create a VPC
@@ -1339,34 +1314,51 @@
         that:
            - 'result.changed'
            - 'not result.group_id'
-  when: not has_ec2_classic
-
   always:
+
     # ============================================================
-    - name: tidy up security group
-      ec2_group:
-        name: '{{ec2_group_name}}'
-        state: absent
-      ignore_errors: yes
+    # Describe state of remaining resources
+
+    - name: Retrieve security group info based on SG VPC
+      ec2_group_info:
+        filters:
+          vpc-id: '{{ vpc_result.vpc.id }}'
+      register: remaining_groups
+
+    - name: Retrieve subnet info based on SG VPC
+      ec2_vpc_subnet_info:
+        filters:
+          vpc-id: '{{ vpc_result.vpc.id }}'
+      register: remaining_subnets
+
+    - name: Retrieve VPC info based on SG VPC
+      ec2_vpc_net_info:
+        vpc_ids:
+          - '{{ vpc_result.vpc.id }}'
+      register: remaining_vpc
 
-    - name: tidy up security group for IPv6 EC2-Classic tests
-      ec2_group:
-        name: '{{ ec2_group_name }}-2'
-        state: absent
-      ignore_errors: yes
+    # ============================================================
+    # Delete all remaining SGs
 
-    - name: tidy up default VPC security group
+    - name: Delete rules from remaining SGs
       ec2_group:
-        name: '{{ec2_group_name}}-default-vpc'
-        state: absent
+        name: '{{ item.group_name }}'
+        group_id: '{{ item.group_id }}'
+        description: '{{ item.description }}'
+        rules: []
+        rules_egress: []
+      loop: '{{ remaining_groups.security_groups }}'
       ignore_errors: yes
 
-    - name: tidy up automatically created SG
+    - name: Delete remaining SGs
       ec2_group:
-        name: "{{ resource_prefix }} - Another security group"
         state: absent
+        group_id: '{{ item.group_id }}'
+      loop: '{{ remaining_groups.security_groups }}'
       ignore_errors: yes
 
+    # ============================================================
+
     - name: tidy up VPC
       ec2_vpc_net:
         name: "{{ resource_prefix }}-vpc"

diff --git a/tests/integration/targets/ec2_group/tasks/rule_group_create.yml b/tests/integration/targets/ec2_group/tasks/rule_group_create.yml
@@ -110,11 +110,12 @@
           - result.warning is not defined
 
   always:
-    - name: tidy up egress rule test security group
+    - name: tidy up egress rule test security group rules
       ec2_group:
         name: '{{ec2_group_name}}-auto-create-{{ item }}'
-        state: absent
-        vpc_id: '{{ vpc_result.vpc.id }}'
+        description: '{{ec2_group_description}}'
+        rules: []
+        rules_egress: []
       ignore_errors: yes
       with_items: [5, 4, 3, 2, 1]
     - name: tidy up egress rule test security group