aws_kms - stabilize and add integration tests (#1052)

aws_kms - stabilize and add integration tests SUMMARY update/add integration tests for various actions return list of policies as a list of jsons for clarity sleep on updates (no kms waiter, attempted manual waiters but still had test failures) ISSUE TYPE Feature Pull Request COMPONENT NAME aws_kms ADDITIONAL INFORMATION I tried adding manual waiters for different actions like waiting for tags to be correct, policy to be updated, etc, but would still fail ~half of the time on idempotency tests. seems like after updating the key's status is a bit buggy. Reviewed-by: Jill R <None> Reviewed-by: Mark Chappell <None> Reviewed-by: Joseph Torcasso <None> Reviewed-by: Mandar Kulkarni <mandar242@gmail.com> Reviewed-by: Markus Bergholz <git@osuv.de> Reviewed-by: Alina Buzachis <None> (cherry picked from commit 558a025)
ansible-collections · Apr 26, 2022 · 936049d · 936049d
1 parent e041fa9
commit 936049d
Show file tree

Hide file tree

Showing 7 changed files with 635 additions and 143 deletions.
diff --git a/changelogs/fragments/1052-aws_kms-stabilize-integration-tests.yml b/changelogs/fragments/1052-aws_kms-stabilize-integration-tests.yml
@@ -0,0 +1,3 @@
+minor_changes:
+  - aws_kms - fix some bugs in integration tests and add check mode support for key rotation as well as document issues with time taken for requested changes to be reflected on AWS (https://github.com/ansible-collections/community.aws/pull/1052).
+  - aws_kms - add extra key/value pair to return data (key_policies) to return each policy as a dictionary rather than json string (https://github.com/ansible-collections/community.aws/pull/1052).
diff --git a/plugins/modules/aws_kms.py b/plugins/modules/aws_kms.py
@@ -199,6 +199,12 @@
 - amazon.aws.aws
 - amazon.aws.ec2
 
+
+notes:
+  - There are known inconsistencies in the amount of time required for updates of KMS keys to be fully reflected on AWS.
+    This can cause issues when running duplicate tasks in succession or using the aws_kms_info module to fetch key metadata
+    shortly after modifying keys.
+    For this reason, it is recommended to use the return data from this module (aws_kms) to fetch a key's metadata.
 '''
 
 EXAMPLES = r'''
@@ -310,6 +316,11 @@
   type: str
   returned: always
   sample: false
+enable_key_rotation:
+  description: Whether the automatic annual key rotation is enabled. Returns None if key rotation status can't be determined.
+  type: bool
+  returned: always
+  sample: false
 aliases:
   description: list of aliases associated with the key
   type: list
@@ -318,9 +329,45 @@
     - aws/acm
     - aws/ebs
 policies:
-  description: list of policy documents for the keys. Empty when access is denied even if there are policies.
+  description: list of policy documents for the key. Empty when access is denied even if there are policies.
   type: list
   returned: always
+  elements: str
+  sample:
+    Version: "2012-10-17"
+    Id: "auto-ebs-2"
+    Statement:
+    - Sid: "Allow access through EBS for all principals in the account that are authorized to use EBS"
+      Effect: "Allow"
+      Principal:
+      AWS: "*"
+      Action:
+      - "kms:Encrypt"
+      - "kms:Decrypt"
+      - "kms:ReEncrypt*"
+      - "kms:GenerateDataKey*"
+      - "kms:CreateGrant"
+      - "kms:DescribeKey"
+      Resource: "*"
+      Condition:
+        StringEquals:
+          kms:CallerAccount: "111111111111"
+          kms:ViaService: "ec2.ap-southeast-2.amazonaws.com"
+    - Sid: "Allow direct access to key metadata to the account"
+      Effect: "Allow"
+      Principal:
+      AWS: "arn:aws:iam::111111111111:root"
+      Action:
+      - "kms:Describe*"
+      - "kms:Get*"
+      - "kms:List*"
+      - "kms:RevokeGrant"
+      Resource: "*"
+key_policies:
+  description: list of policy documents for the key. Empty when access is denied even if there are policies.
+  type: list
+  returned: always
+  elements: dict
   sample:
     Version: "2012-10-17"
     Id: "auto-ebs-2"
@@ -351,6 +398,7 @@
       - "kms:List*"
       - "kms:RevokeGrant"
       Resource: "*"
+  version_added: 3.3.0
 tags:
   description: dictionary of tags applied to the key
   type: dict
@@ -584,6 +632,7 @@ def get_key_details(connection, module, key_id):
     tags = get_kms_tags(connection, module, key_id)
     result['tags'] = boto3_tag_list_to_ansible_dict(tags, 'TagKey', 'TagValue')
     result['policies'] = get_kms_policies(connection, module, key_id)
+    result['key_policies'] = [json.loads(policy) for policy in result['policies']]
     return result
 
 
@@ -817,13 +866,15 @@ def update_key_rotation(connection, module, key, enable_key_rotation):
     except (botocore.exceptions.ClientError, botocore.exceptions.BotoCoreError) as e:  # pylint: disable=duplicate-except
         module.fail_json_aws(e, msg="Unable to get current key rotation status")
 
-    try:
-        if enable_key_rotation:
-            connection.enable_key_rotation(KeyId=key_id)
-        else:
-            connection.disable_key_rotation(KeyId=key_id)
-    except (botocore.exceptions.ClientError, botocore.exceptions.BotoCoreError) as e:
-        module.fail_json_aws(e, msg="Failed to enable/disable key rotation")
+    if not module.check_mode:
+        try:
+            if enable_key_rotation:
+                connection.enable_key_rotation(KeyId=key_id)
+            else:
+                connection.disable_key_rotation(KeyId=key_id)
+        except (botocore.exceptions.ClientError, botocore.exceptions.BotoCoreError) as e:
+            module.fail_json_aws(e, msg="Failed to enable/disable key rotation")
+
     return True
 
 
@@ -1030,6 +1081,11 @@ def canonicalize_alias_name(alias):
 
 
 def fetch_key_metadata(connection, module, key_id, alias):
+    # Note - fetching a key's metadata is very inconsistent shortly after any sort of update to a key has occurred.
+    # Combinations of manual waiters, checking expecting key values to actual key value, and static sleeps
+    #        have all been exhausted, but none of those available options have solved the problem.
+    # Integration tests will wait for 10 seconds to combat this issue.
+    # See https://github.com/ansible-collections/community.aws/pull/1052.
 
     alias = canonicalize_alias_name(module.params.get('alias'))
 
@@ -1104,10 +1160,13 @@ def main():
 
     kms = module.client('kms')
 
+    module.deprecate("The 'policies' return key is deprecated and will be replaced by 'key_policies'. Both values are returned for now.",
+                     date='2024-05-01', collection_name='community.aws')
+
     key_metadata = fetch_key_metadata(kms, module, module.params.get('key_id'), module.params.get('alias'))
     # We can't create keys with a specific ID, if we can't access the key we'll have to fail
     if module.params.get('state') == 'present' and module.params.get('key_id') and not key_metadata:
-        module.fail_json(msg="Could not find key with id %s to update")
+        module.fail_json(msg="Could not find key with id {0} to update".format(module.params.get('key_id')))
 
     if module.params.get('policy_grant_types') or mode == 'deny':
         module.deprecate('Managing the KMS IAM Policy via policy_mode and policy_grant_types is fragile'

diff --git a/plugins/modules/aws_kms_info.py b/plugins/modules/aws_kms_info.py
@@ -150,9 +150,10 @@
         Name: myKey
         Purpose: protecting_stuff
     policies:
-      description: list of policy documents for the keys. Empty when access is denied even if there are policies.
+      description: list of policy documents for the key. Empty when access is denied even if there are policies.
       type: list
       returned: always
+      elements: str
       sample:
         Version: "2012-10-17"
         Id: "auto-ebs-2"
@@ -183,6 +184,42 @@
           - "kms:List*"
           - "kms:RevokeGrant"
           Resource: "*"
+    key_policies:
+      description: list of policy documents for the key. Empty when access is denied even if there are policies.
+      type: list
+      returned: always
+      elements: dict
+      sample:
+        Version: "2012-10-17"
+        Id: "auto-ebs-2"
+        Statement:
+        - Sid: "Allow access through EBS for all principals in the account that are authorized to use EBS"
+          Effect: "Allow"
+          Principal:
+            AWS: "*"
+          Action:
+          - "kms:Encrypt"
+          - "kms:Decrypt"
+          - "kms:ReEncrypt*"
+          - "kms:GenerateDataKey*"
+          - "kms:CreateGrant"
+          - "kms:DescribeKey"
+          Resource: "*"
+          Condition:
+            StringEquals:
+              kms:CallerAccount: "111111111111"
+              kms:ViaService: "ec2.ap-southeast-2.amazonaws.com"
+        - Sid: "Allow direct access to key metadata to the account"
+          Effect: "Allow"
+          Principal:
+            AWS: "arn:aws:iam::111111111111:root"
+          Action:
+          - "kms:Describe*"
+          - "kms:Get*"
+          - "kms:List*"
+          - "kms:RevokeGrant"
+          Resource: "*"
+      version_added: 3.3.0
     grants:
       description: list of grants associated with a key
       type: complex
@@ -240,6 +277,7 @@
           sample: arn:aws:sts::0123456789012:assumed-role/lambda_xyz/xyz
 '''
 
+import json
 
 try:
     import botocore
@@ -418,6 +456,7 @@ def get_key_details(connection, module, key_id, tokens=None):
     result = camel_dict_to_snake_dict(result)
     result['tags'] = boto3_tag_list_to_ansible_dict(tags, 'TagKey', 'TagValue')
     result['policies'] = get_kms_policies(connection, module, key_id)
+    result['key_policies'] = [json.loads(policy) for policy in result['policies']]
     return result
 
 
@@ -460,6 +499,9 @@ def main():
     except (botocore.exceptions.ClientError, botocore.exceptions.BotoCoreError) as e:
         module.fail_json_aws(e, msg='Failed to connect to AWS')
 
+    module.deprecate("The 'policies' return key is deprecated and will be replaced by 'key_policies'. Both values are returned for now.",
+                     date='2024-05-01', collection_name='community.aws')
+
     all_keys = get_kms_info(connection, module)
     filtered_keys = [key for key in all_keys if key_matches_filters(key, module.params['filters'])]
     ret_params = dict(kms_keys=filtered_keys)

diff --git a/tests/integration/targets/aws_kms/aliases b/tests/integration/targets/aws_kms/aliases
@@ -1,6 +1,7 @@
 # Various race conditions - likely needs waiters
 # https://github.com/ansible-collections/community.aws/issues/433
-unstable
+# No KMS supported waiters, and manual waiting for updates didn't fix the issue either.
+# Issue likely from AWS side - added waits on updates in integration tests to workaround this.
 
 cloud/aws
 

diff --git a/tests/integration/targets/aws_kms/defaults/main.yml b/tests/integration/targets/aws_kms/defaults/main.yml
@@ -1,3 +1,2 @@
 ---
-kms_role_name: 'ansible-test-{{ resource_prefix }}-kms'
-kms_key_alias: '{{ resource_prefix }}-kms'
+kms_key_alias: 'ansible-test-{{ resource_prefix }}-kms'