terraform-google-modules / terraform-google-kubernetes-engine

Configures opinionated GKE clusters
https://registry.terraform.io/modules/terraform-google-modules/kubernetes-engine/google
Apache License 2.0
1.15k stars 1.18k forks source link

GKE Standard Clusters with AutoProvisioned NodePools not adding the Firewall `target_tags` #2104

Closed davidholsgrove closed 1 month ago

davidholsgrove commented 1 month ago

TL;DR

The firewall rules created with target_tags = [local.cluster_network_tag] have an expectation that the nodepools will have this tag ("gke-${var.name}") applied.

This tag should be added to network_tags to ensure it is set in node_pool_auto_config for AutoProvisioned NodePools also.

Expected behavior

The generated tags used by the firewall rules should be added to the network_tags for autoprovisioned nodepools the same as manual nodepools

Observed behavior

Firewall rules for allowing admission webhook etc aren't applying to the autoprovisioned nodepools as targets

Terraform Configuration

module "gke" {
  source     = "terraform-google-modules/kubernetes-engine/google//modules/beta-private-cluster-update-variant"
  version    = "~> 32.0"
  project_id = local.project_id
  name       = var.gke_cluster_name

  add_cluster_firewall_rules = true
  firewall_inbound_ports     = ["8080", "8443", "9443", "15017"]

  #
  # `"egress-internet"` is a network-tag based on https://github.com/terraform-google-modules/terraform-example-foundation/blob/master/3-networks-dual-svpc/modules/base_shared_vpc/main.tf#L43
  #
  # manually adding the `"gke-${var.gke_cluster_name}"` is necessary to match the target_tag in the module firewall rules
  #
  network_tags = ["egress-internet", "gke-${var.gke_cluster_name}"]

  remove_default_node_pool = true
  cluster_autoscaling = {
    enabled             = true
    autoscaling_profile = "OPTIMIZE_UTILIZATION"
    min_cpu_cores       = 4
    max_cpu_cores       = 86
    min_memory_gb       = 16
    max_memory_gb       = 256
    disk_size           = 100
    disk_type           = "pd-standard"
    image_type          = "COS_CONTAINERD"
    gpu_resources       = []
    auto_repair         = true
    auto_upgrade        = false
    strategy            = "SURGE"
    max_surge           = 1
    max_unavailable     = 0
  }
 }
}

### Terraform Version

```sh
Terraform v1.9.3
on linux_amd64
+ provider registry.terraform.io/hashicorp/google v5.42.0
+ provider registry.terraform.io/hashicorp/google-beta v5.42.0
+ provider registry.terraform.io/hashicorp/http v3.4.4
+ provider registry.terraform.io/hashicorp/null v3.2.2
+ provider registry.terraform.io/hashicorp/random v3.6.2


### Additional information

The autoprovisioned nodepool also doesnt honour the Resource Manager Tags provided as `node_pools_resource_manager_tags` .

Issue https://github.com/hashicorp/terraform-provider-google/issues/16614 mentioned it was added but only for AutoPilot clusters. The linked documentation (https://cloud.google.com/kubernetes-engine/docs/how-to/tags-firewall-policies#attach-tags-standard) has a cli mechanism to do the same for Standard Clusters
wyardley commented 1 month ago

Seems like #1817 added this support in a narrower way for autopilot. High level, I think this plus an example based on your snippet could work, and I can open a draft PR, but may take a bit to get the tests working, and I'm not super familiar personally with the use case.

Guessing the fix is something like this?

diff --git a/autogen/main/cluster.tf.tmpl b/autogen/main/cluster.tf.tmpl
index 80200fe7..4223db7d 100644
--- a/autogen/main/cluster.tf.tmpl
+++ b/autogen/main/cluster.tf.tmpl
@@ -281,10 +281,10 @@ resource "google_container_cluster" "primary" {

 {% if autopilot_cluster != true %}
   dynamic "node_pool_auto_config" {
-    for_each = var.cluster_autoscaling.enabled && length(var.network_tags) > 0 ? [1] : []
+    for_each = var.cluster_autoscaling.enabled && (length(var.network_tags) > 0 || var.add_cluster_firewall_rules) ? [1] : []
     content {
       network_tags {
-        tags = var.network_tags
+        tags = var.add_cluster_firewall_rules ? (concat(var.network_tags, [local.cluster_network_tag])) : var.network_tags
       }
     }
   }