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.

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
Show file tree
Hide file tree
Changes from 7 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
150 changes: 150 additions & 0 deletions packages/@aws-cdk/ec2/lib/network-util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,4 +86,154 @@ export class NetworkUtils {

}

public static validIp(ipAddress: string): boolean {
return ipAddress.split('.').map((octet: string) => parseInt(octet, 10)).
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't think this function is correct.

Won't it return true for the following:

validIp('1.2.3.4.689')

?

Needs a docstring too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done - added tests to verify

filter((octet: number) => octet >= 0 && octet <= 255).length === 4;
}
/**
*
Copy link
Contributor

Choose a reason for hiding this comment

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

Please put whitespace between functions, start docstrings with a short caption string (start with a capital), and don't put whitespace between the docstring and the function itself. So this:

   return x;
}

/**
 * This function does things.
 *
 * Further explanation...
 */
function doThings() {
}

And not this:

   return x;
}
/**
 *
 * This function does things. Further explanation
 * and then some.
 *
 * Even more explanation...
 */

function doThings() {
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

* takes an IP Address (e.g. 174.66.173.168) and converts to a number
* (e.g 2923605416); currently only supports IPv4
*
* Uses the formula:
* (first octet * 256³) + (second octet * 256²) + (third octet * 256) +
* (fourth octet)
*
* @param {string} the IP address (e.g. 174.66.173.168)
* @returns {number} the integer value of the IP address (e.g 2923605416)
*/

public static ipToNum(ipAddress: string): number {
if (!this.validIp(ipAddress)) {
throw new Error(`${ipAddress} is not valid`);
}

return ipAddress
.split('.')
.reduce(
(p: number, c: string, i: number) => p + parseInt(c, 10) * 256 ** (3 - i),
0
);
}

/**
*
Copy link
Contributor

Choose a reason for hiding this comment

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

Same remark about doc comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

* Takes a number (e.g 2923605416) and converts it to an IPv4 address string
* currently only supports IPv4
*
* @param {number} the integer value of the IP address (e.g 2923605416)
* @returns {string} the IPv4 address (e.g. 174.66.173.168)
*/

public static numToIp(ipNum: number): string {
// this all because bitwise math is signed
let remaining = ipNum;
const address = [];
for (let i = 0; i < 4; i++) {
if (remaining !== 0) {
address.push(Math.floor(remaining / 256 ** (3 - i)));
remaining = remaining % 256 ** (3 - i);
} else {
address.push(0);
}
}
const ipAddress: string = address.join('.');
if ( !this.validIp(ipAddress) ) {
throw new Error(`${ipAddress} is not a valid IP Address`);
}
return ipAddress;
}

}

export class NetworkBuilder {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should have a documentation comment describing the functionality, and a usage example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes anything exported will get full docs. Hopefully I get a complete draft today/tonight

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome. Thanks for contributing too!

public readonly networkCidr: CidrBlock;
protected subnetCidrs: CidrBlock[];
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this class being inherited from? If not, private instead of public.

(And even if so, take a long hard look and see if protected would really be necessary)

Can also be initialized here to an empty array:

private readonly subnetCidrs: CidrBlock[] = [];

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed used private readonly subnetCidrs: CidrBlock[] = [];

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also converted all protected -- I thought I would extend this for NetworkBuilder but chose to separate it.

protected maxIpConsumed: string;

constructor(cidr: string) {
this.networkCidr = new CidrBlock(cidr);
this.subnetCidrs = [];
this.maxIpConsumed = NetworkUtils.numToIp(NetworkUtils.
ipToNum(this.networkCidr.minIp()) - 1);
}

public addSubnet(mask: number): string {
return this.addSubnets(mask, 1)[0];
}

public addSubnets(mask: number, count?: number): string[] {
if (mask < 16 || mask > 28) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am being insanely pedantic - but the /16 is in fact a soft limit 😬

I don't think it's something we need to change right now - but just something we need to think about how we want to handle in the future (more services have limits like this, where like 95% of people will hit the default limits, but not everyone)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree - I was hoping that was out of scope for this first round since /16 was already in here before me.

This brings up a question I was wondering about: https://docs.aws.amazon.com/AmazonVPC/latest/UserGuide/VPC_Subnets.html#VPC_Sizing. The recommended is use Private space and play by the rules, but technically I could deploy things that are not very useful. Where should we draw the line between good practice and rule here? Not sure this PR is the best place for a general discussion, but I'm definitely not sure how to answer this question tonight.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also not enforce that the requested blocks can fit in the remaining address space beforehand? Seems simpler than calling validate() in a loop.

Can we not do something like

if (netSizeOf(mask) * count > this.networkCidr.pastEndIp() - this.nextIp) {
  throw new Error('Not enough space');
}

throw new InvalidCidrRangeError(`x.x.x.x/${mask}`);
}
const subnets: CidrBlock[] = [];
if (!count) {
count = 1;
}
for (let i = 0; i < count; i ++) {
const subnet: CidrBlock = CidrBlock.fromOffsetIp(this.maxIpConsumed, mask);
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems more logical to me to instead of using maxIpConsumed (and initialize to start-1 and subsequently updated to max), to use something like nextAvailableIp.

It gets initialized to networkBlock.minIp() and subsequently updated to subnet.nextBlock().minIp() or subnet.pastEndIp().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, went with nextBlock()

this.maxIpConsumed = subnet.maxIp();
this.subnetCidrs.push(subnet);
if (!this.validate()) {
throw new Error(`${this.networkCidr.cidr} does not fully contain ${subnet.cidr}`);
}
subnets.push(subnet);
}
return subnets.map((subnet) => (subnet.cidr));
}

public get subnets(): string[] {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if subnets is the best name for this - given that we map sunbetCidrs to get only the cidr property?

(I'm not a stickler for names - so I'll defer to how you feel about it)

Copy link
Contributor

Choose a reason for hiding this comment

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

cidrStrings()?

I don't really have a good name for this, but I feel there should be a word that describes the "string representation of a CIDR block" in "x.x.x.x/y" form, which is distinct from CIDR (which just describes the block, afai understand), and subnet (which COULD be this but in the context of AWS I would expect it to represent a Subnet object)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

return this.subnetCidrs.map((subnet) => (subnet.cidr));
}

public validate(): boolean {
return this.subnetCidrs.map(
(cidrBlock) => this.networkCidr.containsCidr(cidrBlock)).
filter( (contains) => (contains === false)).length === 0;
}

}

export class CidrBlock {

public static calculateNetmask(mask: number): string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Is public the right visibility? Seems like we're using this only in the constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good question. I made this public because of the export. I only export to test, perhaps that is a lack of Typescript testing. However, once somebody else could see this class then I would expect this static method to exist like it does in many other languages (e.g. Golang https://golang.org/pkg/net/#CIDRMask). I'm happy to make a change if there is a strong opinion in the other direction, but that's how I got here.

return NetworkUtils.numToIp(2 ** 32 - 2 ** (32 - mask));
}

public static fromOffsetIp(ipAddress: string, mask: number): CidrBlock {
const initialCidr = new CidrBlock(`${ipAddress}/${mask}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Feels like this should just be a method on CidrBlock, no?

Given that you already have a block, you want to answer the question "what's the next block?"

cidrBlock.nextBlock()
cidrBlock.shiftBlock()  # Or maybe this name is better

Which would behave similarly to cidrBlock + 1 (if that were how CIDR math worked?)


Simialrly, I have a feeling that if we were to parse the CidrBlock into two numbers (in the constructor, that we store on the class)

  • networkAddress (integer representation of the network address)
  • netSize (which is 2**(32 - mask))

Then a lot of the operations on this become a lot easier.

nextBlock() == new CidrBlock(networkAddress + netSize, netSize)
minIp() == NumToIp(networkAddress)
maxIp() == NumToIp(networkAddress + netSize - 1)
contains(other) => this.networkAddress <= other.networkAddress && other.maxAddress <= this.maxAddress

Right? There's a lot of numToIp() and and IpToNum() happening that really only needs to happen at the edges, where the system interacts with humans. Internally everything can be binary numbers.

It should be possible to overload the constructor in TypeScript, so that we can pass either (string) or (number, number), so it will become very convenient to do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok -- I think this is done. I'm not sure Typescript and I agree on constructor overloading design, but perhaps there is a simpler way and I didn't know it?

const baseIpNum = NetworkUtils.ipToNum(initialCidr.maxIp()) + 1;
return new this(`${NetworkUtils.numToIp(baseIpNum)}/${mask}`);
}

public readonly cidr: string;
public readonly netmask: string;
public readonly mask: number;

constructor(cidr: string) {
this.cidr = cidr;
this.mask = parseInt(cidr.split('/')[1], 10);
this.netmask = CidrBlock.calculateNetmask(this.mask);
}

public maxIp(): string {
const minIpNum = NetworkUtils.ipToNum(this.minIp());
return NetworkUtils.numToIp(minIpNum + 2 ** (32 - this.mask) - 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think just a comment explaining this line will help future CDKers 😃

}

Copy link
Contributor

Choose a reason for hiding this comment

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

Eventually make sure to add jsdocs to all exported entities

public minIp(): string {
const netmaskOct = this.netmask.split('.');
const ipOct = this.cidr.split('/')[0].split('.');
// tslint:disable:no-bitwise
return netmaskOct.map(
(maskOct, index) => parseInt(maskOct, 10) & parseInt(ipOct[index], 10)).join('.');
Copy link
Contributor

Choose a reason for hiding this comment

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

Likewise.

// tslint:enable:no-bitwise
}

public containsCidr(cidr: CidrBlock): boolean {
return [
NetworkUtils.ipToNum(this.minIp()) <= NetworkUtils.ipToNum(cidr.minIp()),
NetworkUtils.ipToNum(this.maxIp()) >= NetworkUtils.ipToNum(cidr.maxIp()),
].filter((contains) => (contains === false)).length === 0;
}
}
159 changes: 153 additions & 6 deletions packages/@aws-cdk/ec2/lib/vpc.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { AvailabilityZoneProvider, Construct, Tag, Token } from '@aws-cdk/core';
import { ec2 } from '@aws-cdk/resources';
import { NetworkUtils } from './network-util';
import { Obj } from '@aws-cdk/util';
import { NetworkBuilder, NetworkUtils } from './network-util';
import { VpcNetworkId, VpcNetworkRef, VpcSubnetId, VpcSubnetRef } from './vpc-ref';
/**
* VpcNetworkProps allows you to specify configuration options for a VPC
Expand Down Expand Up @@ -58,6 +59,33 @@ export interface VpcNetworkProps {
* @default All AZs in the region
*/
maxAZs?: number;

/**
* Configure the subnets to build for each AZ
*
Copy link
Contributor

Choose a reason for hiding this comment

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

Use "@default" to describe the default behavior

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, what is the default VPC configuration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

currently I left the default as is. Likely I'll keep this end state the same, but replace reduce the split cidr logic.

* The subnets are constructed in the context of the VPC so you only need
* specify the configuration. The VPC details (VPC ID, specific CIDR,
* specific AZ will be calculated during creation)
*
* For example if you want three private subnets and three public subnets
* across 3 AZs then maxAZs = 3 and provide the following:
* subnets: [
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix indentation here.

Strings need quotes.

Some note about optionality of cidrMask would be great,

* {
* cidrMask: 24,
* name: ingress,
* subnetType: SubnetType.Public,
* natGateway: true,
* },
* {
* cidrMask: 24,
* name: application,
* subnetType: SubnetType.Private,
* }
* ]
* @default the VPC CIDR will be evenly divided between 1 public and 1
* private subnet per AZ
*/
subnetConfigurations?: SubnetConfiguration[];
}

/**
Expand Down Expand Up @@ -142,6 +170,11 @@ export class VpcNetwork extends VpcNetworkRef {
*/
public readonly privateSubnets: VpcSubnetRef[] = [];

/**
* List of internal subnets in this VPC
*/
public readonly internalSubnets: VpcSubnetRef[] = [];

/**
* The VPC resource
*/
Expand Down Expand Up @@ -185,7 +218,7 @@ export class VpcNetwork extends VpcNetworkRef {
outboundTraffic === OutboundTrafficMode.FromPublicAndPrivateSubnets;

// Create public and private subnets in each AZ
this.createSubnets(cidrBlock, outboundTraffic, props.maxAZs);
this.createSubnets(cidrBlock, outboundTraffic, props.maxAZs, props.subnetConfigurations);

// Create an Internet Gateway and attach it (if the outbound traffic mode != None)
if (allowOutbound) {
Expand Down Expand Up @@ -213,7 +246,11 @@ export class VpcNetwork extends VpcNetworkRef {
* createSubnets takes a VPC, and creates a public and private subnet
* in each Availability Zone.
*/
private createSubnets(cidr: string, outboundTraffic: OutboundTrafficMode, maxAZs?: number) {
private createSubnets(
cidr: string,
outboundTraffic: OutboundTrafficMode,
maxAZs?: number,
subnets?: SubnetConfiguration[]) {

// Calculate number of public/private subnets based on number of AZs
const zones = new AvailabilityZoneProvider(this).availabilityZones;
Expand All @@ -224,11 +261,16 @@ export class VpcNetwork extends VpcNetworkRef {
zones.splice(maxAZs);
}

// Split the CIDR range into each availablity zone
const ranges = NetworkUtils.splitCIDR(cidr, zones.length);
if (subnets != null) {
const networkBuilder = new NetworkBuilder(cidr);
this.createCustomSubnets(networkBuilder, subnets, zones);
} else {
// Split the CIDR range into each availablity zone
Copy link
Contributor

Choose a reason for hiding this comment

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

Why can't we use NetworkBuilder for this use case as well? If I understand correctly, should be possible... Less branching

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 was just going for minimal change, but I can easily do this.

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 like to see that if subnets is not supplied by the user, it is generated so that it would lead the same effective behavior here without the if. We then make the subnet configuration required. Let's minimize the amount of code paths that are taken.

When you do so, please run the integ tests to verify that the generated template is still the same.

const ranges = NetworkUtils.splitCIDR(cidr, zones.length);

for (let i = 0; i < zones.length; i++) {
for (let i = 0; i < zones.length; i++) {
this.createSubnetPair(ranges[i], zones[i], i + 1, outboundTraffic);
}
}

}
Expand Down Expand Up @@ -263,9 +305,114 @@ export class VpcNetwork extends VpcNetworkRef {
this.privateSubnets.push(privateSubnet);
this.dependencyElements.push(publicSubnet, privateSubnet);
}
private createCustomSubnets(builder: NetworkBuilder, subnets: SubnetConfiguration[], zones: string[]) {

const natGatewayByAZ: Obj<Token> = {};

for (const subnet of subnets) {
let azs = zones;
if (subnet.numAZs != null) {
azs = zones.slice(subnet.numAZs);
}
for (const zone of azs) {
const cidr: string = builder.addSubnet(subnet.cidrMask);
const name: string = `${subnet.name}AZ${zone.substr(-1)}`;
switch (subnet.subnetType) {
case SubnetType.Public:
const publicSubnet = new VpcPublicSubnet(this, name, {
mapPublicIpOnLaunch: subnet.mapPublicIpOnLaunch || true,
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 really bad at type script - but does || true mean always true or does it mean "true unless the other value is set"?

Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated : I'm (mildy) curious why VpcPublicSubnet would take mapPublicIpOnLaunch - when is that not the case?
(That's probably my fault, I'm just thinking aloud)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My only legit use case is a bastion host. The public IPs that get put onto these images cannot be found in any CloudTrail. In some sensitive environments the non-traceable IP that is mapped causes audit trail complexities. Beyond that use case it is very odd to not want a public IP on a host in public subnet, but I suspect there are some obscure use cases beyond mine.

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 really bad at type script - but does || true mean always true or does it mean "true unless the other value is set"?

You're very right, this is a bug! It indeed means always true!

The correct way to write it would have been:

subnet.mapPublicIpOnLaunch !== undefined ? subnet.mapPublicIpOnLaunch : true

vpcId: this.vpcId,
availabilityZone: zone,
cidrBlock: cidr
});
if (subnet.natGateway) {
natGatewayByAZ[zone] = publicSubnet.addNatGateway();
}
this.publicSubnets.push(publicSubnet);
break;
case SubnetType.Private:
const privateSubnet = new VpcPrivateSubnet(this, name, {
mapPublicIpOnLaunch: subnet.mapPublicIpOnLaunch || false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting - what's the case for a subnet having this set to true? Is there ever a case where we'd do this?

(I could totally be wrong - it's been a day and my brain is 💀 )

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 one I have no use case for and I've never seen it done. However, I'm not a networking expert.

vpcId: this.vpcId,
availabilityZone: zone,
cidrBlock: cidr
});
this.privateSubnets.push(privateSubnet);
break;
case SubnetType.Internal:
const internalSubnet = new VpcPrivateSubnet(this, name, {
Copy link
Contributor

Choose a reason for hiding this comment

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

new VpcActuallyPrivateSubnet 🤣

mapPublicIpOnLaunch: subnet.mapPublicIpOnLaunch || false,
Copy link
Contributor

Choose a reason for hiding this comment

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

In reading this - I wonder do we need the mapPublicIpOnLaunch field - if based on the subnet type we're making these defaults.

Why would we do a InternalSubnet over a PrivateSubnet with that set to true.

Just seems like we could drop the field maybe?

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'm guessing behind the scenes there may be some code that was just easier if the data structures were the same. However, I'll leave this up to the team to decide (not sure if that means @eladb or a polly vote in slack -- I'm pretty new here).

Copy link
Contributor

Choose a reason for hiding this comment

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

Definitely it's easier if the internal data structures are the same, but we could and should still catch nonsensical configuration at the edges.

So the only use case we've seen so far is setting this value to false in a PUBLIC subnet, no? Which might make sense if people both:

  • Want a bastion host; and
  • Are tight on address space (because otherwise you would launch the private hosts in an INTERNAL or PRIVATE subnet).

But if you're tight on address space you could also just make the subnets smaller... so doesn't it make more sense to always leave this to the behavior that's natural for the subnet type?

vpcId: this.vpcId,
availabilityZone: zone,
cidrBlock: cidr
});
this.internalSubnets.push(internalSubnet);
break;
}
}
}
(this.privateSubnets as VpcPrivateSubnet[]).forEach((privateSubnet, i) => {
let ngwId = natGatewayByAZ[privateSubnet.availabilityZone];
if (ngwId === undefined) {
const ngwArray = Array.from(Object.values(natGatewayByAZ));
ngwId = ngwArray[i % ngwArray.length];
Copy link
Contributor

Choose a reason for hiding this comment

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

A helpful comment line would be AMAZING 😺

}
privateSubnet.addDefaultNatRouteEntry(ngwId);
});
}

}

/**
* The type of Subnet
*/
export enum SubnetType {
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the concept of defining the network access (and whether NAT gateways etc are required) at a subnet level rather than a VPC level. I do feel that as it is, it's slightly confusing to be providing this at a VpcNetwork level (with OutboundTrafficMode) and at a subnet level (with SubnetType).

I realise you've done it this way to minimise breaking changes on the VpcNetwork construct, however it would be really nice if we:

  1. If no subnet configuration is provided, use a default one that specifies public and private in each AZ, with outbound communication from private via NAT Gateways
  2. Allow users to override by providing a subnetConfiguration
  3. Auto calculate the outbound network mode and requirements (NAT Gateway etc) based on all of the subnets
  4. Remove the concept of OutboundTrafficMode

I think this would be a more coherent API for customers. @rix0rrr what do you think about this (would be a breaking change). From what I can see no other part of the CDK uses this OutboundTrafficMode API today.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would simplify the construct implementation too - You could just define a const DefaultSubnetConfiguration = ..., and use that if none is provided. Especially if we make cidrMask an optional that automatically calculates if not provided (as it wouldn't be in the DefaultSubnetConfiguration).

Copy link
Contributor

Choose a reason for hiding this comment

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

Breaking changes are fine at this stage of the project. We want the best APIs for 1.0.0. Just make sure to document in the commit/PR message under "BREAKING CHANGE" (see CONTRIBUTING guide).

Copy link
Contributor Author

@moofish32 moofish32 Jul 10, 2018

Choose a reason for hiding this comment

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

I like the proposal for the breaking change.

I honestly struggled with this as well. I also flip flopped on the implementation between which one should win; right now it's actually not well enforced and I agree confusing. I'll wait for more consensus before making a change.

Didn't see @eladb comment. I'll work up the change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Something else I would like to bring up:

Does the terminology still make sense?

We now have PUBLIC (clear enough), and both PRIVATE and INTERNAL.

I'm fine if these terms are commonly used and well-understood, but the distinction between private and internal isn't obvious to me from the term.

I guess private is standardized and well-understood, so that means we should find a more descriptive term for internal.

  • very_private?
  • unroutable
  • air_gapped (not technically correct but evocative)
  • something else?

Copy link
Contributor

Choose a reason for hiding this comment

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

isolated

Copy link
Contributor

Choose a reason for hiding this comment

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

YES! private and isolated sound good


/**
* Internal Subnets do not route Outbound traffic
*
* This can be good for subnets with RDS or
* Elasticache endpoints
*/
Internal = 1,

/**
* Public subnets route outbound traffic via an Internet Gateway
*
* If this is set and OutboundTrafficMode.None is configure an error
* will be thrown.
*/
Public = 2,

/**
* Private subnets route outbound traffic via a NAT Gateway
*
* Outbound traffic will be routed via a NAT Gateways preference being in
Copy link
Contributor

Choose a reason for hiding this comment

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

Split doc text into caption line/body.

* the same AZ, but if not available will use another AZ. This is common for
Copy link
Contributor

Choose a reason for hiding this comment

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

The remark about "cost conscious", is that about private subnets in general or just the case where there might be fewer NAT gateways than subnets?

Is that also the purpose of numAZs, to have fewer public subnets than private subnets?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. We create 90 day use accounts for experimentation. In these accounts we commonly use only one NAT GW. I need to test this feature, but you are inferring the right intent. Should I change wording to be more clear?

The purpose of numAZs is the rare case when you don't want the subnet in all AZs. This is where I get a little skeptical but the use case is commonly data replication over a VPN where the IP space is constrained. I don't see this very often, but I thought we could easily support. If we think this is going to add too much complexity you can probably talk me out of this.

* experimental cost conscious accounts or accounts where HA outbound
* traffic is not needed.
*/
Private = 3
}

/**
* Specify configuration parameters for a VPC to be built
*/
export interface SubnetConfiguration {
// the cidr mask value from 16-28
cidrMask: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we make this optional? For example, by splitting unclaimed address space evenly over the subnets?

I'm thinking of the following calculation:

space_per_az = total_space / total_azs
unclaimed_space = space_per_az - sum(subnet.cidrMask for all nets with cidrMask configured)
defaultCidrMask = (space_per_az - unclaimed_space) / count(nets without cidrMask)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me give this a try. I can't think of a reason it will not work, but hard to play out all the situations.

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 is now included

// Public (IGW), Private (Nat GW), Internal (no outbound)
subnetType: SubnetType;
// name that will be used to generate an AZ specific name e.g. name-2a
name: string;
// if true will place a NAT Gateway in this subnet, subnetType must be Public
natGateway?: boolean;
// defaults to true in Subnet.Public, false in Subnet.Private or Subnet.Internal
mapPublicIpOnLaunch?: boolean;
// number of AZs to build this subnet in
numAZs?: number;
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 very curious about the interaction between maxAZs and numAZs, and what the use case is here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see comment below regarding constrained IP space environments. I might be missing some checks to ensure numAZs <= maxAZs

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 now make this check and error if applicable

}

/**
* Specify configuration parameters for a VPC subnet
*/
Expand Down
Loading