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

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

Closed
2 tasks
arachnilith opened this issue Oct 12, 2023 · 6 comments · Fixed by #29769
Closed
2 tasks

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

arachnilith opened this issue Oct 12, 2023 · 6 comments · Fixed by #29769
Labels
@aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1

Comments

@arachnilith
Copy link

arachnilith commented Oct 12, 2023

Describe the feature

There is currently a cyclic dependency between Security Groups and VPCs within NatInstanceProvider.

The dependency chain looks something like this:

Vpc
constructor props: VpcProps
natGatewayProvider: NatProvider
NatInstanceProvider -> NatProvider
constructor props: NatInstanceProps
securityGroup: ISecurityGroup
SecurityGroup -> ISecurityGroup
constructor props: SecurityGroupProps
vpc: IVpc

As discussed during the office hours session, I believe you could replicate the design used for subnet selection to create named security groups. You could add a property to VpcProps that allows the definition of Security groups to be created, which would include some identifying name property in addition to the other parameters. And then you could create a NatInstanceProps parameter that accepts the security group by the identifying name instead of as an ISecurityGroup, or perhaps some other NatProvider class that would integrate such options to construct NAT Gateway instances.

Use Case

The use case is creating NatGateways on VPC construction as appears to be possible given the existing API, but doesn't seem to be fully fleshed out. Currently that feature seems to be broken around the security group assignment for the NAT gateways creating a cyclic dependency on the need for a security group to be assigned a VPC.

Proposed Solution

As discussed during the office hours session, I believe you could replicate the design used for subnet selection to create named security groups. You could add a property to VpcProps that allows the definition of Security groups to be created, which would include some identifying name property in addition to the other parameters. And then you could create a NatInstanceProps parameter that accepts the security group by identifying name instead of as an ISecurityGroup, or perhaps some other NatProvider class that would integrate such options to construct NAT Gateway instances.

Other Information

I think we're constrained by the limitations of the corresponding CloudFormation constructs here, so creating security groups without needing a VPC, such that they may be passed as arguments to the NAT Gateway instance providers and subsequently assigned the VPC doesn't seem to be an option.
Currently the only workarounds seem to include:

  • using entirely L1 constructs
  • dropping the NAT Gateway feature from VPC construction entirely
  • ignoring the NAT Gateway Vpc-construction-time feature and manually adding security groups and NAT gateways after the fact (after the VPC is constructed)

Acknowledgements

  • I may be able to implement this feature request
  • This feature might incur a breaking change

CDK version used

2.99.1

Environment details (OS name and version, etc.)

Any, MacOS/AL2

@arachnilith arachnilith added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Oct 12, 2023
@github-actions github-actions bot added the @aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud label Oct 12, 2023
@peterwoodworth peterwoodworth added p1 effort/medium Medium work item – several days of effort and removed needs-triage This issue or PR still needs to be triaged. labels Oct 12, 2023
@peterwoodworth
Copy link
Contributor

Thanks for the request and for coming to office hours, this would be a great feature

@arachnilith
Copy link
Author

At the office hours where I was told to create this ticket, I was given the impression this might see some prioritization. It's been over a month now with no action; are there plans to follow through with this, or should we start using L1 constructs?

@nmussy
Copy link
Contributor

nmussy commented Apr 4, 2024

I'm not sure if this solves all of the described issues, but I'm adding in #29729 a getter method, natGatewayProvider.gatewayInstances, to retrieve the NAT instances after the VPC has been created. For example, this allowed me to grant additional permissions to the instances' role:

const natGatewayProvider = ec2.NatProvider.instanceV2({
  instanceType: new ec2.InstanceType('t3.small'),
});

const vpc = new ec2.Vpc(this, 'MyVpc', {
  natGatewayProvider,
  natGateways: 2,
});

const bucket = new s3.Bucket(this, 'Bucket');
for (const gateway of natGatewayProvider.gatewayInstances) {
  bucket.grantWrite(gateway);
}

This could just as easily be used to create and attach a new security group:

const vpc = new ec2.Vpc(this, 'MyVpc', {
  natGatewayProvider,
  natGateways: 2,
});

const securityGroup = new ec2.SecurityGroup(this, 'SecurityGroup', { vpc });
securityGroup.addIngressRule(ec2.Peer.anyIpv4(), ec2.Port.tcp(22));
for (const gateway of natGatewayProvider.gatewayInstances) {
  gateway.addSecurityGroup(securityGroup);
}

@arachnilith
Copy link
Author

Interesting workaround. My only concern at this point revolves around the timing. Is addSecurityGroup compile-time logic or instancing time logic? I.e. is the gateway added to the security group after the gateway has already been instanced (which makes it vulnerable for a brief time before it can be added), or is that syntax a compile-time description? Other CDK APIs present similarly confusing syntax.

@nmussy
Copy link
Contributor

nmussy commented Apr 8, 2024

The example listed above wouldn't be any different than running the following, in terms of VPC and security group configuration:

const vpc = new ec2.Vpc(this, 'VPC');

const instance = new ec2.Instance(this, 'inst', {
  instanceType: ec2.InstanceType.of(ec2.InstanceClass.T3, ec2.InstanceSize.MICRO),
  machineImage: new ec2.AmazonLinuxImage({ generation: ec2.AmazonLinuxGeneration.AMAZON_LINUX_2023 }),
  vpc,
});

const securityGroup = new ec2.SecurityGroup(this, 'SecurityGroup', { vpc });
securityGroup.addIngressRule(ec2.Peer.anyIpv4(), ec2.Port.tcp(22));
instance.addSecurityGroup(securityGroup);

The only difference is how the instance is being generated, instantiated using the Instance constructor vs. instantiated using a NatProvider. They will both be wholly available at synth time, and the security group wouldn't be added on the fly, the instance will be spawned with it already configured.

If that was your only concern, I can wait until #29729 is merged to create another PR to close this issue, to documentation this use-case in the README and create an integration test to make sure this is viable.

@mergify mergify bot closed this as completed in #29769 Apr 11, 2024
mergify bot pushed a commit that referenced this issue 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*
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants