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

Allow passing in EIPs for the NAT Gateways #38

Conversation

miguelaferreira
Copy link
Contributor

@miguelaferreira miguelaferreira commented Nov 28, 2017

This PR adds enables passing in external EIPs to be assigned to the NAT Gateways (Fixes #37).

To that end, this PR:

  • Adds two variables: a boolean flag to enable disable the feature and a list of EIP IDs to be used if the feature is enabled.
  • Adds the logic to either create new NAT Gateway IPs or use the EIP IDs passed in to the module
  • Adds a restriction to the required terraform version (>= 0.10.13) since the selection of either created or passed in IPs requires local values.

I have not documented the feature because I would like some input from the maintainers of the module on where to document this. If the PR is accepted, I could add the documentation to the main README file, to the existing examples, or even make a new example.

Copy link
Member

@antonbabenko antonbabenko left a comment

Choose a reason for hiding this comment

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

First, thanks for the contribution! Second, could you please update the main README with a paragraph saying how to use this feature and provide sample configuration there. I don't think we need to have a dedicated example of it in "examples" directory.

@@ -152,16 +156,28 @@ resource "aws_elasticache_subnet_group" "elasticache" {
##############
# NAT Gateway
##############
# Workaround for interpolation not being able to "short-circuit" the evaluation of the conditional branch that doesn't end up being used
Copy link
Member

Choose a reason for hiding this comment

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

Very good explanation.

variables.tf Outdated
@@ -65,6 +65,17 @@ variable "single_nat_gateway" {
default = false
}

variable "reuse_nat_ips" {
description = "Should be true if you don't want EIPs to be created for your NAT Gateways and will instead pass them in via the external_nat_ips variable"
Copy link
Member

Choose a reason for hiding this comment

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

Change: external_nat_ips => external_nat_ip_ids

@miguelaferreira miguelaferreira force-pushed the external-ip-nat-gateway branch 4 times, most recently from 126e813 to 556fb60 Compare December 7, 2017 14:37
@miguelaferreira
Copy link
Contributor Author

Thank you for taking the time to review my PR. I've pushed the changes you requested. Please let me know if there's anything else.

@antonbabenko antonbabenko merged commit 5c111ad into terraform-aws-modules:master Dec 7, 2017
@antonbabenko
Copy link
Member

Thanks! v1.9.0 is now released.

@miguelaferreira
Copy link
Contributor Author

Great! Thank you for the corrections to the documentation and the fast release of a new version.

@github-actions
Copy link

github-actions bot commented Nov 6, 2022

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow passing in the EIPs for the NAT gateways
2 participants