Fix TypeError in ec2_group.py for Python3 when sorting dictionary list (#59844)

* Fix TypeError in ec2_group.py for Python3 when sorting dictionary list

* Using json.loads() and dumps() to replace sorting

* Bug fixes for ec2_group.py

* Dictionaries cannot be compared/sorted in Python3

* Diff will occur when the IpPermissions have the same IpRanges but have different ordering

* 'before' will be sorted by 'Type' with high priority than 'IP', but 'boto3.describe_security_groups()' function cannot get 'Type' from Amazon

* Add some basic diff mode testing to exercise the rule-sorting code

changelogs/fragments/59844-ec2_group-fix-diff-mode-python3.yml

@@ -0,0 +1,2 @@
+bugfixes:
+  - ec2_group - Fix traceback sorting dictionaries using Python 3 and ensure rules shown by diff mode are in a consistent order.

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

@@ -956,7 +956,7 @@ def get_diff_final_resource(client, module, security_group):
                         format_rule['user_id_group_pairs'][0].pop(k)
             final_rules.append(format_rule)
             # Order final rules consistently
-            final_rules.sort(key=lambda x: x.get('cidr_ip', x.get('ip_ranges', x.get('ipv6_ranges', x.get('prefix_list_ids', x.get('user_id_group_pairs'))))))
+            final_rules.sort(key=get_ip_permissions_sort_key)
         return final_rules
     security_group_ingress = security_group.get('ip_permissions', [])
     specified_ingress = module.params['rules']
@@ -996,6 +996,34 @@ def flatten_nested_targets(module, rules):
     return rules
 
 
+def get_rule_sort_key(dicts):
+    if dicts.get('cidr_ip'):
+        return dicts.get('cidr_ip')
+    elif dicts.get('cidr_ipv6'):
+        return dicts.get('cidr_ipv6')
+    elif dicts.get('prefix_list_id'):
+        return dicts.get('prefix_list_id')
+    elif dicts.get('group_id'):
+        return dicts.get('group_id')
+    return None
+
+
+def get_ip_permissions_sort_key(rule):
+    if rule.get('ip_ranges'):
+        rule.get('ip_ranges').sort(key=get_rule_sort_key)
+        return rule.get('ip_ranges')[0]['cidr_ip']
+    elif rule.get('ipv6_ranges'):
+        rule.get('ipv6_ranges').sort(key=get_rule_sort_key)
+        return rule.get('ipv6_ranges')[0]['cidr_ipv6']
+    elif rule.get('prefix_list_ids'):
+        rule.get('prefix_list_ids').sort(key=get_rule_sort_key)
+        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 None
+
+
 def main():
     argument_spec = dict(
         name=dict(),
@@ -1195,6 +1223,8 @@ def main():
     if module._diff:
         if module.params['state'] == 'present':
             after = get_diff_final_resource(client, module, security_group)
+            if before.get('ip_permissions'):
+                before['ip_permissions'].sort(key=get_ip_permissions_sort_key)
 
         security_group['diff'] = [{'before': before, 'after': after}]
 

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

@@ -0,0 +1,184 @@
+---
+  - 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 a rule (CHECK MODE + DIFF)
+    ec2_group:
+      name: '{{ ec2_group_name }}'
+      description: '{{ ec2_group_description }}'
+      state: present
+      rules:
+      - proto: tcp
+        from_port: 80
+        to_port: 80
+        cidr_ip: 0.0.0.0/0
+      rules_egress:
+      - proto: all
+        cidr_ip: 0.0.0.0/0
+      <<: *aws_connection_info
+    register: check_mode_result
+    check_mode: true
+    diff: true
+
+  - assert:
+      that:
+        - check_mode_result.changed
+
+  - name: create a group with a rule (DIFF)
+    ec2_group:
+      name: '{{ ec2_group_name }}'
+      description: '{{ ec2_group_description }}'
+      state: present
+      rules:
+      - proto: tcp
+        from_port: 80
+        to_port: 80
+        cidr_ip: 0.0.0.0/0
+      rules_egress:
+      - proto: all
+        cidr_ip: 0.0.0.0/0
+      <<: *aws_connection_info
+    register: result
+    diff: true
+
+  - assert:
+      that:
+        - result.changed
+        - result.diff.0.after.ip_permissions == check_mode_result.diff.0.after.ip_permissions
+        - result.diff.0.after.ip_permissions_egress == check_mode_result.diff.0.after.ip_permissions_egress
+
+  - name: add rules to make sorting occur (CHECK MODE + DIFF)
+    ec2_group:
+      name: '{{ ec2_group_name }}'
+      description: '{{ ec2_group_description }}'
+      state: present
+      rules:
+      - proto: tcp
+        from_port: 80
+        to_port: 80
+        cidr_ip: 0.0.0.0/0
+      - proto: tcp
+        from_port: 22
+        to_port: 22
+        cidr_ip: 20.0.0.0/8
+      - proto: tcp
+        from_port: 22
+        to_port: 22
+        cidr_ip: 10.0.0.0/8
+      rules_egress:
+      - proto: all
+        cidr_ip: 0.0.0.0/0
+      <<: *aws_connection_info
+    register: check_mode_result
+    check_mode: true
+    diff: true
+
+  - assert:
+      that:
+        - check_mode_result.changed
+
+  - name: add rules in a different order to test sorting consistency (DIFF)
+    ec2_group:
+      name: '{{ ec2_group_name }}'
+      description: '{{ ec2_group_description }}'
+      state: present
+      rules:
+      - proto: tcp
+        from_port: 22
+        to_port: 22
+        cidr_ip: 20.0.0.0/8
+      - proto: tcp
+        from_port: 80
+        to_port: 80
+        cidr_ip: 0.0.0.0/0
+      - proto: tcp
+        from_port: 22
+        to_port: 22
+        cidr_ip: 10.0.0.0/8
+      rules_egress:
+      - proto: all
+        cidr_ip: 0.0.0.0/0
+      <<: *aws_connection_info
+    register: result
+    diff: true
+
+  - assert:
+      that:
+        - result.changed
+        - result.diff.0.after.ip_permissions == check_mode_result.diff.0.after.ip_permissions
+        - result.diff.0.after.ip_permissions_egress == check_mode_result.diff.0.after.ip_permissions_egress
+
+  - name: purge rules (CHECK MODE + DIFF)
+    ec2_group:
+      name: '{{ ec2_group_name }}'
+      description: '{{ ec2_group_description }}'
+      state: present
+      rules:
+      - proto: tcp
+        from_port: 80
+        to_port: 80
+        cidr_ip: 0.0.0.0/0
+      rules_egress: []
+      <<: *aws_connection_info
+    register: check_mode_result
+    check_mode: true
+    diff: true
+
+  - assert:
+      that:
+        - check_mode_result.changed
+
+  - name: purge rules (DIFF)
+    ec2_group:
+      name: '{{ ec2_group_name }}'
+      description: '{{ ec2_group_description }}'
+      state: present
+      rules:
+      - proto: tcp
+        from_port: 80
+        to_port: 80
+        cidr_ip: 0.0.0.0/0
+      rules_egress: []
+      <<: *aws_connection_info
+    register: result
+    diff: true
+
+  - assert:
+      that:
+        - result.changed
+        - result.diff.0.after.ip_permissions == check_mode_result.diff.0.after.ip_permissions
+        - result.diff.0.after.ip_permissions_egress == check_mode_result.diff.0.after.ip_permissions_egress
+
+  - name: delete the security group (CHECK MODE + DIFF)
+    ec2_group:
+      name: '{{ ec2_group_name }}'
+      state: absent
+      <<: *aws_connection_info
+    register: check_mode_result
+    diff: true
+    check_mode: true
+
+  - assert:
+      that:
+        - check_mode_result.changed
+
+  - name: delete the security group (DIFF)
+    ec2_group:
+      name: '{{ ec2_group_name }}'
+      state: absent
+      <<: *aws_connection_info
+    register: result
+    diff: true
+
+  - assert:
+      that:
+        - result.changed
+        - not result.diff.0.after and not check_mode_result.diff.0.after

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

@@ -50,6 +50,7 @@
       register: vpc_result
     #TODO(ryansb): Update CI for VPC peering permissions
     #- include: ./multi_account.yml
+    - include: ./diff_mode.yml
     - include: ./numeric_protos.yml
     - include: ./rule_group_create.yml
     - include: ./egress_tests.yml