fix(autoscaling): error not thrown when associatePublicIpAddress is set to false when specifying launchTemplate #21714
Conversation
…et to false when specifying launchTemplate
|
|
| @@ -543,7 +553,7 @@ export interface AutoScalingGroupProps extends CommonAutoScalingGroupProps { | |||
| /** | |||
| * Type of instance to launch | |||
| * | |||
| * `launchTemplate` must not be specified when this property is specified. | |||
| * Cannot specify this property when `launchTemplate` or `mixedInstancesPolicy` are specified | |||
There was a problem hiding this comment.
This comment stands for each instance here (I won't repeat it because that's just kind of annoying), but I think we actually want the original style comment of:
`whatever` must not be specified when `whatever-else` is specified.
| @@ -1523,7 +1533,7 @@ export class AutoScalingGroup extends AutoScalingGroupBase implements | |||
| if (props.instanceMonitoring) { | |||
| throw new Error('Setting \'instanceMonitoring\' must not be set when \'launchTemplate\' or \'mixedInstancesPolicy\' is set'); | |||
| } | |||
| if (props.associatePublicIpAddress) { | |||
| if (props.associatePublicIpAddress || props.associatePublicIpAddress === false) { | |||
There was a problem hiding this comment.
I'm not sure I get why setting it to false should be an issue here. It seems like we're mishandling it somewhere else and that's where we need to apply the fix.
There was a problem hiding this comment.
This could probably be simplified to if (props.associatePublicIpAddress !== undefined).
It looks like if you specify the launchTemplate then you must specify the associatePublicIpAddress on the LaunchTemplate and not the AutoScalingGroup. The problem in the linked issue is that associatePublicIpAddress must be specified in the LaunchTemplate.networkInterfaces property which is not currently supported, which means that you can't use launchTemplate and set associatePublicIpAddress: false currently. Which is not great.
I think long term we should deprecate the AutoScalingGroup construct and create a AutoScalingGroupV2 (or something) which accepts an ILaunchConfiguration instead of just having all the properties for a LaunchConfiguration in the AutoScalingGroupProps.
There was a problem hiding this comment.
Yep, the design here isn't too great. However I'm not sure what we can do for now except ensure the error gets thrown.
…et to false when specifying launchTemplate
| @@ -217,6 +223,8 @@ export interface CommonAutoScalingGroupProps { | |||
| * You can use block device mappings to specify additional EBS volumes or | |||
| * instance store volumes to attach to an instance when it is launched. | |||
| * | |||
| * `launchTemplate` and `mixedInstancesPolicy` must not be specified when this property is specified | |||
peterwoodworth
dismissed
TheRealAmazonKendra’s stale review
addressed comments
|
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 CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
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). |
…et to false when specifying launchTemplate (aws#21714) Setting `associatePublicIpAddress` prop when also setting `launchTemplate` prop on a new ASG is supposed to throw an error, but doesn't due to `associatePublicIpAddress` being a boolean, and the error message not triggering if a falsy value is used for the prop. This PR will ensure an error is thrown when `associatePublicIpAddress` is set, even when the value passed is falsy. It also updates the documentation on all props which conflict with `launchTemplate` or `mixedInstancesPolicy` fixes: aws#21576 ---- ### All Submissions: * [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Setting
associatePublicIpAddressprop when also settinglaunchTemplateprop on a new ASG is supposed to throw an error, but doesn't due toassociatePublicIpAddressbeing a boolean, and the error message not triggering if a falsy value is used for the prop. This PR will ensure an error is thrown whenassociatePublicIpAddressis set, even when the value passed is falsy. It also updates the documentation on all props which conflict withlaunchTemplateormixedInstancesPolicyfixes: #21576
All Submissions:
Adding new Unconventional Dependencies:
New Features
yarn integto deploy the infrastructure and generate the snapshot (i.e.yarn integwithout--dry-run)?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license