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.

Sign up for GitHub

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

fix(autoscaling): error not thrown when associatePublicIpAddress is set to false when specifying launchTemplate #21714

Merged
merged 3 commits into from
Aug 25, 2022
Merged

fix(autoscaling): error not thrown when associatePublicIpAddress is set to false when specifying launchTemplate #21714

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
84161a8
fix(autoscaling): error not thrown when associatePublicIpAddress is s…
peterwoodworth Aug 22, 2022
fb35b5e
fix(autoscaling): error not thrown when associatePublicIpAddress is s…
peterwoodworth Aug 25, 2022
1bf056c
Merge branch 'main' into peterwoodworth/autoscalingPropBug
mergify[bot] Aug 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 16 additions & 6 deletions packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,8 @@ export interface CommonAutoScalingGroupProps {
/**
* Name of SSH keypair to grant access to instances
*
* Cannot specify this property when `launchTemplate` or `mixedInstancesPolicy` are specified
*
* @default - No SSH access will be possible.
*/
readonly keyName?: string;
Expand Down Expand Up @@ -190,6 +192,8 @@ export interface CommonAutoScalingGroupProps {
* Whether instances in the Auto Scaling Group should have public
* IP addresses associated with them.
*
* Cannot specify this property when `launchTemplate` or `mixedInstancesPolicy` are specified
*
* @default - Use subnet setting.
*/
readonly associatePublicIpAddress?: boolean;
Expand All @@ -198,6 +202,8 @@ export interface CommonAutoScalingGroupProps {
* The maximum hourly price (in USD) to be paid for any Spot Instance launched to fulfill the request. Spot Instances are
* launched when the price you specify exceeds the current Spot market price.
*
* Cannot specify this property when `launchTemplate` or `mixedInstancesPolicy` are specified
*
* @default none
*/
readonly spotPrice?: string;
Expand All @@ -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.
*
* Cannot specify this property when `launchTemplate` or `mixedInstancesPolicy` are specified
*
* @see https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/block-device-mapping-concepts.html
*
* @default - Uses the block device mapping of the AMI
Expand All @@ -243,6 +251,8 @@ export interface CommonAutoScalingGroupProps {
* When detailed monitoring is enabled, Amazon CloudWatch generates metrics every minute and your account
* is charged a fee. When you disable detailed monitoring, CloudWatch generates metrics every 5 minutes.
*
* Cannot specify this property when `launchTemplate` or `mixedInstancesPolicy` are specified
*
* @see https://docs.aws.amazon.com/autoscaling/latest/userguide/as-instance-monitoring.html#enable-as-instance-metrics
*
* @default - Monitoring.DETAILED
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

*
* @default - Do not provide any instance type
*/
Expand All @@ -552,7 +562,7 @@ export interface AutoScalingGroupProps extends CommonAutoScalingGroupProps {
/**
* AMI to launch
*
* `launchTemplate` must not be specified when this property is specified.
* Cannot specify this property when `launchTemplate` or `mixedInstancesPolicy` are specified
*
* @default - Do not provide any machine image
*/
Expand All @@ -561,7 +571,7 @@ export interface AutoScalingGroupProps extends CommonAutoScalingGroupProps {
/**
* Security group to launch the instances in.
*
* `launchTemplate` must not be specified when this property is specified.
* Cannot specify this property when `launchTemplate` or `mixedInstancesPolicy` are specified
*
* @default - A SecurityGroup will be created if none is specified.
*/
Expand All @@ -572,7 +582,7 @@ export interface AutoScalingGroupProps extends CommonAutoScalingGroupProps {
*
* The UserData may still be mutated after creation.
*
* `launchTemplate` must not be specified when this property is specified.
* Cannot specify this property when `launchTemplate` or `mixedInstancesPolicy` are specified
*
* @default - A UserData object appropriate for the MachineImage's
* Operating System is created.
Expand All @@ -584,7 +594,7 @@ export interface AutoScalingGroupProps extends CommonAutoScalingGroupProps {
*
* The role must be assumable by the service principal `ec2.amazonaws.com`:
*
* `launchTemplate` must not be specified when this property is specified.
* Cannot specify this property when `launchTemplate` or `mixedInstancesPolicy` are specified
*
* @example
*
Expand Down Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

throw new Error('Setting \'associatePublicIpAddress\' must not be set when \'launchTemplate\' or \'mixedInstancesPolicy\' is set');
}
if (props.spotPrice) {
Expand Down
10 changes: 9 additions & 1 deletion packages/@aws-cdk/aws-autoscaling/test/auto-scaling-group.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1525,6 +1525,7 @@ describe('auto scaling group', () => {
launchTemplateId: 'test-lt-id',
versionNumber: '0',
});
const vpc = mockVpc(stack);

// THEN
expect(() => {
Expand All @@ -1535,9 +1536,16 @@ describe('auto scaling group', () => {
generation: AmazonLinuxGeneration.AMAZON_LINUX_2,
cpuType: AmazonLinuxCpuType.X86_64,
}),
vpc: mockVpc(stack),
vpc,
});
}).toThrow('Setting \'machineImage\' must not be set when \'launchTemplate\' or \'mixedInstancesPolicy\' is set');
expect(() => {
new autoscaling.AutoScalingGroup(stack, 'imported-lt-asg-2', {
launchTemplate: lt,
associatePublicIpAddress: true,
vpc,
});
}).toThrow('Setting \'associatePublicIpAddress\' must not be set when \'launchTemplate\' or \'mixedInstancesPolicy\' is set');
});

test('Cannot specify Launch Template without instance type', () => {
Expand Down