fix(autoscaling): `AutoScalingGroup` requireImdsv2 with launchTemplate or mixedInstancesPolicy throws unclear error by shinebleu · Pull Request #32220 · aws/aws-cdk

shinebleu · 2024-11-20T20:38:13Z

Issue #27586

The above issue is already marked as closed, but there is some discussion in it about an additional edge case that this pull request fixes.

Reason for this change

When defining an AutoScalingGroup with both the requireImdsv2 prop and the launchTemplate prop, the error message is not clear about the problem.

    // example asg definition
    const asg = new AutoScalingGroup(this, "myasg", {
      vpc,
      launchTemplate: lt,
      requireImdsv2: true,
    });

TypeError: Cannot read properties of undefined (reading 'node')
    at AutoScalingGroupRequireImdsv2Aspect.visit (~/git/cdkApp2/node_modules/aws-cdk-lib/aws-autoscaling/lib/aspects/require-imdsv2-aspect.js:1:715)
    at recurse (~/git/cdkApp2/node_modules/aws-cdk-lib/core/lib/private/synthesis.js:2:2236)
    at recurse (~/git/cdkApp2/node_modules/aws-cdk-lib/core/lib/private/synthesis.js:2:2600)
    at recurse (~/git/cdkApp2/node_modules/aws-cdk-lib/core/lib/private/synthesis.js:2:2600)
    at invokeAspects (~/git/cdkApp2/node_modules/aws-cdk-lib/core/lib/private/synthesis.js:2:1860)
    at synthesize (~/git/cdkApp2/node_modules/aws-cdk-lib/core/lib/private/synthesis.js:1:1473)
    at App.synth (~/git/cdkApp2/node_modules/aws-cdk-lib/core/lib/stage.js:1:2382)
    at process.<anonymous> (~/git/cdkApp2/node_modules/aws-cdk-lib/core/lib/app.js:1:1767)
    at Object.onceWrapper (node:events:632:26)
    at process.emit (node:events:517:28)

It is not clear from the error that requireImdsv2 should be set in the provided launchTemplate itself rather than the AutoScalingGroup.

The error occurs because setting requireImdsv2 on the AutoScalingGroup adds the aspect AutoScalingGroupRequireImdsv2Aspect to it, which expects there to be a child node called either 'LaunchConfig' or 'LaunchTemplate' depending on a feature flag. This child node is only set in the AutoScalingGroup when neither launchTemplate nor mixedInstancesPolicy props are provided.

Description of changes

Add the requireImdsv2 prop to the verifyNoLaunchConfigPropIsGiven method, which throws errors when certains props are set at the same time as launchTemplate or mixedInstancesPolicy.

Description of how you validated changes

Added a unit test
Confirmed the new error message works in a sample cdk app

Error: Setting 'requireImdsv2' must not be set when 'launchTemplate' or 'mixedInstancesPolicy' is set
    at AutoScalingGroup.verifyNoLaunchConfigPropIsGiven (~/git/aws-cdk/packages/aws-cdk-lib/aws-autoscaling/lib/auto-scaling-group.ts:1762:13)
    at new AutoScalingGroup (~/git/aws-cdk/packages/aws-cdk-lib/aws-autoscaling/lib/auto-scaling-group.ts:1333:12)
    at new CdkAppStack (~/git/cdkApp/lib/cdk_app-stack.ts:31:17)
    at Object.<anonymous> (~/git/cdkApp/bin/cdk_app.ts:6:1)
    at Module._compile (node:internal/modules/cjs/loader:1256:14)
    at Module.m._compile (~/git/cdkApp/node_modules/ts-node/src/index.ts:1618:23)
    at Module._extensions..js (node:internal/modules/cjs/loader:1310:10)
    at Object.require.extensions.<computed> [as .ts] (~/git/cdkApp/node_modules/ts-node/src/index.ts:1621:12)
    at Module.load (node:internal/modules/cjs/loader:1119:32)
    at Function.Module._load (node:internal/modules/cjs/loader:960:12)

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

shinebleu · 2024-11-20T21:29:28Z

Exemption Request on Fixes must contain a change to an integration test file and the resulting snapshot.: There is no behavioral change for when props are set correctly and cloud formation is produced. The only change is throwing a clearer error message where another, less clear error would otherwise be thrown anyway.

GavinZZ

Just to confirm that the current behaviour is that CFN will throw an error when you set requireImdsv2 with launchTemplate right?

shinebleu · 2024-12-03T20:29:15Z

Just to confirm that the current behaviour is that CFN will throw an error when you set requireImdsv2 with launchTemplate right?

Yes, that will throw an error under current behaviour. It occurs just before cfn synthesis in the Aspect that requireImdsv2 adds to the construct, because the Aspect doesn't handle the case where launchTemplate is set.

codecov · 2024-12-03T22:17:51Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.67%. Comparing base (231e1bf) to head (123c6bb).

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #32220   +/-   ##
=======================================
  Coverage   78.67%   78.67%           
=======================================
  Files         107      107           
  Lines        7237     7237           
  Branches     1329     1329           
=======================================
  Hits         5694     5694           
  Misses       1357     1357           
  Partials      186      186

Flag	Coverage Δ
suite.unit	`78.67% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
packages/aws-cdk	`78.67% <ø> (ø)`

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

mergify · 2024-12-04T19:27:35Z

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

mergify · 2024-12-05T19:08:22Z

This pull request has been removed from the queue for the following reason: pull request branch update failed.

The pull request can't be updated.

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

Pull request has been modified.

shinebleu · 2024-12-05T20:10:32Z

@Mergifyio requeue

mergify · 2024-12-05T20:10:42Z

requeue

❌ Command disallowed due to command restrictions in the Mergify configuration.

Details

sender-permission >= write

mergify · 2024-12-09T17:22:44Z

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 · 2024-12-09T17:55:25Z

AWS CodeBuild CI Report

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

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

mergify · 2024-12-09T17:55:54Z

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

mergify · 2024-12-09T18:00:29Z

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

github-actions · 2024-12-09T18:00:49Z

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

add requireImdsv2 to asg verifyNoLaunchConfigPropIsGiven

5508780

github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK bug This issue is a bug. effort/small Small work item – less than a day of effort p1 labels Nov 20, 2024

aws-cdk-automation requested a review from a team November 20, 2024 20:38

aws-cdk-automation previously requested changes Nov 20, 2024

View reviewed changes

shinebleu changed the title ~~fix(autoscaling): AutoScalingGroup requireImdsv2 with launchTemplate or mixedInstancesPolicy throws unclear error~~ fix(autoscaling): AutoScalingGroup requireImdsv2 with launchTemplate or mixedInstancesPolicy throws unclear error Nov 20, 2024

Merge branch 'main' into asg-requireImdsv2-with-lt

a59c87c

aws-cdk-automation added pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. pr/needs-maintainer-review This PR needs a review from a Core Team Member labels Nov 20, 2024

shinebleu added 2 commits November 22, 2024 11:25

Merge branch 'main' into asg-requireImdsv2-with-lt

d13515e

Merge branch 'main' into asg-requireImdsv2-with-lt

79f3423

github-actions bot mentioned this pull request Dec 1, 2024

Monthly PR metrics report #32340

Closed

GavinZZ reviewed Dec 3, 2024

View reviewed changes

GavinZZ previously approved these changes Dec 4, 2024

View reviewed changes

GavinZZ added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Dec 4, 2024

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 4, 2024

Merge branch 'main' into asg-requireImdsv2-with-lt

f6ed28f

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 5, 2024

GavinZZ approved these changes Dec 9, 2024

View reviewed changes

Merge branch 'main' into asg-requireImdsv2-with-lt

123c6bb

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 9, 2024

mergify bot merged commit 06cdaac into aws:main Dec 9, 2024

github-actions bot locked as resolved and limited conversation to collaborators Dec 9, 2024

Conversation

shinebleu commented Nov 20, 2024

Issue #27586

Reason for this change

Description of changes

Description of how you validated changes

Checklist

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

shinebleu commented Nov 20, 2024

Uh oh!

GavinZZ left a comment

Choose a reason for hiding this comment

Uh oh!

shinebleu commented Dec 3, 2024

Uh oh!

codecov bot commented Dec 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mergify bot commented Dec 4, 2024

Uh oh!

mergify bot commented Dec 5, 2024

Uh oh!

shinebleu commented Dec 5, 2024

Uh oh!

mergify bot commented Dec 5, 2024

❌ Command disallowed due to command restrictions in the Mergify configuration.

Uh oh!

mergify bot commented Dec 9, 2024

Uh oh!

aws-cdk-automation commented Dec 9, 2024

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Dec 9, 2024

Uh oh!

mergify bot commented Dec 9, 2024

Uh oh!

github-actions bot commented Dec 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Dec 3, 2024 •

edited

Loading