new AWS KMS module #19309
new AWS KMS module #19309
Conversation
- add/remove user/role from a KMS key.
|
Need a rerun on Shippable. |
ansibot
added
affects_2.3
|
|
||
| DOCUMENTATION = ''' | ||
| --- | ||
| module: kms |
There was a problem hiding this comment.
Can you rename this as aws_kms please? Three-letter acronyms have enough risk for overlap that it's probably worth doing.
There was a problem hiding this comment.
done. it's the only aws_ module, fwiw. But I agree on the likely overlap.
| DOCUMENTATION = ''' | ||
| --- | ||
| module: kms | ||
| short_description: Perform various KMS management tasks. |
There was a problem hiding this comment.
Maybe Manage role/user access to a KMS key would be more descriptive.
| required: false | ||
| role_name: | ||
| description: | ||
| - Role to allow/deny access. One of C(role_name) or C(role_arn) are required. |
There was a problem hiding this comment.
Please also add role_arn as a doc param.
| if policy['Version'] != "2012-10-17": | ||
| errors.append('Unknown version/date ({}) of policy. Things are probably different than we assumed they were.'.format(policy['Version'])) | ||
|
|
||
| haz = {} |
There was a problem hiding this comment.
Not everyone on the codebase is aware that has==haz, or even has English-as-a-first-language. Can you rename this to be clearer?
|
|
||
| except Exception as err: | ||
| error_msg = boto_exception(err) | ||
| module.fail_json(msg=error_msg, traceback=traceback.format_exc().splitlines()) |
There was a problem hiding this comment.
When using fail_json, please include exception=traceback.format_exc() as a kwarg.
| kms = ansible.module_utils.ec2.boto3_conn(module, conn_type='client', resource='kms', region=region, endpoint=ec2_url, **aws_connect_kwargs) | ||
| iam = ansible.module_utils.ec2.boto3_conn(module, conn_type='client', resource='iam', region=region, endpoint=ec2_url, **aws_connect_kwargs) | ||
| except botocore.exceptions.NoCredentialsError as e: | ||
| module.fail_json(msg=str(e)) |
There was a problem hiding this comment.
When using fail_json, please include exception=traceback.format_exc() as a kwarg.
There was a problem hiding this comment.
okay. Looks like usage of this for the "create boto conn" exception is all over the map, fwiw.,
ansibot
added
needs_revision
If the KMS world gets orphaned ARNs (pointing to instances that no longer exist, for instance), a policy can't be edited. The cleanup is basically garbage collection; I know no reason to disable it, but leaving it there.
|
Changes are in. Added "arn cleanup" from my own testing. |
ansibot
added
the
needs_revision
| # we're granting and we recognize this statement ID. | ||
|
|
||
| if granttype in granttypes: | ||
| if clean_invalid_entries and len(list(filter(lambda x: not x.startswith('arn:aws:iam::'), statement['Principal']['AWS']))): |
There was a problem hiding this comment.
Can you make this into a couple statements for easier readability? Something like:
invalid_entries = [x for x in statement['Principal']['AWS']
if not x.startswith('arn:aws:iam::)]
if clean_invalid_entries and len(invalid_entries):
for entry in invalid_entries:
statement['Principal']['AWS'].remove(entry)
Or there's always:
original_size = len(statement['Principal']['AWS'])
statement['Principal']['AWS'] = [x for x in statement['Principal']['AWS']
if x.startswith('arn:aws:iam::)]
have_invalid_entries = (original_size == len(statement['Principal']['AWS']))
There was a problem hiding this comment.
yeah, I iterated through this in a few ways. See if you like the latest.. or do you really dislike the filter()?
| type: dict | ||
| returned: always | ||
| sample: { "role": "add", "role grant": "add" } | ||
| have_invalid_entries: |
| module.fail_json(msg='cannot connect to AWS', exception=traceback.format_exc(e)) | ||
|
|
||
|
|
||
| if mode == 'grant' or mode == 'deny': |
There was a problem hiding this comment.
You don't need this conditional, since Ansible enforces that these are the only choices.
| if module.params['key_alias'] and not module.params['key_arn']: | ||
| module.params['key_arn'] = get_arn_from_kms_alias(kms, module.params['key_alias']) | ||
| if not module.params['key_arn']: | ||
| module.fail_json(msg='KMS ARN is required to {}'.format(mode)) |
There was a problem hiding this comment.
Add here that the key_alias or key_arn are both ways to provide it.
|
|
||
| try: | ||
| if len(changes_needed) and not dry_run: | ||
| #policy_json_string = json.dumps({'Policy': policy}) |
There was a problem hiding this comment.
Please remove this comment since it's not needed.
ansibot
removed
the
needs_revision
ansibot
added
needs_revision
|
|
ISSUE TYPE
COMPONENT NAME
kmsANSIBLE VERSION
SUMMARY