Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

core: logical ID validation regex is inconsistant with cloudformation specification #26075

Closed
ashwin300 opened this issue Jun 21, 2023 · 6 comments · Fixed by #26821
Closed
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 good first issue Related to contributions. See CONTRIBUTING.md p2

Comments

@ashwin300
Copy link

Describe the bug

The CloudFormation documentation indicates that logical ID's must be only alphanumeric, yet the regex in the logical ID validation file is /^[A-Za-z][A-Za-z0-9]{1,254}$/. This causes logical IDs such as 030ffab42b5f448fb39750B452 to fail even though they are valid according to the CloudFormation standard.

Expected Behavior

Logical IDs such as 030ffab42b5f448fb39750B452 to be considered valid to the CDK when synthesizing a stack

Current Behavior

Logical IDs such as 030ffab42b5f448fb39750B452 are not considered valid when synthesizing a stack.

Error: Resolution error: Logical ID must adhere to the regular expression: /^[A-Za-z][A-Za-z0-9]{1,254}$/, got '030ffab42b5f448fb39750B452'.

Reproduction Steps

  1. initialize sample cdk app
  2. in the stack, add the following code:
import * as cdk from 'aws-cdk-lib';
import { Construct } from 'constructs';
import * as sqs from 'aws-cdk-lib/aws-sqs';

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

    // The code that defines your stack goes here

    // example resource
    const queue = new sqs.Queue(this, '030ffab42b5f448fb39750B452', {
      visibilityTimeout: cdk.Duration.seconds(300)
    });
  }
}

run cdk synth

Possible Solution

Change the logical ID validation regex to fit CloudFormation documentation, or change the documentation to reflect the regex validation.

Additional Information/Context

No response

CDK CLI Version

2.85.0 (build 4e0d726)

Framework Version

No response

Node.js Version

v14.21.3

OS

Linux

Language

Typescript

Language Version

No response

Other information

No response

@ashwin300 ashwin300 added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jun 21, 2023
@github-actions github-actions bot added the @aws-cdk/core Related to core CDK functionality label Jun 21, 2023
@peterwoodworth peterwoodworth added p1 effort/small Small work item – less than a day of effort needs-review and removed needs-triage This issue or PR still needs to be triaged. labels Jun 21, 2023
@peterwoodworth
Copy link
Contributor

Thanks for reporting this, I think it's likely that CloudFormation used to have this constraint, but does not anymore. We'll see what we can do about this

@peterwoodworth
Copy link
Contributor

I just tried deploying through the console and CLI with a number at the beginning, and I'm getting this error:

An error occurred (ValidationError) when calling the CreateStack operation: 1 validation error detected: Value '0NameWithNumber' at 'stackName' failed to satisfy constraint: Member must satisfy regular expression pattern: [a-zA-Z][-a-zA-Z0-9]*

So I think this is properly constrained by CDK

@peterwoodworth peterwoodworth added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Jun 28, 2023
@ashwin300
Copy link
Author

The regex applies to stack names, however, the same regex is being used for resource names, which can have a number at the beginning.

@peterwoodworth
Copy link
Contributor

Thanks... don't know why I read stack ID here

@peterwoodworth
Copy link
Contributor

Verified you can override the logical ID to deploy successfully, we should be good to accept a PR with this

@peterwoodworth peterwoodworth added good first issue Related to contributions. See CONTRIBUTING.md p2 and removed p1 response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. labels Jun 28, 2023
@mergify mergify bot closed this as completed in #26821 Aug 22, 2023
mergify bot pushed a commit that referenced this issue Aug 22, 2023
The resource logical ID validation regex (`VALID_LOGICALID_REGEX`) is updated to be consistent with the [CloudFormation documentation](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/resources-section-structure.html) which says that logical IDs are alphanumeric.

Closes #26075.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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 good first issue Related to contributions. See CONTRIBUTING.md p2
Projects
None yet
3 participants