feat(ec2): `NatInstanceProviderV2` improvements #29729

nmussy · 2024-04-04T19:50:13Z

Issue # (if applicable)

Might fix other issues, will need check the backlog

Reason for this change

The user data script used to initialize NAT instances was at present incorrect, assuming an incorrect default interface name. I wanted to both update this script, and allow CDK users to update this user data if needed in the future, both in case of an issue with the script or just to extend/override the initialization procedure.

Description of changes

Apologies for the non-atomic PR, as I was working on the unit and integration tests, I noticed additional issues and missing features. These are the initially targeted changes:

Add userData field to NatInstanceProps, allowing the user to overwrite the NAT instances initialization script
- Add static NatInstanceProps.DEFAULT_USER_DATA_COMMANDS field, allowing the user to more easily extend the default script
Fix default userData to query the instance default network interface, instead of always using eth0

These were found along the way:

Add getter gatewayInstances method to NatInstanceProps, allowing the user to interact with the created NAT instances once they're attached to the VPC
- This allowed me to give the instance role additional permissions, but has more potential uses
Fix creditSpecification not being implemented correctly
Updated existing unit and integ tests to increase coverage
Add warning in README about potential issues with NAT instances as is implemented

Description of how you validated changes

I've updated and added unit and integration tests

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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

nmussy · 2024-04-04T19:53:10Z

packages/aws-cdk-lib/aws-ec2/README.md

 [using NAT instances](test/integ.nat-instances.lit.ts) [Deprecated]

-The construct will use the AWS official NAT instance AMI, which has already 
+The V1 `NatProvider.instance` construct will use the AWS official NAT instance AMI, which has already
 reached EOL on Dec 31, 2023. For more information, see the following blog post: 
 [Amazon Linux AMI end of life](https://aws.amazon.com/blogs/aws/update-on-amazon-linux-ami-end-of-life/).



I find the end of this chapter (starting from the deprecated example) to be confusing, I've tried to clear it up a bit. It's also not terribly useful, given it's describing a deprecated behavior. I'd be happy to remove it completely:

nmussy · 2024-04-04T19:54:32Z

packages/aws-cdk-lib/aws-ec2/README.md

+> **Warning**
+> The NAT instances created using this method will be **unmonitored**.
+> They are not part of an Auto Scaling Group,
+> and if they become unavailable or are terminated for any reason,
+> will not be restarted or replaced.


I've ran into this issue in the past, https://github.com/fonzcastellanos/cdk-nat-asg-provider tries to bridge that gap. It might be worth a mention here

nmussy · 2024-04-04T19:57:02Z

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

+        // Unusuable in its current state
+        // The VPC is required to create the security group,
+        // but the NAT Provider is required to create the VPC
+        // See https://github.com/aws/aws-cdk/issues/27527
+        // securityGroup,


I can't see a possible use for securityGroup, it might be worth just deprecating it

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

packages/aws-cdk-lib/aws-ec2/README.md

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

packages/@aws-cdk-testing/framework-integ/test/aws-ec2/test/integ.nat-instances-v2-custom.ts

packages/@aws-cdk-testing/framework-integ/test/aws-ec2/test/integ.nat-instances-v2.ts

msambol

LGTM. Thanks for the thorough integration tests proving functionality. 👍🏼

GavinZZ

Nice, thank you for making the change. I created the NatInstanceProviderV2 and simply thought of it as a replacement for the V1. Glad to see it getting improvements and more customization capability.

mergify · 2024-04-08T22:20:58Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

mergify · 2024-04-08T22:24:51Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2024-04-08T23:25:05Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 8062cae
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2024-04-08T23:25:27Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

### Issue # (if applicable) Closes #27527 ### Reason for this change The current `NatInstanceProviderV2.securityGroup` property is unusable, given the dependency loop between the construct props (`NatInstanceProviderV2` > `VPC` > `SecurityGroup` > `NatInstanceProviderV2`). When creating the integration for #29729, adding a getter for the instances generated by the provider to update the instance role was required to test the `userData` overload. This solution also allows to bypass the circular dependency describe above, given that both the VPC and the instances are generated once the VPC is created with the `natGatewayProvider`. ### Description of changes * Deprecate `NatInstanceProviderV2.securityGroup` * Add `@example` tag to demo `NatInstanceProviderV2.gatewayInstances` * Update `README` to demo setting the security group * Update integ to test the demo ### Description of how you validated changes Updated integration test ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

aws-cdk-automation · 2024-07-25T21:17:54Z

Comments on closed issues and PRs are hard for our team to see. If you need help, please open a new issue that references this one.

nmussy added 7 commits April 4, 2024 17:41

feat(ec2): add userData to NatInstanceProps

d6f8628

fix: creditSpecification

891a8b1

chore: improve error coverage

0803069

chore: update integ

af38e4b

feat: expose provider gateway instances

02fc3d8

chore: add custom props integ test

fa4c5a9

chore: update README

62248ec

aws-cdk-automation requested a review from a team April 4, 2024 19:50

github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort p2 distinguished-contributor [Pilot] contributed 50+ PRs to the CDK labels Apr 4, 2024

nmussy commented Apr 4, 2024

View reviewed changes

nmussy mentioned this pull request Apr 4, 2024

aws-cdk/aws-ec2: Better Support for NAT Gateway Construction in L2 Vpc #27527

Closed

2 tasks

chore: fix README

ea0d07c

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Apr 4, 2024

ivanbarlog approved these changes Apr 5, 2024

View reviewed changes

packages/aws-cdk-lib/aws-ec2/lib/nat.ts Show resolved Hide resolved

msambol reviewed Apr 7, 2024

View reviewed changes

packages/aws-cdk-lib/aws-ec2/README.md Outdated Show resolved Hide resolved

msambol reviewed Apr 7, 2024

View reviewed changes

packages/aws-cdk-lib/aws-ec2/lib/nat.ts Outdated Show resolved Hide resolved

chore: typos

9a6ef45

msambol reviewed Apr 7, 2024

View reviewed changes

packages/@aws-cdk-testing/framework-integ/test/aws-ec2/test/integ.nat-instances-v2-custom.ts Outdated Show resolved Hide resolved

chore: missing new line

ef0cbd7

msambol reviewed Apr 7, 2024

View reviewed changes

packages/@aws-cdk-testing/framework-integ/test/aws-ec2/test/integ.nat-instances-v2.ts Outdated Show resolved Hide resolved

chore: missing new line

d02ff4f

msambol approved these changes Apr 7, 2024

View reviewed changes

aws-cdk-automation added pr/needs-maintainer-review This PR needs a review from a Core Team Member and removed pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels Apr 7, 2024

github-actions bot mentioned this pull request Apr 8, 2024

Weekly PR metrics report #29758

Closed

GavinZZ approved these changes Apr 8, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Apr 8, 2024

Merge branch 'main' into fix-ec2-nat-provider-init-script

8062cae

mergify bot merged commit 4eb02a4 into aws:main Apr 8, 2024
9 checks passed

nmussy deleted the fix-ec2-nat-provider-init-script branch April 9, 2024 04:43

nmussy mentioned this pull request Apr 9, 2024

chore(ec2): NatInstanceProviderV2 security group example #29769

Merged

1 task

github-actions bot mentioned this pull request May 1, 2024

Monthly PR metrics report #30026

Closed

This was referenced May 23, 2024

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#98

Open

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#99

Open

rwlxxvii mentioned this pull request May 27, 2024

[Snyk] Upgrade aws-cdk-lib from 2.136.1 to 2.140.0 rwlxxvii/containers#124

Merged

NOUIY mentioned this pull request May 30, 2024

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#101

Open

NOUIY mentioned this pull request Jun 7, 2024

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#102

Open

niravparikh05 mentioned this pull request Jun 13, 2024

[Snyk] Upgrade aws-cdk from 2.65.0 to 2.142.1 paralus/eks-blueprints-addon#3

Open

This was referenced Jun 14, 2024

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#103

Open

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#104

Open

aws locked as resolved and limited conversation to collaborators Jul 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ec2): `NatInstanceProviderV2` improvements #29729

feat(ec2): `NatInstanceProviderV2` improvements #29729

nmussy commented Apr 4, 2024

nmussy Apr 4, 2024

nmussy Apr 4, 2024

nmussy Apr 4, 2024

msambol left a comment

GavinZZ left a comment

mergify bot commented Apr 8, 2024

mergify bot commented Apr 8, 2024

aws-cdk-automation commented Apr 8, 2024

mergify bot commented Apr 8, 2024

aws-cdk-automation commented Jul 25, 2024

feat(ec2): NatInstanceProviderV2 improvements #29729

feat(ec2): NatInstanceProviderV2 improvements #29729

Conversation

nmussy commented Apr 4, 2024

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

nmussy Apr 4, 2024

Choose a reason for hiding this comment

nmussy Apr 4, 2024

Choose a reason for hiding this comment

nmussy Apr 4, 2024

Choose a reason for hiding this comment

msambol left a comment

Choose a reason for hiding this comment

GavinZZ left a comment

Choose a reason for hiding this comment

mergify bot commented Apr 8, 2024

mergify bot commented Apr 8, 2024

aws-cdk-automation commented Apr 8, 2024

AWS CodeBuild CI Report

mergify bot commented Apr 8, 2024

aws-cdk-automation commented Jul 25, 2024

feat(ec2): `NatInstanceProviderV2` improvements #29729

feat(ec2): `NatInstanceProviderV2` improvements #29729