Address reviewer comments, fix nat_instance_enabled=true

README.md

@@ -39,7 +39,9 @@ many optional inputs to this module are specified as a `list(string)` that can h
 as a `string` that could be empty or `null`. The designation of an input as a `list` type does not necessarily
 mean that you can supply more than one value in the list, so check the input's description before supplying more than one value.
 
-The core feature of this module is dividing up a given CIDR range so that a set of subnets each gets its own 
+The core function of this module is to create 2 sets of subnets, a "public" set with bidirectional access to the
+public internet, and a "private" set behind a firewall with egress-only access to the public internet. This 
+includes dividing up a given CIDR range so that a each subnet gets its own 
 distinct CIDR range within that range, and then creating those subnets in the appropriate availability zones.
 The intention is to keep this module relatively simple and easy to use for the most popular use cases. 
 In its default configuration, this module creates 1 public subnet and 1 private subnet in each
@@ -166,13 +168,15 @@ module "subnets" {
   name                = "app"
   vpc_id              = "vpc-XXXXXXXX"
   igw_id              = ["igw-XXXXXXXX"]
-  ipv4_cidr_block          = "10.0.0.0/16"
+  ipv4_cidr_block     = "10.0.0.0/16"
   availability_zones  = ["us-east-1a", "us-east-1b"]
 }
 ```
 
+Create only private subnets, route to transit gateway:
+
 ```hcl
-module "subnets_with_existing_ips" {
+module "private_tgw_subnets" {
   source = "cloudposse/dynamic-subnets/aws"
   # Cloud Posse recommends pinning every module to a specific version
   # version = "x.x.x"
@@ -181,10 +185,19 @@ module "subnets_with_existing_ips" {
   name                = "app"
   vpc_id              = "vpc-XXXXXXXX"
   igw_id              = ["igw-XXXXXXXX"]
-  cidr_block          = "10.0.0.0/16"
+  ipv4_cidr_block     = "10.0.0.0/16"
   availability_zones  = ["us-east-1a", "us-east-1b"]
-  nat_gateway_enabled = true
-  nat_elastic_ips     = ["1.2.3.4", "1.2.3.5"]
+
+  nat_gateway_enabled    = false
+  public_subnets_enabled = false
+}
+
+resource "aws_route" "private" {
+  count = length(module.private_tgw_subnets.private_route_table_ids)
+
+  route_table_id         = module.private_tgw_subnets.private_route_table_ids[count.index]
+  destination_cidr_block = "0.0.0.0/0"
+  transit_gateway_id     = "tgw-XXXXXXXXX"
 }
 ```
 
@@ -216,7 +229,7 @@ cidr_count = existing_az_count * subnet_type_count
 
 4. Calculate the number of bits needed to enumerate all the CIDRs:
 ```
-subnet_bits = ciel(log(cidr_count, 2))
+subnet_bits = ceil(log(cidr_count, 2))
 ```
 5. Reserve CIDRs for private subnets using [`cidrsubnet`](https://www.terraform.io/language/functions/cidrsubnet): 
 ```
@@ -231,7 +244,7 @@ public_subnet_cidrs = [ for netnumber in range(existing_az_count, existing_az_co
 Note that this means that, for example, in a region with 4 availability zones, if you specify only 3 availability zones 
 in `var.availability_zones`, this module will still reserve CIDRs for the 4th zone. This is so that if you later
 want to expand into that zone, the existing subnet CIDR assignments will not be disturbed. If you do not want
-to reserve these CIDRs
+to reserve these CIDRs, set `max_subnet_count` to the number of zones you are actually using.
 <!-- markdownlint-disable -->
 ## Makefile Targets
 ```text
@@ -352,10 +365,10 @@ Available targets:
 | <a name="input_name"></a> [name](#input\_name) | ID element. Usually the component or solution name, e.g. 'app' or 'jenkins'.<br>This is the only ID element not also included as a `tag`.<br>The "name" tag is set to the full `id` string. There is no tag with the value of the `name` input. | `string` | `null` | no |
 | <a name="input_namespace"></a> [namespace](#input\_namespace) | ID element. Usually an abbreviation of your organization name, e.g. 'eg' or 'cp', to help ensure generated IDs are globally unique | `string` | `null` | no |
 | <a name="input_nat_elastic_ips"></a> [nat\_elastic\_ips](#input\_nat\_elastic\_ips) | Existing Elastic IPs (not EIP IDs) to attach to the NAT Gateway(s) or Instance(s) instead of creating new ones. | `list(string)` | `[]` | no |
-| <a name="input_nat_gateway_enabled"></a> [nat\_gateway\_enabled](#input\_nat\_gateway\_enabled) | Flag to enable/disable NAT Gateways to allow servers in the private subnets to access the Internet | `bool` | `true` | no |
+| <a name="input_nat_gateway_enabled"></a> [nat\_gateway\_enabled](#input\_nat\_gateway\_enabled) | Set `true` to create NAT Gateways to perform IPv4 NAT and NAT64 as needed.<br>Defaults to `true` unless `nat_instance_enabled` is `true`. | `bool` | `null` | no |
 | <a name="input_nat_instance_ami_id"></a> [nat\_instance\_ami\_id](#input\_nat\_instance\_ami\_id) | A list optionally containing the ID of the AMI to use for the NAT instance.<br>If the list is empty (the default), the latest official AWS NAT instance AMI<br>will be used. NOTE: The Official NAT instance AMI is being phased out and<br>does not support NAT64. Use of a NAT gateway is recommended instead. | `list(string)` | `[]` | no |
 | <a name="input_nat_instance_cpu_credits_override"></a> [nat\_instance\_cpu\_credits\_override](#input\_nat\_instance\_cpu\_credits\_override) | NAT Instance credit option for CPU usage. Valid values are "standard" or "unlimited".<br>T3 and later instances are launched as unlimited by default. T2 instances are launched as standard by default. | `string` | `""` | no |
-| <a name="input_nat_instance_enabled"></a> [nat\_instance\_enabled](#input\_nat\_instance\_enabled) | Flag to enable/disable NAT Instances to allow servers in the private subnets to access the Internet | `bool` | `false` | no |
+| <a name="input_nat_instance_enabled"></a> [nat\_instance\_enabled](#input\_nat\_instance\_enabled) | Set `true` to create NAT Instances to perform IPv4 NAT.<br>Defaults to `false`. | `bool` | `null` | no |
 | <a name="input_nat_instance_root_block_device_encrypted"></a> [nat\_instance\_root\_block\_device\_encrypted](#input\_nat\_instance\_root\_block\_device\_encrypted) | Whether to encrypt the root block device on the created NAT instances | `bool` | `true` | no |
 | <a name="input_nat_instance_type"></a> [nat\_instance\_type](#input\_nat\_instance\_type) | NAT Instance type | `string` | `"t3.micro"` | no |
 | <a name="input_open_network_acl_ipv4_rule_number"></a> [open\_network\_acl\_ipv4\_rule\_number](#input\_open\_network\_acl\_ipv4\_rule\_number) | The `rule_no` assigned to the network ACL rules for IPv4 traffic generated by this module | `number` | `100` | no |

README.yaml

@@ -58,7 +58,9 @@ description: |-
   as a `string` that could be empty or `null`. The designation of an input as a `list` type does not necessarily
   mean that you can supply more than one value in the list, so check the input's description before supplying more than one value.
   
-  The core feature of this module is dividing up a given CIDR range so that a set of subnets each gets its own 
+  The core function of this module is to create 2 sets of subnets, a "public" set with bidirectional access to the
+  public internet, and a "private" set behind a firewall with egress-only access to the public internet. This 
+  includes dividing up a given CIDR range so that a each subnet gets its own 
   distinct CIDR range within that range, and then creating those subnets in the appropriate availability zones.
   The intention is to keep this module relatively simple and easy to use for the most popular use cases. 
   In its default configuration, this module creates 1 public subnet and 1 private subnet in each
@@ -124,13 +126,15 @@ usage: |-
     name                = "app"
     vpc_id              = "vpc-XXXXXXXX"
     igw_id              = ["igw-XXXXXXXX"]
-    ipv4_cidr_block          = "10.0.0.0/16"
+    ipv4_cidr_block     = "10.0.0.0/16"
     availability_zones  = ["us-east-1a", "us-east-1b"]
   }
   ```
+  
+  Create only private subnets, route to transit gateway:
 
   ```hcl
-  module "subnets_with_existing_ips" {
+  module "private_tgw_subnets" {
     source = "cloudposse/dynamic-subnets/aws"
     # Cloud Posse recommends pinning every module to a specific version
     # version = "x.x.x"
@@ -139,10 +143,19 @@ usage: |-
     name                = "app"
     vpc_id              = "vpc-XXXXXXXX"
     igw_id              = ["igw-XXXXXXXX"]
-    cidr_block          = "10.0.0.0/16"
+    ipv4_cidr_block     = "10.0.0.0/16"
     availability_zones  = ["us-east-1a", "us-east-1b"]
-    nat_gateway_enabled = true
-    nat_elastic_ips     = ["1.2.3.4", "1.2.3.5"]
+  
+    nat_gateway_enabled    = false
+    public_subnets_enabled = false
+  }
+  
+  resource "aws_route" "private" {
+    count = length(module.private_tgw_subnets.private_route_table_ids)
+
+    route_table_id         = module.private_tgw_subnets.private_route_table_ids[count.index]
+    destination_cidr_block = "0.0.0.0/0"
+    transit_gateway_id     = "tgw-XXXXXXXXX"
   }
   ```
 

docs/design.md

@@ -21,7 +21,7 @@ cidr_count = existing_az_count * subnet_type_count
 
 4. Calculate the number of bits needed to enumerate all the CIDRs:
 ```
-subnet_bits = ciel(log(cidr_count, 2))
+subnet_bits = ceil(log(cidr_count, 2))
 ```
 5. Reserve CIDRs for private subnets using [`cidrsubnet`](https://www.terraform.io/language/functions/cidrsubnet): 
 ```
@@ -36,4 +36,4 @@ public_subnet_cidrs = [ for netnumber in range(existing_az_count, existing_az_co
 Note that this means that, for example, in a region with 4 availability zones, if you specify only 3 availability zones 
 in `var.availability_zones`, this module will still reserve CIDRs for the 4th zone. This is so that if you later
 want to expand into that zone, the existing subnet CIDR assignments will not be disturbed. If you do not want
-to reserve these CIDRs
+to reserve these CIDRs, set `max_subnet_count` to the number of zones you are actually using.

docs/migration-v1-v2.md

@@ -1,7 +1,7 @@
 ## Migration Notes for Dynamic Subnets v2.0
 
 The first version of `terraform-aws-dynamic-subnets` was written for Terraform v0.9.1, 
-which was so limited that it do not even have a boolean data type, and 
+which was so limited that it did not even have a boolean data type, and 
 lists did not have first-class support. Cloud Posse continued to upgrade
 the module over time, but retained many of the awkward constructions required
 by those early Terraform versions.
@@ -34,8 +34,8 @@ rather than a `string` that could be empty or `null`.
 ### `cidr_block` replaced with `ipv4_cidr_block`
 
 Previously this module required an IPv4 CIDR block input as `cidr_block`.
-This value is now optional, and,  Since we had to make 
-a breaking change in type, we took the opportunity to reduce the ambiguity
+This value is now optional, and, since we had to make a breaking change in 
+its type anyway, we took the opportunity to reduce the ambiguity
 of `cidr_block` and renamed it `ipv4_cidr_block`.
 
 ***Migration***: Replace
@@ -69,7 +69,7 @@ igw_id = [aws_internet_gateway.default.id]
 ```
 
 Because the Internet Gateway ID is now optional, you
-can create a "public" set of subnets but not have the routed directly 
+can create a "public" set of subnets but not have them routed directly 
 to the internet. You could, instead, route them to a Transit Gateway, 
 VPC Endpoint, or other egress by adding your own entry into the route table.
 

docs/terraform.md

@@ -106,10 +106,10 @@
 | <a name="input_name"></a> [name](#input\_name) | ID element. Usually the component or solution name, e.g. 'app' or 'jenkins'.<br>This is the only ID element not also included as a `tag`.<br>The "name" tag is set to the full `id` string. There is no tag with the value of the `name` input. | `string` | `null` | no |
 | <a name="input_namespace"></a> [namespace](#input\_namespace) | ID element. Usually an abbreviation of your organization name, e.g. 'eg' or 'cp', to help ensure generated IDs are globally unique | `string` | `null` | no |
 | <a name="input_nat_elastic_ips"></a> [nat\_elastic\_ips](#input\_nat\_elastic\_ips) | Existing Elastic IPs (not EIP IDs) to attach to the NAT Gateway(s) or Instance(s) instead of creating new ones. | `list(string)` | `[]` | no |
-| <a name="input_nat_gateway_enabled"></a> [nat\_gateway\_enabled](#input\_nat\_gateway\_enabled) | Flag to enable/disable NAT Gateways to allow servers in the private subnets to access the Internet | `bool` | `true` | no |
+| <a name="input_nat_gateway_enabled"></a> [nat\_gateway\_enabled](#input\_nat\_gateway\_enabled) | Set `true` to create NAT Gateways to perform IPv4 NAT and NAT64 as needed.<br>Defaults to `true` unless `nat_instance_enabled` is `true`. | `bool` | `null` | no |
 | <a name="input_nat_instance_ami_id"></a> [nat\_instance\_ami\_id](#input\_nat\_instance\_ami\_id) | A list optionally containing the ID of the AMI to use for the NAT instance.<br>If the list is empty (the default), the latest official AWS NAT instance AMI<br>will be used. NOTE: The Official NAT instance AMI is being phased out and<br>does not support NAT64. Use of a NAT gateway is recommended instead. | `list(string)` | `[]` | no |
 | <a name="input_nat_instance_cpu_credits_override"></a> [nat\_instance\_cpu\_credits\_override](#input\_nat\_instance\_cpu\_credits\_override) | NAT Instance credit option for CPU usage. Valid values are "standard" or "unlimited".<br>T3 and later instances are launched as unlimited by default. T2 instances are launched as standard by default. | `string` | `""` | no |
-| <a name="input_nat_instance_enabled"></a> [nat\_instance\_enabled](#input\_nat\_instance\_enabled) | Flag to enable/disable NAT Instances to allow servers in the private subnets to access the Internet | `bool` | `false` | no |
+| <a name="input_nat_instance_enabled"></a> [nat\_instance\_enabled](#input\_nat\_instance\_enabled) | Set `true` to create NAT Instances to perform IPv4 NAT.<br>Defaults to `false`. | `bool` | `null` | no |
 | <a name="input_nat_instance_root_block_device_encrypted"></a> [nat\_instance\_root\_block\_device\_encrypted](#input\_nat\_instance\_root\_block\_device\_encrypted) | Whether to encrypt the root block device on the created NAT instances | `bool` | `true` | no |
 | <a name="input_nat_instance_type"></a> [nat\_instance\_type](#input\_nat\_instance\_type) | NAT Instance type | `string` | `"t3.micro"` | no |
 | <a name="input_open_network_acl_ipv4_rule_number"></a> [open\_network\_acl\_ipv4\_rule\_number](#input\_open\_network\_acl\_ipv4\_rule\_number) | The `rule_no` assigned to the network ACL rules for IPv4 traffic generated by this module | `number` | `100` | no |

examples/complete/outputs.tf

@@ -1,21 +1,26 @@
 output "public_subnet_cidrs" {
-  value = module.subnets.public_subnet_cidrs
+  description = "IPv4 CIDRs assigned to the created public subnets"
+  value       = module.subnets.public_subnet_cidrs
 }
 
 output "private_subnet_cidrs" {
-  value = module.subnets.private_subnet_cidrs
+  description = "IPv4 CIDRs assigned to the created private subnets"
+  value       = module.subnets.private_subnet_cidrs
 }
 
 output "public_subnet_ipv6_cidrs" {
-  value = module.subnets.public_subnet_ipv6_cidrs
+  description = "IPv6 CIDRs assigned to the created public subnets"
+  value       = module.subnets.public_subnet_ipv6_cidrs
 }
 
 output "private_subnet_ipv6_cidrs" {
-  value = module.subnets.private_subnet_ipv6_cidrs
+  description = "IPv6 CIDRs assigned to the created private subnets"
+  value       = module.subnets.private_subnet_ipv6_cidrs
 }
 
 output "vpc_ipv6_cidr" {
-  value = module.vpc.vpc_ipv6_cidr_block
+  description = "Default IPv6 CIDR of the VPC"
+  value       = module.vpc.vpc_ipv6_cidr_block
 }
 
 output "public_route_table_ids" {

main.tf

@@ -170,12 +170,19 @@ locals {
   nat_required = local.public_enabled && (local.private4_enabled || local.public_dns64_enabled)
   nat_count    = min(local.subnet_az_count, var.max_nats)
 
-  nat_gateway_enabled = local.nat_required && var.nat_gateway_enabled
   # An AWS NAT instance does not perform NAT64, and we choose not to try to support NAT64 via NAT instances at this time.
   # It does not make sense to create both a NAT Gateway and a NAT instance, since they perform the same function
   # and occupy the same slot in a network routing table. Rather than try to create both,
-  # we favor the more powerful NAT Gateway over the deprecated NAT Instance and make the former suppress the latter.
-  nat_instance_enabled = local.public_enabled && local.private4_enabled && var.nat_instance_enabled && !local.nat_gateway_enabled
+  # we favor the more powerful NAT Gateway over the deprecated NAT Instance.
+  # However, we do not want to force people to set `nat_gateway_enabled` to `false` to enable a NAT Instance,
+  # so if `nat_instance_enabled` is set to true, we set the default for `nat_gateway_enabled` to `false`.
+  nat_gateway_setting = var.nat_instance_enabled == true ? var.nat_gateway_enabled == true : !(
+    var.nat_gateway_enabled == false # not true or null
+  )
+  nat_instance_setting = local.nat_gateway_setting ? false : var.nat_instance_enabled == true # not false or null
+
+  nat_gateway_enabled  = local.nat_required && local.nat_gateway_setting
+  nat_instance_enabled = local.public_enabled && local.private4_enabled && local.nat_instance_setting
   nat_enabled          = local.nat_gateway_enabled || local.nat_instance_enabled
   need_nat_eips        = local.nat_enabled && length(var.nat_elastic_ips) == 0
   need_nat_eip_data    = local.nat_enabled && length(var.nat_elastic_ips) > 0
@@ -209,7 +216,7 @@ data "aws_eip" "nat" {
 
 
 resource "aws_eip" "default" {
-  count = local.need_nat_eips ? local.subnet_az_count : 0
+  count = local.need_nat_eips ? local.nat_count : 0
   vpc   = true
 
   tags = merge(

variables.tf

@@ -183,14 +183,20 @@ variable "availability_zone_attribute_style" {
 
 variable "nat_gateway_enabled" {
   type        = bool
-  description = "Flag to enable/disable NAT Gateways to allow servers in the private subnets to access the Internet"
-  default     = true
+  description = <<-EOT
+    Set `true` to create NAT Gateways to perform IPv4 NAT and NAT64 as needed.
+    Defaults to `true` unless `nat_instance_enabled` is `true`.
+    EOT
+  default     = null
 }
 
 variable "nat_instance_enabled" {
   type        = bool
-  description = "Flag to enable/disable NAT Instances to allow servers in the private subnets to access the Internet"
-  default     = false
+  description = <<-EOT
+    Set `true` to create NAT Instances to perform IPv4 NAT.
+    Defaults to `false`.
+    EOT
+  default     = null
 }
 
 variable "nat_elastic_ips" {