Merge pull request #635 from kubernetes/revert-628-release-1.4

Revert "Cherry-pick #613 onto 1.4 branch"
kubernetes · Feb 12, 2019 · d647902 · d647902
2 parents ccde57d + db2d8ae
commit d647902
Show file tree

Hide file tree

Showing 4 changed files with 58 additions and 154 deletions.
diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go
@@ -45,7 +45,6 @@ import (
 	"k8s.io/ingress-gce/pkg/annotations"
 	"k8s.io/ingress-gce/pkg/context"
 	"k8s.io/ingress-gce/pkg/controller/translator"
-	"k8s.io/ingress-gce/pkg/flags"
 	"k8s.io/ingress-gce/pkg/loadbalancers"
 	ingsync "k8s.io/ingress-gce/pkg/sync"
 	"k8s.io/ingress-gce/pkg/tls"
@@ -373,13 +372,11 @@ func (lbc *LoadBalancerController) GCBackends(state interface{}) error {
 	}
 
 	for _, ing := range gcState.ingresses {
-		if utils.IsDeletionCandidate(ing.ObjectMeta, utils.FinalizerKey) {
+		if utils.IsDeletionCandidate(ing.ObjectMeta, utils.FinalizerKey()) {
 			ingClient := lbc.ctx.KubeClient.Extensions().Ingresses(ing.Namespace)
-			if flags.F.Features.FinalizerRemove {
-				if err := utils.RemoveFinalizer(&ing, ingClient); err != nil {
-					glog.Errorf("Failed to remove Finalizer from Ingress %v/%v: %v", ing.Namespace, ing.Name, err)
-					return err
-				}
+			if err := utils.RemoveFinalizer(&ing, ingClient); err != nil {
+				glog.Errorf("Failed to remove Finalizer from Ingress %v/%v: %v", ing.Namespace, ing.Name, err)
+				return err
 			}
 		}
 	}
@@ -468,31 +465,29 @@ func (lbc *LoadBalancerController) sync(key string) error {
 	if err != nil {
 		return fmt.Errorf("error getting Ingress for key %s: %v", key, err)
 	}
-	// Get ingress and DeepCopy for assurance that we don't pollute other goroutines with changes.
-	ing, ok := obj.(*extensions.Ingress)
-	if !ok {
-		return fmt.Errorf("invalid object (not of type Ingress), type was %T", obj)
-	}
-
 	ingList, err := lbc.ingLister.ListGCEIngresses()
 	if err != nil {
 		return fmt.Errorf("error getting list of Ingresses: %v", err)
 
 	}
 	gcState := &gcState{ingList.Items, lbNames, gceSvcPorts}
-	if !ingExists || utils.IsDeletionCandidate(ing.ObjectMeta, utils.FinalizerKey) {
+	if !ingExists {
 		glog.V(2).Infof("Ingress %q no longer exists, triggering GC", key)
 		// GC will find GCE resources that were used for this ingress and delete them.
 		return lbc.ingSyncer.GC(gcState)
 	}
 
+	// Get ingress and DeepCopy for assurance that we don't pollute other goroutines with changes.
+	ing, ok := obj.(*extensions.Ingress)
+	if !ok {
+		return fmt.Errorf("invalid object (not of type Ingress), type was %T", obj)
+	}
+
 	ing = ing.DeepCopy()
 	ingClient := lbc.ctx.KubeClient.Extensions().Ingresses(ing.Namespace)
-	if flags.F.Features.FinalizerAdd {
-		if err := utils.AddFinalizer(ing, ingClient); err != nil {
-			glog.Errorf("Failed to add Finalizer to Ingress %q: %v", key, err)
-			return err
-		}
+	if err := utils.AddFinalizer(ing, ingClient); err != nil {
+		glog.Errorf("Failed to add Finalizer to Ingress %q: %v", key, err)
+		return err
 	}
 
 	// Check if ingress class was changed to non-GLBC to remove ingress LB from state and trigger GC

diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go
@@ -37,7 +37,6 @@ import (
 	"k8s.io/kubernetes/pkg/cloudprovider/providers/gce"
 
 	"k8s.io/ingress-gce/pkg/context"
-	"k8s.io/ingress-gce/pkg/flags"
 )
 
 var (
@@ -110,17 +109,9 @@ func updateIngress(lbc *LoadBalancerController, ing *extensions.Ingress) {
 	lbc.ctx.IngressInformer.GetIndexer().Update(ing)
 }
 
-func setDeletionTimestamp(lbc *LoadBalancerController, ing *extensions.Ingress) {
-	ts := meta_v1.NewTime(time.Now())
-	ing.SetDeletionTimestamp(&ts)
-	updateIngress(lbc, ing)
-}
-
 func deleteIngress(lbc *LoadBalancerController, ing *extensions.Ingress) {
-	if len(ing.GetFinalizers()) == 0 {
-		lbc.ctx.KubeClient.Extensions().Ingresses(ing.Namespace).Delete(ing.Name, &meta_v1.DeleteOptions{})
-		lbc.ctx.IngressInformer.GetIndexer().Delete(ing)
-	}
+	lbc.ctx.KubeClient.Extensions().Ingresses(ing.Namespace).Delete(ing.Name, &meta_v1.DeleteOptions{})
+	lbc.ctx.IngressInformer.GetIndexer().Delete(ing)
 }
 
 // getKey returns the key for an ingress.
@@ -162,122 +153,44 @@ func TestIngressSyncError(t *testing.T) {
 	}
 }
 
-// TestIngressCreateDeleteFinalizer asserts that `sync` will will not return an
-// error for a good ingress config. It also tests garbage collection for
-// Ingresses that need to be deleted, and keep the ones that don't, depending
-// on whether Finalizer Adds and/or Removes are enabled.
-func TestIngressCreateDeleteFinalizer(t *testing.T) {
-	testCases := []struct {
-		enableFinalizerAdd    bool
-		enableFinalizerRemove bool
-		ingNames              []string
-		desc                  string
-	}{
-		{
-			enableFinalizerAdd:    true,
-			enableFinalizerRemove: true,
-			ingNames:              []string{"ing-1", "ing-2", "ing-3"},
-			desc:                  "both FinalizerAdd and FinalizerRemove are enabled",
-		},
-		{
-			ingNames: []string{"ing-1", "ing-2", "ing-3"},
-			desc:     "both FinalizerAdd and FinalizerRemove are disabled",
-		},
-		{
-			enableFinalizerAdd: true,
-			ingNames:           []string{"ing-1", "ing-2", "ing-3"},
-			desc:               "FinalizerAdd is enabled",
-		},
-		{
-			enableFinalizerRemove: true,
-			ingNames:              []string{"ing-1", "ing-2", "ing-3"},
-			desc:                  "FinalizerRemove is enabled",
-		},
-	}
-
-	for _, tc := range testCases {
-		t.Run(tc.desc, func(t *testing.T) {
-			flags.F.Features.FinalizerAdd = tc.enableFinalizerAdd
-			flags.F.Features.FinalizerRemove = tc.enableFinalizerRemove
-
-			lbc := newLoadBalancerController()
-			svc := test.NewService(types.NamespacedName{Name: "my-service", Namespace: "default"}, api_v1.ServiceSpec{
-				Type:  api_v1.ServiceTypeNodePort,
-				Ports: []api_v1.ServicePort{{Port: 80}},
-			})
-			addService(lbc, svc)
-			defaultBackend := backend("my-service", intstr.FromInt(80))
-
-			for _, name := range tc.ingNames {
-				ing := test.NewIngress(types.NamespacedName{Name: name, Namespace: "default"},
-					extensions.IngressSpec{
-						Backend: &defaultBackend,
-					})
-				addIngress(lbc, ing)
-
-				ingStoreKey := getKey(ing, t)
-				if err := lbc.sync(ingStoreKey); err != nil {
-					t.Fatalf("lbc.sync(%v) = err %v", ingStoreKey, err)
-				}
-
-				updatedIng, _ := lbc.ctx.KubeClient.Extensions().Ingresses(ing.Namespace).Get(ing.Name, meta_v1.GetOptions{})
-
-				// Check Ingress status has IP.
-				if len(updatedIng.Status.LoadBalancer.Ingress) != 1 || updatedIng.Status.LoadBalancer.Ingress[0].IP == "" {
-					t.Errorf("Get(%q) = status %+v, want non-empty", updatedIng.Name, updatedIng.Status.LoadBalancer.Ingress)
-				}
-
-				// Check Ingress has Finalizer if the FinalizerAdd flag is true
-				if tc.enableFinalizerAdd && len(updatedIng.GetFinalizers()) != 1 {
-					t.Errorf("GetFinalizers() = %+v, want 1", updatedIng.GetFinalizers())
-				}
-
-				// Check Ingress DOES NOT have Finalizer if FinalizerAdd flag is false
-				if !tc.enableFinalizerAdd && len(updatedIng.GetFinalizers()) == 1 {
-					t.Errorf("GetFinalizers() = %+v, want 0", updatedIng.GetFinalizers())
-				}
-			}
-
-			for i, name := range tc.ingNames {
-				ing, _ := lbc.ctx.KubeClient.Extensions().Ingresses("default").Get(name, meta_v1.GetOptions{})
-				setDeletionTimestamp(lbc, ing)
-
-				ingStoreKey := getKey(ing, t)
-				if err := lbc.sync(ingStoreKey); err != nil {
-					t.Fatalf("lbc.sync(%v) = err %v", ingStoreKey, err)
-				}
-
-				updatedIng, _ := lbc.ctx.KubeClient.Extensions().Ingresses("default").Get(name, meta_v1.GetOptions{})
-				deleteIngress(lbc, updatedIng)
+// TestIngressCreateDelete asserts that `sync` will not return an error for a good ingress config
+// and will not return an error when the ingress is deleted.
+func TestIngressCreateDelete(t *testing.T) {
+	lbc := newLoadBalancerController()
 
-				updatedIng, _ = lbc.ctx.KubeClient.Extensions().Ingresses("default").Get(name, meta_v1.GetOptions{})
-				if tc.enableFinalizerAdd && !tc.enableFinalizerRemove {
-					if updatedIng == nil {
-						t.Fatalf("Expected Ingress not to be deleted")
-					}
+	svc := test.NewService(types.NamespacedName{Name: "my-service", Namespace: "default"}, api_v1.ServiceSpec{
+		Type:  api_v1.ServiceTypeNodePort,
+		Ports: []api_v1.ServicePort{{Port: 80}},
+	})
+	addService(lbc, svc)
 
-					if len(updatedIng.GetFinalizers()) != 1 {
-						t.Errorf("GetFinalizers() = %+v, want 0", updatedIng.GetFinalizers())
-					}
+	defaultBackend := backend("my-service", intstr.FromInt(80))
+	ing := test.NewIngress(types.NamespacedName{Name: "my-ingress", Namespace: "default"},
+		extensions.IngressSpec{
+			Backend: &defaultBackend,
+		})
+	addIngress(lbc, ing)
 
-					continue
-				}
+	ingStoreKey := getKey(ing, t)
+	if err := lbc.sync(ingStoreKey); err != nil {
+		t.Fatalf("lbc.sync(%v) = err %v", ingStoreKey, err)
+	}
 
-				if updatedIng != nil {
-					t.Fatalf("Ingress was not deleted, got: %+v", updatedIng)
-				}
+	// Check Ingress status has IP.
+	updatedIng, _ := lbc.ctx.KubeClient.Extensions().Ingresses(ing.Namespace).Get(ing.Name, meta_v1.GetOptions{})
+	if len(updatedIng.Status.LoadBalancer.Ingress) != 1 || updatedIng.Status.LoadBalancer.Ingress[0].IP == "" {
+		t.Errorf("Get(%q) = status %+v, want non-empty", updatedIng.Name, updatedIng.Status.LoadBalancer.Ingress)
+	}
 
-				remainingIngresses, err := lbc.ctx.KubeClient.Extensions().Ingresses("default").List(meta_v1.ListOptions{})
-				if err != nil {
-					t.Fatalf("List() = err %v", err)
-				}
+	deleteIngress(lbc, ing)
+	if err := lbc.sync(ingStoreKey); err != nil {
+		t.Fatalf("lbc.sync(%v) = err %v", ingStoreKey, err)
+	}
 
-				remainingIngCount := len(tc.ingNames) - i - 1
-				if len(remainingIngresses.Items) != remainingIngCount {
-					t.Fatalf("Expected %d Ingresses, got: %d", remainingIngCount, len(remainingIngresses.Items))
-				}
-			}
-		})
+	// Check Ingress has been deleted
+	updatedIng, _ = lbc.ctx.KubeClient.Extensions().Ingresses(ing.Namespace).Get(ing.Name, meta_v1.GetOptions{})
+	if updatedIng != nil {
+		t.Fatalf("Ingress was not deleted, got: %+v", updatedIng)
 	}
 }
 

diff --git a/pkg/flags/flags.go b/pkg/flags/flags.go
@@ -130,19 +130,13 @@ type Features struct {
 	NEGExposed bool
 	// ManagedCertificates enables using ManagedCertificate CRD
 	ManagedCertificates bool
-	// FinalizerAdd enables adding a finalizer on Ingress
-	FinalizerAdd bool
-	// FinalizerRemove enables removing a finalizer on Ingress.
-	FinalizerRemove bool
 }
 
 var DefaultFeatures = &Features{
 	Http2:               true,
 	NEG:                 true,
 	NEGExposed:          true,
 	ManagedCertificates: false,
-	FinalizerAdd:        false,
-	FinalizerRemove:     false,
 }
 
 func EnabledFeatures() *Features {
@@ -220,10 +214,6 @@ L7 load balancing. CSV values accepted. Example: -node-port-ranges=80,8080,400-5
 	flag.StringVar(&F.LeaderElection.LockObjectNamespace, "lock-object-namespace", F.LeaderElection.LockObjectNamespace, "Define the namespace of the lock object.")
 	flag.StringVar(&F.LeaderElection.LockObjectName, "lock-object-name", F.LeaderElection.LockObjectName, "Define the name of the lock object.")
 	flag.BoolVar(&F.Features.ManagedCertificates, "enable-managed-certificates", F.Features.ManagedCertificates, "Enable ManagedCertificates.")
-	flag.BoolVar(&F.Features.FinalizerAdd, "enable-finalizer-add",
-		F.Features.FinalizerAdd, "Enable adding Finalizer to Ingress.")
-	flag.BoolVar(&F.Features.FinalizerRemove, "enable-finalizer-remove",
-		F.Features.FinalizerRemove, "Enable removing Finalizer from Ingress.")
 
 	// Deprecated F.
 	flag.BoolVar(&F.Verbose, "verbose", false,

diff --git a/pkg/utils/finalizer.go b/pkg/utils/finalizer.go
@@ -23,8 +23,9 @@ import (
 	"k8s.io/kubernetes/pkg/util/slice"
 )
 
-// FinalizerKey is the string representing the Ingress finalizer.
-const FinalizerKey = "networking.gke.io/ingress-finalizer"
+// FinalizerKeySuffix is a suffix for finalizers added by the controller.
+// A full key could be something like "ingress.finalizer.cloud.google.com"
+const FinalizerKeySuffix = "finalizer.cloud.google.com"
 
 // IsDeletionCandidate is true if the passed in meta contains the specified finalizer.
 func IsDeletionCandidate(m meta_v1.ObjectMeta, key string) bool {
@@ -41,10 +42,15 @@ func HasFinalizer(m meta_v1.ObjectMeta, key string) bool {
 	return slice.ContainsString(m.Finalizers, key, nil)
 }
 
+// FinalizerKey generates the finalizer string for Ingress
+func FinalizerKey() string {
+	return fmt.Sprintf("%s.%s", "ingress", FinalizerKeySuffix)
+}
+
 // AddFinalizer tries to add a finalizer to an Ingress. If a finalizer
 // already exists, it does nothing.
 func AddFinalizer(ing *extensions.Ingress, ingClient client.IngressInterface) error {
-	ingKey := FinalizerKey
+	ingKey := FinalizerKey()
 	if NeedToAddFinalizer(ing.ObjectMeta, ingKey) {
 		updated := ing.DeepCopy()
 		updated.ObjectMeta.Finalizers = append(updated.ObjectMeta.Finalizers, ingKey)
@@ -60,7 +66,7 @@ func AddFinalizer(ing *extensions.Ingress, ingClient client.IngressInterface) er
 // RemoveFinalizer tries to remove a Finalizer from an Ingress. If a
 // finalizer is not on the Ingress, it does nothing.
 func RemoveFinalizer(ing *extensions.Ingress, ingClient client.IngressInterface) error {
-	ingKey := FinalizerKey
+	ingKey := FinalizerKey()
 	if HasFinalizer(ing.ObjectMeta, ingKey) {
 		updated := ing.DeepCopy()
 		updated.ObjectMeta.Finalizers = slice.RemoveString(updated.ObjectMeta.Finalizers, ingKey, nil)