Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

core: Stage name check allows underscores ('_') and periods ('.') while stack name does not allow them #23098

Open
yuyokk opened this issue Nov 26, 2022 · 2 comments
Labels
@aws-cdk/core Related to core CDK functionality bug This issue is a bug. effort/small Small work item – less than a day of effort p2

Comments

@yuyokk
Copy link
Contributor

yuyokk commented Nov 26, 2022

Describe the bug

The following code

pipeline.addStage(new AppStage(this, "Stage per acme.example.com"));

results into the following synth error

Error: invalid stage name "Stage per acme.example.com". Stage name must start with a letter and contain only alphanumeric characters, hypens ('-'), underscores ('_') and periods ('.')

According to error message I can use underscores and periods. My next try with the code

pipeline.addStage(new AppStage(this, "Stage_per_acme.example.com"));

results into the following synth error

Error: Stack name must match the regular expression: /^[A-Za-z][A-Za-z0-9-]*$/, got 'Stage_per_acme.example.com-HelloCdkStack'

Expected Behavior

Provide clear error message what characters are allowed for stage id.

Current Behavior

Error message reads that underscores and periods are allowed for stage id but synth returns errors for those.

Reproduction Steps

See https://github.com/yuyokk/aws-cdk-stage-name-bug/blob/main/lib/hello-cdk-stack.ts

// lib/hello-cdk-stack.ts

import { Stack, StackProps, Stage, StageProps } from "aws-cdk-lib";
import {
  CodePipeline,
  CodePipelineSource,
  ShellStep,
} from "aws-cdk-lib/pipelines";
import { Construct } from "constructs";

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

    const pipeline = new CodePipeline(this, "Pipeline", {
      crossAccountKeys: true,
      synth: new ShellStep("Synth", {
        input: CodePipelineSource.gitHub("yuyokk/tenant-core", "main"),
        commands: ["npm ci", "npm run build", "npx cdk synth"],
      }),
    });

    pipeline.addStage(new AppStage(this, "Stage per acme.example.com"));
    // pipeline.addStage(new AppStage(this, "Stage_per_acme.example.com"));
  }
}

class AppStage extends Stage {
  constructor(scope: Construct, id: string, props?: StageProps) {
    super(scope, id, props);

    new HelloCdkStack(this, "HelloCdkStack");
  }
}

class HelloCdkStack extends Stack {
  constructor(scope: Construct, id: string, props?: StackProps) {
    super(scope, id, props);
  }
}
// bin/hello-cdk.ts

#!/usr/bin/env node
import "source-map-support/register";
import * as cdk from "aws-cdk-lib";
import { PipelineStack } from "../lib/hello-cdk-stack";

const app = new cdk.App();
new PipelineStack(app, "HelloCdkStack");

Possible Solution

Fix stage name check to be consistent with stack id checks.

PR for a possible fix #23089

Additional Information/Context

No response

CDK CLI Version

2.51.1

Framework Version

No response

Node.js Version

v16.18.1

OS

macOS 13.0.1

Language

Typescript

Language Version

No response

Other information

No response

@yuyokk yuyokk added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Nov 26, 2022
@github-actions github-actions bot added the @aws-cdk/core Related to core CDK functionality label Nov 26, 2022
@peterwoodworth
Copy link
Contributor

I've left a comment on the PR, but I'll comment here too. Thanks for submitting the bug report and PR! I've been able to confirm that your reproduction code is running into errors. We'll need to make sure there are no breaking changes however, so thanks for your patience during the review process 🙂

@peterwoodworth peterwoodworth added p2 effort/small Small work item – less than a day of effort and removed needs-triage This issue or PR still needs to be triaged. labels Nov 29, 2022
@mrgrain
Copy link
Contributor

mrgrain commented Dec 19, 2022

The root cause here is that the second parameter for Stage and Stack are Construct Ids. We don't normally validated the value of a Construct Id directly, but for some reason we do for Stage. You can easily check this for the Stack:

// this throws error
new HelloCdkStack(app, "STACK per acme.example.com");

// this works
new HelloCdkStack(app, "STACK per acme.example.com", {
  stackName: "hello",
});

The validation expressions for the name of both Stack & Stage appear to be rooted in the actual limitation of the services, which makes sense.

There's been some work quite recently to make the stage name configurable independently of the Id, but it missed handling validation.


tl;dr This would be a breaking change and constrain valid uses cases. Maybe the best we can do is improve the error messages around this.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
@aws-cdk/core Related to core CDK functionality bug This issue is a bug. effort/small Small work item – less than a day of effort p2
Projects
None yet
Development

No branches or pull requests

4 participants