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

Added support for additional VPC cidr block associations #9

Merged

Conversation

kieranbrown
Copy link
Contributor

This module currently won't work for VPC's that have multiple CIDR block allocations due to the security group not allowing inbound traffic.

Also fixed (assumably) a typo on the aws_network_interface not using security groups passed into the module.

@RaJiska
Copy link
Owner

RaJiska commented Feb 25, 2024

Thanks for the PR @kieranbrown. Indeed the changes would allow VPCs with multiple CIDRs to work. As for the security groups not passed to the network interface, please note that this isn't a typo.

The NAT instance has a fixed ENI which sole purpose is to be the "reliable interlocutor" between services that need to be NATed within the VPC and the NAT instance. The list of security groups is assigned to the public (ephemeral) ENI, cf ec2.tf.

The PR would be good to be merged once the change on the security group of the static ENI is reverted.

@kieranbrown
Copy link
Contributor Author

Thanks for the PR @kieranbrown. Indeed the changes would allow VPCs with multiple CIDRs to work. As for the security groups not passed to the network interface, please note that this isn't a typo.

The NAT instance has a fixed ENI which sole purpose is to be the "reliable interlocutor" between services that need to be NATed within the VPC and the NAT instance. The list of security groups is assigned to the public (ephemeral) ENI, cf ec2.tf.

The PR would be good to be merged once the change on the security group of the static ENI is reverted.

Makes sense, I’ve reverted this change 👍

@RaJiska RaJiska merged commit 2172a1d into RaJiska:main Feb 25, 2024
1 check passed
@RaJiska
Copy link
Owner

RaJiska commented Feb 25, 2024

Thank you & merged, appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants