fix: Create only required number of NAT gateways

[y-batsianouski]

Description

If private_subnets > public_subnets || elasticache_subnets > public_subnets || database_subnets > public_subnets || redshift_subnets > public_subnets and single_nat_gateway = false this module creates more NAT gateways than public subnets

Motivation and Context

In case we have number of the private_subnets, elasticache_subnets, database_subnets or redshift_subnets greater then public_subnets, this module creates more NAT gateways than really need. This leads some public subnets has multiple NAT gateways. This is obvious and there are no sensible reasons for such configuration and money-wasting.

Breaking Changes

Current users may encounter that some of their NAT gateways will be destroyed. But in general it shouldn't affect their infrastructure.

How Has This Been Tested?

Case 1

module "vpc-test" {
  ...
  cidr             = "10.40.0.0/16"
  private_subnets  = ["10.40.4.0/24", "10.40.5.0/24", "10.40.6.0/24"]
  public_subnets   = ["10.40.1.0/24", "10.40.2.0/24"]
  ...
}

Module creates two NAT gateways. Route table for private subnet 10.40.6.0/24 points to the NAT in public subnet 10.40.1.0/24

Case 2

module "vpc-test" {
  ...
  cidr             = "10.40.0.0/16"
  private_subnets  = ["10.40.4.0/24", "10.40.5.0/24"]
  public_subnets   = ["10.40.1.0/24", "10.40.2.0/24", "10.40.3.0/24"]
  ...
}

Module creates two NAT gateways - one for each private subnet. Public subnet 10.40.3.0/2 hasn't NAT gateway

[y-batsianouski]

@antonbabenko Did you have a chance to look at this? Thanks!

[antonbabenko]

No, I have not looked into this, and not sure when exactly I will be able to do it.

[y-batsianouski]

@antonbabenko Is there anything I can do to help you to approve this PR?

[antonbabenko]

Thanks, but I don't know where you can help with review.

Just by looking at this PR, I have a feeling that this PR is not 100% correct, but where exactly it should be improved will require some time from my side to run through a set of cases (real examples in examples folder).

[antonbabenko]

Thanks, @y-batsianouski !

v2.56.0 has been just released.

PS: I have updated some routes and route_table resources because I don't think we need that. Let me know if I am wrong.

[y-batsianouski]

@antonbabenko Don't mind. Thank you!

[lorengordon]

fwiw, this config is now failing. dropping back to v2.55.0 and it works.

module "vpc" {
  source = "git::https://github.com/terraform-aws-modules/terraform-aws-vpc.git?ref=v2.56.0"

  name            = "testing123"
  cidr            = "10.0.0.0/16"
  azs             = ["us-east-1a", "us-east-1b"]
  private_subnets = ["10.0.1.0/24", "10.0.2.0/24"]
}

errors:

Error: Error in function call

  on .terraform/modules/vpc/main.tf line 987, in resource "aws_route_table_association" "private":
 987:   route_table_id = element(
 988:     aws_route_table.private.*.id,
 989:     var.single_nat_gateway ? 0 : count.index,
 990:   )
    |----------------
    | aws_route_table.private is empty tuple
    | count.index is 0
    | var.single_nat_gateway is false

Call to function "element" failed: cannot use element function with an empty
list.


Error: Error in function call

  on .terraform/modules/vpc/main.tf line 987, in resource "aws_route_table_association" "private":
 987:   route_table_id = element(
 988:     aws_route_table.private.*.id,
 989:     var.single_nat_gateway ? 0 : count.index,
 990:   )
    |----------------
    | aws_route_table.private is empty tuple
    | count.index is 1
    | var.single_nat_gateway is false

Call to function "element" failed: cannot use element function with an empty
list.

[antonbabenko]

private_subnets expects public_subnets to be specified, so that it knows where to create NAT gateways. Alternatively, you can try to disable the creation of NAT gateways.

[lorengordon]

I think when public_subnets = [] it makes sense to create zero NAT gateways. Pretty sure that's how this worked before.

[antonbabenko]

private_subnets will be created as private when there is NAT gateway involved, else they are intra_subnets.

Let me verify if this behavior has changed by this PR and if so, I will make a PR adjusting it now.

[lorengordon]

Ok. I tried adding enable_nat_gateway = false but it still fails the same way...

[Dogers]

I'm hitting exactly this too, although I'm at a loss as to how it's occurring, given this shouldn't affect it..?

From the console:
> var.single_nat_gateway ? 1 : var.one_nat_gateway_per_az ? length(var.azs) : local.max_subnet_length < length(var.public_subnets) ? local.max_subnet_length : length(var.public_subnets) returns 0 so logically nothing should have changed for local.nat_gateway_count
> aws_route_table.private.*.id returns the three route table IDs
> element(aws_route_table.private.*.id, var.single_nat_gateway ? 0 : 1,) returns one of the three RTBs too!

[antonbabenko]

@Dogers Could you show what kind of inputs do you pass into this module?

[Dogers]

It's done via terragrunt, but the things we're setting are: cidr, azs, private_subnets[3 cidrs], enable_dns_hostnames = true, propagate_private_route_tables_vgw = true, finally vpc/subnet tags and name

[antonbabenko]

After careful reading of the issue and thinking about this one more time, I don't see the problem in the behaviour this pull-request was fixing.

I just run this few times trying to reproduce the behavious and it looks to me that the module creates correct amount of NAT gateways in various configurations, here are some poins (in addition to those specified in README):

1. If necessary to create just one NAT per AZ, user should specify one_nat_gateway_per_az = true.
2. User should set single_nat_gateway = true to create just one NAT. It will be placed in the first availability zone specified in azs.
3. If creating NAT, public_subnets should be defined also.

I propose to revert this PR as it does not introduce new functionality nor fixing bug.

Do we all agree?

[Dogers]

I'd agree - looking at what's changed I don't understand how it can be affected this way, but changing 2.55 > 2.56 breaks it and vice versa fixes.. TF bug perhaps?

[lorengordon]

I'd agree also. I think the solution for the described issue is to use one_nat_gateway_per_az = true.

[antonbabenko]

Please use v2.57.0 which is the same as v2.55.0.

Big thanks @lorengordon and @Dogers for bringing this issue!

[github-actions]

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.