fix(codepipeline): cannot deploy pipeline stack with crossAccountKeys twice (under feature flag) by TheRealAmazonKendra · Pull Request #19418 · aws/aws-cdk

TheRealAmazonKendra · 2022-03-16T10:07:51Z

When multiple copies of the same pipeline are deployed in separate stacks, the alias name for the KMS key is the same, causing the deployment to fail. This hcange fixes that using the stack name instead of the stack ID to create a stack safe uniqueId for the alias name. This fix is behind the following feature flag:

@aws-cdk/aws-codepipeline:crossAccountKeyAliasStackSafeUniqueId

Fixes issue #18828.

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

gitpod-io · 2022-03-16T10:07:54Z

rix0rrr

Some minor changes.

About the PR itself:

Love the PR body description (not just a link to an issue but describing the issue and the fix). Do add the feature flag value to the PR description so that people reading the change log can click through to the PR and know what they need to change if they are bitten by this.
The title is going to show up in the changelog, in this case under the heading "Bug fixes". As a convention, we name the PR after the bug [that is fixed], so it makes sense from the perspective of customers reading the changelog. So it would look something like fix(codepipeline): cannot deploy pipeline stack with crossAccountKeys twice

rix0rrr · 2022-03-16T11:18:59Z

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

+   * @returns a unique id based on the construct path
+   */
+  public static uniqueIdWithStackName(construct: Construct, maxUniqueIdLength: number = 256): string {
+    const node = Node.of(construct);


Maybe also add a comment to uniqueId to say something like "uniqueId is not unique if multiple copies of the stack are deployed. Prefer using uniqueIdWithStackName()."

Added this to the comments.

rix0rrr · 2022-03-16T11:30:26Z

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

+  public static uniqueIdWithStackName(construct: Construct, maxUniqueIdLength: number = 256): string {
+    const node = Node.of(construct);
+    const components = node.scopes.slice(1).map(component => {
+      if (Stack.isStack(component) && !unresolved(component.stackName)) {


If stackName is unresolved, this function doesn't help anymore, but feels like we should be able to do something here anyway (by using { Ref: 'AWS::StackName' }). We also don't need the entire construct path anymore, just the path below the stack.

How about we do something like:

const stack = Stack.of(node); const components = scopesUnder(node, stack); // Only return the ids of nodes underneath the given stack const stackNameLength = Token.isUnresolved(stack.stackName) ? 128 : stack.StackName.length; const uniqueId = components.length > 0 ? makeUniqueId(components) : ''; const startIndex = Math.max(0, uniqueId.length - (maxUniqueIdLength) - stackNameLength - 1); return `${stack.stackName}-${uniqueId.substring(startIndex)}`;

In fact we should have something similar to this in calculating CfnOutput export names

Correct me if I'm wrong but the only case where a stack name can be unresolved is in a nested stack. When I tested creating stacks with names that were unresolved, I got the error message when running cdk synth:

/Volumes/workplace/experiments/node_modules/@aws-cdk/core/lib/stack.ts:376 throw new Error(`Stack name must match the regular expression: ${VALID_STACK_NAME_REGEX.toString()}, got '${this.stackName}'`);

With nested stacks, the StackName of the top level stack will still be used as the first part of the uniqueId and then the nested stack ID will be used so they will be unique and no unresolved tokens will be used.

I'll add extra test cases to make sure I'm not working on failed assumptions.

the only case where a stack name can be unresolved is in a nested stack

I might be being overly cautious here. I guess I was thinking of the case where a user would overload stackName to return { Ref: 'AWS::StackName' }. We might say: "that's not allowed", and that might be fair. We can also write code that would be robust against that. This is not a great argument one way or the other though.

A slightly more concrete issue that would be solved: by including the entire construct path in the unique name, moving the Stack in the construct hierarchy becomes a breaking change. If we take the Stack as "root" of the unique name, it doesn't matter where the Stack lives in the construct hierarchy, and we can move it around without affecting the generated names. That seems preferable.

Example:

┌────┐ ┌────┐ │App │ │App │ └────┘ └────┘ │ │ ┌────┴────┐ ┌────┴────┐ │ │ │ │ ┌────┐ ┌────┐ ┌────┐ ┌────┐ │ A │ │ B │ │ A │ │ B │ └────┘ └────┘ └────┘ └────┘ │ │ │ │ ┌───────┐ ┌───────┐ │ Stack │ │ Stack │ └───────┘ ━━━━━━━━━━━━▶ └───────┘ │ │ │ │ ┌────┐ ┌────┐ │Pipe│ │Pipe│ └────┘ └────┘ │ │ │ │ ┌──────────┐ ┌──────────┐ │ KeyAlias │ │ KeyAlias │ └──────────┘ └──────────┘

If the name of KeyAlias depends on A/Stack/Pipeline/KeyAlias then it will be renamed if we ever move the Stack to B.

If its name just depended on Stack/Pipeline/KeyAlias then moving the stack wouldn't matter.

rix0rrr · 2022-03-16T11:35:36Z

packages/@aws-cdk/cx-api/lib/features.ts

+ *
+ * This new implementation creates a uniqueId for the alias name using the stack name instead of the stack ID.
+ */
+export const CODEPIPELINE_CROSS_ACCOUNT_KEY_ALIAS_NAMED_WITH_STACK_NAME = '@aws-cdk/aws-codepipeline:generateNameForDefaultBucketKeyAlias';


The value of the flag doesn't really describe what the flag does. This is the value most users will see, so it should have a descriptive name as well.

rix0rrr · 2022-03-16T11:35:51Z

packages/@aws-cdk/aws-codepipeline/test/pipeline.test.ts

      });
    });
+
+    testFutureBehavior('cross account key alias is named with stack name instead of ID when feature flag is enabled', { [cxapi.CODEPIPELINE_CROSS_ACCOUNT_KEY_ALIAS_NAMED_WITH_STACK_NAME]: true }, cdk.App, (app) => {


<3 on the extensive testing

rix0rrr · 2022-03-16T11:39:28Z

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

+   * @param maxUniqueIdLength The maximum length of the unique id
+   * @returns a unique id based on the construct path
+   */
+  public static uniqueIdWithStackName(construct: Construct, maxUniqueIdLength: number = 256): string {


I'd love a less mechanical name for this method, and a more intentional one, but I'm struggling to come up with one.

I'm thinking something like stackSafeUniqueId, environmentallyUniqueId, ... not coming up with anything great yet. Do you have a good idea for an alternative?

I like stackSafeUniqueId so I went with that one.

rix0rrr · 2022-03-24T09:43:21Z

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

+  public static uniqueIdWithStackName(construct: Construct, maxUniqueIdLength: number = 256): string {
+    const node = Node.of(construct);
+    const components = node.scopes.slice(1).map(component => {
+      if (Stack.isStack(component) && !unresolved(component.stackName)) {


the only case where a stack name can be unresolved is in a nested stack

I might be being overly cautious here. I guess I was thinking of the case where a user would overload stackName to return { Ref: 'AWS::StackName' }. We might say: "that's not allowed", and that might be fair. We can also write code that would be robust against that. This is not a great argument one way or the other though.

A slightly more concrete issue that would be solved: by including the entire construct path in the unique name, moving the Stack in the construct hierarchy becomes a breaking change. If we take the Stack as "root" of the unique name, it doesn't matter where the Stack lives in the construct hierarchy, and we can move it around without affecting the generated names. That seems preferable.

Example:

┌────┐ ┌────┐ │App │ │App │ └────┘ └────┘ │ │ ┌────┴────┐ ┌────┴────┐ │ │ │ │ ┌────┐ ┌────┐ ┌────┐ ┌────┐ │ A │ │ B │ │ A │ │ B │ └────┘ └────┘ └────┘ └────┘ │ │ │ │ ┌───────┐ ┌───────┐ │ Stack │ │ Stack │ └───────┘ ━━━━━━━━━━━━▶ └───────┘ │ │ │ │ ┌────┐ ┌────┐ │Pipe│ │Pipe│ └────┘ └────┘ │ │ │ │ ┌──────────┐ ┌──────────┐ │ KeyAlias │ │ KeyAlias │ └──────────┘ └──────────┘

If the name of KeyAlias depends on A/Stack/Pipeline/KeyAlias then it will be renamed if we ever move the Stack to B.

If its name just depended on Stack/Pipeline/KeyAlias then moving the stack wouldn't matter.

rix0rrr

Things we talked about:

Only include containing (non-nested) stack and below into the unique id
Cut in the middle if necessary

Maybe like this?


        makeUniqueResourceName(construct, {
            maxLength: 64,
            separator: '-',
            allowedSpecialCharacters: '-.',
        });

skinny85

Looks great, thanks for taking care of this @TheRealAmazonKendra!

Some small optional comments from me, feel free to ignore if you disagree 😉.

skinny85 · 2022-04-01T23:59:54Z

packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts

+    } else {
+      const uniqueId = Names.uniqueId(this);
+      // take the last 256 - (prefix length) characters of uniqueId
+      const startIndex = Math.max(0, uniqueId.length - (maxUniqueIdLength));


I think we can kill the parenthesis here:

Suggested change

const startIndex = Math.max(0, uniqueId.length - (maxUniqueIdLength));

const startIndex = Math.max(0, uniqueId.length - maxUniqueIdLength);

Hmm. There was something there that I deleted and then forgot to delete the parenthesis but I most definitely can't remember what. Fixing it.

skinny85 · 2022-04-02T00:01:23Z

packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts

-    const startIndex = Math.max(0, uniqueId.length - (maxAliasLength - prefix.length));
-    return prefix + uniqueId.substring(startIndex).toLowerCase();
+    const maxUniqueIdLength = maxAliasLength - prefix.length;
+    if (FeatureFlags.of(this).isEnabled(cxapi.CODEPIPELINE_CROSS_ACCOUNT_KEY_ALIAS_STACK_SAFE_UNIQUE_ID)) {


Maybe some comments would be nice here, to explain why is this behind a feature flag, and what are the behaviors in each of the branches (feature flag on/feature flag off)?

skinny85 · 2022-04-02T00:03:29Z

packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts

+      const uniqueId = Names.uniqueId(this);
+      // take the last 256 - (prefix length) characters of uniqueId
+      const startIndex = Math.max(0, uniqueId.length - (maxUniqueIdLength));
+      return prefix + uniqueId.substring(startIndex).toLowerCase();


We're repeating return prefix + <sth>.toLowerCase(); in both branches of the if. Maybe extract a variable, something like suffix: string;, in the block above, then set it to different values in the if-else above, and then put return prefix + suffix.toLowerCase(); after the if, as the last statement of the method?

Updated. Looks much better now.

skinny85 · 2022-04-02T00:05:52Z

packages/@aws-cdk/aws-codepipeline/package.json

    "@aws-cdk/aws-kms": "0.0.0",
    "@aws-cdk/aws-s3": "0.0.0",
    "@aws-cdk/core": "0.0.0",
+    "@aws-cdk/cx-api": "0.0.0",


Doesn't this need to be in peerDependencies now too?

skinny85 · 2022-04-02T00:09:36Z

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

+   * @param maxUniqueIdLength The maximum length of the unique id
+   * @returns a unique id based on the construct path
+   */
+  public static stackSafeUniqueId(construct: Construct, maxUniqueIdLength: number = 256): string {


I actually don't really like this maxUniqueIdLength parameter in this API. Can't the caller just truncate this to whatever length they need? I feel like this makes it inconsistent with nodeUniqueId for relatively little benefit.

Thoughts about removing maxUniqueIdLength from here, and moving this truncating part to the caller?

I disagree. The benefit is huge! Every caller will realistically need to do this. Punting it to the consumer is just pushing work on them for no reason.

The fact that nodeUniqueId didn't have it is a miss, imo.

Then how about fixing that mistake, and adding maxUniqueIdLength to Names.uniqueId() too?

Because right now, we have duplication in handling the max length (we do it once here, and once in the Pipeline class). If we did it in both methods in Names, we could extract the max length handling piece to a private method/function, and get rid of this duplication.

I'm actually looking at deprecating uniqueId since it's not aaaaaactually unique. So, for now I'm not making changes to it, but if we think that it's not viable to actually deprecate it, I'll update it to be consistent. I'd like to do that in a separate PR, though.

From my search, Names.uniqeId() is used 62 times in production code, so deprecating it might be a little harsh (although I'm not saying it's not possible - just making you aware of the numbers here 😉).

Because of the high number of usages, I think we won't get rid of it anytime soon, so adding the maxUniqueIdLength parameter to it makes a lot of sense (in my opinion).

skinny85 · 2022-04-02T00:19:49Z

packages/@aws-cdk/aws-codepipeline/test/pipeline.test.ts

  }
 }
+
+const createPipeline = (scope: Construct, pipelineName?: string, pipelineId: string = 'Pipeline') => {


Also, can you put this function below PipelineStack, where it's used? It's much easier to read a file if it reads top to bottom.

skinny85 · 2022-04-02T00:20:24Z

packages/@aws-cdk/aws-codepipeline/test/pipeline.test.ts

+  context: cdk.App,
+  suffix: string,
+  stackId?: string,
+  pipelineId?: string,
+  undefinedStackName?: boolean,
+  nestedStackId?: string,


Adding readonly to the properties here might be nice (although not required).

skinny85 · 2022-04-02T00:21:04Z

packages/@aws-cdk/aws-codepipeline/test/pipeline.test.ts

+  nestedStackId?: string,
+}
+
+const createPipelineStack = (options: CreatePipelineStackOptions): PipelineStack => {


Can you actually use the function keyword for these? I feel like they're a little easier to discern from simple variable declarations that way.

Sure. I tend to like this notation better but you are right that it's easier to discern with the function keyword.

skinny85 · 2022-04-02T00:22:15Z

packages/@aws-cdk/aws-codepipeline/test/pipeline.test.ts

+  nestedStackId?: string,
+}
+
+const createPipelineStack = (options: CreatePipelineStackOptions): PipelineStack => {


Is this createPipelineStack() helper function really needed? What about just instantiating PipelineStack in the tests directly? I feel like using a construct instead of a function makes the tests read more like idiomatic CDK code.

For example, this test:

const stack = createPipelineStack({ context: app, suffix: 'Name', stackId: 'PipelineStack', }); Template.fromStack(stack).hasResourceProperties(kmsAliasResource, { AliasName: 'alias/codepipeline-actualstacknamepipeline0a412eb5', });

I think would read much better if we created the PipelineStack directly, instead of using the createPipelineStack() helper.

skinny85 · 2022-04-02T00:23:20Z

packages/@aws-cdk/aws-codepipeline/test/pipeline.test.ts

  });
+
+  describe('cross account key alias name tests', () => {
+    const kmsAliasResource = 'AWS::KMS::Alias';


I would just inline this variable, honestly 😛. It's not like this will change.

Yeah, but writing it inline means that VS Code doesn't add the suggestion and then I have to type the whole thing over and over again. I'm far too lazy for that.

…alias name (under feature flag) When multiple copies of the same pipeline are deployed in separate stacks, the alias name for the KMS key is the same, causing the deployment to fail. This change fixes that using the stack name instead of the stack ID to create a unique ID for the alias name. Closes issue #18828

… twice (under feature flag) When multiple copies of the same pipeline are deployed in separate stacks, the alias name for the KMS key is the same, causing the deployment to fail. This hcange fixes that using the stack name instead of the stack ID to create a stack safe uniqueId for the alias name. This fix is behind the following feature flag: @aws-cdk/aws-codepipeline:crossAccountKeyAliasStackSafeUniqueId Fixes issue #18828.

rix0rrr

Looks great! We're missing just one more test:

To assert that the unique resource name is now based on the stackName, instead of the stack construct id. They're the same by default, which is why all the integ tests are showing the same identifiers, formatted slightly differently, and one might be tempted to say this fix doesn't materially change anything--but it does, or at least it should.

We need a test to show that with new Stack(..., { stackName: 'bla' }) the right property is chosen.

mergify · 2022-05-11T19:19:01Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2022-05-11T20:43:41Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: 6da046d
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2022-05-11T20:44:00Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

… twice (under feature flag) (aws#19418) When multiple copies of the same pipeline are deployed in separate stacks, the alias name for the KMS key is the same, causing the deployment to fail. This hcange fixes that using the stack name instead of the stack ID to create a stack safe uniqueId for the alias name. This fix is behind the following feature flag: @aws-cdk/aws-codepipeline:crossAccountKeyAliasStackSafeUniqueId Fixes issue aws#18828. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

github-actions bot added the @aws-cdk/aws-codepipeline Related to AWS CodePipeline label Mar 16, 2022

github-actions bot assigned skinny85 Mar 16, 2022

mergify bot added the contribution/core This is a PR that came from AWS. label Mar 16, 2022

rix0rrr requested changes Mar 16, 2022

View reviewed changes

TheRealAmazonKendra force-pushed the TheRealAmazonKendra/cross-account-keys-fix branch from 918cc33 to e4935f0 Compare March 22, 2022 03:42

TheRealAmazonKendra changed the title ~~fix(codepipeline): use stack name instead of id in cross account key alias name (under feature flag)~~ fix(codepipeline): cannot deploy pipeline stack with crossAccountKeys twice (under feature flag) Mar 22, 2022

TheRealAmazonKendra force-pushed the TheRealAmazonKendra/cross-account-keys-fix branch from e4935f0 to 90dda09 Compare March 22, 2022 03:58

rix0rrr requested changes Mar 24, 2022

View reviewed changes

rix0rrr reviewed Mar 31, 2022

View reviewed changes

skinny85 reviewed Apr 2, 2022

View reviewed changes

skinny85 assigned TheRealAmazonKendra and unassigned skinny85 May 5, 2022

TheRealAmazonKendra added 3 commits May 4, 2022 18:17

addresses feedback from previous version

abfd90b

TheRealAmazonKendra force-pushed the TheRealAmazonKendra/cross-account-keys-fix branch from 90dda09 to abfd90b Compare May 5, 2022 01:19

TheRealAmazonKendra requested a review from rix0rrr May 5, 2022 01:24

TheRealAmazonKendra added 3 commits May 4, 2022 20:07

update snapshot

76c49d2

update snapshots

ea6b8c2

update more snapshots

f2af6a3

TheRealAmazonKendra requested a review from a team May 11, 2022 04:20

Update features.ts

b0ad983

rix0rrr requested changes May 11, 2022

View reviewed changes

rix0rrr added the pr/do-not-merge This PR should not be merged at this time. label May 11, 2022

rix0rrr approved these changes May 11, 2022

View reviewed changes

TheRealAmazonKendra removed the pr/do-not-merge This PR should not be merged at this time. label May 11, 2022

Merge branch 'master' into TheRealAmazonKendra/cross-account-keys-fix

6da046d

mergify bot merged commit 7a6f585 into master May 11, 2022

mergify bot deleted the TheRealAmazonKendra/cross-account-keys-fix branch May 11, 2022 20:43

skinny85 mentioned this pull request Jun 8, 2022

(codepipeline): Cannot deploy multiple copies of the same Pipeline with crossAccountKeys: true #18828

Closed

	const startIndex = Math.max(0, uniqueId.length - (maxUniqueIdLength));
	const startIndex = Math.max(0, uniqueId.length - maxUniqueIdLength);

Conversation

TheRealAmazonKendra commented Mar 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gitpod-io bot commented Mar 16, 2022

Uh oh!

rix0rrr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rix0rrr Mar 24, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rix0rrr Mar 24, 2022 • 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!

rix0rrr left a comment

Choose a reason for hiding this comment

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skinny85 Apr 8, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

TheRealAmazonKendra commented Mar 16, 2022 •

edited

Loading

rix0rrr Mar 24, 2022 •

edited

Loading

rix0rrr Mar 24, 2022 •

edited

Loading

skinny85 Apr 8, 2022 •

edited

Loading

rix0rrr left a comment •

edited

Loading