r/kubernetes_cluster: handling breaking changes in the AKS API

[wagnst]

BREAKING CHANGES:

• Azure Kubernetes Service
  • Due to a breaking change in the AKS API, the azurerm_kubernetes_cluster resource features a significant behavioural change where creating Mixed-Mode Authentication clusters (e.g. using a Service Principal with a Managed Identity) is no longer supported.
  • The AKS Team have confirmed that existing clusters will be updated by the Azure API to use only MSI when a change is made to the Cluster (but not the Node Pool). Whilst Terraform could perform this automatically some environments have restrictions on which tags can be added/removed - as such this operation will need to be performed out-of-band. Instead, upon detecting a Mixed-Mode Cluster which has not yet been updated - or upon detecting a former Mixed-Mode Cluster where the Terraform Configuration still contains a service_principal block - Terraform will output instructions on how to proceed.
  • azurerm_kubernetes_cluster_node_pool - clusters with auto-scale disabled must ensure that min_count and max_count are set to null (or omitted) rather than 0 (since 0 isn't a valid value for these fields).

NOTES:

• There's currently a bug in the Azure Kubernetes Service (AKS) API where the Tags on Node Pools are returned in the incorrect case - this bug is being tracked in this issue. This affects the tags field within the default_node_pool block for azurerm_kubernetes_clusters and the tags field for the azurerm_kubernetes_cluster_node_pool resource.
  IMPROVEMENTS:

• dependencies: updating to use version 2020-02-01 of the Containers API [r/kubernetes_cluster: handling breaking changes in the AKS API #6095]

• azurerm_kubernetes_cluster - making the service_principal block optional - so it's now possible to create MSI-only clusters [r/kubernetes_cluster: handling breaking changes in the AKS API #6095]

• azurerm_kubernetes_cluster - making the windows_profile block computed as Windows credentials are now generated by Azure if unspecified [r/kubernetes_cluster: handling breaking changes in the AKS API #6095]

BUG FIXES:

• azurerm_kubernetes_cluster - requiring that min_count and max_count are set to null rather than 0 when auto-scaling is disabled [r/kubernetes_cluster: handling breaking changes in the AKS API #6095]
• azurerm_kubernetes_cluster - ensuring that a value for node_count is always passed to the API to match a requirement in the API [r/kubernetes_cluster: handling breaking changes in the AKS API #6095]
• azurerm_kubernetes_cluster - ensuring that tags are set into the state for the default_node_pool [r/kubernetes_cluster: handling breaking changes in the AKS API #6095]

Fixes #6094
Fixes #6235
Fixes #6257
Fixes #6178

[WodansSon]

@wagnst, hey... this looks mostly good to me, but I would like to push some changes to your repo as a lot of the logic in the expand function no longer makes any sense now that the API version has changed making node_count a required field. I would also like to add a note to the documentation and add a couple of test cases if you don't mind.

[katbyte]

Aside from updating the docs with the new min this LGTM 👍

[aristosvo]

For the MSI it would make sense to also expose the identity profile with kubelet identity, as exposed from the API:

"identityProfile": {
    "kubeletidentity": {
      "clientId": "00000000000000000000000000000000",
      "objectId": "0000000000000000000000000000000",
      "resourceId": "/subscriptions/00000000000000000000000/resourcegroups/MC_xxxxxxxx_westeurope/providers/Microsoft.ManagedIdentity/userAssignedIdentities/xxxxxx-agentpool"
    }
  },

Do you prefer a separate issue and pull request or could it be included here?

[tombuildsstuff]

hey @wagnst

Firstly - thanks for this PR.

The AKS Service has changed quite a bit in the last few weeks - including a number of breaking changes to the AKS API. Having spent some time digging into those API changes - we ended up having a chat with the AKS Team last week to clarify those changes.

With regards to the breaking changes, there's several things to fix here:

1. Making the service_principal block optional (incorporating service_principal in aks no longer required #6205)
2. Making the windows_profile block Computed, since a set of Windows credentials are generated if not specified
3. Handling mixed-mode clusters (e.g. a Service Principal with a Managed Identity) - which are no longer supported due to a breaking change in the API (making any change to the cluster will force-update them on Azure's side to only use MSI)
4. Updating the acceptance tests/documentation to use MSI rather than a Service Principal - since this is the new recommended approach

Previously @WodansSon has pushed a few commits to this PR to try and resolve those breaking changes in the API - and I hope you don't mind but I'm going to rebase this on top of master and then push a few more.

---

From our side, rather than setting the fields min_count and max_count to 0 - it'd be better for these to be set to null (since 0 isn't a valid value for this field). As such I believe the root-cause of this confusion is a bug in the error message, which requires updating here.

When wanting to be able to conditionally create an auto-scaled/manually-scaled cluster using the same code, you can instead use the value null rather than 0 for the min_count and max_count fields; for example:

locals {
  enable_auto_scale = false
  fixed_count = 2
  min_count   = 1
  max_count   = 4
}

resource "azurerm_kubernetes_cluster" "test" {
  # ...

  default_node_pool {
    enable_auto_scaling = local.enable_auto_scale
    node_count          = local.enable_auto_scale ? null : local.fixed_count
    min_count           = local.enable_auto_scale ? local.min_count : null
    max_count           = local.enable_auto_scale ? local.max_count : null
  }
}

This allows the validation for the min_count and max_count fields to be valid for when auto-scaling is enabled. Whilst I appreciate this does diverge from the original intention of this Pull Request - I believe this should fix #6020 whilst also keeping the validation correct here.

As such I hope you don't mind but I'm going to push the changes required to fix the breaking changes in the azurerm_kubernetes_cluster resource to this PR - which should mean that we can ship this fix, and the solution to the breaking API changes, in the v2.5 release of the Azure Provider.

Thanks!

[tombuildsstuff]

Ignoring a few broken tests due to this bug in the AKS API (which is also an issue before this PR) - the acceptance tests look good to me:

[tombuildsstuff]

@aristosvo just seen this - would you mind opening a new issue for that? A PR would be great too - however it's probably easiest to wait until after this PR is merged given the changes to the AKS resource required to handle the breaking changes to the API here

Thanks!

[ghost]

This has been released in version 2.5.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.5.0"
}
# ... other configuration ...

[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!