(BREAKING) Enhancement - Custom VPC Subnets #250

moofish32 · 2018-07-09T09:08:09Z

I have created an initial approach/concept to enable a more customizable
subnet design for VPCs. I have tried to make minimal changes (leaving in
tact the current default system).

By submitting this pull request, I confirm that my contribution is made under
the terms of the beta license.

initial bare bones working vpc

eladb · 2018-07-09T09:21:09Z

@PaulMaddox can you please take a look at this?

eladb

This totally looks like the right direction. I am not an expert in VPC networking (hence requested @PaulMaddox to take a look), but in general I like the concept and the API seems clean.

eladb · 2018-07-09T09:13:46Z

packages/@aws-cdk/ec2/lib/network-util.ts

+ * InvalidSubnetCidrError is thrown when attempting to split a CIDR range into more
+ * subnets than it has IP space for.
+ */
+export class InvalidSubnetCidrError extends Error {


We have an issue with our cross-language binding tech (jsii) that doesn't allow you to subclass Error. I would just use the message content for test assertions:

assert.throws(() => doSomething(), /matchError/);

@eladb should I still create custom Error classes and then only test for the matching text? Or are you saying don't even create subclasses of Error?

Just use Error. I know, it's not pretty.

eladb · 2018-07-09T09:14:29Z

packages/@aws-cdk/ec2/lib/network-util.ts

+    // https://stackoverflow.com/questions/31626231/custom-error-class-in-typescript
+    Object.setPrototypeOf(this, InvalidSubnetCountError.prototype);
+  }
+}


Fix indent to 4 spaces

eladb · 2018-07-09T09:20:29Z

packages/@aws-cdk/ec2/lib/network-util.ts

+  public static validIp(ipAddress: string): boolean {
+    return ipAddress.split('.').map(
+      (octet: string) => (parseInt(octet, 10) >= 0 && parseInt(octet, 10) <= 255)).
+      reduce((valid: boolean, value: boolean) => (valid && value));


Nit: I think "find" might be more readable than map/reduce, and you should also verify the address includes exactly 4 octets:

return ipAddress.split('.').map(parseInt).find(o >= 0 && o <= 255).length === 4

Alternatively, find a well-tested regex...

I'll move to your find solution. The regex googling had mixed reviews.

I moved to filter because find seemed to return only the first match. Still better than reduce since it's not really standard sum type of behavior?

eladb · 2018-07-09T09:22:21Z

packages/@aws-cdk/ec2/lib/network-util.ts

+  }
+
+}
+


Eventually make sure to add jsdocs to all exported entities

eladb · 2018-07-09T09:22:51Z

packages/@aws-cdk/ec2/lib/vpc.ts

+    /**
+     * Define subnets that will be built by default per AZ (maxAZs). If left
+     * empty the default VPC configuration will be used
+     *


Use "@default" to describe the default behavior

Also, what is the default VPC configuration?

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

eladb · 2018-07-09T09:23:22Z

packages/@aws-cdk/ec2/lib/vpc.ts

+     * Define subnets that will be built by default per AZ (maxAZs). If left
+     * empty the default VPC configuration will be used
+     *
+     * The subnets are contructed in the context of the VPC so you only need


contructed => constructed

eladb · 2018-07-09T09:23:55Z

packages/@aws-cdk/ec2/lib/vpc.ts

+     * across 3 AZs then maxAZs = 3 and provide the following:
+     * subnets: [
+     * {
+     *   cidrMask: 24;


"," instead of ";" between hash elements 😄

eladb · 2018-07-09T09:25:12Z

packages/@aws-cdk/ec2/lib/vpc.ts

+          const networkBuilder = new NetworkBuilder(cidr);
+          this.createCustomSubnets(networkBuilder, subnets, zones);
+        } else {
+          // Split the CIDR range into each availablity zone


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

I was just going for minimal change, but I can easily do this.

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.

eladb · 2018-07-09T09:27:39Z

packages/@aws-cdk/ec2/lib/network-util.ts

+    return subnets.map((subnet) => (subnet.cidr));
+  }
+
+  public getSubnets(): string[] {


Use getter: public get subnets(): string[]

eladb · 2018-07-09T09:29:41Z

packages/@aws-cdk/ec2/lib/vpc.ts

+/**
+ * Specify configuration parameters for a VPC to be built
+ */
+export interface VpcSubnetBuilderProps {


Use jsdocs (/** */) for all properties and explicitly call out @defaults

…ability

rix0rrr · 2018-07-10T10:12:18Z

packages/@aws-cdk/ec2/lib/vpc.ts

+          const networkBuilder = new NetworkBuilder(cidr);
+          this.createCustomSubnets(networkBuilder, subnets, zones);
+        } else {
+          // Split the CIDR range into each availablity zone


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.

rix0rrr · 2018-07-10T10:14:18Z

packages/@aws-cdk/ec2/lib/vpc.ts

@@ -58,6 +59,34 @@ export interface VpcNetworkProps {
     * @default All AZs in the region
     */
    maxAZs?: number;
+
+    /**
+     * Define subnets that will be built per AZ (maxAZs), if numAZs is


Simplify the wording a bit here. Something like:

Configure the subnets to build for each AZ

Should be sufficient in the first line. More elaboration can be done in the body of the docstring.

I think the maxAZs setting doesn't need to be invoked, that seems logical enough.

rix0rrr · 2018-07-10T10:16:11Z

packages/@aws-cdk/ec2/lib/vpc.ts

+     * @default the VPC CIDR will be evenly divided between 1 public and 1
+     * private subnet per AZ
+     */
+    subnets?: VpcSubnetBuilderProps[];


Does this name make sense? subnets to me feels like the constructed resources, which this isn't about. This is specifying construction/configuration parameters per AZs.

How about subnetConfiguration? Please change the name of the type to match.

subnetConfiguration is much better.

rix0rrr · 2018-07-10T10:17:29Z

packages/@aws-cdk/ec2/lib/vpc.ts

+     */
+    public readonly internalSubnets: VpcSubnetRef[] = [];
+
+    public readonly natGatewayByAZ: Obj<Token> = {};


Should this be public? We don't like exposing Token, so either define an appropriate subclass for Token or make this private.

In fact, it seems to be used inside a single method only. Why not make it a local variable there?

Agree will move to local

rix0rrr · 2018-07-10T10:19:29Z

packages/@aws-cdk/ec2/lib/vpc.ts

+export enum SubnetType {
+
+    /**
+     * Outbound traffic is not routed. This can be good for subnets with RDS or


Split doc text into caption line/body.

rix0rrr · 2018-07-10T10:19:55Z

packages/@aws-cdk/ec2/lib/vpc.ts

+    Public = 2,
+
+    /**
+     * Outbound traffic will be routed via a NAT Gateways preference being in


Split doc text into caption line/body.

rix0rrr · 2018-07-10T10:22:04Z

packages/@aws-cdk/ec2/lib/vpc.ts

+    // defaults to false if true will map public IPs on launch
+    mapPublicIpOnLaunch?: boolean;
+    // number of AZs to build this subnet in
+    numAZs?: number;


I'm very curious about the interaction between maxAZs and numAZs, and what the use case is here.

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

I now make this check and error if applicable

rix0rrr · 2018-07-10T10:25:28Z

packages/@aws-cdk/ec2/lib/vpc.ts

+ */
+export interface VpcSubnetBuilderProps {
+    // the cidr mask value from 16-28
+    cidrMask: number;


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)

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.

this is now included

rix0rrr · 2018-07-10T10:30:17Z

packages/@aws-cdk/ec2/lib/vpc.ts

+    name: string;
+    // if true will place a NAT Gateway in this subnet, subnetType must be Public
+    natGateway?: boolean;
+    // defaults to false if true will map public IPs on launch


Is that true? I believe it depends on the subnet type, certainly for public subnets it defaults to true.

Bad docs! In public I defaulted to true in private I defaulted to false. https://github.com/awslabs/aws-cdk/pull/250/files#diff-ee81a78878f10abd0d632c84312bcd8dR324

I'll update the docs to match. If you think I should change that behavior let me know.

rix0rrr · 2018-07-10T10:31:51Z

packages/@aws-cdk/ec2/lib/vpc.ts

+
+    /**
+     * Outbound traffic will be routed via a NAT Gateways preference being in
+     * the same AZ, but if not available will use another AZ. This is common for


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?

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.

PaulMaddox · 2018-07-10T11:29:52Z

I will have some time today/tomorrow to look through this

rix0rrr · 2018-07-10T11:30:50Z

Also, thanks for contributing! 🎉

PaulMaddox · 2018-07-10T15:02:38Z

packages/@aws-cdk/ec2/lib/network-util.ts

+
+}
+
+export class NetworkBuilder {


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

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

Awesome. Thanks for contributing too!

…; removed public NatGatewayByAz to local variable

PaulMaddox · 2018-07-10T16:49:22Z

packages/@aws-cdk/ec2/test/test.vpc.ts

+              outboundTraffic: OutboundTrafficMode.FromPublicAndPrivateSubnets,
+              subnets: [
+                {
+                  cidrMask: 24,


Is specifying the cidrMask mandatory here? I think it is looking at the code. Do you think it would be worth making it an optional, and automatically calculate it if not provided (we'd have to document this behaviour).

I could see a scenario where I wanted to provide a custom subnet configuration, but not care too much about the subnet sizes. Having it default to automatically calculating would save customers having to do the CIDR math themselves.

This now works with even splits for subnets.

PaulMaddox · 2018-07-10T16:56:02Z

packages/@aws-cdk/ec2/lib/vpc.ts

+/**
+ * The type of Subnet
+ */
+export enum SubnetType {


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:

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

Allow users to override by providing a subnetConfiguration

Auto calculate the outbound network mode and requirements (NAT Gateway etc) based on all of the subnets

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.

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).

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).

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.

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?

YES! private and isolated sound good

mindstorms6

First off you're my hero.

This change makes a ton of sense - and I'm thrilled about it.

Most of my comments are very minor - so don't feel like they're required - most are just small things :)

🎉

mindstorms6 · 2018-07-11T01:02:36Z

packages/@aws-cdk/ec2/lib/network-util.ts

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


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)

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.

mindstorms6 · 2018-07-11T01:15:01Z

packages/@aws-cdk/ec2/lib/network-util.ts

+        return subnets.map((subnet) => (subnet.cidr));
+    }
+
+    public get subnets(): string[] {


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)

mindstorms6 · 2018-07-11T01:18:05Z

packages/@aws-cdk/ec2/lib/network-util.ts

+
+export class CidrBlock {
+
+    public static calculateNetmask(mask: number): string {


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

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.

mindstorms6 · 2018-07-11T01:21:26Z

packages/@aws-cdk/ec2/lib/network-util.ts

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


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

mindstorms6 · 2018-07-11T01:21:39Z

packages/@aws-cdk/ec2/lib/network-util.ts

+        const ipOct = this.cidr.split('/')[0].split('.');
+        // tslint:disable:no-bitwise
+        return netmaskOct.map(
+            (maskOct, index) => parseInt(maskOct, 10) & parseInt(ipOct[index], 10)).join('.');


mindstorms6 · 2018-07-11T01:26:51Z

packages/@aws-cdk/ec2/lib/vpc.ts

+          switch (subnet.subnetType) {
+            case SubnetType.Public:
+              const publicSubnet = new VpcPublicSubnet(this, name, {
+                  mapPublicIpOnLaunch: subnet.mapPublicIpOnLaunch || true,


I'm really bad at type script - but does || true mean always true or does it mean "true unless the other value is set"?

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)

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.

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

mindstorms6 · 2018-07-11T01:32:05Z

packages/@aws-cdk/ec2/lib/vpc.ts

+              break;
+            case SubnetType.Private:
+              const privateSubnet = new VpcPrivateSubnet(this, name, {
+                  mapPublicIpOnLaunch: subnet.mapPublicIpOnLaunch || false,


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 💀 )

This one I have no use case for and I've never seen it done. However, I'm not a networking expert.

mindstorms6 · 2018-07-11T01:33:58Z

packages/@aws-cdk/ec2/lib/vpc.ts

+              this.privateSubnets.push(privateSubnet);
+              break;
+            case SubnetType.Internal:
+              const internalSubnet = new VpcPrivateSubnet(this, name, {


new VpcActuallyPrivateSubnet 🤣

mindstorms6 · 2018-07-11T01:36:24Z

packages/@aws-cdk/ec2/lib/vpc.ts

+              break;
+            case SubnetType.Internal:
+              const internalSubnet = new VpcPrivateSubnet(this, name, {
+                  mapPublicIpOnLaunch: subnet.mapPublicIpOnLaunch || false,


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?

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).

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?

mindstorms6 · 2018-07-11T01:37:18Z

packages/@aws-cdk/ec2/lib/vpc.ts

+        let ngwId = natGatewayByAZ[privateSubnet.availabilityZone];
+        if (ngwId === undefined) {
+          const ngwArray = Array.from(Object.values(natGatewayByAZ));
+          ngwId = ngwArray[i % ngwArray.length];


A helpful comment line would be AMAZING 😺

mindstorms6 · 2018-07-11T01:39:31Z

Let me know if you need help with any of the requested changes!

💯

moofish32

@rix0rrr -- I am now failing the integ tests as you thought. Since this has moved to full on breaking change, I assume I can modify those tests or is there something special in there? I didn't quite get to looking at them yet.

@eladb - I have a conflict in here and you will be happy to know I can resolve this! Do we have any standards about using merge vs rebase for resolution? I typically rebase but because this PR is long and full of comments I don't want to rebase until the end. If we merge then I'm really hoping I get to squash at the end and clean up my commits -- or I can just create a new branch and PR with a link. End goal is I don't want all my garbage fix commits showing up and I want to keep whatever git practice you guys are using.

moofish32 · 2018-07-11T07:53:36Z

packages/@aws-cdk/ec2/lib/network-util.ts

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


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.

moofish32 · 2018-07-11T07:58:43Z

packages/@aws-cdk/ec2/lib/network-util.ts

+
+export class CidrBlock {
+
+    public static calculateNetmask(mask: number): string {


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.

moofish32 · 2018-07-11T08:11:39Z

packages/@aws-cdk/ec2/lib/vpc.ts

+          switch (subnet.subnetType) {
+            case SubnetType.Public:
+              const publicSubnet = new VpcPublicSubnet(this, name, {
+                  mapPublicIpOnLaunch: subnet.mapPublicIpOnLaunch || true,


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.

moofish32 · 2018-07-11T08:13:02Z

packages/@aws-cdk/ec2/lib/vpc.ts

+              break;
+            case SubnetType.Private:
+              const privateSubnet = new VpcPrivateSubnet(this, name, {
+                  mapPublicIpOnLaunch: subnet.mapPublicIpOnLaunch || false,


This one I have no use case for and I've never seen it done. However, I'm not a networking expert.

moofish32 · 2018-07-11T08:15:46Z

packages/@aws-cdk/ec2/lib/vpc.ts

+              break;
+            case SubnetType.Internal:
+              const internalSubnet = new VpcPrivateSubnet(this, name, {
+                  mapPublicIpOnLaunch: subnet.mapPublicIpOnLaunch || false,


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).

moofish32 · 2018-07-11T08:17:28Z

packages/@aws-cdk/ec2/lib/vpc.ts

+ */
+export interface VpcSubnetBuilderProps {
+    // the cidr mask value from 16-28
+    cidrMask: number;


this is now included

moofish32 · 2018-07-11T08:17:44Z

packages/@aws-cdk/ec2/lib/vpc.ts

+    // defaults to false if true will map public IPs on launch
+    mapPublicIpOnLaunch?: boolean;
+    // number of AZs to build this subnet in
+    numAZs?: number;


I now make this check and error if applicable

moofish32 · 2018-07-11T08:18:30Z

packages/@aws-cdk/ec2/test/test.vpc.ts

+              outboundTraffic: OutboundTrafficMode.FromPublicAndPrivateSubnets,
+              subnets: [
+                {
+                  cidrMask: 24,


This now works with even splits for subnets.

rix0rrr · 2018-07-11T09:47:19Z

Re: failing integ tests.

Depends on what the break is. If it's source API, totally fine, but I would hope that the same Constructs get generated based on the same conceptual configuration as before.

Because if we change those, people downstream consuming the change will have their VPC torn down and recreated, which is probably not going to go down well.

rix0rrr · 2018-07-11T09:57:41Z

Mike (@moofish32), I would like to pull your changes to look at them in my source tree. Could you give me read access to your clone?

Need to look at it again from the top

PaulMaddox · 2018-07-11T10:53:33Z

@rix0rrr, you can checkout the PR without having access to @moofish32's private repo by adding the following to your ~/.git/config:

[remote "origin"]
        url = git@github.com:awslabs/aws-cdk.git
        fetch = +refs/pull/*/head:refs/remotes/origin/pr/*

then do:

$ git fetch origin pull/250/head:f-improve-vpc-network
$ git checkout f-improve-vpc-network

rix0rrr · 2018-07-11T13:59:09Z

@moofish32, after fixing some trivialities (API changes and linter errors), there are two problems:

The names of the generated subnet resources is different, which causes all resources to be recreated. I've changed the algo to generate the same identifiers as before.
The CIDR allocation is different than before.

[~] ⚠️ Replacing VPCPrivateSubnet1Subnet8BCA10E0 (type: AWS::EC2::Subnet)
 └─ [~] .CidrBlock (requires replacement):
     ├─ [-] Old value: 10.0.32.0/19
     └─ [+] New value: 10.0.96.0/19

rix0rrr · 2018-07-22T09:19:51Z

packages/@aws-cdk/ec2/lib/vpc.ts

+    /**
+     * The CIDR Mask or the number of leading 1 bits in the routing mask
+     *
+     * Valid values are 16 - 28


@default Available space is automatically divided over maskless subnets

rix0rrr · 2018-07-22T09:29:11Z

Sooo ... why would you not want mapPublicIP set to true? The only situation I have encountered so far is that these public IPs are not in any Cloudtrail log.

So if you have a 3rd party partner investigating a security incident and they ask you to verify if a public IP was associated to your account during a time period. It's very difficult to do this without an EIP.

Wait, I get confused. In the first sentence, I thought we were talking about mapPublicIP = false on a public subnet. In the second sentence, it sounds like you're talking about mapPublicIP = true on a non-public subnet (or similar).

But in any case, if they did NOT want a public IP in a public subnet, wouldn't the solution then be to launch the instances in a private subnet? I still have a hard time seeing the use case.

rix0rrr · 2018-07-22T09:32:14Z

I created a whole flow where it would be possible to declare more than maxAZs NAT GWs after talking with an SA who explained that a video company did actually saturate a NAT GW. However, I tore that out because it started to become just as verbose as CFN.

Oh I see. Yes, that sounds fair. It seems like a feature request that we can add later on. Or here's another idea: instead of saying "MAX number of nat gateways", rename to natGateways?: number, default to maxAZs, and enforce that the number is not HIGHER than the number of maxAZs.

Then we have a forwards compatible API that makes sense now (lower the amount of gateways) and still makes sense if we want to raise the amount of gateways. We can punt on the actual implementation and do that at a later time.

rix0rrr · 2018-07-22T09:35:09Z

However, we can easily default this to true if private subnets exist. Is that your preference?

I would love that. In fact, does that not get rid of the need to have the parameter at all?

If you have a private subnet, you MUST want NAT routing. Otherwise, you would have used an internal subnet, no?

rix0rrr · 2018-07-22T09:36:01Z

Speaking of Internal subnets... I recall there being a vote to rename the concept of Internal to Isolated (since the distinction between Private and Internal is not immediately obvious, at least not to me).

How do you feel about that?

rix0rrr · 2018-07-22T09:41:57Z

I'm ready to approve this, modulo some small issues:

subnet.mapPublicIP: I still don't completely understand the use case, and why an instance launched in a private subnet wouldn't solve the same issue?
maxNatGateways: rename and restrict range for future extensibility.
subnet.natGateway: feels like it should just be calculated based upon the presence of private subnets?
internal subnet type: I've been talking about this so much that it no longer seems weird to me, but I remember upon first seeing it that the distinction between private and internal was lost on me, and I think we could do with a better moniker. Elad brought up isolated, which I kind of like. Might depend on a bit on how much of this is entrenched terminology as well.
CIDR math: feels to me like the code could be simplified there, but I'm happy to take that myself.

…licIp, and internal -> isolated

…ement

… nextAvailableIp, adding in nextBlock() method

moofish32

@rix0rrr -- I think I have addressed all your comments and the new ones that came after I started my review here. There were a couple points of confusion but hopefully we can clear those up.

moofish32 · 2018-07-20T15:29:56Z

packages/@aws-cdk/ec2/lib/network-util.ts

@@ -86,4 +86,237 @@ export class NetworkUtils {

    }

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


done - added tests to verify

moofish32 · 2018-07-20T15:33:04Z

packages/@aws-cdk/ec2/lib/network-util.ts

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


moofish32 · 2018-07-20T15:33:13Z

packages/@aws-cdk/ec2/lib/network-util.ts

+    }
+
+    /**
+     *


moofish32 · 2018-07-20T15:36:59Z

packages/@aws-cdk/ec2/lib/network-util.ts

+export class NetworkBuilder {
+
+    /**
+     * the CIDR range used when creating the network


moofish32 · 2018-07-20T15:37:36Z

packages/@aws-cdk/ec2/lib/network-util.ts

+    /**
+     * the list of CIDR blocks for subnets within this network
+     */
+    protected subnetCidrs: CidrBlock[];


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

moofish32 · 2018-07-20T20:11:05Z

packages/@aws-cdk/ec2/lib/network-util.ts

+            NetworkUtils.ipToNum(this.maxIpConsumed);
+        const ipsPerSubnet: number = Math.floor(remaining / subnetCount);
+        return 32 - Math.floor(Math.log2(ipsPerSubnet));
+        // return this.addSubnets(mask, subnetCount);


moofish32 · 2018-07-23T07:52:45Z

packages/@aws-cdk/ec2/lib/network-util.ts

+    /**
+     * return the CIDR notation strings for all subnets in the network
+     */
+    public get subnets(): string[] {


moofish32 · 2018-07-23T07:58:10Z

packages/@aws-cdk/ec2/lib/network-util.ts

+}
+
+/**
+ * Creates a CIDR Block


done. I did not implement split() I'm not positive what you were suggesting here.

moofish32 · 2018-07-23T08:17:43Z

packages/@aws-cdk/ec2/lib/network-util.ts

+        const subnets: CidrBlock[] = [];
+        count = count || 1;
+        for (let i = 0; i < count; i ++) {
+            const subnet: CidrBlock = CidrBlock.fromOffsetIp(this.maxIpConsumed, mask);


done, went with nextBlock()

moofish32 · 2018-07-23T08:19:33Z

packages/@aws-cdk/ec2/lib/network-util.ts

+     * CidrBlock.fromOffsetIp('10.0.0.15', 24) returns '10.0.1.0/24'
+     */
+    public static fromOffsetIp(ipAddress: string, mask: number): CidrBlock {
+        const initialCidr = new CidrBlock(`${ipAddress}/${mask}`);


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?

moofish32 · 2018-07-23T08:29:47Z

subnet.mapPublicIP: I still don't completely understand the use case, and why an instance launched in a private subnet wouldn't solve the same issue?

I've removed this. There is still a corner case where a team may want to prohibit public IPs on instances that are not EIPs. However, it is likely obscure and could be addressed if CloudTrail simply logged these IPs. Let's see if we get immediate feature requests instead of complicating the code, as you are you suggesting.

maxNatGateways: rename and restrict range for future extensibility.

This is implemented

subnet.natGateway: feels like it should just be calculated based upon the presence of private subnets?

This is implemented

internal subnet type: I've been talking about this so much that it no longer seems weird to me, but I remember upon first seeing it that the distinction between private and internal was lost on me, and I think we could do with a better moniker. Elad brought up isolated, which I kind of like. Might depend on a bit on how much of this is entrenched terminology as well.

I've implemented Isolated the term Internal was not standard just my best guess. We may want to engage EC2 to see if they have an opinion. The terms Public and Private were around before NATs.

CIDR math: feels to me like the code could be simplified there, but I'm happy to take that myself.

I took a shot at cleaning this up. Somethings did get much simpler, the constructor was not one.

…or consistent

eladb

This looks great! @rix0rrr any other comments?

rix0rrr · 2018-07-23T17:08:48Z

Nope--the API is in great shape. I think we're ready to merge this. I'll approve it. Don't know if Mike will have permissions to merge (if so, squash merge and a critical look at the commit message please, be sure to note that this is a breaking change). If not, either Elad can merge tomorrow or I will do it.

rix0rrr

Great work Mike, thanks for the effort!

moofish32 · 2018-07-23T17:51:44Z

I don't see the ability to squash and merge. @eladb if you want I can take a pass at the commit message since I doubt anybody else is going to make sense of the ones in there?

moofish32 · 2018-07-23T18:04:45Z

@eladb @rix0rrr -- my quick summary/commit message proposal

Adding the ability to customize VpcNetwork configurations:

improved IP space allocation to be more efficient (breaking change to existing VPC stacks; updated existing Integration tests)
customize subnet IP space with SubnetConfiguration arrays
defined 3 distinct subnet types (Public, Private, and Isolated)
added ability to control the number of NAT Gateways and automated creation based on the existence of Private subnets
standardized MapPublicIpOnLaunch for enabled in Public and disabled in other subnet types
enable the ability to evenly split remaining IP space across a set of subnet configurations (e.g. allocate public space and then split remaining network space across 1 to N subnets)
support existing default when no configuration data is provided
added utility classes for working with CIDR blocks (CidrBlock) and for defining network configuration (NetworkBuilder)
Added basic README documentation for new features

moofish32 · 2018-07-24T16:48:19Z

@eladb -- I'll merge in master and resolve these conflicts.

eladb · 2018-07-24T16:55:05Z

Thanks, looking forward. I will pull then.

eladb · 2018-07-24T20:20:26Z

@moofish32 ready for merge?

moofish32 · 2018-07-24T20:25:31Z

Yes. Unless anybody sees another comment/issue 👍

initial network builder design

28aea13

initial bare bones working vpc

eladb reviewed Jul 9, 2018

View reviewed changes

Mike Cowgill and others added 3 commits July 9, 2018 22:25

fixing tabs == 4 spaces in network-util.ts

2beb4a7

moving to get methods for subnets, adding additional comment detail

2a172ed

moving network-util vaildIp func to filter instead of reduce for read…

9da7a2f

…ability

eladb requested review from mindstorms6, RomainMuller, rix0rrr and PaulMaddox July 10, 2018 09:15

rix0rrr previously requested changes Jul 10, 2018

View reviewed changes

PaulMaddox reviewed Jul 10, 2018

View reviewed changes

renaming of VpcSubnetBuilderProps to SubnetConfiguration; doc updates…

55e3666

…; removed public NatGatewayByAz to local variable

PaulMaddox suggested changes Jul 10, 2018

View reviewed changes

moofish32 added 3 commits July 10, 2018 13:25

removing custom error classes and asserting on regex message

a2b527d

removing more bad uses of reduce

c9d4f39

network-utils doc improvements

af75c41

mindstorms6 reviewed Jul 11, 2018

View reviewed changes

moofish32 added 2 commits July 10, 2018 23:36

refactor complete snapping a line to debug

88a6d94

adding error for too many AZs requested

9ac38f4

moofish32 commented Jul 11, 2018

View reviewed changes

additional comments for CIDR math and odd behaviors

60ef32d

addressing code comments and tests for natGateway default behavior

5c1bc5d

rix0rrr reviewed Jul 22, 2018

View reviewed changes

moofish32 added 3 commits July 22, 2018 20:44

addressing comments for maxNatGateway -> natGateways, removing mapPub…

0864bb8

…licIp, and internal -> isolated

refactoring cidrblock and network builder for number centric ip manag…

776f1e0

…ement

refactoring CidrBlock for constructor overloading, renaming nextIp ot…

82246ae

… nextAvailableIp, adding in nextBlock() method

moofish32 commented Jul 23, 2018

View reviewed changes

a couple of comment clarifications and making the CidrBlock construct…

864a341

…or consistent

eladb reviewed Jul 23, 2018

View reviewed changes

moofish32 added 2 commits July 23, 2018 10:03

missed one README typo

9722cdc

final README (ec2) correction to create Advanced Subnet Configuration

bba5e67

rix0rrr approved these changes Jul 23, 2018

View reviewed changes

moofish32 added 3 commits July 24, 2018 11:13

Merge branch 'master' into f-improve-vpc-network

c2baa77

minor tweaks for renames from master merge

31057b2

README modification to match import ec2 = require('@aws-cdk/aws-ec2')

f3f5428

eladb merged commit fbe6711 into aws:master Jul 24, 2018

moofish32 deleted the f-improve-vpc-network branch July 24, 2018 20:30

njlynch mentioned this pull request Jul 23, 2021

Ec2.VPC: incorrect subnet calculation #15682

Closed


		export class CidrBlock {

		public static calculateNetmask(mask: number): string {

(BREAKING) Enhancement - Custom VPC Subnets #250

(BREAKING) Enhancement - Custom VPC Subnets #250

Conversation

moofish32 commented Jul 9, 2018 • edited Loading

eladb commented Jul 9, 2018

eladb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PaulMaddox commented Jul 10, 2018

rix0rrr commented Jul 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moofish32 Jul 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Does the terminology still make sense?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mindstorms6 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mindstorms6 commented Jul 11, 2018

moofish32 commented Jul 9, 2018 •

edited

Loading

moofish32 Jul 10, 2018 •

edited

Loading