Skip to content

Commit

Permalink
feat(vpc): additional validation around Subnet Types (#4668)
Browse files Browse the repository at this point in the history 
Try to improve the usability around VPCs and certain Subnet Type
configurations.

- Make clear that ISOLATED does not mean "no Internet access at all",
  just no Internet access from the point of view of the current VPC.

- If people configure natGateways=0, tell them that they probably meant
  to configure no PRIVATE subnets.

- If people DO configure PRIVATE subnets, make sure they also configure
  PUBLIC subnets, otherwise we won't be able to place the NAT gateways
  anywhere useful.

- If people end up with a VPC without PRIVATE subnets, the default
  behavior of `selectSubnets` is pretty useless, because it can never
  work, and that's not what people are used to from the CDK. Dynamically
  adjust the selection default to whatever subnet types *are* available.

Fixes #3704.
  • Loading branch information
@rix0rrr @mergify
rix0rrr authored and mergify[bot] committed Oct 27, 2019
1 parent 0e96575 commit 9a96c37
Show file tree
Hide file tree
Showing 2 changed files with 140 additions and 78 deletions.
165 changes: 96 additions & 69 deletions packages/@aws-cdk/aws-ec2/lib/vpc.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -118,24 +118,23 @@ export interface IVpc extends IResource {
*/
export enum SubnetType {
/**
* Isolated Subnets do not route Outbound traffic
* Isolated Subnets do not route traffic to the Internet (in this VPC).
*
* This can be good for subnets with RDS or
* Elasticache endpoints
* This can be good for subnets with RDS or Elasticache instances,
* or which route Internet traffic through a peer VPC.
*/
ISOLATED = 'Isolated',

/**
* Subnet that routes to the internet, but not vice versa.
*
* Instances in a private subnet can connect to the Internet, but will not
* allow connections to be initiated from the Internet.
* allow connections to be initiated from the Internet. Internet traffic will
* be routed via a NAT Gateway.
*
* Outbound traffic will be routed via a NAT Gateway. Preference being in
* the same AZ, but if not available will use another AZ (control by
* specifing `maxGateways` on Vpc). This might be used for
* experimental cost conscious accounts or accounts where HA outbound
* traffic is not needed.
* Normally a Private subnet will use a NAT gateway in the same AZ, but
* if `natGateways` is used to reduce the number of NAT gateways, a NAT
* gateway from another AZ will be used instead.
*/
PRIVATE = 'Private',

Expand Down Expand Up @@ -166,7 +165,7 @@ export interface SubnetSelection {
*
* At most one of `subnetType` and `subnetGroupName` can be supplied.
*
* @default SubnetType.PRIVATE
* @default SubnetType.PRIVATE (or ISOLATED or PUBLIC if there are no PRIVATE subnets)
*/
readonly subnetType?: SubnetType;

Expand Down Expand Up @@ -350,7 +349,7 @@ abstract class VpcBase extends Resource implements IVpc {
* Return the subnets appropriate for the placement strategy
*/
protected selectSubnetObjects(selection: SubnetSelection = {}): ISubnet[] {
selection = reifySelectionDefaults(selection);
selection = this.reifySelectionDefaults(selection);

if (selection.subnets !== undefined) {
return selection.subnets;
Expand Down Expand Up @@ -400,6 +399,36 @@ abstract class VpcBase extends Resource implements IVpc {

return subnets;
}

/**
* Validate the fields in a SubnetSelection object, and reify defaults if necessary
*
* In case of default selection, select the first type of PRIVATE, ISOLATED,
* PUBLIC (in that order) that has any subnets.
*/
private reifySelectionDefaults(placement: SubnetSelection): SubnetSelection {
if (placement.subnetName !== undefined) {
if (placement.subnetGroupName !== undefined) {
throw new Error(`Please use only 'subnetGroupName' ('subnetName' is deprecated and has the same behavior)`);
}
placement = {...placement, subnetGroupName: placement.subnetName };
}

const exclusiveSelections: Array<keyof SubnetSelection> = ['subnets', 'subnetType', 'subnetGroupName'];
const providedSelections = exclusiveSelections.filter(key => placement[key] !== undefined);
if (providedSelections.length > 1) {
throw new Error(`Only one of '${providedSelections}' can be supplied to subnet selection.`);
}

if (placement.subnetType === undefined && placement.subnetGroupName === undefined && placement.subnets === undefined) {
// Return default subnet type based on subnets that actually exist
if (this.privateSubnets.length > 0) { return { subnetType: SubnetType.PRIVATE, onePerAz: placement.onePerAz }; }
if (this.isolatedSubnets.length > 0) { return { subnetType: SubnetType.ISOLATED, onePerAz: placement.onePerAz }; }
return { subnetType: SubnetType.PUBLIC, onePerAz: placement.onePerAz };
}

return placement;
}
}

/**
Expand Down Expand Up @@ -579,8 +608,9 @@ export interface VpcProps {
/**
* The number of NAT Gateways to create.
*
* For example, if set this to 1 and your subnet configuration is for 3 Public subnets then only
* one of the Public subnets will have a gateway and all Private subnets will route to this NAT Gateway.
* You can set this number lower than the number of Availability Zones in your
* VPC in order to save on NAT gateway cost. Be aware you may be charged for
* cross-AZ data traffic instead.
*
* @default - One NAT gateway per Availability Zone
*/
Expand Down Expand Up @@ -968,7 +998,11 @@ export class Vpc extends VpcBase {
this.vpcId = this.resource.ref;

this.subnetConfiguration = ifUndefined(props.subnetConfiguration, Vpc.DEFAULT_SUBNETS);
// subnetConfiguration and natGateways must be set before calling createSubnets

const natGatewayPlacement = props.natGatewaySubnets || { subnetType: SubnetType.PUBLIC };
const natGatewayCount = determineNatGatewayCount(props.natGateways, this.subnetConfiguration, this.availabilityZones.length);

// subnetConfiguration must be set before calling createSubnets
this.createSubnets();

const allowOutbound = this.subnetConfiguration.filter(
Expand All @@ -989,17 +1023,19 @@ export class Vpc extends VpcBase {
});

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

(this.privateSubnets as PrivateSubnet[]).forEach((privateSubnet, i) => {
let ngwId = this.natGatewayByAZ[privateSubnet.availabilityZone];
if (ngwId === undefined) {
const ngwArray = Array.from(Object.values(this.natGatewayByAZ));
// round robin the available NatGW since one is not in your AZ
ngwId = ngwArray[i % ngwArray.length];
}
privateSubnet.addDefaultNatRoute(ngwId);
});
if (natGatewayCount > 0) {
this.createNatGateways(natGatewayCount, natGatewayPlacement);

(this.privateSubnets as PrivateSubnet[]).forEach((privateSubnet, i) => {
let ngwId = this.natGatewayByAZ[privateSubnet.availabilityZone];
if (ngwId === undefined) {
const ngwArray = Array.from(Object.values(this.natGatewayByAZ));
// round robin the available NatGW since one is not in your AZ
ngwId = ngwArray[i % ngwArray.length];
}
privateSubnet.addDefaultNatRoute(ngwId);
});
}
}

if ((props.vpnConnections || props.vpnGatewayAsn) && props.vpnGateway === false) {
Expand Down Expand Up @@ -1075,24 +1111,12 @@ export class Vpc extends VpcBase {
});
}

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

const natCount = ifUndefined(gateways,
useNatGateway ? this.availabilityZones.length : 0);

let natSubnets: PublicSubnet[];
if (placement) {
const subnets = this.selectSubnetObjects(placement);
for (const sub of subnets) {
if (this.publicSubnets.indexOf(sub) === -1) {
throw new Error(`natGatewayPlacement ${placement} contains non public subnet ${sub}`);
}
private createNatGateways(natCount: number, placement: SubnetSelection): void {
let natSubnets: PublicSubnet[] = this.selectSubnetObjects(placement) as PublicSubnet[];
for (const sub of natSubnets) {
if (this.publicSubnets.indexOf(sub) === -1) {
throw new Error(`natGatewayPlacement ${placement} contains non public subnet ${sub}`);
}
natSubnets = subnets as PublicSubnet[];
} else {
natSubnets = this.publicSubnets as PublicSubnet[];
}

natSubnets = natSubnets.slice(0, natCount);
Expand Down Expand Up @@ -1462,33 +1486,6 @@ class ImportedVpc extends VpcBase {
}
}

/**
* 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.subnetName !== undefined) {
if (placement.subnetGroupName !== undefined) {
throw new Error(`Please use only 'subnetGroupName' ('subnetName' is deprecated and has the same behavior)`);
}
placement = {...placement, subnetGroupName: placement.subnetName };
}

const exclusiveSelections: Array<keyof SubnetSelection> = ['subnets', 'subnetType', 'subnetGroupName'];
const providedSelections = exclusiveSelections.filter(key => placement[key] !== undefined);
if (providedSelections.length > 1) {
throw new Error(`Only one of '${providedSelections}' can be supplied to subnet selection.`);
}

if (placement.subnetType === undefined && placement.subnetGroupName === undefined && placement.subnets === undefined) {
return { subnetType: SubnetType.PRIVATE, onePerAz: placement.onePerAz };
}

return placement;
}

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

Expand Down Expand Up @@ -1556,6 +1553,36 @@ class ImportedSubnet extends Resource implements ISubnet, IPublicSubnet, IPrivat
}
}

/**
* Determine (and validate) the NAT gateway count w.r.t. the rest of the subnet configuration
*
* We have the following requirements:
*
* - NatGatewayCount = 0 ==> there are no private subnets
* - NatGatewayCount > 0 ==> there must be public subnets
*
* Do we want to require that there are private subnets if there are NatGateways?
* They seem pointless but I see no reason to prevent it.
*/
function determineNatGatewayCount(requestedCount: number | undefined, subnetConfig: SubnetConfiguration[], azCount: number) {
const hasPrivateSubnets = subnetConfig.some(c => c.subnetType === SubnetType.PRIVATE);
const hasPublicSubnets = subnetConfig.some(c => c.subnetType === SubnetType.PUBLIC);

const count = requestedCount !== undefined ? Math.min(requestedCount, azCount) : (hasPrivateSubnets ? azCount : 0);

if (count === 0 && hasPrivateSubnets) {
// tslint:disable-next-line:max-line-length
throw new Error(`If you do not want NAT gateways (natGateways=0), make sure you don't configure any PRIVATE subnets in 'subnetConfiguration' (make them PUBLIC or ISOLATED instead)`);
}

if (count > 0 && !hasPublicSubnets) {
// tslint:disable-next-line:max-line-length
throw new Error(`If you configure PRIVATE subnets in 'subnetConfiguration', you must also configure PUBLIC subnets to put the NAT gateways into (got ${JSON.stringify(subnetConfig)}.`);
}

return count;
}

/**
* There are returned when the provider has not supplied props yet
*
Expand Down
53 changes: 44 additions & 9 deletions packages/@aws-cdk/aws-ec2/test/test.vpc.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -164,8 +164,8 @@ export = {
subnetConfiguration: [
{
cidrMask: 24,
subnetType: SubnetType.PRIVATE,
name: 'Private',
subnetType: SubnetType.PUBLIC,
name: 'Public',
},
{
cidrMask: 24,
Expand All @@ -192,7 +192,7 @@ export = {
{
cidrMask: 24,
name: 'ingress',
subnetType: SubnetType.PRIVATE,
subnetType: SubnetType.PUBLIC,
},
{
cidrMask: 24,
Expand Down Expand Up @@ -415,6 +415,18 @@ export = {
}
test.done();
},

'natGateways = 0 requires there to be no PRIVATE subnets'(test: Test) {
const stack = getTestStack();
test.throws(() => {
new Vpc(stack, 'VPC', {
natGateways: 0,
});
}, /make sure you don't configure any PRIVATE subnets/);
test.done();

},

'with mis-matched nat and subnet configs it throws'(test: Test) {
const stack = getTestStack();
test.throws(() => new Vpc(stack, 'VPC', {
Expand Down Expand Up @@ -480,7 +492,7 @@ export = {
const stack = getTestStack();
new Vpc(stack, 'VPC', {
subnetConfiguration: [
{ subnetType: SubnetType.PRIVATE, name: 'Private' },
{ subnetType: SubnetType.PUBLIC, name: 'Public' },
{ subnetType: SubnetType.ISOLATED, name: 'Isolated' },
],
vpnGateway: true,
Expand Down Expand Up @@ -514,6 +526,7 @@ export = {
const stack = getTestStack();
new Vpc(stack, 'VPC', {
subnetConfiguration: [
{ subnetType: SubnetType.PUBLIC, name: 'Public' },
{ subnetType: SubnetType.PRIVATE, name: 'Private' },
{ subnetType: SubnetType.ISOLATED, name: 'Isolated' },
],
Expand Down Expand Up @@ -749,7 +762,7 @@ export = {
const stack = getTestStack();
const vpc = new Vpc(stack, 'VPC', {
subnetConfiguration: [
{ subnetType: SubnetType.PRIVATE, name: 'Private' },
{ subnetType: SubnetType.PUBLIC, name: 'Public' },
{ subnetType: SubnetType.ISOLATED, name: 'Isolated' },
]
});
Expand All @@ -768,6 +781,7 @@ export = {
const stack = getTestStack();
const vpc = new Vpc(stack, 'VPC', {
subnetConfiguration: [
{ subnetType: SubnetType.PUBLIC, name: 'BlaBla' },
{ subnetType: SubnetType.PRIVATE, name: 'DontTalkToMe' },
{ subnetType: SubnetType.ISOLATED, name: 'DontTalkAtAll' },
]
Expand All @@ -786,6 +800,7 @@ export = {
const stack = getTestStack();
const vpc = new Vpc(stack, 'VPC', {
subnetConfiguration: [
{ subnetType: SubnetType.PUBLIC, name: 'BlaBla' },
{ subnetType: SubnetType.PRIVATE, name: 'DontTalkToMe' },
{ subnetType: SubnetType.ISOLATED, name: 'DontTalkAtAll' },
]
Expand All @@ -799,7 +814,25 @@ export = {
test.done();
},

'selecting default subnets in a VPC with only public subnets throws an error'(test: Test) {
'selecting default subnets in a VPC with only isolated subnets returns the isolateds'(test: Test) {
// GIVEN
const stack = new Stack();
const vpc = Vpc.fromVpcAttributes(stack, 'VPC', {
vpcId: 'vpc-1234',
availabilityZones: ['dummy1a', 'dummy1b', 'dummy1c'],
isolatedSubnetIds: ['iso-1', 'iso-2', 'iso-3'],
isolatedSubnetRouteTableIds: ['rt-1', 'rt-2', 'rt-3'],
});

// WHEN
const subnets = vpc.selectSubnets();

// THEN
test.deepEqual(subnets.subnetIds, ['iso-1', 'iso-2', 'iso-3']);
test.done();
},

'selecting default subnets in a VPC with only public subnets returns the publics'(test: Test) {
// GIVEN
const stack = new Stack();
const vpc = Vpc.fromVpcAttributes(stack, 'VPC', {
Expand All @@ -809,10 +842,11 @@ export = {
publicSubnetRouteTableIds: ['rt-1', 'rt-2', 'rt-3'],
});

test.throws(() => {
vpc.selectSubnets();
}, /There are no 'Private' subnet groups in this VPC. Available types: Public/);
// WHEN
const subnets = vpc.selectSubnets();

// THEN
test.deepEqual(subnets.subnetIds, ['pub-1', 'pub-2', 'pub-3']);
test.done();
},

Expand All @@ -834,6 +868,7 @@ export = {
const vpc = new Vpc(stack, 'VpcNetwork', {
maxAzs: 1,
subnetConfiguration: [
{name: 'lb', subnetType: SubnetType.PUBLIC },
{name: 'app', subnetType: SubnetType.PRIVATE },
{name: 'db', subnetType: SubnetType.PRIVATE },
]
Expand Down

0 comments on commit 9a96c37

Please sign in to comment.