Merge pull request #79372 from RainbowMango/pr_fix_issue_78954_svn_controller_clean

Fix service load balancer may not be cleaned up on corner case of type change

pkg/controller/service/service_controller.go

@@ -136,6 +136,8 @@ func New(
 		cache.ResourceEventHandlerFuncs{
 			AddFunc: func(cur interface{}) {
 				svc, ok := cur.(*v1.Service)
+				// Check cleanup here can provide a remedy when controller failed to handle
+				// changes before it exiting (e.g. crashing, restart, etc.).
 				if ok && (wantsLoadBalancer(svc) || needsCleanup(svc)) {
 					s.enqueueService(cur)
 				}
@@ -438,7 +440,20 @@ 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 {
-	return service.ObjectMeta.DeletionTimestamp != nil && servicehelper.HasLBFinalizer(service)
+	if !servicehelper.HasLBFinalizer(service) {
+		return false
+	}
+
+	if service.ObjectMeta.DeletionTimestamp != nil {
+		return true
+	}
+
+	// Service doesn't want loadBalancer but owns loadBalancer finalizer also need to be cleaned up.
+	if service.Spec.Type != v1.ServiceTypeLoadBalancer {
+		return true
+	}
+
+	return false
 }
 
 // needsUpdate checks if load balancer needs to be updated due to change in attributes.

pkg/controller/service/service_controller_test.go

@@ -833,6 +833,16 @@ func TestProcessServiceDeletion(t *testing.T) {
 
 }
 
+// Test cases:
+// index    finalizer    timestamp    wantLB  |  clean-up
+//   0         0           0            0     |   false    (No finalizer, no clean up)
+//   1         0           0            1     |   false    (Ignored as same with case 0)
+//   2         0           1            0     |   false    (Ignored as same with case 0)
+//   3         0           1            1     |   false    (Ignored as same with case 0)
+//   4         1           0            0     |   true
+//   5         1           0            1     |   false
+//   6         1           1            0     |   true    (Service is deleted, needs clean up)
+//   7         1           1            1     |   true    (Ignored as same with case 6)
 func TestNeedsCleanup(t *testing.T) {
 	testCases := []struct {
 		desc               string
@@ -840,27 +850,31 @@ func TestNeedsCleanup(t *testing.T) {
 		expectNeedsCleanup bool
 	}{
 		{
-			desc:               "service without finalizer without timestamp",
+			desc:               "service without finalizer",
 			svc:                &v1.Service{},
 			expectNeedsCleanup: false,
 		},
 		{
-			desc: "service without finalizer with timestamp",
+			desc: "service with finalizer without timestamp without LB",
-			svc: &v1.Service{
-				ObjectMeta: metav1.ObjectMeta{
-					DeletionTimestamp: &metav1.Time{
-						Time: time.Now(),
-					},
-				},
-			},
-			expectNeedsCleanup: false,
-		},
-		{
-			desc: "service with finalizer without timestamp",
 			svc: &v1.Service{
 				ObjectMeta: metav1.ObjectMeta{
 					Finalizers: []string{servicehelper.LoadBalancerCleanupFinalizer},
 				},
+				Spec: v1.ServiceSpec{
+					Type: v1.ServiceTypeNodePort,
+				},
+			},
+			expectNeedsCleanup: true,
+		},
+		{
+			desc: "service with finalizer without timestamp with LB",
+			svc: &v1.Service{
+				ObjectMeta: metav1.ObjectMeta{
+					Finalizers: []string{servicehelper.LoadBalancerCleanupFinalizer},
+				},
+				Spec: v1.ServiceSpec{
+					Type: v1.ServiceTypeLoadBalancer,
+				},
 			},
 			expectNeedsCleanup: false,
 		},
@@ -868,10 +882,10 @@ func TestNeedsCleanup(t *testing.T) {
 			desc: "service with finalizer with timestamp",
 			svc: &v1.Service{
 				ObjectMeta: metav1.ObjectMeta{
+					Finalizers: []string{servicehelper.LoadBalancerCleanupFinalizer},
 					DeletionTimestamp: &metav1.Time{
 						Time: time.Now(),
 					},
-					Finalizers: []string{servicehelper.LoadBalancerCleanupFinalizer, "unrelated"},
 				},
 			},
 			expectNeedsCleanup: true,