fix(core): use node.path in skip bundling check for consistency with cdk deploy CLI #19950

stevehodgkiss · 2022-04-18T02:28:09Z

The pattern given to aws-cdk on the CLI (cdk deploy $pattern) is matched against stack.hierarchicalId (i.e. displayName, node.path) [1] [2] [3].

The same pattern is given to the CDK app (as bundlingStacks) but is matching against stackName, causing potential for bundling to be skipped under certain scenarios (i.e. using --exclusively with custom stack names).

To make them consistent, the skip bundling check has been changed to match against the same value as cdk deploy $pattern (node.path/hierarchicalId/displayName) when deciding whether to bundle an asset. Making them consistent ensures necessary stacks are bundled, avoiding the issue of uploading the entire project directory since the asset wasn't bundled.

fixes #19927

[1]

aws-cdk/packages/aws-cdk/lib/api/cxapp/cloud-assembly.ts

Line 138 in 1d02704

if (minimatch(stack.hierarchicalId, pattern)) {

[2]

aws-cdk/packages/@aws-cdk/cx-api/lib/cloud-artifact.ts

Lines 143 to 145 in 6cca62d

    
           public get hierarchicalId(): string { 
        
             return this.manifest.displayName ?? this.id; 
        
           }

[3]

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

Line 66 in 6cca62d

displayName: stack.node.path,

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

gitpod-io · 2022-04-18T02:28:18Z

moltar · 2022-04-28T21:41:43Z

Not sure if this PR handles this case, but another issue, in addition to #12898 (fixed) and #19927 (open) is related to this issue: #16830

Because of #16830, I need to prefix the stack name with the pipeline ID (otherwise it will try to destroy stuff).

Example:

cdk deploy -e "PipelineName-V1/AD-FOO/QST"

This then tries to bundle the entire root dir (as described in my comment here: #12898 (comment)).

stevehodgkiss · 2022-05-02T01:44:01Z

@moltar the part about logical ID name changes when in a pipeline or not isn't related to this. The part where deployment with cdk deploy -e "PipelineName-V1/AD-FOO/QST" uploads the entire project dir is likely related to this issue (it's probably not bundling the selected stack on synth, cdk deploy output would indicate if that's the case), and that should be fixed with the changes in this PR.

moltar · 2022-05-02T10:49:07Z

the part about logical ID name changes when in a pipeline or not isn't related to this

Right, did not mean to imply that it was, sorry. 😁 Meant that it was "softly" related in a sense, that pipeline prefix bug causes another corner case, related to this issue.

The part where deployment with cdk deploy -e "PipelineName-V1/AD-FOO/QST" uploads the entire project dir is likely related to this issue

That's what I meant. From my investigation before, it was about the naming. I posted my findings in that same issue.

Basically, it was the difference between PipelineName-V1/AD-FOO/QST and PipelineName-V1-AD-FOO-QST. One place was generating the names with dashes, and the other with slashes, and when it was comparing the two names, it did not match, and then somehow that causes it not to find the resource, and then it decides to bundle the root dir. I don't remember the full details now, but the / vs - I remember being the root cause.

stevehodgkiss · 2022-05-02T11:32:17Z

Basically, it was the difference between PipelineName-V1/AD-FOO/QST and PipelineName-V1-AD-FOO-QST. One place was generating the names with dashes, and the other with slashes, and when it was comparing the two names, it did not match, and then somehow that causes it not to find the resource, and then it decides to bundle the root dir. I don't remember the full details now, but the / vs - I remember being the root cause.

Yep, if bundling is skipped for a stack when it was selected for deploy in the CLI, a downstream affect that should probably be addressed is it uploads the entire root directory of the project. It would be good to track down where this happens and fail with an "asset not found" message. The change in this PR should prevent it from happening by matching against the same value (node.path) in both places.

The inconsistency exists because the value used for pattern matching in the CLI (on cdk deploy $pattern) and the bundlingRequired check in core is different:

CLI (cdk deploy) uses hierarchicalId (node.path):

aws-cdk/packages/aws-cdk/lib/api/cxapp/cloud-assembly.ts

Line 138 in 1d02704

if (minimatch(stack.hierarchicalId, pattern)) {
bundlingRequired in core uses stack.stackName: https://github.com/aws/aws-cdk/blob/v2-main/packages/@aws-cdk/core/lib/stack.ts#L1159-L1167

This mismatch is what causes bundling to be skipped and consequently the project dir to be uploaded instead.

moltar · 2022-05-02T11:36:48Z

It would be good to track down where this happens and fail with an "asset not found" message.

Yes, definitely a good idea. I think uploading the entire dir is not only annoying but could be a security concern.

miekassu · 2022-05-26T00:58:19Z

any progress with this?

stevehodgkiss · 2022-05-26T01:23:49Z

any progress with this?

Waiting for CDK team to review/merge...

kaizencc · 2022-07-12T12:14:45Z

@stevehodgkiss this will need to target the main branch, not v1-main. Do you have any bandwidth to update this? If not, I will at some point, but that will likely result in me nuking this current PR.

The pattern given to aws-cdk on the CLI is matched against `hierarchicalId` (i.e. displayName, node.path) [1] [2] [3]. The same pattern is given to the CDK app but was matching against `stackName`, causing potential for bundling to be skipped under certain scenarios (i.e. using --exclusively with custom stack names). To make them consistent, the skip bundling check has been changed to use the same value (node.path/hierarchicalId/displayName) as aws-cdk when deciding whether to bundle an asset. fixes aws#19927 [1] https://github.com/aws/aws-cdk/blob/1d0270446b3effa6b8518de3c7d76f0c14e626c5/packages/aws-cdk/lib/api/cxapp/cloud-assembly.ts#L138 [2] https://github.com/aws/aws-cdk/blob/6cca62db88866479f2b1d4f52b30beab3d78aeb2/packages/@aws-cdk/cx-api/lib/cloud-artifact.ts#L143-L145 [3] https://github.com/aws/aws-cdk/blob/6cca62db88866479f2b1d4f52b30beab3d78aeb2/packages/@aws-cdk/core/lib/stack-synthesizers/_shared.ts#L66

Pull request has been modified.

stevehodgkiss · 2022-07-12T12:30:30Z

@kaizencc yep, I've rebased and updated the merge target to main.

mergify · 2022-07-13T18:32:58Z

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

…-logic-as-aws-cdk

mergify · 2022-07-13T19:16:47Z

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 · 2022-07-13T19:16:54Z

AWS CodeBuild CI Report

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

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

…cy with cdk deploy CLI (#19950)" This reverts commit 5cff2d9.

… for consistency with cdk deploy CLI" (#21174) #19950 breaks users who use bundling inside a pipeline stack. Previously we converted `Stage/Stack` to `Stage-Stack` and left a comment as to why that was necessary. This comment was both insufficient and disregarded, which resulted in different behavior between the bundling logic and skip bundling logic. The asset path for skip bundling would not longer be the same as the asset path when bundling, so the asset would not be found.

… for consistency with cdk deploy CLI" (aws#21174) aws#19950 breaks users who use bundling inside a pipeline stack. Previously we converted `Stage/Stack` to `Stage-Stack` and left a comment as to why that was necessary. This comment was both insufficient and disregarded, which resulted in different behavior between the bundling logic and skip bundling logic. The asset path for skip bundling would not longer be the same as the asset path when bundling, so the asset would not be found.

…m stack name (#21248) Fixes #19927 [A previous PR](#19950) attempted this fix, but it caused another issue where the default pattern (`['*']`, used on cdk synth/deploy when a pattern isn't supplied) doesn't match stacks under a stage (`stage/stack`), and that caused bundling to be skipped. The default pattern worked (matched all stacks) previously because stackName was being used (and there are no `/` in stack names). See [this comment](#19927 (comment)) in the issue for more details on this. The first commit 5556b60 adds special handling to the default pattern so that it always returns true (bypassed minimatch which would return false if a stack is under a stage). The second commit a9a48a0 aims to remove the duplication of this pattern across the codebase, by defaulting `BUNDLING_STACKS` to `undefined` and returning true in bundlingRequired if that's the case. ---- ### All Submissions: * [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

aws-cdk-automation requested a review from a team April 18, 2022 02:28

github-actions bot added bug This issue is a bug. p2 labels Apr 18, 2022

github-actions bot added the effort/small Small work item – less than a day of effort label May 2, 2022

stevehodgkiss changed the title ~~fix(core): use node.path in skip bundling check for consistency with CLI~~ fix(core): use node.path in skip bundling check for consistency with cdk deploy CLI May 2, 2022

comcalvi previously approved these changes Jul 7, 2022

View reviewed changes

kaizencc changed the base branch from v1-main to main July 12, 2022 12:12

github-actions bot added p1 and removed p2 labels Jul 12, 2022

kaizencc changed the base branch from main to v1-main July 12, 2022 12:13

kaizencc added the pr/do-not-merge This PR should not be merged at this time. label Jul 12, 2022

stevehodgkiss changed the base branch from v1-main to main July 12, 2022 12:26

stevehodgkiss force-pushed the stevehodgkiss/fix-bundling-check-to-use-same-logic-as-aws-cdk branch from c00ff00 to 60e5bbd Compare July 12, 2022 12:26

comcalvi removed the pr/do-not-merge This PR should not be merged at this time. label Jul 13, 2022

comcalvi approved these changes Jul 13, 2022

View reviewed changes

Merge branch 'main' into stevehodgkiss/fix-bundling-check-to-use-same…

d7bede8

…-logic-as-aws-cdk

mergify bot merged commit 5cff2d9 into aws:main Jul 13, 2022

flaminscotsman mentioned this pull request Jul 15, 2022

(core): Stack assets are no longer bundled if contained in a stage #21162

Closed

kaizencc added a commit that referenced this pull request Jul 15, 2022

Revert "fix(core): use node.path in skip bundling check for consisten…

652b2ec

…cy with cdk deploy CLI (#19950)" This reverts commit 5cff2d9.

kaizencc mentioned this pull request Jul 15, 2022

revert(core): revert "fix(core): use node.path in skip bundling check for consistency with cdk deploy CLI" #21174

Merged

stevehodgkiss mentioned this pull request Jul 20, 2022

fix(core): asset bundling skipped when using --exclusively with custom stack name #21248

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(core): use node.path in skip bundling check for consistency with cdk deploy CLI #19950

fix(core): use node.path in skip bundling check for consistency with cdk deploy CLI #19950

stevehodgkiss commented Apr 18, 2022 •

edited

gitpod-io bot commented Apr 18, 2022

moltar commented Apr 28, 2022

stevehodgkiss commented May 2, 2022

moltar commented May 2, 2022 •

edited

stevehodgkiss commented May 2, 2022

moltar commented May 2, 2022

miekassu commented May 26, 2022

stevehodgkiss commented May 26, 2022

kaizencc commented Jul 12, 2022 •

edited

stevehodgkiss commented Jul 12, 2022

mergify bot commented Jul 13, 2022

mergify bot commented Jul 13, 2022

aws-cdk-automation commented Jul 13, 2022

	public get hierarchicalId(): string {
	return this.manifest.displayName ?? this.id;
	}

fix(core): use node.path in skip bundling check for consistency with cdk deploy CLI #19950

fix(core): use node.path in skip bundling check for consistency with cdk deploy CLI #19950

Conversation

stevehodgkiss commented Apr 18, 2022 • edited

All Submissions:

Adding new Unconventional Dependencies:

New Features

gitpod-io bot commented Apr 18, 2022

moltar commented Apr 28, 2022

stevehodgkiss commented May 2, 2022

moltar commented May 2, 2022 • edited

stevehodgkiss commented May 2, 2022

moltar commented May 2, 2022

miekassu commented May 26, 2022

stevehodgkiss commented May 26, 2022

kaizencc commented Jul 12, 2022 • edited

stevehodgkiss commented Jul 12, 2022

mergify bot commented Jul 13, 2022

mergify bot commented Jul 13, 2022

aws-cdk-automation commented Jul 13, 2022

AWS CodeBuild CI Report

stevehodgkiss commented Apr 18, 2022 •

edited

moltar commented May 2, 2022 •

edited

kaizencc commented Jul 12, 2022 •

edited