(aws-cdk): asset bundling skipped when using --exclusively with custom stack name

[stevehodgkiss]

Describe the bug

This is the same as or very similar to #12898.

# cdk ls --long
- id: development/CdkBundlingIssueStack
  name: CustomStackName
  environment:
    account: unknown-account
    region: unknown-region
    name: aws://unknown-account/unknown-region

When using a custom stack name, a stage and cdk deploy "development/*" --exclusively:

• asset bundling is skipped.
• on deploy, the entire repo is bundled into a zip file and uploaded.

Expected Behavior

Asset bundling shouldn't be incorrectly skipped.

There's a separate issue of if it is skipped, the entire repo shouldn't be bundled into a zip file and uploaded.

Current Behavior

Asset bundling is skipped, then the entire repo is uploaded instead of the zip file.

Reproduction Steps

CDK App:

#!/usr/bin/env node
import 'source-map-support/register';
import * as cdk from 'aws-cdk-lib';
import { Stage } from 'aws-cdk-lib';
import { Stack, StackProps } from 'aws-cdk-lib';
import { NodejsFunction } from 'aws-cdk-lib/aws-lambda-nodejs';
import { Construct } from 'constructs';

class CdkBundlingIssueStack extends Stack {
  constructor(scope: Construct, id: string, props?: StackProps) {
    super(scope, id, props);

    new NodejsFunction(this, 'lambda', {})
  }
}

class MyStage extends Stage {
  constructor(scope: Construct, id: string) {
    super(scope, id)

    new CdkBundlingIssueStack(this, 'CdkBundlingIssueStack', {
      stackName: 'CustomStackName'
    });
  }
}

const app = new cdk.App();

new MyStage(app, 'development')

# cdk ls --long
- id: development/CdkBundlingIssueStack
  name: CustomStackName
  environment:
    account: unknown-account
    region: unknown-region
    name: aws://unknown-account/unknown-region

• yarn cdk deploy "development/*" --require-approval never bundles the asset, and the stack is selected for deploy and works as expected.
• yarn cdk deploy "development/*" --require-approval never --exclusively skips bundling, bundles the entire repo into an assets zip file instead of the lambda.

Possible Solution

I think the issue is the pattern is matched against stackName in @aws-cdk/core to determine whether to bundle assets, whereas it's matched against hierarchicalId in aws-cdk to get the list of stacks to deploy. That's how bundling is skipped yet a deploy still happens. heirarchicalId is the stack logical name (development/CdkBundlingIssueStack), whereas stackName in this case is CustomStackName.

I think the fix is to use hierarchicalId in @aws-cdk/core to make the pattern matching consistent.

Additional Information/Context

No response

CDK CLI Version

2.20.0

Framework Version

No response

Node.js Version

16.14.2

OS

MacOS

Language

Typescript

Language Version

No response

Other information

Relevant log lines with --verbose show the entire repo being bundled and uploaded, instead of the zip file. This is after asset bundling is skipped when it shouldn't be:

[0%] check: Check s3://cdk-hnb659fds-assets-12345-us-east-1/5606a614b8f65f4011c41e902adde89ca8048b8e2208efbefbb5645c64ac40d5.zip
[0%] upload: Upload s3://cdk-hnb659fds-assets-12345-us-east-1/69247885b0bf9b5b79e602c4c09ccc4c69154bdf35e933d24991cdcced8fb5ab.json
[0%] build: Zip /Users/steve/src/cdk-bundling-issue -> /Users/steve/src/cdk-bundling-issue/cdk.out/assembly-development/.cache/5606a614b8f65f4011c41e902adde89ca8048b8e2208efbefbb5645c64ac40d5.zip

[corymhall]

@stevehodgkiss it looks like you have a PR to fix this. Someone from the team should pick up the PR
to review.

[xorph]

Any ETA for this issue? My team is currently migrating multiple services to CDK and this is a blocker in some cases.

[github-actions]

This issue has received a significant amount of attention so we are automatically upgrading its priority. A member of the community will see the re-prioritization and provide an update on the issue.

[github-actions]

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

[corymhall]

@stevehodgkiss we had to revert the PR because it caused bundling to be skipped whenever the stack was part of a stage. The comment mentions that the stack name was intentionally converted from Stage/Stack to Stage-Stack

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

Line 1171 in 1fabd50

// bundlingStacks is of the form `Stage/Stack`, convert it to `Stage-Stack` before comparing to stack name

Done in this PR #17210

The correct fix may be to keep node.path and continue doing the conversion, but I haven't looked into it too much. At a minimum we need to add a test here to ensure that bundling is not skipped when a stack is part of a stage. We can also validate that the asset is not empty.

[stevehodgkiss]

@corymhall ah, sorry about that. The issue seems to be related to how minimatch works when matching against strings containing / with a pattern of *.

❯ npx ts-node
> const minimatch = require('minimatch')
'use strict'

With * as the pattern, it doesn't match values containing / (i.e. stacks under a stage). This seems to be the underlying reason for the regression, and it's likely the reason for converting / to - in the first place. A test to verify this behaviour (* matches stacks under a stage like stage-name/stack-name) should also be added to prevent this regression in future, as you mentioned.

> minimatch('stage/stack', '*')
false

Converting to - gets around this:

> minimatch('stage-stack', '*')
true

Matches specifying stage/* appear to work:

> minimatch('stage/stack', 'stage/*')
true

Perhaps, as you mention, the correct fix is to convert / to - for both the supplied pattern and the node path (previously stackName, which would have had this conversion done automatically since / isn't supported in cloudformation stack names):

bundlingStacks.some(pattern => minimatch(
  this.node.path.replace('/', '-'),
  pattern.replace('/', '-'),
));

> minimatch('stage-stack', 'stage-*')
true

Another option might be to use globstar (**) as the default pattern instead of *:

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

Line 1169 in d7ac3bb

const bundlingStacks: string[] = this.node.tryGetContext(cxapi.BUNDLING_STACKS) ?? ['*'];

https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk/lib/api/cxapp/exec.ts#L43-L44

> minimatch('stage/stack', '**')
true

[stevehodgkiss]

The CDK CLI doesn't .replace('/', -) when using minimatch. I think it'd be best to do the same - match against the same input & pattern as the CLI to prevent edge cases where the CLI matches the stack but bundlingRequired() doesn't. A contrived example where replacing with - would cause different matching behaviour:

> minimatch('development-one/two'.replace('/', '-'), 'development/*'.replace('/', '-'))
true
> minimatch('development-one/two', 'development/*')
false

Using globstar ** as the default pattern is one option to avoid having to use .replace('/', '-') (a default pattern is set in bundlingRequired and in exec.ts).

When a pattern isn't supplied on the CLI, minimatch isn't used to filter the stack list. Perhaps there's another way to simply return true if a pattern isn't supplied on the CLI (although BUNDLING_STACKS is currently always set to ['*'] regardless of whether a pattern is supplied as a CLI arg).

[stevehodgkiss]

I've created a draft PR that aims to address the issue the first PR had. Interested in your feedback @corymhall @kaizencc. #21248

[github-actions]

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.