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

aws_ecs.AsgCapacityProvider: CDK should be smart enough to auto-name ASG capacity provider something valid if stack name starts with illegal keyword #29151

Closed
JellyKid opened this issue Feb 17, 2024 · 6 comments · Fixed by #29235
Assignees
Labels
@aws-cdk/aws-ecs Related to Amazon Elastic Container bug This issue is a bug. effort/medium Medium work item – several days of effort p2

Comments

@JellyKid
Copy link

Describe the bug

If your stack name begins with "aws", "ecs", or "fargate", you will receive the following error when creating an ASG capacity provider if you don't specify a capacity provider name.

"Invalid request provided: CreateCapacityProvider error: The specified capacity provider name is invalid. Up to 255 characters are allowed, including letters (
upper and lowercase), numbers, underscores, and hyphens. The name cannot be prefixed with "aws", "ecs", or "fargate". Specify a valid name and try again."

Expected Behavior

I expected the ASG capacity provider to be created without issue no matter what the stack name is.

Current Behavior

I described it pretty well above, but if you have any specific questions. I can't think of anything more relevant that would apply here.

Reproduction Steps

Init a new stack with the CDK in a directory that starts with "aws-"

Create an ASG capacity provider, but don't specify the "capacityProviderName" property so the CDK will autogenerate a name for you

Deploy it

Possible Solution

Maybe don't let a project be initialized with illegal keywords. You could also do a simple regex check when creating the ASG capacity provider to see if the autogenerated name contains those illegal keywords and either remove them or add some other word that is valid.

Additional Information/Context

If we are going to ban the words aws across the board then the CDK should specify that so a linter can catch things like that.

CDK CLI Version

2.128.0 (build d995261)

Framework Version

No response

Node.js Version

v20.10.0

OS

Linux fedora 6.7.4-200.fc39.x86_64 #1 SMP PREEMPT_DYNAMIC Mon Feb 5 22:21:14 UTC 2024 x86_64 GNU/Linux

Language

TypeScript

Language Version

No response

Other information

No response

@JellyKid JellyKid added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Feb 17, 2024
@github-actions github-actions bot added the @aws-cdk/aws-ecs Related to Amazon Elastic Container label Feb 17, 2024
@JellyKid
Copy link
Author

I believe this also happens for log groups? The only reason I came upon this issue was because I was working on another issue related to CloudFormation, so I made an example stack to show the behavior to an AWS employee.

https://github.com/JellyKid/aws-cloudformation-bug

The problem is that the CDK doesn't give you a clean way to rename the stack/project. So if you've already spent a bunch of time building something, it's going to be a major pain to try and rename everything. Shouldn't the CDK be smart enough to know what the naming requirements are for each product?

@msambol
Copy link
Contributor

msambol commented Feb 18, 2024

@JellyKid The CDK does catch erroneously-named capacity providers, per this PR, before the stack gets deployed. You can also not specify the name and the CDK will generate a name. It is not typical of the CDK to change the name of something when you've specified a different name in your code. Hope this helps.

@pahud
Copy link
Contributor

pahud commented Feb 20, 2024

it would be tested if the name is provided otherwise auto generated by CFN, which would violate if your stack name is having those prefixes.

You can work it around like this

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

    new ecs.AsgCapacityProvider(this, 'Provider', {
      capacityProviderName: this.getProviderName(),
      autoScalingGroup: new AutoScalingGroup(this, 'ASG', {
        vpc: getDefaultVpc(this),
        machineImage: ec2.MachineImage.latestAmazonLinux2023(),
        instanceType: ec2.InstanceType.of(ec2.InstanceClass.T3, ec2.InstanceSize.LARGE),
      })
    })
  }
  private getProviderName(): string | undefined {
    if (!(/^(?!aws|ecs|fargate).+/gm.test(Stack.of(this).stackName))) {
      return `cp-${Names.uniqueResourceName(this, { allowedSpecialCharacters: '-', maxLength: 24 })}`
    } else {
      return undefined
    }
  }
}

And I agree we should have a PR for it.

@pahud pahud added p2 effort/medium Medium work item – several days of effort and removed needs-triage This issue or PR still needs to be triaged. labels Feb 20, 2024
@pahud pahud self-assigned this Feb 20, 2024
@msambol
Copy link
Contributor

msambol commented Feb 20, 2024

@pahud ill do a PR for this

@pahud
Copy link
Contributor

pahud commented Feb 21, 2024

@msambol 👍 Thank you!

@mergify mergify bot closed this as completed in #29235 Mar 1, 2024
mergify bot pushed a commit that referenced this issue Mar 1, 2024
#29235)

Closes #29151.

----

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

github-actions bot commented Mar 1, 2024

⚠️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/aws-ecs Related to Amazon Elastic Container bug This issue is a bug. effort/medium Medium work item – several days of effort p2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants