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

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",

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.aws.core import AnsibleAWSModule
-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 camel_dict_to_snake_dict
-from ansible.module_utils.ec2 import HAS_BOTO3, AWSRetry
+from ansible.module_utils.ec2 import AWSRetry
-
-import traceback
 
 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:
+    except (BotoCoreError, ClientError) as e:
-        module.fail_json(msg=e.message, exception=traceback.format_exc(),
+        module.fail_json_aws(e, msg="Couldn't get group")
-                         **camel_dict_to_snake_dict(e.response))
 
     # 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:
+        except (BotoCoreError, ClientError) as e:
-            module.fail_json(msg=e.message, exception=traceback.format_exc(),
+            module.fail_json_aws(e, msg="Couldn't create group")
-                             **camel_dict_to_snake_dict(e.response))
-        except ParamValidationError as e:
-            module.fail_json(msg=e.message, exception=traceback.format_exc())
 
     # 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:
+                    except (BotoCoreError, ClientError) as e:
-                        module.fail_json(msg=e.message, exception=traceback.format_exc(),
+                        module.fail_json_aws(e, msg="Couldn't detach policy from group %s" % params['GroupName'])
-                                         **camel_dict_to_snake_dict(e.response))
-                    except ParamValidationError as e:
-                        module.fail_json(msg=e.message, exception=traceback.format_exc())
         # 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:
+                    except (BotoCoreError, ClientError) as e:
-                        module.fail_json(msg=e.message, exception=traceback.format_exc(),
+                        module.fail_json_aws(e, msg="Couldn't attach policy to group %s" % params['GroupName'])
-                                         **camel_dict_to_snake_dict(e.response))
-                    except ParamValidationError as e:
-                        module.fail_json(msg=e.message, exception=traceback.format_exc())
 
     # Manage group memberships
     try:
         current_group_members = get_group(connection, module, params['GroupName'])['Users']
-    except ClientError as e:
+    except (BotoCoreError, ClientError) as e:
-        module.fail_json(msg=e.message, exception=traceback.format_exc(),
+        module.fail_json_aws(e, "Couldn't get group %s" % params['GroupName'])
-                         **camel_dict_to_snake_dict(e.response))
 
     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:
+                    except (BotoCoreError, ClientError) as e:
-                        module.fail_json(msg=e.message, exception=traceback.format_exc(),
+                        module.fail_json_aws(e, msg="Couldn't remove user %s from group %s" % (user, params['GroupName']))
-                                         **camel_dict_to_snake_dict(e.response))
-                    except ParamValidationError as e:
-                        module.fail_json(msg=e.message, exception=traceback.format_exc())
         # 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:
+                    except (BotoCoreError, ClientError) as e:
-                        module.fail_json(msg=e.message, exception=traceback.format_exc(),
+                        module.fail_json_aws(e, msg="Couldn't add user %s to group %s" % (user, params['GroupName']))
-                                         **camel_dict_to_snake_dict(e.response))
-                    except ParamValidationError as e:
-                        module.fail_json(msg=e.message, exception=traceback.format_exc())
     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:
+    except (BotoCoreError, ClientError) as e:
-        module.fail_json(msg=e.message, exception=traceback.format_exc(),
+        module.fail_json_aws(e, "Couldn't get group %s" % params['GroupName'])
-                         **camel_dict_to_snake_dict(e.response))
 
     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:
+    except (BotoCoreError, ClientError) as e:
-        module.fail_json(msg=e.message, exception=traceback.format_exc(),
+        module.fail_json_aws(e, "Couldn't get group %s" % params['GroupName'])
-                         **camel_dict_to_snake_dict(e.response))
     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:
+        except (BotoCoreError, ClientError) as e:
-            module.fail_json(msg=e.message, exception=traceback.format_exc(),
+            module.fail_json_aws(e, msg="Couldn't remove policy from group %s" % params['GroupName'])
-                             **camel_dict_to_snake_dict(e.response))
-        except ParamValidationError as e:
-            module.fail_json(msg=e.message, exception=traceback.format_exc())
 
         # 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:
+        except (BotoCoreError, ClientError) as e:
-            module.fail_json(msg=e.message, exception=traceback.format_exc(),
+            module.fail_json_aws(e, "Couldn't get group %s" % params['GroupName'])
-                             **camel_dict_to_snake_dict(e.response))
         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:
+            except (BotoCoreError, ClientError) as e:
-                module.fail_json(msg=e.message, exception=traceback.format_exc(),
+                module.fail_json_aws(e, "Couldn't remove user %s from group %s" % (user, params['GroupName']))
-                                 **camel_dict_to_snake_dict(e.response))
-            except ParamValidationError as e:
-                module.fail_json(msg=e.message, exception=traceback.format_exc())
 
         try:
             connection.delete_group(**params)
-        except ClientError as e:
+        except (BotoCoreError, ClientError) as e:
-            module.fail_json(msg=e.message, exception=traceback.format_exc(),
+            module.fail_json_aws(e, "Couldn't delete group %s" % params['GroupName'])
-                             **camel_dict_to_snake_dict(e.response))
-        except ParamValidationError as e:
-            module.fail_json(msg=e.message, exception=traceback.format_exc())
 
     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 = dict(
-    argument_spec.update(
+        name=dict(required=True),
-        dict(
+        managed_policy=dict(default=[], type='list'),
-            name=dict(required=True, type='str'),
+        users=dict(default=[], type='list'),
-            managed_policy=dict(default=[], type='list'),
+        state=dict(choices=['present', 'absent'], required=True),
-            users=dict(default=[], type='list'),
+        purge_users=dict(default=False, type='bool'),
-            state=dict(choices=['present', 'absent'], required=True),
+        purge_policy=dict(default=False, type='bool')
-            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:
+    connection = module.client('iam')
-        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)
 
     state = module.params.get("state")
 

test/integration/targets/iam_group/aliases

@@ -0,0 +1,2 @@
+unsupported
+cloud/aws

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