Merge pull request #628 from agau4779/release-1.4

Cherry-pick #613 onto 1.4 branch
kubernetes · Feb 6, 2019 · a7199af · a7199af
2 parents bf09ebf + dc5d311
commit a7199af
Show file tree

Hide file tree

Showing 4 changed files with 154 additions and 58 deletions.
diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go
@@ -45,6 +45,7 @@ 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"
@@ -372,11 +373,13 @@ 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 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 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
+				}
 			}
 		}
 	}
@@ -465,29 +468,31 @@ 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 {
+	if !ingExists || utils.IsDeletionCandidate(ing.ObjectMeta, utils.FinalizerKey) {
 		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 err := utils.AddFinalizer(ing, ingClient); err != nil {
-		glog.Errorf("Failed to add Finalizer to Ingress %q: %v", key, err)
-		return err
+	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
+		}
 	}
 
 	// 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,6 +37,7 @@ import (
 	"k8s.io/kubernetes/pkg/cloudprovider/providers/gce"
 
 	"k8s.io/ingress-gce/pkg/context"
+	"k8s.io/ingress-gce/pkg/flags"
 )
 
 var (
@@ -109,9 +110,17 @@ 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) {
-	lbc.ctx.KubeClient.Extensions().Ingresses(ing.Namespace).Delete(ing.Name, &meta_v1.DeleteOptions{})
-	lbc.ctx.IngressInformer.GetIndexer().Delete(ing)
+	if len(ing.GetFinalizers()) == 0 {
+		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.
@@ -153,44 +162,122 @@ func TestIngressSyncError(t *testing.T) {
 	}
 }
 
-// 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()
+// 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",
+		},
+	}
 
-	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)
+	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())
+				}
+			}
 
-	defaultBackend := backend("my-service", intstr.FromInt(80))
-	ing := test.NewIngress(types.NamespacedName{Name: "my-ingress", Namespace: "default"},
-		extensions.IngressSpec{
-			Backend: &defaultBackend,
-		})
-	addIngress(lbc, ing)
+			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)
-	}
+				ingStoreKey := getKey(ing, t)
+				if err := lbc.sync(ingStoreKey); err != nil {
+					t.Fatalf("lbc.sync(%v) = err %v", ingStoreKey, err)
+				}
 
-	// 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)
-	}
+				updatedIng, _ := lbc.ctx.KubeClient.Extensions().Ingresses("default").Get(name, meta_v1.GetOptions{})
+				deleteIngress(lbc, updatedIng)
 
-	deleteIngress(lbc, ing)
-	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{})
+				if tc.enableFinalizerAdd && !tc.enableFinalizerRemove {
+					if updatedIng == nil {
+						t.Fatalf("Expected Ingress not to be deleted")
+					}
+
+					if len(updatedIng.GetFinalizers()) != 1 {
+						t.Errorf("GetFinalizers() = %+v, want 0", updatedIng.GetFinalizers())
+					}
+
+					continue
+				}
 
-	// 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)
+				if updatedIng != nil {
+					t.Fatalf("Ingress was not deleted, got: %+v", updatedIng)
+				}
+
+				remainingIngresses, err := lbc.ctx.KubeClient.Extensions().Ingresses("default").List(meta_v1.ListOptions{})
+				if err != nil {
+					t.Fatalf("List() = err %v", err)
+				}
+
+				remainingIngCount := len(tc.ingNames) - i - 1
+				if len(remainingIngresses.Items) != remainingIngCount {
+					t.Fatalf("Expected %d Ingresses, got: %d", remainingIngCount, len(remainingIngresses.Items))
+				}
+			}
+		})
 	}
 }
 

diff --git a/pkg/flags/flags.go b/pkg/flags/flags.go
@@ -130,13 +130,19 @@ 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 {
@@ -214,6 +220,10 @@ 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,9 +23,8 @@ import (
 	"k8s.io/kubernetes/pkg/util/slice"
 )
 
-// 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"
+// FinalizerKey is the string representing the Ingress finalizer.
+const FinalizerKey = "networking.gke.io/ingress-finalizer"
 
 // IsDeletionCandidate is true if the passed in meta contains the specified finalizer.
 func IsDeletionCandidate(m meta_v1.ObjectMeta, key string) bool {
@@ -42,15 +41,10 @@ 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)
@@ -66,7 +60,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)