Improvement: azurerm_kubernetes_cluster - Add support for load_balancer_profile

[evenh]

Re-basing #4472

Fixes #4322

[evenh]

This PR has been open in various forms since September 20th 2019 - any chance of this getting merged into 1.42.0 @katbyte? 🤞

[katbyte]

Thanks for rebasing and reopening this @evenh,

I've got a few questions and comments i've left inline but otherwise this is looking good.

[evenh]

I’ve fixed your comments @katbyte :)

[mbfrahry]

Hey @evenh. I've got a couple of issues that need to be addressed, one is a potential panic and another is potentially not setting a nil value. I'm still running through the tests on our end but 🤞 we don't have any more snags. Thanks for your patience here

[evenh]

Thanks for the review @mbfrahry. I've pushed a commit with the requested changes.

[mbfrahry]

Apologies on the delay, the tests for this resource take ages and despite this looking like a straightforward addition, it raised some tests failures. It looks like Azure puts default values based on the load_balancer_sku. You can see that by running the TestAccAzureRMKubernetesCluster_standardLoadBalancer tests. These tests failed with the error:

After applying this step, the plan was not empty:

          network_profile.0.load_balancer_profile.#:                                     "1" => "0" (forces new resource)
          network_profile.0.load_balancer_profile.0.effective_outbound_ips.#:            "1" => "" (forces new resource)
          network_profile.0.load_balancer_profile.0.managed_outbound_ip_count:           "1" => "" (forces new resource)
          network_profile.0.load_balancer_profile.0.outbound_ip_address_ids.#:           "0" => "" (forces new resource)
          network_profile.0.load_balancer_profile.0.outbound_ip_prefix_ids.#:            "0" => "" (forces new resource)

I looked at the payload being returned from the api and can confirm that the api is defaulting these values. It looks like we may want to add Computed: true to each of the fields you're adding. As well as ConfigMode: schema.SchemaConfigModeAttr, to each of the Sets so they can be modified/removed if need be.

You'll want to run TestAccAzureRMKubernetesCluster_standardLoadBalancer after those changes are in to confirm we're all good.

There were a couple other misses from tests not being run in the suite. They do pass when run manually though it'd be good to get them into our nightly test run as well

[mbfrahry]

Tests pass and this LGTM. Thanks for the effort here @evenh

[evenh]

Hooray 🎉 Thanks for the thorough reviews!

[ghost]

This has been released in version 1.42.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.42.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!