[IPv6] Fix reconcileFrontendIPConfigs() #3914

lzhecheng · 2023-05-19T07:34:50Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

[IPv6] Fix reconcileFrontendIPConfigs(). Current logic handles lb.FrontendIPConfigurations according to Service's IP family, which is incorrect. For an IPv6 cluster, there're still some IPv4 FIPs due to Azure limitation, which will be removed. For an IPv4 cluster, all resources are of IPv4, which is not affected.

Which issue(s) this PR fixes:

Fixes #4038

Special notes for your reviewer:

Does this PR introduce a user-facing change?

[IPv6] Fix reconcileFrontendIPConfigs(). Current logic handles lb.FrontendIPConfigurations according to Service's IP family, which is incorrect. For an IPv6 cluster, there're still some IPv4 FIPs due to Azure limitation, which will be removed. For an IPv4 cluster, all resources are of IPv4, which is not affected.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

netlify · 2023-05-19T07:34:56Z

✅ Deploy Preview for kubernetes-sigs-cloud-provide-azure canceled.

Name	Link
🔨 Latest commit	`df99c1c`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cloud-provide-azure/deploys/647daa8e3d64720008e43471

k8s-ci-robot · 2023-05-19T07:35:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lzhecheng

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~~ [lzhecheng]

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

lzhecheng · 2023-05-19T09:04:50Z

/test pull-cloud-provider-azure-e2e-ccm-ipv6-capz

lzhecheng · 2023-05-19T12:02:47Z

/test pull-cloud-provider-azure-e2e-ccm-ipv6-capz
/test pull-cloud-provider-azure-e2e-ccm-ipv6-vmss-capz

pkg/provider/azure_utils.go

pkg/provider/azure_loadbalancer.go

nilo19 · 2023-05-22T09:11:47Z

pkg/provider/azure_loadbalancer.go

 	handleFrontendIPConfig := func(isIPv6 bool) error {
-		ownedFIPConfig, toDeleteConfigsSingleStack, changed, newConfigs, err := az.reconcileFrontendIPConfigsSingleStack(clusterName, service, lb, lbStatus, wantLb, isIPv6, lbFrontendIPConfigName[isIPv6], &subnet)
+		ownedFIPConfig, toDeleteConfigsSingleStack, changed, newConfigs, err := az.reconcileFrontendIPConfigsSingleStack(clusterName, service, lb, lbStatus, wantLb, isIPv6, newConfigs[isIPv6], lbFrontendIPConfigName[isIPv6], &subnet)


Since we have had lb here in the parameters, why bother to build a list fip configs outside of the function?

Because vXEnabled -> handleFrontendIPConfig() -> reconcileFrontendIPConfigsSingleStack().
If the Service is single stack, we need to handle the newConfigs of the other IP family outside.

lzhecheng · 2023-05-25T02:36:49Z

/test pull-cloud-provider-azure-e2e-ccm-ipv6-capz
/test pull-cloud-provider-azure-e2e-ccm-ipv6-vmss-capz

jwtty · 2023-05-25T04:13:06Z

LGTM

nilo19 · 2023-05-26T02:58:10Z

pkg/provider/azure_utils.go

 		}
-		if pip.PublicIPAddressPropertiesFormat != nil {
+		if exist && pip.PublicIPAddressPropertiesFormat != nil {


Is it possible that the ipv6 public IP has not yet been created?

It won't. One evidence is that the warning in isFIPIPv6() shouldn't appear in CCM log: "Checking IP Family of frontend IP configuration %q but it is not clear. It's considered to be IPv4".

nilo19 · 2023-05-26T02:58:23Z

pkg/provider/azure_utils.go

 	}
+
+	klog.Errorf("Checking IP Family of frontend IP configuration %q but it is not clear. It's considered to be IPv4", pointer.StringDeref(fip.Name, ""))


It should be Warningf here.

nilo19 · 2023-05-26T07:15:45Z

/lgtm

lzhecheng · 2023-05-26T07:16:00Z

/hold
Will manually check ccm log after all tests finish to see if isFIPIPv6 is working properly.

lzhecheng · 2023-05-31T14:17:49Z

/retest

lzhecheng · 2023-06-02T01:11:04Z

/test pull-cloud-provider-azure-e2e-ccm-ipv6-capz
/test pull-cloud-provider-azure-e2e-ccm-ipv6-vmss-capz

lzhecheng · 2023-06-02T02:50:03Z

/test pull-cloud-provider-azure-e2e-ccm-ipv6-vmss-capz

lzhecheng · 2023-06-02T03:20:17Z

/test pull-cloud-provider-azure-e2e-ccm-vmss-capz

lzhecheng · 2023-06-02T06:05:52Z

/retest

lzhecheng · 2023-06-02T06:41:38Z

/retest

lzhecheng · 2023-06-02T14:10:38Z

/retest

nilo19 · 2023-06-05T01:16:09Z

pkg/provider/azure_loadbalancer_test.go

+		expectedDirty bool
+		expectedFIPs  []network.FrontendIPConfiguration
+	}{
+		{


can you add a test case for reconciling existing configs owned by the service without touching the others?

also add some negative cases please

nilo19 · 2023-06-05T01:34:19Z

pkg/provider/azure_utils.go

+	// First check private
+	// For an external LB, the FIP's private IP address is empty. The logic will
+	// go through this part and check public IP address then.
+	if fip.FrontendIPConfigurationPropertiesFormat != nil {


I checked all the refs of this function. If we can make sure the fip is always owned by the service, the pip check is not needed.

The FIP also needs to be isPrimaryService==true. If it is the secondary Service, I think the pip check is still needed.

If the fip is owned by a single stack service, we can check the service instead of pip, no matter if it is primary or secondary.

It makes sense for single stack svc. Updated.

feiskyer · 2023-06-05T04:49:12Z

Will manually check ccm log after all tests finish to see if isFIPIPv6 is working properly.

Have the test finished?

lzhecheng · 2023-06-05T05:03:47Z

/test pull-cloud-provider-azure-e2e-ccm-ipv6-capz
/test pull-cloud-provider-azure-e2e-ccm-ipv6-vmss-capz

@feiskyer the tests are manually triggered like above. Those runs before were successful

Signed-off-by: Zhecheng Li <zhechengli@microsoft.com>

lzhecheng · 2023-06-05T07:33:09Z

/test pull-cloud-provider-azure-e2e-ccm-ipv6-capz
/test pull-cloud-provider-azure-e2e-ccm-ipv6-vmss-capz

Current logic handles lb.FrontendIPConfigurations according to Service's IP family, which is incorrect. For an IPv6 cluster, there're still some IPv4 FIPs due to Azure limitation, which will be removed. For an IPv4 cluster, all resources are of IPv4, which is not affected. Signed-off-by: Zhecheng Li <zhechengli@microsoft.com>

lzhecheng · 2023-06-05T09:38:02Z

/test pull-cloud-provider-azure-e2e-ccm-ipv6-capz
/test pull-cloud-provider-azure-e2e-ccm-ipv6-vmss-capz

lzhecheng · 2023-06-05T23:14:48Z

/retest

MartinForReal · 2023-06-06T01:39:47Z

/lgtm

jwtty · 2023-06-06T01:47:43Z

pkg/provider/azure_loadbalancer.go

-			if err := az.fillSubnet(subnet, *subnetName); err != nil {
-				return false, err
+			if subnet == nil {
+				return false, fmt.Errorf("isFrontendIPChanged: Unexpected nil subnet")


nit: can we include subnet name in the error msg?

Let's do it in the next PR.

lzhecheng · 2023-06-06T02:22:36Z

/unhold

lzhecheng · 2023-06-06T06:46:26Z

/retest

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 19, 2023

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 19, 2023

k8s-ci-robot requested review from feiskyer and MartinForReal May 19, 2023 07:35

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

lzhecheng force-pushed the fix-ipv6-fip branch from 20dfd9c to 9a1491d Compare May 19, 2023 07:50

lzhecheng force-pushed the fix-ipv6-fip branch from 9a1491d to d462f8f Compare May 19, 2023 12:00

MartinForReal reviewed May 22, 2023

View reviewed changes

pkg/provider/azure_utils.go Show resolved Hide resolved

pkg/provider/azure_utils.go Outdated Show resolved Hide resolved

jwtty reviewed May 22, 2023

View reviewed changes

pkg/provider/azure_loadbalancer.go Outdated Show resolved Hide resolved

jwtty reviewed May 22, 2023

View reviewed changes

pkg/provider/azure_loadbalancer.go Outdated Show resolved Hide resolved

nilo19 reviewed May 22, 2023

View reviewed changes

lzhecheng force-pushed the fix-ipv6-fip branch 3 times, most recently from 66a7f31 to 1c4679d Compare May 22, 2023 23:40

nilo19 reviewed May 26, 2023

View reviewed changes

lzhecheng force-pushed the fix-ipv6-fip branch from 1c4679d to 0c775ac Compare May 26, 2023 07:14

k8s-ci-robot assigned nilo19 May 26, 2023

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

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 26, 2023

lzhecheng force-pushed the fix-ipv6-fip branch from 46c78e0 to 64c4f11 Compare June 2, 2023 00:49

nilo19 reviewed Jun 5, 2023

View reviewed changes

lzhecheng force-pushed the fix-ipv6-fip branch 2 times, most recently from 8169b11 to 98b7615 Compare June 5, 2023 02:51

lzhecheng mentioned this pull request Jun 5, 2023

Fix or reduce frequency or switch off perma-failing jobs #4038

Closed

[e2e] Adjust for IPv6 VMSS

3b6e917

Signed-off-by: Zhecheng Li <zhechengli@microsoft.com>

lzhecheng force-pushed the fix-ipv6-fip branch from 98b7615 to 5c41d5b Compare June 5, 2023 07:02

lzhecheng force-pushed the fix-ipv6-fip branch from 5c41d5b to df99c1c Compare June 5, 2023 09:27

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

jwtty reviewed Jun 6, 2023

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 6, 2023

k8s-ci-robot merged commit 9216589 into kubernetes-sigs:master Jun 6, 2023
23 checks passed

lzhecheng deleted the fix-ipv6-fip branch June 6, 2023 09:03

		}

		klog.Errorf("Checking IP Family of frontend IP configuration %q but it is not clear. It's considered to be IPv4", pointer.StringDeref(fip.Name, ""))

[IPv6] Fix reconcileFrontendIPConfigs() #3914

[IPv6] Fix reconcileFrontendIPConfigs() #3914

Conversation

lzhecheng commented May 19, 2023 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

netlify bot commented May 19, 2023 • edited

✅ Deploy Preview for kubernetes-sigs-cloud-provide-azure canceled.

k8s-ci-robot commented May 19, 2023

lzhecheng commented May 19, 2023

lzhecheng commented May 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzhecheng commented May 25, 2023

jwtty commented May 25, 2023

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

nilo19 commented May 26, 2023

lzhecheng commented May 26, 2023

lzhecheng commented May 31, 2023

lzhecheng commented Jun 2, 2023

lzhecheng commented Jun 2, 2023

lzhecheng commented Jun 2, 2023

lzhecheng commented Jun 2, 2023

lzhecheng commented Jun 2, 2023

lzhecheng commented Jun 2, 2023

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

feiskyer commented Jun 5, 2023

lzhecheng commented Jun 5, 2023

lzhecheng commented Jun 5, 2023

lzhecheng commented Jun 5, 2023

lzhecheng commented Jun 5, 2023

MartinForReal commented Jun 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzhecheng commented Jun 6, 2023

lzhecheng commented Jun 6, 2023

lzhecheng commented May 19, 2023 •

edited

netlify bot commented May 19, 2023 •

edited