Repository: ansible/ansible
Pull request: s3_bucket: policy comparison - fixes #25428
##### SUMMARY
fixes #25428
Since policy statements are a list of dicts and values of said dicts may be identical except different types, comparison is faulty. Before this fix listing a single resource as a list in a policy always results in changed == True (because boto or the S3 API "fixes" this and removes unnecessary [] leaving only the list content). Before this fix reordering the statements of a policy while keeping the contents identical also resulted in changed == True. Another reproducer for this bug can be specifying the Principal like `"Principal": {"AWS": ["arn:aws:iam::XXXXXXXXXXXX:user/name"]}` (as opposed to `"Principal": {"AWS": "arn:aws:iam::XXXXXXXXXXXX:user/name"}`), which will always show changed as being True before this fix.

##### ISSUE TYPE
 - Bugfix Pull Request

##### COMPONENT NAME
lib/ansible/modules/cloud/amazon/s3_bucket.py

##### ANSIBLE VERSION
```
2.4.0
```


Problem statement:
s3_module bug when specifying IAM principle in bucket policy
<!---
Verify first that your issue/request is not already reported on GitHub.
Also test if the latest release, and master branch are affected too.
-->

##### ISSUE TYPE
<!--- Pick one below and delete the rest: -->
 - Bug Report


##### COMPONENT NAME
<!--- Name of the module/plugin/task/feature -->
s3_module
##### ANSIBLE VERSION
<!--- Paste verbatim output from “ansible --version” between quotes below -->
```
ansible 2.3.0.0
  config file = /etc/ansible/ansible.cfg
  configured module search path = Default w/o overrides
  python version = 2.7.13 (default, Dec 18 2016, 07:03:39) [GCC 4.2.1 Compatible Apple LLVM 8.0.0 (clang-800.0.42.1)]
```

##### CONFIGURATION
<!---
Mention any settings you have changed/added/removed in ansible.cfg
(or using the ANSIBLE_* environment variables).
-->
#pipelining = False 
##### OS / ENVIRONMENT
<!---
Mention the OS you are running Ansible from, and the OS you are
managing, or say “N/A” for anything that is not platform-specific.
-->
Mac 10.12.5 Sierra
##### SUMMARY
<!--- Explain the problem briefly -->
s3_module shows change when running bucket play with policy specifying aws account principle. Remove the principle from the policy and ansible returns to normal working order. Add the principle back and concurrent runs show changes being made to the bucket even when the config is static.
##### STEPS TO REPRODUCE
<!---
For bugs, show exactly how to reproduce the problem, using a minimal test-case.
For new features, show how the feature would be used.
-->
Create a bucket with a sample policy doing say SSE enforcement and IP blocking. Behavior is proper, rerunning the play yields "no change" effect, reinforcing the config.

However when adding in a principle entity to allow access the policy is written, but subsequent runs show changing to the bucket even when your config remains static. I have tested this and found this issue goes away when removal of user principle within the bucket policy.

    {
      "Sid": "Stmt1496841720453",
      "Action": "s3:*",
      "Effect": "Allow",
      "Resource": "arn:aws:s3:::bucketname",
      "Principal": {
        "AWS": [
          "arn:aws:iam::<acct_number_no_hyphens>:user/<userid>"
        ]
      }
    }


<!--- Paste example playbooks or commands between quotes below -->
```yaml

```

<!--- You can also paste gist.github.com links for larger files -->

##### EXPECTED RESULTS
<!--- What did you expect to happen when running the steps above? -->
See notes above, ansible makes the change, but subsequent runs show change when no config change has been made.
##### ACTUAL RESULTS
<!--- What actually happened? If possible run with extra verbosity (-vvvv) -->

<!--- Paste verbatim command output between quotes below -->
```

```

Hints from issue discussion:
Hi @shdobxr, thanks for opening the issue. Are you using the s3 module or s3_bucket module?
Hey there, s3_bucket module.

Dan

On Thu, Jun 8, 2017 at 8:18 AM, Sloane Hertel <notifications@github.com>
wrote:

> Hi @shdobxr <https://github.com/shdobxr>, thanks for opening the issue.
> Are you using the s3 module or s3_bucket module?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/ansible/ansible/issues/25428#issuecomment-307086173>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABlR-x2jnt9ZWPIsVXZchhxBn30LrdYvks5sB-asgaJpZM4Nytr2>
> .
>

A workaround for this is to not specify the brackets around the principal if there is only one. Working on a solution though!
Much thanks. I typically lend brackets to my Jon templates for variables.
Even if the variable list is only one element. Valid point.

On Jun 14, 2017 2:52 PM, "Sloane Hertel" <notifications@github.com> wrote:

> A workaround for this is to not specify the brackets around the principal
> if there is only one. Working on a solution though!
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/ansible/ansible/issues/25428#issuecomment-308524786>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABlR-z1LpIWoqsLBHt_5ioUYnjRncFDZks5sECvsgaJpZM4Nytr2>
> .
>

Is "Jon" an autocorrect of jinja, or are you doing some further var templating? 
Sorry typo by autocorrect.

For my bucket policies I build json templates with variables that I then
leverage in my play.

On Jun 14, 2017 3:03 PM, "Ryan Brown" <notifications@github.com> wrote:

Is "Jon" an autocorrect of jinja, or are you doing some further var
templating?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<https://github.com/ansible/ansible/issues/25428#issuecomment-308527712>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABlR-0T91Dvy_DamxJAyaBMNmu5gBrSpks5sEC6XgaJpZM4Nytr2>
.

I've made a fix but it seems like a lot of new code to introduce for so little a fix so I don't know if it's worth merging. Maybe there is a better way. But your feedback on it would be appreciated if you want to test it. :) 
Sure! I can pull the branch and run it today.

While not the first time I have pulled a branch. How do I pull your code
just to be sure.

On Jun 14, 2017 5:56 PM, "Sloane Hertel" <notifications@github.com> wrote:

> I've made a fix but it seems like a lot of new code to introduce for so
> little a fix so I don't know if it's worth merging. But your feedback on it
> would be appreciated if you want to test it. :)
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/ansible/ansible/issues/25428#issuecomment-308569732>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABlR-x6zFcn8HcH4kvnl-r-kuQ0S3Vn1ks5sEFb-gaJpZM4Nytr2>
> .
>

No obligation!

If you want to test:
I’m not sure if you’ve cloned the Ansible repo…If you haven't, that is where I would start.
* In the directory of the cloned repo, start on the devel branch `git checkout devel`
* make sure the devel branch is up to date (`git pull git@github.com:ansible/ansible.git devel`, or if you have added the upstream as a remote `git pull upstream devel`)
* Checkout a new branch: `git checkout -b scratch-branch`
* Pulling my changes: `git pull git@github.com:s-hertel/ansible.git s3_bucket_policy`
* Before testing those changes: `source hacking/env-setup` (this will ensure scratch-branch is the Ansible you're running in case you have it installed)

Feel free to ping me here or on IRC (shertel) as well.

Commit message: abbreviate example and logic

Patch to review:
diff --git a/lib/ansible/modules/cloud/amazon/s3_bucket.py b/lib/ansible/modules/cloud/amazon/s3_bucket.py
index 1e2243705e8c46..92fcb9d9b5c271 100644
--- a/lib/ansible/modules/cloud/amazon/s3_bucket.py
+++ b/lib/ansible/modules/cloud/amazon/s3_bucket.py
@@ -120,6 +120,7 @@
 
 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,60 @@ 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.
+    """
+    if set(hashable_policy(new_policy, [])) != set(hashable_policy(current_policy, [])):
+        return True
+
+    return False
+
+
 def _create_or_update_bucket(connection, module, location):
 
     policy = module.params.get("policy")
@@ -195,9 +250,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 +275,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))
-                changed = True
+                if changed:
+                    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 +348,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 +406,7 @@ def is_walrus(s3_url):
     else:
         return False
 
+
 def main():
 
     argument_spec = ec2_argument_spec()
@@ -434,8 +492,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
-        module.fail_json(msg ='Unknown error, failed to create s3 connection, no information from boto.')
+    if connection is None:  # this should never happen
+        module.fail_json(msg='Unknown error, failed to create s3 connection, no information from boto.')
 
     state = module.params.get("state")
 
diff --git a/test/sanity/pep8/legacy-files.txt b/test/sanity/pep8/legacy-files.txt
index a40429de7286a7..db2e8e02532768 100644
--- a/test/sanity/pep8/legacy-files.txt
+++ b/test/sanity/pep8/legacy-files.txt
@@ -59,7 +59,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


Code review comments:
diff_hunk: @@ -155,6 +156,60 @@ 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.
+    """
+    if set(hashable_policy(new_policy, [])) != set(hashable_policy(current_policy, [])):
path: lib/ansible/modules/cloud/amazon/s3_bucket.py
text: @user1:
This can just be `return set(hashable_policy(new_policy, [])) != set(hashable_policy(current_policy, []))`