fix(cloudfront): make long function name deterministic #30392

Leo10Gama · 2024-05-30T17:49:58Z

Issue # (if applicable)

Closes #20017 as well as #15523 and #28629

Reason for this change

Due to the way function names are generated using token strings with either single- or double-digit numbers, longer function names can be truncated differently, leading to inconsistency in generated CloudFormation templates.

Description of changes

To ensure backwards compatibility, if names are longer than 64 characters and use region tokens, if the token uses a single-digit region number, it takes the first 31 characters + the last 32 characters; if the token uses a double-digit region number or otherwise, it takes the first 32 characters + the last 32 characters. This ensures it will always take the same first chunk of the actual function's name.

Description of how you validated changes

A new unit test was added to verify the consistency of function names in the template.

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.

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

…into fix-cf-fn-name

scanlonp

Hey @Leo10Gama, I like the nature of the changes. A couple things to think about.

First, this looks like it may be a breaking change. From the comments in #15523 and #28641, changing this name will replace the CloudFront function. We may need to put this behind a feature flag.

Second, I think we can solve the consistency problems as well keeping the region in the name by adding the region to the name separately from the truncation. Say we do

const uniqueName = Lazy.string({
        produce: () => Names.uniqueResourceName(this, { maxLength: 40, allowedSpecialCharacters: '-_' }),
});
return Stack.of(this).region + uniqueName;

If the current longest region name is 14 characters (ap-southeast-1), we need to make sure the uniqueName is max 50 characters. Why not bake in some wiggleroom for future region names an make the max length 40.

#28641 used Names.uniqueResourceName in order to control the length, but truncating Names.uniqueId is also an option.

Leo10Gama · 2024-05-31T17:08:12Z

Hey @scanlonp, thanks for the feedback!

With regards to whether this would be a breaking change, I tried to find a solution that would only affect users facing this issue, hence why I wrote the if-else logic the way I did. My idea was to preserve the original logic, but rework it so that the newer logic only happens for affected users, hence the .startsWith() check at the beginning. In any other situation, the original name generation is preserved, so users who weren't affected won't see a difference, thus I don't see the need for using a flag.

The Lazy.string() approach seems like it would also work, since it would replace the name with a token like the region, but that would definitely introduce breaking changes and would need to go behind a feature flag as you said. Since this is a bug that's been affecting users for a while (given a few issues opened about it), I think it would be good to try prioritizing a fix that isn't hidden behind a flag, for ease of use for users.

I wouldn't be opposed to making the lazy string change if the flag is necessary, since it does seem like a better way to generate names as well compared to the existing logic! But if we can find a way to maintain the original logic while only fixing the issue for affected users, we ought to do that. If breaking changes are unavoidable, then the flag and lazy string approach would definitely be the way to go, but I'd be open to trying to rework this logic if you know of any cases this would break.

If we go with the current implementation, though, I can also replace the magic number indicating how many digits are in the region token with a computed value for clarity lol

scanlonp · 2024-06-01T00:55:22Z

@Leo10Gama, I certainly agree with preserving the logic for users who do not run into this bug.

To what extent we can "break" customers depends on who is facing this bug right now. The non-deterministic behavior occurs when the function name is sufficiently long. Since we have non-deterministic behavior, we are possibly forcing replacements on every deploy, so it should be ok to just make the change without a feature flag in those cases. For customers whose function name is not that long, they have never faced the issue. I appreciate your concern for not changing the behavior for this second group of users.

With that in mind, I did some testing, and found that if the user's stack and function id's are 34 or more characters combined, we see the non-determinism.
It appears that Names.uniqueResourceName() and Names.uniqueId() essentially have the same output - uniqueResourceName() just allows some configuration, such as the max length. Therefore, for users whose stack and function id's are 33 or less characters combined, Names.uniqueResourceName()with max characters >33 will generate the same name as the old composition, so those users will see no difference. (Lazy.string() also resolves the token at synth time, so there is not a concern there about introducing more tokens to the template or breaking users in that way.)

For users with stack + FunctionName >= 34, they are already broken, so the new generation will make their names standard. I also like using a function specialized for accommodating resource names with a max length rather than hacking it together with logic in the construct itself.

This is to say I believe we are free to change the name generation in this way without a feature flag at all!

Since affected users are already redeploying due to non- deterministic names, we can change the logic entirely to make it cleaner and ensure consistency

scanlonp

Do we need to check the length at all? I think we can just generate the name consistently from now on. I believe that the behavior will not change for users whose function name was <64 characters.

Leo10Gama · 2024-06-04T22:31:25Z

Yes, behaviour should be consistent now thanks to doing the name-splitting before appending the region. As you said, the current longest region name is 14 characters, so leaving the name at 40 shouldn't cause any issues, but I could add a quick check before the new logic's return to verify it's <64 characters, if that's preferred.

scanlonp · 2024-06-04T22:49:36Z

I meant that we would not even have to create the initial name and verify if it is less than 64 characters at all. We could simply use the new logic every time.

I am trying to test if this would make any difference. So far, it has not, but I need to look at the impact for when the Function is nested in the construct tree - defined within another construct or stack.

Since the affected names are longer than 64 characters anyways, we can simplify all the logic, as the name won't be trimmed if it is shorter than 40 characters anyway.

…into fix-cf-fn-name

### Reason for this change Add support for newly supported `8.0.mysql_aurora.3.07.0`. https://docs.aws.amazon.com/AmazonRDS/latest/AuroraMySQLReleaseNotes/AuroraMySQL.Updates.3070.html ### Description of changes Add a new version as a new property to `AuroraMysqlEngineVersion` class. ### Description of how you validated changes I used the AWS CLI to verify that the new version is available. ```bash aws rds describe-db-engine-versions --engine aurora-mysql --query "DBEngineVersions[?EngineVersion=='8.0.mysql_aurora.3.07.0']" [ { "Engine": "aurora-mysql", "EngineVersion": "8.0.mysql_aurora.3.07.0", "DBParameterGroupFamily": "aurora-mysql8.0", "DBEngineDescription": "Aurora MySQL", "DBEngineVersionDescription": "Aurora MySQL 3.07.0 (compatible with MySQL 8.0.36)", "ValidUpgradeTarget": [], "ExportableLogTypes": [ "audit", "error", "general", "slowquery" ], "SupportsLogExportsToCloudwatchLogs": true, "SupportsReadReplica": false, "SupportedEngineModes": [ "provisioned" ], "SupportedFeatureNames": [ "Bedrock" ], "Status": "available", "SupportsParallelQuery": true, "SupportsGlobalDatabases": true, "MajorEngineVersion": "8.0", "SupportsBabelfish": false, "SupportsLimitlessDatabase": false, "SupportsCertificateRotationWithoutRestart": true, "SupportedCACertificateIdentifiers": [ "rds-ca-2019", "rds-ca-ecc384-g1", "rds-ca-rsa4096-g1", "rds-ca-rsa2048-g1" ], "SupportsLocalWriteForwarding": true, "SupportsIntegrations": true } ] ``` Hopefully this will be automated in the future. ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Updates the L1 CloudFormation resource definitions with the latest changes from `@aws-cdk/aws-service-spec` **L1 CloudFormation resource definition changes:** ``` ├[~] service aws-autoscaling │ └ resources │ └[~] resource AWS::AutoScaling::ScalingPolicy │ └ types │ ├[~] type CustomizedMetricSpecification │ │ └ properties │ │ ├ MetricName: - string (required) │ │ │ + string │ │ ├[+] Metrics: Array<TargetTrackingMetricDataQuery> │ │ ├ Namespace: - string (required) │ │ │ + string │ │ └ Statistic: - string (required) │ │ + string │ ├[+] type TargetTrackingMetricDataQuery │ │ ├ name: TargetTrackingMetricDataQuery │ │ └ properties │ │ ├Label: string │ │ ├MetricStat: TargetTrackingMetricStat │ │ ├Id: string (required) │ │ ├ReturnData: boolean │ │ └Expression: string │ └[+] type TargetTrackingMetricStat │ ├ name: TargetTrackingMetricStat │ └ properties │ ├Metric: Metric (required) │ ├Stat: string (required) │ └Unit: string ├[~] service aws-connect │ └ resources │ └[~] resource AWS::Connect::Rule │ └ types │ ├[~] type Actions │ │ └ properties │ │ └[+] SubmitAutoEvaluationActions: Array<SubmitAutoEvaluationAction> │ └[+] type SubmitAutoEvaluationAction │ ├ documentation: The definition of submit auto evaluation action. │ │ name: SubmitAutoEvaluationAction │ └ properties │ └EvaluationFormArn: string (required) ├[~] service aws-ec2 │ └ resources │ └[~] resource AWS::EC2::TransitGatewayRoute ├[~] service aws-ecs │ └ resources │ └[~] resource AWS::ECS::Cluster │ └ types │ ├[~] type ClusterConfiguration │ │ └ properties │ │ └[+] ManagedStorageConfiguration: ManagedStorageConfiguration │ └[+] type ManagedStorageConfiguration │ ├ name: ManagedStorageConfiguration │ └ properties │ ├FargateEphemeralStorageKmsKeyId: string │ └KmsKeyId: string ├[~] service aws-pipes │ └ resources │ └[~] resource AWS::Pipes::Pipe │ └ types │ ├[+] type DimensionMapping │ │ ├ name: DimensionMapping │ │ └ properties │ │ ├DimensionValue: string (required) │ │ ├DimensionValueType: string (required) │ │ └DimensionName: string (required) │ ├[+] type MultiMeasureAttributeMapping │ │ ├ name: MultiMeasureAttributeMapping │ │ └ properties │ │ ├MeasureValue: string (required) │ │ ├MeasureValueType: string (required) │ │ └MultiMeasureAttributeName: string (required) │ ├[+] type MultiMeasureMapping │ │ ├ name: MultiMeasureMapping │ │ └ properties │ │ ├MultiMeasureName: string (required) │ │ └MultiMeasureAttributeMappings: Array<MultiMeasureAttributeMapping> (required) │ ├[~] type PipeTargetParameters │ │ └ properties │ │ └[+] TimestreamParameters: PipeTargetTimestreamParameters │ ├[+] type PipeTargetTimestreamParameters │ │ ├ name: PipeTargetTimestreamParameters │ │ └ properties │ │ ├TimeValue: string (required) │ │ ├EpochTimeUnit: string │ │ ├TimeFieldType: string │ │ ├TimestampFormat: string │ │ ├VersionValue: string (required) │ │ ├DimensionMappings: Array<DimensionMapping> (required) │ │ ├SingleMeasureMappings: Array<SingleMeasureMapping> │ │ └MultiMeasureMappings: Array<MultiMeasureMapping> │ └[+] type SingleMeasureMapping │ ├ name: SingleMeasureMapping │ └ properties │ ├MeasureValue: string (required) │ ├MeasureValueType: string (required) │ └MeasureName: string (required) ├[~] service aws-rolesanywhere │ └ resources │ └[~] resource AWS::RolesAnywhere::Profile │ ├ properties │ │ └[+] AttributeMappings: Array<AttributeMapping> │ └ types │ ├[+] type AttributeMapping │ │ ├ name: AttributeMapping │ │ └ properties │ │ ├MappingRules: Array<MappingRule> (required) │ │ └CertificateField: string (required) │ └[+] type MappingRule │ ├ name: MappingRule │ └ properties │ └Specifier: string (required) └[~] service aws-securitylake └ resources └[~] resource AWS::SecurityLake::DataLake └ properties └ MetaStoreManagerRoleArn: - string (immutable) + string ```

### Issue # (if applicable) Closes aws#29049. ### Reason for this change Allow the Topic construct to expose a method to grant subscription permissions to a grantable resource. It's useful when you want to allow entities, such as another AWS account or resources created later, to subscribe to the topic at their own pace, separating permission granting from the actual subscription process. ### Description of changes Add grantSubscribe method to ITopic interface and TopicBase class. ### Description of how you validated changes Add unit tests and integ tests. ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

### Issue # (if applicable) N/A ### Reason for this change Cluster can be upgraded/created to Postgres 15.6 via the console/CLI but not CDK. ### Description of changes Adds support for Postgres 15.6 Aurora cluster, 15.5 instances are already supported. ### Description of how you validated changes N/A ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…#30351) ### Issue # (if applicable) N/A ### Reason for this change AppRunner supported Dual Stack. https://aws.amazon.com/about-aws/whats-new/2023/11/aws-app-runner-supports-ipv6-public-inbound-traffic/?nc1=h_ls But current L2 Construct (alpha module) does not support it. ### Description of changes Add ipAddressType property to the Service class ### Description of how you validated changes Add unit tests and integ tests. ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…into fix-cf-fn-name

scanlonp

Nice work @Leo10Gama, looks great!

Pretty funny that the two integ tests straddle the character limit exactly.

mergify · 2024-06-11T15:57:28Z

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 · 2024-06-11T16:27:47Z

AWS CodeBuild CI Report

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

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

mergify · 2024-06-11T16:28:05Z

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

### Issue # (if applicable) Closes aws#20017 as well as aws#15523 and aws#28629 ### Reason for this change Due to the way function names are generated using token strings with either single- or double-digit numbers, longer function names can be truncated differently, leading to inconsistency in generated CloudFormation templates. ### Description of changes To ensure backwards compatibility, if names are longer than 64 characters and use region tokens, if the token uses a single-digit region number, it takes the first **31** characters + the last 32 characters; if the token uses a double-digit region number or otherwise, it takes the first **32** characters + the last 32 characters. This ensures it will always take the same first chunk of the actual function's name. ### Description of how you validated changes A new unit test was added to verify the consistency of function names in the template. ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

aws-cdk-automation · 2024-07-25T20:50:16Z

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.

Fix cloudfront function name gen logic

496e264

github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort p1 labels May 30, 2024

Merge branch 'main' into fix-cf-fn-name

30194d5

aws-cdk-automation requested a review from a team May 30, 2024 17:50

mergify bot added the contribution/core This is a PR that came from AWS. label May 30, 2024

aws-cdk-automation previously requested changes May 30, 2024

View reviewed changes

Update integ tests

c0a1051

Leo10Gama and others added 3 commits May 30, 2024 13:22

Merge branch 'main' into fix-cf-fn-name

085bc1d

Fix formatting

8d16fb6

Merge branch 'fix-cf-fn-name' of https://github.com/Leo9Gama/aws-cdk …

22ad2eb

…into fix-cf-fn-name

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label May 30, 2024

Leo10Gama mentioned this pull request May 30, 2024

fix(cloudfront): function name is non-deterministic (under feature flag) #28641

Closed

scanlonp requested changes May 31, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label May 31, 2024

This was referenced Jun 1, 2024

Monthly PR metrics report #30414

Closed

Weekly PR metrics report #30434

Closed

Leonardo Gama and others added 3 commits June 3, 2024 13:36

Simplify long-name logic

40d3444

Since affected users are already redeploying due to non- deterministic names, we can change the logic entirely to make it cleaner and ensure consistency

Merge branch 'main' into fix-cf-fn-name

b5efa09

Merge branch 'main' into fix-cf-fn-name

36322da

scanlonp requested changes Jun 4, 2024

View reviewed changes

Leonardo Gama and others added 3 commits June 6, 2024 11:19

Remove exclusivity logic

aa8cc03

Since the affected names are longer than 64 characters anyways, we can simplify all the logic, as the name won't be trimmed if it is shorter than 40 characters anyway.

Merge branch 'fix-cf-fn-name' of https://github.com/Leo10Gama/aws-cdk …

0518fb3

…into fix-cf-fn-name

Merge branch 'main' into fix-cf-fn-name

ed15c36

AWS CDK Team and others added 15 commits June 11, 2024 09:55

chore(release): 2.145.0

cf2550a

chore: update CHANGELOG.v2.md

69a9343

Preserve parsing logic from original method

8d9a9c0

Update integ test

509b897

Implement requested changes

21f7935

Untrack feature flag

8bda505

Untrack feature flag

1c0aa3e

Merge branch 'fix-cf-fn-name' of https://github.com/Leo10Gama/aws-cdk …

88b5243

…into fix-cf-fn-name

Untrack feature flag

35acca0

Update integ test

225d2c4

scanlonp approved these changes Jun 11, 2024

View reviewed changes

Merge branch 'main' into fix-cf-fn-name

724640d

mergify bot merged commit e19d18b into aws:main Jun 11, 2024
9 checks passed

Leo10Gama deleted the fix-cf-fn-name branch June 11, 2024 18:32

This was referenced Jul 5, 2024

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#107

Open

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#108

Open

This was referenced Jul 16, 2024

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#109

Open

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#110

Open

NOUIY mentioned this pull request Jul 23, 2024

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#111

Open

aws locked as resolved and limited conversation to collaborators Jul 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cloudfront): make long function name deterministic #30392

fix(cloudfront): make long function name deterministic #30392

Leo10Gama commented May 30, 2024

aws-cdk-automation left a comment

scanlonp left a comment •

edited

Loading

Leo10Gama commented May 31, 2024

scanlonp commented Jun 1, 2024

scanlonp left a comment

Leo10Gama commented Jun 4, 2024

scanlonp commented Jun 4, 2024

scanlonp left a comment

mergify bot commented Jun 11, 2024

aws-cdk-automation commented Jun 11, 2024

mergify bot commented Jun 11, 2024

aws-cdk-automation commented Jul 25, 2024

fix(cloudfront): make long function name deterministic #30392

fix(cloudfront): make long function name deterministic #30392

Conversation

Leo10Gama commented May 30, 2024

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

aws-cdk-automation left a comment

Choose a reason for hiding this comment

scanlonp left a comment • edited Loading

Choose a reason for hiding this comment

Leo10Gama commented May 31, 2024

scanlonp commented Jun 1, 2024

scanlonp left a comment

Choose a reason for hiding this comment

Leo10Gama commented Jun 4, 2024

scanlonp commented Jun 4, 2024

scanlonp left a comment

Choose a reason for hiding this comment

mergify bot commented Jun 11, 2024

aws-cdk-automation commented Jun 11, 2024

AWS CodeBuild CI Report

mergify bot commented Jun 11, 2024

aws-cdk-automation commented Jul 25, 2024

scanlonp left a comment •

edited

Loading