Overhaul for IPv6 and flexiblity #159

Nuru · 2022-05-07T01:57:56Z

what

Full support for IPv6

why

Requested feature

references

Closes IPv6 Support #21
Resolves More flexible nat instance provision #73
Fixes Apply fails when changing an AZ #120
Supersedes and closes Feat/ipv6 #139
Supersedes and closes enable ipv6 #147
Fixes The network acl ids are required in the output for adding or removing security rules #148
Supersedes and closes Output Acls #151
Supersedes and closes Update to PR 139: Ipv6 support #152
Fixes Current module does not provide EIP IDs #153
Supersedes and closes PROPOSAL: Single NGW option #154

notes

Migration notes are here

Nuru · 2022-05-07T02:02:46Z

/test all

Nuru · 2022-05-07T02:26:03Z

/test all

public.tf

main.tf

nat-instance.tf

public.tf

nat-instance.tf

bridgecrew

⚠️ Due to 370c550 - Minor cleanups - 5 errors were fixed.

Change details

Error ID	Change	Path	Resource
BC_AWS_NETWORKING_53	Fixed	/public.tf	module.subnets.aws_subnet.public
BC_AWS_GENERAL_68	Fixed	/nat-instance.tf	module.subnets.aws_instance.nat_instance
BC_AWS_LOGGING_26	Fixed	/nat-instance.tf	module.subnets.aws_instance.nat_instance
BC_AWS_NETWORKING_53	Fixed	/public.tf	aws_subnet.public
BC_AWS_NETWORKING_48	Fixed	/main.tf	aws_eip.default

Nuru · 2022-05-07T02:43:31Z

/test all

Nuru · 2022-05-07T02:59:29Z

/test all

main.tf

private.tf

main.tf

Nuru · 2022-05-08T01:08:06Z

/test all

Nuru · 2022-05-12T18:07:09Z

/test all

mcalhoun

I made a few suggestions, but overall looks good!

mcalhoun · 2022-05-13T18:26:59Z

docs/design.md


-    5. Assign private subnets according to AZ number (we're using `count.index` for that).
-    6. Assign public subnets according to AZ number but with a shift according to the number of AZs in the region (see step 2)
+Note that this means that, for example, in a region with 4 availability zones, if you specify only 3 availability zones 


nit: consider eliminating the second that:

Note that this means, for example, ...

Eliminating optional/implied words in the fashion you suggest works for me, but it seems to be a problem for readers for whom English is not their native language.

mcalhoun · 2022-05-13T18:49:37Z

main.tf

+
+  use_az_ids      = local.e && length(var.availability_zone_ids) > 0
+  use_az_var      = local.e && length(var.availability_zones) > 0
+  use_default_azs = local.e && !(local.use_az_ids || local.use_az_var)


this appears to be unused

I am not concerned about having unused local variables. They serve as documentation and maintain parallel equivalence (ipv4 vs ipv6) making the code easier to understand and modify in the future.

In this particular case use_default_azs is not used because it is the only remaining choice in the conditional testing for the other 2 conditions.

mcalhoun · 2022-05-13T18:50:27Z

main.tf

+
+
+  subnet_az_count = local.e ? length(local.subnet_availability_zones) : 0
+  subnet_count    = ((local.public_enabled ? 1 : 0) + (local.private_enabled ? 1 : 0)) * local.subnet_az_count


this appears to be unused

I am not concerned about having unused local variables. They serve as documentation and maintain parallel equivalence (ipv4 vs ipv6) making the code easier to understand and modify in the future.

In this particular case, subnet_count is the total number of subnets created. The fact that it turns out we never need this number is not a concern to me.

mcalhoun · 2022-05-13T18:51:01Z

main.tf

+  # an IPv6 Egress-only Internet Gateway, not if it *requires* its use.
+  ipv6_egress_only_configured = local.ipv6_enabled && length(var.ipv6_egress_only_igw_id) > 0
+
+  public4_enabled  = local.public_enabled && local.ipv4_enabled


this appears to be unused

OK, I will use it

variables.tf

mcalhoun · 2022-05-13T19:07:29Z

test/src/go.mod

 	github.com/gruntwork-io/go-commons v0.8.0 // indirect
 	github.com/hashicorp/errwrap v1.0.0 // indirect
 	github.com/hashicorp/go-cleanhttp v0.5.2 // indirect
-	github.com/hashicorp/go-getter v1.5.9 // indirect
+	github.com/hashicorp/go-getter v1.5.11 // indirect
 	github.com/hashicorp/go-multierror v1.1.0 // indirect
 	github.com/hashicorp/go-safetemp v1.0.0 // indirect
 	github.com/hashicorp/go-version v1.3.0 // indirect


it looks like go.sum is out of sync with go.mod. To fix you can run:

$ go mod tidy -go=1.16 && go mod tidy -go=1.17

What makes you say they are out of sync? They look in sync to me?

terraform-aws-dynamic-subnets/test/src/go.mod

Line 35 in e584396

github.com/hashicorp/go-getter v1.5.11 // indirect

terraform-aws-dynamic-subnets/test/src/go.sum

Line 222 in e584396

github.com/hashicorp/go-getter v1.5.11 h1:wioTuNmaBU3IE9vdFtFMcmZWj0QzLc6DYaP6sNe5onY=

variables.tf

docs/migration-v1-v2.md

This Pull Request has been updated, so we're dismissing all reviews.

Nuru · 2022-05-13T23:09:22Z

/test all

Nuru · 2022-05-14T00:09:32Z

/test all

aknysh · 2022-05-14T22:17:40Z

examples/existing-ips/outputs.tf

@@ -1,7 +1,9 @@
 output "existing_ips" {
-  value = values(aws_eip.nat_ips).*.public_ip
+  description = "IP Addresses created by this module for use by NAT"


Suggested change

description = "IP Addresses created by this module for use by NAT"

description = "Elastic IP Addresses created by this module for use by NAT"

aknysh · 2022-05-14T22:22:42Z

outputs.tf

  value       = aws_route_table.private.*.id
 }

+output "public_network_acl" {


Suggested change

output "public_network_acl" {

output "public_network_acl_id" {

aknysh · 2022-05-14T22:22:53Z

outputs.tf

+  value       = local.public_open_network_acl_enabled ? aws_network_acl.public[0].id : null
+}
+
+output "private_network_acl" {


Suggested change

output "private_network_acl" {

output "private_network_acl_id" {

aknysh · 2022-05-14T22:24:40Z

outputs.tf

 output "nat_ips" {
-  description = "IP Addresses in use for NAT"
-  value       = coalescelist(aws_eip.default.*.public_ip, aws_eip.nat_instance.*.public_ip, data.aws_eip.nat_ips.*.public_ip, tolist([""]))
+  description = "IP Addresses in use by NAT"


Suggested change

description = "IP Addresses in use by NAT"

description = "Elastic IP Addresses in use by NAT"

aknysh · 2022-05-14T22:28:33Z

variables.tf

+variable "max_nats" {
+  type        = number
+  description = "Maximum number of NAT Gateways or NAT instances to create"
+  default     = 999


why 999? should it be equal to the number of subnets or AZs?

Really the default should be MAX_INT but Terraform doesn't provide such a value, so I used 999 as a "big enough" number that it will never take effect. It needs to be a value known at plan time, so it cannot be dependent on how many AZs are available.

aknysh · 2022-05-14T22:29:34Z

variables.tf

+variable "public_label" {
  type        = string
-  description = "Base CIDR block which will be divided into subnet CIDR blocks (e.g. `10.0.0.0/16`)"
+  description = "The string to use in IDs and elsewhere to distinguish resources for the private subnets from resources for the public subnets"


Suggested change

description = "The string to use in IDs and elsewhere to distinguish resources for the private subnets from resources for the public subnets"

description = "The string to use in IDs and elsewhere to distinguish resources for the public subnets from resources for the private subnets"

aknysh · 2022-05-14T22:29:54Z

variables.tf

-variable "availability_zones" {
+variable "ipv4_enabled" {
+  type        = bool
+  description = "Set true to enable IPv4 addresses in the subnets"


Suggested change

description = "Set true to enable IPv4 addresses in the subnets"

description = "Set `true` to enable IPv4 addresses in the subnets"

aknysh · 2022-05-14T22:30:03Z

variables.tf

+
+variable "ipv6_enabled" {
+  type        = bool
+  description = "Set true to enable IPv6 addresses in the subnets"


Suggested change

description = "Set true to enable IPv6 addresses in the subnets"

description = "Set `true` to enable IPv6 addresses in the subnets"

aknysh · 2022-05-14T22:37:41Z

variables.tf

+    If `true`, a single network ACL be created and it will be associated with every private subnet, and a rule (number 100)
+    will be created allowing all ingress and all egress. You can add additional rules to this network ACL
+    using the `aws_network_acl_rule` resource.
+    If `false`, you will will need to manage the network ACL external to this module.


Suggested change

If `false`, you will will need to manage the network ACL external to this module.

If `false`, you will need to manage the network ACL external to this module.

aknysh · 2022-05-14T22:37:55Z

variables.tf

+    If `true`, a single network ACL be created and it will be associated with every public subnet, and a rule
+    will be created allowing all ingress and all egress. You can add additional rules to this network ACL
+    using the `aws_network_acl_rule` resource.
+    If `false`, you will will need to manage the network ACL external to this module.


Suggested change

If `false`, you will will need to manage the network ACL external to this module.

If `false`, you will need to manage the network ACL external to this module.

aknysh · 2022-05-14T22:38:55Z

variables.tf

+variable "private_route_table_enabled" {
+  type        = bool
+  description = <<-EOT
+    If true, a network route table and default route to the NAT gateway, NAT instance, or egress-only gateway


Suggested change

If true, a network route table and default route to the NAT gateway, NAT instance, or egress-only gateway

If `true`, a network route table and default route to the NAT gateway, NAT instance, or egress-only gateway

aknysh · 2022-05-14T22:41:36Z

variables.tf

+    condition     = length(var.nat_instance_ami_id) < 2
+    error_message = "Only 1 NAT Instance AMI ID can be provided."
+  }
+


can remove this empty line

Suggested change

aknysh

please see comments

Nuru · 2022-05-15T00:19:33Z

/test all

Nuru added 2 commits April 28, 2022 16:44

Update testing framework

40dc06e

Overhaul for IPv6 and flexibility

00adbb7

Minor cleanups

370c550

Nuru force-pushed the ipv6 branch from 95b3a4e to 370c550 Compare May 7, 2022 02:25