Skip to content

Commit

Permalink
put adding Finalizer behind feature flag
Browse files Browse the repository at this point in the history
  • Loading branch information
@agau4779
agau4779 committed Jan 28, 2019
1 parent fcbc8fb commit 6df3812
Show file tree
Hide file tree
Showing 4 changed files with 148 additions and 52 deletions.
21 changes: 13 additions & 8 deletions pkg/controller/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}
}
}
}
Expand Down Expand Up @@ -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)
Expand All @@ -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
Expand Down
155 changes: 121 additions & 34 deletions pkg/controller/controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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))
}
}
})
}
}

Expand Down
10 changes: 10 additions & 0 deletions pkg/flags/flags.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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,
Expand Down
14 changes: 4 additions & 10 deletions pkg/utils/finalizer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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)
Expand All @@ -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)
Expand Down

0 comments on commit 6df3812

Please sign in to comment.