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

identity_namespace output is "enabled" instead of [project_id].svc.id.goog for beta-private-cluster #489

Closed
schostin opened this issue Apr 15, 2020 · 4 comments · Fixed by #500
Labels
bug Something isn't working good first issue Good for newcomers P2 high priority issues triaged Scoped and ready for work

Comments

@schostin
Copy link
Contributor

We are using module version 8.1.0 and when I create a beta-private-cluster with the default identity_namespace = "enabled" the output value of identity_namespace is also "enabled". I would expect [project_id].svc.id.goog as output here. Any opinions on this? Happy to issue a PR if needed :)

@morgante
Copy link
Contributor

Yeah looks like a bug in the output. Just need to update it to point to the actual value from the cluster.

@morgante morgante added bug Something isn't working good first issue Good for newcomers P2 high priority issues triaged Scoped and ready for work labels Apr 15, 2020
@schostin
Copy link
Contributor Author

I was just able to get all tests working on my machine and would love to fix this. So therefore a question popped up:

  • What is the rational of having the workload identity configuration block as dynamic. Currently there is only one workload identity namespace supported and therefore the module could just reflect this. If the workload identity namespace is a dynamic based on a list, the output of identity_namespace should therefore be a list as well?

Any opinions on this? Imho there are 2 possibilities:

  1. Having workload_identity_config as a dynamic as it is now. Change the inputs / outputs to be a list as well to match this.
  2. Having workload_identity_config be a single "resource" and remove the dynamic part, only a small change on the output side would be needed then.

I would favor version 2 here since it is not clear, when and if multiple workload identity namespace will come.

@morgante
Copy link
Contributor

It's dynamic because workload identity can be disabled. In such cases we omit the block entirely. The block should be kept as-is (dynamic).

We shouldn't change the output much either. There's no need to make it a list.

This should work fine:

output "identity_namespace" {
  description = "Workload Identity namespace"
  value       = length(local.cluster_workload_identity_config) > 0 ? local.local.cluster_workload_identity_config[0].identity_namespace : null
  depends_on = [
    google_container_cluster.primary
  ]
}

@schostin
Copy link
Contributor Author

Fine with me. Will adjust the output tomorrow and try to add a test for it. Will issue the PR once it's ready and I've played around with tests a bit.

morgante pushed a commit that referenced this issue May 4, 2020
* Fixes #489 Identity namespace output for beta clusters

The identity namespace flag was "enabled". Changed the output value to reference the actual identity namespace of the cluster / the project.

* Fixed tests by re-building the module
CPL-markus pushed a commit to WALTER-GROUP/terraform-google-kubernetes-engine that referenced this issue Jul 15, 2024
…oogle-modules#500)

* Fixes terraform-google-modules#489 Identity namespace output for beta clusters

The identity namespace flag was "enabled". Changed the output value to reference the actual identity namespace of the cluster / the project.

* Fixed tests by re-building the module
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers P2 high priority issues triaged Scoped and ready for work
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants