From dc5d3119f9a58833861c86ac3d0a09d1a0253b8b Mon Sep 17 00:00:00 2001 From: Ashley Gau Date: Tue, 22 Jan 2019 13:08:47 -0800 Subject: [PATCH] put adding Finalizer behind feature flag --- pkg/controller/controller.go | 33 ++++--- pkg/controller/controller_test.go | 155 +++++++++++++++++++++++------- pkg/flags/flags.go | 10 ++ pkg/utils/finalizer.go | 14 +-- 4 files changed, 154 insertions(+), 58 deletions(-) diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index 2057bac501..c41d28776b 100644 --- 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 index 790d5d64e5..9cb8372bdc 100644 --- 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 index cbeaf75948..5f4d9e94bf 100644 --- a/pkg/flags/flags.go +++ b/pkg/flags/flags.go @@ -130,6 +130,10 @@ 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{ @@ -137,6 +141,8 @@ var DefaultFeatures = &Features{ 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 index 495b690342..fb93af61fa 100644 --- 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)