Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
davidholsgrove opened this issue Sep 19, 2024 · 1 comment · Fixed by #2118
Labels
bug Something isn't working

Comments

@davidholsgrove
Copy link
Contributor

davidholsgrove commented Sep 19, 2024

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
@davidholsgrove davidholsgrove added the bug Something isn't working label Sep 19, 2024
@wyardley
Copy link
Contributor

wyardley commented Sep 25, 2024

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
       }
     }
   }

wyardley added a commit to wyardley/terraform-google-kubernetes-engine that referenced this issue Sep 26, 2024
While terraform-google-modules#1817 added autopilot support for adding tags to
`node_pool_auto_config` when `add_cluster_firewall_rules` is set to
`true`, the same change did not apply for standard (non-autopilot)
clusters with cluster level autoscaling (nodepool autoprovisioning) in
place,

Fixes terraform-google-modules#2104

Signed-off-by: William Yardley <[email protected]>
wyardley added a commit to wyardley/terraform-google-kubernetes-engine that referenced this issue Sep 26, 2024
While terraform-google-modules#1817 added autopilot support for adding tags to
`node_pool_auto_config` when `add_cluster_firewall_rules` is set to
`true`, the same change did not apply for standard (non-autopilot)
clusters with cluster level autoscaling (nodepool autoprovisioning) in
place,

Fixes terraform-google-modules#2104

Signed-off-by: William Yardley <[email protected]>
wyardley added a commit to wyardley/terraform-google-kubernetes-engine that referenced this issue Sep 26, 2024
While terraform-google-modules#1817 added autopilot support for adding tags to
`node_pool_auto_config` when `add_cluster_firewall_rules` is set to
`true`, the same change did not apply for standard (non-autopilot)
clusters with cluster level autoscaling (nodepool autoprovisioning) in
place,

Fixes terraform-google-modules#2104

Signed-off-by: William Yardley <[email protected]>
wyardley added a commit to wyardley/terraform-google-kubernetes-engine that referenced this issue Sep 27, 2024
While terraform-google-modules#1817 added autopilot support for adding tags to
`node_pool_auto_config` when `add_cluster_firewall_rules` is set to
`true`, the same change did not apply for standard (non-autopilot)
clusters with cluster level autoscaling (nodepool autoprovisioning) in
place,

Fixes terraform-google-modules#2104

Signed-off-by: William Yardley <[email protected]>
wyardley added a commit to wyardley/terraform-google-kubernetes-engine that referenced this issue Sep 27, 2024
While terraform-google-modules#1817 added autopilot support for adding tags to
`node_pool_auto_config` when `add_cluster_firewall_rules` is set to
`true`, the same change did not apply for standard (non-autopilot)
clusters with cluster level autoscaling (nodepool autoprovisioning) in
place,

Fixes terraform-google-modules#2104

Signed-off-by: William Yardley <[email protected]>
wyardley added a commit to wyardley/terraform-google-kubernetes-engine that referenced this issue Sep 27, 2024
While terraform-google-modules#1817 added autopilot support for adding tags to
`node_pool_auto_config` when `add_cluster_firewall_rules` is set to
`true`, the same change did not apply for standard (non-autopilot)
clusters with cluster level autoscaling (nodepool autoprovisioning) in
place,

Fixes terraform-google-modules#2104

Signed-off-by: William Yardley <[email protected]>
wyardley added a commit to wyardley/terraform-google-kubernetes-engine that referenced this issue Oct 9, 2024
While terraform-google-modules#1817 added autopilot support for adding tags to
`node_pool_auto_config` when `add_cluster_firewall_rules` is set to
`true`, the same change did not apply for standard (non-autopilot)
clusters with cluster level autoscaling (nodepool autoprovisioning) in
place,

Fixes terraform-google-modules#2104

Signed-off-by: William Yardley <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants