Merge pull request #81213 from nilo19/t-qini-add_annotation_to_lookup_pip_through_resource_name

Add service annotation for specifying load balancer's pip with name.

staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer.go

@@ -25,6 +25,7 @@ import (
 	"strings"
 
 	v1 "k8s.io/api/core/v1"
+	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/apimachinery/pkg/util/sets"
 	cloudprovider "k8s.io/cloud-provider"
 	servicehelpers "k8s.io/cloud-provider/service/helpers"
@@ -72,6 +73,9 @@ const (
 	// to specify the resource group of load balancer objects that are not in the same resource group as the cluster.
 	ServiceAnnotationLoadBalancerResourceGroup = "service.beta.kubernetes.io/azure-load-balancer-resource-group"
 
+	// ServiceAnnotationPIPName specifies the pip that will be applied to load balancer
+	ServiceAnnotationPIPName = "service.beta.kubernetes.io/azure-pip-name"
+
 	// ServiceAnnotationAllowedServiceTag is the annotation used on the service
 	// to specify a list of allowed service tags separated by comma
 	// Refer https://docs.microsoft.com/en-us/azure/virtual-network/security-overview#service-tags for all supported service tags.
@@ -410,26 +414,32 @@ func (az *Cloud) getServiceLoadBalancerStatus(service *v1.Service, lb *network.L
 	return nil, nil
 }
 
-func (az *Cloud) determinePublicIPName(clusterName string, service *v1.Service) (string, error) {
+func (az *Cloud) determinePublicIPName(clusterName string, service *v1.Service) (string, bool, error) {
+	var shouldPIPExisted bool
+	if name, found := service.Annotations[ServiceAnnotationPIPName]; found && name != "" {
+		shouldPIPExisted = true
+		return name, shouldPIPExisted, nil
+	}
+
 	loadBalancerIP := service.Spec.LoadBalancerIP
 	if len(loadBalancerIP) == 0 {
-		return az.getPublicIPName(clusterName, service), nil
+		return az.getPublicIPName(clusterName, service), shouldPIPExisted, nil
 	}
 
 	pipResourceGroup := az.getPublicIPAddressResourceGroup(service)
 
 	pips, err := az.ListPIP(service, pipResourceGroup)
 	if err != nil {
-		return "", err
+		return "", shouldPIPExisted, err
 	}
 
 	for _, pip := range pips {
 		if pip.PublicIPAddressPropertiesFormat.IPAddress != nil &&
 			*pip.PublicIPAddressPropertiesFormat.IPAddress == loadBalancerIP {
-			return *pip.Name, nil
+			return *pip.Name, shouldPIPExisted, nil
 		}
 	}
-	return "", fmt.Errorf("user supplied IP Address %s was not found in resource group %s", loadBalancerIP, pipResourceGroup)
+	return "", shouldPIPExisted, fmt.Errorf("user supplied IP Address %s was not found in resource group %s", loadBalancerIP, pipResourceGroup)
 }
 
 func flipServiceInternalAnnotation(service *v1.Service) *v1.Service {
@@ -476,7 +486,7 @@ func (az *Cloud) findServiceIPAddress(ctx context.Context, clusterName string, s
 	return lbStatus.Ingress[0].IP, nil
 }
 
-func (az *Cloud) ensurePublicIPExists(service *v1.Service, pipName string, domainNameLabel, clusterName string) (*network.PublicIPAddress, error) {
+func (az *Cloud) ensurePublicIPExists(service *v1.Service, pipName string, domainNameLabel, clusterName string, shouldPIPExisted bool) (*network.PublicIPAddress, error) {
 	pipResourceGroup := az.getPublicIPAddressResourceGroup(service)
 	pip, existsPip, err := az.getPublicIPAddress(pipResourceGroup, pipName)
 	if err != nil {
@@ -487,6 +497,11 @@ func (az *Cloud) ensurePublicIPExists(service *v1.Service, pipName string, domai
 	}
 
 	serviceName := getServiceName(service)
+
+	if shouldPIPExisted {
+		return nil, fmt.Errorf("PublicIP from annotation azure-pip-name=%s for service %s doesn't exist", pipName, serviceName)
+	}
+
 	pip.Name = to.StringPtr(pipName)
 	pip.Location = to.StringPtr(az.Location)
 	pip.PublicIPAddressPropertiesFormat = &network.PublicIPAddressPropertiesFormat{
@@ -582,7 +597,7 @@ func (az *Cloud) isFrontendIPChanged(clusterName string, config network.Frontend
 	if loadBalancerIP == "" {
 		return false, nil
 	}
-	pipName, err := az.determinePublicIPName(clusterName, service)
+	pipName, _, err := az.determinePublicIPName(clusterName, service)
 	if err != nil {
 		return false, err
 	}
@@ -720,12 +735,12 @@ func (az *Cloud) reconcileLoadBalancer(clusterName string, service *v1.Service,
 
 				fipConfigurationProperties = &configProperties
 			} else {
-				pipName, err := az.determinePublicIPName(clusterName, service)
+				pipName, shouldPIPExisted, err := az.determinePublicIPName(clusterName, service)
 				if err != nil {
 					return nil, err
 				}
 				domainNameLabel := getPublicIPDomainNameLabel(service)
-				pip, err := az.ensurePublicIPExists(service, pipName, domainNameLabel, clusterName)
+				pip, err := az.ensurePublicIPExists(service, pipName, domainNameLabel, clusterName, shouldPIPExisted)
 				if err != nil {
 					return nil, err
 				}
@@ -1355,8 +1370,9 @@ func (az *Cloud) reconcilePublicIP(clusterName string, service *v1.Service, lbNa
 	var lb *network.LoadBalancer
 	var desiredPipName string
 	var err error
+	var shouldPIPExisted bool
 	if !isInternal && wantLb {
-		desiredPipName, err = az.determinePublicIPName(clusterName, service)
+		desiredPipName, shouldPIPExisted, err = az.determinePublicIPName(clusterName, service)
 		if err != nil {
 			return nil, err
 		}
@@ -1377,32 +1393,45 @@ func (az *Cloud) reconcilePublicIP(clusterName string, service *v1.Service, lbNa
 		return nil, err
 	}
 
+	var found bool
+	var pipsToBeDeleted []*network.PublicIPAddress
 	for i := range pips {
 		pip := pips[i]
+		pipName := *pip.Name
 		if serviceOwnsPublicIP(&pip, clusterName, serviceName) {
 			// We need to process for pips belong to this service
-			pipName := *pip.Name
 			if wantLb && !isInternal && pipName == desiredPipName {
 				// This is the only case we should preserve the
 				// Public ip resource with match service tag
+				found = true
 			} else {
-				klog.V(2).Infof("reconcilePublicIP for service(%s): pip(%s) - deleting", serviceName, pipName)
+				pipsToBeDeleted = append(pipsToBeDeleted, &pip)
-				err := az.safeDeletePublicIP(service, pipResourceGroup, &pip, lb)
-				if err != nil {
-					klog.Errorf("safeDeletePublicIP(%s) failed with error: %v", pipName, err)
-					return nil, err
-				}
-				klog.V(2).Infof("reconcilePublicIP for service(%s): pip(%s) - finished", serviceName, pipName)
 			}
+		} else if wantLb && !isInternal && pipName == desiredPipName {
+			found = true
 		}
-
+	}
+	if !isInternal && shouldPIPExisted && !found && wantLb {
+		return nil, fmt.Errorf("reconcilePublicIP for service(%s): pip(%s) not found", serviceName, desiredPipName)
+	}
+	var deleteFuncs []func() error
+	for _, pip := range pipsToBeDeleted {
+		pipCopy := *pip
+		deleteFuncs = append(deleteFuncs, func() error {
+			klog.V(2).Infof("reconcilePublicIP for service(%s): pip(%s) - deleting", serviceName, *pip.Name)
+			return az.safeDeletePublicIP(service, pipResourceGroup, &pipCopy, lb)
+		})
+	}
+	errs := utilerrors.AggregateGoroutines(deleteFuncs...)
+	if errs != nil {
+		return nil, utilerrors.Flatten(errs)
 	}
 
 	if !isInternal && wantLb {
 		// Confirm desired public ip resource exists
 		var pip *network.PublicIPAddress
 		domainNameLabel := getPublicIPDomainNameLabel(service)
-		if pip, err = az.ensurePublicIPExists(service, desiredPipName, domainNameLabel, clusterName); err != nil {
+		if pip, err = az.ensurePublicIPExists(service, desiredPipName, domainNameLabel, clusterName, shouldPIPExisted); err != nil {
 			return nil, err
 		}
 		return pip, nil

staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer_test.go

@@ -932,7 +932,7 @@ func TestDeterminePublicIPName(t *testing.T) {
 				t.Fatalf("TestCase[%d] meets unexpected error: %v", i, err)
 			}
 		}
-		ip, err := az.determinePublicIPName("testCluster", &service)
+		ip, _, err := az.determinePublicIPName("testCluster", &service)
 		assert.Equal(t, test.expectedIP, ip, "TestCase[%d]: %s", i, test.desc)
 		assert.Equal(t, test.expectedError, err != nil, "TestCase[%d]: %s", i, test.desc)
 	}
@@ -1713,6 +1713,7 @@ func TestReconcilePublicIP(t *testing.T) {
 	testCases := []struct {
 		desc          string
 		wantLb        bool
+		annotations   map[string]string
 		existingPIPs  []network.PublicIPAddress
 		expectedID    string
 		expectedPIP   *network.PublicIPAddress
@@ -1743,11 +1744,73 @@ func TestReconcilePublicIP(t *testing.T) {
 			expectedID: "/subscriptions/subscription/resourceGroups/rg/providers/" +
 				"Microsoft.Network/publicIPAddresses/testCluster-atest1",
 		},
+		{
+			desc:        "reconcilePublicIP shall report error if the given PIP name doesn't exist in the resource group",
+			wantLb:      true,
+			annotations: map[string]string{ServiceAnnotationPIPName: "testPIP"},
+			existingPIPs: []network.PublicIPAddress{
+				{
+					Name: to.StringPtr("pip1"),
+					Tags: map[string]*string{"service": to.StringPtr("default/test1")},
+				},
+				{
+					Name: to.StringPtr("pip2"),
+					Tags: map[string]*string{"service": to.StringPtr("default/test1")},
+				},
+			},
+			expectedError: true,
+		},
+		{
+			desc:        "reconcilePublicIP shall delete unwanted PIP when given the name of desired PIP",
+			wantLb:      true,
+			annotations: map[string]string{ServiceAnnotationPIPName: "testPIP"},
+			existingPIPs: []network.PublicIPAddress{
+				{
+					Name: to.StringPtr("pip1"),
+					Tags: map[string]*string{"service": to.StringPtr("default/test1")},
+				},
+				{
+					Name: to.StringPtr("pip2"),
+					Tags: map[string]*string{"service": to.StringPtr("default/test1")},
+				},
+				{
+					Name: to.StringPtr("testPIP"),
+					Tags: map[string]*string{"service": to.StringPtr("default/test1")},
+				},
+			},
+			expectedPIP: &network.PublicIPAddress{
+				ID:   to.StringPtr("/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Network/publicIPAddresses/testPIP"),
+				Name: to.StringPtr("testPIP"),
+				Tags: map[string]*string{"service": to.StringPtr("default/test1")},
+			},
+		},
+		{
+			desc:        "reconcilePublicIP shall find the PIP by given name and shall not delete the PIP which is not owned by service",
+			wantLb:      true,
+			annotations: map[string]string{ServiceAnnotationPIPName: "testPIP"},
+			existingPIPs: []network.PublicIPAddress{
+				{
+					Name: to.StringPtr("pip1"),
+				},
+				{
+					Name: to.StringPtr("pip2"),
+					Tags: map[string]*string{"service": to.StringPtr("default/test1")},
+				},
+				{
+					Name: to.StringPtr("testPIP"),
+				},
+			},
+			expectedPIP: &network.PublicIPAddress{
+				ID:   to.StringPtr("/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Network/publicIPAddresses/testPIP"),
+				Name: to.StringPtr("testPIP"),
+			},
+		},
 	}
 
 	for i, test := range testCases {
 		az := getTestCloud()
 		service := getTestService("test1", v1.ProtocolTCP, nil, 80)
+		service.Annotations = test.annotations
 		for _, pip := range test.existingPIPs {
 			_, err := az.PublicIPAddressesClient.CreateOrUpdate(context.TODO(), "rg", to.String(pip.Name), pip)
 			if err != nil {
@@ -1797,7 +1860,7 @@ func TestEnsurePublicIPExists(t *testing.T) {
 				t.Fatalf("TestCase[%d] meets unexpected error: %v", i, err)
 			}
 		}
-		pip, err := az.ensurePublicIPExists(&service, "pip1", "", "")
+		pip, err := az.ensurePublicIPExists(&service, "pip1", "", "", false)
 		if test.expectedID != "" {
 			assert.Equal(t, test.expectedID, to.String(pip.ID), "TestCase[%d]: %s", i, test.desc)
 		} else {

staging/src/k8s.io/legacy-cloud-providers/azure/azure_vmss.go

@@ -28,7 +28,7 @@ import (
 	"github.com/Azure/azure-sdk-for-go/services/network/mgmt/2018-08-01/network"
 	"github.com/Azure/go-autorest/autorest/to"
 
-	"k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/types"
 	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 	cloudprovider "k8s.io/cloud-provider"