fix(cloudfront): function name is non-deterministic (under feature flag) #28641
Conversation
This fixes an issue where having a long stack name or resource ID could result in inconsistent naming of the function (replacing it on each deployment) because the length of the token that holds the region may not be consistent. This uses a more deterministic method based on checking whether the region is resolved and using the `uniqueResourceName` function. It is important to note that while this doesn't change the name of any current resources in the unit/integration tests, I am not 100% certain that this won't impact any existing users.
github-actions
bot
added
the
star-contributor
github-actions
bot
added
bug
aws-cdk-automation
added
the
pr/needs-community-review
| // then we use the name of the longest known region. | ||
| const serviceMaxNameLength = 64; | ||
| const region = Stack.of(this).region; | ||
| const regionNameLength = !Token.isUnresolved(region) ? region.length : Math.max(...Fact.regions.map((r) => r.length)); |
There was a problem hiding this comment.
It seems to me that this can potentially trigger re-deploy for those cases where region is not resolved.
Given that the issue needs to be fixed and it's probably going to be a breaking change anyway, I think it may be worth considering a refactor/simplification of the name generation function (under a feature flag) to avoid future issues, eg:
return Lazy.string({
produce: () => Names.uniqueResourceName(this, { maxLength: 64, allowedSpecialCharacters: '-_' }),
});There was a problem hiding this comment.
I think that including the region name is, unfortunately, a very intentional decision so that the same stack can be deployed in multiple regions. #14511 (comment)
But then again, any callers can always just pass functionName: `${this.env.region}Name`, if that's really an use case for them under the feature flag.
There was a problem hiding this comment.
Added the feature flag and including a note about region inclusion as part of the detailsMd. Let me know what you think!
laurelmay
changed the title
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
aws-cdk-automation
added
pr/needs-maintainer-review
For my own understanding, how does the existing behaviour cause redeployment if stack name and id are not changed? |
You should be able to see this if you run the newly added integration tests a couple of times. You will see the function name property change. The CloudFormation documentation for this resource is wrong -- editing the While replacing a CloudFront function is a minimally disruptive operation, it is still disruptive and a resource has been replaced. We should avoid arbitrarily changing resource physical names within the CDK. |
GavinZZ
dismissed
their stale review
Dismissing my approval as I noticed a lot of conflicts. Please address the conflicts and ping me again for another pass.
|
@kylelaker would you be able to fix the merge conflicts? This PR looks good and I'm happy to approve once conflicts are resolved. |
aaythapa
added
the
pr-linter/do-not-close
aws-cdk-automation
removed
the
pr/needs-maintainer-review
|
Closing this as it seems the PR has not been active for quite some time, and another PR has been opened to resolve this issue. |
This fixes an issue where having a long stack name or resource ID could
result in inconsistent naming of the function (replacing it on each
deployment) because the length of the token that holds the region may
not be consistent. This uses a more deterministic method based on
checking whether the region is resolved and using the
uniqueResourceNamefunction.This change is behind a feature flag,
@aws-cdk/aws-cloudfront:useStableGeneratedFunctionNameto avoid issueswith replacing existing functions.
Closes #28629
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license