azurerm_kubernetes_cluster - Support in-place update of network_profile.network_policy

[jkroepke]

Community Note

• Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
• Please do not leave "+1" or "me too" comments, they generate extra noise for PR followers and do not help prioritize for review

Description

This is an follow-up PR from #23342

Reading the AKS docs https://learn.microsoft.com/en-us/azure/aks/use-network-policies it's now possible to upgrade from none to calico, azure or cillium. It also possible to uninstall calico and azure.

There are now too many situation that they are allowed are now. Removing the ForceNew flag should be the best option here.

PR Checklist

• I have followed the guidelines in our Contributing Documentation.
• I have checked to ensure there aren't other open Pull Requests for the same update/change.
• I have checked if my changes close any open issues. If so please include appropriate closing keywords below.
• I have updated/added Documentation as required written in a helpful and kind way to assist users that may be unfamiliar with the resource / data source.
• I have used a meaningful PR title to help maintainers and other users understand this change and help prevent duplicate work.
  For example: “resource_name_here - description of change e.g. adding property new_property_name_here”

Changes to existing Resource / Data Source

• I have added an explanation of what my changes do and why I'd like you to include them (This may be covered by linking to an issue above, but may benefit from additional explanation).
• I have written new tests for my resource or datasource changes & updated any relevent documentation.
• I have successfully run tests with my changes locally. If not, please provide details on testing challenges that prevented you running the tests.
• (For changes that include a state migration only). I have manually tested the migration path between relevant versions of the provider.

Testing

• My submission includes Test coverage as described in the Contribution Guide and the tests pass. (if this is not possible for any reason, please include details of why you did or could not add test coverage)

==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test -v ./internal/services/containers '-run=TestAccKubernetesCluster_.*Policy.*' -timeout 300m -ldflags="-X=github.com/hashicorp/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccKubernetesCluster_addonProfileAzurePolicy
=== PAUSE TestAccKubernetesCluster_addonProfileAzurePolicy
=== RUN   TestAccKubernetesCluster_advancedNetworkingAzureCalicoPolicy
=== PAUSE TestAccKubernetesCluster_advancedNetworkingAzureCalicoPolicy
=== RUN   TestAccKubernetesCluster_advancedNetworkingAzureCiliumPolicy
=== PAUSE TestAccKubernetesCluster_advancedNetworkingAzureCiliumPolicy
=== RUN   TestAccKubernetesCluster_advancedNetworkingAzureCiliumPolicyUpdate
=== PAUSE TestAccKubernetesCluster_advancedNetworkingAzureCiliumPolicyUpdate
=== RUN   TestAccKubernetesCluster_advancedNetworkingAzureAzurePolicyUpdate
=== PAUSE TestAccKubernetesCluster_advancedNetworkingAzureAzurePolicyUpdate
=== RUN   TestAccKubernetesCluster_advancedNetworkingAzureCalicoPolicyUpdate
=== PAUSE TestAccKubernetesCluster_advancedNetworkingAzureCalicoPolicyUpdate
=== RUN   TestAccKubernetesCluster_advancedNetworkingAzureCalicoPolicyComplete
=== PAUSE TestAccKubernetesCluster_advancedNetworkingAzureCalicoPolicyComplete
=== RUN   TestAccKubernetesCluster_advancedNetworkingAzureCalicoPolicyNetworkModeTransparent
=== PAUSE TestAccKubernetesCluster_advancedNetworkingAzureCalicoPolicyNetworkModeTransparent
=== RUN   TestAccKubernetesCluster_advancedNetworkingAzureNPMPolicy
=== PAUSE TestAccKubernetesCluster_advancedNetworkingAzureNPMPolicy
=== RUN   TestAccKubernetesCluster_advancedNetworkingAzureNPMPolicyComplete
=== PAUSE TestAccKubernetesCluster_advancedNetworkingAzureNPMPolicyComplete
=== CONT  TestAccKubernetesCluster_addonProfileAzurePolicy
=== CONT  TestAccKubernetesCluster_advancedNetworkingAzureCalicoPolicyUpdate
=== CONT  TestAccKubernetesCluster_advancedNetworkingAzureCiliumPolicyUpdate
=== CONT  TestAccKubernetesCluster_advancedNetworkingAzureAzurePolicyUpdate
=== CONT  TestAccKubernetesCluster_advancedNetworkingAzureNPMPolicyComplete
=== CONT  TestAccKubernetesCluster_advancedNetworkingAzureCalicoPolicy
=== CONT  TestAccKubernetesCluster_advancedNetworkingAzureCalicoPolicyNetworkModeTransparent
=== CONT  TestAccKubernetesCluster_advancedNetworkingAzureCiliumPolicy
--- PASS: TestAccKubernetesCluster_advancedNetworkingAzureCalicoPolicy (411.53s)
=== CONT  TestAccKubernetesCluster_advancedNetworkingAzureCalicoPolicyComplete
--- PASS: TestAccKubernetesCluster_advancedNetworkingAzureCalicoPolicyNetworkModeTransparent (503.15s)
=== CONT  TestAccKubernetesCluster_advancedNetworkingAzureNPMPolicy
--- PASS: TestAccKubernetesCluster_advancedNetworkingAzureNPMPolicyComplete (560.39s)
--- PASS: TestAccKubernetesCluster_advancedNetworkingAzureCiliumPolicy (667.14s)
--- PASS: TestAccKubernetesCluster_advancedNetworkingAzureCiliumPolicyUpdate (683.03s)
--- PASS: TestAccKubernetesCluster_addonProfileAzurePolicy (806.21s)
--- PASS: TestAccKubernetesCluster_advancedNetworkingAzureCalicoPolicyComplete (500.62s)
--- PASS: TestAccKubernetesCluster_advancedNetworkingAzureNPMPolicy (511.80s)
--- PASS: TestAccKubernetesCluster_advancedNetworkingAzureAzurePolicyUpdate (1031.01s)
--- PASS: TestAccKubernetesCluster_advancedNetworkingAzureCalicoPolicyUpdate (1039.71s)
PASS
ok  	github.com/hashicorp/terraform-provider-azurerm/internal/services/containers	1039.872s

I had to add node_resource_group = "${azurerm_resource_group.test.name}-nodes" to each test.

Running tests in a location with a long name, e.g. germanywestcentral, results into an error:

        Resource Group Name: "acctestRG-aks-240531233634701724"
        Kubernetes Cluster Name: "acctestaks240531233634701724"): performing CreateOrUpdate: unexpected status 400 (400 Bad Request) with response: {
          "code": "InvalidParameter",
          "details": null,
          "message": "The length of the node resource group name is too long. The maximum length is 80 and the length of the value provided is 83. Please see https://aka.ms/aks-naming-rules for more details.",
          "subcode": "",
          "target": "name"
         }

Had to use germanywestcentral because westeurope was full.

Change Log

Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.

• azurerm_kubernetes_cluster - Changing network_profile.network_policy does not force recreation.

This is a (please select all that apply):

• Bug Fix
• New Feature (ie adding a service, resource, or data source)
• Enhancement
• Breaking Change

Related Issue(s)

Fixes #0000

Note

If this PR changes meaningfully during the course of review please update the title and description as required.

[stephybun]

Thanks for this PR @jkroepke.

This feature in requested over in #25597 which contains an explanation for why we're halting the addition of new preview features to the AKS resource.

Given the explanation linked above we're unable to add support for this behaviour at this point in time so I'm going to close this PR for the time being. When a resolution is reached and/or the feature has come out of preview and is available in a stable API version we will be happy to re-open this. Thanks again for your effort and understanding on this!

[jkroepke]

@stephybun I disagree. If Issue you mention is about allow network_policy to "none", and starts about an discussion about the API Version. However, this PR does not allow the "none" value, nor changing the API version. It simply removing the ForceNew flag. There is new feature introduction in this PR.

I can't understand, whats the connection between removing a ForeNew flag and #25597

This PR is just removing an limitation which not longer exists at Microsoft API side.

---

Please note that install a Network Policy Engine on existing cluster is GA.

The page https://learn.microsoft.com/en-us/azure/aks/azure-cni-overlay?tabs=kubectl does not mention preview somewhere.

[stephybun]

@jkroepke, allowing any kind of update on the network_policy field by removing the ForceNew on it, allows certain preview functionalities into the resource.

Your tests check that network_policy can be removed, this I believe implies a network_policy of none which is the step required to uninstall Azure's Network Policy Manager or Calico - according to the documentation linked, that is a preview feature.

https://learn.microsoft.com/en-us/azure/aks/use-network-policies#uninstall-azure-network-policy-manager-or-calico-preview

[jkroepke]

In summarize to unblock this PR, I can re-introduce ForceNew flag which triggers if is the new value is empty and the old not?

[jkroepke]

this I believe implies a network_policy of none

I test that in an local test to validate that statement:

func TestAccKubernetesCluster_advancedNetworkingAzureAzurePolicyUpdate(t *testing.T) {
	data := acceptance.BuildTestData(t, "azurerm_kubernetes_cluster", "test")
	r := KubernetesClusterResource{}

	data.ResourceTest(t, r, []acceptance.TestStep{
		{
			Config: r.advancedNetworkingWithOptionalPolicyConfig(data, ""),
			Check: acceptance.ComposeTestCheckFunc(
				check.That(data.ResourceName).ExistsInAzure(r),
			),
		},
		data.ImportStep(),
		{
			Config: r.advancedNetworkingWithOptionalPolicyConfig(data, "azure"),
			Check: acceptance.ComposeTestCheckFunc(
				check.That(data.ResourceName).ExistsInAzure(r),
				check.That(data.ResourceName).Key("network_profile.0.network_policy").Exists(),
				check.That(data.ResourceName).Key("network_profile.0.network_policy").HasValue("azure"),
			),
		},
		data.ImportStep(),
		{
			Config: r.advancedNetworkingWithOptionalPolicyConfig(data, ""),
			Check: acceptance.ComposeTestCheckFunc(
				check.That(data.ResourceName).ExistsInAzure(r),
				check.That(data.ResourceName).Key("network_profile.0.network_policy").Exists(),
				check.That(data.ResourceName).Key("network_profile.0.network_policy").HasValue("azure"),
			),
		},
		data.ImportStep(),
	})
}

func TestAccKubernetesCluster_advancedNetworkingAzureCalicoPolicyUpdate(t *testing.T) {
	data := acceptance.BuildTestData(t, "azurerm_kubernetes_cluster", "test")
	r := KubernetesClusterResource{}

	data.ResourceTest(t, r, []acceptance.TestStep{
		{
			Config: r.advancedNetworkingWithOptionalPolicyConfig(data, ""),
			Check: acceptance.ComposeTestCheckFunc(
				check.That(data.ResourceName).ExistsInAzure(r),
			),
		},
		data.ImportStep(),
		{
			Config: r.advancedNetworkingWithOptionalPolicyConfig(data, "calico"),
			Check: acceptance.ComposeTestCheckFunc(
				check.That(data.ResourceName).ExistsInAzure(r),
				check.That(data.ResourceName).Key("network_profile.0.network_policy").Exists(),
				check.That(data.ResourceName).Key("network_profile.0.network_policy").HasValue("calico"),
			),
		},
		data.ImportStep(),
		{
			Config: r.advancedNetworkingWithOptionalPolicyConfig(data, ""),
			Check: acceptance.ComposeTestCheckFunc(
				check.That(data.ResourceName).ExistsInAzure(r),
				check.That(data.ResourceName).Key("network_profile.0.network_policy").Exists(),
				check.That(data.ResourceName).Key("network_profile.0.network_policy").HasValue("calico"),
			),
		},
		data.ImportStep(),
	})
}

TF_ACC=1 go test -v ./internal/services/containers '-run=TestAccKubernetesCluster_advancedNetworking.+PolicyUpdate' -timeout 300m -ldflags="-X=github.com/hashicorp/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccKubernetesCluster_advancedNetworkingAzureCiliumPolicyUpdate
=== PAUSE TestAccKubernetesCluster_advancedNetworkingAzureCiliumPolicyUpdate
=== RUN   TestAccKubernetesCluster_advancedNetworkingAzureAzurePolicyUpdate
=== PAUSE TestAccKubernetesCluster_advancedNetworkingAzureAzurePolicyUpdate
=== RUN   TestAccKubernetesCluster_advancedNetworkingAzureCalicoPolicyUpdate
=== PAUSE TestAccKubernetesCluster_advancedNetworkingAzureCalicoPolicyUpdate
=== CONT  TestAccKubernetesCluster_advancedNetworkingAzureCiliumPolicyUpdate
=== CONT  TestAccKubernetesCluster_advancedNetworkingAzureCalicoPolicyUpdate
=== CONT  TestAccKubernetesCluster_advancedNetworkingAzureAzurePolicyUpdate
--- PASS: TestAccKubernetesCluster_advancedNetworkingAzureCiliumPolicyUpdate (614.03s)
--- PASS: TestAccKubernetesCluster_advancedNetworkingAzureAzurePolicyUpdate (1009.71s)
--- PASS: TestAccKubernetesCluster_advancedNetworkingAzureCalicoPolicyUpdate (1169.64s)
PASS
ok  	github.com/hashicorp/terraform-provider-azurerm/internal/services/containers	1169.753s

It turn out, omit the value would not uninstall the Network Policy. Uninstall the Network Policy requires an explicit none.

But I would agree that this may confuse end-users, too. I re-add the ForceNew condition which should deny that mention situation. But in any situation, a preview feature is not used in this PR.

[stephybun]

Thanks for doing some local testing and apologies for needing to drill down into the details here @jkroepke, but given the explanation in the comment I linked we're trying to be careful about introducing functionality that may need to be removed in future. The documentation for this also appears ambiguous, some operations are preview whereas others are not, so being diligent here will save us some potential headaches in the future.

It would help if you could explain the specific use case you're trying to support? i.e. updating the network_policy from a -> b?

Furthermore

It turn out, omit the value would not uninstall the Network Policy

If we're omitting the value sent, then what is the API sending back/what is the cluster falling back on for its network policy?

[stephybun]

Thanks @jkroepke. Could you please address the comments left regarding the tests? Existing test configs can be re-used and several of the specific Exists and HasValue checks are taken care of by the ImportStep and can be removed.

Would you also mind updating the documentation?

[jkroepke]

Would you also mind updating the documentation?

I looked at the documentation also looked at the PR where the change to cilium was introduced. I feel that the current description still matches, no idea what I can improve here.

[stephybun]

Thanks @jkroepke LGTM 👍

[stephybun]

@jkroepke running the tests locally revealed that the ForceNewIf condition is applying in the wrong scenarios and the tests are passing because the cluster is being deleted and recreated.

Updating the condition and running the test again results in plan diff:

=== CONT  TestAccKubernetesCluster_advancedNetworkingCalicoPolicyUpdate

# azurerm_kubernetes_cluster.test will be updated in-place
          ~ resource "azurerm_kubernetes_cluster" "test" {
                id                                  = "/subscriptions/1a6092a6-137e-4025-9a7c-ef77f76f2c02/resourceGroups/acctestRG-aks-240610111117484138/providers/Microsoft.ContainerService/managedClusters/acctestaks240610111117484138"
                name                                = "acctestaks240610111117484138"
                tags                                = {}
                # (28 unchanged attributes hidden)
        
              ~ network_profile {
                  + network_policy          = "calico"
                    # (10 unchanged attributes hidden)
        
                    # (1 unchanged block hidden)
                }
        
                # (5 unchanged blocks hidden)
            }

To me it looks like there are some issues with the API and I think we will need to revert this change.

Update: Computed is potentially also an issue here. Since we're not sending the updated value for network_policy in the update call. Either way this needs to be re-worked.

[jkroepke]

I really ask myself, why I did not had that behavior and what I did wrong.

Edit: tbh. I did not re-run the tests after the code suggestions again.

[stephybun]

@jkroepke I ran the tests in TeamCity and they all passed, but tests that are supposed to check updates don't flag if the update causes the resource to be recreated. You can really only see that behaviour if you're looking at the API requests/responses that the provider is sending while the test is running, or by building the provider locally and running through the steps manually.

This was in part a mistake on my end and one of the review comments I left was also false. You did everything correct in that you followed instruction, I was not thorough enough in my review and too eager to have this merged, so the fault is with me.

I'm looking at this in detail and will open a PR shortly to fix the behaviour.

[jkroepke]

What I have to configure that I see the output that you shows me? That would help me in the future to indicate if the behavior is correctly.

[stephybun]

I assume you're referring to the output in this comment?

This is just the output from running the test locally after I updated the ForceNewIf condition to not trigger a recreation when network_policy was updated.

If you're asking how to view the API requests/responses from the tests, this can't be configured as output in the tests. You either need to check the debug logs, or be running the tests with a proxy that can capture the traffic. I hope that answers your questions.

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.