From 1e3d41ea007e3dd0424bce39f86d74bcb30c86e3 Mon Sep 17 00:00:00 2001
From: Rico Huijbers <rix0rrr@gmail.com>
Date: Tue, 26 Mar 2019 14:05:49 +0100
Subject: [PATCH] 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()`.
---
 .../aws-autoscaling/lib/auto-scaling-group.ts |  3 +++
 .../test/test.auto-scaling-group.ts           | 21 +++++++++++++++++++
 packages/@aws-cdk/aws-ec2/lib/vpc-ref.ts      | 19 ++++++-----------
 packages/@aws-cdk/aws-ec2/lib/vpc.ts          |  2 +-
 4 files changed, 31 insertions(+), 14 deletions(-)

diff --git a/packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts b/packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts
index 697862ddf6bcd..92161dbfc05ca 100644
--- a/packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts
+++ b/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 --git a/packages/@aws-cdk/aws-autoscaling/test/test.auto-scaling-group.ts b/packages/@aws-cdk/aws-autoscaling/test/test.auto-scaling-group.ts
index 8871e86c958ad..f833fe4eec5b7 100644
--- a/packages/@aws-cdk/aws-autoscaling/test/test.auto-scaling-group.ts
+++ b/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 --git a/packages/@aws-cdk/aws-ec2/lib/vpc-ref.ts b/packages/@aws-cdk/aws-ec2/lib/vpc-ref.ts
index 69972d73fcc8d..25b1eb46daeab 100644
--- a/packages/@aws-cdk/aws-ec2/lib/vpc-ref.ts
+++ b/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 --git a/packages/@aws-cdk/aws-ec2/lib/vpc.ts b/packages/@aws-cdk/aws-ec2/lib/vpc.ts
index 9465fdcf6dfc7..241d9b276d80f 100644
--- a/packages/@aws-cdk/aws-ec2/lib/vpc.ts
+++ b/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}`);
         }
       }