feat(kms): allow fromLookup method to return dummy key if target key was not found

[go-to-k]

Issue # (if applicable)

Closes #31574.

Reason for this change

The fromLookup method causes an error if the target key was not found. However it would be also good not to cause an error in that case.

Description of changes

Added returnDummyKeyOnMissing in KeyLookupOptions. If the property is set to true, the context method will not cause an error and will return a dummy key if the key was not found.

Originally, I thought to make the method to return undefined in that case, but the return type of method is IKey. If we change the type to IKey | undefined, it should be a breaking change.

  public static fromLookup(scope: Construct, id: string, options: KeyLookupOptions): IKey {

So I decided to return a dummy key with a dummy key id '1234abcd-12ab-34cd-56ef-1234567890ab'. The dummy key id had been defined originally (see: https://github.com/aws/aws-cdk/blob/v2.161.0/packages/aws-cdk-lib/aws-kms/lib/key.ts#L686).

The property returnDummyKeyOnMissing will be passed to ignoreErrorOnMissingContext added in the PR. If the ignoreErrorOnMissingContext is true and the key doesn't exist, an error will be suppressed in the ContextProvider.

Additional information

see: #31574 (comment)

Description of how you validated changes

Both of unit and integ tests

Checklist

• My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[aws-cdk-automation]

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

[go-to-k]

Exemption Request: The code changes in Context Providers only affect the KMS context (lookup) method, they could be covered by unit tests for context-providers and integ tests for KMS module.

[laurelmay]

Is there a more user-friendly way to allow detecting dummy results? Maybe the 1234abcd-12ab-34cd-56ef-1234567890ab value can be exposed as a constant from the KMS package (something like kms.Key.DUMMY_KEY_ID)? Or potentially even more friendly would be a method like:

public get isLookupDummy() {
  return this.keyId === Key.DEFAULT_DUMMY_KEY_ID;
}

And then in the future if the implementation were to change to, for example, allow more dynamic dummy values (such as what the SSM parameter does) that could be done without a breaking API change.

[go-to-k]

That's a good idea! I will add a new static variable DEFAULT_DUMMY_KEY_ID to the Key class.

However, as this dummy id is used by an imported key (IKey), the isLookupDummy method needs to be added to the interface as well as the KeyBase. But it would be a breaking change. This is because users who implement their own constructs using the interface will get an error.

[go-to-k]

The following approach is possible, but is this necessary enough to make it a static method...?

export class Key extends KeyBase {
  // ...
  // ...
  public static isLookupDummy(key: IKey): boolean {
    return key.keyId === Key.DEFAULT_DUMMY_KEY_ID;
  }

I didn't add this for now, just added DEFAULT_DUMMY_KEY_ID:

9e8a164

I set the variable name to "DEFAULT_"DUMMY_KEY_ID in anticipation of changing the dummy key ID in the future. If I set it to DUMMY_KEY_ID, I thought we would be in trouble if we made it possible to change the dummy key ID in the future. Please let me know if you feel different.

[laurelmay]

Oh good call on the interface issue!

I do in a way like the static method. It's almost reminiscent of, for example, iam.Role.isRole. I could envision that in the future (hypothetically, it'd be possible to someday detect this instead in fromLookup and return a class other than Import) the body would instead read something like:

public static isLookupDummy(key: IKey): boolean {
  return LOOKUP_DUMMY_SYMBOL in key;
}

In this case, the method actually allows us to make that change (otherwise, we'd be locked to the documented keyId-based approach).

This non-failing dummy result for context lookups just feels a bit new overall and I think unlike the SSM parameter situation, detecting this case is actually pretty impactful. We may want to give ourselves some flexibility via abstractions.

But then again, I see and appreciate how simple and straightforward DEFAULT_DUMMY_KEY_ID is; so I am not going to withhold an approval over this. If you feel that the static method may actually be too much abstraction for now, I'll happily go ahead and add my approval 😄

[go-to-k]

In this case, the method actually allows us to make that change (otherwise, we'd be locked to the documented keyId-based approach).

Yeah, agree with you.

I have changed, could you please take a look at it?

[laurelmay]

Thanks! I like that! I appreciate you making the adjustments.

[GavinZZ]

Nicely written, appreciate it!

[GavinZZ]

I have kicked off the build to run in the test pipeline. Will update the labels once the build finishes running.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: 6bb2351
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[github-actions]

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.