Merge pull request #881 from skmatti/keyfunc-fix

BugFix: Update ingress key function used for GC
kubernetes · Oct 2, 2019 · fe7fc86 · fe7fc86
2 parents 7d0b5f2 + e8e06c8
commit fe7fc86
Show file tree

Hide file tree

Showing 4 changed files with 29 additions and 27 deletions.
diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go
@@ -49,7 +49,6 @@ 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/namer"
 	"k8s.io/klog"
 )
 
@@ -128,12 +127,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", namer.IngressKeyFunc(addIng), annotations.IngressClassKey)
+				klog.V(4).Infof("Ignoring add for ingress %v based on annotation %v", utils.IngressKeyFunc(addIng), annotations.IngressClassKey)
 				return
 			}
 
-			klog.V(3).Infof("Ingress %v added, enqueuing", namer.IngressKeyFunc(addIng))
-			lbc.ctx.Recorder(addIng.Namespace).Eventf(addIng, apiv1.EventTypeNormal, "ADD", namer.IngressKeyFunc(addIng))
+			klog.V(3).Infof("Ingress %v added, enqueuing", utils.IngressKeyFunc(addIng))
+			lbc.ctx.Recorder(addIng.Namespace).Eventf(addIng, apiv1.EventTypeNormal, "ADD", utils.IngressKeyFunc(addIng))
 			lbc.ingQueue.Enqueue(obj)
 		},
 		DeleteFunc: func(obj interface{}) {
@@ -143,16 +142,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", namer.IngressKeyFunc(delIng))
+				klog.V(2).Infof("Ignoring delete event for Ingress %v, deletion will be handled via the finalizer", utils.IngressKeyFunc(delIng))
 				return
 			}
 
 			if !utils.IsGLBCIngress(delIng) {
-				klog.V(4).Infof("Ignoring delete for ingress %v based on annotation %v", namer.IngressKeyFunc(delIng), annotations.IngressClassKey)
+				klog.V(4).Infof("Ignoring delete for ingress %v based on annotation %v", utils.IngressKeyFunc(delIng), annotations.IngressClassKey)
 				return
 			}
 
-			klog.V(3).Infof("Ingress %v deleted, enqueueing", namer.IngressKeyFunc(delIng))
+			klog.V(3).Infof("Ingress %v deleted, enqueueing", utils.IngressKeyFunc(delIng))
 			lbc.ingQueue.Enqueue(obj)
 		},
 		UpdateFunc: func(old, cur interface{}) {
@@ -162,16 +161,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", namer.IngressKeyFunc(curIng))
+					klog.V(4).Infof("Ingress %v class was changed, enqueuing", utils.IngressKeyFunc(curIng))
 					lbc.ingQueue.Enqueue(cur)
 					return
 				}
 				return
 			}
 			if reflect.DeepEqual(old, cur) {
-				klog.V(3).Infof("Periodic enqueueing of %v", namer.IngressKeyFunc(curIng))
+				klog.V(3).Infof("Periodic enqueueing of %v", utils.IngressKeyFunc(curIng))
 			} else {
-				klog.V(3).Infof("Ingress %v changed, enqueuing", namer.IngressKeyFunc(curIng))
+				klog.V(3).Infof("Ingress %v changed, enqueuing", utils.IngressKeyFunc(curIng))
 			}
 
 			lbc.ingQueue.Enqueue(cur)
@@ -630,7 +629,14 @@ func toLbNames(ings []*v1beta1.Ingress) []string {
 		if !utils.IsGCEIngress(ing) {
 			continue
 		}
-		lbNames = append(lbNames, namer.IngressKeyFunc(ing))
+		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/neg/controller.go b/pkg/neg/controller.go
@@ -44,7 +44,6 @@ import (
 	"k8s.io/ingress-gce/pkg/neg/readiness"
 	negtypes "k8s.io/ingress-gce/pkg/neg/types"
 	"k8s.io/ingress-gce/pkg/utils"
-	namer_util "k8s.io/ingress-gce/pkg/utils/namer"
 	"k8s.io/klog"
 )
 
@@ -144,15 +143,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", namer_util.IngressKeyFunc(addIng), annotations.IngressClassKey)
+				klog.V(4).Infof("Ignoring add for ingress %v based on annotation %v", utils.IngressKeyFunc(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", namer_util.IngressKeyFunc(delIng), annotations.IngressClassKey)
+				klog.V(4).Infof("Ignoring delete for ingress %v based on annotation %v", utils.IngressKeyFunc(delIng), annotations.IngressClassKey)
 				return
 			}
 			negController.enqueueIngressServices(delIng)
@@ -161,7 +160,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", namer_util.IngressKeyFunc(curIng), annotations.IngressClassKey)
+				klog.V(4).Infof("Ignoring update for ingress %v based on annotation %v", utils.IngressKeyFunc(curIng), annotations.IngressClassKey)
 				return
 			}
 			keys := gatherIngressServiceKeys(oldIng)

diff --git a/pkg/utils/namer/utils.go b/pkg/utils/namer/utils.go
@@ -13,18 +13,6 @@ limitations under the License.
 
 package namer
 
-import (
-	"k8s.io/api/networking/v1beta1"
-	"k8s.io/apimachinery/pkg/types"
-)
-
-func IngressKeyFunc(ing *v1beta1.Ingress) string {
-	if ing == nil {
-		return ""
-	}
-	return types.NamespacedName{Namespace: ing.Namespace, Name: ing.Name}.String()
-}
-
 // TrimFieldsEvenly trims the fields evenly and keeps the total length
 // <= max. Truncation is spread in ratio with their original length,
 // meaning smaller fields will be truncated less than longer ones.

diff --git a/pkg/utils/utils.go b/pkg/utils/utils.go
@@ -349,6 +349,15 @@ func JoinErrs(errs []error) error {
 	return errors.New(strings.Join(errStrs, "; "))
 }
 
+// IngressKeyFunc returns ingress key for given ingress.
+// Note: This is used for logging.
+func IngressKeyFunc(ing *v1beta1.Ingress) string {
+	if ing == nil {
+		return ""
+	}
+	return types.NamespacedName{Namespace: ing.Namespace, Name: ing.Name}.String()
+}
+
 // TraverseIngressBackends traverse thru all backends specified in the input ingress and call process
 // If process return true, then return and stop traversing the backends
 func TraverseIngressBackends(ing *v1beta1.Ingress, process func(id ServicePortID) bool) {