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

feat(aws-ec2) Configure NAT to Subnet Placement #874

Merged
merged 3 commits into from
Oct 10, 2018
Merged

feat(aws-ec2) Configure NAT to Subnet Placement #874

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
a6fb990
BREAKING(aws-ec2) Configure NAT to Subnet Placement
moofish32 Oct 9, 2018
ef3e4a0
feat(aws-ec2) Configure NAT to Subnet Placement (#874)
moofish32 Oct 9, 2018
83f61e1
Update nat gateway placement docs
rix0rrr Oct 10, 2018
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
87 changes: 59 additions & 28 deletions packages/@aws-cdk/aws-ec2/lib/vpc.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import cdk = require('@aws-cdk/cdk');
import { cloudformation } from './ec2.generated';
import { NetworkBuilder } from './network-util';
import { DEFAULT_SUBNET_NAME, subnetId } from './util';
import { DEFAULT_SUBNET_NAME, subnetId, subnetName } from './util';
import { SubnetType, VpcNetworkRef, VpcSubnetRef } from './vpc-ref';

/**
Expand Down Expand Up @@ -61,16 +61,9 @@ export interface VpcNetworkProps {
maxAZs?: number;

/**
* Define the maximum number of NAT Gateways for this VPC
*
* Setting this number enables a VPC to trade availability for the cost of
* running a NAT Gateway. For example, if set this to 1 and your subnet
* configuration is for 3 Public subnets with natGateway = `true` then only
* one of the Public subnets will have a gateway and all Private subnets
* will route to this NAT Gateway.
* @default maxAZs
* Define the NAT Gateway Configuration for this VPC
*/
natGateways?: number;
natGateways?: NatGatewayConfiguration;
Copy link
Contributor

Choose a reason for hiding this comment

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

If we put both those arguments on the top-level object this doesn't even have to be a breaking change.


/**
* Configure the subnets to build for each AZ
Expand Down Expand Up @@ -123,6 +116,27 @@ export enum DefaultInstanceTenancy {
Dedicated = 'dedicated'
}

export interface NatGatewayConfiguration {
/**
* 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.
* @default maxAZs
*/
gatewayCount?: number;

/**
* The names of the subnets that will have NAT Gateways
*
* The names of the corresponding subnets in `SubnetConfiguration` that will
* have a NAT Gateway. If a corresponding subnet name is not found this will
* throw an error. By default the first public subnets will receive NAT
* Gateways until the `gatewayCount` is reached.
*/
subnetName?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not a VpcPlacementStrategy object?

}

/**
* Specify configuration parameters for a VPC to be built
*/
Expand Down Expand Up @@ -231,13 +245,6 @@ export class VpcNetwork extends VpcNetworkRef implements cdk.ITaggable {
*/
public readonly tags: cdk.TagManager;

/**
* Maximum Number of NAT Gateways used to control cost
*
* @default {VpcNetworkProps.maxAZs}
*/
private readonly natGateways: number;

/**
* The VPC resource
*/
Expand Down Expand Up @@ -303,8 +310,6 @@ export class VpcNetwork extends VpcNetworkRef implements cdk.ITaggable {
this.subnetConfiguration = ifUndefined(props.subnetConfiguration, VpcNetwork.DEFAULT_SUBNETS);
const useNatGateway = this.subnetConfiguration.filter(
subnet => (subnet.subnetType === SubnetType.Private)).length > 0;
this.natGateways = ifUndefined(props.natGateways,
useNatGateway ? this.availabilityZones.length : 0);

// subnetConfiguration and natGateways must be set before calling createSubnets
this.createSubnets();
Expand All @@ -321,12 +326,33 @@ export class VpcNetwork extends VpcNetworkRef implements cdk.ITaggable {
internetGatewayId: igw.ref,
vpcId: this.resource.ref
});
this.dependencyElements.push(igw, att);

const natConfig = ifUndefined(props.natGateways, {
Copy link
Contributor

Choose a reason for hiding this comment

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

A shorter way to get the same effect would be:

const natConfig = props.natGateways || {};

(Which works in this case because props.natGateways MUST be an object, so only undefined will test as falsey, and missing members of an object will also return as undefined)

gatewayCount: undefined,
subnetName: undefined,
});
const natCount = ifUndefined(natConfig.gatewayCount,
useNatGateway ? this.availabilityZones.length : 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, what if you set both gatewayCount=3 and useNatGateway=false? That doesn't make sense, you should get slapped.

How about useNatGateway goes away and you can simulate useNatGateway=false by setting gatewayCount=0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

useNatGateway is logic based and not exposed to the developer. I can try to bring this closer together, but the only way to know the count should be 0 is by checking if there are no private subnets. Are you asking for something different there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This comment needs one more review with the latest commit

const natSubnet = natConfig.subnetName;

if (natSubnet !== undefined) {
const subnetNames = (this.publicSubnets as VpcPublicSubnet[]).map( subnet => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest taking a VpcPlacementStrategy, using this.subnets() to turn the strategy into a list of subnets, then using isPublicSubnet() to test if they're public.

Then you can subnets.slice() to the maximum number of allowed gateways, and loop over the remaining array to add a gateway to each of them.

return subnetName(subnet);
});
if (!subnetNames.includes(natSubnet)) {
throw new Error(`NatGatewayConfiguration contains subnet name ${natSubnet} which is not a Public Subnet in SubnetConfiguration`);
}
}

(this.publicSubnets as VpcPublicSubnet[]).forEach(publicSubnet => {
publicSubnet.addDefaultIGWRouteEntry(igw.ref);
const currentNatCount = Object.values(this.natGatewayByAZ).length;
if (addNatGatewy(publicSubnet, natSubnet, natCount, currentNatCount)) {
this.natGatewayByAZ[publicSubnet.availabilityZone] = publicSubnet.addNatGateway();
}
});

this.dependencyElements.push(igw, att);

(this.privateSubnets as VpcPrivateSubnet[]).forEach((privateSubnet, i) => {
let ngwId = this.natGatewayByAZ[privateSubnet.availabilityZone];
if (ngwId === undefined) {
Expand Down Expand Up @@ -386,12 +412,6 @@ export class VpcNetwork extends VpcNetworkRef implements cdk.ITaggable {
switch (subnetConfig.subnetType) {
case SubnetType.Public:
const publicSubnet = new VpcPublicSubnet(this, name, subnetProps);
if (this.natGateways > 0) {
const ngwArray = Array.from(Object.values(this.natGatewayByAZ));
if (ngwArray.length < this.natGateways) {
this.natGatewayByAZ[zone] = publicSubnet.addNatGateway();
}
}
this.publicSubnets.push(publicSubnet);
break;
case SubnetType.Private:
Expand Down Expand Up @@ -561,5 +581,16 @@ export class VpcPrivateSubnet extends VpcSubnet {
}

function ifUndefined<T>(value: T | undefined, defaultValue: T): T {
return value !== undefined ? value : defaultValue;
return value !== undefined ? value : defaultValue;
}

function addNatGatewy(subnet: VpcPublicSubnet, natSubnet: string | undefined, maxNats: number, natsCreated: number): boolean {
const name = subnetName(subnet);
if (natSubnet !== undefined && natSubnet !== name) {
return false;
}
if (natsCreated >= maxNats) {
return false;
}
return true;
}
67 changes: 63 additions & 4 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 @@ -171,7 +171,7 @@ export = {
const stack = getTestStack();
new VpcNetwork(stack, 'TheVPC', {
cidr: '10.0.0.0/21',
natGateways: 2,
natGateways: { gatewayCount: 2},
subnetConfiguration: [
{
cidrMask: 24,
Expand Down Expand Up @@ -251,17 +251,76 @@ export = {
},
"with natGateway set to 1"(test: Test) {
const stack = getTestStack();
new VpcNetwork(stack, 'VPC', { natGateways: 1 });
new VpcNetwork(stack, 'VPC', {
natGateways: { gatewayCount: 1 }
});
expect(stack).to(countResources("AWS::EC2::Subnet", 6));
expect(stack).to(countResources("AWS::EC2::Route", 6));
expect(stack).to(countResources("AWS::EC2::Subnet", 6));
expect(stack).to(countResources("AWS::EC2::NatGateway", 1));
expect(stack).to(haveResource("AWS::EC2::Route", {
DestinationCidrBlock: '0.0.0.0/0',
NatGatewayId: { },
}));
test.done();
}
},
'with natGateway subnets defined'(test: Test) {
const stack = getTestStack();
new VpcNetwork(stack, 'VPC', {
subnetConfiguration: [
{
cidrMask: 24,
name: 'ingress',
subnetType: SubnetType.Public,
},
{
cidrMask: 24,
name: 'egress',
subnetType: SubnetType.Public,
},
{
cidrMask: 24,
name: 'private',
subnetType: SubnetType.Private,
},
],
natGateways: {
subnetName: 'egress',
},
});
expect(stack).to(countResources("AWS::EC2::NatGateway", 3));
for (let i = 1; i < 4; i++) {
expect(stack).to(haveResource("AWS::EC2::NatGateway", {
Tags: [
{
Key: 'Name',
Value: `VPC/egressSubnet${i}`,
}
]
}));
}
test.done();
},
'with mis-matched nat and subnet configs it throws'(test: Test) {
const stack = getTestStack();
test.throws(() => new VpcNetwork(stack, 'VPC', {
subnetConfiguration: [
{
cidrMask: 24,
name: 'ingress',
subnetType: SubnetType.Public,
},
{
cidrMask: 24,
name: 'private',
subnetType: SubnetType.Private,
},
],
natGateways: {
subnetName: 'notthere',
},
}));
test.done();
},

},

Expand Down