feat: add cluster_telemetry to beta submodules

[nguyenhoaibao]

Closes #645

[comment-bot-dev]

Thanks for the PR! 🚀
✅ Lint checks have passed.

[morgante]

CI is returning this error:

       Error: expected cluster_telemetry.0.type to be one of [DISABLED ENABLED SYSTEM_ONLY], got ENABLE
       
         on ../../../modules/beta-public-cluster/cluster.tf line 60, in resource "google_container_cluster" "primary":
         60:     type = var.cluster_telemetry_type
       
       
       
       Error: ConflictsWith
       
         on ../../../modules/beta-public-cluster/cluster.tf line 62, in resource "google_container_cluster" "primary":
         62:   logging_service    = var.logging_service
       
       "logging_service": conflicts with cluster_telemetry
       
       
       Error: ConflictsWith
       
         on ../../../modules/beta-public-cluster/cluster.tf line 63, in resource "google_container_cluster" "primary":
         63:   monitoring_service = var.monitoring_service
       
       "monitoring_service": conflicts with cluster_telemetry

[bharathkkb]

@nguyenhoaibao Looks like the provider docs needs to be updated, supported values are ENABLED, DISABLED, SYSTEM_ONLY
GoogleCloudPlatform/magic-modules#3853

[nguyenhoaibao]

@morgante @bharathkkb I fixed the cluster_telemetry supported values. Can you guys take a look?

[bharathkkb]

We may want to keep logging_service and monitoring_service to keep supporting 1.14 clusters as the GKE API also supports these concurrently.

[nguyenhoaibao]

Currently cluster_telemetry and logging_service/monitoring_service can't be set at the same time, see this and this. Also from the output of the failed CI above, when I set both cluster_telemetry and logging_service/monitoring_service:

Error: ConflictsWith
       
         on ../../../modules/beta-public-cluster/cluster.tf line 62, in resource "google_container_cluster" "primary":
         62:   logging_service    = var.logging_service
       
       "logging_service": conflicts with cluster_telemetry
       
       
       Error: ConflictsWith
       
         on ../../../modules/beta-public-cluster/cluster.tf line 63, in resource "google_container_cluster" "primary":
         63:   monitoring_service = var.monitoring_service
       
       "monitoring_service": conflicts with cluster_telemetry

[morgante]

Right, but the conditional should probably be based in the Terraform not the templating layer.

[nguyenhoaibao]

Can you explain more? How can I use conditional in resource block? Is something like this ok?

locals {
  cluster_telemetry_type_is_set = var.cluster_telemetry_type != "" ? [1] : [0]
}

{% if beta_cluster %}
  dynamic "cluster_telemetry" {
    for_each = local.cluster_telemetry_type_is_set
    content {
      type = var.cluster_telemetry_type
    }
  }
{% else %}
  dynamic "cluster_telemetry" {
    for_each = length(local.cluster_telemetry_type_is_set) > 0 ? [] : [1]
    content {
      type = var.cluster_telemetry_type
    }
  }
  logging_service    = var.logging_service
  monitoring_service = var.monitoring_service
{% endif %}

[morgante]

I was thinking something more like this:

locals {
  cluster_telemetry_type_is_set = var.cluster_telemetry_type != ""
}

dynamic "cluster_telemetry" {
    for_each = local.cluster_telemetry_type_is_set > 0 ? [] : [1]
    content {
      type = var.cluster_telemetry_type
    }
  }
  logging_service    = local.cluster_telemetry_type_is_set ? null : var.logging_service
  monitoring_service =  local.cluster_telemetry_type_is_set ? null : var.monitoring_service
}

[morgante]

It looks like an unrelated ephemeral error, re-running.

[bharathkkb]

There might be an edge case where for 1.14 clusters you can use legacy logging_service="logging.googleapis.com" and this change will try to change to cluster_telemetry="ENABLED" since we set cluster_telemetry_type to ENABLED by default. However, this should be okay as it will be caught by the API and an error will be thrown.

[morgante]

Hmm looks like we're getting an error on the sandbox-enabled-local example:

       
       Error: Required attribute is not set
       
         on ../../../modules/beta-public-cluster/cluster.tf line 22, in resource "google_container_cluster" "primary":
         22: resource "google_container_cluster" "primary" {
       
       
       
       Error: ConflictsWith
       
         on ../../../modules/beta-public-cluster/cluster.tf line 65, in resource "google_container_cluster" "primary":
         65:   logging_service    = local.cluster_telemetry_type_is_set ? null : var.logging_service
       
       "logging_service": conflicts with cluster_telemetry
       
       
       Error: ConflictsWith
       
         on ../../../modules/beta-public-cluster/cluster.tf line 66, in resource "google_container_cluster" "primary":
         66:   monitoring_service = local.cluster_telemetry_type_is_set ? null : var.monitoring_service
       
       "monitoring_service": conflicts with cluster_telemetry

[nguyenhoaibao]

Hmm you have any idea to fix this?

[bharathkkb]

@nguyenhoaibao I think this should be

- for_each = local.cluster_telemetry_type_is_set ? [1] : [0]
+ for_each = local.cluster_telemetry_type_is_set ? [1] : []

[0] is still a list with 1 element which is causing the creation of the dynamic block.

[bharathkkb]

LGTM