Skip to content

Commit

Permalink
fix(ec2): descriptive error message when selecting 0 subnets (#2025)
Browse files Browse the repository at this point in the history
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.

BREAKING CHANGE: `vpcPlacement` has been renamed to `vpcSubnets`
on all objects, `subnetsToUse` has been renamed to `subnetType`.
`natGatewayPlacement` has been renamed to `natGatewaySubnets`.
  • Loading branch information
rix0rrr authored Mar 21, 2019
1 parent 86e9d03 commit 0de2206
Show file tree
Hide file tree
Showing 20 changed files with 270 additions and 141 deletions.
5 changes: 2 additions & 3 deletions packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ export interface CommonAutoScalingGroupProps {
/**
* Where to place instances within the VPC
*/
vpcPlacement?: ec2.VpcPlacementStrategy;
vpcSubnets?: ec2.SubnetSelection;

/**
* SNS topic to send notifications about fleet changes
Expand Down Expand Up @@ -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.vpcSubnets);

this.autoScalingGroup = new CfnAutoScalingGroup(this, 'ASG', asgProps);
this.osType = machineImage.os.type;
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-ec2/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ instances for your project.

Our default `VpcNetwork` class creates a private and public subnet for every
availability zone. Classes that use the VPC will generally launch instances
into all private subnets, and provide a parameter called `vpcPlacement` to
into all private subnets, and provide a parameter called `vpcSubnets` to
allow you to override the placement. [Read more about
subnets](https://docs.aws.amazon.com/AmazonVPC/latest/UserGuide/VPC_Subnets.html).

Expand Down
144 changes: 116 additions & 28 deletions packages/@aws-cdk/aws-ec2/lib/vpc-ref.ts
Original file line number Diff line number Diff line change
@@ -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 {
Expand Down Expand Up @@ -61,9 +61,20 @@ export interface IVpcNetwork extends IConstruct {
readonly vpnGatewayId?: string;

/**
* Return the subnets appropriate for the placement strategy
* Return IDs of the subnets appropriate for the given selection 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.
*/
subnets(placement?: VpcPlacementStrategy): IVpcSubnet[];
subnetIds(selection?: SubnetSelection): string[];

/**
* Return a dependable object representing internet connectivity for the given subnets
*/
subnetInternetDependencies(selection?: SubnetSelection): IDependable;

/**
* Return whether the given subnet is one of this VPC's public subnets.
Expand Down Expand Up @@ -125,29 +136,29 @@ export enum SubnetType {
}

/**
* Customize how instances are placed inside a VPC
* Customize subnets that are selected for placement of ENIs
*
* Constructs that allow customization of VPC placement use parameters of this
* type to provide placement settings.
*
* By default, the instances are placed in the private subnets.
*/
export interface VpcPlacementStrategy {
export interface SubnetSelection {
/**
* Place the instances in the subnets of the given type
*
* At most one of `subnetsToUse` and `subnetName` can be supplied.
* At most one of `subnetType` and `subnetName` can be supplied.
*
* @default SubnetType.Private
*/
subnetsToUse?: SubnetType;
subnetType?: SubnetType;

/**
* Place the instances in the subnets with the given name
*
* (This is the name supplied in subnetConfiguration).
*
* At most one of `subnetsToUse` and `subnetName` can be supplied.
* At most one of `subnetType` and `subnetName` can be supplied.
*
* @default name
*/
Expand Down Expand Up @@ -200,31 +211,30 @@ export abstract class VpcNetworkBase extends Construct implements IVpcNetwork {
public readonly natDependencies = new Array<IConstruct>();

/**
* Return the subnets appropriate for the placement strategy
* Returns IDs of selected subnets
*/
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');
}
public subnetIds(selection: SubnetSelection = {}): string[] {
selection = reifySelectionDefaults(selection);

// Select by name
if (placement.subnetName !== undefined) {
const allSubnets = this.privateSubnets.concat(this.publicSubnets).concat(this.isolatedSubnets);
const selectedSubnets = allSubnets.filter(s => subnetName(s) === placement.subnetName);
if (selectedSubnets.length === 0) {
throw new Error(`No subnets with name: ${placement.subnetName}`);
}
return selectedSubnets;
const nets = this.subnets(selection);
if (nets.length === 0) {
throw new Error(`There are no ${describeSelection(selection)} in this VPC. Use a different VPC subnet selection.`);
}

// Select by type
if (placement.subnetsToUse === undefined) { return this.privateSubnets; }
return nets.map(n => n.subnetId);
}

return {
[SubnetType.Isolated]: this.isolatedSubnets,
[SubnetType.Private]: this.privateSubnets,
[SubnetType.Public]: this.publicSubnets,
}[placement.subnetsToUse];
/**
* Return a dependable object representing internet connectivity for the given subnets
*/
public subnetInternetDependencies(selection: SubnetSelection = {}): IDependable {
selection = reifySelectionDefaults(selection);

const ret = new CompositeDependable();
for (const subnet of this.subnets(selection)) {
ret.add(subnet.internetConnectivityEstablished);
}
return ret;
}

/**
Expand Down Expand Up @@ -260,6 +270,31 @@ export abstract class VpcNetworkBase extends Construct implements IVpcNetwork {
return this.node.stack.region;
}

/**
* Return the subnets appropriate for the placement strategy
*/
protected subnets(selection: SubnetSelection = {}): IVpcSubnet[] {
selection = reifySelectionDefaults(selection);

// Select by name
if (selection.subnetName !== undefined) {
const allSubnets = this.privateSubnets.concat(this.publicSubnets).concat(this.isolatedSubnets);
const selectedSubnets = allSubnets.filter(s => subnetName(s) === selection.subnetName);
if (selectedSubnets.length === 0) {
throw new Error(`No subnets with name: ${selection.subnetName}`);
}
return selectedSubnets;
}

// Select by type
if (selection.subnetType === undefined) { return this.privateSubnets; }

return {
[SubnetType.Isolated]: this.isolatedSubnets,
[SubnetType.Private]: this.privateSubnets,
[SubnetType.Public]: this.publicSubnets,
}[selection.subnetType];
}
}

/**
Expand Down Expand Up @@ -335,3 +370,56 @@ 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 reifySelectionDefaults(placement: SubnetSelection): SubnetSelection {
if (placement.subnetType !== undefined && placement.subnetName !== undefined) {
throw new Error('Only one of subnetType and subnetName can be supplied');
}

if (placement.subnetType === undefined && placement.subnetName === undefined) {
return { subnetType: SubnetType.Private };
}

return placement;
}

/**
* Describe the given placement strategy
*/
function describeSelection(placement: SubnetSelection): string {
if (placement.subnetType !== undefined) {
return `'${DEFAULT_SUBNET_NAME[placement.subnetType]}' subnets`;
}
if (placement.subnetName !== undefined) {
return `subnets named '${placement.subnetName}'`;
}
return JSON.stringify(placement);
}

class CompositeDependable implements IDependable {
private readonly dependables = new Array<IDependable>();

/**
* Add a construct to the dependency roots
*/
public add(dep: IDependable) {
this.dependables.push(dep);
}

/**
* Retrieve the current set of dependency roots
*/
public get dependencyRoots(): IConstruct[] {
const ret = [];
for (const dep of this.dependables) {
ret.push(...dep.dependencyRoots);
}
return ret;
}
}
8 changes: 4 additions & 4 deletions packages/@aws-cdk/aws-ec2/lib/vpc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { CfnRouteTable, CfnSubnet, CfnSubnetRouteTableAssociation, CfnVPC, CfnVP
import { NetworkBuilder } from './network-util';
import { DEFAULT_SUBNET_NAME, ExportSubnetGroup, ImportSubnetGroup, subnetId } from './util';
import { VpcNetworkProvider, VpcNetworkProviderProps } from './vpc-network-provider';
import { IVpcNetwork, IVpcSubnet, SubnetType, VpcNetworkBase, VpcNetworkImportProps, VpcPlacementStrategy, VpcSubnetImportProps } from './vpc-ref';
import { IVpcNetwork, IVpcSubnet, SubnetSelection, SubnetType, VpcNetworkBase, VpcNetworkImportProps, VpcSubnetImportProps } from './vpc-ref';
import { VpnConnectionOptions, VpnConnectionType } from './vpn';

/**
Expand Down Expand Up @@ -80,7 +80,7 @@ export interface VpcNetworkProps {
*
* @default All public subnets
*/
natGatewayPlacement?: VpcPlacementStrategy;
natGatewaySubnets?: SubnetSelection;

/**
* Configure the subnets to build for each AZ
Expand Down Expand Up @@ -365,7 +365,7 @@ export class VpcNetwork extends VpcNetworkBase {
});

// if gateways are needed create them
this.createNatGateways(props.natGateways, props.natGatewayPlacement);
this.createNatGateways(props.natGateways, props.natGatewaySubnets);

(this.privateSubnets as VpcPrivateSubnet[]).forEach((privateSubnet, i) => {
let ngwId = this.natGatewayByAZ[privateSubnet.availabilityZone];
Expand Down Expand Up @@ -445,7 +445,7 @@ export class VpcNetwork extends VpcNetworkBase {
};
}

private createNatGateways(gateways?: number, placement?: VpcPlacementStrategy): void {
private createNatGateways(gateways?: number, placement?: SubnetSelection): void {
const useNatGateway = this.subnetConfiguration.filter(
subnet => (subnet.subnetType === SubnetType.Private)).length > 0;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ new ec2.SecurityGroup(stack, 'SecurityGroup', {
});

// Try subnet selection
new cdk.CfnOutput(stack, 'PublicSubnets', { value: 'ids:' + vpc.subnets({ subnetsToUse: ec2.SubnetType.Public }).map(s => s.subnetId).join(',') });
new cdk.CfnOutput(stack, 'PrivateSubnets', { value: 'ids:' + vpc.subnets({ subnetsToUse: ec2.SubnetType.Private }).map(s => s.subnetId).join(',') });
new cdk.CfnOutput(stack, 'PublicSubnets', { value: 'ids:' + vpc.publicSubnets.map(s => s.subnetId).join(',') });
new cdk.CfnOutput(stack, 'PrivateSubnets', { value: 'ids:' + vpc.privateSubnets.map(s => s.subnetId).join(',') });

app.run();
Loading

0 comments on commit 0de2206

Please sign in to comment.