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

(BREAKING) Enhancement - Custom VPC Subnets #250

Merged
merged 37 commits into from
Jul 24, 2018
Merged

(BREAKING) Enhancement - Custom VPC Subnets #250

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
28aea13
initial network builder design
Jul 6, 2018
2beb4a7
fixing tabs == 4 spaces in network-util.ts
Jul 10, 2018
2a172ed
moving to `get` methods for subnets, adding additional comment detail
Jul 10, 2018
9da7a2f
moving network-util vaildIp func to filter instead of reduce for read…
moofish32 Jul 10, 2018
55e3666
renaming of VpcSubnetBuilderProps to SubnetConfiguration; doc updates…
moofish32 Jul 10, 2018
a2b527d
removing custom error classes and asserting on regex message
moofish32 Jul 10, 2018
c9d4f39
removing more bad uses of reduce
moofish32 Jul 10, 2018
af75c41
network-utils doc improvements
moofish32 Jul 10, 2018
88a6d94
refactor complete snapping a line to debug
moofish32 Jul 11, 2018
9ac38f4
adding error for too many AZs requested
moofish32 Jul 11, 2018
60ef32d
additional comments for CIDR math and odd behaviors
moofish32 Jul 11, 2018
0f0079a
integ test can run but we are breaking existing here
moofish32 Jul 11, 2018
abd5086
making default vpc backwards compatible
moofish32 Jul 11, 2018
6dabebe
fixing linter errors
moofish32 Jul 11, 2018
7be9cc5
refactoring to keep backwards compatibility with vpc subnet generatio…
moofish32 Jul 12, 2018
2d1bb07
Merge remote-tracking branch 'origin/master' into f-improve-vpc-network
moofish32 Jul 12, 2018
1ef7db6
fixing imports removed incorrectly on conflict resolution
moofish32 Jul 12, 2018
06350b4
removed numazs
moofish32 Jul 12, 2018
455d9ef
fixing private subnet routes in default
moofish32 Jul 13, 2018
701858c
refactor to remove SubnetConfigFinalized
moofish32 Jul 15, 2018
5ecb64a
updating to support maxNatGateways
moofish32 Jul 16, 2018
a5ce5d9
updates for integ tests for rds and vpc
moofish32 Jul 18, 2018
2fbb213
fixing route53
moofish32 Jul 19, 2018
1777874
updating Changelog, README, and comments; reordered SubnetType to be …
moofish32 Jul 19, 2018
9d8067f
minor README in ec2 update
moofish32 Jul 19, 2018
d639de4
changes from the readme (impact to vpc.ts because the rename of the s…
moofish32 Jul 19, 2018
f2d4571
updating to fix `||` idiom; updating Changelog to explain breaking
moofish32 Jul 19, 2018
5c1bc5d
addressing code comments and tests for natGateway default behavior
moofish32 Jul 20, 2018
0864bb8
addressing comments for maxNatGateway -> natGateways, removing mapPub…
moofish32 Jul 23, 2018
776f1e0
refactoring cidrblock and network builder for number centric ip manag…
moofish32 Jul 23, 2018
82246ae
refactoring CidrBlock for constructor overloading, renaming nextIp ot…
moofish32 Jul 23, 2018
864a341
a couple of comment clarifications and making the CidrBlock construct…
moofish32 Jul 23, 2018
9722cdc
missed one README typo
moofish32 Jul 23, 2018
bba5e67
final README (ec2) correction to create Advanced Subnet Configuration
moofish32 Jul 23, 2018
c2baa77
Merge branch 'master' into f-improve-vpc-network
moofish32 Jul 24, 2018
31057b2
minor tweaks for renames from master merge
moofish32 Jul 24, 2018
f3f5428
README modification to match `import ec2 = require('@aws-cdk/aws-ec2')`
moofish32 Jul 24, 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
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* [FIXED] `cdk docs` works but a message __Unknown command: docs__ is printed ([#256])
* Lambda (feature): add `role` parameter, making it possible to specify an
externally defined execution role.
* VpcNetwork (BREAKING): add the ability customize subnet configurations ([#250])
* VpcNetwork (BREAKING): add the ability customize subnet configurations ([#250]). Subnet allocation was changed to improve IP space efficiency but this forces `VpcNetwork` instances to be replaced

[#258]: https://github.com/awslabs/aws-cdk/pull/258
[#250]: https://github.com/awslabs/aws-cdk/pull/250
Expand Down
35 changes: 24 additions & 11 deletions packages/@aws-cdk/ec2/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ import { VpcNetwork } from '@aws-cdk/ec2';

new VpcNetwork(stack, 'TheVPC', {
cidr: '10.0.0.0/21',
subnetConfigurations: [
subnetConfiguration: [
{
cidrMask: 24,
name: 'Ingress',
Expand All @@ -55,6 +55,9 @@ new VpcNetwork(stack, 'TheVPC', {
});
```

The example above is one possible configuration, but the user can use the
constructs above to implement many other network configurations.

The `VpcNetwork` from the above configuration in a Region with three
availability zones will be the following:
* IngressSubnet1: 10.0.0.0/24
Expand All @@ -67,26 +70,36 @@ availability zones will be the following:
* DatabaseSubnet2: 10.0.6.16/28
* DatabaseSubnet3: 10.0.6.32/28

There will be a NAT Gateway in each of the Ingress Subnets and each Application
Subnet will have a route to the NAT Gateway in it's respective availability
zone. The subnet numbers [1-3] will consistently map to the three availability
zones for the region. The Database Subnets will not have an outbound traffic
route and would be a good fit for managed data services like RDS, Elasticache,
and Redshift.
Each `Public` Subnet will have a NAT Gateway. Each `Private` Subnet will have a
route to the NAT Gateway in the same availability zone. Each `Internal` subnet
will not have a route to the internet, but is routeable inside the VPC. The
numbers [1-3] will consistently map to availability zones (e.g. IngressSubnet1
and ApplicaitonSubnet1 will be in the same avialbility zone).

`Internal` Subnets provide simplified secure networking principles, but come at
an operational complexity. The lack of an internet route means that if you deploy
instances in this subnet you will not be able to patch from the internet, this is
commonly reffered to as
[fully baked images](https://aws.amazon.com/answers/configuration-management/aws-ami-design/).
Features such as
[cfn-signal](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/cfn-signal.html)
are also unavailable. Using these subnets for managed services (RDS,
Elasticache, Redshift) is a very practical use because the managed services do
not incur additional operational overhead.

Many times when you plan to build an application you don't know how many
instances of the application you will need and therefore you don't know how much
IP space to allocate. For example, you know the application will only have
Elasitc Loadbalancers in the public subnets and you know you will have 1-3 RDS
Elastic Loadbalancers in the public subnets and you know you will have 1-3 RDS
databases for your data tier, and the rest of the IP space should just be evenly
distributed for the application.
distributed for the application.

```ts
import { VpcNetwork } from '@aws-cdk/ec2';

new VpcNetwork(stack, 'TheVPC', {
cidr: '10.0.0.0/16',
subnetConfigurations: [
subnetConfiguration: [
{
cidrMask: 26,
name: 'Public',
Expand Down Expand Up @@ -133,7 +146,7 @@ import { VpcNetwork } from '@aws-cdk/ec2';
new VpcNetwork(stack, 'TheVPC', {
cidr: '10.0.0.0/16',
maxNatGateways: 1,
subnetConfigurations: [
subnetConfiguration: [
{
cidrMask: 26,
name: 'Public',
Expand Down
152 changes: 100 additions & 52 deletions packages/@aws-cdk/ec2/lib/vpc.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,28 +75,32 @@ export interface VpcNetworkProps {
*
* For example if you want 1 public subnet, 1 private subnet, and 1 internal
* subnet in each AZ provide the following:
* subnets: [
* {
* cidrMask: 24,
* name: application,
* subnetType: SubnetType.Private,
* },
* {
* cidrMask: 26,
* name: ingress,
* subnetType: SubnetType.Public,
* natGateway: true,
* },
* {
* cidrMask: 28,
* name: rds,
* subnetType: SubnetType.Internal,
* }
* subnetConfiguration: [
* {
* cidrMask: 24,
* name: 'ingress',
* subnetType: SubnetType.Public,
* natGateway: true,
* },
* {
* cidrMask: 24,
* name: 'application',
* subnetType: SubnetType.Private,
* },
* {
* cidrMask: 28,
* name: 'rds',
* subnetType: SubnetType.Internal,
* }
* ]
*
* `cidrMask` is optional and if not provided the IP space in the VPC will be
* evenly divided between the requested subnets.
*
* @default the VPC CIDR will be evenly divided between 1 public and 1
* private subnet per AZ
*/
subnetConfigurations?: SubnetConfiguration[];
subnetConfiguration?: SubnetConfiguration[];
}

/**
Expand Down Expand Up @@ -128,20 +132,25 @@ export enum SubnetType {
Internal = 1,

/**
* Private subnets route outbound traffic via a NAT Gateway
* 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.
*
* Outbound traffic will be routed via a NAT Gateways preference being in
* Outbound traffic will be routed via a NAT Gateway. Preference being in
* the same AZ, but if not available will use another AZ. This is common for
* experimental cost conscious accounts or accounts where HA outbound
* traffic is not needed.
*/
Private = 2,

/**
* Public subnets route outbound traffic via an Internet Gateway
* Subnet connected to the Internet
*
* If this is set and OutboundTrafficMode.None is configure an error
* will be thrown.
* Instances in a Public subnet can connect to the Internet and can be
* connected to from the Internet as long as they are launched with public IPs.
*
* Public subnets route outbound traffic via an Internet Gateway.
*/
Public = 3

Expand All @@ -151,15 +160,44 @@ export enum SubnetType {
* Specify configuration parameters for a VPC to be built
*/
export interface SubnetConfiguration {
// the cidr mask value from 16-28
/**
* The CIDR Mask or the number of leading 1 bits in the routing mask
*
* Valid values are 16 - 28
Copy link
Contributor

Choose a reason for hiding this comment

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

@default Available space is automatically divided over maskless subnets

*/
cidrMask?: number;
// Public (IGW), Private (Nat GW), Internal (no outbound)

/**
* The type of Subnet to configure.
*
* The Subnet type will control the ability to route and connect to the
* Internet.
*/
subnetType: SubnetType;
// name that will be used to generate an AZ specific name e.g. name-2a

/**
* The common Logical Name for the `VpcSubnet`
*
* Thi name will be suffixed with an integer correlating to a specific
* availability zone.
*/
name: string;
// if true will place a NAT Gateway in this subnet, subnetType must be Public

/**
* Controls the creation a NAT Gateway in this subnet
*
* If true will place a NAT Gateway in this subnet, subnetType must be Public.
* Defaults to true in Subnet.Public, false in Subnet.Private or Subnet.Internal.
* NAT Gateways in non-public subnets will functionally not work and are
* therefore not possible to create.
*/
natGateway?: boolean;
// defaults to true in Subnet.Public, false in Subnet.Private or Subnet.Internal

/**
* Controls if a public IP is associated to an instance at launch
*
* Defaults to true in Subnet.Public, false in Subnet.Private or Subnet.Internal.
*/
mapPublicIpOnLaunch?: boolean;
}

Expand Down Expand Up @@ -193,17 +231,7 @@ export class VpcNetwork extends VpcNetworkRef {
public static readonly DEFAULT_CIDR_RANGE: string = '10.0.0.0/16';

/**
* The default maximum number of NAT Gateways
*
* @link https://docs.aws.amazon.com/AmazonVPC/latest/UserGuide/VPC_Appendix_Limits.html
* defaulting 256 is an arbitrary max that should be impracticel to reach.
* This can be overriden using VpcNetworkProps when creating a VPCNetwork resource.
* e.g. new VpcResource(this, { maxNatGateways: 1 })
*/
public static readonly MAX_NAT_GATEWAYS: number = 256;

/**
* The deafult subnet configuration
* The default subnet configuration
*
* 1 Public and 1 Private subnet per AZ evenly split
*/
Expand Down Expand Up @@ -243,7 +271,7 @@ export class VpcNetwork extends VpcNetworkRef {
/**
* Maximum Number of NAT Gateways used to control cost
*
* @default {VpcNetwork.MAX_NAT_GATEWAYS}
* @default {VpcNetworkProps.maxAZs}
*/
private readonly maxNatGateways: number;

Expand All @@ -265,7 +293,7 @@ export class VpcNetwork extends VpcNetworkRef {
/**
* Subnet configurations for this VPC
*/
private subnetConfigurations: SubnetConfiguration[] = [];
private subnetConfiguration: SubnetConfiguration[] = [];

/**
* Maximum AZs to Uses for this VPC
Expand All @@ -288,7 +316,7 @@ export class VpcNetwork extends VpcNetworkRef {
throw new Error('To use DNS Hostnames, DNS Support must be enabled, however, it was explicitly disabled.');
}

const cidrBlock = props.cidr || VpcNetwork.DEFAULT_CIDR_RANGE;
const cidrBlock = ifUndefined(props.cidr, VpcNetwork.DEFAULT_CIDR_RANGE);
this.networkBuilder = new NetworkBuilder(cidrBlock);

const enableDnsHostnames = props.enableDnsHostnames == null ? true : props.enableDnsHostnames;
Expand All @@ -314,15 +342,15 @@ export class VpcNetwork extends VpcNetworkRef {
this.vpcId = this.resource.ref;
this.dependencyElements.push(this.resource);

this.maxNatGateways = props.maxNatGateways || VpcNetwork.MAX_NAT_GATEWAYS;
this.maxNatGateways = ifUndefined(props.maxNatGateways, this.availabilityZones.length);

this.subnetConfigurations = props.subnetConfigurations || VpcNetwork.DEFAULT_SUBNETS;
this.subnetConfiguration = ifUndefined(props.subnetConfiguration, VpcNetwork.DEFAULT_SUBNETS);
this.createSubnets();

const allowOutbound = this.subnetConfigurations.filter(
(subnet) => (subnet.subnetType !== SubnetType.Internal)).length > 0;
const allowOutbound = this.subnetConfiguration.filter(
subnet => (subnet.subnetType !== SubnetType.Internal)).length > 0;

// Create an Internet Gateway and attach it (if the outbound traffic mode != None)
// Create an Internet Gateway and attach it if necessary
if (allowOutbound) {
const igw = new cloudformation.InternetGatewayResource(this, 'IGW');
const att = new cloudformation.VPCGatewayAttachmentResource(this, 'VPCGW', {
Expand Down Expand Up @@ -363,10 +391,7 @@ export class VpcNetwork extends VpcNetworkRef {

// Calculate number of public/private subnets based on number of AZs

for (const subnet of this.subnetConfigurations) {
subnet.mapPublicIpOnLaunch = subnet.mapPublicIpOnLaunch ||
(subnet.subnetType === SubnetType.Public);

for (const subnet of this.subnetConfiguration) {
if (subnet.cidrMask === undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

With less variables and state munging:

// Partition subnets into defined size, remaining size
const definedSizeSubnets = this.subnetConfigurations.filter(s => s.cidrMask !== undefined);
const remainingSpaceSubnets = this.subnetConfigurations.filter(s => s.cidrMask === undefined);

// Allocate defines sizes
definedSizeSubnets.forEach(subnet => this.createSubnetResources(subnet, subnet.cidrMask));

// Allocate remaining sizes
...

But we still don't sort our CIDRs in the same order as in the argument list, do we? Because the automatically allocated ones will always go at the end now. So we should actually do the mask calculation before starting to add any subnets at all.

I see how this will be some hard math to do properly, especially as soon as there are multiple unmasked blocks. At the same time, I DO like to have the guarantee that the subnets are laid out in the space in the order mentioned in the config. So if it's too hard to do this math, then we add a restriction that unassigned cidrMasks must all be at the end of the array. That allows us to keep the invariant until we figure out a better way to deal with this calculation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should have discussed this in more detail on the call. I think we still have a gap here.

First the filter change. Will filter always preserve ordering in other languages? If so I'll make that change.

Re: do the mask calculation before starting -> I agree and considered this, but I don't think it buys us much. Even if we do it first and then create the subnetConfiguration and do it for real, the user can still almost never update the subnet configurations because a change will likely modify the "remaining space" and cloudformation will try to destroy/create.

What I think currently is updating a VPC is already a low probability event once you get to prod (even today with CFN it rarely happens). So if you plan to make modifications to your VPC, then you really should not use any remaining space subnets. Does that mean we should prevent the mixing of these types or leave it similar to how it functions currently? I think I lean towards separating the two because it's an easier mental model. However, I already have docs that discuss the likely use case of mixing them.

Copy link
Contributor

@rix0rrr rix0rrr Jul 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Array.filter does maintain order, so that is safe.

And I agree that automatic space allocation combined with extending the nets is probably always a bad idea. In the best case, the automatic allocation left some wasted space that you now have to explicitly specify, and you get to create some nets in the previously wasted space.

Even though it should have been, this wasn't immediately obvious to me :). I guess it might be worthwhile to add a comment on the cidrMask property to this effect ("if you choose to have this value automatically calculated for one of the subnets, it will be impossible to add additional subnets later without replacing them" or similar).

All that is to say: you're right, no need to change this.

remainingSpaceSubnets.push(subnet);
continue;
Expand All @@ -388,7 +413,8 @@ export class VpcNetwork extends VpcNetworkRef {
availabilityZone: zone,
vpcId: this.vpcId,
cidrBlock: this.networkBuilder.addSubnet(cidrMask),
mapPublicIpOnLaunch: subnetConfig.mapPublicIpOnLaunch
mapPublicIpOnLaunch: ifUndefined(subnetConfig.mapPublicIpOnLaunch,
(subnetConfig.subnetType === SubnetType.Public)),
};

switch (subnetConfig.subnetType) {
Expand Down Expand Up @@ -419,9 +445,27 @@ export class VpcNetwork extends VpcNetworkRef {
* Specify configuration parameters for a VPC subnet
*/
export interface VpcSubnetProps {

/**
* The availability zone for the subnet
*/
availabilityZone: string;

/**
* The VPC which this subnet is part of
*/
vpcId: Token;

/**
* The CIDR notation for this subnet
*/
cidrBlock: string;

/**
* Controls if a public IP is associated to an instance at launch
*
* Defaults to true in Subnet.Public, false in Subnet.Private or Subnet.Internal.
*/
mapPublicIpOnLaunch?: boolean;
}

Expand Down Expand Up @@ -532,3 +576,7 @@ export class VpcPrivateSubnet extends VpcSubnet {
this.addDefaultRouteToNAT(natGatewayId);
}
}

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