Merge pull request #4102 from aledbf/listingresses

Refactor ListIngresses to add filters

internal/ingress/controller/controller.go

@@ -122,8 +122,7 @@ func (n *NGINXController) syncIngress(interface{}) error {
 		return nil
 	}
 
-	ings := n.store.ListIngresses()
-
+	ings := n.store.ListIngresses(nil)
 	hosts, servers, pcfg := n.getConfiguration(ings)
 
 	if n.runningConfig.Equal(pcfg) {
@@ -202,37 +201,36 @@ func (n *NGINXController) syncIngress(interface{}) error {
 // CheckIngress returns an error in case the provided ingress, when added
 // to the current configuration, generates an invalid configuration
 func (n *NGINXController) CheckIngress(ing *extensions.Ingress) error {
+	//TODO: this is wrong
 	if n == nil {
 		return fmt.Errorf("cannot check ingress on a nil ingress controller")
 	}
+
 	if ing == nil {
 		// no ingress to add, no state change
 		return nil
 	}
+
 	if !class.IsValid(ing) {
 		klog.Infof("ignoring ingress %v in %v based on annotation %v", ing.Name, ing.ObjectMeta.Namespace, class.IngressKey)
 		return nil
 	}
+
 	if n.cfg.Namespace != "" && ing.ObjectMeta.Namespace != n.cfg.Namespace {
 		klog.Infof("ignoring ingress %v in namespace %v different from the namespace watched %s", ing.Name, ing.ObjectMeta.Namespace, n.cfg.Namespace)
 		return nil
 	}
 
-	ings := n.store.ListIngresses()
-	newIngress := &ingress.Ingress{
-		Ingress:           *ing,
-		ParsedAnnotations: annotations.NewAnnotationExtractor(n.store).Extract(ing),
+	filter := func(toCheck *ingress.Ingress) bool {
+		return toCheck.ObjectMeta.Namespace == ing.ObjectMeta.Namespace &&
+			toCheck.ObjectMeta.Name == ing.ObjectMeta.Name
 	}
 
-	for i, ingress := range ings {
-		if ingress.Ingress.ObjectMeta.Name == ing.ObjectMeta.Name && ingress.Ingress.ObjectMeta.Namespace == ing.ObjectMeta.Namespace {
-			ings[i] = newIngress
-			newIngress = nil
-		}
-	}
-	if newIngress != nil {
-		ings = append(ings, newIngress)
-	}
+	ings := n.store.ListIngresses(filter)
+	ings = append(ings, &ingress.Ingress{
+		Ingress:           *ing,
+		ParsedAnnotations: annotations.NewAnnotationExtractor(n.store).Extract(ing),
+	})
 
 	_, _, pcfg := n.getConfiguration(ings)
 
@@ -244,12 +242,14 @@ func (n *NGINXController) CheckIngress(ing *extensions.Ingress) error {
 		n.metricCollector.IncCheckErrorCount(ing.ObjectMeta.Namespace, ing.Name)
 		return err
 	}
+
 	err = n.testTemplate(content)
 	if err != nil {
 		n.metricCollector.IncCheckErrorCount(ing.ObjectMeta.Namespace, ing.Name)
 	} else {
 		n.metricCollector.IncCheckCount(ing.ObjectMeta.Namespace, ing.Name)
 	}
+
 	return err
 }
 

internal/ingress/controller/controller_test.go

@@ -30,12 +30,13 @@ import (
 	"time"
 
 	"github.com/eapache/channels"
-	"k8s.io/api/core/v1"
 	corev1 "k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"
 	extensions "k8s.io/api/extensions/v1beta1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/client-go/kubernetes/fake"
+
 	"k8s.io/ingress-nginx/internal/file"
 	"k8s.io/ingress-nginx/internal/ingress"
 	"k8s.io/ingress-nginx/internal/ingress/annotations"
@@ -76,7 +77,7 @@ func (fakeIngressStore) GetServiceEndpoints(key string) (*corev1.Endpoints, erro
 	return nil, fmt.Errorf("test error")
 }
 
-func (fis fakeIngressStore) ListIngresses() []*ingress.Ingress {
+func (fis fakeIngressStore) ListIngresses(store.IngressFilterFunc) []*ingress.Ingress {
 	return fis.ingresses
 }
 
@@ -209,7 +210,8 @@ func TestCheckIngress(t *testing.T) {
 			nginx.store = fakeIngressStore{
 				ingresses: []*ingress.Ingress{
 					{
-						Ingress: *ing,
+						Ingress:           *ing,
+						ParsedAnnotations: &annotations.Ingress{},
 					},
 				},
 			}

internal/ingress/controller/nginx.go

@@ -45,6 +45,9 @@ import (
 	v1core "k8s.io/client-go/kubernetes/typed/core/v1"
 	"k8s.io/client-go/tools/record"
 	"k8s.io/client-go/util/flowcontrol"
+	"k8s.io/klog"
+	"k8s.io/kubernetes/pkg/util/filesystem"
+
 	adm_controler "k8s.io/ingress-nginx/internal/admission/controller"
 	"k8s.io/ingress-nginx/internal/file"
 	"k8s.io/ingress-nginx/internal/ingress"
@@ -62,8 +65,6 @@ import (
 	"k8s.io/ingress-nginx/internal/nginx"
 	"k8s.io/ingress-nginx/internal/task"
 	"k8s.io/ingress-nginx/internal/watch"
-	"k8s.io/klog"
-	"k8s.io/kubernetes/pkg/util/filesystem"
 )
 
 const (

internal/ingress/controller/store/store.go

@@ -56,6 +56,9 @@ import (
 	"k8s.io/ingress-nginx/internal/k8s"
 )
 
+// IngressFilterFunc decides if an Ingress should be ommited or not
+type IngressFilterFunc func(*ingress.Ingress) bool
+
 // Storer is the interface that wraps the required methods to gather information
 // about ingresses, services, secrets and ingress annotations.
 type Storer interface {
@@ -75,7 +78,7 @@ type Storer interface {
 	GetServiceEndpoints(key string) (*corev1.Endpoints, error)
 
 	// ListIngresses returns a list of all Ingresses in the store.
-	ListIngresses() []*ingress.Ingress
+	ListIngresses(IngressFilterFunc) []*ingress.Ingress
 
 	// GetRunningControllerPodsCount returns the number of Running ingress-nginx controller Pods.
 	GetRunningControllerPodsCount() int
@@ -758,11 +761,16 @@ func (s *k8sStore) getIngress(key string) (*extensions.Ingress, error) {
 }
 
 // ListIngresses returns the list of Ingresses
-func (s *k8sStore) ListIngresses() []*ingress.Ingress {
+func (s *k8sStore) ListIngresses(filter IngressFilterFunc) []*ingress.Ingress {
 	// filter ingress rules
 	ingresses := make([]*ingress.Ingress, 0)
 	for _, item := range s.listers.IngressWithAnnotation.List() {
 		ing := item.(*ingress.Ingress)
+
+		if filter != nil && filter(ing) {
+			continue
+		}
+
 		ingresses = append(ingresses, ing)
 	}
 

internal/ingress/controller/store/store_test.go

@@ -1029,7 +1029,7 @@ func TestListIngresses(t *testing.T) {
 	}
 	s.listers.IngressWithAnnotation.Add(ingressWithNginxClass)
 
-	ingresses := s.ListIngresses()
+	ingresses := s.ListIngresses(nil)
 
 	if s := len(ingresses); s != 3 {
 		t.Errorf("Expected 3 Ingresses but got %v", s)

internal/ingress/status/status.go

@@ -35,6 +35,7 @@ import (
 	"k8s.io/kubernetes/pkg/kubelet/util/sliceutils"
 
 	"k8s.io/ingress-nginx/internal/ingress"
+	"k8s.io/ingress-nginx/internal/ingress/controller/store"
 	"k8s.io/ingress-nginx/internal/k8s"
 	"k8s.io/ingress-nginx/internal/task"
 )
@@ -52,7 +53,7 @@ type Syncer interface {
 
 type ingressLister interface {
 	// ListIngresses returns the list of Ingresses
-	ListIngresses() []*ingress.Ingress
+	ListIngresses(store.IngressFilterFunc) []*ingress.Ingress
 }
 
 // Config ...
@@ -255,7 +256,7 @@ func sliceToStatus(endpoints []string) []apiv1.LoadBalancerIngress {
 
 // updateStatus changes the status information of Ingress rules
 func (s *statusSync) updateStatus(newIngressPoint []apiv1.LoadBalancerIngress) {
-	ings := s.IngressLister.ListIngresses()
+	ings := s.IngressLister.ListIngresses(nil)
 
 	p := pool.NewLimited(10)
 	defer p.Close()

internal/ingress/status/status_test.go

@@ -28,6 +28,7 @@ import (
 
 	"k8s.io/ingress-nginx/internal/ingress"
 	"k8s.io/ingress-nginx/internal/ingress/annotations/class"
+	"k8s.io/ingress-nginx/internal/ingress/controller/store"
 	"k8s.io/ingress-nginx/internal/k8s"
 	"k8s.io/ingress-nginx/internal/task"
 )
@@ -232,7 +233,7 @@ func buildExtensionsIngresses() []extensions.Ingress {
 type testIngressLister struct {
 }
 
-func (til *testIngressLister) ListIngresses() []*ingress.Ingress {
+func (til *testIngressLister) ListIngresses(store.IngressFilterFunc) []*ingress.Ingress {
 	var ingresses []*ingress.Ingress
 	ingresses = append(ingresses, &ingress.Ingress{
 		Ingress: extensions.Ingress{