feat(ec2): support NAT instances, AMI lookups by rix0rrr · Pull Request #4898 · aws/aws-cdk

rix0rrr · 2019-11-07T14:49:11Z

Add support for NAT instances (as opposed to NAT gateways) on VPCs. This
change introduces the concept of a 'NAT provider', and provides two
implementations out of the box: one for gateways, one for instances.

Instances are not guarded against termination; a future implementation
should use ASGs to make sure there are always instances running.

To make it easier to pick the right AMI for the NAT instance,
add an AMI context provider, which will look up AMIs available to
the user.

Fixes #4876.

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

Add support for NAT instances (as opposed to NAT gateways) on VPCs. This change introduces the concept of a 'NAT provider', and provides two implementations out of the box: one for gateways, one for instances. Instances are not guarded against termination; a future implementation should use ASGs to make sure there are always instances running. To make it easier to pick the right AMI for the NAT instance, add an AMI context provider, which will look up AMIs available to the user. Fixes #4876.

mergify · 2019-11-07T14:49:50Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

rix0rrr · 2019-11-07T14:50:24Z

I explicitly pushed out the CX API protocol bump a couple of versions. I don't expect to merge this before the next release (maybe the one after that).

aws-cdk-automation · 2019-11-07T14:52:01Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2019-11-07T15:04:13Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2019-11-07T15:14:06Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2019-11-07T15:44:50Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

eladb

Wow! When did you do this??

eladb · 2019-11-07T21:16:16Z

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

+   * If you have a specific AMI ID you want to use, pass a `GenericLinuxImage`. For example:
+   *
+   * ```ts
+   * NatProvider.instance({


@example?

Yeeahhhh... not a big fan of @example. But I suppose I could.

Examples please!!!

eladb · 2019-11-07T21:16:33Z

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

+  /**
+   * Instance type of the NAT instance
+   */
+  readonly instanceType: InstanceType;


It's not optional.

Let's make it optional and pick a sensible default, no?

eladb · 2019-11-07T21:18:07Z

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

+   * Select between NAT gateways or NAT instances. NAT gateways
+   * may not be available in all AWS regions.
+   *
+   * @default - NatProvider.gateway()


If you have a concrete value (NatProvider.gateway()) you should use it instead of -, no?

Yes, you are right.

eladb · 2019-11-07T21:20:38Z

packages/@aws-cdk/cx-api/lib/versioning.ts

  }

+  if (manifest.version === '1.16.0') {
+    // Added AMI context provider


a note why it is "safe" to upgrade

aws-cdk-automation · 2019-11-07T21:48:18Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

nija-at · 2019-11-08T10:31:29Z

packages/@aws-cdk/aws-ec2/lib/machine-image.ts

+ * evict the value from the cache using the `cdk context` command. See
+ * https://docs.aws.amazon.com/cdk/latest/guide/context.html for more information.
+ */
+export class LookupMachineImage implements IMachineImage {


I believe we need to get this documented under the 'Context Methods' section in here

Good catch.

aws-cdk-automation · 2019-11-08T12:46:43Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2019-11-11T08:42:57Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2019-11-11T09:21:04Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

rix0rrr requested review from RomainMuller, eladb, nija-at, shivlaks and skinny85 November 7, 2019 14:49

rix0rrr self-assigned this Nov 7, 2019

rix0rrr added the pr/do-not-merge This PR should not be merged at this time. label Nov 7, 2019

Set CX version to 1.17.0

78a31aa

Add missing 'readonly's

4230bec

Document @default

a126c91

Delete sdk-api-metadata.json

e51ef54

eladb approved these changes Nov 7, 2019

View reviewed changes

nija-at reviewed Nov 8, 2019

View reviewed changes

Improve docs

253aaf8

rix0rrr mentioned this pull request Nov 8, 2019

feat(ec2): provide readonly access to NAT gateways in VPC #4926

Closed

SomayaB added the contribution/core This is a PR that came from AWS. label Nov 8, 2019

Target integration with upcoming release

3e077aa

Fix test

4b973de

rix0rrr merged commit dca9a24 into master Nov 11, 2019

rix0rrr deleted the huijbers/nat-instances branch November 11, 2019 09:30

travtarr mentioned this pull request Nov 17, 2019

VPC: allow configuring NAT instances instead of gateways (and a 0 NAT gateways bug) #1305

Closed

Conversation

rix0rrr commented Nov 7, 2019

Uh oh!

mergify bot commented Nov 7, 2019

Uh oh!

rix0rrr commented Nov 7, 2019

Uh oh!

aws-cdk-automation commented Nov 7, 2019

AWS CodeBuild CI Report

Uh oh!

aws-cdk-automation commented Nov 7, 2019

AWS CodeBuild CI Report

Uh oh!

aws-cdk-automation commented Nov 7, 2019

AWS CodeBuild CI Report

Uh oh!

aws-cdk-automation commented Nov 7, 2019

AWS CodeBuild CI Report

Uh oh!

eladb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aws-cdk-automation commented Nov 7, 2019

AWS CodeBuild CI Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aws-cdk-automation commented Nov 8, 2019

AWS CodeBuild CI Report

Uh oh!

aws-cdk-automation commented Nov 11, 2019

AWS CodeBuild CI Report

Uh oh!

aws-cdk-automation commented Nov 11, 2019

AWS CodeBuild CI Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants