kms_key - integration test race condition (ansible-collections#1886)

kms_key - integration test race condition SUMMARY Because kms_key doesn't have any waiters we sometimes hit the race condition with the deletion test that we return before the service fully registers that a deletion is in progress. This change uses kms_key_info to check that the deletion and relevant dates are correctly set after a short pause to allow propagation. ISSUE TYPE Bugfix Pull Request COMPONENT NAME kms_key ADDITIONAL INFORMATION Reviewed-by: Alina Buzachis
mandar242 · Dec 12, 2023 · a530e32 · a530e32
1 parent c0e2ab6
commit a530e32
Show file tree

Hide file tree

Showing 10 changed files with 183 additions and 106 deletions.
diff --git a/tests/integration/targets/kms_key/main.yml b/tests/integration/targets/kms_key/main.yml
@@ -3,8 +3,9 @@
 # To add new tests you'll need to add a new host to the inventory and a matching
 # '{{ inventory_hostname }}'.yml file in roles/aws_kms/tasks/
 
-- hosts: all
+- name: Run integrationtests for kms_key in parallel
+  hosts: all
   gather_facts: false
-  strategy: free
+  strategy: ansible.builtin.free # noqa: run-once[play]
   roles:
-    - aws_kms
+    - kms_key
diff --git a/...s/kms_key/roles/aws_kms/defaults/main.yml → ...s/kms_key/roles/kms_key/defaults/main.yml b/...s/kms_key/roles/aws_kms/defaults/main.yml → ...s/kms_key/roles/kms_key/defaults/main.yml
@@ -1,2 +1,2 @@
 ---
-kms_key_alias: ansible-test-{{ inventory_hostname | replace('_','-') }}{{ tiny_prefix }}
+kms_key_alias: ansible-test-{{ inventory_hostname | replace('_', '-') }}{{ tiny_prefix }}
diff --git a/...gets/kms_key/roles/aws_kms/tasks/main.yml → ...gets/kms_key/roles/kms_key/tasks/main.yml b/...gets/kms_key/roles/aws_kms/tasks/main.yml → ...gets/kms_key/roles/kms_key/tasks/main.yml
@@ -1,5 +1,5 @@
 ---
-- name: kms_key integration tests
+- name: Integration tests for kms_key
   collections:
     - community.aws
   module_defaults:
@@ -9,4 +9,5 @@
       session_token: "{{ security_token | default(omit) }}"
       region: "{{ aws_region }}"
   block:
-    - ansible.builtin.include_tasks: ./test_{{ inventory_hostname }}.yml
+    - name: Run test suite
+      ansible.builtin.include_tasks: ./test_{{ inventory_hostname }}.yml
diff --git a/...s_key/roles/aws_kms/tasks/test_grants.yml → ...s_key/roles/kms_key/tasks/test_grants.yml b/...s_key/roles/aws_kms/tasks/test_grants.yml → ...s_key/roles/kms_key/tasks/test_grants.yml
@@ -1,21 +1,27 @@
 ---
-- block:
+- name: Run tests related to grants
+  block:
     # ============================================================
     #   PREPARATION
     #
     # Get some information about who we are before starting our tests
     # we'll need this as soon as we start working on the policies
-    - name: get ARN of calling user
+    - name: Get ARN of calling user
       amazon.aws.aws_caller_info:
       register: aws_caller_info
-    - name: create an IAM role that can do nothing
+    - name: Create an IAM role that can do nothing
       community.aws.iam_role:
         name: "{{ kms_key_alias }}"
         state: present
-        assume_role_policy_document: '{"Version": "2012-10-17", "Statement": {"Action": "sts:AssumeRole", "Principal": {"Service": "ec2.amazonaws.com"}, "Effect":
-          "Deny"} }'
+        assume_role_policy_document:
+          Version: "2012-10-17"
+          Statement:
+            Action: "sts:AssumeRole"
+            Principal:
+              Service: "ec2.amazonaws.com"
+            Effect: "Deny"
       register: iam_role_result
-    - name: create a key
+    - name: Create a key
       amazon.aws.kms_key:
         alias: "{{ kms_key_alias }}"
         tags:
@@ -24,7 +30,7 @@
         enabled: true
         enable_key_rotation: false
       register: key
-    - name: assert that state is enabled
+    - name: Assert that state is enabled
       ansible.builtin.assert:
         that:
           - key is changed
@@ -66,13 +72,14 @@
               - RetireGrant
       register: key
       check_mode: true
-    - name: assert grant would have been added
+    - name: Assert grant would have been added
       ansible.builtin.assert:
         that:
           - key.changed
 
     # Roles can take a little while to get ready, pause briefly to give it chance
-    - ansible.builtin.wait_for:
+    - name: Pause for role creation to fully propagate
+      ansible.builtin.wait_for:
         timeout: 20
     - name: Add grant
       amazon.aws.kms_key:
@@ -91,7 +98,7 @@
               - Decrypt
               - RetireGrant
       register: key
-    - name: assert grant added
+    - name: Assert grant added
       ansible.builtin.assert:
         that:
           - key.changed
@@ -134,7 +141,8 @@
               - RetireGrant
       register: key
       check_mode: true
-    - ansible.builtin.assert:
+    - name: Assert no changes expected
+      ansible.builtin.assert:
         that:
           - not key.changed
 
@@ -155,7 +163,8 @@
               - Decrypt
               - RetireGrant
       register: key
-    - ansible.builtin.assert:
+    - name: Assert no changes made
+      ansible.builtin.assert:
         that:
           - not key.changed
           - '"key_id" in key'
@@ -337,14 +346,14 @@
   always:
     # ============================================================
     #   CLEAN-UP
-    - name: finish off by deleting keys
+    - name: Finish off by deleting keys
       amazon.aws.kms_key:
         state: absent
         alias: "{{ kms_key_alias }}"
         pending_window: 7
-      ignore_errors: true
-    - name: remove the IAM role
+      ignore_errors: true # noqa: ignore-errors
+    - name: Remove the IAM role
       community.aws.iam_role:
         name: "{{ kms_key_alias }}"
         state: absent
-      ignore_errors: true
+      ignore_errors: true # noqa: ignore-errors
diff --git a/...s_key/roles/aws_kms/tasks/test_modify.yml → ...s_key/roles/kms_key/tasks/test_modify.yml b/...s_key/roles/aws_kms/tasks/test_modify.yml → ...s_key/roles/kms_key/tasks/test_modify.yml
@@ -1,21 +1,27 @@
 ---
-- block:
+- name: Run tests related to basic key modification
+  block:
     # ============================================================
     #   PREPARATION
     #
     # Get some information about who we are before starting our tests
     # we'll need this as soon as we start working on the policies
-    - name: get ARN of calling user
+    - name: Get ARN of calling user
       amazon.aws.aws_caller_info:
       register: aws_caller_info
-    - name: create an IAM role that can do nothing
+    - name: Create an IAM role that can do nothing
       community.aws.iam_role:
         name: "{{ kms_key_alias }}"
         state: present
-        assume_role_policy_document: '{"Version": "2012-10-17", "Statement": {"Action": "sts:AssumeRole", "Principal": {"Service": "ec2.amazonaws.com"}, "Effect":
-          "Deny"} }'
+        assume_role_policy_document:
+          Version: "2012-10-17"
+          Statement:
+            Action: "sts:AssumeRole"
+            Principal:
+              Service: "ec2.amazonaws.com"
+            Effect: "Deny"
       register: iam_role_result
-    - name: create a key
+    - name: Create a key
       amazon.aws.kms_key:
         alias: "{{ kms_key_alias }}"
         tags:
@@ -24,7 +30,7 @@
         enabled: true
         enable_key_rotation: false
       register: key
-    - name: assert that state is enabled
+    - name: Assert that state is enabled
       ansible.builtin.assert:
         that:
           - key is changed
@@ -52,11 +58,11 @@
       ansible.builtin.set_fact:
         kms_key_id: "{{ key.key_id }}"
         kms_key_arn: "{{ key.key_arn }}"
-    - name: find facts about the key (by ID)
+    - name: Find facts about the key (by ID)
       amazon.aws.kms_key_info:
         key_id: "{{ kms_key_id }}"
       register: new_key
-    - name: check that a key was found
+    - name: Check that a key was found
       ansible.builtin.assert:
         that:
           - '"key_id" in new_key.kms_keys[0]'
@@ -83,7 +89,8 @@
         policy: "{{ lookup('template', 'console-policy.j2') }}"
       register: key
       check_mode: true
-    - ansible.builtin.assert:
+    - name: Assert that change is expected
+      ansible.builtin.assert:
         that:
           - key is changed
 
@@ -123,7 +130,8 @@
         policy: "{{ lookup('template', 'console-policy.j2') }}"
       register: key
       check_mode: true
-    - ansible.builtin.assert:
+    - name: Assert no change expected
+      ansible.builtin.assert:
         that:
           - not key.changed
 
@@ -132,7 +140,8 @@
         alias: alias/{{ kms_key_alias }}
         policy: "{{ lookup('template', 'console-policy.j2') }}"
       register: key
-    - ansible.builtin.assert:
+    - name: Assert that no changes occurred
+      ansible.builtin.assert:
         that:
           - not key.changed
           - '"key_id" in key'
@@ -162,7 +171,8 @@
         description: test key for testing
       register: key
       check_mode: true
-    - ansible.builtin.assert:
+    - name: Assert change expected
+      ansible.builtin.assert:
         that:
           - key.changed
 
@@ -172,7 +182,8 @@
         state: present
         description: test key for testing
       register: key
-    - ansible.builtin.assert:
+    - name: Assert that description changed
+      ansible.builtin.assert:
         that:
           - key.changed
           - '"key_id" in key'
@@ -203,7 +214,8 @@
         description: test key for testing
       register: key
       check_mode: true
-    - ansible.builtin.assert:
+    - name: Assert that no change was expected
+      ansible.builtin.assert:
         that:
           - not key.changed
 
@@ -213,7 +225,8 @@
         state: present
         description: test key for testing
       register: key
-    - ansible.builtin.assert:
+    - name: Assert no change occurred
+      ansible.builtin.assert:
         that:
           - not key.changed
           - '"key_id" in key'
@@ -236,12 +249,13 @@
 
     # ------------------------------------------------------------------------------------------
 
-    - name: update policy to remove access to key rotation status
+    - name: Update policy to remove access to key rotation status
       amazon.aws.kms_key:
         alias: alias/{{ kms_key_alias }}
         policy: "{{ lookup('template', 'console-policy-no-key-rotation.j2') }}"
       register: key
-    - ansible.builtin.assert:
+    - name: Assert that policy was updated
+      ansible.builtin.assert:
         that:
           - '"key_id" in key'
           - key.key_id | length >= 36
@@ -265,14 +279,14 @@
   always:
     # ============================================================
     #   CLEAN-UP
-    - name: finish off by deleting keys
+    - name: Finish off by deleting keys
       amazon.aws.kms_key:
         state: absent
         alias: "{{ kms_key_alias }}"
         pending_window: 7
-      ignore_errors: true
-    - name: remove the IAM role
+      ignore_errors: true # noqa: ignore-errors
+    - name: Remove the IAM role
       community.aws.iam_role:
         name: "{{ kms_key_alias }}"
         state: absent
-      ignore_errors: true
+      ignore_errors: true # noqa: ignore-errors
diff --git a/...roles/aws_kms/tasks/test_multi_region.yml → ...roles/kms_key/tasks/test_multi_region.yml b/...roles/aws_kms/tasks/test_multi_region.yml → ...roles/kms_key/tasks/test_multi_region.yml
@@ -1,17 +1,18 @@
 ---
-- block:
+- name: Run tests related to multi-region keys
+  block:
     # ============================================================
     #   PREPARATION
     #
     # Get some information about who we are before starting our tests
     # we'll need this as soon as we start working on the policies
-    - name: get ARN of calling user
+    - name: Get ARN of calling user
       amazon.aws.aws_caller_info:
       register: aws_caller_info
     - name: See whether key exists and its current state
       amazon.aws.kms_key_info:
         alias: "{{ kms_key_alias }}"
-    - name: create a multi region key - check mode
+    - name: Create a multi region key - check mode
       amazon.aws.kms_key:
         alias: "{{ kms_key_alias }}-check"
         tags:
@@ -21,17 +22,17 @@
         enabled: true
       register: key_check
       check_mode: true
-    - name: find facts about the check mode key
+    - name: Find facts about the check mode key
       amazon.aws.kms_key_info:
         alias: "{{ kms_key_alias }}-check"
       register: check_key
-    - name: ensure that check mode worked as expected
+    - name: Ensure that check mode worked as expected
       ansible.builtin.assert:
         that:
           - check_key.kms_keys | length == 0
           - key_check is changed
 
-    - name: create a multi region key
+    - name: Create a multi region key
       amazon.aws.kms_key:
         alias: "{{ kms_key_alias }}"
         tags:
@@ -41,7 +42,7 @@
         multi_region: true
         enable_key_rotation: false
       register: key
-    - name: assert that state is enabled
+    - name: Assert that state is enabled
       ansible.builtin.assert:
         that:
           - key is changed
@@ -68,7 +69,7 @@
       ansible.builtin.wait_for:
         timeout: 45
 
-    - name: create a key (expect failure)
+    - name: Create a key (expect failure)
       amazon.aws.kms_key:
         alias: "{{ kms_key_alias }}"
         tags:
@@ -77,9 +78,10 @@
         enabled: true
         multi_region: true
       register: result
-      ignore_errors: true
+      ignore_errors: true # noqa: ignore-errors
 
-    - ansible.builtin.assert:
+    - name: Assert that we failed with a friendly message
+      ansible.builtin.assert:
         that:
           - result is failed
           - result.msg != "MODULE FAILURE"
@@ -89,12 +91,12 @@
   always:
     # ============================================================
     #   CLEAN-UP
-    - name: finish off by deleting keys
+    - name: Finish off by deleting keys
       amazon.aws.kms_key:
         state: absent
         alias: "{{ item }}"
         pending_window: 7
-      ignore_errors: true
+      ignore_errors: true # noqa: ignore-errors
       loop:
         - "{{ kms_key_alias }}"
         - "{{ kms_key_alias }}-diff-spec-usage"