fix(core): Add stage prefix to stack name shortening process by QustodioPablo · Pull Request #24443 · aws/aws-cdk

QustodioPablo · 2023-03-03T16:48:06Z

There is an issue in the stack name generation process where the prefix generated from assembly's stage name is not taken into account when shortening a stack name to meet the requirement of being equal or less than 128 characters longs.

This can lead to generating an invalid stack name greater than 128 characters: since the stack name is shortened to 128 characters, when the prefix is added, the limit is exceeded.

Current solution:

Add the prefix into the first ID if any prefix is present.

Alternative solution:

Add the prefix at the start of the ID list. Doing this would require to add more conditionals and consider more corner cases to keep the names consistent with the old algorithm.

Fixes #23628

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.

QustodioPablo · 2023-03-03T16:55:45Z

Clarification Request: I was unable to run the integration tests, but created a unit test, and made sure the change was not breaking the already existing unit tests. Do I need to create integration tests for this PR?

…blo/fix-stack-name-too-long

…stodioPablo/aws-cdk into QustodioPablo/fix-stack-name-too-long

TheRealAmazonKendra

We do this in a lot of places, adding prefixes. I'm thinking it's a better options to add prefix as an optional prop in makeUniqueResourceName and adding the prefix there. What do you think? Also, I if this isn't impacting the outcome of any integ tests, I think omitting one is fine.

TheRealAmazonKendra · 2023-03-28T21:17:59Z

packages/@aws-cdk/core/lib/stack.ts

 */
 function makeStackName(components: string[]) {
-  if (components.length === 1) { return components[0]; }
+  if (components.length === 1 && components[0].length <= 128) { return components[0]; }


Iirc, this check is done inside makeUniqueResourceName so it shouldn't be needed here.

The check done inside makeUniqueResourceName will remove the - from the prefix, as it removes anything that is not a number or a character, unless you specify to keep a special character though parameters, but I was afraid of breaking compatibility with already existing stacks if doing so.
I'll make changes to pass the prefix to the makeUniqueResourceName as an optional parameter as you mentioned in another comment.
Thanks!

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

…blo/fix-stack-name-too-long

Pull request has been modified.

QustodioPablo · 2023-03-29T16:49:09Z

packages/@aws-cdk/core/lib/private/unique-resource-name.ts


  // Calculate the hash from the full path, included unresolved tokens so the hash value is always unique
  const hash = pathHash(components);
+  if (prefix) {


The prefix is added after the hash calculation to preserver the same hash for already existing stacks that fall within this scenario.
I'm not sure if I'm worrying too much, as I'm not sure what happens if you create a stack with CDK, then after this change you run CDK again, and the old created stack's name does not match the new name generated.

…blo/fix-stack-name-too-long

QustodioPablo · 2023-03-30T12:27:44Z

packages/@aws-cdk/core/lib/stack.ts

+  if (components.length === 1) {
+    const stack_name = prefix + components[0];
+    if (stack_name.length <= 128) {
+      return prefix + components[0];


When there is a single component and a prefix, passing both the components and the prefix to the makeUniqueResourceName might seem like the simplest and most straight forward implementation, but there is an issue with this approach.
Doing so will make some tests fail, where a stage with an empty stack is defined. Doing so will generate a prefix (the stage name), and a single component with the name of the default stack. The makeUniqueResourceName function removes all components with the default name, leaving us with no resources in the list, which makes makeUniqueResourceName throw an error.

…blo/fix-stack-name-too-long

TheRealAmazonKendra

Totally understand your concerns about these changes causing breaking changes, but this is adding a lot of extra handling to avoid that. I'm thinking that it might be best to put these changes behind a feature flag so that the changes can be made without worrying about that. What do you think?

TheRealAmazonKendra · 2023-04-05T04:29:46Z

packages/aws-cdk-lib/core/lib/private/unique-resource-name.ts

 const HASH_LEN = 8;

-export function makeUniqueResourceName(components: string[], options: MakeUniqueResourceNameOptions) {
+export function makeUniqueResourceName(components: string[], options: MakeUniqueResourceNameOptions, prefix: string='') {


Let's add this as an option instead independently like this.

…stodioPablo/aws-cdk into QustodioPablo/fix-stack-name-too-long

…blo/fix-stack-name-too-long

Pull request has been modified.

QustodioPablo · 2023-04-05T14:10:20Z

Totally understand your concerns about these changes causing breaking changes, but this is adding a lot of extra handling to avoid that. I'm thinking that it might be best to put these changes behind a feature flag so that the changes can be made without worrying about that. What do you think?

I'm ok with adding this behind a feature flag, but we have stacks deployed with an older version that needs updating. We were not able to upgrade our CDK version yet because of this issue, so I'd like to try and fix this issue while keeping it compatible with older versions as much as possible, since afaik it would be mean having to redeploy everything in case the naming changes.

Any suggestion?

mergify · 2023-04-11T14:04:22Z

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 · 2023-04-11T14:35:36Z

AWS CodeBuild CI Report

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

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

mergify · 2023-04-11T14:36:44Z

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

…24443)" This reverts commit 55621ad.

…process (#25163) Reverts #24443. Fixes #25130.

QustodioPablo · 2023-04-26T08:29:47Z

Gah, I was away when the PR was approved and merged, and didnt have a chance to implement the feature flag.
Will look into it and create a new PR when done.

There is an issue in the stack name generation process where the prefix generated from assembly's stage name is not taken into account when shortening a stack name to meet the requirement of being equal or less than 128 characters longs. This can lead to generating an invalid stack name greater than 128 characters: since the stack name is shortened to 128 characters, when the prefix is added, the limit is exceeded. Current solution: - Adding a feature flag - With the feature flag on, the prefix is processed within the generateUniqueName function. - With the feature flag off, stack name generation is not changed Fixes #23628 NOTE: This PR was previously opened, but it was merged before I was able to add a feature flag, which ended up adding breaking changes and the changes of the PR were rolled back. Old PR: #24443

Add prefix to stack name shortening process

8abb431

github-actions bot added bug This issue is a bug. p2 labels Mar 3, 2023

aws-cdk-automation requested a review from a team March 3, 2023 16:48

github-actions bot added the beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK label Mar 3, 2023

aws-cdk-automation previously requested changes Mar 3, 2023

View reviewed changes

aws-cdk-automation added the pr/reviewer-clarification-requested The contributor has requested clarification on feedback, a failing build, or a failing PR Linter run label Mar 3, 2023

QustodioPablo and others added 6 commits March 3, 2023 17:59

Merge branch 'main' into QustodioPablo/fix-stack-name-too-long

668539c

Merge branch 'main' into QustodioPablo/fix-stack-name-too-long

6c1532b

Merge branch 'main' into QustodioPablo/fix-stack-name-too-long

7f06cd7

Merge branch 'main' into QustodioPablo/fix-stack-name-too-long

b28f46f

Merge branch 'main' of https://github.com/aws/aws-cdk into QustodioPa…

0cf310a

…blo/fix-stack-name-too-long

Merge branch 'QustodioPablo/fix-stack-name-too-long' of github.com:Qu…

b126e20

…stodioPablo/aws-cdk into QustodioPablo/fix-stack-name-too-long

TheRealAmazonKendra previously requested changes Mar 28, 2023

View reviewed changes

TheRealAmazonKendra reviewed Mar 28, 2023

View reviewed changes

TheRealAmazonKendra added pr-linter/exempt-integ-test The PR linter will not require integ test changes and removed pr/reviewer-clarification-requested The contributor has requested clarification on feedback, a failing build, or a failing PR Linter run labels Mar 28, 2023

aws deleted a comment from aws-cdk-automation Mar 28, 2023

Merge branch 'main' of https://github.com/aws/aws-cdk into QustodioPa…

1f5b03b

…blo/fix-stack-name-too-long

Process prefix inside the makeUniqueResourceName function

b151d21

QustodioPablo commented Mar 29, 2023

View reviewed changes

QustodioPablo added 4 commits March 30, 2023 10:32

Merge branch 'main' of https://github.com/aws/aws-cdk into QustodioPa…

b1aee08

…blo/fix-stack-name-too-long

Fixed scenario of Stage with only one default component

ecfc9cb

Fixed missing brace

297273a

Merge branch 'main' of https://github.com/aws/aws-cdk into QustodioPa…

16e96c1

…blo/fix-stack-name-too-long

QustodioPablo requested a review from TheRealAmazonKendra March 30, 2023 11:53

QustodioPablo commented Mar 30, 2023

View reviewed changes

QustodioPablo added 4 commits March 31, 2023 11:49

Use variable

0e28b23

Merge branch 'main' of https://github.com/aws/aws-cdk into QustodioPa…

f52804e

…blo/fix-stack-name-too-long

Merge branch 'main' of https://github.com/aws/aws-cdk into QustodioPa…

adecdbc

…blo/fix-stack-name-too-long

Recovered changes

f979e85

QustodioPablo requested a review from aws-cdk-automation April 3, 2023 16:24

Merge branch 'main' into QustodioPablo/fix-stack-name-too-long

e9cd174

TheRealAmazonKendra previously requested changes Apr 5, 2023

View reviewed changes

QustodioPablo added 3 commits April 5, 2023 10:10

Added prefix as option

c6c6053

Merge branch 'QustodioPablo/fix-stack-name-too-long' of github.com:Qu…

7655cd2

…stodioPablo/aws-cdk into QustodioPablo/fix-stack-name-too-long

Merge branch 'main' of https://github.com/aws/aws-cdk into QustodioPa…

56bd828

…blo/fix-stack-name-too-long

Merge branch 'main' into QustodioPablo/fix-stack-name-too-long

5dd29bc

otaviomacedo approved these changes Apr 11, 2023

View reviewed changes

Merge branch 'main' into QustodioPablo/fix-stack-name-too-long

729f23e

mergify bot merged commit 55621ad into aws:main Apr 11, 2023

kellertk added a commit that referenced this pull request Apr 17, 2023

Revert "fix(core): Add stage prefix to stack name shortening process (#…

e0b8f00

…24443)" This reverts commit 55621ad.

kellertk mentioned this pull request Apr 17, 2023

chore(core): Revert #24443 add stage prefix to stack name shortening process #25163

Merged

mergify bot pushed a commit that referenced this pull request Apr 17, 2023

chore(core): Revert #24443 add stage prefix to stack name shortening …

8492dfe

…process (#25163) Reverts #24443. Fixes #25130.

comcalvi pushed a commit that referenced this pull request Apr 18, 2023

chore(core): Revert #24443 add stage prefix to stack name shortening …

cfac3f3

…process (#25163) Reverts #24443. Fixes #25130.

QustodioPablo mentioned this pull request Apr 28, 2023

fix(core): Add stage prefix to stack name shortening process #25359

Merged

filletofish mentioned this pull request Jul 2, 2023

feat(scheduler): ScheduleGroup #26196

Merged

Conversation

QustodioPablo commented Mar 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

QustodioPablo commented Mar 3, 2023

Uh oh!

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

Uh oh!

TheRealAmazonKendra Mar 28, 2023

Choose a reason for hiding this comment

Uh oh!

QustodioPablo Mar 29, 2023

Choose a reason for hiding this comment

Uh oh!

QustodioPablo Mar 29, 2023

Choose a reason for hiding this comment

Uh oh!

QustodioPablo Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

Uh oh!

TheRealAmazonKendra Apr 5, 2023

Choose a reason for hiding this comment

Uh oh!

QustodioPablo Apr 5, 2023

Choose a reason for hiding this comment

Uh oh!

QustodioPablo commented Apr 5, 2023

Uh oh!

mergify bot commented Apr 11, 2023

Uh oh!

aws-cdk-automation commented Apr 11, 2023

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Apr 11, 2023

Uh oh!

QustodioPablo commented Apr 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

QustodioPablo commented Mar 3, 2023 •

edited

Loading

QustodioPablo Mar 30, 2023 •

edited

Loading