Merge pull request #613 from agau4779/finalizers

Add Finalizers for Ingress

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"
@@ -368,6 +369,19 @@ func (lbc *LoadBalancerController) GCBackends(state interface{}) error {
 			return err
 		}
 	}
+
+	for _, ing := range gcState.ingresses {
+		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
+				}
+			}
+		}
+	}
+
 	return nil
 }
 
@@ -418,10 +432,7 @@ func (lbc *LoadBalancerController) PostProcess(state interface{}) error {
 	}
 
 	// Update the ingress status.
-	if err := lbc.updateIngressStatus(syncState.l7, syncState.ing); err != nil {
-		return fmt.Errorf("update ingress status error: %v", err)
-	}
-	return nil
+	return lbc.updateIngressStatus(syncState.l7, syncState.ing)
 }
 
 // sync manages Ingress create/updates/deletes events from queue.
@@ -440,27 +451,38 @@ func (lbc *LoadBalancerController) sync(key string) error {
 	// gceSvcPorts contains the ServicePorts used by only single-cluster ingress.
 	gceSvcPorts := lbc.ToSvcPorts(gceIngresses)
 	lbNames := lbc.ctx.Ingresses().ListKeys()
-	gcState := &gcState{lbNames, gceSvcPorts}
 
 	ing, ingExists, err := lbc.ctx.Ingresses().GetByKey(key)
 	if err != nil {
 		return fmt.Errorf("error getting Ingress for key %s: %v", key, err)
 	}
-	if !ingExists {
+	gcState := &gcState{lbc.ctx.Ingresses().List(), lbNames, gceSvcPorts}
+	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 = 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
+		}
+	}
 
 	// Check if ingress class was changed to non-GLBC to remove ingress LB from state and trigger GC
 	if !utils.IsGLBCIngress(ing) {
 		glog.V(2).Infof("Ingress %q class was changed, triggering GC", key)
 		// Remove lb from state for GC
 		gcState.lbNames = slice.RemoveString(gcState.lbNames, key, nil)
-		return lbc.ingSyncer.GC(gcState)
+		if gcErr := lbc.ingSyncer.GC(gcState); gcErr != nil {
+			return gcErr
+		}
+
+		return nil
 	}
 
 	// Bootstrap state for GCP sync.

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,38 +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
+				}
+
+				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))
+				}
+			}
+		})
 	}
 }
 

pkg/controller/types.go

@@ -25,8 +25,9 @@ import (
 
 // gcState is used by the controller to maintain state for garbage collection routines.
 type gcState struct {
-	lbNames  []string
-	svcPorts []utils.ServicePort
+	ingresses []*extensions.Ingress
+	lbNames   []string
+	svcPorts  []utils.ServicePort
 }
 
 // syncState is used by the controller to maintain state for routines that sync GCP resources of an Ingress.

pkg/flags/flags.go

@@ -131,13 +131,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 {
@@ -218,6 +224,10 @@ L7 load balancing. CSV values accepted. Example: -node-port-ranges=80,8080,400-5
 	flag.DurationVar(&F.NegGCPeriod, "neg-gc-period", 120*time.Second,
 		`Relist and garbage collect NEGs this often.`)
 	flag.StringVar(&F.NegSyncerType, "neg-syncer-type", "transaction", "Define the NEG syncer type to use. Valid values are \"batch\" and \"transaction\"")
+	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,

pkg/utils/finalizer.go

@@ -14,13 +14,17 @@ limitations under the License.
 package utils
 
 import (
+	"fmt"
+
+	"github.com/golang/glog"
+	extensions "k8s.io/api/extensions/v1beta1"
 	meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	client "k8s.io/client-go/kubernetes/typed/extensions/v1beta1"
 	"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 {
@@ -36,3 +40,35 @@ func NeedToAddFinalizer(m meta_v1.ObjectMeta, key string) bool {
 func HasFinalizer(m meta_v1.ObjectMeta, key string) bool {
 	return slice.ContainsString(m.Finalizers, key, nil)
 }
+
+// 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
+	if NeedToAddFinalizer(ing.ObjectMeta, ingKey) {
+		updated := ing.DeepCopy()
+		updated.ObjectMeta.Finalizers = append(updated.ObjectMeta.Finalizers, ingKey)
+		if _, err := ingClient.Update(updated); err != nil {
+			return fmt.Errorf("error updating Ingress %s/%s: %v", ing.Namespace, ing.Name, err)
+		}
+		glog.V(3).Infof("Added finalizer %q for Ingress %s/%s", ingKey, ing.Namespace, ing.Name)
+	}
+
+	return nil
+}
+
+// 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
+	if HasFinalizer(ing.ObjectMeta, ingKey) {
+		updated := ing.DeepCopy()
+		updated.ObjectMeta.Finalizers = slice.RemoveString(updated.ObjectMeta.Finalizers, ingKey, nil)
+		if _, err := ingClient.Update(updated); err != nil {
+			return fmt.Errorf("error updating Ingress %s/%s: %v", ing.Namespace, ing.Name, err)
+		}
+		glog.V(3).Infof("Removed finalizer %q for Ingress %s/%s", ingKey, ing.Namespace, ing.Name)
+	}
+
+	return nil
+}