fix(autoscaling): verify public subnets for associatePublicIpAddress (#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()`.

Author: rix0rrr

Diff for: packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts

@@ -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;

Diff for: packages/@aws-cdk/aws-autoscaling/test/test.auto-scaling-group.ts

@@ -418,6 +418,8 @@ export = {
       minCapacity: 0,
       maxCapacity: 0,
       desiredCapacity: 0,
+
+      vpcSubnets: { subnetType: ec2.SubnetType.Public },
       associatePublicIpAddress: true,
     });
 
@@ -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();

Diff for: packages/@aws-cdk/aws-ec2/lib/vpc-ref.ts

@@ -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
@@ -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));
   }
 
   /**

Diff for: packages/@aws-cdk/aws-ec2/lib/vpc.ts

@@ -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}`);
         }
       }