Skip to content

Commit

Permalink
fix(autoscaling): error not thrown when associatePublicIpAddress is s…
Browse files Browse the repository at this point in the history
…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*
  • Loading branch information
peterwoodworth authored and josephedward committed Aug 30, 2022
1 parent 824eb05 commit 594ed44
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 7 deletions.
22 changes: 16 additions & 6 deletions packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts
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
*
* `launchTemplate` and `mixedInstancesPolicy` must not be specified when this property is 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.
*
* `launchTemplate` and `mixedInstancesPolicy` must not be specified when this property is 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.
*
* `launchTemplate` and `mixedInstancesPolicy` must not be specified when this property is 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.
*
* `launchTemplate` and `mixedInstancesPolicy` must not be specified when this property is 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.
*
* `launchTemplate` and `mixedInstancesPolicy` must not be specified when this property is 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.
* `launchTemplate` and `mixedInstancesPolicy` must not be specified when this property 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.
* `launchTemplate` and `mixedInstancesPolicy` must not be specified when this property is 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.
* `launchTemplate` and `mixedInstancesPolicy` must not be specified when this property is 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.
* `launchTemplate` and `mixedInstancesPolicy` must not be specified when this property is 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.
* `launchTemplate` and `mixedInstancesPolicy` must not be specified when this property is 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 !== undefined) {
throw new Error('Setting \'associatePublicIpAddress\' must not be set when \'launchTemplate\' or \'mixedInstancesPolicy\' is set');
}
if (props.spotPrice) {
Expand Down
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

0 comments on commit 594ed44

Please sign in to comment.