Merge pull request #109858 from panslava/skip-deleting-updating-rbs-services

GCE: skip updating and deleting external loadbalancer if service is managed by ingress-gce

staging/src/k8s.io/cloud-provider/controllers/service/controller.go

@@ -388,7 +388,11 @@ func (s *Controller) syncLoadBalancerIfNeeded(ctx context.Context, service *v1.S
 			klog.V(2).Infof("Deleting existing load balancer for service %s", key)
 			s.eventRecorder.Event(service, v1.EventTypeNormal, "DeletingLoadBalancer", "Deleting load balancer")
 			if err := s.balancer.EnsureLoadBalancerDeleted(ctx, s.clusterName, service); err != nil {
-				return op, fmt.Errorf("failed to delete load balancer: %v", err)
+				if err == cloudprovider.ImplementedElsewhere {
+					klog.V(4).Infof("LoadBalancer for service %s implemented by a different controller %s, Ignoring error on deletion", key, s.cloud.ProviderName())
+				} else {
+					return op, fmt.Errorf("failed to delete load balancer: %v", err)
+				}
 			}
 		}
 		// Always remove finalizer when load balancer is deleted, this ensures Services

staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_external.go

@@ -35,14 +35,12 @@ import (
 	servicehelpers "k8s.io/cloud-provider/service/helpers"
 	utilnet "k8s.io/utils/net"
 
-	compute "google.golang.org/api/compute/v1"
+	"google.golang.org/api/compute/v1"
 	"k8s.io/klog/v2"
 )
 
 const (
 	errStrLbNoHosts = "cannot EnsureLoadBalancer() with no hosts"
-
-	ELBRbsFinalizer = "gke.networking.io/l4-netlb-v2"
 )
 
 // ensureExternalLoadBalancer is the external implementation of LoadBalancer.EnsureLoadBalancer.
@@ -54,16 +52,8 @@ const (
 // new load balancers and updating existing load balancers, recognizing when
 // each is needed.
 func (g *Cloud) ensureExternalLoadBalancer(clusterName string, clusterID string, apiService *v1.Service, existingFwdRule *compute.ForwardingRule, nodes []*v1.Node) (*v1.LoadBalancerStatus, error) {
-	// Skip service handling if managed by ingress-gce using Regional Backend Services
+	// Skip service handling if it uses Regional Backend Services and handled by other controllers
-	if val, ok := apiService.Annotations[RBSAnnotationKey]; ok && val == RBSEnabled {
+	if usesL4RBS(apiService, existingFwdRule) {
-		return nil, cloudprovider.ImplementedElsewhere
-	}
-	// Skip service handling if service has Regional Backend Services finalizer
-	if hasFinalizer(apiService, ELBRbsFinalizer) {
-		return nil, cloudprovider.ImplementedElsewhere
-	}
-	// Skip service handling if it has Regional Backend Service created by Ingress-GCE
-	if existingFwdRule != nil && existingFwdRule.BackendService != "" {
 		return nil, cloudprovider.ImplementedElsewhere
 	}
 
@@ -301,6 +291,11 @@ func (g *Cloud) ensureExternalLoadBalancer(clusterName string, clusterID string,
 
 // updateExternalLoadBalancer is the external implementation of LoadBalancer.UpdateLoadBalancer.
 func (g *Cloud) updateExternalLoadBalancer(clusterName string, service *v1.Service, nodes []*v1.Node) error {
+	// Skip service update if it uses Regional Backend Services and handled by other controllers
+	if usesL4RBS(service, nil) {
+		return cloudprovider.ImplementedElsewhere
+	}
+
 	hosts, err := g.getInstancesByNames(nodeNames(nodes))
 	if err != nil {
 		return err
@@ -312,6 +307,11 @@ func (g *Cloud) updateExternalLoadBalancer(clusterName string, service *v1.Servi
 
 // ensureExternalLoadBalancerDeleted is the external implementation of LoadBalancer.EnsureLoadBalancerDeleted
 func (g *Cloud) ensureExternalLoadBalancerDeleted(clusterName, clusterID string, service *v1.Service) error {
+	// Skip service deletion if it uses Regional Backend Services and handled by other controllers
+	if usesL4RBS(service, nil) {
+		return cloudprovider.ImplementedElsewhere
+	}
+
 	loadBalancerName := g.GetLoadBalancerName(context.TODO(), clusterName, service)
 	serviceName := types.NamespacedName{Namespace: service.Namespace, Name: service.Name}
 	lbRefStr := fmt.Sprintf("%v(%v)", loadBalancerName, serviceName)

staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_external_test.go

@@ -585,35 +585,54 @@ func TestEnsureExternalLoadBalancerRBSAnnotation(t *testing.T) {
 
 	for desc, tc := range map[string]struct {
 		annotations map[string]string
-		expectError *error
+		wantError   *error
 	}{
 		"When RBS enabled": {
 			annotations: map[string]string{RBSAnnotationKey: RBSEnabled},
-			expectError: &cloudprovider.ImplementedElsewhere,
+			wantError:   &cloudprovider.ImplementedElsewhere,
 		},
 		"When RBS not enabled": {
 			annotations: map[string]string{},
-			expectError: nil,
+			wantError:   nil,
 		},
 		"When RBS annotation has wrong value": {
 			annotations: map[string]string{RBSAnnotationKey: "WrongValue"},
-			expectError: nil,
+			wantError:   nil,
 		},
 	} {
 		t.Run(desc, func(t *testing.T) {
 			vals := DefaultTestClusterValues()
-			gce, err := fakeGCECloud(DefaultTestClusterValues())
+			gce, err := fakeGCECloud(vals)
-			require.NoError(t, err)
+			if err != nil {
-			nodeNames := []string{"test-node-1"}
+				t.Fatalf("fakeGCECloud(%v) returned error %v, want nil", vals, err)
+			}
 
+			nodeNames := []string{"test-node-1"}
 			nodes, err := createAndInsertNodes(gce, nodeNames, vals.ZoneName)
-			require.NoError(t, err)
+			if err != nil {
+				t.Fatalf("createAndInsertNodes(_, %v, %v) returned error %v, want nil", nodeNames, vals.ZoneName, err)
+			}
 
 			svc := fakeLoadbalancerService("")
 			svc.Annotations = tc.annotations
+
 			_, err = gce.ensureExternalLoadBalancer(vals.ClusterName, vals.ClusterID, svc, nil, nodes)
-			if tc.expectError != nil {
+			if tc.wantError != nil {
-				assert.EqualError(t, err, (*tc.expectError).Error())
+				assert.EqualError(t, err, (*tc.wantError).Error())
+			} else {
+				assert.NoError(t, err, "Should not return an error "+desc)
+			}
+
+			err = gce.updateExternalLoadBalancer(vals.ClusterName, svc, nodes)
+			if tc.wantError != nil {
+				assert.EqualError(t, err, (*tc.wantError).Error())
+			} else {
+				assert.NoError(t, err, "Should not return an error "+desc)
+			}
+
+			err = gce.ensureExternalLoadBalancerDeleted(vals.ClusterName, vals.ClusterID, svc)
+			if tc.wantError != nil {
+				assert.EqualError(t, err, (*tc.wantError).Error())
 			} else {
 				assert.NoError(t, err, "Should not return an error "+desc)
 			}
@@ -625,32 +644,52 @@ func TestEnsureExternalLoadBalancerRBSFinalizer(t *testing.T) {
 	t.Parallel()
 
 	for desc, tc := range map[string]struct {
-		finalizers  []string
+		finalizers []string
-		expectError *error
+		wantError  *error
 	}{
 		"When has ELBRbsFinalizer": {
-			finalizers:  []string{ELBRbsFinalizer},
+			finalizers: []string{NetLBFinalizerV2},
-			expectError: &cloudprovider.ImplementedElsewhere,
+			wantError:  &cloudprovider.ImplementedElsewhere,
 		},
 		"When has no finalizer": {
-			finalizers:  []string{},
+			finalizers: []string{},
-			expectError: nil,
+			wantError:  nil,
 		},
 	} {
 		t.Run(desc, func(t *testing.T) {
 			vals := DefaultTestClusterValues()
-			gce, err := fakeGCECloud(DefaultTestClusterValues())
-			require.NoError(t, err)
-			nodeNames := []string{"test-node-1"}
 
+			gce, err := fakeGCECloud(vals)
+			if err != nil {
+				t.Fatalf("fakeGCECloud(%v) returned error %v, want nil", vals, err)
+			}
+
+			nodeNames := []string{"test-node-1"}
 			nodes, err := createAndInsertNodes(gce, nodeNames, vals.ZoneName)
-			require.NoError(t, err)
+			if err != nil {
+				t.Fatalf("createAndInsertNodes(_, %v, %v) returned error %v, want nil", nodeNames, vals.ZoneName, err)
+			}
 
 			svc := fakeLoadbalancerService("")
 			svc.Finalizers = tc.finalizers
+
 			_, err = gce.ensureExternalLoadBalancer(vals.ClusterName, vals.ClusterID, svc, nil, nodes)
-			if tc.expectError != nil {
+			if tc.wantError != nil {
-				assert.EqualError(t, err, (*tc.expectError).Error())
+				assert.EqualError(t, err, (*tc.wantError).Error())
+			} else {
+				assert.NoError(t, err, "Should not return an error "+desc)
+			}
+
+			err = gce.updateExternalLoadBalancer(vals.ClusterName, svc, nodes)
+			if tc.wantError != nil {
+				assert.EqualError(t, err, (*tc.wantError).Error())
+			} else {
+				assert.NoError(t, err, "Should not return an error "+desc)
+			}
+
+			err = gce.ensureExternalLoadBalancerDeleted(vals.ClusterName, vals.ClusterID, svc)
+			if tc.wantError != nil {
+				assert.EqualError(t, err, (*tc.wantError).Error())
 			} else {
 				assert.NoError(t, err, "Should not return an error "+desc)
 			}
@@ -663,38 +702,43 @@ func TestEnsureExternalLoadBalancerExistingFwdRule(t *testing.T) {
 
 	for desc, tc := range map[string]struct {
 		existingForwardingRule *compute.ForwardingRule
-		expectError            *error
+		wantError              *error
 	}{
 		"When has existingForwardingRule with backend service": {
 			existingForwardingRule: &compute.ForwardingRule{
 				BackendService: "exists",
 			},
-			expectError: &cloudprovider.ImplementedElsewhere,
+			wantError: &cloudprovider.ImplementedElsewhere,
 		},
 		"When has existingForwardingRule with empty backend service": {
 			existingForwardingRule: &compute.ForwardingRule{
 				BackendService: "",
 			},
-			expectError: nil,
+			wantError: nil,
 		},
 		"When has no existingForwardingRule": {
 			existingForwardingRule: nil,
-			expectError:            nil,
+			wantError:              nil,
 		},
 	} {
 		t.Run(desc, func(t *testing.T) {
 			vals := DefaultTestClusterValues()
-			gce, err := fakeGCECloud(DefaultTestClusterValues())
-			require.NoError(t, err)
-			nodeNames := []string{"test-node-1"}
 
+			gce, err := fakeGCECloud(vals)
+			if err != nil {
+				t.Fatalf("fakeGCECloud(%v) returned error %v, want nil", vals, err)
+			}
+
+			nodeNames := []string{"test-node-1"}
 			nodes, err := createAndInsertNodes(gce, nodeNames, vals.ZoneName)
-			require.NoError(t, err)
+			if err != nil {
+				t.Fatalf("createAndInsertNodes(_, %v, %v) returned error %v, want nil", nodeNames, vals.ZoneName, err)
+			}
 
 			svc := fakeLoadbalancerService("")
 			_, err = gce.ensureExternalLoadBalancer(vals.ClusterName, vals.ClusterID, svc, tc.existingForwardingRule, nodes)
-			if tc.expectError != nil {
+			if tc.wantError != nil {
-				assert.EqualError(t, err, (*tc.expectError).Error())
+				assert.EqualError(t, err, (*tc.wantError).Error())
 			} else {
 				assert.NoError(t, err, "Should not return an error "+desc)
 			}

staging/src/k8s.io/legacy-cloud-providers/gce/gce_util.go

@@ -31,11 +31,9 @@ import (
 	"sync"
 
 	"cloud.google.com/go/compute/metadata"
-
 	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud"
 	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta"
 	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/mock"
-
 	compute "google.golang.org/api/compute/v1"
 	"google.golang.org/api/googleapi"
 
@@ -48,6 +46,11 @@ import (
 	netutils "k8s.io/utils/net"
 )
 
+const (
+	// NetLBFinalizerV2 is the finalizer used by newer controllers that manage L4 External LoadBalancer services.
+	NetLBFinalizerV2 = "gke.networking.io/l4-netlb-v2"
+)
+
 func fakeGCECloud(vals TestClusterValues) (*Cloud, error) {
 	gce := NewFakeGCECloud(vals)
 
@@ -390,3 +393,23 @@ func removeString(slice []string, s string) []string {
 	}
 	return newSlice
 }
+
+// usesL4RBS checks if service uses Regional Backend Service as a Backend.
+// Such services implemented in other controllers and
+// should not be handled by Service Controller.
+func usesL4RBS(service *v1.Service, forwardingRule *compute.ForwardingRule) bool {
+	// Detect RBS by annotation
+	if val, ok := service.Annotations[RBSAnnotationKey]; ok && val == RBSEnabled {
+		return true
+	}
+	// Detect RBS by finalizer
+	if hasFinalizer(service, NetLBFinalizerV2) {
+		return true
+	}
+	// Detect RBS by existing forwarding rule with Backend Service attached
+	if forwardingRule != nil && forwardingRule.BackendService != "" {
+		return true
+	}
+
+	return false
+}