Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Allow overriding NAT Gateway for fallback #89

Merged
merged 1 commit into from
Apr 23, 2024
Merged

Allow overriding NAT Gateway for fallback #89

merged 1 commit into from
Apr 23, 2024

Conversation

kristian-lesko
Copy link
Contributor

@kristian-lesko kristian-lesko commented Apr 18, 2024

Hello, thanks a lot for this wonderful module! I'd like to propose a simple extension that may be useful for some users.

It may be the case that only NAT Gateway is kept for fallback from alternat instances, since paying for separate gateways per AZ may not be worth the extra NatGatewayHours costs.

In such case, it can be useful to manage the single NAT Gateway outside of the module, and provide its ID to Lambda functions. This would override the default behaviour of selecting the NAT Gateway in the same subnet for fallback.

resource "aws_nat_gateway" "this" {}

module "alternat" {
  create_nat_gateways = false
  lambda_environment_variables = {
    NAT_GATEWAY_ID = aws_nat_gateway.this.id
  }
}

@kristian-lesko kristian-lesko requested review from a team as code owners April 18, 2024 12:00
@bwhaley
Copy link
Member

bwhaley commented Apr 18, 2024

Cool! I think this is a good idea. Note that you will incur $0.01/GB in each direction by routing across AZs.

It could be even easier to use if we added a new optional variable nat_gateway_id to the Terraform module. If set, it would set up the env var on both of the Lambdas. Similar to lambda_has_ipv6 but for both functions. Then we can add another note in the README in the "other considerations" section to explain it.

Would you be up for that small tweak?

@kristian-lesko
Copy link
Contributor Author

Hi @bwhaley,

Sure, sounds good! I had an extra idea, which was to extend the vpc_az_maps variable with a nat_gateway_id attribute:

variable "vpc_az_maps" {
  description = "A map of az to private route tables that the NAT instances will manage."
  type = list(object({
    az                 = string
    nat_gateway_id     = optional(string)
    private_subnet_ids = list(string)
    public_subnet_id   = string
    route_table_ids    = list(string)
  }))
}

This might be the most flexible approach, since it would cover all situations when there are less NAT Gateways deployed than there are availability zones.

Would that work in your opinion? Thanks!

@bwhaley
Copy link
Member

bwhaley commented Apr 19, 2024

I think that would work if you plumb it through to both of the Lambda function handlers. That would be more flexible than the original idea. It's not as DRY - you might use the same NAT gateway ID for each object in the list, like for your use case. Still, it doesn't seem that bad. Want to make an attempt at this approach?

@kristian-lesko
Copy link
Contributor Author

Hello @bwhaley,

After further thought, I decided to try and implement your first suggestion from #89 (comment). I didn't realise previously that getting the proper NAT Gateway ID value to the autoscaling hook, which spans through all subnets, would be more tedious. For the use case I have, a single override NAT Gateway ID is plenty.

Can you please re-check the code to see if this would work for you? Thanks a lot!

Copy link
Member

@bwhaley bwhaley left a comment

Choose a reason for hiding this comment

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

Just one nit and I think we can ship it.

It may be the case that only NAT Gateway is kept
for fallback from alternat instances, since paying
for separate gateways per AZ may not be worth the
extra `NatGatewayHours` costs. In such case, it can
be useful to manage the single NAT Gateway outside
of the module, and provide its ID to Lambda functions:
```
resource "aws_nat_gateway" "this" {}

module "alternat" {
  lambda_environment_variables = {
    NAT_GATEWAY_ID = aws_nat_gateway.this.id
  }
}
```
Copy link
Member

@bwhaley bwhaley 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 this contribution.

@bwhaley bwhaley merged commit 01420b2 into chime:main Apr 23, 2024
@kristian-lesko kristian-lesko deleted the nat_gateway_id_override branch April 24, 2024 08:02
bwhaley pushed a commit to armona/alternat that referenced this pull request Apr 24, 2024
It may be the case that only NAT Gateway is kept
for fallback from alternat instances, since paying
for separate gateways per AZ may not be worth the
extra `NatGatewayHours` costs. In such case, it can
be useful to manage the single NAT Gateway outside
of the module, and provide its ID to Lambda functions:
```
resource "aws_nat_gateway" "this" {}

module "alternat" {
  lambda_environment_variables = {
    NAT_GATEWAY_ID = aws_nat_gateway.this.id
  }
}
```
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants