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

moofish32 · 2018-10-09T08:01:54Z

BREAKING change to VPC configuration
closes NatGateway Config for Public Subnet Control to Allow NACL Controls #741
Enable NAT placement by subnet name
Change VpcNeworkProps.natGateways from number to
{ gatewayCount: number; subnetName: string }

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

* BREAKING change to VPC configuration only if natGateways was previously set * closes aws#741 * Enable NAT placement by subnet name * Change VpcNeworkProps.natGateways from number to { gatewayCount: number; subnetName: string }

rix0rrr · 2018-10-09T08:16:19Z

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

+ * throw an error. By default the first public subnets will receive NAT
+ * Gateways until the `gatewayCount` is reached.
+ */
+ subnetName?: string;


Why not a VpcPlacementStrategy object?

rix0rrr · 2018-10-09T08:16:39Z

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

 */
- natGateways?: number;
+ natGateways?: NatGatewayConfiguration;


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

rix0rrr · 2018-10-09T08:17:58Z

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

@@ -321,12 +326,33 @@ export class VpcNetwork extends VpcNetworkRef implements cdk.ITaggable {
 internetGatewayId: igw.ref,
 vpcId: this.resource.ref
 });
+ this.dependencyElements.push(igw, att);
+
+ const natConfig = ifUndefined(props.natGateways, {


A shorter way to get the same effect would be:

const natConfig = props.natGateways || {};

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

rix0rrr · 2018-10-09T08:18:55Z

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

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


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

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

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

This comment needs one more review with the latest commit

rix0rrr · 2018-10-09T08:20:34Z

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

+ const natSubnet = natConfig.subnetName;
+
+ if (natSubnet !== undefined) {
+ const subnetNames = (this.publicSubnets as VpcPublicSubnet[]).map( subnet => {


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

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

* closes aws#741 * Enable NAT placement by `VpcPlacementStrategy` * Add `natGatewayPlacement` to `VpcNeworkProps` * Refactor NAT creation logic into one place

moofish32 · 2018-10-09T18:26:40Z

@rix0rrr -- great comments I think this is a better approach; only comment left that I think I didn't take your suggestion on is here: #874 (comment)

rix0rrr · 2018-10-10T07:58:12Z

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

+ *
+ * The names of the corresponding subnets in `SubnetConfiguration` that will
+ * have a NAT Gateway. If a corresponding subnet name is not found this will
+ * throw an error. By default the first public subnets will receive NAT


This last sentence needs to be in a @default annotation.

rix0rrr · 2018-10-10T08:00:41Z

One doc change and then good to go!

rix0rrr · 2018-10-10T08:00:50Z

You know what, I can just change that myself :)

BREAKING CHANGES ================ * The `cdk.App` initializer doesn't accept any arguments and the `app.run()` method does not return a `string` anymore. All AWS CDK apps in all languages would need to be modified to adhere to the new API of the `cdk.App` construct. Instead of: const app = new App(process.argv); // ERROR // add stacks process.stdout.write(app.run()); // ERROR The new usage is: const app = new App(); // add stacks app.run(); In order to interact with applications written using this version, the CDK Toolkit must also be update using: $ npm i -g aws-cdk * **aws-iam:** This change moves the `PolicyDocument`, `PolicyStatement` and all `PolicyPrincipal` classes from the @aws-cdk/cdk module and into the @aws-cdk/aws-iam module. * **jsx:** The CDK is no longer shipped with built-in support for JSX. You can still use JSX but you will have to manually configure it. Features ======== * **aws-apigateway:** "LambdaRestApi" and "addProxy" routes ([#867](#867)) ([a733bd1](a733bd1)) * **aws-cdk:** add maven wrapper to java template ([#811](#811)) ([1ee729e](1ee729e)) * **aws-cloudfront:** Support Security Policy ([#804](#804)) ([d69b1d6](d69b1d6)), closes [#795](#795) * **aws-codedeploy:** support setting a load balancer on a Deployment Group. ([#786](#786)) ([dc0af46](dc0af46)) * **aws-codepipeline:** allow specifying the runOrder property when creating Actions. ([#776](#776)) ([8302541](8302541)) * **aws-dynamodb:** IAM grants support ([#870](#870)) ([f6c7760](f6c7760)) * **aws-dynamodb:** support Global Secondary Indexes ([#760](#760)) ([737b481](737b481)) * **aws-dynamodb:** tags support ([#814](#814)) ([c76d8c1](c76d8c1)) * **aws-dynamodB:** support Local Secondary Indexes ([#825](#825)) ([fdb4974](fdb4974)) * Manage IAM permissions for (some) CFN CodePipeline actions ([#843](#843)) ([5f2cb9f](5f2cb9f)) * Resolve paths to nyc & nodeunit ([#887](#887)) ([6d71a87](6d71a87)) * upgrade to jsii v0.7.7 ([43d2d9e](43d2d9e)) * **aws-ec2:** allow configuring subnets for NAT gateway ([#874](#874)) ([958dce6](958dce6)) * **aws-ec2:** support UDP port ranges in SecurityGroups ([#835](#835)) ([6920b9c](6920b9c)) * **aws-s3:** support granting public access to objects ([#886](#886)) ([50e0c41](50e0c41)), closes [#877](#877) * **cdk:** Add support for UseOnlineResharding with UpdatePolicies ([#881](#881)) ([a95f081](a95f081)) Bug Fixes ========= * **aws-apigateway:** allow + in path parts ([#769](#769)) ([6905b7e](6905b7e)), closes [#768](#768) * **aws-cdk:** continue after exceptions in stack monitor ([#791](#791)) ([b7c244f](b7c244f)), closes [#787](#787) * **aws-cloudfront:** properly support loggingConfig ([#809](#809)) ([d279a1d](d279a1d)), closes [#721](#721) * **aws-ec2:** Add Burstable Generation 3 Instances ([#812](#812)) ([cf62e9d](cf62e9d)) * **aws-s3:** properly export bucketDomainName ([#844](#844)) ([9a53069](9a53069)) * Emit valid YAML-1.1 ([#876](#876)) ([6c98b73](6c98b73)), closes [#875](#875) * **aws-sqs:** Queue.import() doesn't return a value ([#885](#885)) ([c38c3e7](c38c3e7)), closes [#879](#879) * **cdk:** fix TagManager to evaluate to undefined if no tags are included ([#882](#882)) ([be65a04](be65a04)) * **cdk:** jsx support conflicts with React usage ([#884](#884)) ([2a979cc](2a979cc)), closes [#830](#830) * **docs:** update supported languages in README ([#819](#819), [#450](#450)) ([#820](#820)) ([1ec443e](1ec443e)) Code Refactoring ================ * **aws-iam:** move IAM classes cdk to aws-iam ([#866](#866)) ([6c58556](6c58556)), closes [#196](#196) * remove app boilerplate and improvements to cx protocol ([#868](#868)) ([7bb5a60](7bb5a60)), closes [#216](#216)

Bug Fixes --------- * **aws-apigateway:** allow + in path parts ([#769](#769)) ([0c50d27](0c50d27)), closes [#768](#768) * **aws-cdk:** continue after exceptions in stack monitor ([#791](#791)) ([b0f3298](b0f3298)), closes [#787](#787) * **aws-cloudfront:** check for undefined and determining of the defaultRootObject prop is set or not ([#801](#801)) ([32a74c6](32a74c6)) * **aws-cloudfront:** properly support loggingConfig ([#809](#809)) ([5512f70](5512f70)), closes [#721](#721) * **aws-codecommit:** typo in README ([#780](#780)) ([0e79c2d](0e79c2d)) * **aws-ec2:** Add Burstable Generation 3 Instances ([#812](#812)) ([d36ee6d](d36ee6d)) * **aws-ec2:** fix capitalization of "VPCEndpointType" to "VpcEndpointType" ([#789](#789)) ([7a8ee2c](7a8ee2c)), closes [#765](#765) * **aws-ec2:** fix typo in resource identifier ([#818](#818)) ([f529c80](f529c80)) * **aws-elbv2:** fix load balancer registration ([#890](#890)) ([8cc9abe](8cc9abe)) * **aws-s3:** properly export bucketDomainName ([#844](#844)) ([a65060d](a65060d)) * **aws-sqs:** Queue.import() doesn't return a value ([#885](#885)) ([c592b7f](c592b7f)), closes [#879](#879) * **cdk:** fix TagManager to evaluate to undefined if no tags are included ([#882](#882)) ([477c827](477c827)) * **cdk:** init templates were not upgraded to typescript ^3.0.0 ([#904](#904)) ([2cc7475](2cc7475)) * **cdk:** jsx support conflicts with React usage ([#884](#884)) ([76d8031](76d8031)), closes [#830](#830) * **cfn2ts:** expect Token instead of CloudFormationToken ([#896](#896)) ([6eee1d2](6eee1d2)) * **docs:** fix issue [#718](#718) (Aurora DB example) ([#783](#783)) ([016f3a8](016f3a8)) * **docs:** update supported languages in README ([#819](#819), [#450](#450)) ([#820](#820)) ([ffac98c](ffac98c)) * Correct heading level of CHANGELOG.md 0.10.0 ([40d9ef0](40d9ef0)) * Emit valid YAML-1.1 ([#876](#876)) ([ff857ea](ff857ea)), closes [#875](#875) * **toolkit:** improve error message for large templates ([#900](#900)) ([a41f48f](a41f48f)), closes [#34](#34) Code Refactoring ---------------- * **aws-iam:** move IAM classes cdk to aws-iam ([#866](#866)) ([d46a95b](d46a95b)), closes [#196](#196) * **util:** remove [@aws-cdk](https://github.com/aws-cdk)/util ([#745](#745)) ([10015cb](10015cb)), closes [#709](#709) * **framework:** remove app boilerplate and improvements to cx protocol ([#868](#868)) ([005beec](005beec)), closes [#216](#216) Features -------- * **aws-apigateway:** "LambdaRestApi" and "addProxy" routes ([#867](#867)) ([905a95d](905a95d)) * **aws-cdk:** add maven wrapper to java template ([#811](#811)) ([72aa872](72aa872)) * **aws-cloudformation:** rename the CFN CodePipeline Actions. ([#771](#771)) ([007e7b4](007e7b4)) * **aws-cloudformation:** update the ReadMe of the module to reflect the new Action names. ([#775](#775)) ([6c0e75b](6c0e75b)), closes [#771](#771) * **aws-cloudfront:** Support Security Policy ([#804](#804)) ([b39bf11](b39bf11)), closes [#795](#795) * **aws-codedeploy:** Add the auto-scaling groups property to ServerDeploymentGroup. ([#739](#739)) ([0b28886](0b28886)) * **aws-codedeploy:** Deployment Configuration Construct. ([#653](#653)) ([e6b67ad](e6b67ad)) * **aws-codedeploy:** support setting a load balancer on a Deployment Group. ([#786](#786)) ([e7af9f5](e7af9f5)) * **aws-codepipeline:** allow specifying the runOrder property when creating Actions. ([#776](#776)) ([d146c8d](d146c8d)) * **aws-codepipeline, aws-codecommit, aws-s3:** change the convention for naming the source Actions to XxxSourceAction. ([#753](#753)) ([9c3ce7f](9c3ce7f)) * **aws-dynamodb:** IAM grants support ([#870](#870)) ([c5a4200](c5a4200)) * **aws-dynamodb:** support Global Secondary Indexes ([#760](#760)) ([3601440](3601440)) * **aws-dynamodb:** tags support ([#814](#814)) ([924c84e](924c84e)) * **aws-dynamodB:** support Local Secondary Indexes ([#825](#825)) ([3175af3](3175af3)) * **aws-ec2:** add support for ICMP protocol's classification Types & Codes to SecurityGroupRule ([#893](#893)) ([85bd3c0](85bd3c0)) * **aws-ec2:** allow configuring subnets for NAT gateway ([#874](#874)) ([8ec761c](8ec761c)) * **aws-ec2:** support UDP port ranges in SecurityGroups ([#835](#835)) ([b42ef90](b42ef90)) * **aws-elasticloadbalancingv2:** support for ALB/NLB ([#750](#750)) ([bd9ee01](bd9ee01)) * **aws-s3:** support granting public access to objects ([#886](#886)) ([bdee191](bdee191)), closes [#877](#877) * **cdk:** Add support for UseOnlineResharding with UpdatePolicies ([#881](#881)) ([1f717e1](1f717e1)) * **cdk:** configurable default SSM context provider ([#889](#889)) ([353412b](353412b)) * **core:** resource overrides (escape hatch) ([#784](#784)) ([5054eef](5054eef)), closes [#606](#606) * **aws-codepipeline**: Manage IAM permissions for (some) CFN CodePipeline actions ([#843](#843)) ([4c69118](4c69118)) * **toolkit:** Stop creating 'empty' stacks ([#779](#779)) ([1dddd8a](1dddd8a)) * **aws-autoscaling, aws-ec2:** Tagging support for AutoScaling/SecurityGroup ([#766](#766)) ([3d48eb2](3d48eb2)) ### BREAKING CHANGES * **framework:** The `cdk.App` constructor doesn't accept any arguments, and `app.run()` does not return a `string` anymore. All AWS CDK apps in all languages would need to be modified to adhere to the new API of the `cdk.App` construct. Instead of: const app = new App(process.argv); // ERROR // add stacks process.stdout.write(app.run()); // ERROR The new usage is: const app = new App(); // add stacks app.run(); * **framework:** The CDK is no longer shipped with built-in support for JSX. You can still use JSX but you will have to manually configure it. * **aws-iam:** `PolicyDocument`, `PolicyStatement` and all `PolicyPrincipal` classes moved from the @aws-cdk/cdk module and into the @aws-cdk/aws-iam module. * **aws-codepipeline-api**: `Artifact.subartifact` method of the CodePipeline API was renamed to `Artifact.atPath`. * constructor signature of `TagManager` has changed. `initialTags` is now passed inside a props object. * **util:** @aws-cdk/util is no longer available * **aws-elasticloadbalancingv2:** Adds classes for modeling Application and Network Load Balancers. AutoScalingGroups now implement the interface that makes constructs a load balancing target. The breaking change is that Security Group rule identifiers have been changed in order to make adding rules more reliable. No code changes are necessary but existing deployments may experience unexpected changes. * **aws-cloudformation:** this renames all CloudFormation Actions for CodePipeline to bring them in line with Actions defined in other service packages. * **aws-codepipeline, aws-codecommit, aws-s3:** change the names of the source Actions from XxxSource to XxxSourceAction. This is to align them with the other Actions, like Build. Also, CodeBuild has the concept of Sources, so it makes sense to strongly differentiate between the two.

BREAKING(aws-ec2) Configure NAT to Subnet Placement

a6fb990

* BREAKING change to VPC configuration only if natGateways was previously set * closes aws#741 * Enable NAT placement by subnet name * Change VpcNeworkProps.natGateways from number to { gatewayCount: number; subnetName: string }

moofish32 force-pushed the f-nat-gw-subnet branch from bbbeb65 to a6fb990 Compare October 9, 2018 08:04

moofish32 changed the title ~~feat(aws-ec2) Configure NAT to Subnet Placement~~ BREAKING(aws-ec2) Configure NAT to Subnet Placement Oct 9, 2018

rix0rrr reviewed Oct 9, 2018

View reviewed changes

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

ef3e4a0

* closes aws#741 * Enable NAT placement by `VpcPlacementStrategy` * Add `natGatewayPlacement` to `VpcNeworkProps` * Refactor NAT creation logic into one place

moofish32 changed the title ~~BREAKING(aws-ec2) Configure NAT to Subnet Placement~~ feat(aws-ec2) Configure NAT to Subnet Placement Oct 9, 2018

rix0rrr approved these changes Oct 10, 2018

View reviewed changes

Update nat gateway placement docs

83f61e1

rix0rrr merged commit 4a76287 into aws:master Oct 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

moofish32 commented Oct 9, 2018

rix0rrr Oct 9, 2018

rix0rrr Oct 9, 2018

rix0rrr Oct 9, 2018

rix0rrr Oct 9, 2018

moofish32 Oct 9, 2018

moofish32 Oct 9, 2018

rix0rrr Oct 9, 2018

moofish32 commented Oct 9, 2018

rix0rrr Oct 10, 2018

rix0rrr commented Oct 10, 2018

rix0rrr commented Oct 10, 2018

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

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

Conversation

moofish32 commented Oct 9, 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

moofish32 commented Oct 9, 2018

Choose a reason for hiding this comment

rix0rrr commented Oct 10, 2018

rix0rrr commented Oct 10, 2018