From d2569a3f7d62ca67c14d53599a754986990af602 Mon Sep 17 00:00:00 2001 From: Will Thames Date: Tue, 18 Sep 2018 09:53:44 +1000 Subject: [PATCH] Improve iam_group exception handling (#45599) * Improve iam_group exception handling Use AnsibleAWSModule for iam_group and handle BotoCoreErrors as well as ClientErrors. Use fail_json_aws to improve error messages * Add minimal iam_group test suite Update some of the read-only IAM permissions (this is not sufficient to run the test suite but it gets further than it did until it tries to add a (non-existent) user) * Clean up after tests --- .../testing_policies/security-policy.json | 4 + lib/ansible/modules/cloud/amazon/iam_group.py | 117 ++++++------------ test/integration/targets/iam_group/aliases | 2 + .../targets/iam_group/tasks/main.yml | 70 +++++++++++ 4 files changed, 115 insertions(+), 78 deletions(-) create mode 100644 test/integration/targets/iam_group/aliases create mode 100644 test/integration/targets/iam_group/tasks/main.yml diff --git a/hacking/aws_config/testing_policies/security-policy.json b/hacking/aws_config/testing_policies/security-policy.json index e2dea9726af..302b60b0851 100644 --- a/hacking/aws_config/testing_policies/security-policy.json +++ b/hacking/aws_config/testing_policies/security-policy.json @@ -3,12 +3,16 @@ "Statement": [ { "Action": [ + "iam:GetGroup", "iam:GetInstanceProfile", "iam:GetPolicy", "iam:GetPolicyVersion", "iam:GetRole", "iam:GetRolePolicy", + "iam:GetUser", + "iam:ListAttachedGroupPolicies", "iam:ListAttachedRolePolicies", + "iam:ListAttachedUserPolicies", "iam:ListGroups", "iam:ListInstanceProfiles", "iam:ListInstanceProfilesForRole", diff --git a/lib/ansible/modules/cloud/amazon/iam_group.py b/lib/ansible/modules/cloud/amazon/iam_group.py index ab8ffe5f148..765ebb25acd 100644 --- a/lib/ansible/modules/cloud/amazon/iam_group.py +++ b/lib/ansible/modules/cloud/amazon/iam_group.py @@ -161,14 +161,12 @@ users: sample: / ''' -from ansible.module_utils.basic import AnsibleModule -from ansible.module_utils.ec2 import camel_dict_to_snake_dict, ec2_argument_spec, get_aws_connection_info, boto3_conn -from ansible.module_utils.ec2 import HAS_BOTO3, AWSRetry - -import traceback +from ansible.module_utils.aws.core import AnsibleAWSModule +from ansible.module_utils.ec2 import camel_dict_to_snake_dict +from ansible.module_utils.ec2 import AWSRetry try: - from botocore.exceptions import ClientError, ParamValidationError + from botocore.exceptions import BotoCoreError, ClientError, ParamValidationError except ImportError: pass # caught by imported HAS_BOTO3 @@ -232,9 +230,8 @@ def create_or_update_group(connection, module): # Get group try: group = get_group(connection, module, params['GroupName']) - except ClientError as e: - module.fail_json(msg=e.message, exception=traceback.format_exc(), - **camel_dict_to_snake_dict(e.response)) + except (BotoCoreError, ClientError) as e: + module.fail_json_aws(e, msg="Couldn't get group") # If group is None, create it if group is None: @@ -245,11 +242,8 @@ def create_or_update_group(connection, module): try: group = connection.create_group(**params) changed = True - except ClientError as e: - module.fail_json(msg=e.message, exception=traceback.format_exc(), - **camel_dict_to_snake_dict(e.response)) - except ParamValidationError as e: - module.fail_json(msg=e.message, exception=traceback.format_exc()) + except (BotoCoreError, ClientError) as e: + module.fail_json_aws(e, msg="Couldn't create group") # Manage managed policies current_attached_policies = get_attached_policy_list(connection, module, params['GroupName']) @@ -266,11 +260,8 @@ def create_or_update_group(connection, module): if not module.check_mode: try: connection.detach_group_policy(GroupName=params['GroupName'], PolicyArn=policy_arn) - except ClientError as e: - module.fail_json(msg=e.message, exception=traceback.format_exc(), - **camel_dict_to_snake_dict(e.response)) - except ParamValidationError as e: - module.fail_json(msg=e.message, exception=traceback.format_exc()) + except (BotoCoreError, ClientError) as e: + module.fail_json_aws(e, msg="Couldn't detach policy from group %s" % params['GroupName']) # If there are policies to adjust that aren't in the current list, then things have changed # Otherwise the only changes were in purging above if set(managed_policies) - set(current_attached_policies_arn_list): @@ -280,18 +271,14 @@ def create_or_update_group(connection, module): for policy_arn in managed_policies: try: connection.attach_group_policy(GroupName=params['GroupName'], PolicyArn=policy_arn) - except ClientError as e: - module.fail_json(msg=e.message, exception=traceback.format_exc(), - **camel_dict_to_snake_dict(e.response)) - except ParamValidationError as e: - module.fail_json(msg=e.message, exception=traceback.format_exc()) + except (BotoCoreError, ClientError) as e: + module.fail_json_aws(e, msg="Couldn't attach policy to group %s" % params['GroupName']) # Manage group memberships try: current_group_members = get_group(connection, module, params['GroupName'])['Users'] - except ClientError as e: - module.fail_json(msg=e.message, exception=traceback.format_exc(), - **camel_dict_to_snake_dict(e.response)) + except (BotoCoreError, ClientError) as e: + module.fail_json_aws(e, "Couldn't get group %s" % params['GroupName']) current_group_members_list = [] for member in current_group_members: @@ -307,11 +294,8 @@ def create_or_update_group(connection, module): if not module.check_mode: try: connection.remove_user_from_group(GroupName=params['GroupName'], UserName=user) - except ClientError as e: - module.fail_json(msg=e.message, exception=traceback.format_exc(), - **camel_dict_to_snake_dict(e.response)) - except ParamValidationError as e: - module.fail_json(msg=e.message, exception=traceback.format_exc()) + except (BotoCoreError, ClientError) as e: + module.fail_json_aws(e, msg="Couldn't remove user %s from group %s" % (user, params['GroupName'])) # If there are users to adjust that aren't in the current list, then things have changed # Otherwise the only changes were in purging above if set(users) - set(current_group_members_list): @@ -321,20 +305,16 @@ def create_or_update_group(connection, module): for user in users: try: connection.add_user_to_group(GroupName=params['GroupName'], UserName=user) - except ClientError as e: - module.fail_json(msg=e.message, exception=traceback.format_exc(), - **camel_dict_to_snake_dict(e.response)) - except ParamValidationError as e: - module.fail_json(msg=e.message, exception=traceback.format_exc()) + except (BotoCoreError, ClientError) as e: + module.fail_json_aws(e, msg="Couldn't add user %s to group %s" % (user, params['GroupName'])) if module.check_mode: module.exit_json(changed=changed) # Get the group again try: group = get_group(connection, module, params['GroupName']) - except ClientError as e: - module.fail_json(msg=e.message, exception=traceback.format_exc(), - **camel_dict_to_snake_dict(e.response)) + except (BotoCoreError, ClientError) as e: + module.fail_json_aws(e, "Couldn't get group %s" % params['GroupName']) module.exit_json(changed=changed, iam_group=camel_dict_to_snake_dict(group)) @@ -346,9 +326,8 @@ def destroy_group(connection, module): try: group = get_group(connection, module, params['GroupName']) - except ClientError as e: - module.fail_json(msg=e.message, exception=traceback.format_exc(), - **camel_dict_to_snake_dict(e.response)) + except (BotoCoreError, ClientError) as e: + module.fail_json_aws(e, "Couldn't get group %s" % params['GroupName']) if group: # Check mode means we would remove this group if module.check_mode: @@ -358,37 +337,27 @@ def destroy_group(connection, module): try: for policy in get_attached_policy_list(connection, module, params['GroupName']): connection.detach_group_policy(GroupName=params['GroupName'], PolicyArn=policy['PolicyArn']) - except ClientError as e: - module.fail_json(msg=e.message, exception=traceback.format_exc(), - **camel_dict_to_snake_dict(e.response)) - except ParamValidationError as e: - module.fail_json(msg=e.message, exception=traceback.format_exc()) + except (BotoCoreError, ClientError) as e: + module.fail_json_aws(e, msg="Couldn't remove policy from group %s" % params['GroupName']) # Remove any users in the group otherwise deletion fails current_group_members_list = [] try: current_group_members = get_group(connection, module, params['GroupName'])['Users'] - except ClientError as e: - module.fail_json(msg=e.message, exception=traceback.format_exc(), - **camel_dict_to_snake_dict(e.response)) + except (BotoCoreError, ClientError) as e: + module.fail_json_aws(e, "Couldn't get group %s" % params['GroupName']) for member in current_group_members: current_group_members_list.append(member['UserName']) for user in current_group_members_list: try: connection.remove_user_from_group(GroupName=params['GroupName'], UserName=user) - except ClientError as e: - module.fail_json(msg=e.message, exception=traceback.format_exc(), - **camel_dict_to_snake_dict(e.response)) - except ParamValidationError as e: - module.fail_json(msg=e.message, exception=traceback.format_exc()) + except (BotoCoreError, ClientError) as e: + module.fail_json_aws(e, "Couldn't remove user %s from group %s" % (user, params['GroupName'])) try: connection.delete_group(**params) - except ClientError as e: - module.fail_json(msg=e.message, exception=traceback.format_exc(), - **camel_dict_to_snake_dict(e.response)) - except ParamValidationError as e: - module.fail_json(msg=e.message, exception=traceback.format_exc()) + except (BotoCoreError, ClientError) as e: + module.fail_json_aws(e, "Couldn't delete group %s" % params['GroupName']) else: module.exit_json(changed=False) @@ -423,29 +392,21 @@ def get_attached_policy_list(connection, module, name): def main(): - argument_spec = ec2_argument_spec() - argument_spec.update( - dict( - name=dict(required=True, type='str'), - managed_policy=dict(default=[], type='list'), - users=dict(default=[], type='list'), - state=dict(choices=['present', 'absent'], required=True), - purge_users=dict(default=False, type='bool'), - purge_policy=dict(default=False, type='bool') - ) + argument_spec = dict( + name=dict(required=True), + managed_policy=dict(default=[], type='list'), + users=dict(default=[], type='list'), + state=dict(choices=['present', 'absent'], required=True), + purge_users=dict(default=False, type='bool'), + purge_policy=dict(default=False, type='bool') ) - module = AnsibleModule( + module = AnsibleAWSModule( argument_spec=argument_spec, supports_check_mode=True ) - if not HAS_BOTO3: - module.fail_json(msg='boto3 required for this module') - - region, ec2_url, aws_connect_params = get_aws_connection_info(module, boto3=True) - - connection = boto3_conn(module, conn_type='client', resource='iam', region=region, endpoint=ec2_url, **aws_connect_params) + connection = module.client('iam') state = module.params.get("state") diff --git a/test/integration/targets/iam_group/aliases b/test/integration/targets/iam_group/aliases new file mode 100644 index 00000000000..67ae2cc73b6 --- /dev/null +++ b/test/integration/targets/iam_group/aliases @@ -0,0 +1,2 @@ +unsupported +cloud/aws diff --git a/test/integration/targets/iam_group/tasks/main.yml b/test/integration/targets/iam_group/tasks/main.yml new file mode 100644 index 00000000000..1a5146fb8f7 --- /dev/null +++ b/test/integration/targets/iam_group/tasks/main.yml @@ -0,0 +1,70 @@ +- name: set up aws connection info + set_fact: + aws_connection_info: &aws_connection_info + aws_access_key: "{{ aws_access_key }}" + aws_secret_key: "{{ aws_secret_key }}" + security_token: "{{ security_token }}" + region: "{{ aws_region }}" + no_log: yes + +- name: ensure ansible user exists + iam_user: + name: AnsibleTestUser + state: present + <<: *aws_connection_info + +- name: ensure group exists + iam_group: + name: ansible_test + users: + - AnsibleTestUser + state: present + <<: *aws_connection_info + register: iam_group + +- assert: + that: + - iam_group.users + +- name: add non existent user to group + iam_group: + name: ansible_test + users: + - AnsibleTestUser + - NonExistentUser + state: present + <<: *aws_connection_info + ignore_errors: yes + register: iam_group + +- name: assert that adding non existent user to group fails with helpful message + assert: + that: + - iam_group is failed + - iam_group.msg.startswith("Couldn't add user NonExistentUser to group ansible_test") + +- name: remove a user + iam_group: + name: ansible_test + purge_users: True + users: [] + state: present + <<: *aws_connection_info + register: iam_group + +- assert: + that: + - iam_group.changed + - not iam_group.users + +- name: remove group + iam_group: + name: ansible_test + state: absent + <<: *aws_connection_info + +- name: remove ansible user + iam_user: + name: AnsibleTestUser + state: absent + <<: *aws_connection_info