diff --git a/staging/src/k8s.io/cloud-provider/controllers/service/controller.go b/staging/src/k8s.io/cloud-provider/controllers/service/controller.go index c57a3d9a314..ae717e6bd50 100644 --- a/staging/src/k8s.io/cloud-provider/controllers/service/controller.go +++ b/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 diff --git a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_external.go b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_external.go index 6e13b4b698e..d1a28a91582 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_external.go +++ b/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 - if val, ok := apiService.Annotations[RBSAnnotationKey]; ok && val == RBSEnabled { - 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 != "" { + // Skip service handling if it uses Regional Backend Services and handled by other controllers + if usesL4RBS(apiService, existingFwdRule) { 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) diff --git a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_external_test.go b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_external_test.go index 9a6c9cfffa5..05aac53a6b3 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_external_test.go +++ b/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()) - 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.Annotations = tc.annotations + _, err = gce.ensureExternalLoadBalancer(vals.ClusterName, vals.ClusterID, svc, nil, nodes) - if tc.expectError != nil { - assert.EqualError(t, err, (*tc.expectError).Error()) + if tc.wantError != nil { + 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 - expectError *error + finalizers []string + wantError *error }{ "When has ELBRbsFinalizer": { - finalizers: []string{ELBRbsFinalizer}, - expectError: &cloudprovider.ImplementedElsewhere, + finalizers: []string{NetLBFinalizerV2}, + wantError: &cloudprovider.ImplementedElsewhere, }, "When has no finalizer": { - finalizers: []string{}, - expectError: nil, + finalizers: []string{}, + 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 { - assert.EqualError(t, err, (*tc.expectError).Error()) + if tc.wantError != nil { + 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 { - assert.EqualError(t, err, (*tc.expectError).Error()) + if tc.wantError != nil { + assert.EqualError(t, err, (*tc.wantError).Error()) } else { assert.NoError(t, err, "Should not return an error "+desc) } diff --git a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_util.go b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_util.go index 931d8ccd953..0a2729dd88c 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_util.go +++ b/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 +}