-
Notifications
You must be signed in to change notification settings - Fork 4.3k
fix(cloudfront): unstable callerReference in the public key #34756
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- Introduced a feature flag `@aws-cdk/aws-cloudfront:stablePublicKeyCallerReference` to enable stable caller references for CloudFront PublicKey constructs. - Updated the PublicKey class to use a stable caller reference when the feature flag is enabled, preventing update failures due to changes in the construct tree. - Enhanced tests to verify the behavior of the stable caller reference under different scenarios, including feature flag toggling and construct tree changes. - Updated documentation to reflect the new feature and its usage.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Enhance the generateStableCallerReference() method to use a more intelligent truncation strategy when
leonmk-aws
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, generating the caller reference with Names.uniqueId does not solve the problem as this function generates a unique id based on the path of the construct in the tree, which means it changes when the PublicKey is moved in the tree (e.g during a refactoring) see:
| const components = node.scopes.slice(1).map(c => Node.of(c).id); |
|
Hi @leonmk-aws Thank you for the excellent feedback! You're absolutely correct about Current Implementation DetailsOur implementation uses What we DON'T use (path-based, unstable): // This would be unstable - changes when construct is moved
const unstableId = Names.uniqueId(this); // Uses full construct pathWhat we DO use (stable across tree changes): private generateStableCallerReference(): string {
const stack = Stack.of(this);
const constructId = this.node.id; // ← Only the immediate construct ID, not the full path
const stackName = stack.stackName;
const stableComponents = [
stackName,
constructId, // ← This is stable when construct is moved
stack.account || 'unknown-account',
stack.region || 'unknown-region',
];
// Create hash for uniqueness and length management
const hash = crypto.createHash('sha256')
.update(stableComponents.join('-'))
.digest('hex')
.substring(0, 8);
return `${stackName}-${constructId}-${hash}`;
}Demonstration of StabilityHere's a concrete example showing that our implementation is stable across construct tree changes: // Test 1: PublicKey directly in stack
new PublicKey(stack, 'MyPublicKey', { encodedKey: publicKey });
// Generated caller reference: "TestStack-MyPublicKey-0cc31391"
// Test 2: Same PublicKey moved to nested construct (same construct ID)
const wrapper = new WrapperConstruct(stack, 'Wrapper');
const deepWrapper = new DeepWrapper(wrapper, 'DeepWrapper');
new PublicKey(deepWrapper, 'MyPublicKey', { encodedKey: publicKey });
// Generated caller reference: "TestStack-MyPublicKey-0cc31391" ← Same!Key Differences from
|
| Approach | Uses | Stability | Example |
|---|---|---|---|
Names.uniqueId() |
Full construct path | ❌ Changes when moved | StackWrapperDeepWrapperMyPublicKey12AB34CD |
| Our implementation | this.node.id only |
✅ Stable when moved | TestStack-MyPublicKey-0cc31391 |
Test Coverage
We've added comprehensive tests that specifically validate this stability:
test('stable caller reference remains same when construct is moved in tree', () => {
// Test shows identical caller references despite different tree positions
expect(callerReference1).toEqual(callerReference2); // ✅ Passes
});
test('node.addr changes but stable caller reference does not when construct is moved', () => {
// Demonstrates the problem with node.addr and how we solve it
expect(publicKey1.node.addr).not.toEqual(publicKey2.node.addr); // Different paths
expect(stableCallerReference1).toEqual(stableCallerReference2); // Same caller ref ✅
});Why This Approach Works
this.node.idis the immediate construct identifier (e.g., "MyPublicKey") - it doesn't change when the construct is moved in the tree- Stack name provides uniqueness across different stacks
- Account/region ensures uniqueness across environments
- Hash suffix handles edge cases and length limits
The caller reference remains stable during refactoring because it's based on the construct's identity (its ID within the stack) rather than its location (its path in the tree).
Let me know if there's any other concern not addressed.
leonmk-aws
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately having the feature flag on makes this stack (that deploys fine with the FF off) impossible to deploy :
Invalid request provided: AWS::CloudFront::PublicKey: The caller reference that you are using to create a CloudFront public key is associated with another public key in your account.
const key = new cloudfront.PublicKey(this, 'TestPublicKey1', {
encodedKey: publicKey,
publicKeyName: 'test-public-key-1',
comment: 'Test public key for reproduction'
});
console.log("FF Enabled:", FeatureFlags.of(this).isEnabled(cdk.cx_api.CLOUDFRONT_STABLE_PUBLIC_KEY_CALLER_REFERENCE));
const wrapper = new Construct(this, 'Wrapper');
const key2 = new cloudfront.PublicKey(wrapper, 'TestPublicKey1', {
encodedKey: publicKey,
publicKeyName: 'test-public-key-2',
comment: 'Test public key for reproduction'
});
This is because of the way the caller reference is being built in the implementation: all the stableComponents are the same for key and key2. On possible way to move forward would be to use a combination of publicKeyName and the node id. This is because the key name has to be unique in Cloudfront. Unfortunately this would only work when the key name is set by the user.
Issue # (if applicable)
Closes #15301.
Reason for this change
CloudFront PublicKey constructs currently use
node.addras the caller reference, which changes when the construct tree structure is modified (e.g., moving constructs, renaming, or refactoring). This causes CloudFormation deployment failures with the error "Invalid request provided: AWS::CloudFront::PublicKey" because CloudFront treats caller reference changes as attempts to create new resources rather than updates to existing ones.This is a critical issue for users who need to refactor their CDK code or update their public keys, as any structural changes to the construct tree break subsequent deployments.
Description of changes
Core Changes:
@aws-cdk/aws-cloudfront:stablePublicKeyCallerReferenceincx-api/lib/features.tswithrecommendedValue: trueaws-cloudfront/lib/public-key.tsto:FeatureFlags.of(this).isEnabled()this.node.addrwhen flag is disabled (backward compatibility)Stable Caller Reference Implementation:
The new stable caller reference is generated using:
this.node.id(not the full path) for stabilityExample:
MyStack-MyPublicKey-a1b2c3d4(instead of path-basednode.addr)Why this approach solves the issue:
this.node.idinstead ofthis.node.addr, so moving constructs in the tree doesn't change the caller referenceComparison of approaches:
this.node.addr(Old/Unstable): Changes when you refactor code structure, even if the logical hierarchy is the sameDescribe any new or updated permissions being added
No new or updated IAM permissions are required. This change only affects the caller reference field in CloudFormation templates, which is a metadata field and doesn't impact AWS API permissions.
Description of how you validated changes
Unit Tests:
Added comprehensive test suite in
aws-cloudfront/test/public-key.test.tscovering:node.addrnode.addrchanges but stable caller reference doesn'tIntegration Tests:
integ.public-key-stable-caller-reference.tsdemonstrating real-world usageKey Test Results:
Checklist