Fix subnet configuration in cloud nat module (#2171)

* support optional secondary ranges in net-cloudnat module * fix subnet configuration * fix packer blueprint
GoogleCloudPlatform · Mar 22, 2024 · a590deb · a590deb
1 parent 1ee7494
commit a590deb
Show file tree

Hide file tree

Showing 5 changed files with 184 additions and 86 deletions.
diff --git a/blueprints/cloud-operations/packer-image-builder/main.tf b/blueprints/cloud-operations/packer-image-builder/main.tf
@@ -82,19 +82,19 @@ module "firewall" {
 }
 
 module "nat" {
-  source                = "../../../modules/net-cloudnat"
-  project_id            = module.project.project_id
-  region                = var.region
-  name                  = "default"
-  router_network        = module.vpc.name
-  config_source_subnets = "LIST_OF_SUBNETWORKS"
-  subnetworks = [
-    {
-      self_link            = module.vpc.subnet_self_links["${var.region}/${local.compute_subnet_name}"]
-      config_source_ranges = ["ALL_IP_RANGES"]
-      secondary_ranges     = null
-    }
-  ]
+  source         = "../../../modules/net-cloudnat"
+  project_id     = module.project.project_id
+  region         = var.region
+  name           = "default"
+  router_network = module.vpc.name
+  config_source_subnetworks = {
+    all = false
+    subnetworks = [
+      {
+        self_link = module.vpc.subnet_self_links["${var.region}/${local.compute_subnet_name}"]
+      }
+    ]
+  }
 }
 
 resource "google_service_account_iam_binding" "sa-image-builder-token-creators" {

diff --git a/modules/net-cloudnat/README.md b/modules/net-cloudnat/README.md
@@ -4,7 +4,7 @@ Simple Cloud NAT management, with optional router creation.
 
 <!-- BEGIN TOC -->
 - [Basic Example](#basic-example)
-- [Subnetwork ranges](#subnetwork-ranges)
+- [Subnetwork configuration](#subnetwork-configuration)
 - [Reserved IPs and custom rules](#reserved-ips-and-custom-rules)
 - [Variables](#variables)
 - [Outputs](#outputs)
@@ -23,11 +23,15 @@ module "nat" {
 # tftest modules=1 resources=2 e2e
 ```
 
-## Subnetwork ranges
+## Subnetwork configuration
 
-When specifying subnets the default for IP ranges is to consider all ranges (primary and secondaries).
+Subnetwork configuration is defined via the `config_source_subnetworks` variable:
 
-More control can be obtained via the `all_ip_ranges` subnetwork attribute: when set to `false` only the primary subnet range is considered, unless secondary ranges are specified via the `secondary_ranges` attribute.
+- the default is to configure all ranges for all subnets
+- to only configure primary ranges set `config_source_subnetworks.primary_ranges_only` to `true`
+- to specify a list of subnets set `config_source_subnetworks.all` to `false` and provide a list of subnets in `config_source_subnetworks.subnetworks`
+
+When specifying subnets the default for IP ranges is to consider all ranges (primary and secondaries). More control can be obtained via the `all` subnetwork attribute: when set to `false` only the primary subnet range is considered, unless secondary ranges are specified via the `secondary_ranges` attribute.
 
 ```hcl
 module "nat" {
@@ -36,23 +40,26 @@ module "nat" {
   region         = var.region
   name           = "default"
   router_network = var.vpc.self_link
-  subnetworks = [
-    {
-      # all ip ranges
-      self_link = "projects/foo/regions/europe-west1/subnetworks/net"
-    },
-    {
-      # primary range only
-      self_link     = "projects/foo/regions/europe-west3/subnetworks/net"
-      all_ip_ranges = false
-    },
-    {
-      # both primary and specified secondary ranges
-      self_link        = "projects/foo/regions/europe-west8/subnetworks/net"
-      all_ip_ranges    = false
-      secondary_ranges = ["pods"]
-    }
-  ]
+  config_source_subnetworks = {
+    all = false
+    subnetworks = [
+      {
+        # all ip ranges
+        self_link = "projects/${var.project_id}/regions/${var.region}/subnetworks/net-0"
+      },
+      {
+        # primary range only
+        self_link  = "projects/${var.project_id}/regions/${var.region}/subnetworks/net-1"
+        all_ranges = false
+      },
+      {
+        # both primary and specified secondary ranges
+        self_link        = "projects/${var.project_id}/regions/${var.region}/subnetworks/net-2"
+        all_ranges       = false
+        secondary_ranges = ["pods"]
+      }
+    ]
+  }
 }
 # tftest modules=1 resources=2
 ```
@@ -100,20 +107,19 @@ module "nat" {
 
 | name | description | type | required | default |
 |---|---|:---:|:---:|:---:|
-| [name](variables.tf#L64) | Name of the Cloud NAT resource. | <code>string</code> | ✓ |  |
-| [project_id](variables.tf#L69) | Project where resources will be created. | <code>string</code> | ✓ |  |
-| [region](variables.tf#L74) | Region where resources will be created. | <code>string</code> | ✓ |  |
+| [name](variables.tf#L77) | Name of the Cloud NAT resource. | <code>string</code> | ✓ |  |
+| [project_id](variables.tf#L82) | Project where resources will be created. | <code>string</code> | ✓ |  |
+| [region](variables.tf#L87) | Region where resources will be created. | <code>string</code> | ✓ |  |
 | [addresses](variables.tf#L17) | Optional list of external address self links. | <code>list&#40;string&#41;</code> |  | <code>&#91;&#93;</code> |
 | [config_port_allocation](variables.tf#L23) | Configuration for how to assign ports to virtual machines. min_ports_per_vm and max_ports_per_vm have no effect unless enable_dynamic_port_allocation is set to 'true'. | <code title="object&#40;&#123;&#10;  enable_endpoint_independent_mapping &#61; optional&#40;bool, true&#41;&#10;  enable_dynamic_port_allocation      &#61; optional&#40;bool, false&#41;&#10;  min_ports_per_vm                    &#61; optional&#40;number, 64&#41;&#10;  max_ports_per_vm                    &#61; optional&#40;number, 65536&#41;&#10;&#125;&#41;">object&#40;&#123;&#8230;&#125;&#41;</code> |  | <code>&#123;&#125;</code> |
-| [config_source_subnets](variables.tf#L39) | Subnetwork configuration (ALL_SUBNETWORKS_ALL_IP_RANGES, ALL_SUBNETWORKS_ALL_PRIMARY_IP_RANGES, LIST_OF_SUBNETWORKS). | <code>string</code> |  | <code>&#34;ALL_SUBNETWORKS_ALL_IP_RANGES&#34;</code> |
-| [config_timeouts](variables.tf#L45) | Timeout configurations. | <code title="object&#40;&#123;&#10;  icmp            &#61; optional&#40;number, 30&#41;&#10;  tcp_established &#61; optional&#40;number, 1200&#41;&#10;  tcp_time_wait   &#61; optional&#40;number, 120&#41;&#10;  tcp_transitory  &#61; optional&#40;number, 30&#41;&#10;  udp             &#61; optional&#40;number, 30&#41;&#10;&#125;&#41;">object&#40;&#123;&#8230;&#125;&#41;</code> |  | <code>&#123;&#125;</code> |
-| [logging_filter](variables.tf#L58) | Enables logging if not null, value is one of 'ERRORS_ONLY', 'TRANSLATIONS_ONLY', 'ALL'. | <code>string</code> |  | <code>null</code> |
-| [router_asn](variables.tf#L79) | Router ASN used for auto-created router. | <code>number</code> |  | <code>null</code> |
-| [router_create](variables.tf#L85) | Create router. | <code>bool</code> |  | <code>true</code> |
-| [router_name](variables.tf#L91) | Router name, leave blank if router will be created to use auto generated name. | <code>string</code> |  | <code>null</code> |
-| [router_network](variables.tf#L97) | Name of the VPC used for auto-created router. | <code>string</code> |  | <code>null</code> |
-| [rules](variables.tf#L103) | List of rules associated with this NAT. | <code title="list&#40;object&#40;&#123;&#10;  description &#61; optional&#40;string&#41;,&#10;  match       &#61; string&#10;  source_ips  &#61; list&#40;string&#41;&#10;&#125;&#41;&#41;">list&#40;object&#40;&#123;&#8230;&#125;&#41;&#41;</code> |  | <code>&#91;&#93;</code> |
-| [subnetworks](variables.tf#L114) | Subnetworks to NAT, only used when config_source_subnets equals LIST_OF_SUBNETWORKS. | <code title="list&#40;object&#40;&#123;&#10;  self_link     &#61; string&#10;  all_ip_ranges &#61; optional&#40;bool, true&#41;&#10;  secondary_ranges &#61; optional&#40;list&#40;string&#41;&#41;&#10;&#125;&#41;&#41;">list&#40;object&#40;&#123;&#8230;&#125;&#41;&#41;</code> |  | <code>&#91;&#93;</code> |
+| [config_source_subnetworks](variables.tf#L39) | Subnetwork configuration. | <code title="object&#40;&#123;&#10;  all                 &#61; optional&#40;bool, true&#41;&#10;  primary_ranges_only &#61; optional&#40;bool&#41;&#10;  subnetworks &#61; optional&#40;list&#40;object&#40;&#123;&#10;    self_link        &#61; string&#10;    all_ranges       &#61; optional&#40;bool, true&#41;&#10;    secondary_ranges &#61; optional&#40;list&#40;string&#41;&#41;&#10;  &#125;&#41;&#41;, &#91;&#93;&#41;&#10;&#125;&#41;">object&#40;&#123;&#8230;&#125;&#41;</code> |  | <code>&#123;&#125;</code> |
+| [config_timeouts](variables.tf#L58) | Timeout configurations. | <code title="object&#40;&#123;&#10;  icmp            &#61; optional&#40;number, 30&#41;&#10;  tcp_established &#61; optional&#40;number, 1200&#41;&#10;  tcp_time_wait   &#61; optional&#40;number, 120&#41;&#10;  tcp_transitory  &#61; optional&#40;number, 30&#41;&#10;  udp             &#61; optional&#40;number, 30&#41;&#10;&#125;&#41;">object&#40;&#123;&#8230;&#125;&#41;</code> |  | <code>&#123;&#125;</code> |
+| [logging_filter](variables.tf#L71) | Enables logging if not null, value is one of 'ERRORS_ONLY', 'TRANSLATIONS_ONLY', 'ALL'. | <code>string</code> |  | <code>null</code> |
+| [router_asn](variables.tf#L92) | Router ASN used for auto-created router. | <code>number</code> |  | <code>null</code> |
+| [router_create](variables.tf#L98) | Create router. | <code>bool</code> |  | <code>true</code> |
+| [router_name](variables.tf#L104) | Router name, leave blank if router will be created to use auto generated name. | <code>string</code> |  | <code>null</code> |
+| [router_network](variables.tf#L110) | Name of the VPC used for auto-created router. | <code>string</code> |  | <code>null</code> |
+| [rules](variables.tf#L116) | List of rules associated with this NAT. | <code title="list&#40;object&#40;&#123;&#10;  description &#61; optional&#40;string&#41;,&#10;  match       &#61; string&#10;  source_ips  &#61; list&#40;string&#41;&#10;&#125;&#41;&#41;">list&#40;object&#40;&#123;&#8230;&#125;&#41;&#41;</code> |  | <code>&#91;&#93;</code> |
 
 ## Outputs
 

diff --git a/modules/net-cloudnat/main.tf b/modules/net-cloudnat/main.tf
@@ -20,6 +20,15 @@ locals {
     ? try(google_compute_router.router[0].name, null)
     : var.router_name
   )
+  subnet_config = (
+    var.config_source_subnetworks.all != true
+    ? "LIST_OF_SUBNETWORKS"
+    : (
+      var.config_source_subnetworks.primary_ranges_only == true
+      ? "ALL_SUBNETWORKS_ALL_PRIMARY_IP_RANGES"
+      : "ALL_SUBNETWORKS_ALL_IP_RANGES"
+    )
+  )
 }
 
 resource "google_compute_router" "router" {
@@ -38,47 +47,62 @@ resource "google_compute_router" "router" {
 }
 
 resource "google_compute_router_nat" "nat" {
-  project                             = var.project_id
-  region                              = var.region
-  name                                = var.name
-  router                              = local.router_name
-  nat_ips                             = var.addresses
-  nat_ip_allocate_option              = length(var.addresses) > 0 ? "MANUAL_ONLY" : "AUTO_ONLY"
-  source_subnetwork_ip_ranges_to_nat  = var.config_source_subnets
-  icmp_idle_timeout_sec               = var.config_timeouts.icmp
-  udp_idle_timeout_sec                = var.config_timeouts.udp
-  tcp_established_idle_timeout_sec    = var.config_timeouts.tcp_established
-  tcp_time_wait_timeout_sec           = var.config_timeouts.tcp_time_wait
-  tcp_transitory_idle_timeout_sec     = var.config_timeouts.tcp_transitory
-  enable_endpoint_independent_mapping = var.config_port_allocation.enable_endpoint_independent_mapping
-  enable_dynamic_port_allocation      = var.config_port_allocation.enable_dynamic_port_allocation
-  min_ports_per_vm                    = var.config_port_allocation.min_ports_per_vm
-  max_ports_per_vm                    = var.config_port_allocation.max_ports_per_vm
+  project = var.project_id
+  region  = var.region
+  name    = var.name
+  router  = local.router_name
+  nat_ips = var.addresses
+  nat_ip_allocate_option = (
+    length(var.addresses) > 0 ? "MANUAL_ONLY" : "AUTO_ONLY"
+  )
+  source_subnetwork_ip_ranges_to_nat = local.subnet_config
+  icmp_idle_timeout_sec              = var.config_timeouts.icmp
+  udp_idle_timeout_sec               = var.config_timeouts.udp
+  tcp_established_idle_timeout_sec   = var.config_timeouts.tcp_established
+  tcp_time_wait_timeout_sec          = var.config_timeouts.tcp_time_wait
+  tcp_transitory_idle_timeout_sec    = var.config_timeouts.tcp_transitory
+  enable_endpoint_independent_mapping = (
+    var.config_port_allocation.enable_endpoint_independent_mapping
+  )
+  enable_dynamic_port_allocation = (
+    var.config_port_allocation.enable_dynamic_port_allocation
+  )
+  min_ports_per_vm = (
+    var.config_port_allocation.min_ports_per_vm
+  )
+  max_ports_per_vm = (
+    var.config_port_allocation.max_ports_per_vm
+  )
 
   log_config {
     enable = var.logging_filter == null ? false : true
     filter = var.logging_filter == null ? "ALL" : var.logging_filter
   }
 
   dynamic "subnetwork" {
-    for_each = var.subnetworks
-    iterator = subnet
+    for_each = toset(
+      local.subnet_config == "LIST_OF_SUBNETWORKS"
+      ? var.config_source_subnetworks.subnetworks
+      : []
+    )
     content {
-      name = subnet.value.self_link
+      name = subnetwork.value.self_link
       source_ip_ranges_to_nat = (
-        subnet.value.all_ip_ranges == true ? ["ALL_IP_RANGES"] : concat(
+        subnetwork.value.all_ranges == true
+        ? ["ALL_IP_RANGES"]
+        : concat(
           ["PRIMARY_IP_RANGE"],
           (
-            subnet.value.secondary_ranges == null
+            subnetwork.value.secondary_ranges == null
             ? []
             : ["LIST_OF_SECONDARY_IP_RANGES"]
           )
         )
       )
       secondary_ip_range_names = (
-        subnet.value.all_ip_ranges == true
+        subnetwork.value.all_ranges == true
         ? null
-        : subnet.value.secondary_ranges
+        : subnetwork.value.secondary_ranges
       )
     }
   }

diff --git a/modules/net-cloudnat/variables.tf b/modules/net-cloudnat/variables.tf
@@ -36,10 +36,23 @@ variable "config_port_allocation" {
   }
 }
 
-variable "config_source_subnets" {
-  description = "Subnetwork configuration (ALL_SUBNETWORKS_ALL_IP_RANGES, ALL_SUBNETWORKS_ALL_PRIMARY_IP_RANGES, LIST_OF_SUBNETWORKS)."
-  type        = string
-  default     = "ALL_SUBNETWORKS_ALL_IP_RANGES"
+variable "config_source_subnetworks" {
+  description = "Subnetwork configuration."
+  type = object({
+    all                 = optional(bool, true)
+    primary_ranges_only = optional(bool)
+    subnetworks = optional(list(object({
+      self_link        = string
+      all_ranges       = optional(bool, true)
+      secondary_ranges = optional(list(string))
+    })), [])
+  })
+  nullable = false
+  default  = {}
+}
+
+output "foo" {
+  value = var.config_source_subnetworks.subnetworks
 }
 
 variable "config_timeouts" {
@@ -110,16 +123,3 @@ variable "rules" {
   default  = []
   nullable = false
 }
-
-variable "subnetworks" {
-  description = "Subnetworks to NAT, only used when config_source_subnets equals LIST_OF_SUBNETWORKS."
-  type = list(object({
-    self_link     = string
-    all_ip_ranges = optional(bool, true)
-    # secondary ranges are only meaningful when all_ip_ranges is false
-    # in that case we populate config with LIST_OF_SECONDARY_IP_RANGES
-    # only if secondary_ranges is not null
-    secondary_ranges = optional(list(string))
-  }))
-  default = []
-}
diff --git a/tests/modules/net_cloudnat/examples/subnets.yaml b/tests/modules/net_cloudnat/examples/subnets.yaml
@@ -0,0 +1,68 @@
+# Copyright 2024 Google LLC
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+values:
+  module.nat.google_compute_router.router[0]:
+    bgp: []
+    description: null
+    encrypted_interconnect_router: null
+    name: default-nat
+    network: projects/xxx/global/networks/aaa
+    project: project-id
+    region: europe-west8
+    timeouts: null
+  module.nat.google_compute_router_nat.nat:
+    drain_nat_ips: null
+    enable_dynamic_port_allocation: false
+    enable_endpoint_independent_mapping: true
+    icmp_idle_timeout_sec: 30
+    log_config:
+    - enable: false
+      filter: ALL
+    max_ports_per_vm: 65536
+    min_ports_per_vm: 64
+    name: default
+    nat_ip_allocate_option: AUTO_ONLY
+    nat_ips: null
+    project: project-id
+    region: europe-west8
+    router: default-nat
+    rules: []
+    source_subnetwork_ip_ranges_to_nat: LIST_OF_SUBNETWORKS
+    subnetwork:
+    - name: projects/project-id/regions/europe-west8/subnetworks/net-0
+      secondary_ip_range_names: []
+      source_ip_ranges_to_nat:
+      - ALL_IP_RANGES
+    - name: projects/project-id/regions/europe-west8/subnetworks/net-1
+      secondary_ip_range_names: []
+      source_ip_ranges_to_nat:
+      - PRIMARY_IP_RANGE
+    - name: projects/project-id/regions/europe-west8/subnetworks/net-2
+      secondary_ip_range_names:
+      - pods
+      source_ip_ranges_to_nat:
+      - LIST_OF_SECONDARY_IP_RANGES
+      - PRIMARY_IP_RANGE
+    tcp_established_idle_timeout_sec: 1200
+    tcp_time_wait_timeout_sec: 120
+    tcp_transitory_idle_timeout_sec: 30
+    timeouts: null
+    udp_idle_timeout_sec: 30
+
+counts:
+  google_compute_router: 1
+  google_compute_router_nat: 1
+  modules: 1
+  resources: 2