From fcbc8fb915ba6d2c8a294a68d0b7ce0026b3c288 Mon Sep 17 00:00:00 2001 From: Ashley Gau Date: Wed, 16 Jan 2019 14:50:40 -0800 Subject: [PATCH 1/2] add finalizers --- pkg/controller/controller.go | 29 ++++++++++++++++----- pkg/controller/controller_test.go | 6 +++++ pkg/controller/types.go | 5 ++-- pkg/utils/finalizer.go | 42 +++++++++++++++++++++++++++++++ 4 files changed, 74 insertions(+), 8 deletions(-) diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index 090df4995c..9d043f363b 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -368,6 +368,17 @@ 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 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 +429,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,12 +448,12 @@ 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) } + gcState := &gcState{lbc.ctx.Ingresses().List(), lbNames, gceSvcPorts} 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. @@ -454,13 +462,22 @@ func (lbc *LoadBalancerController) sync(key string) error { // 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 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. diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go index 9804bbf011..448ca7fe58 100644 --- a/pkg/controller/controller_test.go +++ b/pkg/controller/controller_test.go @@ -186,6 +186,12 @@ func TestIngressCreateDelete(t *testing.T) { if err := lbc.sync(ingStoreKey); err != nil { t.Fatalf("lbc.sync(%v) = err %v", ingStoreKey, err) } + + // 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) + } } // TestIngressClassChange asserts that `sync` will not return an error for a good ingress config diff --git a/pkg/controller/types.go b/pkg/controller/types.go index c3355c83c2..399985b4e8 100644 --- a/pkg/controller/types.go +++ b/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. diff --git a/pkg/utils/finalizer.go b/pkg/utils/finalizer.go index b228e50ec2..495b690342 100644 --- a/pkg/utils/finalizer.go +++ b/pkg/utils/finalizer.go @@ -14,7 +14,12 @@ 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" ) @@ -36,3 +41,40 @@ 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) } + +// 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() + 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 +} From 6df38123ced0ac0d8ccc3c09934ec3eb00214b32 Mon Sep 17 00:00:00 2001 From: Ashley Gau Date: Tue, 22 Jan 2019 13:08:47 -0800 Subject: [PATCH 2/2] put adding Finalizer behind feature flag --- pkg/controller/controller.go | 21 ++-- pkg/controller/controller_test.go | 155 +++++++++++++++++++++++------- pkg/flags/flags.go | 10 ++ pkg/utils/finalizer.go | 14 +-- 4 files changed, 148 insertions(+), 52 deletions(-) diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index 9d043f363b..67269e8281 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" @@ -370,11 +371,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 + } } } } @@ -454,7 +457,7 @@ func (lbc *LoadBalancerController) sync(key string) error { return fmt.Errorf("error getting Ingress for key %s: %v", key, err) } gcState := &gcState{lbc.ctx.Ingresses().List(), 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) @@ -463,9 +466,11 @@ func (lbc *LoadBalancerController) sync(key string) error { // 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 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 448ca7fe58..d26d1e9c10 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 f62184ea4e..cae0976c21 100644 --- a/pkg/flags/flags.go +++ b/pkg/flags/flags.go @@ -131,6 +131,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{ @@ -138,6 +142,8 @@ var DefaultFeatures = &Features{ 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, 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)