[release-1.24] Fix k8s-azure-dns-label-service tag not deleted with Service

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,22 +3623,15 @@ 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 {
 		klog.Warningf("cannot find the service %s in the corresponding PIP", serviceName)
 	}
 
 	_, 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

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)
 	}
 }

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())

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,15 +143,15 @@ 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())
 		}()
 
 		serviceName2 := serviceName + "-share"
 		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())
 		}()
 

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)

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()