allow setting the NatGateway name if it's empty when update AzureCluster #3551

xiujuanx · 2023-05-10T04:15:02Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
Now, we default use the NAT gateway for the node outbound connection if cluster is not using IPv6. So we should allow setting the NatGateway name if it's empty when update AzureCluster.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #
The AzureCluster webhook is validating if the old gateway name is same with the new gateway name when update:

			if (subnet.NatGateway.Name != oldSubnet.NatGateway.Name) {
				allErrs = append(allErrs,
					field.Invalid(field.NewPath("spec", "networkSpec", "subnets").Index(oldSubnetIndex[subnet.Name]).Child("NatGateway").Child("Name"),
						c.Spec.NetworkSpec.Subnets[i].NatGateway.Name, "field is immutable"),
				)
			}

If they are different, then below error will happen:

E0509 15:24:33.726961       1 controller.go:329] "Reconciler error" err="error reconciling the Cluster topology: failed to create patch helper for AzureCluster/wl-antrea-1-5f692: server side apply dry-run failed for modified object: admission webhook \"validation.azurecluster.infrastructure.cluster.x-k8s.io\" denied the request: AzureCluster.infrastructure.cluster.x-k8s.io \"wl-antrea-1-5f692\" is invalid: spec.networkSpec.subnets[1].NatGateway.Name: Invalid value: \"wl-antrea-1-5f692-node-natgw-1\": field is immutable" controller="topology/cluster" controllerGroup="cluster.x-k8s.io" controllerKind="Cluster" Cluster="default/wl-antrea-1" namespace="default" name="wl-antrea-1" reconcileID=59bb6d5c-88c6-44b6-a90d-a32817f9d700
E0509 15:24:33.735506       1 controller.go:329] "Reconciler error" err="error reconciling the Cluster topology: failed to create patch helper for AzureCluster/tkg-upgrade-10900mgmt-azure-tpmd7: server side apply dry-run failed for modified object: admission webhook \"validation.azurecluster.infrastructure.cluster.x-k8s.io\" denied the request: AzureCluster.infrastructure.cluster.x-k8s.io \"tkg-upgrade-10900mgmt-azure-tpmd7\" is invalid: spec.networkSpec.subnets[1].NatGateway.Name: Invalid value: \"tkg-upgrade-10900mgmt-azure-tpmd7-node-natgw-1\": field is immutable" controller="topology/cluster" controllerGroup="cluster.x-k8s.io" controllerKind="Cluster" Cluster="tkg-system/tkg-upgrade-10900mgmt-azure" namespace="tkg-system" name="tkg-upgrade-10900mgmt-azure" reconcileID=cea35543-a8d9-494b-8498-fb3dc831844d

This error is expected if a NatGateway has created before upgrade.
But the current situation is the NatGateway name is empty, then we will generate a new one default. This makes the old NatGateway name is different with the new one, so webhook complains error. This is not expected. We should allow setting the NatGateway name if it's empty when update AzureCluster

Special notes for your reviewer:

cherry-pick candidate

TODOs:

squashed commits
includes documentation
adds unit tests

Release note:

Allow setting `spec.networkSpec.subnets[i].NatGateway.Name` for AzureCluster object if it's empty when upgrade

…object

k8s-ci-robot · 2023-05-10T04:15:12Z

Hi @xiujuanx. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

yastij · 2023-05-10T08:19:15Z

/ok-to-test

yastij

/lgtm

k8s-ci-robot · 2023-05-10T09:22:45Z

LGTM label has been added.

Git tree hash: 407a5a378361b1c4248905153cfe592be21b5da8

yastij · 2023-05-10T09:23:34Z

@xiujuanx - can you add a release note please?

xiujuanx · 2023-05-10T09:29:25Z

@xiujuanx - can you add a release note please?

@yastij Done, thanks!

CecileRobertMichon · 2023-05-10T16:50:42Z

/cherry-pick release-1.9

k8s-infra-cherrypick-robot · 2023-05-10T16:50:43Z

@CecileRobertMichon: once the present PR merges, I will cherry-pick it on top of release-1.9 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.9

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

CecileRobertMichon

/lgtm
/assign @sonasingh46 @willie-yao

Another one to look into why the upgrade test didn't catch it

CecileRobertMichon · 2023-05-10T18:01:36Z

api/v1beta1/azurecluster_webhook.go

@@ -168,7 +168,7 @@ func (c *AzureCluster) validateSubnetUpdate(old *AzureCluster) field.ErrorList {
 						c.Spec.NetworkSpec.Subnets[i].RouteTable.Name, "field is immutable"),
 				)
 			}
-			if subnet.NatGateway.Name != oldSubnet.NatGateway.Name {
+			if (subnet.NatGateway.Name != oldSubnet.NatGateway.Name) && (oldSubnet.NatGateway.Name != "") {


nit: we could use webhookutils.ValidateZeroTransition for this

but I see we're not doing this consistently in this current file so also good with merging this as is for now

jackfrancis · 2023-05-10T21:42:54Z

@CecileRobertMichon is the cluster scenario we're using in the upgrade test using NatGateway?

sonasingh46

/lgtm

willie-yao · 2023-05-11T18:41:48Z

is the cluster scenario we're using in the upgrade test using NatGateway?

The upgrade test upgrades from the v1.0.2 default prow template, which includes the natgateway. However, the latest version of the prow template does not have the natgateway field, which is probably why this test didn't catch it:

v1.0.2 template: https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/main/test/e2e/data/infrastructure-azure/v1.0.2/cluster-template-prow.yaml
current template: https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/main/templates/test/ci/cluster-template-prow.yaml

CecileRobertMichon · 2023-05-11T18:53:40Z

/lgtm
/approve

k8s-ci-robot · 2023-05-11T18:54:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [CecileRobertMichon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-infra-cherrypick-robot · 2023-05-11T18:55:58Z

@CecileRobertMichon: new pull request created: #3554

In response to this:

/cherry-pick release-1.9

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

allow set the NatGateway name if it's empty when update AzureCluster …

1b48700

…object

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 10, 2023

k8s-ci-robot requested review from CecileRobertMichon and willie-yao May 10, 2023 04:15

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 10, 2023

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 10, 2023

yastij reviewed May 10, 2023

View reviewed changes

k8s-ci-robot assigned yastij May 10, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 10, 2023

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels May 10, 2023

xiujuanx requested a review from yastij May 10, 2023 09:29

CecileRobertMichon reviewed May 10, 2023

View reviewed changes

k8s-ci-robot assigned sonasingh46, willie-yao and CecileRobertMichon May 10, 2023

sonasingh46 approved these changes May 11, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 11, 2023

k8s-ci-robot merged commit 2ceb6a1 into kubernetes-sigs:main May 11, 2023

k8s-ci-robot added this to the v1.10 milestone May 11, 2023

k8s-infra-cherrypick-robot mentioned this pull request May 11, 2023

[release-1.9] allow setting the NatGateway name if it's empty when update AzureCluster #3554

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow setting the NatGateway name if it's empty when update AzureCluster #3551

allow setting the NatGateway name if it's empty when update AzureCluster #3551

xiujuanx commented May 10, 2023 •

edited

Loading

k8s-ci-robot commented May 10, 2023

yastij commented May 10, 2023

yastij left a comment

k8s-ci-robot commented May 10, 2023

yastij commented May 10, 2023

xiujuanx commented May 10, 2023

CecileRobertMichon commented May 10, 2023

k8s-infra-cherrypick-robot commented May 10, 2023

CecileRobertMichon left a comment

CecileRobertMichon May 10, 2023

jackfrancis commented May 10, 2023

sonasingh46 left a comment

willie-yao commented May 11, 2023

CecileRobertMichon commented May 11, 2023

k8s-ci-robot commented May 11, 2023

k8s-infra-cherrypick-robot commented May 11, 2023

allow setting the NatGateway name if it's empty when update AzureCluster #3551

allow setting the NatGateway name if it's empty when update AzureCluster #3551

Conversation

xiujuanx commented May 10, 2023 • edited Loading

k8s-ci-robot commented May 10, 2023

yastij commented May 10, 2023

yastij left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 10, 2023

yastij commented May 10, 2023

xiujuanx commented May 10, 2023

CecileRobertMichon commented May 10, 2023

k8s-infra-cherrypick-robot commented May 10, 2023

CecileRobertMichon left a comment

Choose a reason for hiding this comment

CecileRobertMichon May 10, 2023

Choose a reason for hiding this comment

jackfrancis commented May 10, 2023

sonasingh46 left a comment

Choose a reason for hiding this comment

willie-yao commented May 11, 2023

CecileRobertMichon commented May 11, 2023

k8s-ci-robot commented May 11, 2023

k8s-infra-cherrypick-robot commented May 11, 2023

xiujuanx commented May 10, 2023 •

edited

Loading