Skip to content

Commit

Permalink
fix(autoscaling): verify public subnets for associatePublicIpAddress (#…
Browse files Browse the repository at this point in the history
…2077)

The AutoScalingGroup construct allows setting associatePublicIpAddress,
but that is pointless when you're not in a Public subnet because your
shiny public IP address will still not be routable.

Adding the check get rids of another sharp edge around EC2 networking
that people need to be aware of.

Also change the 'isPublicSubnet()' method on VPC to work with subnet IDs
instead of objects, to align better with the 'subnetIds()' function.

BREAKING CHANGE: `VpcNetwork.isPublicSubnet()` has been renamed to
`VpcNetwork.isPublicSubnetIds()`.
  • Loading branch information
rix0rrr authored Mar 26, 2019
1 parent 6699fed commit 1e3d41e
Show file tree
Hide file tree
Showing 4 changed files with 31 additions and 14 deletions.
3 changes: 3 additions & 0 deletions packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,9 @@ export class AutoScalingGroup extends cdk.Construct implements IAutoScalingGroup
}

asgProps.vpcZoneIdentifier = props.vpc.subnetIds(props.vpcSubnets);
if (!props.vpc.isPublicSubnets(asgProps.vpcZoneIdentifier) && props.associatePublicIpAddress) {
throw new Error("To set 'associatePublicIpAddress: true' you must select Public subnets (vpcSubnets: { subnetType: SubnetType.Public })");
}

this.autoScalingGroup = new CfnAutoScalingGroup(this, 'ASG', asgProps);
this.osType = machineImage.os.type;
Expand Down
21 changes: 21 additions & 0 deletions packages/@aws-cdk/aws-autoscaling/test/test.auto-scaling-group.ts
Original file line number Diff line number Diff line change
Expand Up @@ -418,6 +418,8 @@ export = {
minCapacity: 0,
maxCapacity: 0,
desiredCapacity: 0,

vpcSubnets: { subnetType: ec2.SubnetType.Public },
associatePublicIpAddress: true,
});

Expand All @@ -428,6 +430,25 @@ export = {
));
test.done();
},
'association of public IP address requires public subnet'(test: Test) {
// GIVEN
const stack = new cdk.Stack();
const vpc = mockVpc(stack);

// WHEN
test.throws(() => {
new autoscaling.AutoScalingGroup(stack, 'MyStack', {
instanceType: new ec2.InstanceTypePair(ec2.InstanceClass.M4, ec2.InstanceSize.Micro),
machineImage: new ec2.AmazonLinuxImage(),
vpc,
minCapacity: 0,
maxCapacity: 0,
desiredCapacity: 0,
associatePublicIpAddress: true,
});
});
test.done();
},
'allows disassociation of public IP address'(test: Test) {
// GIVEN
const stack = new cdk.Stack();
Expand Down
19 changes: 6 additions & 13 deletions packages/@aws-cdk/aws-ec2/lib/vpc-ref.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,13 +77,9 @@ export interface IVpcNetwork extends IConstruct {
subnetInternetDependencies(selection?: SubnetSelection): IDependable;

/**
* Return whether the given subnet is one of this VPC's public subnets.
*
* The subnet must literally be one of the subnet object obtained from
* this VPC. A subnet that merely represents the same subnet will
* never return true.
* Return whether all of the given subnets are from the VPC's public subnets.
*/
isPublicSubnet(subnet: IVpcSubnet): boolean;
isPublicSubnets(subnetIds: string[]): boolean;

/**
* Adds a new VPN connection to this VPC
Expand Down Expand Up @@ -253,14 +249,11 @@ export abstract class VpcNetworkBase extends Construct implements IVpcNetwork {
public abstract export(): VpcNetworkImportProps;

/**
* Return whether the given subnet is one of this VPC's public subnets.
*
* The subnet must literally be one of the subnet object obtained from
* this VPC. A subnet that merely represents the same subnet will
* never return true.
* Return whether all of the given subnets are from the VPC's public subnets.
*/
public isPublicSubnet(subnet: IVpcSubnet) {
return this.publicSubnets.indexOf(subnet) > -1;
public isPublicSubnets(subnetIds: string[]): boolean {
const pubIds = new Set(this.publicSubnets.map(n => n.subnetId));
return subnetIds.every(pubIds.has.bind(pubIds));
}

/**
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-ec2/lib/vpc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -456,7 +456,7 @@ export class VpcNetwork extends VpcNetworkBase {
if (placement) {
const subnets = this.subnets(placement);
for (const sub of subnets) {
if (!this.isPublicSubnet(sub)) {
if (this.publicSubnets.indexOf(sub) === -1) {
throw new Error(`natGatewayPlacement ${placement} contains non public subnet ${sub}`);
}
}
Expand Down

0 comments on commit 1e3d41e

Please sign in to comment.