Fix ec2_group for numbered protocols (GRE) (ansible#42765)

* Fix spurious `changed=True` when int is passed as tag

* Fix for all AWS module using compare_aws_tags

* Handle improperly stringified protocols and allow inconsistency between None/-1 on non-tcp protocols

* Add integration test that reproduces the same bug

* Return false if the comparsison is not equal

lib/ansible/module_utils/ec2.py

@@ -714,7 +714,7 @@ def compare_aws_tags(current_tags_dict, new_tags_dict, purge_tags=True):
             tag_keys_to_unset.append(key)
 
     for key in set(new_tags_dict.keys()) - set(tag_keys_to_unset):
-        if new_tags_dict[key] != current_tags_dict.get(key):
+        if to_text(new_tags_dict[key]) != current_tags_dict.get(key):
             tag_key_value_pairs_to_set[key] = new_tags_dict[key]
 
     return tag_key_value_pairs_to_set, tag_keys_to_unset

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

@@ -292,6 +292,7 @@
 
 import json
 import re
+import itertools
 from time import sleep
 from collections import namedtuple
 from ansible.module_utils.aws.core import AnsibleAWSModule, is_boto3_error_code
@@ -317,7 +318,13 @@
 def rule_cmp(a, b):
     """Compare rules without descriptions"""
     for prop in ['port_range', 'protocol', 'target', 'target_type']:
-        if getattr(a, prop) != getattr(b, prop):
+        if prop == 'port_range' and to_text(a.protocol) == to_text(b.protocol):
+            # equal protocols can interchange `(-1, -1)` and `(None, None)`
+            if a.port_range in ((None, None), (-1, -1)) and b.port_range in ((None, None), (-1, -1)):
+                continue
+            elif getattr(a, prop) != getattr(b, prop):
+                return False
+        elif getattr(a, prop) != getattr(b, prop):
             return False
     return True
 
@@ -390,7 +397,7 @@ def ports_from_permission(p):
             # there may be several IP ranges here, which is ok
             yield Rule(
                 ports_from_permission(perm),
-                perm['IpProtocol'],
+                to_text(perm['IpProtocol']),
                 r[target_subkey],
                 target_type,
                 r.get('Description')
@@ -416,7 +423,7 @@ def ports_from_permission(p):
 
             yield Rule(
                 ports_from_permission(perm),
-                perm['IpProtocol'],
+                to_text(perm['IpProtocol']),
                 target,
                 'group',
                 pair.get('Description')
@@ -803,6 +810,15 @@ def await_rules(group, desired_rules, purge, rule_key):
             current_rules = set(sum([list(rule_from_group_permission(p)) for p in group[rule_key]], []))
             if purge and len(current_rules ^ set(desired_rules)) == 0:
                 return group
+            elif purge:
+                conflicts = current_rules ^ set(desired_rules)
+                # For cases where set comparison is equivalent, but invalid port/proto exist
+                for a, b in itertools.combinations(conflicts, 2):
+                    if rule_cmp(a, b):
+                        conflicts.discard(a)
+                        conflicts.discard(b)
+                if not len(conflicts):
+                    return group
             elif current_rules.issuperset(desired_rules) and not purge:
                 return group
             sleep(10)
@@ -1039,11 +1055,19 @@ def main():
                     rule['proto'] = '-1'
                     rule['from_port'] = None
                     rule['to_port'] = None
+                try:
+                    int(rule.get('proto', 'tcp'))
+                    rule['proto'] = to_text(rule.get('proto', 'tcp'))
+                    rule['from_port'] = None
+                    rule['to_port'] = None
+                except ValueError:
+                    # rule does not use numeric protocol spec
+                    pass
 
                 named_tuple_rule_list.append(
                     Rule(
                         port_range=(rule['from_port'], rule['to_port']),
-                        protocol=rule.get('proto', 'tcp'),
+                        protocol=to_text(rule.get('proto', 'tcp')),
                         target=target, target_type=target_type,
                         description=rule.get('rule_desc'),
                     )

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

@@ -42,6 +42,7 @@
           Name: "{{ resource_prefix }}-vpc"
           Description: "Created by ansible-test"
       register: vpc_result
+    - include: ./numeric_protos.yml
     - include: ./rule_group_create.yml
     - include: ./egress_tests.yml
     - include: ./data_validation.yml

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

@@ -0,0 +1,71 @@
+---
+- block:
+    - name: set up aws connection info
+      set_fact:
+        group_tmp_name: '{{ec2_group_name}}-numbered-protos'
+        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 numbered protocol (GRE)
+      ec2_group:
+        name: '{{ group_tmp_name }}'
+        vpc_id: '{{ vpc_result.vpc.id }}'
+        description: '{{ ec2_group_description }}'
+        rules:
+        - proto: 47
+          to_port: -1
+          from_port: -1
+          cidr_ip: 0.0.0.0/0
+        <<: *aws_connection_info
+        state: present
+      register: result
+
+    - name: Create a group with a quoted proto
+      ec2_group:
+        name: '{{ group_tmp_name }}'
+        vpc_id: '{{ vpc_result.vpc.id }}'
+        description: '{{ ec2_group_description }}'
+        rules:
+        - proto: '47'
+          to_port: -1
+          from_port: -1
+          cidr_ip: 0.0.0.0/0
+        <<: *aws_connection_info
+        state: present
+      register: result
+    - assert:
+        that:
+          - result is not changed
+    - name: Add a tag with a numeric value
+      ec2_group:
+        name: '{{ group_tmp_name }}'
+        vpc_id: '{{ vpc_result.vpc.id }}'
+        description: '{{ ec2_group_description }}'
+        tags:
+          foo: 1
+        <<: *aws_connection_info
+    - name: Read a tag with a numeric value
+      ec2_group:
+        name: '{{ group_tmp_name }}'
+        vpc_id: '{{ vpc_result.vpc.id }}'
+        description: '{{ ec2_group_description }}'
+        tags:
+          foo: 1
+        <<: *aws_connection_info
+      register: result
+    - assert:
+        that:
+          - result is not changed
+
+  always:
+    - name: tidy up egress rule test security group
+      ec2_group:
+        name: '{{group_tmp_name}}'
+        state: absent
+        vpc_id: '{{ vpc_result.vpc.id }}'
+        <<: *aws_connection_info
+      ignore_errors: yes