Fix #117 - Add one_nat_gateway_per_az functionality

[sc250024]

Description

Fixes #117 by adding a new variable called one_nat_gateway_per_az. This enables the feature that only one NAT gateway is created for each availability zone. By default, the number of NAT Gateways created is equal to max_subnet_length, which is calculated as follows

max_subnet_length = "${max(length(var.private_subnets), length(var.elasticache_subnets), length(var.database_subnets), length(var.redshift_subnets))}"

For example, if a user specified three database subnet CIDR blocks, four ElastiCache subnet CIDR blocks, and five private subnet CIDR blocks, then five NAT Gateways would be created since five is the maximum of those lists.

Motivation and Context

As per #117, the initial thought was that the default behavior of creating max_subnet_length number of NAT Gateways was a bit overkill considering that:

• Each NAT Gateway instance costs money per hour just being active: https://aws.amazon.com/vpc/pricing/#natgatewaypricing
• Most organizations will not hit the 5 - 45 Gbps limit set by each NAT Gateway instance: https://docs.aws.amazon.com/AmazonVPC/latest/UserGuide/vpc-nat-gateway.html#nat-gateway-limits

However, as @antonbabenko suggested in the issue (#117 (comment)), multiple scenarios should be supported.

How Has This Been Tested?

Ran terraform apply on the following modification of the Complete VPC example code:

provider "aws" {
  region = "us-east-1"
}

module "vpc" {
  source = "../../"

  name = "complete-example"

  cidr = "10.37.0.0/16"

  azs                 = ["us-east-1a", "us-east-1b", "us-east-1c", "us-east-1d", "us-east-1e"]
  private_subnets     = ["10.37.1.0/24", "10.37.2.0/24", "10.37.3.0/24", "10.37.4.0/24", "10.37.5.0/24", "10.37.6.0/24", "10.37.7.0/24", "10.37.8.0/24"]
  public_subnets      = ["10.37.11.0/24", "10.37.12.0/24", "10.37.13.0/24", "10.37.14.0/24", "10.37.15.0/24"]
  database_subnets    = ["10.37.21.0/24", "10.37.22.0/24", "10.37.23.0/24"]
  elasticache_subnets = ["10.37.31.0/24", "10.37.32.0/24", "10.37.33.0/24"]
  redshift_subnets    = ["10.37.41.0/24", "10.37.42.0/24", "10.37.43.0/24", "10.37.44.0/24"]

  create_database_subnet_group = false

  enable_nat_gateway = true
  enable_vpn_gateway = true

  enable_s3_endpoint       = true
  enable_dynamodb_endpoint = true

  one_nat_gateway_per_az = true

  enable_dhcp_options              = true
  dhcp_options_domain_name         = "service.consul"
  dhcp_options_domain_name_servers = ["127.0.0.1", "10.37.0.2"]

  tags = {
    Owner       = "user"
    Environment = "staging"
    Name        = "complete"
  }
}

It is confirmed that the number of NAT gateways (5 since the AZ count is 5) created does not exceed the 8 number of private subnet CIDR blocks specified in private_subnets.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.

[sc250024]

@antonbabenko I imagine there will be documentation changes, so just let me know what you want updated.

[antonbabenko]

Thanks Scott! PR looks pretty elegant. I will try to review it during Monday-Tuesday.

[antonbabenko]

I spent 20 minutes to try to break it and I have failed :) Good work!

Few questions before merge:

1. Could you please add a section describing the supported relations between AZs and nr of NAT gateways into README.md? It should help people to have a better understanding of features.
2. What if we make one_nat_gateway_per_az = true by default? It will promote better design for people who want to get VPC done with minimum efforts. People who are using master branch of this repo will have to specify one_nat_gateway_per_az = true if they can't afford recreation of NAT gateways and routes.

[sc250024]

@antonbabenko

For item 1: Yes, I will do that. Since this feature is a bit different, probably having something in the README.md makes sense.

For item 2: I wouldn't be opposed to it. When I specified false it was because I didn't want to break people's existing VPC setups. Technically that would be a breaking change if we specified that to true by default.

If we set one_nat_gateway_per_az to true by default, and go with that breaking change, as a suggestion, maybe we could roll this PR and also the following PRs / issues into a v2.0.0 release?

• PR Provide separate route tables for db/elasticache/redshift #122
• PR Adds support for ClassicLink DNS #121
• PR Fixed aws_vpn_gateway_route_propagation for default route table #94
• Issue Any way to configure database subnets to be the private subnets? #128

I could help with some of the testing in those stale PRs as well if you need.

[antonbabenko]

Good docs. Let's set one_nat_gateway_per_az = true and cut major release v2.0.0.

After that, it would be great if you can follow up and verify issues&PRs you listed so we get some of them merged and publish releases as usual.

[sc250024]

@antonbabenko Check out the latest commit 73e96af. It has some of the consequences of setting that to be true by default.

Notable, if enable_nat_gateway is true, then both var.azs and var.public_subnets will be required.

Does the column "required" in the README mean always required? Technically those two aren't always required, but if one_nat_gateway_per_az = true, then they are.

[antonbabenko]

Right, then let's leave it false and release as minor release. It will be still great to have this feature available.

[sc250024]

@antonbabenko Reverted back. Should be good to go!

[antonbabenko]

v1.32.0 has been released.

[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.