Skip to content

Commit

Permalink
fix(autoscaling): AutoScalingGroup requireImdsv2 with launchTemplat…
Browse files Browse the repository at this point in the history
…e or mixedInstancesPolicy throws unclear error (#32220)

### Issue #27586

Fixes #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
- [X] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
shinebleu authored Dec 9, 2024
1 parent 231e1bf commit 06cdaac
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -1759,6 +1759,9 @@ export class AutoScalingGroup extends AutoScalingGroupBase implements
if (props.blockDevices) {
throw new Error('Setting \'blockDevices\' must not be set when \'launchTemplate\' or \'mixedInstancesPolicy\' is set');
}
if (props.requireImdsv2) {
throw new Error('Setting \'requireImdsv2\' must not be set when \'launchTemplate\' or \'mixedInstancesPolicy\' is set');
}
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2947,6 +2947,26 @@ describe('InstanceMaintenancePolicy', () => {
});
}).toThrow(/The difference between minHealthyPercentage and maxHealthyPercentage cannot be greater than 100, got 200/);
});

test('throws if requireImdsv2 set when launchTemplate is set', () => {
// GIVEN
const stack = new cdk.Stack();
stack.node.setContext(AUTOSCALING_GENERATE_LAUNCH_TEMPLATE, true);
const vpc = mockVpc(stack);
const lt = LaunchTemplate.fromLaunchTemplateAttributes(stack, 'imported-lt', {
launchTemplateId: 'test-lt-id',
versionNumber: '0',
});

// THEN
expect(() => {
new autoscaling.AutoScalingGroup(stack, 'MyFleet', {
vpc,
launchTemplate: lt,
requireImdsv2: true,
});
}).toThrow(/Setting \'requireImdsv2\' must not be set when \'launchTemplate\' or \'mixedInstancesPolicy\' is set/);
});
});

function mockSecurityGroup(stack: cdk.Stack) {
Expand Down

0 comments on commit 06cdaac

Please sign in to comment.