From 034f96c90998814b6e9f6d336d4512c1d4334e1d Mon Sep 17 00:00:00 2001 From: t-qini Date: Fri, 9 Aug 2019 14:56:44 +0800 Subject: [PATCH 1/2] Add service annotation for specifying load balancer's pip with name. --- .../legacy-cloud-providers/azure/azure_loadbalancer.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer.go b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer.go index c5481326fb3c..9fe85ec11864 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer.go +++ b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer.go @@ -72,6 +72,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" + // ServiceAnnotationLoadBalancerPIPName specifies the pip that will be applied to load balancer + ServiceAnnotationLoadBalancerPIPName = "service.beta.kubernetes.io/azure-load-balancer-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. @@ -393,6 +396,9 @@ func (az *Cloud) getServiceLoadBalancerStatus(service *v1.Service, lb *network.L if err != nil { return nil, fmt.Errorf("get(%s): lb(%s) - failed to get LB PublicIPAddress Name from ID(%s)", serviceName, *lb.Name, *pipID) } + if name, found := service.Annotations[ServiceAnnotationLoadBalancerPIPName]; found && name != "" { + pipName = name + } pip, existsPip, err := az.getPublicIPAddress(az.getPublicIPAddressResourceGroup(service), pipName) if err != nil { return nil, err @@ -411,6 +417,10 @@ func (az *Cloud) getServiceLoadBalancerStatus(service *v1.Service, lb *network.L } func (az *Cloud) determinePublicIPName(clusterName string, service *v1.Service) (string, error) { + if name, found := service.Annotations[ServiceAnnotationLoadBalancerPIPName]; found && name != "" { + return name, nil + } + loadBalancerIP := service.Spec.LoadBalancerIP if len(loadBalancerIP) == 0 { return az.getPublicIPName(clusterName, service), nil From 3facb631d4eacec1384515487c698ea19c54ebc4 Mon Sep 17 00:00:00 2001 From: t-qini Date: Mon, 12 Aug 2019 11:17:03 +0800 Subject: [PATCH 2/2] Modify the logic to discover corresponding errors. --- .../azure/azure_loadbalancer.go | 73 ++++++++++++------- .../azure/azure_loadbalancer_test.go | 67 ++++++++++++++++- .../azure/azure_vmss.go | 2 +- 3 files changed, 112 insertions(+), 30 deletions(-) diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer.go b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer.go index 9fe85ec11864..723157162b0c 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer.go +++ b/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,8 +73,8 @@ 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" - // ServiceAnnotationLoadBalancerPIPName specifies the pip that will be applied to load balancer - ServiceAnnotationLoadBalancerPIPName = "service.beta.kubernetes.io/azure-load-balancer-pip-name" + // 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 @@ -396,9 +397,6 @@ func (az *Cloud) getServiceLoadBalancerStatus(service *v1.Service, lb *network.L if err != nil { return nil, fmt.Errorf("get(%s): lb(%s) - failed to get LB PublicIPAddress Name from ID(%s)", serviceName, *lb.Name, *pipID) } - if name, found := service.Annotations[ServiceAnnotationLoadBalancerPIPName]; found && name != "" { - pipName = name - } pip, existsPip, err := az.getPublicIPAddress(az.getPublicIPAddressResourceGroup(service), pipName) if err != nil { return nil, err @@ -416,30 +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) { - if name, found := service.Annotations[ServiceAnnotationLoadBalancerPIPName]; found && name != "" { - return name, nil +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 { @@ -486,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 { @@ -497,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{ @@ -592,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 } @@ -730,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 } @@ -1363,8 +1368,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 } @@ -1385,32 +1391,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) - 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) + pipsToBeDeleted = append(pipsToBeDeleted, &pip) } + } 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 diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer_test.go b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer_test.go index fcd81719e02c..9e43362f3f0f 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer_test.go +++ b/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 { diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_vmss.go b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_vmss.go index 5bdd24c6bf5c..74a3cfb38956 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_vmss.go +++ b/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"