Repository: kubernetes-sigs/cloud-provider-azure
Pull request #3823 (2023-04-27 06:30:23)

[DualStack] IPv6 PIP uses suffix only when DualStack
<!--  Thanks for sending a pull request!  Here are some tips for you:

1. If this is your first time, please read our contributor guidelines: https://git.k8s.io/community/contributors/guide/first-contribution.md#your-first-contribution and developer guide https://git.k8s.io/community/contributors/devel/development.md#development-guide
2. Please label this pull request according to what type of issue you are addressing, especially if this is a release targeted pull request. For reference on required PR/issue labels, read here:
https://git.k8s.io/community/contributors/devel/sig-release/release.md#issuepr-kind-label
3. Ensure you have added or ran the appropriate tests for your PR: https://git.k8s.io/community/contributors/devel/sig-testing/testing.md
4. If you want *faster* PR reviews, read how: https://git.k8s.io/community/contributors/guide/pull-requests.md#best-practices-for-faster-reviews
5. If the PR is unfinished, see how to mark it: https://git.k8s.io/community/contributors/guide/pull-requests.md#marking-unfinished-pull-requests
-->

#### What type of PR is this?
/kind cleanup
<!--
Add one of the following kinds:
/kind bug
/kind cleanup
/kind documentation
/kind feature
/kind design

Optionally add one or more of the following kinds if applicable:
/kind api-change
/kind deprecation
/kind failing-test
/kind flake
/kind regression
-->

#### What this PR does / why we need it:
[DualStack] IPv6 PIP uses suffix only when DualStack. For CCM v1.27.1, the IPv6 PIP created has suffix. After CCM is upgraded, such PIP will be recreated.
#### Which issue(s) this PR fixes:
<!--
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
_If PR is about `failing-tests or flakes`, please post the related issues/tests in a comment and do not use `Fixes`_*
-->
Fixes #

#### Special notes for your reviewer:

#### Does this PR introduce a user-facing change?
<!--
If no, just write "NONE" in the release-note block below.
If yes, a release note is required:
Enter your extended release note in the block below. If the PR requires additional action from users switching to the new release, include the string "action required".

For more information on release notes see: https://git.k8s.io/community/contributors/guide/release-notes.md
-->
```release-note
[DualStack] IPv6 PIP uses suffix only when DualStack. For CCM v1.27.1, the IPv6 PIP created has suffix. After CCM is upgraded, such PIP will be recreated.
```

#### Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

<!--
This section can be blank if this pull request does not require a release note.

When adding links which point to resources within git repositories, like
KEPs or supporting documentation, please reference a specific commit and avoid
linking directly to the master branch. This ensures that links reference a
specific point in time, rather than a document that may change over time.

See here for guidance on getting permanent links to files: https://help.github.com/en/articles/getting-permanent-links-to-files

Please use the following format for linking documentation:
- [KEP]: <link>
- [Usage]: <link>
- [Other doc]: <link>
-->
```docs

```


netlify[bot]: ### <span aria-hidden="true">👷</span> Deploy Preview for *kubernetes-sigs-cloud-provide-azure* processing.


|  Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 99139c8191821b005cdcfd0a140f770980b056e4 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/sites/kubernetes-sigs-cloud-provide-azure/deploys/644a168037c4e700085c95f4 |
k8s-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-sigs/cloud-provider-azure/pull/3823#" title="Author self-approved">lzhecheng</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Fcloud-provider-azure).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/kubernetes-sigs/cloud-provider-azure/blob/master/OWNERS)~~ [lzhecheng]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
k8s-ci-robot: @lzhecheng: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
pull-cloud-provider-azure-unit | 99139c8191821b005cdcfd0a140f770980b056e4 | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cloud-provider-azure/3823/pull-cloud-provider-azure-unit/1651473856201756672) | true | `/test pull-cloud-provider-azure-unit`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-sigs&repo=cloud-provider-azure&pr=3823). [Your PR dashboard](https://prow.k8s.io/pr?query=is%3Apr%20state%3Aopen%20author%3Alzhecheng). Please help us cut down on flakes by [linking to](https://git.k8s.io/community/contributors/devel/sig-testing/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/kubernetes-sigs/cloud-provider-azure/issues?q=is:issue+is:open) when you hit one in your PR.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
k8s-ci-robot: @lzhecheng: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
pull-cloud-provider-azure-unit | 09397b00ba3aeec7019425685c8d782d41aa4190 | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cloud-provider-azure/3823/pull-cloud-provider-azure-unit/1651477503673372672) | true | `/test pull-cloud-provider-azure-unit`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-sigs&repo=cloud-provider-azure&pr=3823). [Your PR dashboard](https://prow.k8s.io/pr?query=is%3Apr%20state%3Aopen%20author%3Alzhecheng). Please help us cut down on flakes by [linking to](https://git.k8s.io/community/contributors/devel/sig-testing/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/kubernetes-sigs/cloud-provider-azure/issues?q=is:issue+is:open) when you hit one in your PR.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->

Patch:
diff --git a/pkg/provider/azure_loadbalancer.go b/pkg/provider/azure_loadbalancer.go
index 60ee2ffac..9386bb4b9 100644
--- a/pkg/provider/azure_loadbalancer.go
+++ b/pkg/provider/azure_loadbalancer.go
@@ -869,7 +869,7 @@ func (az *Cloud) determinePublicIPName(clusterName string, service *v1.Service,
 
 	pipResourceGroup := az.getPublicIPAddressResourceGroup(service)
 	if id := getServicePIPPrefixID(service, isIPv6); id != "" {
-		pipName, err := az.getPublicIPName(clusterName, service, pipResourceGroup, isIPv6)
+		pipName, err := az.getPublicIPName(clusterName, service, isIPv6)
 		return pipName, false, err
 	}
 
@@ -878,7 +878,7 @@ func (az *Cloud) determinePublicIPName(clusterName string, service *v1.Service,
 	// Assume that the service without loadBalancerIP set is a primary service.
 	// If a secondary service doesn't set the loadBalancerIP, it is not allowed to share the IP.
 	if len(loadBalancerIP) == 0 {
-		pipName, err := az.getPublicIPName(clusterName, service, pipResourceGroup, isIPv6)
+		pipName, err := az.getPublicIPName(clusterName, service, isIPv6)
 		return pipName, false, err
 	}
 
@@ -896,24 +896,6 @@ func (az *Cloud) determinePublicIPName(clusterName string, service *v1.Service,
 	return "", false, fmt.Errorf("user supplied IP Address %s was not found in resource group %s", loadBalancerIP, pipResourceGroup)
 }
 
-// findMatchedPIPByIPFamilyAndServiceName is for IPv6. It tries to find a matching PIP whose name doesn't have IPv6 suffix.
-// Such PIP comes from old CCM without dual-stack support.
-func (az *Cloud) findMatchedPIPByIPFamilyAndServiceName(clusterName string, service *v1.Service, pipResourceGroup string) (*network.PublicIPAddress, error) {
-	pips, err := az.listPIP(pipResourceGroup)
-	if err != nil {
-		return nil, fmt.Errorf("findMatchedPIPByIPFamilyAndServiceName: failed to listPIP: %w", err)
-	}
-	baseName := az.GetLoadBalancerName(context.TODO(), clusterName, service)
-	for _, pip := range pips {
-		pip := pip
-		if pointer.StringDeref(pip.Name, "") == baseName && pip.PublicIPAddressPropertiesFormat != nil &&
-			pip.PublicIPAddressPropertiesFormat.PublicIPAddressVersion == network.IPv6 {
-			return &pip, nil
-		}
-	}
-	return nil, nil
-}
-
 func (az *Cloud) findMatchedPIPByLoadBalancerIP(service *v1.Service, loadBalancerIP, pipResourceGroup string) (*network.PublicIPAddress, error) {
 	pips, err := az.listPIP(pipResourceGroup)
 	if err != nil {
@@ -2984,6 +2966,8 @@ func shouldReleaseExistingOwnedPublicIP(existingPip *network.PublicIPAddress, lb
 		// #2 - If the load balancer is internal, and thus doesn't require public exposure
 		lbIsInternal ||
 		// #3 - If the name of this public ip does not match the desired name,
+		// NOTICE: For IPv6 Service created with CCM v1.27.1, the created PIP has IPv6 suffix.
+		// We need to recreate such PIP and current logic to delete needs no change.
 		(pipName != desiredPipName) ||
 		// #4 If the service annotations have specified the ip tags that the public ip must have, but they do not match the ip tags of the existing instance
 		(ipTagRequest.IPTagsRequestedByAnnotation && !areIPTagsEquivalent(currentIPTags, ipTagRequest.IPTags))
diff --git a/pkg/provider/azure_standard.go b/pkg/provider/azure_standard.go
index b3ed6e488..d8c9d43c7 100644
--- a/pkg/provider/azure_standard.go
+++ b/pkg/provider/azure_standard.go
@@ -326,18 +326,8 @@ func (az *Cloud) getRulePrefix(service *v1.Service) string {
 	return az.GetLoadBalancerName(context.TODO(), "", service)
 }
 
-func (az *Cloud) getPublicIPName(clusterName string, service *v1.Service, pipResourceGroup string, isIPv6 bool) (string, error) {
-	// For IPv6, the PIP may not have IPv6 suffix because it may be one created before dual-stack support.
-	if isIPv6 {
-		pip, err := az.findMatchedPIPByIPFamilyAndServiceName(clusterName, service, pipResourceGroup)
-		if err != nil {
-			return "", err
-		}
-		if pip != nil {
-			return pointer.StringDeref(pip.Name, ""), nil
-		}
-	}
-
+func (az *Cloud) getPublicIPName(clusterName string, service *v1.Service, isIPv6 bool) (string, error) {
+	isDualStack := isServiceDualStack(service)
 	pipName := fmt.Sprintf("%s-%s", clusterName, az.GetLoadBalancerName(context.TODO(), clusterName, service))
 	if id := getServicePIPPrefixID(service, isIPv6); id != "" {
 		id, err := getLastSegment(id, "/")
@@ -345,7 +335,7 @@ func (az *Cloud) getPublicIPName(clusterName string, service *v1.Service, pipRes
 			pipName = fmt.Sprintf("%s-%s", pipName, id)
 		}
 	}
-	return getResourceByIPFamily(pipName, isIPv6), nil
+	return getResourceByIPFamily(pipName, isDualStack, isIPv6), nil
 }
 
 // TODO: UT
diff --git a/pkg/provider/azure_utils.go b/pkg/provider/azure_utils.go
index c9dd45b78..4fdc8d4b2 100644
--- a/pkg/provider/azure_utils.go
+++ b/pkg/provider/azure_utils.go
@@ -435,8 +435,12 @@ func getServicePIPPrefixID(service *v1.Service, isIPv6 bool) string {
 	return service.Annotations[consts.ServiceAnnotationPIPPrefixIDDualStack[isIPv6]]
 }
 
-func getResourceByIPFamily(resource string, isIPv6 bool) string {
-	if isIPv6 {
+// getResourceByIPFamily returns the resource name of with IPv6 suffix when
+// it is a dual-stack Service and the resource is of IPv6.
+// NOTICE: For PIPs of IPv6 Services created with CCM v1.27.1, after the CCM is upgraded,
+// the old PIPs will be recreated.
+func getResourceByIPFamily(resource string, isDualStack, isIPv6 bool) string {
+	if isDualStack && isIPv6 {
 		return fmt.Sprintf("%s-%s", resource, v6Suffix)
 	}
 	return resource


Test patch:
diff --git a/pkg/provider/azure_loadbalancer_test.go b/pkg/provider/azure_loadbalancer_test.go
index 7275d81ce..8bb54109f 100644
--- a/pkg/provider/azure_loadbalancer_test.go
+++ b/pkg/provider/azure_loadbalancer_test.go
@@ -100,7 +100,7 @@ func TestExistsPip(t *testing.T) {
 			func(client *mockpublicipclient.MockInterface) {
 				pips := []network.PublicIPAddress{
 					{
-						Name: pointer.String("testCluster-aservice-IPv6"),
+						Name: pointer.String("testCluster-aservice"),
 						PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
 							IPAddress: pointer.String("fe::1"),
 						},
@@ -110,6 +110,22 @@ func TestExistsPip(t *testing.T) {
 			},
 			true,
 		},
+		{
+			"IPv6 not exists - should not have suffix",
+			getTestService("service", v1.ProtocolTCP, nil, true, 80),
+			func(client *mockpublicipclient.MockInterface) {
+				pips := []network.PublicIPAddress{
+					{
+						Name: pointer.String("testCluster-aservice-IPv6"),
+						PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
+							IPAddress: pointer.String("fe::1"),
+						},
+					},
+				}
+				client.EXPECT().List(gomock.Any(), "rg").Return(pips, nil).MaxTimes(2)
+			},
+			false,
+		},
 		{
 			"DualStack exists",
 			getTestServiceDualStack("service", v1.ProtocolTCP, nil, 80),
@@ -1129,6 +1145,8 @@ func TestShouldReleaseExistingOwnedPublicIP(t *testing.T) {
 			},
 		},
 	}
+	existingPipWithTagIPv6Suffix := existingPipWithTag
+	existingPipWithTagIPv6Suffix.Name = pointer.String("testPIP-IPv6")
 
 	existingPipWithNoPublicIPAddressFormatProperties := network.PublicIPAddress{
 		ID:                              pointer.String("/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Network/publicIPAddresses/testPIP"),
@@ -1237,6 +1255,25 @@ func TestShouldReleaseExistingOwnedPublicIP(t *testing.T) {
 			},
 			expectedShouldRelease: true,
 		},
+		{
+			// This test is for IPv6 PIP created with CCM v1.27.1 and CCM gets upgraded.
+			// Such PIP should be recreated.
+			desc:           "matching except PIP name (with IPv6 suffix), expect release",
+			existingPip:    existingPipWithTagIPv6Suffix,
+			lbShouldExist:  true,
+			lbIsInternal:   false,
+			desiredPipName: *existingPipWithTag.Name,
+			ipTagRequest: serviceIPTagRequest{
+				IPTagsRequestedByAnnotation: true,
+				IPTags: &[]network.IPTag{
+					{
+						IPTagType: pointer.String("tag1"),
+						Tag:       pointer.String("tag1value"),
+					},
+				},
+			},
+			expectedShouldRelease: true,
+		},
 		{
 			desc:           "should delete orphaned managed public IP",
 			existingPip:    existingPipWithTag,
@@ -1277,13 +1314,15 @@ func TestShouldReleaseExistingOwnedPublicIP(t *testing.T) {
 		},
 	}
 
-	for i, c := range tests {
-		if c.tags != nil {
-			c.existingPip.Tags = c.tags
-		}
-		existingPip := c.existingPip
-		actualShouldRelease := shouldReleaseExistingOwnedPublicIP(&existingPip, c.lbShouldExist, c.lbIsInternal, c.isUserAssignedPIP, c.desiredPipName, c.ipTagRequest)
-		assert.Equal(t, c.expectedShouldRelease, actualShouldRelease, "TestCase[%d]: %s", i, c.desc)
+	for _, c := range tests {
+		t.Run(c.desc, func(t *testing.T) {
+			if c.tags != nil {
+				c.existingPip.Tags = c.tags
+			}
+			existingPip := c.existingPip
+			actualShouldRelease := shouldReleaseExistingOwnedPublicIP(&existingPip, c.lbShouldExist, c.lbIsInternal, c.isUserAssignedPIP, c.desiredPipName, c.ipTagRequest)
+			assert.Equal(t, c.expectedShouldRelease, actualShouldRelease)
+		})
 	}
 }
 
diff --git a/pkg/provider/azure_standard_test.go b/pkg/provider/azure_standard_test.go
index 1945b20d2..97a00032c 100644
--- a/pkg/provider/azure_standard_test.go
+++ b/pkg/provider/azure_standard_test.go
@@ -2378,7 +2378,6 @@ func TestGetPublicIPName(t *testing.T) {
 					IPFamilies: []v1.IPFamily{v1.IPv4Protocol, v1.IPv6Protocol},
 				},
 			},
-			pips:            []network.PublicIPAddress{},
 			isIPv6:          true,
 			expectedPIPName: "azure-auid-prefix-id-ipv6-IPv6",
 		},
@@ -2392,30 +2391,30 @@ func TestGetPublicIPName(t *testing.T) {
 					IPFamilies: []v1.IPFamily{v1.IPv6Protocol},
 				},
 			},
-			pips: []network.PublicIPAddress{
-				{
-					Name: pointer.String("auid"),
-					PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{
-						PublicIPAddressVersion: network.IPv6,
-					},
+			isIPv6:          true,
+			expectedPIPName: "azure-auid",
+		},
+		{
+			desc: "Service PIP IPv6 only with existing PIP - dualstack",
+			svc: &v1.Service{
+				ObjectMeta: meta.ObjectMeta{
+					UID: types.UID("uid"),
+				},
+				Spec: v1.ServiceSpec{
+					IPFamilies: []v1.IPFamily{v1.IPv4Protocol, v1.IPv6Protocol},
 				},
 			},
 			isIPv6:          true,
-			expectedPIPName: "auid",
+			expectedPIPName: "azure-auid-IPv6",
 		},
 	}
 
 	ctrl := gomock.NewController(t)
 	defer ctrl.Finish()
-	rg := "rg0"
 	for _, tc := range testcases {
 		t.Run(tc.desc, func(t *testing.T) {
 			az := GetTestCloud(ctrl)
-			if tc.isIPv6 {
-				mockPIPsClient := az.PublicIPAddressesClient.(*mockpublicipclient.MockInterface)
-				mockPIPsClient.EXPECT().List(gomock.Any(), rg).Return(tc.pips, nil)
-			}
-			name, err := az.getPublicIPName("azure", tc.svc, rg, tc.isIPv6)
+			name, err := az.getPublicIPName("azure", tc.svc, tc.isIPv6)
 			assert.Nil(t, err)
 			assert.Equal(t, tc.expectedPIPName, name)
 		})
diff --git a/pkg/provider/azure_utils_test.go b/pkg/provider/azure_utils_test.go
index 008d60b12..4216fdf44 100644
--- a/pkg/provider/azure_utils_test.go
+++ b/pkg/provider/azure_utils_test.go
@@ -856,24 +856,41 @@ func TestGetResourceByIPFamily(t *testing.T) {
 		desc             string
 		resource         string
 		isIPv6           bool
+		isDualStack      bool
 		expectedResource string
 	}{
 		{
-			"IPv4",
+			"DualStack - IPv4",
 			"resource0",
 			false,
+			true,
 			"resource0",
 		},
 		{
-			"IPv6",
+			"DualStack - IPv6",
 			"resource0",
 			true,
+			true,
 			"resource0-IPv6",
 		},
+		{
+			"SingleStack - IPv4",
+			"resource0",
+			false,
+			false,
+			"resource0",
+		},
+		{
+			"SingleStack - IPv6",
+			"resource0",
+			true,
+			false,
+			"resource0",
+		},
 	}
 	for _, tc := range testcases {
 		t.Run(tc.desc, func(t *testing.T) {
-			resource := getResourceByIPFamily(tc.resource, tc.isIPv6)
+			resource := getResourceByIPFamily(tc.resource, tc.isDualStack, tc.isIPv6)
 			assert.Equal(t, tc.expectedResource, resource)
 		})
 	}