fix(events): omit KmsKeyIdentifier from Archive when no KMS key is configured - #37865
fix(events): omit KmsKeyIdentifier from Archive when no KMS key is configured#37865Zelys-DFKH wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
The pull request linter fails with the following errors:
❌ Fixes must contain a change to an integration test file and the resulting snapshot.
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.
✅ A exemption request has been requested. Please wait for a maintainer's review.
|
Two CI jobs show failures that are unrelated to the code change:
Neither is required for merge. Happy to investigate further if helpful. |
|
Exemption Request The integ test requirement makes total sense for changes that affect real AWS resource behavior. This one is different. The bug is that The fix is a one-line If the team thinks a deployment test is warranted here, I'll add one. Just checking first. |
…key is configured When no kmsKey is provided to the Archive construct, the L2 was emitting KmsKeyIdentifier: "" in the synthesized CloudFormation template. The empty string causes GeneralServiceException failures in some AWS regions that reject it as an invalid value (the property must either hold a valid KMS key ARN or be absent entirely). The fix removes the || '' fallback so the property is omitted when kmsKeyIdentifier resolves to undefined, consistent with how the EventBus L2 handles the same property. The test that previously asserted KmsKeyIdentifier: '' has been corrected to assert Match.absent(), and the misleading comment explaining the empty string as intentional has been removed. Fixes aws#37847
e22058d to
c39966b
Compare
|
CI is red on one integration test snapshot. Dropping the empty KmsKeyIdentifier changes the synthesized template, so integ.archive comes back CHANGED. I have not run cdk-integ with --dry-run or edited the snapshot by hand, since CONTRIBUTING asks contributors not to. Could a maintainer run the integration test so it gets regenerated from a real deployment? |
The fix stops emitting an empty KmsKeyIdentifier when no KMS key is configured, so the Archive resource in the snapshot loses that property. Regenerated integ.archive.
…omit-kms-when-unset
…-unset' into fix/events-archive-omit-kms-when-unset
Issue # (if applicable)
Closes #37847.
Credit to @pahud, who did the full investigation here — the root cause and fix direction are his. This is the implementation.
Reason for this change
When
Archiveis created without akmsKeyprop, the L2 construct synthesizesKmsKeyIdentifier: ""into the CloudFormation template. An empty string is not a valid value forKmsKeyIdentifier: when no CMK is configured, the property should be absent so CloudFormation falls back to the AWS-managed default. In some regions, the empty string causes aGeneralServiceExceptionon deployment.The
EventBusL2 already handles this correctly: it passesprops?.kmsKey?.keyArnwith no fallback.Archivewas inconsistent. The comment claiming the empty string was "necessary to enforce an update that removes any previously used CMK" is also wrong: removing a CMK means omitting the property, not supplying an empty string.Description of changes
archive.ts: removed the|| ''fallback fromkmsKeyIdentifier, so the property isundefined(and therefore absent from the synthesized template) when no key is provided. Removed the two misleading comment lines above theCfnArchiveconstructor call.archive.test.ts: addedMatchto the import. Renamed the no-key test to better describe what it verifies. Updated the assertion fromKmsKeyIdentifier: ''toKmsKeyIdentifier: Match.absent().Describe any new or updated permissions being added
None.
Description of how you validated changes
All five archive unit tests pass:
The CMK test still asserts
KmsKeyIdentifieris present and matches the key ARN. The CMK path is unaffected.No existing snapshot tests reference
KmsKeyIdentifier: "", so no snapshot updates are required.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license