Check service finalizer on upgrade test and fix-up test cases

pkg/controller/service/service_controller_test.go

@@ -618,7 +618,7 @@ func TestProcessServiceCreateOrUpdate(t *testing.T) {
 }
 
 // TestProcessServiceCreateOrUpdateK8sError tests processServiceCreateOrUpdate
-// with various kubernetes errors.
+// with various kubernetes errors when patching status.
 func TestProcessServiceCreateOrUpdateK8sError(t *testing.T) {
 	svcName := "svc-k8s-err"
 	conflictErr := apierrors.NewConflict(schema.GroupResource{}, svcName, errors.New("object conflict"))
@@ -644,6 +644,8 @@ func TestProcessServiceCreateOrUpdateK8sError(t *testing.T) {
 	for _, tc := range testCases {
 		t.Run(tc.desc, func(t *testing.T) {
 			svc := newService(svcName, types.UID("123"), v1.ServiceTypeLoadBalancer)
+			// Preset finalizer so k8s error only happens when patching status.
+			svc.Finalizers = []string{servicehelper.LoadBalancerCleanupFinalizer}
 			controller, _, client := newController()
 			client.PrependReactor("patch", "services", func(action core.Action) (bool, runtime.Object, error) {
 				return true, nil, tc.k8sErr

test/e2e/framework/service/BUILD

@@ -34,6 +34,7 @@ go_library(
         "//staging/src/k8s.io/client-go/rest:go_default_library",
         "//staging/src/k8s.io/client-go/tools/cache:go_default_library",
         "//staging/src/k8s.io/client-go/util/retry:go_default_library",
+        "//staging/src/k8s.io/cloud-provider/service/helpers:go_default_library",
         "//test/e2e/framework:go_default_library",
         "//test/e2e/framework/log:go_default_library",
         "//test/e2e/framework/node:go_default_library",

test/e2e/framework/service/wait.go

@@ -20,11 +20,15 @@ import (
 	"context"
 	"fmt"
 
-	"github.com/onsi/ginkgo"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/wait"
 	clientset "k8s.io/client-go/kubernetes"
+	servicehelper "k8s.io/cloud-provider/service/helpers"
 	"k8s.io/kubernetes/test/e2e/framework"
 	e2elog "k8s.io/kubernetes/test/e2e/framework/log"
+
+	"github.com/onsi/ginkgo"
 )
 
 // WaitForServiceResponding waits for the service to be responding.
@@ -63,3 +67,52 @@ func WaitForServiceResponding(c clientset.Interface, ns, name string) error {
 		return true, nil
 	})
 }
+
+// WaitForServiceDeletedWithFinalizer waits for the service with finalizer to be deleted.
+func WaitForServiceDeletedWithFinalizer(cs clientset.Interface, namespace, name string) {
+	ginkgo.By("Delete service with finalizer")
+	if err := cs.CoreV1().Services(namespace).Delete(name, nil); err != nil {
+		e2elog.Failf("Failed to delete service %s/%s", namespace, name)
+	}
+
+	ginkgo.By("Wait for service to disappear")
+	if pollErr := wait.PollImmediate(LoadBalancerPollInterval, GetServiceLoadBalancerCreationTimeout(cs), func() (bool, error) {
+		svc, err := cs.CoreV1().Services(namespace).Get(name, metav1.GetOptions{})
+		if err != nil {
+			if apierrors.IsNotFound(err) {
+				e2elog.Logf("Service %s/%s is gone.", namespace, name)
+				return true, nil
+			}
+			return false, err
+		}
+		e2elog.Logf("Service %s/%s still exists with finalizers: %v", namespace, name, svc.Finalizers)
+		return false, nil
+	}); pollErr != nil {
+		e2elog.Failf("Failed to wait for service to disappear: %v", pollErr)
+	}
+}
+
+// WaitForServiceUpdatedWithFinalizer waits for the service to be updated to have or
+// don't have a finalizer.
+func WaitForServiceUpdatedWithFinalizer(cs clientset.Interface, namespace, name string, hasFinalizer bool) {
+	ginkgo.By(fmt.Sprintf("Wait for service to hasFinalizer=%t", hasFinalizer))
+	if pollErr := wait.PollImmediate(LoadBalancerPollInterval, GetServiceLoadBalancerCreationTimeout(cs), func() (bool, error) {
+		svc, err := cs.CoreV1().Services(namespace).Get(name, metav1.GetOptions{})
+		if err != nil {
+			return false, err
+		}
+		foundFinalizer := false
+		for _, finalizer := range svc.Finalizers {
+			if finalizer == servicehelper.LoadBalancerCleanupFinalizer {
+				foundFinalizer = true
+			}
+		}
+		if foundFinalizer != hasFinalizer {
+			e2elog.Logf("Service %s/%s hasFinalizer=%t, want %t", namespace, name, foundFinalizer, hasFinalizer)
+			return false, nil
+		}
+		return true, nil
+	}); pollErr != nil {
+		e2elog.Failf("Failed to wait for service to hasFinalizer=%t: %v", hasFinalizer, pollErr)
+	}
+}

test/e2e/network/service.go

@@ -29,7 +29,6 @@ import (
 	compute "google.golang.org/api/compute/v1"
 
 	v1 "k8s.io/api/core/v1"
-	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/util/intstr"
@@ -1916,39 +1915,13 @@ var _ = SIGDescribe("Services", func() {
 		}
 	})
 
-	// This test verifies if service load balancer cleanup finalizer can be removed
-	// when feature gate isn't enabled on the cluster.
-	// This ensures downgrading from higher version cluster will not break LoadBalancer
-	// type service.
-	ginkgo.It("should remove load balancer cleanup finalizer when service is deleted [Slow]", func() {
-		jig := e2eservice.NewTestJig(cs, "lb-remove-finalizer")
-
-		ginkgo.By("Create load balancer service")
-		svc := jig.CreateTCPServiceOrFail(f.Namespace.Name, func(svc *v1.Service) {
-			svc.Spec.Type = v1.ServiceTypeLoadBalancer
-		})
-
-		defer func() {
-			waitForServiceDeletedWithFinalizer(cs, svc.Namespace, svc.Name)
-		}()
-
-		ginkgo.By("Wait for load balancer to serve traffic")
-		svc = jig.WaitForLoadBalancerOrFail(svc.Namespace, svc.Name, e2eservice.GetServiceLoadBalancerCreationTimeout(cs))
-
-		ginkgo.By("Manually add load balancer cleanup finalizer to service")
-		svc.Finalizers = append(svc.Finalizers, "service.kubernetes.io/load-balancer-cleanup")
-		if _, err := cs.CoreV1().Services(svc.Namespace).Update(svc); err != nil {
-			e2elog.Failf("Failed to add finalizer to service %s/%s: %v", svc.Namespace, svc.Name, err)
-		}
-	})
-
 	// This test verifies if service load balancer cleanup finalizer is properly
 	// handled during service lifecycle.
 	// 1. Create service with type=LoadBalancer. Finalizer should be added.
 	// 2. Update service to type=ClusterIP. Finalizer should be removed.
 	// 3. Update service to type=LoadBalancer. Finalizer should be added.
 	// 4. Delete service with type=LoadBalancer. Finalizer should be removed.
-	ginkgo.It("should handle load balancer cleanup finalizer for service [Slow] [Feature:ServiceFinalizer]", func() {
+	ginkgo.It("should handle load balancer cleanup finalizer for service [Slow]", func() {
 		jig := e2eservice.NewTestJig(cs, "lb-finalizer")
 
 		ginkgo.By("Create load balancer service")
@@ -1957,71 +1930,26 @@ var _ = SIGDescribe("Services", func() {
 		})
 
 		defer func() {
-			waitForServiceDeletedWithFinalizer(cs, svc.Namespace, svc.Name)
+			ginkgo.By("Check that service can be deleted with finalizer")
+			e2eservice.WaitForServiceDeletedWithFinalizer(cs, svc.Namespace, svc.Name)
 		}()
 
 		ginkgo.By("Wait for load balancer to serve traffic")
 		svc = jig.WaitForLoadBalancerOrFail(svc.Namespace, svc.Name, e2eservice.GetServiceLoadBalancerCreationTimeout(cs))
 
 		ginkgo.By("Check if finalizer presents on service with type=LoadBalancer")
-		waitForServiceUpdatedWithFinalizer(cs, svc.Namespace, svc.Name, true)
+		e2eservice.WaitForServiceUpdatedWithFinalizer(cs, svc.Namespace, svc.Name, true)
 
 		ginkgo.By("Check if finalizer is removed on service after changed to type=ClusterIP")
 		jig.ChangeServiceType(svc.Namespace, svc.Name, v1.ServiceTypeClusterIP, e2eservice.GetServiceLoadBalancerCreationTimeout(cs))
-		waitForServiceUpdatedWithFinalizer(cs, svc.Namespace, svc.Name, false)
+		e2eservice.WaitForServiceUpdatedWithFinalizer(cs, svc.Namespace, svc.Name, false)
 
 		ginkgo.By("Check if finalizer is added back to service after changed to type=LoadBalancer")
 		jig.ChangeServiceType(svc.Namespace, svc.Name, v1.ServiceTypeLoadBalancer, e2eservice.GetServiceLoadBalancerCreationTimeout(cs))
-		waitForServiceUpdatedWithFinalizer(cs, svc.Namespace, svc.Name, true)
+		e2eservice.WaitForServiceUpdatedWithFinalizer(cs, svc.Namespace, svc.Name, true)
 	})
 })
 
-func waitForServiceDeletedWithFinalizer(cs clientset.Interface, namespace, name string) {
-	ginkgo.By("Delete service with finalizer")
-	if err := cs.CoreV1().Services(namespace).Delete(name, nil); err != nil {
-		e2elog.Failf("Failed to delete service %s/%s", namespace, name)
-	}
-
-	ginkgo.By("Wait for service to disappear")
-	if pollErr := wait.PollImmediate(e2eservice.LoadBalancerPollInterval, e2eservice.GetServiceLoadBalancerCreationTimeout(cs), func() (bool, error) {
-		svc, err := cs.CoreV1().Services(namespace).Get(name, metav1.GetOptions{})
-		if err != nil {
-			if apierrors.IsNotFound(err) {
-				e2elog.Logf("Service %s/%s is gone.", namespace, name)
-				return true, nil
-			}
-			return false, err
-		}
-		e2elog.Logf("Service %s/%s still exists with finalizers: %v", namespace, name, svc.Finalizers)
-		return false, nil
-	}); pollErr != nil {
-		e2elog.Failf("Failed to wait for service to disappear: %v", pollErr)
-	}
-}
-
-func waitForServiceUpdatedWithFinalizer(cs clientset.Interface, namespace, name string, hasFinalizer bool) {
-	ginkgo.By(fmt.Sprintf("Wait for service to hasFinalizer=%t", hasFinalizer))
-	if pollErr := wait.PollImmediate(e2eservice.LoadBalancerPollInterval, e2eservice.GetServiceLoadBalancerCreationTimeout(cs), func() (bool, error) {
-		svc, err := cs.CoreV1().Services(namespace).Get(name, metav1.GetOptions{})
-		if err != nil {
-			return false, err
-		}
-		foundFinalizer := false
-		for _, finalizer := range svc.Finalizers {
-			if finalizer == "service.kubernetes.io/load-balancer-cleanup" {
-				foundFinalizer = true
-			}
-		}
-		if foundFinalizer != hasFinalizer {
-			e2elog.Logf("Service %s/%s hasFinalizer=%t, want %t", namespace, name, foundFinalizer, hasFinalizer)
-			return false, nil
-		}
-		return true, nil
-	}); pollErr != nil {
-		e2elog.Failf("Failed to wait for service to hasFinalizer=%t: %v", hasFinalizer, pollErr)
-	}
-}
-
 // TODO: Get rid of [DisabledForLargeClusters] tag when issue #56138 is fixed.
 var _ = SIGDescribe("ESIPP [Slow] [DisabledForLargeClusters]", func() {
 	f := framework.NewDefaultFramework("esipp")

test/e2e/upgrades/services.go

@@ -84,12 +84,12 @@ func (t *ServiceUpgradeTest) Setup(f *framework.Framework) {
 func (t *ServiceUpgradeTest) Test(f *framework.Framework, done <-chan struct{}, upgrade UpgradeType) {
 	switch upgrade {
 	case MasterUpgrade, ClusterUpgrade:
-		t.test(f, done, true)
+		t.test(f, done, true, true)
 	case NodeUpgrade:
 		// Node upgrades should test during disruption only on GCE/GKE for now.
-		t.test(f, done, shouldTestPDBs())
+		t.test(f, done, shouldTestPDBs(), false)
 	default:
-		t.test(f, done, false)
+		t.test(f, done, false, false)
 	}
 }
 
@@ -98,7 +98,7 @@ func (t *ServiceUpgradeTest) Teardown(f *framework.Framework) {
 	// rely on the namespace deletion to clean up everything
 }
 
-func (t *ServiceUpgradeTest) test(f *framework.Framework, done <-chan struct{}, testDuringDisruption bool) {
+func (t *ServiceUpgradeTest) test(f *framework.Framework, done <-chan struct{}, testDuringDisruption, testFinalizer bool) {
 	if testDuringDisruption {
 		// Continuous validation
 		ginkgo.By("continuously hitting the pod through the service's LoadBalancer")
@@ -115,4 +115,13 @@ func (t *ServiceUpgradeTest) test(f *framework.Framework, done <-chan struct{},
 	ginkgo.By("hitting the pod through the service's LoadBalancer")
 	t.jig.TestReachableHTTP(t.tcpIngressIP, t.svcPort, e2eservice.LoadBalancerLagTimeoutDefault)
 	t.jig.SanityCheckService(t.tcpService, v1.ServiceTypeLoadBalancer)
+
+	if testFinalizer {
+		defer func() {
+			ginkgo.By("Check that service can be deleted with finalizer")
+			e2eservice.WaitForServiceDeletedWithFinalizer(t.jig.Client, t.tcpService.Namespace, t.tcpService.Name)
+		}()
+		ginkgo.By("Check that finalizer is present on loadBalancer type service")
+		e2eservice.WaitForServiceUpdatedWithFinalizer(t.jig.Client, t.tcpService.Namespace, t.tcpService.Name, true)
+	}
 }