Allow updating of ec2_group rules with EC2 classic ELB targets (#62374)

* Allow updating of ec2_group rules with EC2 classic ELB targets

Fix regression introduced in #45296 with EC2 Classic SGs

Fixes: #57247

Also add (unsupported) ec2 classic test suite with test case for this scenario

* move ec2 classic tests to conditional within ec2_group target

* clean up ec2_classic tests

* ec2_classic account can't run most ec2_group tests

changelogs/fragments/62374-ec2-clb-group-rules.yml

@@ -0,0 +1,2 @@
+bugfixes:
+  - ec2_group - Fix regression with revoking security groups in EC2 Classic Load Balancers.

lib/ansible/modules/cloud/amazon/ec2_group.py

@@ -409,7 +409,11 @@ def rule_from_group_permission(perm):
             )
     if 'UserIdGroupPairs' in perm and perm['UserIdGroupPairs']:
         for pair in perm['UserIdGroupPairs']:
-            target = pair['GroupId']
+            target = (
+                pair.get('UserId', None),
+                pair.get('GroupId', None),
+                pair.get('GroupName', None),
+            )
             if pair.get('UserId', '').startswith('amazon-'):
                 # amazon-elb and amazon-prefix rules don't need
                 # group-id specified, so remove it when querying

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

@@ -0,0 +1,88 @@
+- module_defaults:
+    group/aws:
+      aws_access_key: "{{ aws_access_key }}"
+      aws_secret_key: "{{ aws_secret_key }}"
+      security_token: "{{ security_token }}"
+      region: "{{ aws_region }}"
+  block:
+  - name: Get available AZs
+    aws_az_facts:
+      aws_access_key: "{{ aws_connection_info['aws_access_key'] }}"
+      aws_secret_key: "{{ aws_connection_info['aws_secret_key'] }}"
+      filters:
+        region-name: "{{ aws_connection_info['region'] }}"
+    register: az_facts
+
+  - name: Create a classic ELB with classic networking
+    ec2_elb_lb: 
+      name: "{{ resource_prefix }}-elb"
+      state: present
+      zones:
+        - "{{ az_facts['availability_zones'][0]['zone_name'] }}"
+        - "{{ az_facts['availability_zones'][1]['zone_name'] }}"
+      listeners:
+        - protocol: http # options are http, https, ssl, tcp
+          load_balancer_port: 80
+          instance_port: 80
+          proxy_protocol: True
+    register: classic_elb
+
+  - name: Assert the elb was created
+    assert:
+      that:
+        - classic_elb.changed
+
+  - name: Create a security group with a classic elb-sg rule
+    ec2_group:
+      name: "{{ resource_prefix }}-sg-a"
+      description: "EC2 classic test security group"
+      rules:
+      - proto: tcp
+        ports: 80
+        group_id: amazon-elb/amazon-elb-sg
+      state: present
+    register: classic_sg
+
+  - name: Assert the SG was created
+    assert:
+      that:
+        - classic_sg.changed
+        - "{{ classic_sg.ip_permissions | length }} == 1"
+
+  - set_fact:
+      elb_sg_id: "{{ classic_sg.ip_permissions[0].user_id_group_pairs[0].user_id }}/{{ classic_sg.ip_permissions[0].user_id_group_pairs[0].group_id }}/{{ classic_sg.ip_permissions[0].user_id_group_pairs[0].group_name }}"
+
+  - name: Update the security group
+    ec2_group:
+      name: "{{ resource_prefix }}-sg-a"
+      description: "EC2 classic test security group"
+      rules:
+      - proto: tcp
+        ports: 8080
+        group_id: "{{ elb_sg_id }}"
+      - proto: tcp
+        ports:
+          - 80
+        cidr_ip: 0.0.0.0/0
+      state: present
+    register: updated_classic_sg
+
+
+  - name: Assert the SG was updated
+    assert:
+      that:
+        - updated_classic_sg.changed
+        - "{{ updated_classic_sg.ip_permissions | length }} == 2"
+        - "{{ classic_sg.ip_permissions[0]}} not in {{ updated_classic_sg.ip_permissions }}"
+
+  # ===========================================
+  always:
+    - name: Terminate classic ELB
+      ec2_elb_lb:
+        name: "{{ resource_prefix }}-classic-elb"
+        state: absent
+
+    - name: Delete security group
+      ec2_group:
+        name: "{{ resource_prefix }}-sg-a"
+        state: absent

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

@@ -9,35 +9,60 @@
 # - include: ../../setup_ec2/tasks/common.yml module_name: ec2_group
 
 - include: ./credential_tests.yml
-- module_defaults:
-    group/aws:
+# ============================================================
+# 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 `aws_account_attribute` plugin.
+
+- name: determine if this is an EC2 Classic account
+  set_fact:
+    has_ec2_classic: "{{ lookup('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=security_token,
+                                wantlist=True) }}"
+# ============================================================
+-
+- 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: 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
+  module_defaults:
+      group/aws:
+        aws_access_key: "{{ aws_access_key }}"
+        aws_secret_key: "{{ aws_secret_key }}"
+        security_token: "{{ security_token }}"
+        region: "{{ aws_region }}"
   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: determine if there is a default VPC
       set_fact:
         defaultvpc: "{{ lookup('aws_account_attribute',
-                               attribute='default-vpc',
-                               region=aws_region,
-                               aws_access_key=aws_access_key,
-                               aws_secret_key=aws_secret_key,
-                               aws_security_token=security_token) }}"
+                              attribute='default-vpc',
+                              region=aws_region,
+                              aws_access_key=aws_access_key,
+                              aws_secret_key=aws_secret_key,
+                              aws_security_token=security_token) }}"
       register: default_vpc
 
-    # ============================================================
     - name: create a VPC
       ec2_vpc_net:
         name: "{{ resource_prefix }}-vpc"
@@ -1470,6 +1495,7 @@
         that:
            - 'result.changed'
            - 'not result.group_id'
+  when: not has_ec2_classic
 
   always:
     # ============================================================