feat(safer-cluster): add create_service_account variable

[jlubawy]

Adds a var.create_service_account to the safer-cluster modules that when explicitly set to false avoids the following error:

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 service account is being created at the same time (so the name/email is not computed yet):

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:

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
}

Happy to make any changes to help get this merged, it worked for my use-case. This also seemed like the easiest change to make while keeping backwards compatibility, but I'm open to other approaches.

[apeabody]

/gcbrun

[apeabody]

/gcbrun

[jlubawy]

Thanks for the contribution @jlubawy!

This might be considered a breaking change, regardless can you please add an upgrade similar to the examples at to example how/when this should be used: https://github.com/terraform-google-modules/terraform-google-kubernetes-engine/blob/master/docs/upgrading_to_v33.0.md

Hi @apeabody, I added a docs/upgrading_to_v34.0.md describing the change. For what it's worth I think it is backwards compatible the way I wrote it.

I also added some additional comments around the new variable to explain the reasoning and rebased again on master just now.

[apeabody]

/gcbrun

[apeabody]

Thanks for the contribution @jlubawy! This might be considered a breaking change, regardless can you please add an upgrade similar to the examples at to example how/when this should be used: https://github.com/terraform-google-modules/terraform-google-kubernetes-engine/blob/master/docs/upgrading_to_v33.0.md

Hi @apeabody, I added a docs/upgrading_to_v34.0.md describing the change. For what it's worth I think it is backwards compatible the way I wrote it.

I also added some additional comments around the new variable to explain the reasoning and rebased again on master just now.

Thanks @jlubawy! Agreed, it should be backwards compatible for existing users. The doc will help explain when users may want to use it.

[apeabody]

/gcbrun

[apeabody]

/gcbrun