azurerm_kubernetes_cluster - support for the only_critical_addons_enabled property

[favoretti]

Fixes #9183

[tombuildsstuff]

hey @favoretti

Thanks for this PR / pushing those changes - taking a look through if we can fix up the minor comments then this otherwise LGTM 👍

Thanks!

[favoretti]

$ TF_ACC=1 go test -v ./azurerm/internal/services/containers -timeout=1000m -run=TestAccKubernetesCluster_criticalAddonsTaint
=== RUN   TestAccKubernetesCluster_criticalAddonsTaint
=== PAUSE TestAccKubernetesCluster_criticalAddonsTaint
=== CONT  TestAccKubernetesCluster_criticalAddonsTaint
--- PASS: TestAccKubernetesCluster_criticalAddonsTaint (691.63s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/containers	692.807s

[tombuildsstuff]

LGTM - thanks for pushing those changes @favoretti

[t3mi]

@tombuildsstuff sorry for chiming in but these changes aren't correct as API allows multiple soft taints + strict hard taint.

[tombuildsstuff]

@t3mi can you give a little more context? The default node pool and additional node pools have different constraints for these fields - so this is only affecting the Default Node Pool?

[t3mi]

I'm talking about default node pool as I haven't checked constraints for ordinary ones.
Relevant issues - AKS#1833, AKS#1484
Shortly speaking, API allows settings n number of soft taints (the ones with PreferNoSchedule effect) while only hard taint with key CriticalAddonsOnly are allowed. Also setting hardcoded taint with PreferNoShedule doesn't make sense as it won't prevent user pods to be scheduled on the default node pool as was described in primary use case for this.

[favoretti]

@t3mi If I remove PreferNoSchedule, from the taint, i.e. make it a hard taint - would this be acceptable for now? We can talk about other soft taints in a separate PR? :) Also, this PR is for default node pool only, hence has no effect on additional pools.

[t3mi]

@favoretti I'm not a decision maker here, just shared my concerns. From my point it doesn't make sense in introducing separate parameter just to remove it later for soft taints as its not needed. Soft taints are the ones with effect PreferNoSchedule, for hard taint effect is NoSchedule. Hard taint could be only with key CriticalAddonsOnly (value isn't checked), for soft taint there is no such restriction. Both of them could be combined and placed on a default node pool.

Validation from the API just for reference:
Placing custom taints on system pool is not supported(except 'CriticalAddonsOnly' taint or taint effect is 'PreferNoSchedule'). Please refer to https://aka.ms/aks/system-taints for detail"

[favoretti]

@t3mi Ok, I flipped this option to a hard taint for now, to allow for this to make it to the next release and for people to keep their clusters clean, let's talk about reintroducing general soft taints on default node pool in a separate change?

[favoretti]

$ TF_ACC=1 go test -v ./azurerm/internal/services/containers -timeout=1000m -run=TestAccKubernetesCluster_criticalAddonsTaint
=== RUN   TestAccKubernetesCluster_criticalAddonsTaint
=== PAUSE TestAccKubernetesCluster_criticalAddonsTaint
=== CONT  TestAccKubernetesCluster_criticalAddonsTaint
--- PASS: TestAccKubernetesCluster_criticalAddonsTaint (649.52s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/containers	650.759s

Acceptance test still passes after the last changes.

[dhirschfeld]

It seems this missed the boat for 2.46 :(

Since the CI is green, is there anything preventing this PR being merged so it can make it in the next release?

[luddskunk]

I'm also wondering the same. I was tracking it yesterday and it was there as green.

Could you please elaborate why it was moved to v.2.47.0 @tombuildsstuff ? Thanks!

[favoretti]

Mainly due to $reasons and the fact that acceptance tests take over 12 hours. I hope Tom will merge it early next week and it ships Thursday.

Edit: CI on GH doesn't run acceptance, it happens elsewhere.

[katbyte]

Thanks @favoretti - LGTM 👍

[ghost]

This has been released in version 2.47.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.47.0"
}
# ... other configuration ...

[Cyanopus]

@favoretti We tried internally with this version and the error persists.

"Error: expanding default_node_pool: The AKS API has removed support for tainting all nodes in the default node pool and it is no longer possible to configure this. To taint a node pool, create a separate one"

To change only_critical_addons_enabled requires cluster to be recreated. This means that minor version in azure provider are not safe at all.

[favoretti]

@Cyanopus node_labels and node_taints require nodepool to be recreated, so only_critical_addons_enabled is no different. Azure API doesn't support patching those, so unfortunately there's nothing I can do about a reroll, I'm sorry.

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!