Repository: ansible/ansible
Pull request: aws_kms: don't append to unicode - fixes #25786
##### SUMMARY
The statement may not have a principal, and if there is only one AWS principal it will not be a list as expected. Fixes #25786. Also added the exception message to the exception handling for more transparency and fixed changed always showing False.

##### ISSUE TYPE
 - Bugfix Pull Request

##### COMPONENT NAME
lib/ansible/modules/cloud/amazon/aws_kms.py

##### ANSIBLE VERSION
```
2.4.0
```


Problem statement:
aws_kms cannot append role_name/role_arn to key
<!---
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
aws_kms

##### ANSIBLE VERSION
<!--- Paste verbatim output from “ansible --version” between quotes below -->
```
ansible 2.3.0.0
```

##### CONFIGURATION
<!---
-->

##### OS / ENVIRONMENT
macOS 10.12.5, Amazon Linux 2017.03

##### SUMMARY
When attempting to add a role to a kms key Ansible will error out if the key policy does not return a list.  So if there's only 1 role in a key policy that has grant permissions, the key policy that AWS will return a string instead of a list.  Line 184 has ```statement['Principal']['AWS'].append(role_arn)``` .  There needs to be logic for when there is 0 entries, or 1 entry otherwise Ansible will throw an error.

##### STEPS TO REPRODUCE
<!---
1. Create a kms key with 1 key user
2.  Launch ansible to add a role
-->

<!--- Paste example playbooks or commands between quotes below -->
```yaml
- hosts: all
  tasks:
    - name: Update KMS key permissions
      aws_kms:
        mode: grant
        clean_invalid_entries: yes
        key_alias: "alias/key"
        role_name: "appInstanceRole"
        grant_types: "role"

```

<!--- You can also paste gist.github.com links for larger files -->

##### EXPECTED RESULTS
I expect that two roles will appear in the KMS key's key user policy.

##### ACTUAL RESULTS
<!--- What actually happened? If possible run with extra verbosity (-vvvv) -->

<!--- Paste verbatim command output between quotes below -->
```
The full traceback is:
Traceback (most recent call last):
  File "/var/folders/yx/qwr3s2r17n7bmnlhnd0885v40000gn/T/ansible_Ze4JvQ/ansible_module_aws_kms.py", line 286, in main
    ret = do_grant(kms, module.params['key_arn'], module.params['role_arn'], module.params['grant_types'], mode=mode, dry_run=module.check_mode, clean_invalid_entries=module.params['clean_invalid_entries'])
  File "/var/folders/yx/qwr3s2r17n7bmnlhnd0885v40000gn/T/ansible_Ze4JvQ/ansible_module_aws_kms.py", line 184, in do_grant
    statement['Principal']['AWS'].append(role_arn)
AttributeError: 'unicode' object has no attribute 'append'

fatal: [localhost]: FAILED! => {
    "changed": false, 
    "failed": true, 
    "invocation": {
        "module_args": {
            "aws_access_key": null, 
            "aws_secret_key": null, 
            "clean_invalid_entries": true, 
            "ec2_url": null, 
            "grant_types": [
                "role"
            ], 
            "key_alias": null, 
            "key_arn": "arn:aws:kms:us-east-1:1234567890:key/e56407e5-807c-4b50-a734-22102fc4ca91", 
            "mode": "grant", 
            "profile": null, 
            "region": null, 
            "role_arn": "arn:aws:iam::1234567890:role/lambda-test", 
            "role_name": "lambda-test", 
            "security_token": null, 
            "validate_certs": true
        }
    }, 
    "msg": "'unicode' object has no attribute 'append' 'unicode' object has no attribute 'append' - <type 'exceptions.AttributeError'>"
}

```

Hints from issue discussion:
cc @willthames
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->

Commit message: remove len(), only catch exception in function for json.dumps() failure

Patch to review:
diff --git a/lib/ansible/modules/cloud/amazon/aws_kms.py b/lib/ansible/modules/cloud/amazon/aws_kms.py
index 81e36e3dfa890b..477860929eb17b 100644
--- a/lib/ansible/modules/cloud/amazon/aws_kms.py
+++ b/lib/ansible/modules/cloud/amazon/aws_kms.py
@@ -165,12 +165,18 @@ def do_grant(kms, keyarn, role_arn, granttypes, mode='grant', dry_run=True, clea
             # do we want this grant type? Are we on its statement?
             # and does the role have this grant type?
 
+            # Ensure statement looks as expected
+            if not statement.get('Principal'):
+                statement['Principal'] = {'AWS': []}
+            if not isinstance(statement['Principal']['AWS'], list):
+                statement['Principal']['AWS'] = [statement['Principal']['AWS']]
+
             if mode == 'grant' and statement['Sid'] == statement_label[granttype]:
                 # we're granting and we recognize this statement ID.
 
                 if granttype in granttypes:
                     invalid_entries = list(filter(lambda x: not x.startswith('arn:aws:iam::'), statement['Principal']['AWS']))
-                    if clean_invalid_entries and len(list(invalid_entries)):
+                    if clean_invalid_entries and invalid_entries:
                         # we have bad/invalid entries. These are roles that were deleted.
                         # prune the list.
                         valid_entries = filter(lambda x: x.startswith('arn:aws:iam::'), statement['Principal']['AWS'])
@@ -197,12 +203,12 @@ def do_grant(kms, keyarn, role_arn, granttypes, mode='grant', dry_run=True, clea
     try:
         if len(changes_needed) and not dry_run:
             policy_json_string = json.dumps(policy)
-            kms.put_key_policy(KeyId=keyarn, PolicyName='default', Policy=policy_json_string)
-    except:
-        raise Exception("{}: // {}".format("e", policy_json_string))
+    except Exception as e:
+            raise Exception("{}: // {}".format(e, policy_json_string))
+    kms.put_key_policy(KeyId=keyarn, PolicyName='default', Policy=policy_json_string)
 
-        # returns nothing, so we have to just assume it didn't throw
-        ret['changed'] = True
+    # returns nothing, so we have to just assume it didn't throw
+    ret['changed'] = changes_needed and not had_invalid_entries
 
     ret['changes_needed'] = changes_needed
     ret['had_invalid_entries'] = had_invalid_entries


Code review comments:
diff_hunk: @@ -197,12 +203,12 @@ def do_grant(kms, keyarn, role_arn, granttypes, mode='grant', dry_run=True, clea
     try:
         if len(changes_needed) and not dry_run:
             policy_json_string = json.dumps(policy)
-            kms.put_key_policy(KeyId=keyarn, PolicyName='default', Policy=policy_json_string)
-    except:
-        raise Exception("{}: // {}".format("e", policy_json_string))
+    except Exception as e:
+            raise Exception("{}: // {}".format(e, policy_json_string))
path: lib/ansible/modules/cloud/amazon/aws_kms.py
text: @user1:
There is one problem with doing it this way.  If json.dumps() raises an exception then policy_json_string won't be set to a value.  Instead, you return the repr of the policy dict:
``` python
raise Exception("{0} // {1}".format(e, repr(policy))
```

(I also remembered that the "{}" format string isn't legal on python-2.6 so we have to use the numbers inside of them.

@author:
You're right. Sorry!