Amend service controller code/test regarding finalizer GA

pkg/controller/service/controller.go

@@ -79,11 +79,6 @@ const (
 	// originated from: https://github.com/kubernetes/kubernetes/blob/28e800245e/pkg/features/kube_features.go#L178
 	serviceNodeExclusionFeature = "ServiceNodeExclusion"
 
-	// serviceLoadBalancerFinalizerFeature is the feature gate name that
-	// enables Finalizer Protection for Service LoadBalancers.
-	// originated from: https://github.com/kubernetes/kubernetes/blob/28e800245e/pkg/features/kube_features.go#L433
-	serviceLoadBalancerFinalizerFeature = "ServiceLoadBalancerFinalizer"
-
 	// legacyNodeRoleBehaviro is the feature gate name that enables legacy
 	// behavior to vary cluster functionality on the node-role.kubernetes.io
 	// labels.
@@ -103,12 +98,13 @@ type serviceCache struct {
 // Controller keeps cloud provider service resources
 // (like load balancers) in sync with the registry.
 type Controller struct {
-	cloud               cloudprovider.Interface
+	cloud            cloudprovider.Interface
-	knownHosts          []*v1.Node
+	knownHosts       []*v1.Node
-	servicesToUpdate    []*v1.Service
+	servicesToUpdate []*v1.Service
-	kubeClient          clientset.Interface
+	kubeClient       clientset.Interface
-	clusterName         string
+	clusterName      string
-	balancer            cloudprovider.LoadBalancer
+	balancer         cloudprovider.LoadBalancer
+	// TODO(#85155): Stop relying on this and remove the cache completely.
 	cache               *serviceCache
 	serviceLister       corelisters.ServiceLister
 	serviceListerSynced cache.InformerSynced
@@ -170,15 +166,8 @@ func New(
 					s.enqueueService(cur)
 				}
 			},
-			DeleteFunc: func(old interface{}) {
+			// No need to handle deletion event because the deletion would be handled by
-				if utilfeature.DefaultFeatureGate.Enabled(serviceLoadBalancerFinalizerFeature) {
+			// the update path when the deletion timestamp is added.
-					// No need to handle deletion event if finalizer feature gate is
-					// enabled. Because the deletion would be handled by the update
-					// path when the deletion timestamp is added.
-					return
-				}
-				s.enqueueService(old)
-			},
 		},
 		serviceSyncPeriod,
 	)
@@ -335,10 +324,8 @@ func (s *Controller) syncLoadBalancerIfNeeded(service *v1.Service, key string) (
 				return op, fmt.Errorf("failed to delete load balancer: %v", err)
 			}
 		}
-		// Always try to remove finalizer when load balancer is deleted.
+		// Always remove finalizer when load balancer is deleted, this ensures Services
-		// It will be a no-op if finalizer does not exist.
+		// can be deleted after all corresponding load balancer resources are deleted.
-		// Note this also clears up finalizer if the cluster is downgraded
-		// from a version that attaches finalizer to a version that doesn't.
 		if err := s.removeFinalizer(service); err != nil {
 			return op, fmt.Errorf("failed to remove load balancer cleanup finalizer: %v", err)
 		}
@@ -348,15 +335,10 @@ func (s *Controller) syncLoadBalancerIfNeeded(service *v1.Service, key string) (
 		op = ensureLoadBalancer
 		klog.V(2).Infof("Ensuring load balancer for service %s", key)
 		s.eventRecorder.Event(service, v1.EventTypeNormal, "EnsuringLoadBalancer", "Ensuring load balancer")
-		if utilfeature.DefaultFeatureGate.Enabled(serviceLoadBalancerFinalizerFeature) {
+		// Always add a finalizer prior to creating load balancers, this ensures Services
-			// Always try to add finalizer prior to load balancer creation.
+		// can't be deleted until all corresponding load balancer resources are also deleted.
-			// It will be a no-op if finalizer already exists.
+		if err := s.addFinalizer(service); err != nil {
-			// Note this also retrospectively puts on finalizer if the cluster
+			return op, fmt.Errorf("failed to add load balancer cleanup finalizer: %v", err)
-			// is upgraded from a version that doesn't attach finalizer to a
-			// version that does.
-			if err := s.addFinalizer(service); err != nil {
-				return op, fmt.Errorf("failed to add load balancer cleanup finalizer: %v", err)
-			}
 		}
 		newStatus, err = s.ensureLoadBalancer(service)
 		if err != nil {

pkg/controller/service/controller_test.go

@@ -93,7 +93,6 @@ func newController() (*Controller, *fakecloud.Cloud, *fake.Clientset) {
 func TestSyncLoadBalancerIfNeeded(t *testing.T) {
 	testCases := []struct {
 		desc                 string
-		enableFeatureGate    bool
 		service              *v1.Service
 		lbExists             bool
 		expectOp             loadBalancerOperation
@@ -155,9 +154,10 @@ func TestSyncLoadBalancerIfNeeded(t *testing.T) {
 					Type: v1.ServiceTypeLoadBalancer,
 				},
 			},
-			expectOp:            ensureLoadBalancer,
+			expectOp:             ensureLoadBalancer,
-			expectCreateAttempt: true,
+			expectCreateAttempt:  true,
-			expectPatchStatus:   true,
+			expectPatchStatus:    true,
+			expectPatchFinalizer: true,
 		},
 		{
 			desc: "tcp service that wants LB",
@@ -175,9 +175,10 @@ func TestSyncLoadBalancerIfNeeded(t *testing.T) {
 					Type: v1.ServiceTypeLoadBalancer,
 				},
 			},
-			expectOp:            ensureLoadBalancer,
+			expectOp:             ensureLoadBalancer,
-			expectCreateAttempt: true,
+			expectCreateAttempt:  true,
-			expectPatchStatus:   true,
+			expectPatchStatus:    true,
+			expectPatchFinalizer: true,
 		},
 		{
 			desc: "sctp service that wants LB",
@@ -195,9 +196,10 @@ func TestSyncLoadBalancerIfNeeded(t *testing.T) {
 					Type: v1.ServiceTypeLoadBalancer,
 				},
 			},
-			expectOp:            ensureLoadBalancer,
+			expectOp:             ensureLoadBalancer,
-			expectCreateAttempt: true,
+			expectCreateAttempt:  true,
-			expectPatchStatus:   true,
+			expectPatchStatus:    true,
+			expectPatchFinalizer: true,
 		},
 		// Finalizer test cases below.
 		{
@@ -259,8 +261,7 @@ func TestSyncLoadBalancerIfNeeded(t *testing.T) {
 			expectPatchFinalizer: true,
 		},
 		{
-			desc:              "service without finalizer that wants LB",
+			desc: "service without finalizer that wants LB",
-			enableFeatureGate: true,
 			service: &v1.Service{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:      "basic-service1",
@@ -281,8 +282,7 @@ func TestSyncLoadBalancerIfNeeded(t *testing.T) {
 			expectPatchFinalizer: true,
 		},
 		{
-			desc:              "service with finalizer that wants LB",
+			desc: "service with finalizer that wants LB",
-			enableFeatureGate: true,
 			service: &v1.Service{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:       "basic-service1",
@@ -307,8 +307,6 @@ func TestSyncLoadBalancerIfNeeded(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.desc, func(t *testing.T) {
-			defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, serviceLoadBalancerFinalizerFeature, tc.enableFeatureGate)()
-
 			controller, cloud, client := newController()
 			cloud.Exists = tc.lbExists
 			key := fmt.Sprintf("%s/%s", tc.service.Namespace, tc.service.Name)
@@ -392,7 +390,7 @@ func TestSyncLoadBalancerIfNeeded(t *testing.T) {
 				}
 			}
 			if numPatches != expectNumPatches {
-				t.Errorf("Expected %d patches, got %d instead. Actions: %v", numPatches, expectNumPatches, actions)
+				t.Errorf("Got %d patches, expect %d instead. Actions: %v", numPatches, expectNumPatches, actions)
 			}
 		})
 	}