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

feat(ec2): NatInstanceProviderV2 improvements #29729

Merged
merged 12 commits into from
Apr 8, 2024

Conversation

nmussy
Copy link
Contributor

@nmussy nmussy commented Apr 4, 2024

Issue # (if applicable)

Closes #29720

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


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

@aws-cdk-automation aws-cdk-automation requested a review from a team April 4, 2024 19:50
@github-actions 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
Comment on lines 248 to 253
[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/).

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 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:

image

Comment on lines +197 to +201
> **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.
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'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

Comment on lines +1648 to +1652
// 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,
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 can't see a possible use for securityGroup, it might be worth just deprecating it

@aws-cdk-automation 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
Copy link
Contributor

@msambol msambol left a comment

Choose a reason for hiding this comment

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

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

@aws-cdk-automation 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
Copy link
Contributor

@GavinZZ GavinZZ left a comment

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

mergify bot commented Apr 8, 2024

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 aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Apr 8, 2024
Copy link
Contributor

mergify bot commented Apr 8, 2024

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
Copy link
Collaborator

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 mergify bot merged commit 4eb02a4 into aws:main Apr 8, 2024
9 checks passed
Copy link
Contributor

mergify bot commented Apr 8, 2024

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

@nmussy nmussy deleted the fix-ec2-nat-provider-init-script branch April 9, 2024 04:43
mergify bot pushed a commit that referenced this pull request Apr 11, 2024
### 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
Copy link
Collaborator

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.

@aws aws locked as resolved and limited conversation to collaborators Jul 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug This issue is a bug. distinguished-contributor [Pilot] contributed 50+ PRs to the CDK effort/small Small work item – less than a day of effort p2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aws-ec2: NatProvider.instanceV2 primary network interface
5 participants