s3_bucket: fix python3 sorting incompatibility (#27502)

* s3_bucket: fix policy sorting for python3 so strings are evaluated as less than tuples.

Add tests to ensure this behavior is maintained.

* Fix s3_bucket comparison function to work on both Python 3.5 and 3.6

* s3_bucket: document that cmp_to_key is used for python 2.7.

Add another test for s3_bucket to compare policies of different sizes.

* fix pep8

* Work around code-smell grepping by not using the word 'cmp'.

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

@@ -134,6 +134,14 @@ try:
 except ImportError:
     HAS_BOTO = False
 
+try:
+    # Although this is to allow Python 3 the ability to use the custom comparison as a key, Python 2.7 also
+    # uses this (and it works as expected). Python 2.6 will trigger the ImportError.
+    from functools import cmp_to_key
+    PY3_COMPARISON = True
+except ImportError:
+    PY3_COMPARISON = False
+
 
 def get_request_payment_status(bucket):
 
@@ -196,10 +204,35 @@ def hashable_policy(policy, policy_list):
     if len(policy_list) == 1 and isinstance(policy_list[0], tuple):
         policy_list = policy_list[0]
     if isinstance(policy_list, list):
+        if PY3_COMPARISON:
+            policy_list.sort(key=cmp_to_key(py3cmp))
+        else:
             policy_list.sort()
     return policy_list
 
 
+def py3cmp(a, b):
+    """ Python 2 can sort lists of mixed types. Strings < tuples. Without this function this fails on Python 3."""
+    try:
+        if a > b:
+            return 1
+        elif a < b:
+            return -1
+        else:
+            return 0
+    except TypeError as e:
+        # check to see if they're tuple-string
+        # always say strings are less than tuples (to maintain compatibility with python2)
+        str_ind = to_text(e).find('str')
+        tup_ind = to_text(e).find('tuple')
+        if -1 not in (str_ind, tup_ind):
+            if str_ind < tup_ind:
+                return -1
+            elif tup_ind < str_ind:
+                return 1
+        raise
+
+
 def compare_policies(current_policy, new_policy):
     """ Compares the existing policy and the updated policy
         Returns True if there is a difference between policies.

test/units/modules/cloud/amazon/test_s3_bucket.py

@@ -0,0 +1,164 @@
+# (c) 2017 Red Hat Inc.
+#
+# This file is part of Ansible
+#
+# Ansible is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# Ansible is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with Ansible.  If not, see <http://www.gnu.org/licenses/>.
+
+# (c) 2017 Red Hat Inc.
+
+from ansible.modules.cloud.amazon.s3_bucket import compare_policies
+
+small_policy_one = {
+    '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']}
+        }
+    ]
+}
+
+# The same as small_policy_one, except the single resource is in a list and the contents of Statement are jumbled
+small_policy_two = {
+    'Version': '2012-10-17',
+    'Statement': [
+        {
+            'Effect': 'Allow',
+            'Action': 's3:PutObjectAcl',
+            'Principal': {'AWS': ['arn:aws:iam::XXXXXXXXXXXX:user/username1', 'arn:aws:iam::XXXXXXXXXXXX:user/username2']},
+            'Resource': ['arn:aws:s3:::test_policy/*'],
+            'Sid': 'AddCannedAcl2'
+        }
+    ]
+}
+
+larger_policy_one = {
+    "Version": "2012-10-17",
+    "Statement": [
+        {
+            "Sid": "Test",
+            "Effect": "Allow",
+            "Principal": {
+                "AWS": [
+                    "arn:aws:iam::XXXXXXXXXXXX:user/testuser1",
+                    "arn:aws:iam::XXXXXXXXXXXX:user/testuser2"
+                ]
+            },
+            "Action": "s3:PutObjectAcl",
+            "Resource": "arn:aws:s3:::test_policy/*"
+        },
+        {
+            "Effect": "Allow",
+            "Principal": {
+                "AWS": "arn:aws:iam::XXXXXXXXXXXX:user/testuser2"
+            },
+            "Action": [
+                "s3:PutObject",
+                "s3:PutObjectAcl"
+            ],
+            "Resource": "arn:aws:s3:::test_policy/*"
+        }
+    ]
+}
+
+# The same as larger_policy_one, except having a list of length 1 and jumbled contents
+larger_policy_two = {
+    "Version": "2012-10-17",
+    "Statement": [
+        {
+            "Principal": {
+               "AWS": ["arn:aws:iam::XXXXXXXXXXXX:user/testuser2"]
+            },
+            "Effect": "Allow",
+            "Resource": "arn:aws:s3:::test_policy/*",
+            "Action": [
+                "s3:PutObject",
+                "s3:PutObjectAcl"
+            ]
+        },
+        {
+            "Action": "s3:PutObjectAcl",
+            "Principal": {
+                "AWS": [
+                    "arn:aws:iam::XXXXXXXXXXXX:user/testuser1",
+                    "arn:aws:iam::XXXXXXXXXXXX:user/testuser2"
+                ]
+            },
+            "Sid": "Test",
+            "Resource": "arn:aws:s3:::test_policy/*",
+            "Effect": "Allow"
+        }
+    ]
+}
+
+# Different than larger_policy_two: a different principal is given
+larger_policy_three = {
+    "Version": "2012-10-17",
+    "Statement": [
+        {
+            "Principal": {
+                "AWS": ["arn:aws:iam::XXXXXXXXXXXX:user/testuser2"]
+            },
+            "Effect": "Allow",
+            "Resource": "arn:aws:s3:::test_policy/*",
+            "Action": [
+                "s3:PutObject",
+                "s3:PutObjectAcl"]
+        },
+        {
+            "Action": "s3:PutObjectAcl",
+            "Principal": {
+                "AWS": [
+                    "arn:aws:iam::XXXXXXXXXXXX:user/testuser1",
+                    "arn:aws:iam::XXXXXXXXXXXX:user/testuser3"
+                ]
+            },
+            "Sid": "Test",
+            "Resource": "arn:aws:s3:::test_policy/*",
+            "Effect": "Allow"
+        }
+    ]
+}
+
+
+def test_compare_small_policies_without_differences():
+    """ Testing two small policies which are identical except for:
+            * The contents of the statement are in different orders
+            * The second policy contains a list of length one whereas in the first it is a string
+    """
+    assert compare_policies(small_policy_one, small_policy_two) is False
+
+
+def test_compare_large_policies_without_differences():
+    """ Testing two larger policies which are identical except for:
+            * The statements are in different orders
+            * The contents of the statements are also in different orders
+            * The second contains a list of length one for the Principal whereas in the first it is a string
+    """
+    assert compare_policies(larger_policy_one, larger_policy_two) is False
+
+
+def test_compare_larger_policies_with_difference():
+    """ Testing two larger policies which are identical except for:
+            * one different principal
+    """
+    assert compare_policies(larger_policy_two, larger_policy_three)
+
+
+def test_compare_smaller_policy_with_larger():
+    """ Testing two policies of different sizes """
+    assert compare_policies(larger_policy_one, small_policy_one)