fix(380): apply labels and taints to nodes in default node pool

[arkhoss]

This MR is an attempt to resolve #380 .

This is my first time, so be gentle! 😉

[alessandroargentieri]

The len(rawLabels) > 0 guard means that if a user removes all labels (goes from having labels to an empty map), newLabels stays nil, and the old labels from the API (copied at line 424) are never cleared. You should allow an empty map through so labels can be removed:

if rawLabels, ok := newPool["labels"].(map[string]interface{}); ok {
   newLabels = make(map[string]string, len(rawLabels))
   for k, v := range rawLabels {
        if strVal, ok := v.(string); ok {
             newLabels[k] = strVal
        }
   }
}

[arkhoss]

Hi @alessandroargentieri ,

removed the condition, and this should allow an empty map for labels to be removed.

		// Extract labels from new desired state
		var newLabels map[string]string
		if rawLabels, ok := newPool["labels"].(map[string]interface{}); ok {
			newLabels = make(map[string]string, len(rawLabels))
			for k, v := range rawLabels {
				if strVal, ok := v.(string); ok {
					newLabels[k] = strVal
				}
			}
		}

[alessandroargentieri]

Same issue — if a user removes all taints, newTaints stays nil, and old taints from line 424 are preserved. Should initialize to an empty slice when the key exists but the set is empty:

 if taintSet, ok := newPool["taint"].(*schema.Set); ok {
      newTaints = make([]corev1.Taint, 0, taintSet.Len())
      for _, taintInterface := range taintSet.List() {
           ...
      }
  }

[arkhoss]

Hi @alessandroargentieri , changes should allow the newTaints to stay nil, and old taints preserved.

[alessandroargentieri]

This updates labels/taints only when d.HasChange("pools") is true. But the outer if d.HasChange("pools") block is a coarse check — if only labels changed, does d.HasChange("pools") fire? It should, since labels and taint are nested inside the pools schema, but worth verifying with a test. If a user only changes labels without changing node_count, the diff must still trigger this block.

[alessandroargentieri]

Can you add a unit test for flattenNodePool and updateNodePool that covers:

• Creating a cluster with labels+taints on the default pool
• Verifying no drift on a second plan
• Updating labels/taints and verifying they change
• Removing labels/taints and verifying they're cleared

[arkhoss]

Hi @alessandroargentieri ,

I added the tests as requested, but sadly, I think I hit an error or panic from the API itself, and not sure how to debug it from here, leaving you a paste bin link with the output I got, and here is the command I used to run the tests:

$ CIVO_REGION=NYC1 CIVO_TOKEN=<token> TF_ACC=1 /usr/local/go/bin/go test ./civo/kubernetes -v -run TestAccCivoKubernetesCluster_labels_update

https://pastebin.com/SVjEWjEJ

not sure if all I added helps you, but I ran a bunch of tests with it, and I feel I'm close, but I lack the knowledge to see it through.

[arkhoss]

Hi @alessandroargentieri , thank you for such detailed review! I will do my best and try to fix all the comments soon, thanks again!

[alessandroargentieri]

@arkhoss we have some customers blocked on these issues? Are you planning to complete the PRs soon or can I take over them and solve to help our customers? Many thanks for any effort you're getting into this anyway 🙏🏼

[alessandroargentieri]

Thanks @arkhoss for tackling this, and welcome — first PR or not, this is a solid attempt and the direction is right. A few things I want to sort out before we merge, though.

two that are blocking ones for me:
1. clearing labels probably doesn't actually work. When the config has no labels, newLabels ends up as an empty (but non-nil) map. KubernetesClusterPoolUpdateConfig.Labels is json:"labels,omitempty", so an empty map gets dropped from the PUT body entirely — meaning that the API keeps the old labels despite we would like to clear them up. Then waitForPoolLabelsAndTaints sits in this branch forever:

if len(expectedLabels) == 0 && len(labels) > 0 { return cluster, "PENDING", nil }

…so the update just spins until TimeoutUpdate. That's exactly what your "Remove labels completely" acceptance step exercises, so I think it'd fail if the acc suite were actually run (it's TF_ACC-gated, so it almost certainly didn't run in CI). Can you double-check how the API actually clears labels and confirm that step goes green? Taints are fine btw — that field isn't omitempty.

2. updateNodePool is dead code now. After the refactor, nothing in the provider calls it anymore — the update path builds poolUpdate inline and calls UpdateKubernetesClusterPool directly. The only thing reaching it is ExportUpdateNodePool in the test. So we've widened its signature and written a test for a function we no longer use, which gives us false confidence. Either wire it back into the real flow or just delete it along with the export and TestAccCivoUpdateNodePool.

, one I'm less sure about but want to flag:
3. possible state drift. labels/taint are Optional only (not Computed), and flattenNodePool now writes everything the API returns back into state. If Civo/k3s injects its own managed labels (the kubernetes.civo.com/... kind) we'd get a permanent non-empty plan, and it'd also make #1 worse — len(labels) > 0 would always be true so clearing could never finish.

apert from that, smaller stuff, not blocking:

• The two unit tests are named TestAccCivo... but they're pure unit tests (no API, no TF_ACC). The TestAcc prefix is our convention for the real acceptance tests. Mind renaming to TestFlattenNodePool / TestUpdateNodePool?
• There's a leftover debug loop in TestAccCivoKubernetesCluster_labels_update — the fmt.Println("--- ATTRIBUTES ---") block. Drop that before merge.
• Tiny one: the CNI test config string picked up a trailing space (} -> } ). Accidental, just revert it.
• waitForClusterActive is basically a copy of the inline createStateConf in create — could share one helper, but I won't die on that hill.
  Thanks for all your effort, I love the approach, just need #1 and #2 sorted (and ideally an answer on #3) and we're good to go

[arkhoss]

Hi @alessandroargentieri ,

Thanks again for such detailed answer, here is a list of the things I did in the last commits:

• Define kubernetesClusterPoolUpdatePayload and updateKubernetesClusterPoolHelper in kubernetes.go to address the scenario when labels is empty.
• Filter out Civo-managed labels (kubernetes.civo.com/) in flattenNodePool in kubernetes.go.
• Filter out Civo-managed labels in waitForPoolLabelsAndTaints in resource_kubernetes_cluster.go.
• Update resourceKubernetesClusterCreate and resourceKubernetesClusterUpdate to use the new helper in resource_kubernetes_cluster.go.
• Filter out Civo-managed labels in resourceKubernetesClusterNodePoolRead in resource_kubernetes_cluster_nodepool.go.
• Update resourceKubernetesClusterNodePoolUpdate to use the new helper in resource_kubernetes_cluster_nodepool.go.
• Remove dead function updateNodePool from resource_kubernetes_cluster_nodepool.go.
• Clean up export_test.go.
• Clean up and rename unit tests in resource_kubernetes_cluster_nodepool_test.go.
• Clean up debug output and trailing space in resource_kubernetes_cluster_test.go.
• Read node pool labels/taints from RequiredPools as the primary source of truth in flattenNodePool.
• Check RequiredPools before Pools in waitForPoolLabelsAndTaints to avoid eventual consistency lag.
• Fix region mismatch in TestAccCivoKubernetesCluster_labels_update config by defining a dedicated config helper.

Prioritized checking RequiredPools in waitForPoolLabelsAndTaints and flattenNodePool to correctly capture and reconcile user-defined node pool labels/taints without hitting asynchronous API.

With all these changes, the tests are now passing, build, unit tests and acceptance test too:

$ CIVO_REGION=NYC1 TF_ACC=1 go test -v ./civo/kubernetes -run TestAccCivoKubernetesCluster_labels_update 
=== RUN   TestAccCivoKubernetesCluster_labels_update
--- PASS: TestAccCivoKubernetesCluster_labels_update (268.96s)
PASS
ok      github.com/civo/terraform-provider-civo/civo/kubernetes 268.972s