fix(ec2): descriptive error message when selecting 0 subnets

Selecting 0 subnets is invalid for most CFN resources. We move the check
for this upstream to the VPC construct, which can throw a nicely
descriptive error message if it happens to not contain any subnets that
match the type we're looking for.

The alternative is that the error happens during CloudFormation
deployment, but then it's not very clear anymore what the cause for
the error was.

Fixes #2011.

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

@@ -291,8 +291,7 @@ export class AutoScalingGroup extends cdk.Construct implements IAutoScalingGroup
       });
     }
 
-    const subnets = props.vpc.subnets(props.vpcPlacement);
-    asgProps.vpcZoneIdentifier = subnets.map(n => n.subnetId);
+    asgProps.vpcZoneIdentifier = props.vpc.subnetIds(props.vpcPlacement);
 
     this.autoScalingGroup = new CfnAutoScalingGroup(this, 'ASG', asgProps);
     this.osType = machineImage.os.type;

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

@@ -1,5 +1,5 @@
 import { Construct, IConstruct, IDependable } from "@aws-cdk/cdk";
-import { subnetName } from './util';
+import { DEFAULT_SUBNET_NAME, subnetName } from './util';
 import { VpnConnection, VpnConnectionOptions } from './vpn';
 
 export interface IVpcSubnet extends IConstruct {
@@ -62,9 +62,23 @@ export interface IVpcNetwork extends IConstruct {
 
   /**
    * Return the subnets appropriate for the placement strategy
+   *
+   * Prefer using {@link subnetIds} if you need to pass subnet IDs to a
+   * CloudFormation Resource.
    */
   subnets(placement?: VpcPlacementStrategy): IVpcSubnet[];
 
+  /**
+   * Return IDs of the subnets appropriate for the given placement strategy
+   *
+   * Requires that at least once subnet is matched, throws a descriptive
+   * error message otherwise.
+   *
+   * Prefer to use this method over {@link subnets} if you need to pass subnet
+   * IDs to a CloudFormation Resource.
+   */
+  subnetIds(placement?: VpcPlacementStrategy): string[];
+
   /**
    * Return whether the given subnet is one of this VPC's public subnets.
    *
@@ -203,9 +217,7 @@ export abstract class VpcNetworkBase extends Construct implements IVpcNetwork {
    * Return the subnets appropriate for the placement strategy
    */
   public subnets(placement: VpcPlacementStrategy = {}): IVpcSubnet[] {
-    if (placement.subnetsToUse !== undefined && placement.subnetName !== undefined) {
-      throw new Error('At most one of subnetsToUse and subnetName can be supplied');
-    }
+    placement = reifyDefaultsPlacement(placement);
 
     // Select by name
     if (placement.subnetName !== undefined) {
@@ -227,6 +239,20 @@ export abstract class VpcNetworkBase extends Construct implements IVpcNetwork {
     }[placement.subnetsToUse];
   }
 
+  /**
+   * Returns IDs of selected subnets
+   */
+  public subnetIds(placement: VpcPlacementStrategy = {}): string[] {
+    placement = reifyDefaultsPlacement(placement);
+
+    const nets = this.subnets(placement);
+    if (nets.length === 0) {
+      throw new Error(`There are no ${describePlacement(placement)} in this VPC. Use a different VPC placement strategy.`);
+    }
+
+    return nets.map(n => n.subnetId);
+  }
+
   /**
    * Adds a new VPN connection to this VPC
    */
@@ -335,3 +361,34 @@ export interface VpcSubnetImportProps {
    */
   subnetId: string;
 }
+
+/**
+ * If the placement strategy is completely "default", reify the defaults so
+ * consuming code doesn't have to reimplement the same analysis every time.
+ *
+ * Returns "private subnets" by default.
+ */
+function reifyDefaultsPlacement(placement: VpcPlacementStrategy): VpcPlacementStrategy {
+    if (placement.subnetsToUse !== undefined && placement.subnetName !== undefined) {
+      throw new Error('At most one of subnetsToUse and subnetName can be supplied');
+    }
+
+    if (placement.subnetsToUse === undefined && placement.subnetName === undefined) {
+      return { subnetsToUse: SubnetType.Private };
+    }
+
+    return placement;
+}
+
+/**
+ * Describe the given placement strategy
+ */
+function describePlacement(placement: VpcPlacementStrategy) {
+  if (placement.subnetsToUse !== undefined) {
+    return `'${DEFAULT_SUBNET_NAME[placement.subnetsToUse]}' subnets`;
+  }
+  if (placement.subnetName !== undefined) {
+    return `subnets named '${placement.subnetName}'`;
+  }
+  return '???';
+}

packages/@aws-cdk/aws-ec2/test/test.vpc.ts

@@ -666,7 +666,24 @@ export = {
 
       test.done();
     },
+
+    'selecting default subnets in a VPC with only public subnets'(test: Test) {
+      // GIVEN
+      const stack = new Stack();
+      const vpc = VpcNetwork.import(stack, 'VPC', {
+        vpcId: 'vpc-1234',
+        availabilityZones: ['dummy1a', 'dummy1b', 'dummy1c'],
+        publicSubnetIds: ['pub-1', 'pub-2', 'pub-3'],
+      });
+
+      test.throws(() => {
+        vpc.subnetIds();
+      }, /There are no 'Private' subnets in this VPC/);
+
+      test.done();
+    }
   },
+
 };
 
 function getTestStack(): Stack {

packages/@aws-cdk/aws-ecs/lib/base/base-service.ts

@@ -184,13 +184,12 @@ export abstract class BaseService extends cdk.Construct
     if (securityGroup === undefined) {
       securityGroup = new ec2.SecurityGroup(this, 'SecurityGroup', { vpc });
     }
-    const subnets = vpc.subnets(vpcPlacement);
     this.connections.addSecurityGroup(securityGroup);
 
     this.networkConfiguration = {
       awsvpcConfiguration: {
         assignPublicIp: assignPublicIp ? 'ENABLED' : 'DISABLED',
-        subnets: subnets.map(x => x.subnetId),
+        subnets: vpc.subnetIds(vpcPlacement),
         securityGroups: new cdk.Token(() => [securityGroup!.securityGroupId]),
       }
     };

packages/@aws-cdk/aws-eks/lib/cluster.ts

@@ -161,7 +161,7 @@ export class Cluster extends ClusterBase {
 
     // Get subnetIds for all selected subnets
     const placements = props.vpcPlacements || [{ subnetsToUse: ec2.SubnetType.Public }, { subnetsToUse: ec2.SubnetType.Private }];
-    const subnetIds = flatMap(placements, p => this.vpc.subnets(p)).map(s => s.subnetId);
+    const subnetIds = flatMap(placements, p => this.vpc.subnetIds(p));
 
     const resource = new CfnCluster(this, 'Resource', {
       name: props.clusterName,

packages/@aws-cdk/aws-elasticloadbalancingv2/lib/shared/base-load-balancer.ts

@@ -98,20 +98,23 @@ export abstract class BaseLoadBalancer extends cdk.Construct implements route53.
 
     const internetFacing = ifUndefined(baseProps.internetFacing, false);
 
-    const subnets = baseProps.vpc.subnets(ifUndefined(baseProps.vpcPlacement,
-      { subnetsToUse: internetFacing ? ec2.SubnetType.Public : ec2.SubnetType.Private }));
+    const vpcPlacement = ifUndefined(baseProps.vpcPlacement,
+      { subnetsToUse: internetFacing ? ec2.SubnetType.Public : ec2.SubnetType.Private });
+
+    const subnets = baseProps.vpc.subnetIds(vpcPlacement);
 
     this.vpc = baseProps.vpc;
 
     const resource = new CfnLoadBalancer(this, 'Resource', {
       name: baseProps.loadBalancerName,
-      subnets: subnets.map(s => s.subnetId),
+      subnets,
       scheme: internetFacing ? 'internet-facing' : 'internal',
       loadBalancerAttributes: new cdk.Token(() => renderAttributes(this.attributes)),
       ...additionalProps
     });
     if (internetFacing) {
-      resource.node.addDependency(...subnets.map(s => s.internetConnectivityEstablished));
+      const subnetObjs = baseProps.vpc.subnets(vpcPlacement);
+      resource.node.addDependency(...subnetObjs.map(s => s.internetConnectivityEstablished));
     }
 
     if (baseProps.deletionProtection) { this.setAttribute('deletion_protection.enabled', 'true'); }

packages/@aws-cdk/aws-lambda/lib/function.ts

@@ -504,15 +504,21 @@ export class Function extends FunctionBase {
 
     // Pick subnets, make sure they're not Public. Routing through an IGW
     // won't work because the ENIs don't get a Public IP.
+    // Why are we not simply forcing vpcPlacement? Because you might still be choosing
+    // Isolated networks or selecting among 2 sets of Private subnets by name.
     const subnets = props.vpc.subnets(props.vpcPlacement);
     for (const subnet of subnets) {
       if (props.vpc.isPublicSubnet(subnet)) {
         throw new Error('Not possible to place Lambda Functions in a Public subnet');
       }
     }
 
+    // List can't be empty here, if we got this far you intended to put your Lambda
+    // in subnets. We're going to guarantee that we get the nice error message by
+    // making VpcNetwork do the selection again.
+
     return {
-      subnetIds: subnets.map(s => s.subnetId),
+      subnetIds: props.vpc.subnetIds(props.vpcPlacement),
       securityGroupIds: [securityGroup.securityGroupId]
     };
   }

packages/@aws-cdk/aws-rds/lib/cluster.ts

@@ -139,16 +139,16 @@ export class DatabaseCluster extends cdk.Construct implements IDatabaseCluster {
   constructor(scope: cdk.Construct, id: string, props: DatabaseClusterProps) {
     super(scope, id);
 
-    const subnets = props.instanceProps.vpc.subnets(props.instanceProps.vpcPlacement);
+    const subnetIds = props.instanceProps.vpc.subnetIds(props.instanceProps.vpcPlacement);
 
     // Cannot test whether the subnets are in different AZs, but at least we can test the amount.
-    if (subnets.length < 2) {
-      throw new Error(`Cluster requires at least 2 subnets, got ${subnets.length}`);
+    if (subnetIds.length < 2) {
+      throw new Error(`Cluster requires at least 2 subnets, got ${subnetIds.length}`);
     }
 
     const subnetGroup = new CfnDBSubnetGroup(this, 'Subnets', {
       dbSubnetGroupDescription: `Subnets for ${id} database`,
-      subnetIds: subnets.map(s => s.subnetId)
+      subnetIds,
     });
 
     const securityGroup = props.instanceProps.securityGroup !== undefined ?
@@ -187,6 +187,8 @@ export class DatabaseCluster extends cdk.Construct implements IDatabaseCluster {
       throw new Error('At least one instance is required');
     }
 
+    // Get the actual subnet objects so we can depend on internet connectivity.
+    const subnets = props.instanceProps.vpc.subnets(props.instanceProps.vpcPlacement);
     for (let i = 0; i < instanceCount; i++) {
       const instanceIndex = i + 1;