iam_policy - update examples, add RETURN, add required_if case (#1093)

iam_policy - update examples, add RETURN, add required_if case SUMMARY fix broken example add RETURN documentation require one of policy_document or policy_json when state is present add extra integration tests for check mode idempotency cases ISSUE TYPE Bugfix Pull Request COMPONENT NAME iam_policy Reviewed-by: Mark Chappell <None> Reviewed-by: Markus Bergholz <[email protected]> (cherry picked from commit c978414)
ansible-collections · Apr 26, 2022 · f63ca2a · f63ca2a
1 parent 5eec058
commit f63ca2a
Show file tree

Hide file tree

Showing 3 changed files with 176 additions and 13 deletions.
diff --git a/changelogs/fragments/1093-iam_policy-update-docs-and-add-required_if.yml b/changelogs/fragments/1093-iam_policy-update-docs-and-add-required_if.yml
@@ -0,0 +1,4 @@
+minor_changes:
+  - iam_policy - update broken examples and add RETURN section to documentation; add extra integration tests for idempotency check mode runs (https://github.com/ansible-collections/community.aws/pull/1093).
+bugfixes:
+  - iam_policy - require one of `policy_document` and `policy_json` when state is present to prevent MalformedPolicyDocumentException from being thrown (https://github.com/ansible-collections/community.aws/pull/1093).
diff --git a/plugins/modules/iam_policy.py b/plugins/modules/iam_policy.py
@@ -82,8 +82,7 @@
 # Advanced example, create two new groups and add a READ-ONLY policy to both
 # groups.
 - name: Create Two Groups, Mario and Luigi
-  community.aws.iam:
-    iam_type: group
+  community.aws.iam_group:
     name: "{{ item }}"
     state: present
   loop:
@@ -94,9 +93,9 @@
 - name: Apply READ-ONLY policy to new groups that have been recently created
   community.aws.iam_policy:
     iam_type: group
-    iam_name: "{{ item.created_group.group_name }}"
+    iam_name: "{{ item.iam_group.group.group_name }}"
     policy_name: "READ-ONLY"
-    policy_document: readonlypolicy.json
+    policy_json: "{{ lookup('template', 'readonly.json.j2') }}"
     state: present
   loop: "{{ new_groups.results }}"
 
@@ -107,22 +106,31 @@
     iam_name: "{{ item.user }}"
     policy_name: "s3_limited_access_{{ item.prefix }}"
     state: present
-    policy_json: " {{ lookup( 'template', 's3_policy.json.j2') }} "
+    policy_json: "{{ lookup('template', 's3_policy.json.j2') }}"
     loop:
       - user: s3_user
         prefix: s3_user_prefix
 
 '''
+RETURN = '''
+policies:
+    description: A list of names of the inline policies embedded in the specified IAM resource (user, group, or role).
+    returned: always
+    type: list
+    elements: str
+'''
+
 import json
 
 try:
     from botocore.exceptions import BotoCoreError, ClientError
 except ImportError:
     pass
 
-from ansible_collections.amazon.aws.plugins.module_utils.core import AnsibleAWSModule
-from ansible_collections.amazon.aws.plugins.module_utils.ec2 import compare_policies, AWSRetry
 from ansible.module_utils.six import string_types
+from ansible_collections.amazon.aws.plugins.module_utils.core import AnsibleAWSModule
+from ansible_collections.amazon.aws.plugins.module_utils.ec2 import AWSRetry
+from ansible_collections.amazon.aws.plugins.module_utils.ec2 import compare_policies
 
 
 class PolicyError(Exception):
@@ -174,9 +182,9 @@ def delete(self):
             self.changed = False
             return
 
-        self.changed = True
         if not self.check_mode:
             self._delete(self.name, self.policy_name)
+        self.changed = True
 
     def get_policy_text(self):
         try:
@@ -298,8 +306,16 @@ def main():
         skip_duplicates=dict(type='bool', default=None, required=False)
     )
     mutually_exclusive = [['policy_document', 'policy_json']]
-
-    module = AnsibleAWSModule(argument_spec=argument_spec, mutually_exclusive=mutually_exclusive, supports_check_mode=True)
+    required_if = [
+        ('state', 'present', ('policy_document', 'policy_json'), True),
+    ]
+
+    module = AnsibleAWSModule(
+        argument_spec=argument_spec,
+        mutually_exclusive=mutually_exclusive,
+        required_if=required_if,
+        supports_check_mode=True
+    )
 
     skip_duplicates = module.params.get('skip_duplicates')
 

diff --git a/tests/integration/targets/iam_policy/tasks/object.yml b/tests/integration/targets/iam_policy/tasks/object.yml
@@ -30,6 +30,22 @@
       - iam_policy_info is succeeded
 
   # ============================================================
+  - name: 'Invalid creation of policy for {{ iam_type }} - missing required parameters'
+    iam_policy:
+      state: present
+      iam_type: '{{ iam_type }}'
+      iam_name: '{{ iam_name }}'
+      policy_name: '{{ iam_policy_name_a }}'
+      skip_duplicates: yes
+    register: result
+    ignore_errors: yes
+
+  - name: Assert task failed with correct error message
+    assert:
+      that:
+        - result.failed
+        - "'state is present but any of the following are missing: policy_document, policy_json' in result.msg"
+
   - name: 'Create policy using document for {{ iam_type }} (check mode)'
     check_mode: yes
     iam_policy:
@@ -75,6 +91,22 @@
       - iam_policy_info.policies[0].policy_name == iam_policy_name_a
       - '"Id" not in iam_policy_info.policies[0].policy_document'
 
+  - name: 'Create policy using document for {{ iam_type }} (idempotency - check mode)'
+    iam_policy:
+      state: present
+      iam_type: '{{ iam_type }}'
+      iam_name: '{{ iam_name }}'
+      policy_name: '{{ iam_policy_name_a }}'
+      policy_document: '{{ tmpdir.path }}/no_access.json'
+      skip_duplicates: yes
+    register: result
+    check_mode: yes
+
+  - name: 'Assert no change would occur'
+    assert:
+      that:
+      - result is not changed
+
   - name: 'Create policy using document for {{ iam_type }} (idempotency)'
     iam_policy:
       state: present
@@ -208,6 +240,22 @@
       - iam_policy_info.policies[0].policy_name == iam_policy_name_b
       - '"Id" not in iam_policy_info.policies[0].policy_document'
 
+  - name: 'Create policy using document for {{ iam_type }} (idempotency - check mode) (skip_duplicates = no)'
+    iam_policy:
+      state: present
+      skip_duplicates: no
+      iam_type: '{{ iam_type }}'
+      iam_name: '{{ iam_name }}'
+      policy_name: '{{ iam_policy_name_b }}'
+      policy_document: '{{ tmpdir.path }}/no_access.json'
+    register: result
+    check_mode: yes
+
+  - name: 'Assert no change would occur'
+    assert:
+      that:
+      - result is not changed
+
   - name: 'Create policy using document for {{ iam_type }} (idempotency) (skip_duplicates = no)'
     iam_policy:
       state: present
@@ -294,6 +342,22 @@
       - iam_policy_info.policies[0].policy_name == iam_policy_name_c
       - iam_policy_info.policies[0].policy_document.Id == 'MyId'
 
+  - name: 'Create policy using json for {{ iam_type }} (idempotency - check mode)'
+    iam_policy:
+      state: present
+      iam_type: '{{ iam_type }}'
+      iam_name: '{{ iam_name }}'
+      policy_name: '{{ iam_policy_name_c }}'
+      policy_json: '{{ lookup("file", "{{ tmpdir.path }}/no_access_with_id.json") }}'
+      skip_duplicates: yes
+    register: result
+    check_mode: yes
+
+  - name: 'Assert no change would occur'
+    assert:
+      that:
+      - result is not changed
+
   - name: 'Create policy using json for {{ iam_type }} (idempotency)'
     iam_policy:
       state: present
@@ -340,7 +404,7 @@
       policy_name: '{{ iam_policy_name_d }}'
     register: iam_policy_info
 
-  - name: 'Assert policy would be added for {{ iam_type }}'
+  - name: 'Assert policy would not be added for {{ iam_type }}'
     assert:
       that:
       - result is not changed
@@ -436,6 +500,22 @@
       - iam_policy_info.policies[0].policy_name == iam_policy_name_d
       - iam_policy_info.policies[0].policy_document.Id == 'MyId'
 
+  - name: 'Create policy using json for {{ iam_type }} (idempotency - check mode) (skip_duplicates = no)'
+    iam_policy:
+      state: present
+      skip_duplicates: no
+      iam_type: '{{ iam_type }}'
+      iam_name: '{{ iam_name }}'
+      policy_name: '{{ iam_policy_name_d }}'
+      policy_json: '{{ lookup("file", "{{ tmpdir.path }}/no_access_with_id.json") }}'
+    register: result
+    check_mode: yes
+
+  - name: 'Assert no change would occur'
+    assert:
+      that:
+      - result is not changed
+
   - name: 'Create policy using json for {{ iam_type }} (idempotency) (skip_duplicates = no)'
     iam_policy:
       state: present
@@ -506,7 +586,7 @@
       policy_name: '{{ iam_policy_name_a }}'
     register: iam_policy_info
 
-  - name: 'Assert policy would be added for {{ iam_type }}'
+  - name: 'Assert policy would not be added for {{ iam_type }}'
     assert:
       that:
       - result is not changed
@@ -587,6 +667,22 @@
       - result[iam_object_key] == iam_name
       - iam_policy_info.policies[0].policy_document.Id == 'MyId'
 
+  - name: 'Update policy using document for {{ iam_type }} (idempotency - check mode) (skip_duplicates = no)'
+    iam_policy:
+      state: present
+      skip_duplicates: no
+      iam_type: '{{ iam_type }}'
+      iam_name: '{{ iam_name }}'
+      policy_name: '{{ iam_policy_name_a }}'
+      policy_document: '{{ tmpdir.path }}/no_access_with_id.json'
+    register: result
+    check_mode: yes
+
+  - name: 'Assert no change would occur'
+    assert:
+      that:
+      - result is not changed
+
   - name: 'Update policy using document for {{ iam_type }} (idempotency) (skip_duplicates = no)'
     iam_policy:
       state: present
@@ -655,7 +751,7 @@
       policy_name: '{{ iam_policy_name_c }}'
     register: iam_policy_info
 
-  - name: 'Assert policy would be added for {{ iam_type }}'
+  - name: 'Assert policy would not be added for {{ iam_type }}'
     assert:
       that:
       - result is not changed
@@ -731,6 +827,22 @@
       - result[iam_object_key] == iam_name
       - '"Id" not in iam_policy_info.policies[0].policy_document'
 
+  - name: 'Update policy using json for {{ iam_type }} (idempotency - check mode) (skip_duplicates = no)'
+    iam_policy:
+      state: present
+      skip_duplicates: no
+      iam_type: '{{ iam_type }}'
+      iam_name: '{{ iam_name }}'
+      policy_name: '{{ iam_policy_name_c }}'
+      policy_json: '{{ lookup("file", "{{ tmpdir.path }}/no_access.json") }}'
+    register: result
+    check_mode: yes
+
+  - name: 'Assert no change would occur'
+    assert:
+      that:
+      - result is not changed
+
   - name: 'Update policy using json for {{ iam_type }} (idempotency) (skip_duplicates = no)'
     iam_policy:
       state: present
@@ -824,6 +936,21 @@
       - result[iam_object_key] == iam_name
       - iam_policy_info.policies[0].policy_document.Id == 'MyOtherId'
 
+  - name: 'Update policy using document for {{ iam_type }} (idempotency - check mode)'
+    iam_policy:
+      state: present
+      iam_type: '{{ iam_type }}'
+      iam_name: '{{ iam_name }}'
+      policy_name: '{{ iam_policy_name_b }}'
+      policy_document: '{{ tmpdir.path }}/no_access_with_second_id.json'
+    register: result
+    check_mode: yes
+
+  - name: 'Assert no change would occur'
+    assert:
+      that:
+      - result is not changed
+
   - name: 'Update policy using document for {{ iam_type }} (idempotency)'
     iam_policy:
       state: present
@@ -916,6 +1043,22 @@
       - result[iam_object_key] == iam_name
       - iam_policy_info.policies[0].policy_document.Id == 'MyOtherId'
 
+  - name: 'Update policy using json for {{ iam_type }} (idempotency - check mode)'
+    iam_policy:
+      state: present
+      skip_duplicates: no
+      iam_type: '{{ iam_type }}'
+      iam_name: '{{ iam_name }}'
+      policy_name: '{{ iam_policy_name_d }}'
+      policy_json: '{{ lookup("file", "{{ tmpdir.path }}/no_access_with_second_id.json") }}'
+    register: result
+    check_mode: yes
+
+  - name: 'Assert no change would occur'
+    assert:
+      that:
+      - result is not changed
+
   - name: 'Update policy using json for {{ iam_type }} (idempotency)'
     iam_policy:
       state: present