feat(safer-cluster): add create_service_account variable (#2138)

Author: jlubawy

autogen/safer-cluster/main.tf.tmpl

@@ -119,11 +119,16 @@ module "gke" {
   //   All applications should run with an identity defined via Workload Identity anyway.
   // - Use a service account passed as a parameter to the module, in case the user
   //   wants to maintain control of their service accounts.
-  create_service_account = var.compute_engine_service_account == "" ? true : false
   service_account        = var.compute_engine_service_account
   registry_project_ids   = var.registry_project_ids
   grant_registry_access  = var.grant_registry_access
 
+  // If create_service_account is explicitly set to false we short-circuit the
+  // compute_engine_service_account check to potentially avoid an error (see variables.tf documentation).
+  // Otherwise if true (the default), we check if compute_engine_service_account is set for backwards compatability
+  // before the create_service_account variable was added.
+  create_service_account = var.create_service_account == false ? var.create_service_account : (var.compute_engine_service_account == "" ? true : false)
+
   issue_client_certificate = false
 
   cluster_resource_labels = var.cluster_resource_labels
@@ -209,7 +214,7 @@ module "gke" {
   // Enabling vulnerability and audit for workloads
   workload_vulnerability_mode = var.workload_vulnerability_mode
   workload_config_audit_mode  = var.workload_config_audit_mode
-  
+
   // Enabling security posture
   security_posture_mode               = var.security_posture_mode
   security_posture_vulnerability_mode = var.security_posture_vulnerability_mode

autogen/safer-cluster/variables.tf.tmpl

@@ -411,10 +411,16 @@ variable "authenticator_security_group" {
 
 variable "compute_engine_service_account" {
   type        = string
-  description = "Use the given service account for nodes rather than creating a new dedicated service account."
+  description = "Use the given service account for nodes rather than creating a new dedicated service account. If set then also set var.create_service_account to false to avoid 'value depends on resource attributes that cannot be determined until apply' errors."
   default     = ""
 }
 
+variable "create_service_account" {
+  type        = bool
+  description = "Defines if service account specified to run nodes should be created. Explicitly set to false if var.compute_engine_service_account is set to avoid 'value depends on resource attributes that cannot be determined until apply' errors."
+  default     = true
+}
+
 variable "enable_shielded_nodes" {
   type        = bool
   description = "Enable Shielded Nodes features on all nodes in this cluster."

docs/upgrading_to_v34.0.md

@@ -0,0 +1,54 @@
+# Upgrading to v34.0
+
+The v34.0 release of _kubernetes-engine_ is a backwards incompatible release.
+
+### safer-cluster modules: Added create_service_account variable
+
+This only affects users of the `safer-cluster` modules that have set `var.compute_engine_service_account` to something other than the default `""`.
+
+A variable `var.create_service_account` was added to the `safer-cluster` modules that when explicitly set to `false` avoids the following error withing the `private-cluster` modules:
+
+```sh
+Error: Invalid count argument
+
+  on .terraform/modules/gke_cluster.gke/modules/beta-private-cluster/sa.tf line 35, in resource "random_string" "cluster_service_account_suffix":
+  35:   count   = var.create_service_account && var.service_account_name == "" ? 1 : 0
+
+The "count" value depends on resource attributes that cannot be determined
+until apply, so Terraform cannot predict how many instances will be created.
+To work around this, use the -target argument to first apply only the
+resources that the count depends on.
+```
+
+This seems to happen if `var.compute_engine_service_account` is passed in, and the externally created service account is being created at the same time, so the name/email is not computed yet:
+
+```terraform
+resource "google_service_account" "cluster_service_account" {
+  project      = var.project_id
+  account_id   = "tf-gke-${var.cluster_name}-${random_string.cluster_service_account_suffix.result}"
+  display_name = "Terraform-managed service account for cluster ${var.cluster_name}"
+}
+
+module "gke" {
+  source  = "terraform-google-modules/kubernetes-engine/google//modules/safer-cluster"
+  version = "~> 33.0"
+
+  project_id  = var.project_id
+  name        = var.cluster_name
+
+  create_service_account         = false
+  compute_engine_service_account = google_service_account.cluster_service_account.email
+}
+```
+
+By explicitly passing a `var.create_service_account = false` it short circuits the calculations dependent on `var.service_account_name`:
+
+```terraform
+resource "random_string" "cluster_service_account_suffix" {
+  count   = var.create_service_account && var.service_account_name == "" ? 1 : 0
+  upper   = false
+  lower   = true
+  special = false
+  length  = 4
+}
+```

modules/safer-cluster-update-variant/README.md

@@ -209,8 +209,9 @@ For simplicity, we suggest using `roles/container.admin` and
 | cluster\_dns\_provider | Which in-cluster DNS provider should be used. PROVIDER\_UNSPECIFIED (default) or PLATFORM\_DEFAULT or CLOUD\_DNS. | `string` | `"PROVIDER_UNSPECIFIED"` | no |
 | cluster\_dns\_scope | The scope of access to cluster DNS records. DNS\_SCOPE\_UNSPECIFIED (default) or CLUSTER\_SCOPE or VPC\_SCOPE. | `string` | `"DNS_SCOPE_UNSPECIFIED"` | no |
 | cluster\_resource\_labels | The GCE resource labels (a map of key/value pairs) to be applied to the cluster | `map(string)` | `{}` | no |
-| compute\_engine\_service\_account | Use the given service account for nodes rather than creating a new dedicated service account. | `string` | `""` | no |
+| compute\_engine\_service\_account | Use the given service account for nodes rather than creating a new dedicated service account. If set then also set var.create\_service\_account to false to avoid 'value depends on resource attributes that cannot be determined until apply' errors. | `string` | `""` | no |
 | config\_connector | Whether ConfigConnector is enabled for this cluster. | `bool` | `false` | no |
+| create\_service\_account | Defines if service account specified to run nodes should be created. Explicitly set to false if var.compute\_engine\_service\_account is set to avoid 'value depends on resource attributes that cannot be determined until apply' errors. | `bool` | `true` | no |
 | database\_encryption | Application-layer Secrets Encryption settings. The object format is {state = string, key\_name = string}. Valid values of state are: "ENCRYPTED"; "DECRYPTED". key\_name is the name of a CloudKMS key. | `list(object({ state = string, key_name = string }))` | <pre>[<br>  {<br>    "key_name": "",<br>    "state": "DECRYPTED"<br>  }<br>]</pre> | no |
 | datapath\_provider | The desired datapath provider for this cluster. By default, `ADVANCED_DATAPATH` enables Dataplane-V2 feature. `DATAPATH_PROVIDER_UNSPECIFIED` enables the IPTables-based kube-proxy implementation as a fallback since upgrading to V2 requires a cluster re-creation. | `string` | `"ADVANCED_DATAPATH"` | no |
 | default\_max\_pods\_per\_node | The maximum number of pods to schedule per node | `number` | `110` | no |

modules/safer-cluster-update-variant/main.tf

@@ -115,10 +115,15 @@ module "gke" {
   //   All applications should run with an identity defined via Workload Identity anyway.
   // - Use a service account passed as a parameter to the module, in case the user
   //   wants to maintain control of their service accounts.
-  create_service_account = var.compute_engine_service_account == "" ? true : false
-  service_account        = var.compute_engine_service_account
-  registry_project_ids   = var.registry_project_ids
-  grant_registry_access  = var.grant_registry_access
+  service_account       = var.compute_engine_service_account
+  registry_project_ids  = var.registry_project_ids
+  grant_registry_access = var.grant_registry_access
+
+  // If create_service_account is explicitly set to false we short-circuit the
+  // compute_engine_service_account check to potentially avoid an error (see variables.tf documentation).
+  // Otherwise if true (the default), we check if compute_engine_service_account is set for backwards compatability
+  // before the create_service_account variable was added.
+  create_service_account = var.create_service_account == false ? var.create_service_account : (var.compute_engine_service_account == "" ? true : false)
 
   issue_client_certificate = false
 

modules/safer-cluster-update-variant/variables.tf

@@ -411,10 +411,16 @@ variable "authenticator_security_group" {
 
 variable "compute_engine_service_account" {
   type        = string
-  description = "Use the given service account for nodes rather than creating a new dedicated service account."
+  description = "Use the given service account for nodes rather than creating a new dedicated service account. If set then also set var.create_service_account to false to avoid 'value depends on resource attributes that cannot be determined until apply' errors."
   default     = ""
 }
 
+variable "create_service_account" {
+  type        = bool
+  description = "Defines if service account specified to run nodes should be created. Explicitly set to false if var.compute_engine_service_account is set to avoid 'value depends on resource attributes that cannot be determined until apply' errors."
+  default     = true
+}
+
 variable "enable_shielded_nodes" {
   type        = bool
   description = "Enable Shielded Nodes features on all nodes in this cluster."

modules/safer-cluster/README.md

@@ -209,8 +209,9 @@ For simplicity, we suggest using `roles/container.admin` and
 | cluster\_dns\_provider | Which in-cluster DNS provider should be used. PROVIDER\_UNSPECIFIED (default) or PLATFORM\_DEFAULT or CLOUD\_DNS. | `string` | `"PROVIDER_UNSPECIFIED"` | no |
 | cluster\_dns\_scope | The scope of access to cluster DNS records. DNS\_SCOPE\_UNSPECIFIED (default) or CLUSTER\_SCOPE or VPC\_SCOPE. | `string` | `"DNS_SCOPE_UNSPECIFIED"` | no |
 | cluster\_resource\_labels | The GCE resource labels (a map of key/value pairs) to be applied to the cluster | `map(string)` | `{}` | no |
-| compute\_engine\_service\_account | Use the given service account for nodes rather than creating a new dedicated service account. | `string` | `""` | no |
+| compute\_engine\_service\_account | Use the given service account for nodes rather than creating a new dedicated service account. If set then also set var.create\_service\_account to false to avoid 'value depends on resource attributes that cannot be determined until apply' errors. | `string` | `""` | no |
 | config\_connector | Whether ConfigConnector is enabled for this cluster. | `bool` | `false` | no |
+| create\_service\_account | Defines if service account specified to run nodes should be created. Explicitly set to false if var.compute\_engine\_service\_account is set to avoid 'value depends on resource attributes that cannot be determined until apply' errors. | `bool` | `true` | no |
 | database\_encryption | Application-layer Secrets Encryption settings. The object format is {state = string, key\_name = string}. Valid values of state are: "ENCRYPTED"; "DECRYPTED". key\_name is the name of a CloudKMS key. | `list(object({ state = string, key_name = string }))` | <pre>[<br>  {<br>    "key_name": "",<br>    "state": "DECRYPTED"<br>  }<br>]</pre> | no |
 | datapath\_provider | The desired datapath provider for this cluster. By default, `ADVANCED_DATAPATH` enables Dataplane-V2 feature. `DATAPATH_PROVIDER_UNSPECIFIED` enables the IPTables-based kube-proxy implementation as a fallback since upgrading to V2 requires a cluster re-creation. | `string` | `"ADVANCED_DATAPATH"` | no |
 | default\_max\_pods\_per\_node | The maximum number of pods to schedule per node | `number` | `110` | no |

modules/safer-cluster/main.tf

@@ -115,10 +115,15 @@ module "gke" {
   //   All applications should run with an identity defined via Workload Identity anyway.
   // - Use a service account passed as a parameter to the module, in case the user
   //   wants to maintain control of their service accounts.
-  create_service_account = var.compute_engine_service_account == "" ? true : false
-  service_account        = var.compute_engine_service_account
-  registry_project_ids   = var.registry_project_ids
-  grant_registry_access  = var.grant_registry_access
+  service_account       = var.compute_engine_service_account
+  registry_project_ids  = var.registry_project_ids
+  grant_registry_access = var.grant_registry_access
+
+  // If create_service_account is explicitly set to false we short-circuit the
+  // compute_engine_service_account check to potentially avoid an error (see variables.tf documentation).
+  // Otherwise if true (the default), we check if compute_engine_service_account is set for backwards compatability
+  // before the create_service_account variable was added.
+  create_service_account = var.create_service_account == false ? var.create_service_account : (var.compute_engine_service_account == "" ? true : false)
 
   issue_client_certificate = false
 

modules/safer-cluster/variables.tf

@@ -411,10 +411,16 @@ variable "authenticator_security_group" {
 
 variable "compute_engine_service_account" {
   type        = string
-  description = "Use the given service account for nodes rather than creating a new dedicated service account."
+  description = "Use the given service account for nodes rather than creating a new dedicated service account. If set then also set var.create_service_account to false to avoid 'value depends on resource attributes that cannot be determined until apply' errors."
   default     = ""
 }
 
+variable "create_service_account" {
+  type        = bool
+  description = "Defines if service account specified to run nodes should be created. Explicitly set to false if var.compute_engine_service_account is set to avoid 'value depends on resource attributes that cannot be determined until apply' errors."
+  default     = true
+}
+
 variable "enable_shielded_nodes" {
   type        = bool
   description = "Enable Shielded Nodes features on all nodes in this cluster."