Repository: kubernetes-sigs/cloud-provider-azure
Pull request #2911 (2022-12-12 01:31:29)

[release-1.24] Fix k8s-azure-dns-label-service tag not deleted with Service

Signed-off-by: Zhecheng Li <zhechengli@microsoft.com>

<!--  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 bug
<!--
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:
Fix k8s-azure-dns-label-service tag not deleted with Service
Bug: When a Service with DNS label is deleted, k8s-azure-dns-label-service tag won't be deleted and it leads to ensurePublicIPExists error.

#### 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
Fix k8s-azure-dns-label-service tag not deleted with Service. Bug: When a Service with DNS label is deleted, k8s-azure-dns-label-service tag won't be deleted and it leads to ensurePublicIPExists error.

```

#### 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

```


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/2911#" 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/release-1.24/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":[]} -->
coveralls: 
[![Coverage Status](https://coveralls.io/builds/55049832/badge)](https://coveralls.io/builds/55049832)

Coverage decreased (-0.02%) to 80.005% when pulling **611caac96f52d4002dfd4ac36dac1d8a2514374c on lzhecheng:cp-2842-24** into **1fe38c442a5af8956768e8d3bb8176475c6a8bd7 on kubernetes-sigs:release-1.24**.

lzhecheng: /retest

Patch:
diff --git a/pkg/provider/azure_loadbalancer.go b/pkg/provider/azure_loadbalancer.go
index d67412cce..3bd29159a 100644
--- a/pkg/provider/azure_loadbalancer.go
+++ b/pkg/provider/azure_loadbalancer.go
@@ -1223,6 +1223,11 @@ func getServiceFromPIPDNSTags(tags map[string]*string) string {
 	return ""
 }
 
+func deleteServicePIPDNSTags(tags *map[string]*string) {
+	delete(*tags, consts.ServiceUsingDNSKey)
+	delete(*tags, consts.LegacyServiceUsingDNSKey)
+}
+
 func getServiceFromPIPServiceTags(tags map[string]*string) string {
 	v, ok := tags[consts.ServiceTagKey]
 	if ok && v != nil {
@@ -3123,10 +3128,9 @@ func (az *Cloud) getPublicIPUpdates(
 		owns, isUserAssignedPIP := serviceOwnsPublicIP(service, &pip, clusterName)
 		if owns {
 			var dirtyPIP, toBeDeleted bool
-			if !wantLb && !isUserAssignedPIP {
+			if !wantLb {
 				klog.V(2).Infof("reconcilePublicIP for service(%s): unbinding the service from pip %s", serviceName, *pip.Name)
-				err = unbindServiceFromPIP(&pip, service, serviceName, clusterName)
-				if err != nil {
+				if err = unbindServiceFromPIP(&pip, service, serviceName, clusterName, isUserAssignedPIP); err != nil {
 					return false, nil, false, nil, err
 				}
 				dirtyPIP = true
@@ -3598,11 +3602,19 @@ func bindServicesToPIP(pip *network.PublicIPAddress, incomingServiceNames []stri
 	return addedNew, nil
 }
 
-func unbindServiceFromPIP(pip *network.PublicIPAddress, service *v1.Service, serviceName, clusterName string) error {
+func unbindServiceFromPIP(pip *network.PublicIPAddress, service *v1.Service,
+	serviceName, clusterName string, isUserAssignedPIP bool) error {
 	if pip == nil || pip.Tags == nil {
 		return fmt.Errorf("nil public IP or tags")
 	}
 
+	if existingServiceName := getServiceFromPIPDNSTags(pip.Tags); existingServiceName != "" && strings.EqualFold(existingServiceName, serviceName) {
+		deleteServicePIPDNSTags(&pip.Tags)
+	}
+	if isUserAssignedPIP {
+		return nil
+	}
+
 	// skip removing tags for user assigned pips
 	serviceTagValue := to.StringPtr(getServiceFromPIPServiceTags(pip.Tags))
 	existingServiceNames := parsePIPServiceTag(serviceTagValue)
@@ -3611,6 +3623,7 @@ func unbindServiceFromPIP(pip *network.PublicIPAddress, service *v1.Service, ser
 		if strings.EqualFold(existingServiceNames[i], serviceName) {
 			existingServiceNames = append(existingServiceNames[:i], existingServiceNames[i+1:]...)
 			found = true
+			break
 		}
 	}
 	if !found {
@@ -3618,15 +3631,7 @@ func unbindServiceFromPIP(pip *network.PublicIPAddress, service *v1.Service, ser
 	}
 
 	_, err := bindServicesToPIP(pip, existingServiceNames, true)
-	if err != nil {
-		return err
-	}
-
-	if existingServiceName := getServiceFromPIPDNSTags(pip.Tags); existingServiceName != "" && strings.EqualFold(existingServiceName, serviceName) {
-		pip.Tags[consts.ServiceUsingDNSKey] = to.StringPtr("")
-	}
-
-	return nil
+	return err
 }
 
 // ensureLoadBalancerTagged ensures every load balancer in the resource group is tagged as configured


Test patch:
diff --git a/pkg/provider/azure_loadbalancer_test.go b/pkg/provider/azure_loadbalancer_test.go
index 2269dd9d7..a7298f0d5 100644
--- a/pkg/provider/azure_loadbalancer_test.go
+++ b/pkg/provider/azure_loadbalancer_test.go
@@ -4877,7 +4877,7 @@ func TestUnbindServiceFromPIP(t *testing.T) {
 	}
 
 	for i, pip := range pips {
-		_ = unbindServiceFromPIP(pip, &service, serviceName, "")
+		_ = unbindServiceFromPIP(pip, &service, serviceName, "", false)
 		assert.Equal(t, expectedTags[i], pip.Tags)
 	}
 }
diff --git a/tests/e2e/network/ensureloadbalancer.go b/tests/e2e/network/ensureloadbalancer.go
index 5004b00d2..ef9417290 100644
--- a/tests/e2e/network/ensureloadbalancer.go
+++ b/tests/e2e/network/ensureloadbalancer.go
@@ -733,7 +733,7 @@ var _ = Describe("EnsureLoadBalancer should not update any resources when servic
 		service, err := cs.CoreV1().Services(ns.Name).Get(context.TODO(), testServiceName, metav1.GetOptions{})
 		defer func() {
 			By("Cleaning up")
-			err := utils.DeleteServiceIfExists(cs, ns.Name, testServiceName)
+			err := utils.DeleteService(cs, ns.Name, testServiceName)
 			Expect(err).NotTo(HaveOccurred())
 		}()
 		Expect(err).NotTo(HaveOccurred())
@@ -775,7 +775,7 @@ var _ = Describe("EnsureLoadBalancer should not update any resources when servic
 		service, err = cs.CoreV1().Services(ns.Name).Get(context.TODO(), testServiceName, metav1.GetOptions{})
 		defer func() {
 			By("Cleaning up")
-			err := utils.DeleteServiceIfExists(cs, ns.Name, testServiceName)
+			err := utils.DeleteService(cs, ns.Name, testServiceName)
 			Expect(err).NotTo(HaveOccurred())
 		}()
 		Expect(err).NotTo(HaveOccurred())
@@ -814,7 +814,7 @@ var _ = Describe("EnsureLoadBalancer should not update any resources when servic
 		service, err = cs.CoreV1().Services(ns.Name).Get(context.TODO(), testServiceName, metav1.GetOptions{})
 		defer func() {
 			By("Cleaning up")
-			err := utils.DeleteServiceIfExists(cs, ns.Name, testServiceName)
+			err := utils.DeleteService(cs, ns.Name, testServiceName)
 			Expect(err).NotTo(HaveOccurred())
 		}()
 		Expect(err).NotTo(HaveOccurred())
@@ -848,7 +848,7 @@ var _ = Describe("EnsureLoadBalancer should not update any resources when servic
 		service, err := cs.CoreV1().Services(ns.Name).Get(context.TODO(), testServiceName, metav1.GetOptions{})
 		defer func() {
 			By("Cleaning up")
-			err := utils.DeleteServiceIfExists(cs, ns.Name, testServiceName)
+			err := utils.DeleteService(cs, ns.Name, testServiceName)
 			Expect(err).NotTo(HaveOccurred())
 		}()
 		Expect(err).NotTo(HaveOccurred())
diff --git a/tests/e2e/network/network_security_group.go b/tests/e2e/network/network_security_group.go
index f4cd83421..d35528ce0 100644
--- a/tests/e2e/network/network_security_group.go
+++ b/tests/e2e/network/network_security_group.go
@@ -93,7 +93,7 @@ var _ = Describe("Network security group", Label(utils.TestSuiteLabelNSG), func(
 		ip := createAndExposeDefaultServiceWithAnnotation(cs, serviceName, ns.Name, labels, map[string]string{}, ports)
 		defer func() {
 			By("Cleaning up")
-			err := utils.DeleteServiceIfExists(cs, ns.Name, serviceName)
+			err := utils.DeleteService(cs, ns.Name, serviceName)
 			Expect(err).NotTo(HaveOccurred())
 		}()
 
@@ -143,7 +143,7 @@ var _ = Describe("Network security group", Label(utils.TestSuiteLabelNSG), func(
 		ip1 := createAndExposeDefaultServiceWithAnnotation(cs, serviceName, ns.Name, labels, annotation, ports)
 
 		defer func() {
-			err := utils.DeleteServiceIfExists(cs, ns.Name, serviceName)
+			err := utils.DeleteService(cs, ns.Name, serviceName)
 			Expect(err).NotTo(HaveOccurred())
 		}()
 
@@ -151,7 +151,7 @@ var _ = Describe("Network security group", Label(utils.TestSuiteLabelNSG), func(
 		ip2 := createAndExposeDefaultServiceWithAnnotation(cs, serviceName2, ns.Name, labels, annotation, ports)
 		defer func() {
 			By("Cleaning up")
-			err := utils.DeleteServiceIfExists(cs, ns.Name, serviceName2)
+			err := utils.DeleteService(cs, ns.Name, serviceName2)
 			Expect(err).NotTo(HaveOccurred())
 		}()
 
diff --git a/tests/e2e/network/service_annotations.go b/tests/e2e/network/service_annotations.go
index 4da6278ad..91369d128 100644
--- a/tests/e2e/network/service_annotations.go
+++ b/tests/e2e/network/service_annotations.go
@@ -115,9 +115,15 @@ var _ = Describe("Service with annotation", Label(utils.TestSuiteLabelServiceAnn
 	})
 
 	It("should support service annotation 'service.beta.kubernetes.io/azure-dns-label-name'", func() {
-		By("Create service")
-		serviceDomainNamePrefix := serviceName + string(uuid.NewUUID())
-
+		// This test creates/deletes/updates some Services:
+		// 1. Create a Service with managed PIP and check connectivity with DNS
+		// 2. Delete the Service
+		// 3. Create a Service with user assigned PIP
+		// 4. Delete the Servcie and check tags
+		// 5. Create a Service with different name
+		// 6. Update the Service with new tag
+		By("Create a Service with managed PIP")
+		serviceDomainNamePrefix := fmt.Sprintf("%s-%s", serviceName, uuid.NewUUID())
 		annotation := map[string]string{
 			consts.ServiceAnnotationDNSLabelName: serviceDomainNamePrefix,
 		}
@@ -137,19 +143,74 @@ var _ = Describe("Service with annotation", Label(utils.TestSuiteLabelServiceAnn
 			err = utils.DeletePod(cs, ns.Name, agnhostPod)
 			Expect(err).NotTo(HaveOccurred())
 		}()
-		Expect(result).To(BeTrue())
 		Expect(err).NotTo(HaveOccurred())
+		Expect(result).To(BeTrue())
 
 		serviceDomainName := utils.GetServiceDomainName(serviceDomainNamePrefix)
 		By(fmt.Sprintf("Validating External domain name %q", serviceDomainName))
 		err = utils.ValidateServiceConnectivity(ns.Name, agnhostPod, serviceDomainName, int(ports[0].Port), v1.ProtocolTCP)
 		Expect(err).NotTo(HaveOccurred(), "Fail to get response from the domain name")
 
+		By("Delete the Service")
+		err = utils.DeleteService(cs, ns.Name, serviceName)
+		Expect(err).NotTo(HaveOccurred())
+
+		By("Create a PIP")
+		ipName := fmt.Sprintf("%s-public-IP-%s", basename, uuid.NewUUID()[0:4])
+		nsName := ns.Name
+		rgName := tc.GetResourceGroup()
+		pip, err := utils.WaitCreatePIP(tc, ipName, rgName, defaultPublicIPAddress(ipName, tc.IPFamily == utils.IPv6))
+		defer func() {
+			err := utils.DeletePIPWithRetry(tc, ipName, rgName)
+			Expect(err).NotTo(HaveOccurred())
+		}()
+		Expect(err).NotTo(HaveOccurred())
+		targetIP := to.String(pip.IPAddress)
+		pipName := to.String(pip.Name)
+		utils.Logf("PIP %q to %q", pipName, targetIP)
+
+		By("Create a Service which will be deleted with the PIP")
+		oldServiceName := fmt.Sprintf("%s-old", serviceName)
+		service := utils.CreateLoadBalancerServiceManifest(oldServiceName, annotation, labels, nsName, ports)
+		service = updateServiceLBIP(service, false, targetIP)
+
+		// create service with given annotation and wait it to expose
+		_, err = cs.CoreV1().Services(nsName).Create(context.TODO(), service, metav1.CreateOptions{})
+		defer func() {
+			utils.Logf("Delete test Service %q", oldServiceName)
+			err := utils.DeleteService(cs, ns.Name, oldServiceName)
+			Expect(err).NotTo(HaveOccurred())
+		}()
+		Expect(err).NotTo(HaveOccurred())
+		_, err = utils.WaitServiceExposureAndValidateConnectivity(cs, nsName, oldServiceName, targetIP)
+		Expect(err).NotTo(HaveOccurred())
+
+		By("Delete the old Service")
+		err = utils.DeleteService(cs, ns.Name, oldServiceName)
+		Expect(err).NotTo(HaveOccurred())
+
+		By("Check if PIP DNS label is deleted")
+		deleted, err := ifPIPDNSLabelDeleted(tc, pipName)
+		Expect(err).NotTo(HaveOccurred())
+		Expect(deleted).To(BeTrue())
+
+		By("Create a different Service with the same azure-dns-label-name tag")
+		service.Name = serviceName
+		_, err = cs.CoreV1().Services(nsName).Create(context.TODO(), service, metav1.CreateOptions{})
+		defer func() {
+			utils.Logf("Delete test Service %q", serviceName)
+			err := utils.DeleteService(cs, ns.Name, serviceName)
+			Expect(err).NotTo(HaveOccurred())
+		}()
+		Expect(err).NotTo(HaveOccurred())
+
+		serviceDomainName = utils.GetServiceDomainName(serviceDomainNamePrefix)
+		By(fmt.Sprintf("Validating External domain name %q", serviceDomainName))
+		err = utils.ValidateServiceConnectivity(ns.Name, agnhostPod, serviceDomainName, int(ports[0].Port), v1.ProtocolTCP)
+		Expect(err).NotTo(HaveOccurred(), "Fail to get response from the domain name")
+
 		By("Update service")
-		annotation = map[string]string{
-			consts.ServiceAnnotationDNSLabelName: serviceDomainNamePrefix + "new",
-		}
-		service := utils.CreateLoadBalancerServiceManifest(serviceName, annotation, labels, ns.Name, ports)
+		service.Annotations[consts.ServiceAnnotationDNSLabelName] = fmt.Sprintf("%s-new", serviceDomainNamePrefix)
 		_, err = cs.CoreV1().Services(ns.Name).Update(context.TODO(), service, metav1.UpdateOptions{})
 		Expect(err).NotTo(HaveOccurred())
 
@@ -325,7 +386,7 @@ var _ = Describe("Service with annotation", Label(utils.TestSuiteLabelServiceAnn
 
 		defer func() {
 			By("Cleaning up test service")
-			err := utils.DeleteServiceIfExists(cs, ns.Name, serviceName)
+			err := utils.DeleteService(cs, ns.Name, serviceName)
 			Expect(err).NotTo(HaveOccurred())
 		}()
 
@@ -392,7 +453,7 @@ var _ = Describe("Service with annotation", Label(utils.TestSuiteLabelServiceAnn
 		_, err = cs.CoreV1().Services(ns.Name).Create(context.TODO(), service, metav1.CreateOptions{})
 		defer func() {
 			By("Cleaning up test service")
-			err := utils.DeleteServiceIfExists(cs, ns.Name, serviceName)
+			err := utils.DeleteService(cs, ns.Name, serviceName)
 			Expect(err).NotTo(HaveOccurred())
 		}()
 		Expect(err).NotTo(HaveOccurred())
@@ -450,7 +511,7 @@ var _ = Describe("Service with annotation", Label(utils.TestSuiteLabelServiceAnn
 
 		defer func() {
 			By("Cleaning up test service")
-			Expect(utils.DeleteServiceIfExists(cs, ns.Name, serviceName)).NotTo(HaveOccurred())
+			Expect(utils.DeleteService(cs, ns.Name, serviceName)).NotTo(HaveOccurred())
 		}()
 
 		By("Waiting for the service to expose")
@@ -970,6 +1031,26 @@ func waitComparePIPTags(tc *utils.AzureTestClient, expectedTags map[string]*stri
 	return err
 }
 
+func ifPIPDNSLabelDeleted(tc *utils.AzureTestClient, pipName string) (bool, error) {
+	if err := wait.PollImmediate(10*time.Second, 2*time.Minute, func() (done bool, err error) {
+		pip, err := utils.WaitGetPIP(tc, pipName)
+		if err != nil {
+			return false, err
+		}
+		keyDeleted, legacyKeyDeleted := false, false
+		if name, ok := pip.Tags[consts.ServiceUsingDNSKey]; !ok || name == nil {
+			keyDeleted = true
+		}
+		if name, ok := pip.Tags[consts.LegacyServiceUsingDNSKey]; !ok || name == nil {
+			legacyKeyDeleted = true
+		}
+		return keyDeleted && legacyKeyDeleted, nil
+	}); err != nil {
+		return false, err
+	}
+	return true, nil
+}
+
 func getPIPFrontendConfigurationID(tc *utils.AzureTestClient, pip, pipResourceGroup, lbResourceGroup string) string {
 	utils.Logf("Getting public IPs in the resourceGroup " + pipResourceGroup)
 	pipList, err := tc.ListPublicIPs(pipResourceGroup)
diff --git a/tests/e2e/utils/service_utils.go b/tests/e2e/utils/service_utils.go
index 9eecaed83..0783333b8 100644
--- a/tests/e2e/utils/service_utils.go
+++ b/tests/e2e/utils/service_utils.go
@@ -42,11 +42,14 @@ const (
 	ExecAgnhostPod = "exec-agnhost-pod"
 )
 
-// DeleteService deletes a service
+// DeleteService deletes a service if it exists, return nil if not exists.
 func DeleteService(cs clientset.Interface, ns string, serviceName string) error {
-	Logf("Deleting service %s in namespace %s", serviceName, ns)
-	err := cs.CoreV1().Services(ns).Delete(context.TODO(), serviceName, metav1.DeleteOptions{})
-	if err != nil {
+	Logf("Deleting service %q in namespace %q", serviceName, ns)
+	if err := cs.CoreV1().Services(ns).Delete(context.TODO(), serviceName, metav1.DeleteOptions{}); err != nil {
+		if apierrs.IsNotFound(err) {
+			Logf("Service %q does not exist, no need to delete", serviceName)
+			return nil
+		}
 		return err
 	}
 	return wait.PollImmediate(poll, deletionTimeout, func() (bool, error) {
@@ -57,16 +60,6 @@ func DeleteService(cs clientset.Interface, ns string, serviceName string) error
 	})
 }
 
-// DeleteServiceIfExists deletes a service if it exists, return nil if not exists
-func DeleteServiceIfExists(cs clientset.Interface, ns string, serviceName string) error {
-	err := DeleteService(cs, ns, serviceName)
-	if apierrs.IsNotFound(err) {
-		Logf("Service %s does not exist, no need to delete", serviceName)
-		return nil
-	}
-	return err
-}
-
 // GetServiceDomainName cat prefix and azure suffix
 func GetServiceDomainName(prefix string) (ret string) {
 	suffix := extractSuffix()