Add deletionTimestamp check to avoid race condition

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)

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)")
+			}
+		})
+	}
+}