Skip to content

Conversation

pahud
Copy link
Contributor

@pahud pahud commented Aug 4, 2025

Issue # (if applicable)

Closes #35136.

Reason for this change

DynamoDB TableV2 construct with customer-managed KMS encryption causes CloudFormation drift detection to report false positives. This occurs because CDK generates CloudFormation templates using KMS key ARNs (Fn::GetAtt with Arn), but DynamoDB internally stores only the key ID, leading to a mismatch during drift detection.

Description of changes

Root Cause: The _renderReplicaSseSpecification method in encryption.ts was using tableKey.keyArn which generates Fn::GetAtt with Arn, but the CloudFormation L1 property KMSMasterKeyId expects a key ID, not an ARN. While DynamoDB accepts ARNs at deployment time, it internally stores and returns only the key ID, causing drift detection mismatches.

Solution: Changed tableKey.keyArn to tableKey.keyId on line 73 of encryption.ts. This generates a Ref to the KMS key resource, which produces the key ID format that aligns with:

Files Modified:

  • packages/aws-cdk-lib/aws-dynamodb/lib/encryption.ts: Updated KMS key reference from ARN to ID
  • packages/aws-cdk-lib/aws-dynamodb/test/table-v2.test.ts: Updated test expectation to match corrected CloudFormation output

Impact: This fix eliminates false positive drift detection for DynamoDB TableV2 constructs using customer-managed KMS encryption while maintaining full backward compatibility.

Describe any new or updated permissions being added

No new or updated IAM permissions are required. This change only affects the CloudFormation template generation format to properly align with the KMSMasterKeyId property specification.

Description of how you validated changes

Unit Tests:

  • All 325 DynamoDB unit tests pass
  • Updated test expectation in table-v2.test.ts to verify correct CloudFormation output format (Ref vs Fn::GetAtt)
  • Added validation test for key ID format

Integration Tests:

  • All 25 existing DynamoDB integration tests pass without requiring snapshot updates
  • Verified that existing deployments continue to work seamlessly

Manual Validation:

  • Confirmed CloudFormation template now generates Ref instead of Fn::GetAtt for KMS key references
  • Verified the fix addresses the specific drift detection scenario described in the issue
  • Validated that the generated key ID format matches the KMSMasterKeyId property expectation

Regression Testing:

  • No breaking changes to existing functionality
  • Backward compatibility maintained for existing stacks (CloudFormation accepts both formats)
  • Cross-language (JSII) compatibility preserved

Checklist


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

@aws-cdk-automation aws-cdk-automation requested a review from a team August 4, 2025 18:37
@github-actions github-actions bot added bug This issue is a bug. effort/medium Medium work item – several days of effort p1 labels Aug 4, 2025
@pahud pahud changed the title fix(aws-dynamodb): use keyId instead of keyArn for TableV2 replica encryption fix(dynamodb): use keyId instead of keyArn for TableV2 replica encryption Aug 4, 2025
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Aug 4, 2025
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(This review is outdated)

@pahud
Copy link
Contributor Author

pahud commented Aug 4, 2025

Exemption Request

Why no new integration test is needed:

  • The change is purely about CloudFormation template generation format
  • The unit tests validate the correct CloudFormation template format is generated

@aws-cdk-automation aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Aug 4, 2025
@pahud pahud marked this pull request as ready for review August 4, 2025 21:43
@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Aug 4, 2025
@Abogical Abogical self-assigned this Aug 6, 2025
@Abogical Abogical added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Aug 6, 2025
@aws-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Aug 6, 2025
@aws-cdk-automation aws-cdk-automation dismissed their stale review August 6, 2025 15:15

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

Copy link
Contributor

mergify bot commented Aug 6, 2025

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).

Copy link
Contributor

mergify bot commented Aug 6, 2025

This pull request has been removed from the queue for the following reason: pull request branch update failed.

The pull request can't be updated.

You should update or rebase your pull request manually. If you do, this pull request will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

@Abogical
Copy link
Member

Abogical commented Aug 6, 2025

@Mergifyio rebase

pahud added 6 commits August 6, 2025 15:24
…cryption

Fixes CloudFormation drift detection for DynamoDB TableV2 with customer-managed KMS encryption.
Previously, CDK generated templates using KMS key ARN, but DynamoDB internally stores only the
key ID, causing false positive drift detection.

Changes:
- encryption.ts: Use tableKey.keyId instead of tableKey.keyArn in _renderReplicaSseSpecification
- Update test expectation to match corrected CloudFormation output (Ref vs Fn::GetAtt)

Fixes aws#35136
Copy link
Contributor

mergify bot commented Aug 6, 2025

rebase

✅ Branch has been successfully rebased

Copy link
Contributor

mergify bot commented Aug 6, 2025

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).

Copy link
Contributor

mergify bot commented Aug 6, 2025

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
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@mergify mergify bot merged commit 787b8ed into aws:main Aug 6, 2025
20 checks passed
Copy link
Contributor

github-actions bot commented Aug 6, 2025

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 github-actions bot locked as resolved and limited conversation to collaborators Aug 6, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug This issue is a bug. contribution/core This is a PR that came from AWS. effort/medium Medium work item – several days of effort p1 pr-linter/exempt-integ-test The PR linter will not require integ test changes pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dynamodb: KMSMasterKeyId gets deployed with id rather than arn leading to drift
3 participants