Azure Kubernetes Service: 2020-06 updates

[tombuildsstuff]

There's a number of Pull Requests open for Azure Kubernetes Service - so that we can merge these without conflicting in every Pull Request - this PR consolidates these changes into a single branch.

To achieve this these branches have been pulled locally, squashed, had any fixes applied and then cherry-picked onto this "combined" branch - so the original authors credit should still apply.

At the same time this PR adds support for a number of Feature Requests - specific details are below:

• dependencies: updating to use version 2020-03-01 of the containerservice API
• Data Source: azurerm_kubernetes_cluster
  • bug fix: fixing an issue where some read-only fields were unintentionally marked as user-configurable.
  • exposing the orchestrator_version being used for each Node Pool
  • exposing the field disk_encryption_set_id
• New Data Source: azurerm_kubernetes_cluster_node_pool
• azurerm_kubernetes_cluster
  • Azure China:
    • the Azure Policy add-on is not available and no longer sent
  • Azure US Government:
    • the Azure Policy add-on is not available and no longer sent
    • the Kubernetes Dashboard add-on is not available and no longer sent
  • Support for configuring/updating the version of Kubernetes used in the Default Node Pool (Add Orchestrator version for Kubernetes Cluster and Node Pool #6047 by @titilambert)
  • Support for Azure Active Directory (Managed) Integration v2 (incorporating Enable AKS AAD integration v2 + SDK version bump #6530 by @jlpedrosa)
  • Support for using a Disk Encryption Set (incorporating Add support for encrypted disks in aks #7110 by @jlpedrosa)
  • Support for configuring the Auto Scale Profile (incorporating kubernetes_cluster - Add support for auto_scale_profile #6620 by @aristosvo)
  • Support for configuring outbound_ports_allocated and idle_timeout_in_minutes within the load_balancer_profile block (incorporating Support more properties for azurerm_kubernetes_cluster #5824 by @neil-yechenwei)
  • Support for the Uptime SLA / Paid SKU (tracked in Support for AKS uptime SLA #6912)
  • Exposing the private_fqdn of the cluster
  • Checking for a System Node Pool when importing
• azurerm_kubernetes_cluster_node_pool
  • Support for configuring/updating the version of Kubernetes being used (incorporating Add Orchestrator version for Kubernetes Cluster and Node Pool #6047 by @titilambert)
  • Support for Spot Node Pools (superseding the work done in Spot Priority support for AKS #7145 by @pbrit / WIP : Support for spot node pool #6231 by @rajalokan)
  • Support for configuring System/User Node Pools (tracked in Support for mode:system pools in AKS #6058)
  • Changes to the node_taints now force a new resource, matching the updated API behaviour
• azurerm_linux_virtual_machine_scale_set
  • adding validation for the max_bid_price field
• azurerm_windows_virtual_machine_scale_set
  • adding validation for the max_bid_price field

Whilst pulling this many changes into a single Pull Request isn't ideal (and we'd generally avoid it) - this allows us to merge the changes above whilst avoiding merge conflicts in all the open PR's and so appeared to be the most pragmatic approach to move forward.

---

One thing in particular to call out is updating Node Pools - where a Kubernetes Cluster/Control Plane must be updated before the Node Pools can be updated; as such we've added a check during the Apply to ensure that the version of Kubernetes used for the Node Pool is supported by the Control Plane (or if we need to upgrade that first). Example of that error:

Error:
The Kubernetes/Orchestrator Version "1.16.9" is not available for Node Pool "default".

Please confirm that this version is supported by the Kubernetes Cluster "tom-dev-aks"
(Resource Group "tom-dev-rg") - which may need to be upgraded first.

The Kubernetes Cluster is running version "1.16.8".

The supported Orchestrator Versions for this Node Pool/supported by this Kubernetes Cluster are:
 * 1.14.7
 * 1.16.8
 * 1.15.10
 * 1.14.8
 * 1.15.11

Node Pools cannot use a version of Kubernetes that is not supported on the Control Plane. More
details can be found at https://aka.ms/version-skew-policy.

---

Fixes #1915
Fixes #4327
Fixes #5134
Fixes #5487
Fixes #5541
Fixes #5646
Fixes #6058
Fixes #6086
Fixes #6462
Fixes #6464
Fixes #6612
Fixes #6702
Fixes #6912
Fixes #6994
Fixes #7092
Fixes #7136
Fixes #7198

[mbfrahry]

LGTM with some minor additional checks

[pbrit]

@tombuildsstuff Thank you for your work.

However, I'm concerned about your approach. Yes, it's an effective way to add more functionality, yet, it's equally effective for introducing more bugs.

Even after a short review session, I see many issues with the way SpotPriority was re-implemented. It's just one chunk, there are 10+ more which I'm not familiar with.

I'm going to take this branch for a ride and will provide the feedback.
Also, I'm wondering what the rest of community think of about it.

[tombuildsstuff]

@pbrit

Due to the nature of AKS unfortunately we're frequently faced with breaking changes (either behaviourally, or code changes) within AKS every few months. As such over time we've built up an extensive test suite for AKS which covers all kinds of configurations, functionality and bugs - which we're continually adding to as new functionality gets added/bugs get fixed.

We'd had a heads-up of some potentially breaking changes coming to the AKS API's in the near-future - as such we've been trying to find out more information about the specific details. In the interim, there's been a number of PR's opened (including yours) which have looked to add new functionality/fix bugs in this resource.

We've recently got more details about these changes and it appears that they won't break us in the way we'd been expecting - as such we're good to proceed with these PR's, however having a number of open PR's puts us in an unfortunate position where merging one causes merge conflicts in the other.

Having spent some time reviewing these PR's and determining how's best to proceed, we came to the conclusion that consolidating these (7 PR's) together was the most pragmatic way forward. Since a number of these PR's also required (and vendored) a newer API version - the best way to do this was to squash the commits down, remove any vendoring changes and then fix comments from PR review.

Whilst generally speaking I'd agree with you regarding a single large PR's vs multiple small PR's - in this instance we're fortunate to have sufficient test coverage of the AKS Resources that we're able to detect bugs both in our code and in the AKS API's themselves. To give one specific example here: the test suite had detected a breaking behavioural change in the AKS API when updating a Load Balancer in a given configuration (which I'd also spotted during development) - but will be pushing a fix for shortly.

Whilst we appreciate there's some downsides to this approach - and we'd have preferred to have gone through and merged those PR's individually - after digging into it consolidating these appeared to be the more pragmatic solution. That said, generally speaking we'd tend to prefer/merge more, smaller PR's where possible - this is only possible due to the large test coverage we've got for AKS.

---

Even after a short review session, I see many issues with the way SpotPriority was re-implemented. It's just one chunk, there are 10+ more which I'm not familiar with.

To answer your specific questions around Spot Max Price - unfortunately different Azure Teams can refer to the same functionality using different terminology. As such, whilst we could seek to use a consistent name here - in this instance we're following the name used by the AKS Service, rather than the Compute Service (since this is exposed as "Spot Max Price" in the AKS API rather than "Spot Bid Price" in the VMSS API).

If you've noticed specific issues/bugs in the PR then please feel free to comment on the PR Review and we can take a look/work through those however :)

[aristosvo]

@tombuildsstuff Thanks for refactoring 'my' code regarding the delta-updates for the load_balancer_profile. I'm still a bit afraid #6534 might resurface, as it's not clear from the code and the tests how this works out.

I probably won't have time available to write the test for it, but it comes down to the following scenario:

• if idle_timeout_in_minutes is changed
• outbound_ip_address_ids was configured, but not changed
  Is the outbound_ip_adress picked up or will it trigger an error?

[tombuildsstuff]

@aristosvo thanks for looking - for context there's a breaking change in the new API version where the field idle_timeout_in_minutes now always needs to be specified (which caused the tests TestAccAzureRMKubernetesCluster_addAgent and TestAccAzureRMKubernetesCluster_removeAgent to fail) - which is why this is being updated.

From what I can tell, the scenario for #6534 is captured in the Test TestAccAzureRMKubernetesCluster_changingLoadBalancerProfile (which is currently failing / I'm working through fixing at the moment) - but we can add an additional one if that's not sufficient, we'll see shortly :)

[jackofallops]

LGTM 👍 Thanks Tom!

[tombuildsstuff]

Running the test suite for this, the tests look good:

Of the failures:

• 1 was a permissions error (since resolved)
• 3 are previously known/open Azure API bugs
• 2 require re-working - for one AKS changed it's implementation a while back and these need updating, the other will be fixed by fixing Changing "azurerm_kubernetes_cluster" default_node_pool.vm_size forces replacement of the whole kubernetes cluster #7093

So this should be good 👍

[tombuildsstuff]

Data Source test passes after updating the CheckDestroy helper:

$ TF_PROVIDER_SPLIT_COMBINED_TESTS=1 TF_ACC=1 go test -v ./azurerm/internal/services/containers/tests/ -timeout=60m -run=TestAccAzureRMKubernetesClusterNodePoolDataSource_basic
=== RUN   TestAccAzureRMKubernetesClusterNodePoolDataSource_basic
=== PAUSE TestAccAzureRMKubernetesClusterNodePoolDataSource_basic
=== CONT  TestAccAzureRMKubernetesClusterNodePoolDataSource_basic
--- PASS: TestAccAzureRMKubernetesClusterNodePoolDataSource_basic (1428.48s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/containers/tests	1428.528s

[ghost]

This has been released in version 2.14.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.14.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 👉 hashibot-feedback@hashicorp.com. Thanks!