Merge pull request #883 from skmatti/common-pkg

Refactor ingress key function and finalizer into separate package
kubernetes · Oct 3, 2019 · a52c0ed · a52c0ed
2 parents 7bbb829 + 19f6fb8
commit a52c0ed
Show file tree

Hide file tree

Showing 11 changed files with 116 additions and 76 deletions.
diff --git a/cmd/e2e-test/finalizer_test.go b/cmd/e2e-test/finalizer_test.go
@@ -25,7 +25,7 @@ import (
 	"k8s.io/ingress-gce/pkg/e2e"
 	"k8s.io/ingress-gce/pkg/fuzz"
 	"k8s.io/ingress-gce/pkg/fuzz/features"
-	"k8s.io/ingress-gce/pkg/utils"
+	"k8s.io/ingress-gce/pkg/utils/common"
 )
 
 // TestFinalizer asserts that finalizer is added/ removed appropriately during the life cycle
@@ -58,8 +58,8 @@ func TestFinalizer(t *testing.T) {
 		ing = waitForStableIngress(true, ing, s, t)
 
 		ingFinalizers := ing.GetFinalizers()
-		if len(ingFinalizers) != 1 || ingFinalizers[0] != utils.FinalizerKey {
-			t.Fatalf("GetFinalizers() = %+v, want [%q]", ingFinalizers, utils.FinalizerKey)
+		if len(ingFinalizers) != 1 || ingFinalizers[0] != common.FinalizerKey {
+			t.Fatalf("GetFinalizers() = %+v, want [%q]", ingFinalizers, common.FinalizerKey)
 		}
 
 		gclb := checkGCLB(t, s, ing, numForwardingRules, numBackendServices)
@@ -103,8 +103,8 @@ func TestFinalizerIngressClassChange(t *testing.T) {
 		ing = waitForStableIngress(true, ing, s, t)
 
 		ingFinalizers := ing.GetFinalizers()
-		if len(ingFinalizers) != 1 || ingFinalizers[0] != utils.FinalizerKey {
-			t.Fatalf("GetFinalizers() = %+v, want [%q]", ingFinalizers, utils.FinalizerKey)
+		if len(ingFinalizers) != 1 || ingFinalizers[0] != common.FinalizerKey {
+			t.Fatalf("GetFinalizers() = %+v, want [%q]", ingFinalizers, common.FinalizerKey)
 		}
 
 		gclb := checkGCLB(t, s, ing, numForwardingRules, numBackendServices)
@@ -170,12 +170,12 @@ func TestFinalizerIngressesWithSharedResources(t *testing.T) {
 		otherIng = waitForStableIngress(true, otherIng, s, t)
 
 		ingFinalizers := ing.GetFinalizers()
-		if len(ingFinalizers) != 1 || ingFinalizers[0] != utils.FinalizerKey {
-			t.Fatalf("GetFinalizers() = %+v, want [%q]", ingFinalizers, utils.FinalizerKey)
+		if len(ingFinalizers) != 1 || ingFinalizers[0] != common.FinalizerKey {
+			t.Fatalf("GetFinalizers() = %+v, want [%q]", ingFinalizers, common.FinalizerKey)
 		}
 		otherIngFinalizers := otherIng.GetFinalizers()
-		if len(otherIngFinalizers) != 1 || otherIngFinalizers[0] != utils.FinalizerKey {
-			t.Fatalf("GetFinalizers() = %+v, want [%q]", otherIngFinalizers, utils.FinalizerKey)
+		if len(otherIngFinalizers) != 1 || otherIngFinalizers[0] != common.FinalizerKey {
+			t.Fatalf("GetFinalizers() = %+v, want [%q]", otherIngFinalizers, common.FinalizerKey)
 		}
 
 		gclb := checkGCLB(t, s, ing, numForwardingRules, numBackendServices)

diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go
@@ -49,6 +49,7 @@ import (
 	ingsync "k8s.io/ingress-gce/pkg/sync"
 	"k8s.io/ingress-gce/pkg/tls"
 	"k8s.io/ingress-gce/pkg/utils"
+	"k8s.io/ingress-gce/pkg/utils/common"
 	"k8s.io/klog"
 )
 
@@ -127,12 +128,12 @@ func NewLoadBalancerController(
 		AddFunc: func(obj interface{}) {
 			addIng := obj.(*v1beta1.Ingress)
 			if !utils.IsGLBCIngress(addIng) {
-				klog.V(4).Infof("Ignoring add for ingress %v based on annotation %v", utils.IngressKeyFunc(addIng), annotations.IngressClassKey)
+				klog.V(4).Infof("Ignoring add for ingress %v based on annotation %v", common.ToString(addIng), annotations.IngressClassKey)
 				return
 			}
 
-			klog.V(3).Infof("Ingress %v added, enqueuing", utils.IngressKeyFunc(addIng))
-			lbc.ctx.Recorder(addIng.Namespace).Eventf(addIng, apiv1.EventTypeNormal, "ADD", utils.IngressKeyFunc(addIng))
+			klog.V(3).Infof("Ingress %v added, enqueuing", common.ToString(addIng))
+			lbc.ctx.Recorder(addIng.Namespace).Eventf(addIng, apiv1.EventTypeNormal, "ADD", common.ToString(addIng))
 			lbc.ingQueue.Enqueue(obj)
 		},
 		DeleteFunc: func(obj interface{}) {
@@ -142,16 +143,16 @@ func NewLoadBalancerController(
 				return
 			}
 			if delIng.ObjectMeta.DeletionTimestamp != nil {
-				klog.V(2).Infof("Ignoring delete event for Ingress %v, deletion will be handled via the finalizer", utils.IngressKeyFunc(delIng))
+				klog.V(2).Infof("Ignoring delete event for Ingress %v, deletion will be handled via the finalizer", common.ToString(delIng))
 				return
 			}
 
 			if !utils.IsGLBCIngress(delIng) {
-				klog.V(4).Infof("Ignoring delete for ingress %v based on annotation %v", utils.IngressKeyFunc(delIng), annotations.IngressClassKey)
+				klog.V(4).Infof("Ignoring delete for ingress %v based on annotation %v", common.ToString(delIng), annotations.IngressClassKey)
 				return
 			}
 
-			klog.V(3).Infof("Ingress %v deleted, enqueueing", utils.IngressKeyFunc(delIng))
+			klog.V(3).Infof("Ingress %v deleted, enqueueing", common.ToString(delIng))
 			lbc.ingQueue.Enqueue(obj)
 		},
 		UpdateFunc: func(old, cur interface{}) {
@@ -161,16 +162,16 @@ func NewLoadBalancerController(
 				// If ingress was GLBC Ingress, we need to track ingress class change
 				// and run GC to delete LB resources.
 				if utils.IsGLBCIngress(oldIng) {
-					klog.V(4).Infof("Ingress %v class was changed, enqueuing", utils.IngressKeyFunc(curIng))
+					klog.V(4).Infof("Ingress %v class was changed, enqueuing", common.ToString(curIng))
 					lbc.ingQueue.Enqueue(cur)
 					return
 				}
 				return
 			}
 			if reflect.DeepEqual(old, cur) {
-				klog.V(3).Infof("Periodic enqueueing of %v", utils.IngressKeyFunc(curIng))
+				klog.V(3).Infof("Periodic enqueueing of %v", common.ToString(curIng))
 			} else {
-				klog.V(3).Infof("Ingress %v changed, enqueuing", utils.IngressKeyFunc(curIng))
+				klog.V(3).Infof("Ingress %v changed, enqueuing", common.ToString(curIng))
 			}
 
 			lbc.ingQueue.Enqueue(cur)
@@ -376,11 +377,14 @@ func (lbc *LoadBalancerController) SyncBackends(state interface{}) error {
 
 // GCBackends implements Controller.
 func (lbc *LoadBalancerController) GCBackends(toKeep []*v1beta1.Ingress) error {
-	svcPortsToKeep := lbc.ToSvcPorts(toKeep)
+	// Only GCE ingress associated resources are managed by this controller.
+	GCEIngresses := operator.Ingresses(toKeep).Filter(utils.IsGCEIngress).AsList()
+	svcPortsToKeep := lbc.ToSvcPorts(GCEIngresses)
 	if err := lbc.backendSyncer.GC(svcPortsToKeep); err != nil {
 		return err
 	}
 	// TODO(ingress#120): Move this to the backend pool so it mirrors creation
+	// Do not delete instance group if there exists a GLBC ingress.
 	if len(toKeep) == 0 {
 		igName := lbc.ctx.ClusterNamer.InstanceGroup()
 		klog.Infof("Deleting instance group %v", igName)
@@ -416,7 +420,9 @@ func (lbc *LoadBalancerController) SyncLoadBalancer(state interface{}) error {
 
 // GCLoadBalancers implements Controller.
 func (lbc *LoadBalancerController) GCLoadBalancers(toKeep []*v1beta1.Ingress) error {
-	return lbc.l7Pool.GC(toLbNames(toKeep))
+	// Only GCE ingress associated resources are managed by this controller.
+	GCEIngresses := operator.Ingresses(toKeep).Filter(utils.IsGCEIngress).AsList()
+	return lbc.l7Pool.GC(common.ToIngressKeys(GCEIngresses))
 }
 
 // MaybeRemoveFinalizers cleans up Finalizers if needed.
@@ -427,7 +433,7 @@ func (lbc *LoadBalancerController) MaybeRemoveFinalizers(toCleanup []*v1beta1.In
 	}
 	for _, ing := range toCleanup {
 		ingClient := lbc.ctx.KubeClient.NetworkingV1beta1().Ingresses(ing.Namespace)
-		if err := utils.RemoveFinalizer(ing, ingClient); err != nil {
+		if err := common.RemoveFinalizer(ing, ingClient); err != nil {
 			klog.Errorf("Failed to remove Finalizer from Ingress %s/%s: %v", ing.Namespace, ing.Name, err)
 			return err
 		}
@@ -472,7 +478,7 @@ func (lbc *LoadBalancerController) sync(key string) error {
 	ing = ing.DeepCopy()
 	ingClient := lbc.ctx.KubeClient.NetworkingV1beta1().Ingresses(ing.Namespace)
 	if flags.F.FinalizerAdd {
-		if err := utils.AddFinalizer(ing, ingClient); err != nil {
+		if err := common.AddFinalizer(ing, ingClient); err != nil {
 			klog.Errorf("Failed to add Finalizer to Ingress %q: %v", key, err)
 			return err
 		}
@@ -547,7 +553,7 @@ func (lbc *LoadBalancerController) updateIngressStatus(l7 *loadbalancers.L7, ing
 
 // toRuntimeInfo returns L7RuntimeInfo for the given ingress.
 func (lbc *LoadBalancerController) toRuntimeInfo(ing *v1beta1.Ingress, urlMap *utils.GCEURLMap) (*loadbalancers.L7RuntimeInfo, error) {
-	k, err := utils.KeyFunc(ing)
+	k, err := common.KeyFunc(ing)
 	if err != nil {
 		return nil, fmt.Errorf("cannot get key for Ingress %v/%v: %v", ing.Namespace, ing.Name, err)
 	}
@@ -606,37 +612,13 @@ func updateAnnotations(client kubernetes.Interface, name, namespace string, anno
 	return nil
 }
 
-// ToSvcPorts returns a list of SVC ports owned by this controller given a list of ingresses.
+// ToSvcPorts returns a list of SVC ports given a list of ingresses.
 // Note: This method is used for GC.
 func (lbc *LoadBalancerController) ToSvcPorts(ings []*v1beta1.Ingress) []utils.ServicePort {
 	var knownPorts []utils.ServicePort
 	for _, ing := range ings {
-		// Only resources associated with GCE Ingress are managed by this controller.
-		if !utils.IsGCEIngress(ing) {
-			continue
-		}
 		urlMap, _ := lbc.Translator.TranslateIngress(ing, lbc.ctx.DefaultBackendSvcPort.ID)
 		knownPorts = append(knownPorts, urlMap.AllServicePorts()...)
 	}
 	return knownPorts
 }
-
-// toLbNames returns a list of load balancers owned by this controller given a list of ingresses.
-func toLbNames(ings []*v1beta1.Ingress) []string {
-	lbNames := make([]string, 0, len(ings))
-	for _, ing := range ings {
-		// Only resources associated with GCE Ingress are managed by this controller.
-		if !utils.IsGCEIngress(ing) {
-			continue
-		}
-		ingKey, err := utils.KeyFunc(ing)
-		// An error is returned only if ingress object does not have a valid meta.
-		// So, logging the error would be sufficient here.
-		if err != nil {
-			klog.Errorf("Cannot get key for Ingress %v/%v: %v", ing.Namespace, ing.Name, err)
-			continue
-		}
-		lbNames = append(lbNames, ingKey)
-	}
-	return lbNames
-}
diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go
@@ -41,6 +41,7 @@ import (
 	"k8s.io/ingress-gce/pkg/test"
 	"k8s.io/ingress-gce/pkg/tls"
 	"k8s.io/ingress-gce/pkg/utils"
+	"k8s.io/ingress-gce/pkg/utils/common"
 	namer_util "k8s.io/ingress-gce/pkg/utils/namer"
 	"k8s.io/legacy-cloud-providers/gce"
 )
@@ -143,7 +144,7 @@ func deleteIngress(lbc *LoadBalancerController, ing *v1beta1.Ingress) {
 
 // getKey returns the key for an ingress.
 func getKey(ing *v1beta1.Ingress, t *testing.T) string {
-	key, err := utils.KeyFunc(ing)
+	key, err := common.KeyFunc(ing)
 	if err != nil {
 		t.Fatalf("Unexpected error getting key for Ingress %v: %v", ing.Name, err)
 	}
@@ -336,8 +337,8 @@ func TestIngressClassChangeWithFinalizer(t *testing.T) {
 		t.Fatalf("Get(%v) = %v, want nil", ingStoreKey, err)
 	}
 	ingFinalizers := updatedIng.GetFinalizers()
-	if len(ingFinalizers) != 1 || ingFinalizers[0] != utils.FinalizerKey {
-		t.Fatalf("updatedIng.GetFinalizers() = %+v, want [%s]; failed to add finalizer, updatedIng = %+v", ingFinalizers, utils.FinalizerKey, updatedIng)
+	if len(ingFinalizers) != 1 || ingFinalizers[0] != common.FinalizerKey {
+		t.Fatalf("updatedIng.GetFinalizers() = %+v, want [%s]; failed to add finalizer, updatedIng = %+v", ingFinalizers, common.FinalizerKey, updatedIng)
 	}
 
 	anns[annotations.IngressClassKey] = "new-class"
@@ -470,8 +471,8 @@ func TestEnableFinalizer(t *testing.T) {
 		t.Fatalf("Get(%v) = %v, want nil", ingStoreKey, err)
 	}
 	ingFinalizers := updatedIng.GetFinalizers()
-	if len(ingFinalizers) != 1 || ingFinalizers[0] != utils.FinalizerKey {
-		t.Fatalf("updatedIng.GetFinalizers() = %+v, want [%s]; failed to add finalizer, updatedIng = %+v", ingFinalizers, utils.FinalizerKey, updatedIng)
+	if len(ingFinalizers) != 1 || ingFinalizers[0] != common.FinalizerKey {
+		t.Fatalf("updatedIng.GetFinalizers() = %+v, want [%s]; failed to add finalizer, updatedIng = %+v", ingFinalizers, common.FinalizerKey, updatedIng)
 	}
 }
 

diff --git a/pkg/e2e/helpers.go b/pkg/e2e/helpers.go
@@ -18,14 +18,13 @@ package e2e
 
 import (
 	"context"
-	"fmt"
-	"net"
-	"time"
-
 	"encoding/json"
+	"fmt"
 	"io/ioutil"
+	"net"
 	"net/http"
 	"strings"
+	"time"
 
 	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud"
 	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta"
@@ -41,7 +40,7 @@ import (
 	"k8s.io/ingress-gce/pkg/fuzz"
 	"k8s.io/ingress-gce/pkg/fuzz/features"
 	"k8s.io/ingress-gce/pkg/fuzz/whitebox"
-	"k8s.io/ingress-gce/pkg/utils"
+	"k8s.io/ingress-gce/pkg/utils/common"
 	"k8s.io/klog"
 )
 
@@ -131,7 +130,7 @@ func WaitForFinalizer(s *Sandbox, ingName string) error {
 			return false, nil
 		}
 		ingFinalizers := ing.GetFinalizers()
-		if len(ingFinalizers) != 1 || ingFinalizers[0] != utils.FinalizerKey {
+		if len(ingFinalizers) != 1 || ingFinalizers[0] != common.FinalizerKey {
 			klog.Infof("WaitForFinalizer(%s/%s) = %v, finalizer not added for Ingress %v", s.Namespace, ingName, ingFinalizers, ing)
 			return false, nil
 		}

diff --git a/pkg/firewalls/controller_test.go b/pkg/firewalls/controller_test.go
@@ -27,6 +27,7 @@ import (
 	backendconfigclient "k8s.io/ingress-gce/pkg/backendconfig/client/clientset/versioned/fake"
 	test "k8s.io/ingress-gce/pkg/test"
 	"k8s.io/ingress-gce/pkg/utils"
+	"k8s.io/ingress-gce/pkg/utils/common"
 	"k8s.io/legacy-cloud-providers/gce"
 
 	meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -77,7 +78,7 @@ func TestFirewallCreateDelete(t *testing.T) {
 	fwc.ctx.KubeClient.NetworkingV1beta1().Ingresses(ing.Namespace).Create(ing)
 	fwc.ctx.IngressInformer.GetIndexer().Add(ing)
 
-	key, _ := utils.KeyFunc(queueKey)
+	key, _ := common.KeyFunc(queueKey)
 	if err := fwc.sync(key); err != nil {
 		t.Fatalf("fwc.sync() = %v, want nil", err)
 	}

diff --git a/pkg/fuzz/features/neg.go b/pkg/fuzz/features/neg.go
@@ -34,6 +34,7 @@ import (
 	"k8s.io/ingress-gce/pkg/annotations"
 	"k8s.io/ingress-gce/pkg/fuzz"
 	"k8s.io/ingress-gce/pkg/utils"
+	"k8s.io/ingress-gce/pkg/utils/common"
 )
 
 // NEG is a feature in GCP to support pod as Loadbalancer backends
@@ -86,7 +87,7 @@ func (v *negValidator) CheckResponse(host, path string, resp *http.Response, bod
 		return fuzz.CheckResponseContinue, err
 	}
 
-	key, err := utils.KeyFunc(v.ing)
+	key, err := common.KeyFunc(v.ing)
 	if err != nil {
 		return fuzz.CheckResponseContinue, err
 	}

diff --git a/pkg/neg/controller.go b/pkg/neg/controller.go
@@ -44,6 +44,7 @@ import (
 	"k8s.io/ingress-gce/pkg/neg/readiness"
 	negtypes "k8s.io/ingress-gce/pkg/neg/types"
 	"k8s.io/ingress-gce/pkg/utils"
+	"k8s.io/ingress-gce/pkg/utils/common"
 	"k8s.io/klog"
 )
 
@@ -143,15 +144,15 @@ func NewController(
 		AddFunc: func(obj interface{}) {
 			addIng := obj.(*v1beta1.Ingress)
 			if !utils.IsGLBCIngress(addIng) {
-				klog.V(4).Infof("Ignoring add for ingress %v based on annotation %v", utils.IngressKeyFunc(addIng), annotations.IngressClassKey)
+				klog.V(4).Infof("Ignoring add for ingress %v based on annotation %v", common.ToString(addIng), annotations.IngressClassKey)
 				return
 			}
 			negController.enqueueIngressServices(addIng)
 		},
 		DeleteFunc: func(obj interface{}) {
 			delIng := obj.(*v1beta1.Ingress)
 			if !utils.IsGLBCIngress(delIng) {
-				klog.V(4).Infof("Ignoring delete for ingress %v based on annotation %v", utils.IngressKeyFunc(delIng), annotations.IngressClassKey)
+				klog.V(4).Infof("Ignoring delete for ingress %v based on annotation %v", common.ToString(delIng), annotations.IngressClassKey)
 				return
 			}
 			negController.enqueueIngressServices(delIng)
@@ -160,7 +161,7 @@ func NewController(
 			oldIng := cur.(*v1beta1.Ingress)
 			curIng := cur.(*v1beta1.Ingress)
 			if !utils.IsGLBCIngress(curIng) {
-				klog.V(4).Infof("Ignoring update for ingress %v based on annotation %v", utils.IngressKeyFunc(curIng), annotations.IngressClassKey)
+				klog.V(4).Infof("Ignoring update for ingress %v based on annotation %v", common.ToString(curIng), annotations.IngressClassKey)
 				return
 			}
 			keys := gatherIngressServiceKeys(oldIng)