Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(vpc): additional validation around Subnet Types #4668

Merged
merged 3 commits into from
Oct 27, 2019
Merged

feat(vpc): additional validation around Subnet Types #4668

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0a74433
feat(vpc): additional validation around Subnet Types
rix0rrr Oct 24, 2019
e3053c7
Merge remote-tracking branch 'origin/master' into huijbers/vpcs-subne…
rix0rrr Oct 25, 2019
9f760eb
Merge branch 'master' into huijbers/vpcs-subnet-types
mergify[bot] Oct 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
161 changes: 94 additions & 67 deletions packages/@aws-cdk/aws-ec2/lib/vpc.ts
View file
Open in desktop
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this description of @default is super clear... ISOLATED or PUBLIC? I don't think this is a valid value... Do you mean whichever value resolves to a subnet first in that order?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do mean that. I can come up with a description of that behavior that is super technically correct ("The first one out of PRIVATE, ISOLATED, PUBLIC that has subnets of that type configured"), but not one that's easy to read.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, @rix0rrr I like you proposed change (and the value should be -). It should be something like:

/**
 * @default - the default subnet type will be the first available type in the following order: PRIVATE, ISOLATE, PUBLIC
 */

Something like that.

*/
readonly subnetType?: SubnetType;

Expand Down Expand Up @@ -337,7 +336,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.subnetGroupName !== undefined) { // Select by name
return this.selectSubnetObjectsByName(selection.subnetGroupName);
Expand Down Expand Up @@ -384,6 +383,34 @@ 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 };
}

if (placement.subnetType !== undefined && placement.subnetGroupName !== undefined) {
throw new Error(`Only one of 'subnetType' and 'subnetGroupName' can be supplied`);
}

if (placement.subnetType === undefined && placement.subnetGroupName === 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 @@ -563,8 +590,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 @@ -952,7 +980,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 @@ -973,17 +1005,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 @@ -1059,24 +1093,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 @@ -1446,31 +1468,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 };
}

if (placement.subnetType !== undefined && placement.subnetGroupName !== undefined) {
throw new Error(`Only one of 'subnetType' and 'subnetGroupName' can be supplied`);
}

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

return placement;
}

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

Expand Down Expand Up @@ -1538,6 +1535,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
Open in desktop
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/);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps provide the rationale for this in the error message or an accompanying link to an issue or doc

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