[cloud] s3_bucket: policy comparison - fixes #25428 (#25723)

Creates a new way to compare bucket policies by making their elements hashable & comparing that way after normalizing syntax like single-item lists.

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

@@ -120,6 +120,7 @@ import xml.etree.ElementTree as ET
 
 import ansible.module_utils.six.moves.urllib.parse as urlparse
 from ansible.module_utils.six import string_types
+from ansible.module_utils._text import to_text
 from ansible.module_utils.basic import AnsibleModule
 from ansible.module_utils.ec2 import get_aws_connection_info, ec2_argument_spec
 from ansible.module_utils.ec2 import sort_json_policy_dict
@@ -155,6 +156,57 @@ def create_tags_container(tags):
     return tags_obj
 
 
+def hashable_policy(policy, policy_list):
+    """
+        Takes a policy and returns a list, the contents of which are all hashable and sorted.
+        Example input policy:
+        {'Version': '2012-10-17',
+         'Statement': [{'Action': 's3:PutObjectAcl',
+                        'Sid': 'AddCannedAcl2',
+                        'Resource': 'arn:aws:s3:::test_policy/*',
+                        'Effect': 'Allow',
+                        'Principal': {'AWS': ['arn:aws:iam::XXXXXXXXXXXX:user/username1', 'arn:aws:iam::XXXXXXXXXXXX:user/username2']}
+                       }]}
+        Returned value:
+        [('Statement',  ((('Action', (u's3:PutObjectAcl',)),
+                          ('Effect', (u'Allow',)),
+                          ('Principal', ('AWS', ((u'arn:aws:iam::XXXXXXXXXXXX:user/username1',), (u'arn:aws:iam::XXXXXXXXXXXX:user/username2',)))),
+                          ('Resource', (u'arn:aws:s3:::test_policy/*',)), ('Sid', (u'AddCannedAcl2',)))),
+         ('Version', (u'2012-10-17',)))]
+
+    """
+    if isinstance(policy, list):
+        for each in policy:
+            tupleified = hashable_policy(each, [])
+            if isinstance(tupleified, list):
+                tupleified = tuple(tupleified)
+            policy_list.append(tupleified)
+    elif isinstance(policy, string_types):
+        return [(to_text(policy))]
+    elif isinstance(policy, dict):
+        sorted_keys = list(policy.keys())
+        sorted_keys.sort()
+        for key in sorted_keys:
+            tupleified = hashable_policy(policy[key], [])
+            if isinstance(tupleified, list):
+                tupleified = tuple(tupleified)
+            policy_list.append((key, tupleified))
+
+    # ensure we aren't returning deeply nested structures of length 1
+    if len(policy_list) == 1 and isinstance(policy_list[0], tuple):
+        policy_list = policy_list[0]
+    if isinstance(policy_list, list):
+        policy_list.sort()
+    return policy_list
+
+
+def compare_policies(current_policy, new_policy):
+    """ Compares the existing policy and the updated policy
+        Returns True if there is a difference between policies.
+    """
+    return set(hashable_policy(new_policy, [])) != set(hashable_policy(current_policy, []))
+
+
 def _create_or_update_bucket(connection, module, location):
 
     policy = module.params.get("policy")
@@ -195,9 +247,9 @@ def _create_or_update_bucket(connection, module, location):
     requester_pays_status = get_request_payment_status(bucket)
     if requester_pays_status != requester_pays:
         if requester_pays:
-            payer='Requester'
+            payer = 'Requester'
         else:
-            payer='BucketOwner'
+            payer = 'BucketOwner'
         bucket.set_request_payment(payer=payer)
         changed = True
         requester_pays_status = get_request_payment_status(bucket)
@@ -220,9 +272,11 @@ def _create_or_update_bucket(connection, module, location):
             changed = bool(current_policy)
 
         elif sort_json_policy_dict(current_policy) != sort_json_policy_dict(policy):
+            # doesn't necessarily mean the policy has changed; syntax could differ
+            changed = compare_policies(sort_json_policy_dict(current_policy), sort_json_policy_dict(policy))
             try:
-                bucket.set_policy(json.dumps(policy))
+                if changed:
-                changed = True
+                    bucket.set_policy(json.dumps(policy))
                 current_policy = json.loads(bucket.get_policy())
             except S3ResponseError as e:
                 module.fail_json(msg=e.message)
@@ -291,7 +345,7 @@ def _destroy_bucket(connection, module):
 
 
 def _create_or_update_bucket_ceph(connection, module, location):
-    #TODO: add update
+    # TODO: add update
 
     name = module.params.get("name")
 
@@ -349,6 +403,7 @@ def is_walrus(s3_url):
     else:
         return False
 
+
 def main():
 
     argument_spec = ec2_argument_spec()
@@ -434,8 +489,8 @@ def main():
     except Exception as e:
         module.fail_json(msg='Failed to connect to S3: %s' % str(e))
 
-    if connection is None: # this should never happen
+    if connection is None:  # this should never happen
-        module.fail_json(msg ='Unknown error, failed to create s3 connection, no information from boto.')
+        module.fail_json(msg='Unknown error, failed to create s3 connection, no information from boto.')
 
     state = module.params.get("state")
 

test/sanity/pep8/legacy-files.txt

@@ -57,7 +57,6 @@ lib/ansible/modules/cloud/amazon/rds_subnet_group.py
 lib/ansible/modules/cloud/amazon/redshift.py
 lib/ansible/modules/cloud/amazon/route53_health_check.py
 lib/ansible/modules/cloud/amazon/s3.py
-lib/ansible/modules/cloud/amazon/s3_bucket.py
 lib/ansible/modules/cloud/amazon/s3_lifecycle.py
 lib/ansible/modules/cloud/amazon/s3_logging.py
 lib/ansible/modules/cloud/amazon/s3_website.py