aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.62k stars 3.91k forks source link

feat(aws-cdk-lib/aws-kms): Allow testing for the existence of a KMS key by alias name #31574

Closed neoakris closed 1 week ago

neoakris commented 4 weeks ago

Describe the feature

Here's a code snippet for some context:

import * as kms from 'aws-cdk-lib/aws-kms';
let kmsKey = kms.Key.fromLookup(stack, "pre-existing-kms-key", { aliasName: "alias/eks/test" })

Current State of CDK 2.133.0:

Feature Request/Proposed Solution:

Use Case

While using EKS Blueprints, every time I delete and recreate a cluster (semi-frequently for ephemeral sandbox / test environments), due to its defaults it'll create a new KMS key each time, then on delete I get orphaned kms keys. EKS Blueprints allows passing in a pre-existing kms key to avoid this.

However I'd like to add logic for the following use case:

  1. specify a desired kms alias
  2. detect if it exists
  3. if not exist: create new key with kms alias, and use the newly created kms key.
  4. if exists: use kms key with alias.

Basically I wanted to be able to create a function named ensure_existence_of_kms_key_with_alias()

This will allow me to implement logic where when I create a cluster it'll create a kms key, but if I delete and recreate the cluster it'll reuse the original kms key. (This avoids orphaned keys, and is also advantageous in the scenario of if disk backups were ever created using a kms key, then restore might be easier if a consistent kms key were used.)

Other Information

The proposed feature/solution shouldn't be a breaking change.

Acknowledgements

CDK version used

2.133.0 (build dcc1e75)

Environment details (OS name and version, etc.)

MacOS Sonoma 14.6.1

neoakris commented 4 weeks ago

If anyone else runs into a similar scenario The following is a snippet of a temporary workaround I'm considering/might use.

import { execSync } from 'child_process'; //work around kms UX issue
import console = require('console'); //can help debug feedback loop

const alias_key_to_lookup = "alias/eks/test";
const cmd = `aws kms list-aliases | jq '.Aliases[] | select(.AliasName == "${alias_key_to_lookup}") | .TargetKeyId'`
const cmd_results = execSync(cmd).toString();
console.log(cmd_results)
//TO DO: never pass un-sanitized input / would need tight input sanitization.

cmd_results = "" if not found cmd_results = "ac97959f-c5fb-4de0-a584-2d741c05ea0b" if found ^-- basically a more graceful failure that's a lot easier to handle with if statement checks.

khushail commented 4 weeks ago

Hi @neoakris , thanks for requesting and sharing the workaround as well. I think this can be achieved using a flag type variable, say, KeyAliasExists(), which would return false if its not existing. Your usecase makes sense to me.

However if you would like to contribute, please feel free to follow our contribution guide. I would be marking this feature request as P3 for keeping it open for community contribution as well. Thanks.

neoakris commented 4 weeks ago

You're right a flag makes more sense, it'd be just as effective as a string comparison, yet cleaner.

Thanks for the triage and suggestions.

go-to-k commented 2 weeks ago

Hi @khushail @neoakris ,

I have submitted the PR, where the fromLookup method returns a dummy key if the new option returnDummyKeyOnMissing is set to true for now.

The reasons for this are:

Allowing undefined in the return value of fromLookup is a breaking change, so I decided not to do that. I also considered allowing a dummy value to be freely specified (like the SSM Parameter case), but decided against that too as I didn't think there would be many use cases for it.

Of course, we can create a new method that is separate from the fromLookup method, such as Key.keyAliasExists(). However, given the design philosophy of the original code, I thought it would be better to fix it to the correct implementation of fromLookup.

Please let me know if you have any feedback.

neoakris commented 2 weeks ago

@go-to-k my understanding of what you've said is once implemented/in a live release, if I specify the returnDummyKeyOnMissing option. I'll be able to do an if statement check against 1234abcd-12ab-34cd-56ef-1234567890ab when the key isn't pre-existing that works for me.

Thanks for the update!

go-to-k commented 2 weeks ago

That's right!

khushail commented 2 weeks ago

@go-to-k , your implementation with the existing design is very thoughful. Thanks for your efforts in submitting a PR!

go-to-k commented 2 weeks ago

@khushail Thank you! The pr/needs-cli-test-run label has been attached, what should I do? If I just wait, will the maintainer run the test for me?

khushail commented 2 weeks ago

@go-to-k , I am not really sure why this label is attached. You could ask in the PR from the maintainer.

github-actions[bot] commented 1 week ago

Comments on closed issues and PRs are hard for our team to see. If you need help, please open a new issue that references this one.

github-actions[bot] commented 1 week ago

Comments on closed issues and PRs are hard for our team to see. If you need help, please open a new issue that references this one.