EC2_group module refactor (formerly pr/37255) (#38678)

* Refactor ec2_group

Replace nested for loops with list comprehensions

Purge rules before adding new ones in case sg has maximum permitted rules

* Add check mode tests for ec2_group

* add tests

* Remove dead code

* Fix integration test assertions for old boto versions

* Add waiter for security group that is autocreated

* Add support for in-account group rules

* Add common util to get AWS account ID

Fixes #31383

* Fix protocol number and add separate tests for egress rule handling

* Return egress rule treatment to be backwards compatible

* Remove functions that were obsoleted by `Rule` namedtuple

* IP tests

* Move description updates to a function

* Fix string formatting missing index

* Add tests for auto-creation of the same group in quick succession

* Resolve use of brand-new group in a rule without a description

* Clean up duplicated get-security-group function

* Add reverse cleanup in case of dependency issues

* Add crossaccount ELB group support

* Deal with non-STS calls to account API

* Add filtering of owner IDs that match the current account

test/integration/targets/ec2_group/tasks/credential_tests.yml

@@ -0,0 +1,161 @@
+---
+# A Note about ec2 environment variable name preference:
+#  - EC2_URL -> AWS_URL
+#  - EC2_ACCESS_KEY -> AWS_ACCESS_KEY_ID -> AWS_ACCESS_KEY
+#  - EC2_SECRET_KEY -> AWS_SECRET_ACCESS_KEY -> AWX_SECRET_KEY
+#  - EC2_REGION -> AWS_REGION
+#
+
+# - include: ../../setup_ec2/tasks/common.yml module_name: ec2_group
+
+- block:
+    # ============================================================
+    - name: test failure with no parameters
+      ec2_group:
+      register: result
+      ignore_errors: true
+
+    - name: assert failure with no parameters
+      assert:
+        that:
+           - 'result.failed'
+           - 'result.msg == "one of the following is required: name, group_id"'
+
+    # ============================================================
+    - name: test failure with only name
+      ec2_group:
+        name: '{{ec2_group_name}}'
+      register: result
+      ignore_errors: true
+
+    - name: assert failure with only name
+      assert:
+        that:
+           - 'result.failed'
+           - 'result.msg == "Must provide description when state is present."'
+
+    # ============================================================
+    - name: test failure with only description
+      ec2_group:
+        description: '{{ec2_group_description}}'
+      register: result
+      ignore_errors: true
+
+    - name: assert failure with only description
+      assert:
+        that:
+           - 'result.failed'
+           - 'result.msg == "one of the following is required: name, group_id"'
+
+    # ============================================================
+    - name: test failure with empty description (AWS API requires non-empty string desc)
+      ec2_group:
+        name: '{{ec2_group_name}}'
+        description: ''
+        region: '{{ec2_region}}'
+      register: result
+      ignore_errors: true
+
+    - name: assert failure with empty description
+      assert:
+        that:
+           - 'result.failed'
+           - 'result.msg == "Must provide description when state is present."'
+
+    # ============================================================
+    - name: test valid region parameter
+      ec2_group:
+        name: '{{ec2_group_name}}'
+        description: '{{ec2_group_description}}'
+        region: '{{ec2_region}}'
+      register: result
+      ignore_errors: true
+
+    - name: assert valid region parameter
+      assert:
+        that:
+           - 'result.failed'
+           - '"Unable to locate credentials" in result.msg'
+
+    # ============================================================
+    - name: test environment variable EC2_REGION
+      ec2_group:
+        name: '{{ec2_group_name}}'
+        description: '{{ec2_group_description}}'
+      environment:
+        EC2_REGION: '{{ec2_region}}'
+      register: result
+      ignore_errors: true
+
+    - name: assert environment variable EC2_REGION
+      assert:
+        that:
+           - 'result.failed'
+           - '"Unable to locate credentials" in result.msg'
+
+    # ============================================================
+    - name: test invalid ec2_url parameter
+      ec2_group:
+        name: '{{ec2_group_name}}'
+        description: '{{ec2_group_description}}'
+      environment:
+        EC2_URL: bogus.example.com
+      register: result
+      ignore_errors: true
+
+    - name: assert invalid ec2_url parameter
+      assert:
+        that:
+           - 'result.failed'
+           - 'result.msg.startswith("The ec2_group module requires a region")'
+
+    # ============================================================
+    - name: test valid ec2_url parameter
+      ec2_group:
+        name: '{{ec2_group_name}}'
+        description: '{{ec2_group_description}}'
+      environment:
+        EC2_URL: '{{ec2_url}}'
+      register: result
+      ignore_errors: true
+
+    - name: assert valid ec2_url parameter
+      assert:
+        that:
+           - 'result.failed'
+           - 'result.msg.startswith("The ec2_group module requires a region")'
+
+    # ============================================================
+    - name: test credentials from environment
+      ec2_group:
+        name: '{{ec2_group_name}}'
+        description: '{{ec2_group_description}}'
+      environment:
+        EC2_REGION: '{{ec2_region}}'
+        EC2_ACCESS_KEY: bogus_access_key
+        EC2_SECRET_KEY: bogus_secret_key
+      register: result
+      ignore_errors: true
+
+    - name: assert ec2_group with valid ec2_url
+      assert:
+        that:
+           - 'result.failed'
+           - '"validate the provided access credentials" in result.msg'
+
+    # ============================================================
+    - name: test credential parameters
+      ec2_group:
+        name: '{{ec2_group_name}}'
+        description: '{{ec2_group_description}}'
+        ec2_region: '{{ec2_region}}'
+        ec2_access_key: 'bogus_access_key'
+        ec2_secret_key: 'bogus_secret_key'
+      register: result
+      ignore_errors: true
+
+    - name: assert credential parameters
+      assert:
+        that:
+           - 'result.failed'
+           - '"validate the provided access credentials" in result.msg'

test/integration/targets/ec2_group/tasks/data_validation.yml

@@ -0,0 +1,44 @@
+---
+- block:
+    - name: set up aws connection info
+      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: yes
+    - name: Create a group with only the default rule
+      ec2_group:
+        name: '{{ec2_group_name}}-input-tests'
+        vpc_id: '{{ vpc_result.vpc.id }}'
+        description: '{{ec2_group_description}}'
+        <<: *aws_connection_info
+
+    - name: Run through some common weird port specs
+      ec2_group:
+        name: '{{ec2_group_name}}-input-tests'
+        description: '{{ec2_group_description}}'
+        <<: *aws_connection_info
+        rules:
+          - "{{ item }}"
+      with_items:
+        - proto: tcp
+          from_port: "8182"
+          to_port: 8182
+          cidr_ipv6: "64:ff9b::/96"
+          rule_desc: Mixed string and non-string ports
+        - proto: tcp
+          ports:
+          - "9000"
+          - 9001
+          - 9002-9005
+          cidr_ip: "1.2.3.0/24"
+  always:
+    - name: tidy up input testing group
+      ec2_group:
+        name: '{{ec2_group_name}}-input-tests'
+        vpc_id: '{{ vpc_result.vpc.id }}'
+        state: absent
+        <<: *aws_connection_info
+      ignore_errors: yes

test/integration/targets/ec2_group/tasks/egress_tests.yml

@@ -0,0 +1,175 @@
+---
+- block:
+    - name: set up aws connection info
+      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: yes
+
+
+    - name: Create a group with only the default rule
+      ec2_group:
+        name: '{{ec2_group_name}}-egress-tests'
+        vpc_id: '{{ vpc_result.vpc.id }}'
+        description: '{{ec2_group_description}}'
+        <<: *aws_connection_info
+        state: present
+      register: result
+
+    - name: assert default rule is in place (expected changed=true)
+      assert:
+        that:
+          - result is changed
+          - result.ip_permissions|length == 0
+          - result.ip_permissions_egress|length == 1
+          - result.ip_permissions_egress[0].ip_ranges[0].cidr_ip == '0.0.0.0/0'
+
+    - name: Create a group with only the default rule
+      ec2_group:
+        name: '{{ec2_group_name}}-egress-tests'
+        vpc_id: '{{ vpc_result.vpc.id }}'
+        description: '{{ec2_group_description}}'
+        purge_rules_egress: false
+        <<: *aws_connection_info
+        state: present
+      register: result
+
+    - name: assert default rule is not purged (expected changed=false)
+      assert:
+        that:
+          - result is not changed
+          - result.ip_permissions|length == 0
+          - result.ip_permissions_egress|length == 1
+          - result.ip_permissions_egress[0].ip_ranges[0].cidr_ip == '0.0.0.0/0'
+
+    - name: Pass empty egress rules without purging, should leave default rule in place
+      ec2_group:
+        name: '{{ec2_group_name}}-egress-tests'
+        description: '{{ec2_group_description}}'
+        vpc_id: '{{ vpc_result.vpc.id }}'
+        purge_rules_egress: false
+        rules_egress: []
+        <<: *aws_connection_info
+        state: present
+      register: result
+
+    - name: assert default rule is not purged (expected changed=false)
+      assert:
+        that:
+          - result is not changed
+          - result.ip_permissions|length == 0
+          - result.ip_permissions_egress|length == 1
+          - result.ip_permissions_egress[0].ip_ranges[0].cidr_ip == '0.0.0.0/0'
+
+    - name: Purge rules, including the default
+      ec2_group:
+        name: '{{ec2_group_name}}-egress-tests'
+        description: '{{ec2_group_description}}'
+        vpc_id: '{{ vpc_result.vpc.id }}'
+        purge_rules_egress: true
+        rules_egress: []
+        <<: *aws_connection_info
+        state: present
+      register: result
+
+    - name: assert default rule is not purged (expected changed=false)
+      assert:
+        that:
+          - result is changed
+          - result.ip_permissions|length == 0
+          - result.ip_permissions_egress|length == 0
+
+    - name: Add a custom egress rule
+      ec2_group:
+        name: '{{ec2_group_name}}-egress-tests'
+        description: '{{ec2_group_description}}'
+        vpc_id: '{{ vpc_result.vpc.id }}'
+        rules_egress:
+        - proto: tcp
+          ports:
+          - 1212
+          cidr_ip: 1.2.1.2/32
+        <<: *aws_connection_info
+        state: present
+      register: result
+
+    - name: assert first rule is here
+      assert:
+        that:
+          - result.ip_permissions_egress|length == 1
+
+    - name: Add a second custom egress rule
+      ec2_group:
+        name: '{{ec2_group_name}}-egress-tests'
+        description: '{{ec2_group_description}}'
+        purge_rules_egress: false
+        vpc_id: '{{ vpc_result.vpc.id }}'
+        rules_egress:
+        - proto: tcp
+          ports:
+          - 2323
+          cidr_ip: 2.3.2.3/32
+        <<: *aws_connection_info
+        state: present
+      register: result
+
+    - name: assert the first rule is not purged
+      assert:
+        that:
+          - result.ip_permissions_egress|length == 2
+
+    - name: Purge the second rule
+      ec2_group:
+        name: '{{ec2_group_name}}-egress-tests'
+        description: '{{ec2_group_description}}'
+        vpc_id: '{{ vpc_result.vpc.id }}'
+        rules_egress:
+        - proto: tcp
+          ports:
+          - 1212
+          cidr_ip: 1.2.1.2/32
+        <<: *aws_connection_info
+        state: present
+      register: result
+
+    - name: assert first rule is here
+      assert:
+        that:
+          - result.ip_permissions_egress|length == 1
+          - result.ip_permissions_egress[0].ip_ranges[0].cidr_ip == '1.2.1.2/32'
+
+    - name: add a rule for all TCP ports
+      ec2_group:
+        name: '{{ec2_group_name}}-egress-tests'
+        description: '{{ec2_group_description}}'
+        rules_egress:
+        - proto: tcp
+          ports: 0-65535
+          cidr_ip: 0.0.0.0/0
+        <<: *aws_connection_info
+        state: present
+        vpc_id: '{{ vpc_result.vpc.id }}'
+      register: result
+
+    - name: Re-add the default rule
+      ec2_group:
+        name: '{{ec2_group_name}}-egress-tests'
+        description: '{{ec2_group_description}}'
+        rules_egress:
+        - proto: -1
+          cidr_ip: 0.0.0.0/0
+        <<: *aws_connection_info
+        state: present
+        vpc_id: '{{ vpc_result.vpc.id }}'
+      register: result
+  always:
+    - name: tidy up egress rule test security group
+      ec2_group:
+        name: '{{ec2_group_name}}-egress-tests'
+        state: absent
+        vpc_id: '{{ vpc_result.vpc.id }}'
+        <<: *aws_connection_info
+      ignore_errors: yes

test/integration/targets/ec2_group/tasks/ipv6_default_tests.yml

@@ -0,0 +1,103 @@
+---
+- name: set up aws connection info
+  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: yes
+# ============================================================
+- name: test state=present for ipv6 (expected changed=true) (CHECK MODE)
+  ec2_group:
+    name: '{{ec2_group_name}}'
+    description: '{{ec2_group_description}}'
+    <<: *aws_connection_info
+    state: present
+    rules:
+    - proto: "tcp"
+      from_port: 8182
+      to_port: 8182
+      cidr_ipv6: "64:ff9b::/96"
+  check_mode: true
+  register: result
+
+- name: assert state=present (expected changed=true)
+  assert:
+    that:
+       - 'result.changed'
+
+# ============================================================
+- name: test state=present for ipv6 (expected changed=true)
+  ec2_group:
+    name: '{{ec2_group_name}}'
+    description: '{{ec2_group_description}}'
+    <<: *aws_connection_info
+    state: present
+    rules:
+    - proto: "tcp"
+      from_port: 8182
+      to_port: 8182
+      cidr_ipv6: "64:ff9b::/96"
+  register: result
+
+- name: assert state=present (expected changed=true)
+  assert:
+    that:
+       - 'result.changed'
+       - 'result.group_id.startswith("sg-")'
+
+# ============================================================
+- name: test rules_egress state=present for ipv6 (expected changed=true) (CHECK MODE)
+  ec2_group:
+    name: '{{ec2_group_name}}'
+    description: '{{ec2_group_description}}'
+    <<: *aws_connection_info
+    state: present
+    rules:
+    - proto: "tcp"
+      from_port: 8182
+      to_port: 8182
+      cidr_ipv6: "64:ff9b::/96"
+    rules_egress:
+    - proto: "tcp"
+      from_port: 8181
+      to_port: 8181
+      cidr_ipv6: "64:ff9b::/96"
+  check_mode: true
+  register: result
+
+- name: assert state=present (expected changed=true)
+  assert:
+    that:
+       - 'result.changed'
+
+# ============================================================
+- name: test rules_egress state=present for ipv6 (expected changed=true)
+  ec2_group:
+    name: '{{ec2_group_name}}'
+    description: '{{ec2_group_description}}'
+    <<: *aws_connection_info
+    state: present
+    rules:
+    - proto: "tcp"
+      from_port: 8182
+      to_port: 8182
+      cidr_ipv6: "64:ff9b::/96"
+    rules_egress:
+    - proto: "tcp"
+      from_port: 8181
+      to_port: 8181
+      cidr_ipv6: "64:ff9b::/96"
+  register: result
+
+- name: assert state=present (expected changed=true)
+  assert:
+    that:
+       - 'result.changed'
+       - 'result.group_id.startswith("sg-")'
+- name: delete it
+  ec2_group:
+    name: '{{ec2_group_name}}'
+    <<: *aws_connection_info
+    state: absent

test/integration/targets/ec2_group/tasks/rule_group_create.yml

@@ -0,0 +1,132 @@
+---
+- block:
+    - name: set up aws connection info
+      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: yes
+
+    - name: Create a group with self-referring rule
+      ec2_group:
+        name: '{{ec2_group_name}}-auto-create-1'
+        vpc_id: '{{ vpc_result.vpc.id }}'
+        description: '{{ec2_group_description}}'
+        rules:
+        - proto: "tcp"
+          from_port: 8000
+          to_port: 8100
+          group_name: '{{ec2_group_name}}-auto-create-1'
+        <<: *aws_connection_info
+        state: present
+      register: result
+
+    - name: Create a second group rule
+      ec2_group:
+        name: '{{ec2_group_name}}-auto-create-2'
+        vpc_id: '{{ vpc_result.vpc.id }}'
+        description: '{{ec2_group_description}}'
+        <<: *aws_connection_info
+        state: present
+
+    - name: Create a series of rules with a recently created group as target
+      ec2_group:
+        name: '{{ec2_group_name}}-auto-create-1'
+        vpc_id: '{{ vpc_result.vpc.id }}'
+        description: '{{ec2_group_description}}'
+        purge_rules: false
+        rules:
+        - proto: "tcp"
+          from_port: "{{ item }}"
+          to_port: "{{ item }}"
+          group_name: '{{ec2_group_name}}-auto-create-2'
+        <<: *aws_connection_info
+        state: present
+      register: result
+      with_items:
+        - 20
+        - 40
+        - 60
+        - 80
+
+    - name: Create a group with only the default rule
+      ec2_group:
+        name: '{{ec2_group_name}}-auto-create-1'
+        vpc_id: '{{ vpc_result.vpc.id }}'
+        description: '{{ec2_group_description}}'
+        rules:
+        - proto: "tcp"
+          from_port: 8182
+          to_port: 8182
+          group_name: '{{ec2_group_name}}-auto-create-3'
+        <<: *aws_connection_info
+        state: present
+      register: result
+      ignore_errors: true
+
+    - name: assert you can't create a new group from a rule target with no description
+      assert:
+        that:
+          - result is failed
+
+    - name: Create a group with a target of a separate group
+      ec2_group:
+        name: '{{ec2_group_name}}-auto-create-1'
+        vpc_id: '{{ vpc_result.vpc.id }}'
+        description: '{{ec2_group_description}}'
+        rules:
+        - proto: tcp
+          ports:
+            - 22
+            - 80
+          group_name: '{{ec2_group_name}}-auto-create-3'
+          group_desc: '{{ec2_group_description}}'
+        <<: *aws_connection_info
+        state: present
+      register: result
+
+    - name: Create a 4th group
+      ec2_group:
+        name: '{{ec2_group_name}}-auto-create-4'
+        vpc_id: '{{ vpc_result.vpc.id }}'
+        description: '{{ec2_group_description}}'
+        <<: *aws_connection_info
+        state: present
+        rules:
+        - proto: tcp
+          ports:
+            - 22
+          cidr_ip: 0.0.0.0/0
+
+    - name: use recently created group in a rule
+      ec2_group:
+        name: '{{ec2_group_name}}-auto-create-5'
+        vpc_id: '{{ vpc_result.vpc.id }}'
+        description: '{{ec2_group_description}}'
+        rules:
+        - proto: tcp
+          ports:
+            - 443
+          group_name: '{{ec2_group_name}}-auto-create-4'
+        <<: *aws_connection_info
+        state: present
+
+  always:
+    - name: tidy up egress rule test security group
+      ec2_group:
+        name: '{{ec2_group_name}}-auto-create-{{ item }}'
+        state: absent
+        vpc_id: '{{ vpc_result.vpc.id }}'
+        <<: *aws_connection_info
+      ignore_errors: yes
+      with_items: [5, 4, 3, 2, 1]
+    - name: tidy up egress rule test security group
+      ec2_group:
+        name: '{{ec2_group_name}}-auto-create-{{ item }}'
+        state: absent
+        vpc_id: '{{ vpc_result.vpc.id }}'
+        <<: *aws_connection_info
+      ignore_errors: yes
+      with_items: [1, 2, 3, 4, 5]

test/sanity/pylint/ignore.txt

@@ -2,7 +2,6 @@ lib/ansible/module_utils/network/iosxr/iosxr.py ansible-format-automatic-specifi
 lib/ansible/modules/cloud/amazon/aws_api_gateway.py ansible-format-automatic-specification
 lib/ansible/modules/cloud/amazon/aws_kms.py ansible-format-automatic-specification
 lib/ansible/modules/cloud/amazon/ec2_eip.py ansible-format-automatic-specification
-lib/ansible/modules/cloud/amazon/ec2_group.py ansible-format-automatic-specification
 lib/ansible/modules/cloud/amazon/ecs_ecr.py ansible-format-automatic-specification
 lib/ansible/modules/cloud/amazon/sns.py ansible-format-automatic-specification
 lib/ansible/modules/cloud/azure/azure_rm_acs.py ansible-format-automatic-specification

test/units/modules/cloud/amazon/test_ec2_group.py

@@ -0,0 +1,80 @@
+import pytest
+
+from ansible.modules.cloud.amazon import ec2_group as group_module
+
+
+def test_from_permission():
+    internal_http = {
+        u'FromPort': 80,
+        u'IpProtocol': 'tcp',
+        u'IpRanges': [
+            {
+                u'CidrIp': '10.0.0.0/8',
+                u'Description': 'Foo Bar Baz'
+            },
+        ],
+        u'Ipv6Ranges': [
+            {u'CidrIpv6': 'fe80::94cc:8aff:fef6:9cc/64'},
+        ],
+        u'PrefixListIds': [],
+        u'ToPort': 80,
+        u'UserIdGroupPairs': [],
+    }
+    perms = list(group_module.rule_from_group_permission(internal_http))
+    assert len(perms) == 2
+    assert perms[0].target == '10.0.0.0/8'
+    assert perms[0].target_type == 'ipv4'
+    assert perms[0].description == 'Foo Bar Baz'
+    assert perms[1].target == 'fe80::94cc:8aff:fef6:9cc/64'
+
+    global_egress = {
+        'IpProtocol': '-1',
+        'IpRanges': [{'CidrIp': '0.0.0.0/0'}],
+        'Ipv6Ranges': [],
+        'PrefixListIds': [],
+        'UserIdGroupPairs': []
+    }
+    perms = list(group_module.rule_from_group_permission(global_egress))
+    assert len(perms) == 1
+    assert perms[0].target == '0.0.0.0/0'
+    assert perms[0].port_range == (None, None)
+
+    internal_prefix_http = {
+        u'FromPort': 80,
+        u'IpProtocol': 'tcp',
+        u'PrefixListIds': [
+            {'PrefixListId': 'p-1234'}
+        ],
+        u'ToPort': 80,
+        u'UserIdGroupPairs': [],
+    }
+    perms = list(group_module.rule_from_group_permission(internal_prefix_http))
+    assert len(perms) == 1
+    assert perms[0].target == 'p-1234'
+
+
+def test_rule_to_permission():
+    tests = [
+        group_module.Rule((22, 22), 'udp', 'sg-1234567890', 'group', None),
+        group_module.Rule((1, 65535), 'tcp', '0.0.0.0/0', 'ipv4', "All TCP from everywhere"),
+        group_module.Rule((443, 443), 'tcp', 'ip-123456', 'ip_prefix', "Traffic to privatelink IPs"),
+        group_module.Rule((443, 443), 'tcp', 'feed:dead:::beef/64', 'ipv6', None),
+    ]
+    for test in tests:
+        perm = group_module.to_permission(test)
+        assert perm['FromPort'], perm['ToPort'] == test.port_range
+        assert perm['IpProtocol'] == test.protocol
+
+
+def test_validate_ip():
+    class Warner(object):
+        def warn(self, msg):
+            return
+    ips = [
+        ('1.1.1.1/24', '1.1.1.0/24'),
+        ('192.168.56.101/16', '192.168.0.0/16'),
+        ('1203:8fe0:fe80:b897:8990:8a7c:99bf:323d/64', '1203:8fe0:fe80::/64'),
+    ]
+
+    for ip, net in ips:
+        assert group_module.validate_ip(Warner(), ip) == net