diff --git a/pkg/controller/service/controller.go b/pkg/controller/service/controller.go index 6a5f55889..f0ea37dbe 100644 --- a/pkg/controller/service/controller.go +++ b/pkg/controller/service/controller.go @@ -538,14 +538,14 @@ func (s *serviceCache) delete(serviceName string) { // needsCleanup checks if load balancer needs to be cleaned up as indicated by finalizer. func needsCleanup(service *v1.Service) bool { - if !servicehelper.HasLBFinalizer(service) { - return false - } - if service.ObjectMeta.DeletionTimestamp != nil { return true } + if !servicehelper.HasLBFinalizer(service) { + return false + } + // Service doesn't want loadBalancer but owns loadBalancer finalizer also need to be cleaned up. if service.Spec.Type != v1.ServiceTypeLoadBalancer { return true @@ -738,7 +738,7 @@ func (c *Controller) syncNodes(ctx context.Context, workers int) sets.String { func (c *Controller) nodeSyncService(svc *v1.Service) bool { const retSuccess = false const retNeedRetry = true - if svc == nil || !WantsLoadBalancer(svc) { + if svc == nil || svc.DeletionTimestamp != nil || !WantsLoadBalancer(svc) { return retSuccess } newNodes, err := listWithPredicates(c.nodeLister) diff --git a/pkg/controller/service/controller_test.go b/pkg/controller/service/controller_test.go new file mode 100644 index 000000000..a90e55d5f --- /dev/null +++ b/pkg/controller/service/controller_test.go @@ -0,0 +1,141 @@ +package service + +import ( + "testing" + + v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + servicehelper "k8s.io/cloud-provider/service/helpers" +) + +// TestNeedsCleanup verifies that services with deletionTimestamp set are +// correctly identified as needing cleanup, regardless of finalizer state. +// This prevents a race condition where node updates trigger reconciliation +// after the finalizer is removed but before the service is deleted from etcd. +func TestNeedsCleanup(t *testing.T) { + now := metav1.Now() + + testCases := []struct { + name string + service *v1.Service + expected bool + }{ + { + name: "service with deletionTimestamp and finalizer should need cleanup", + service: &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + DeletionTimestamp: &now, + Finalizers: []string{servicehelper.LoadBalancerCleanupFinalizer}, + }, + Spec: v1.ServiceSpec{ + Type: v1.ServiceTypeLoadBalancer, + }, + }, + expected: true, + }, + { + name: "service with deletionTimestamp but no finalizer should need cleanup", + service: &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + DeletionTimestamp: &now, + Finalizers: []string{}, + }, + Spec: v1.ServiceSpec{ + Type: v1.ServiceTypeLoadBalancer, + }, + }, + expected: true, + }, + { + name: "service without deletionTimestamp and without finalizer should not need cleanup", + service: &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Finalizers: []string{}, + }, + Spec: v1.ServiceSpec{ + Type: v1.ServiceTypeLoadBalancer, + }, + }, + expected: false, + }, + { + name: "service without deletionTimestamp but with finalizer should not need cleanup", + service: &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Finalizers: []string{servicehelper.LoadBalancerCleanupFinalizer}, + }, + Spec: v1.ServiceSpec{ + Type: v1.ServiceTypeLoadBalancer, + }, + }, + expected: false, + }, + { + name: "non-LoadBalancer service with finalizer should need cleanup", + service: &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Finalizers: []string{servicehelper.LoadBalancerCleanupFinalizer}, + }, + Spec: v1.ServiceSpec{ + Type: v1.ServiceTypeClusterIP, + }, + }, + expected: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + result := needsCleanup(tc.service) + if result != tc.expected { + t.Errorf("needsCleanup() = %v, expected %v", result, tc.expected) + } + }) + } +} + +// TestNodeSyncService_DeletionTimestamp verifies that nodeSyncService returns +// success (false) without attempting to sync when a service has deletionTimestamp set. +func TestNodeSyncService_DeletionTimestamp(t *testing.T) { + now := metav1.Now() + + testCases := []struct { + name string + service *v1.Service + }{ + { + name: "nil service should return success", + service: nil, + }, + { + name: "service with deletionTimestamp should return success without syncing", + service: &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + DeletionTimestamp: &now, + }, + Spec: v1.ServiceSpec{ + Type: v1.ServiceTypeLoadBalancer, + }, + }, + }, + { + name: "non-LoadBalancer service should return success", + service: &v1.Service{ + Spec: v1.ServiceSpec{ + Type: v1.ServiceTypeClusterIP, + }, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + c := &Controller{} + + result := c.nodeSyncService(tc.service) + if result != false { + t.Errorf("nodeSyncService() returned needRetry=true, expected success (false)") + } + }) + } +}