r/kubernetes_cluster: supporting conditional updates / introducing `default_node_pool` #4898

tombuildsstuff · 2019-11-17T10:11:43Z

This PR introduces support for Conditional Updates for the azurerm_kubernetes_cluster resource - allowing users to use the ignore_changes functionality on the lifecycle block.

Since the resource was introduced there's been a breaking changes in the Azure Kubernetes Service API which requires that this resource behave differently - and there's another on the way:

When multiple node pools are used the Primary needs to be specified within the main resource (and modified there) - secondaries are now modified using the separate Node Pools API.
The default type for AKS Nodes is changing from AvailabilitySet to VirtualMachineScaleSets - as tracked in Support planned AKS API default swap to VMSS + SLB (from VMAS + BLB) #4465

After spending a ton of time trying to reconcile the different AKS PR's (#4676, #4543, #4472 and #4256) this PR supersedes them by:

introducing a new default_node_pool block which supersedes the agent_pool_profile block (which can then be removed in 2.0)
requiring that additional node pools are managed via the azurerm_kubernetes_cluster_node_pool resource being tracked in Add kubernetes_cluster_agent_pool #4046
allowing partial/conditional updates to the AKS resource, so that Terraform's ignore_changes block can be used on fields / configuration updates are applied prior to kubernetes updates

Whilst superseding PR's isn't ideal - introducing a replacement block with these changes allows this block to handle conditional updates from the start, rather than introducing a breaking behavioural change/waiting until 2.0.

Once this PR is merged, I'll rebase both #4046 (which adds support the azurerm_kubernetes_cluster_node_pool resource) and #4472 (which supports using Outbound IP's with a Standard Load Balancer) on top of this PR so that all of these changes can be shipped in the same release; and then open a separate PR which updates the examples to use the new default_node_pool block.

Fixes #3428
Fixes #3549
Fixes #3835
Fixes #3971
Fixes #4075
Fixes #4465
Fixes #4560
Fixes #4830

Supersedes #4256
Supersedes #4543
Supersedes #4676

~~Waiting on Azure/azure-rest-api-specs#7746~~
~~Waiting on Azure/azure-sdk-for-go#6325 to be merged/released - at which point we'll upgrade the PR in another PR which should unblock this~~

@titilambert

Co-authored-by: titilambert <[email protected]> Co-authored-by: djsly <[email protected]> This commit rebases @titilambert's original commits on top of #4898 In addition it also makes a couple of changes: - The resource has been renamed `azurerm_kubernetes_cluster_node_pool` to match the latest terminology used in the Portal - The tests have been updated to use a Default Node Pool block in the AKS resource - During creation we now check for the presence of the parent Cluster and then confirm that the Default Node Pool is a VirtualMachineScaleSet type - Removes support for `orchestrator_version` temporarily - since this wants to be added to both the Default Node Pool and this resource at the same time/in the same PR since this wants some more specific tests (which'd be easier to review in a separate PR) - Matches the name/schema for all fields with the `default_node_pool` block in the AKS resource I've ended up `git reset HEAD~N` here to work around the merge conflict due to the large changes within the AKS resource in #4898, but this seemed like the most pragmatic way to ship these changes.

@titilambert

Co-authored-by: titilambert <[email protected]> Co-authored-by: djsly <[email protected]> This commit rebases @titilambert's original commits on top of #4898 In addition it also makes a couple of changes: - The resource has been renamed `azurerm_kubernetes_cluster_node_pool` to match the latest terminology used in the Portal - The tests have been updated to use a Default Node Pool block in the AKS resource - During creation we now check for the presence of the parent Cluster and then confirm that the Default Node Pool is a VirtualMachineScaleSet type - Removes support for `orchestrator_version` temporarily - since this wants to be added to both the Default Node Pool and this resource at the same time/in the same PR since this wants some more specific tests (which'd be easier to review in a separate PR) - Matches the name/schema for all fields with the `default_node_pool` block in the AKS resource I've ended up `git reset HEAD~N` here to work around the merge conflict due to the large changes within the AKS resource in #4898, but this seemed like the most pragmatic way to ship these changes.

@titilambert

Co-authored-by: titilambert <[email protected]> Co-authored-by: djsly <[email protected]> This commit rebases @titilambert's original commits on top of #4898 In addition it also makes a couple of changes: - The resource has been renamed `azurerm_kubernetes_cluster_node_pool` to match the latest terminology used in the Portal - The tests have been updated to use a Default Node Pool block in the AKS resource - During creation we now check for the presence of the parent Cluster and then confirm that the Default Node Pool is a VirtualMachineScaleSet type - Removes support for `orchestrator_version` temporarily - since this wants to be added to both the Default Node Pool and this resource at the same time/in the same PR since this wants some more specific tests (which'd be easier to review in a separate PR) - Matches the name/schema for all fields with the `default_node_pool` block in the AKS resource I've ended up `git reset HEAD~N` here to work around the merge conflict due to the large changes within the AKS resource in #4898, but this seemed like the most pragmatic way to ship these changes.

@titilambert

Co-authored-by: titilambert <[email protected]> Co-authored-by: djsly <[email protected]> This commit rebases @titilambert's original commits on top of #4898 In addition it also makes a couple of changes: - The resource has been renamed `azurerm_kubernetes_cluster_node_pool` to match the latest terminology used in the Portal - The tests have been updated to use a Default Node Pool block in the AKS resource - During creation we now check for the presence of the parent Cluster and then confirm that the Default Node Pool is a VirtualMachineScaleSet type - Removes support for `orchestrator_version` temporarily - since this wants to be added to both the Default Node Pool and this resource at the same time/in the same PR since this wants some more specific tests (which'd be easier to review in a separate PR) - Matches the name/schema for all fields with the `default_node_pool` block in the AKS resource I've ended up `git reset HEAD~N` here to work around the merge conflict due to the large changes within the AKS resource in #4898, but this seemed like the most pragmatic way to ship these changes.

@titilambert

Co-authored-by: titilambert <[email protected]> Co-authored-by: djsly <[email protected]> This commit rebases @titilambert's original commits on top of #4898 In addition it also makes a couple of changes: - The resource has been renamed `azurerm_kubernetes_cluster_node_pool` to match the latest terminology used in the Portal - The tests have been updated to use a Default Node Pool block in the AKS resource - During creation we now check for the presence of the parent Cluster and then confirm that the Default Node Pool is a VirtualMachineScaleSet type - Removes support for `orchestrator_version` temporarily - since this wants to be added to both the Default Node Pool and this resource at the same time/in the same PR since this wants some more specific tests (which'd be easier to review in a separate PR) - Matches the name/schema for all fields with the `default_node_pool` block in the AKS resource I've ended up `git reset HEAD~N` here to work around the merge conflict due to the large changes within the AKS resource in #4898, but this seemed like the most pragmatic way to ship these changes.

@titilambert

Co-authored-by: titilambert <[email protected]> Co-authored-by: djsly <[email protected]> This commit rebases @titilambert's original commits on top of #4898 In addition it also makes a couple of changes: - The resource has been renamed `azurerm_kubernetes_cluster_node_pool` to match the latest terminology used in the Portal - The tests have been updated to use a Default Node Pool block in the AKS resource - During creation we now check for the presence of the parent Cluster and then confirm that the Default Node Pool is a VirtualMachineScaleSet type - Removes support for `orchestrator_version` temporarily - since this wants to be added to both the Default Node Pool and this resource at the same time/in the same PR since this wants some more specific tests (which'd be easier to review in a separate PR) - Matches the name/schema for all fields with the `default_node_pool` block in the AKS resource I've ended up `git reset HEAD~N` here to work around the merge conflict due to the large changes within the AKS resource in #4898, but this seemed like the most pragmatic way to ship these changes.

@titilambert

Co-authored-by: titilambert <[email protected]> Co-authored-by: djsly <[email protected]> This commit rebases @titilambert's original commits on top of #4898 In addition it also makes a couple of changes: - The resource has been renamed `azurerm_kubernetes_cluster_node_pool` to match the latest terminology used in the Portal - The tests have been updated to use a Default Node Pool block in the AKS resource - During creation we now check for the presence of the parent Cluster and then confirm that the Default Node Pool is a VirtualMachineScaleSet type - Removes support for `orchestrator_version` temporarily - since this wants to be added to both the Default Node Pool and this resource at the same time/in the same PR since this wants some more specific tests (which'd be easier to review in a separate PR) - Matches the name/schema for all fields with the `default_node_pool` block in the AKS resource I've ended up `git reset HEAD~N` here to work around the merge conflict due to the large changes within the AKS resource in #4898, but this seemed like the most pragmatic way to ship these changes.

@titilambert

Co-authored-by: titilambert <[email protected]> Co-authored-by: djsly <[email protected]> This commit rebases @titilambert's original commits on top of #4898 In addition it also makes a couple of changes: - The resource has been renamed `azurerm_kubernetes_cluster_node_pool` to match the latest terminology used in the Portal - The tests have been updated to use a Default Node Pool block in the AKS resource - During creation we now check for the presence of the parent Cluster and then confirm that the Default Node Pool is a VirtualMachineScaleSet type - Removes support for `orchestrator_version` temporarily - since this wants to be added to both the Default Node Pool and this resource at the same time/in the same PR since this wants some more specific tests (which'd be easier to review in a separate PR) - Matches the name/schema for all fields with the `default_node_pool` block in the AKS resource I've ended up `git reset HEAD~N` here to work around the merge conflict due to the large changes within the AKS resource in #4898, but this seemed like the most pragmatic way to ship these changes.

@titilambert

Co-authored-by: titilambert <[email protected]> Co-authored-by: djsly <[email protected]> This commit rebases @titilambert's original commits on top of #4898 In addition it also makes a couple of changes: - The resource has been renamed `azurerm_kubernetes_cluster_node_pool` to match the latest terminology used in the Portal - The tests have been updated to use a Default Node Pool block in the AKS resource - During creation we now check for the presence of the parent Cluster and then confirm that the Default Node Pool is a VirtualMachineScaleSet type - Removes support for `orchestrator_version` temporarily - since this wants to be added to both the Default Node Pool and this resource at the same time/in the same PR since this wants some more specific tests (which'd be easier to review in a separate PR) - Matches the name/schema for all fields with the `default_node_pool` block in the AKS resource I've ended up `git reset HEAD~N` here to work around the merge conflict due to the large changes within the AKS resource in #4898, but this seemed like the most pragmatic way to ship these changes.

…ve changed

Also adding a note about using ignore_changes with auto-scaling

…e ResourceData

…thod since it's different Create&Update

…the top level object not

…te api

Also adding a new test to confirm removing a node pool

mbfrahry

I'll need to defer to your implementation around certain bits of knowledge/logic but I found quite a few places we should check to confirm that something isn't nil. Also, I called out a few todos that might need to be done before this PR is merged but I'll defer that to you as well.

azurerm/internal/services/containers/kubernetes_addons.go

azurerm/internal/services/containers/kubernetes_nodepool.go

azurerm/resource_arm_kubernetes_cluster.go

…d `availabilityZones`

tombuildsstuff · 2019-11-20T10:59:57Z

Tests pass:

@titilambert

Co-authored-by: titilambert <[email protected]> Co-authored-by: djsly <[email protected]> This commit rebases @titilambert's original commits on top of #4898 In addition it also makes a couple of changes: - The resource has been renamed `azurerm_kubernetes_cluster_node_pool` to match the latest terminology used in the Portal - The tests have been updated to use a Default Node Pool block in the AKS resource - During creation we now check for the presence of the parent Cluster and then confirm that the Default Node Pool is a VirtualMachineScaleSet type - Removes support for `orchestrator_version` temporarily - since this wants to be added to both the Default Node Pool and this resource at the same time/in the same PR since this wants some more specific tests (which'd be easier to review in a separate PR) - Matches the name/schema for all fields with the `default_node_pool` block in the AKS resource I've ended up `git reset HEAD~N` here to work around the merge conflict due to the large changes within the AKS resource in #4898, but this seemed like the most pragmatic way to ship these changes.

@titilambert

Co-authored-by: titilambert <[email protected]> Co-authored-by: djsly <[email protected]> This commit rebases @titilambert's original commits on top of #4898 In addition it also makes a couple of changes: - The resource has been renamed `azurerm_kubernetes_cluster_node_pool` to match the latest terminology used in the Portal - The tests have been updated to use a Default Node Pool block in the AKS resource - During creation we now check for the presence of the parent Cluster and then confirm that the Default Node Pool is a VirtualMachineScaleSet type - Removes support for `orchestrator_version` temporarily - since this wants to be added to both the Default Node Pool and this resource at the same time/in the same PR since this wants some more specific tests (which'd be easier to review in a separate PR) - Matches the name/schema for all fields with the `default_node_pool` block in the AKS resource I've ended up `git reset HEAD~N` here to work around the merge conflict due to the large changes within the AKS resource in #4898, but this seemed like the most pragmatic way to ship these changes.

artburkart · 2019-11-20T14:13:36Z

azurerm/resource_arm_kubernetes_cluster_auth_test.go

+    name           = "default"
+    node_count     = 1
+    vm_size        = "Standard_DS2_v2"
+    vnet_subnet_id = azurerm_subnet.test.id


@tombuildsstuff – I was wondering, would it be worth surfacing the fact that the vnet_subnet_id on the default_node_pool is also the subnet that the entire cluster gets launched into?

az aks create -h | grep vnet-subnet -A1 --vnet-subnet-id : The ID of a subnet in an existing VNet into which to deploy the cluster.

https://docs.microsoft.com/en-us/cli/azure/aks?view=azure-cli-latest#az-aks-create

FWIW we plan to support assigning a separate subnet per agentpool, but haven't implemented it quite yet. Hence today the subnet property on additional agentpools doesn't do much yet.

Oh interesting. I hadn't even considered that.

I agree with you surfacing that info is helpful, just want to make sure it's easily updated when that new func is available :)

@jluk just to ensure I've got this right - from testing IIRC at this time all of the Subnet ID's need to be set to the same value for the meantime; rather than just being defined on the top level resource?

Correct today all of the subnetIds need to be the same.

When creating a new agent pool, if the subnetid is not provided, the default will be inferred from the primary node pool's subnetid. So you can either input the same id across all of them or not provide a value for it to be inferred, any other inputs will receive an error.

@jluk thanks for confirming - will get that updated 👍

Resolves this comment: #4898 (comment)

ghost · 2019-11-26T08:33:55Z

This has been released in version 1.37.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 = "~> 1.37.0"
}
# ... other configuration ...

maarek · 2019-12-06T16:50:08Z

@tombuildsstuff Should the data source for azurerm_kubernetes_cluster have been changed to represent this over agent_pool_profile as well? Is there a data source for the individual node pools?

tombuildsstuff · 2019-12-06T17:10:11Z

@maarek the Data Source intentionally still uses agent_pool_profiles since we didn't feel a separate data source for the individual node pools would be directly useful; that said we can add it if it is - mind opening a new issue if you're interested with a use-case? :)

jmcshane · 2019-12-11T21:19:03Z

Looks like @maarek has not submitted an issue, I'll write one up now.

ghost · 2019-12-20T14:50:24Z

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!

tombuildsstuff added enhancement service/kubernetes-cluster labels Nov 17, 2019

tombuildsstuff added this to the v1.37.0 milestone Nov 17, 2019

tombuildsstuff requested a review from a team November 17, 2019 10:11

tombuildsstuff self-assigned this Nov 17, 2019

ghost added size/XXL documentation labels Nov 17, 2019

This was referenced Nov 17, 2019

AKS cluster agent pool with fix for autoscaling and count. #4543

Closed

Closes #4560: Compare agent node pools from Azure API in order of terraform config #4676

Closed

r/kubernetes_cluster: autoscaling-related improvements #4256

Closed

tombuildsstuff mentioned this pull request Nov 17, 2019

New Resource: azurerm_kubernetes_cluster_node_pool #4899

Merged

tombuildsstuff mentioned this pull request Nov 19, 2019

dependencies: updating github.com/Azure/azure-sdk-for-go to v36.2.0 #4913

Merged

tombuildsstuff force-pushed the f/aks-conditional-updates branch from 40224b6 to a153867 Compare November 19, 2019 08:52

tombuildsstuff added 8 commits November 19, 2019 22:02

r/kubernetes_cluster: documenting the new enable_node_public_ip field

c15831e

r/kubernetes_cluster: conditionally updating the cluster if fields ha…

cf947f8

…ve changed

r/kubernetes_cluster: sorting the documentation

5f8b887

Also adding a note about using ignore_changes with auto-scaling

r/kubernetes_cluster: switching to pass in the objects rather than th…

f3d22cc

…e ResourceData

r/kubernetes_cluster: inlining the expandServicePrincipalProfile me…

2e74e7d

…thod since it's different Create&Update

r/kubernetes_cluster: updating the versions of kubernetes being used

6814035

r/kubernetes_cluster: making the addon_profile elements computed but …

657aced

…the top level object not

r/kubernetes_cluster: refactoring the addon helpers out

0f8d8eb

tombuildsstuff added 2 commits November 19, 2019 22:02

r/kubernetes_cluster: updating the default node pool using the separa…

4c28b2e

…te api

r/kubernetes_cluster: fixing a bug where count changes weren't detected

ae21a2b

Also adding a new test to confirm removing a node pool

tombuildsstuff force-pushed the f/aks-conditional-updates branch from a153867 to ae21a2b Compare November 19, 2019 20:25

tombuildsstuff changed the title ~~[WIP] r/kubernetes_cluster: supporting conditional updates / introducing default_node_pool~~ r/kubernetes_cluster: supporting conditional updates / introducing default_node_pool Nov 19, 2019

mbfrahry approved these changes Nov 19, 2019

View reviewed changes

tombuildsstuff added 6 commits November 20, 2019 09:11

r/kubernetes_cluster: switching to use the value rather than keying in

9ef91d7

r/kubernetes_cluster: always submitting the value for nodeTaints an…

b55a402

…d `availabilityZones`

r/kubernetes_cluster: making the VM SKU case sensitive

9d6e5e3

r/kubernetes_cluster: adding nil-check to sub-properties

3b98107

r/kubernetes_cluster: updating the tag to allow scheduling pods

82e6437

r/kubernetes_cluster: conditionally setting availability_zones

bcefb73

r/kubernetes_cluster: documentation fixes

d928e22

tombuildsstuff merged commit 81cce4d into master Nov 20, 2019

tombuildsstuff deleted the f/aks-conditional-updates branch November 20, 2019 12:03

tombuildsstuff added a commit that referenced this pull request Nov 20, 2019

Updating to include #4898

857e3d4

tombuildsstuff mentioned this pull request Nov 20, 2019

Panic when block is empty #3688

Closed

artburkart reviewed Nov 20, 2019

View reviewed changes

tombuildsstuff mentioned this pull request Nov 21, 2019

r/kubernetes_cluster: documenting the vnet_subnet_id field limitation #4946

Merged

katbyte pushed a commit that referenced this pull request Nov 21, 2019

r/kubernetes_cluster: documenting the vnet_subnet_id field li… (#4946)

59fa627

Resolves this comment: #4898 (comment)

secustor mentioned this pull request Nov 26, 2019

AKS agent_pool_profile block leads to cluster recreation #4987

Closed

jmcshane mentioned this pull request Dec 11, 2019

Support for accessing data about single AKS cluster node pool #5134

Closed

ghost locked and limited conversation to collaborators Dec 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

r/kubernetes_cluster: supporting conditional updates / introducing `default_node_pool` #4898

r/kubernetes_cluster: supporting conditional updates / introducing `default_node_pool` #4898

tombuildsstuff commented Nov 17, 2019 •

edited

Loading

mbfrahry left a comment

tombuildsstuff commented Nov 20, 2019

artburkart Nov 20, 2019

jluk Nov 20, 2019

artburkart Nov 20, 2019

jluk Nov 20, 2019

tombuildsstuff Nov 20, 2019

jluk Nov 20, 2019

jluk Nov 20, 2019

tombuildsstuff Nov 21, 2019

tombuildsstuff Nov 21, 2019

ghost commented Nov 26, 2019

maarek commented Dec 6, 2019 •

edited

Loading

tombuildsstuff commented Dec 6, 2019

jmcshane commented Dec 11, 2019

ghost commented Dec 20, 2019

r/kubernetes_cluster: supporting conditional updates / introducing default_node_pool #4898

r/kubernetes_cluster: supporting conditional updates / introducing default_node_pool #4898

Conversation

tombuildsstuff commented Nov 17, 2019 • edited Loading

mbfrahry left a comment

Choose a reason for hiding this comment

tombuildsstuff commented Nov 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Nov 26, 2019

maarek commented Dec 6, 2019 • edited Loading

tombuildsstuff commented Dec 6, 2019

jmcshane commented Dec 11, 2019

ghost commented Dec 20, 2019

r/kubernetes_cluster: supporting conditional updates / introducing `default_node_pool` #4898

r/kubernetes_cluster: supporting conditional updates / introducing `default_node_pool` #4898

tombuildsstuff commented Nov 17, 2019 •

edited

Loading

maarek commented Dec 6, 2019 •

edited

Loading