Merge pull request #3437 from Shopify/ingress-annotations

Use struct to pack Ingress and its annotations

internal/ingress/controller/controller.go

@@ -372,18 +372,14 @@ func (n *NGINXController) getDefaultUpstream() *ingress.Backend {
 // getBackendServers returns a list of Upstream and Server to be used by the
 // backend.  An upstream can be used in multiple servers if the namespace,
 // service name and port are the same.
-func (n *NGINXController) getBackendServers(ingresses []*extensions.Ingress) ([]*ingress.Backend, []*ingress.Server) {
+func (n *NGINXController) getBackendServers(ingresses []*ingress.Ingress) ([]*ingress.Backend, []*ingress.Server) {
 	du := n.getDefaultUpstream()
 	upstreams := n.createUpstreams(ingresses, du)
 	servers := n.createServers(ingresses, upstreams, du)
 
 	for _, ing := range ingresses {
 		ingKey := k8s.MetaNamespaceKey(ing)
-
-		anns, err := n.store.GetIngressAnnotations(ingKey)
-		if err != nil {
-			glog.Errorf("Error getting Ingress annotations %q: %v", ingKey, err)
-		}
+		anns := ing.ParsedAnnotations
 
 		for _, rule := range ing.Spec.Rules {
 			host := rule.Host
@@ -631,17 +627,12 @@ func (n *NGINXController) getBackendServers(ingresses []*extensions.Ingress) ([]
 
 // createUpstreams creates the NGINX upstreams (Endpoints) for each Service
 // referenced in Ingress rules.
-func (n *NGINXController) createUpstreams(data []*extensions.Ingress, du *ingress.Backend) map[string]*ingress.Backend {
+func (n *NGINXController) createUpstreams(data []*ingress.Ingress, du *ingress.Backend) map[string]*ingress.Backend {
 	upstreams := make(map[string]*ingress.Backend)
 	upstreams[defUpstreamName] = du
 
 	for _, ing := range data {
-		ingKey := k8s.MetaNamespaceKey(ing)
-
-		anns, err := n.store.GetIngressAnnotations(ingKey)
-		if err != nil {
-			glog.Errorf("Error getting Ingress annotations %q: %v", ingKey, err)
-		}
+		anns := ing.ParsedAnnotations
 
 		var defBackend string
 		if ing.Spec.Backend != nil {
@@ -878,7 +869,7 @@ func (n *NGINXController) serviceEndpoints(svcKey, backendPort string) ([]ingres
 // createServers builds a map of host name to Server structs from a map of
 // already computed Upstream structs. Each Server is configured with at least
 // one root location, which uses a default backend if left unspecified.
-func (n *NGINXController) createServers(data []*extensions.Ingress,
+func (n *NGINXController) createServers(data []*ingress.Ingress,
 	upstreams map[string]*ingress.Backend,
 	du *ingress.Backend) map[string]*ingress.Server {
 
@@ -932,11 +923,7 @@ func (n *NGINXController) createServers(data []*extensions.Ingress,
 	// initialize all other servers
 	for _, ing := range data {
 		ingKey := k8s.MetaNamespaceKey(ing)
-
-		anns, err := n.store.GetIngressAnnotations(ingKey)
-		if err != nil {
-			glog.Errorf("Error getting Ingress annotations %q: %v", ingKey, err)
-		}
+		anns := ing.ParsedAnnotations
 
 		// default upstream name
 		un := du.Name
@@ -1015,11 +1002,7 @@ func (n *NGINXController) createServers(data []*extensions.Ingress,
 	// configure default location, alias, and SSL
 	for _, ing := range data {
 		ingKey := k8s.MetaNamespaceKey(ing)
-
-		anns, err := n.store.GetIngressAnnotations(ingKey)
-		if err != nil {
-			glog.Errorf("Error getting Ingress annotations %q: %v", ingKey, err)
-		}
+		anns := ing.ParsedAnnotations
 
 		for _, rule := range ing.Spec.Rules {
 			host := rule.Host
@@ -1126,7 +1109,7 @@ func (n *NGINXController) createServers(data []*extensions.Ingress,
 // If a match is found, we know that this server should back the alternative backend and add the alternative backend
 // to a backend's alternative list.
 // If no match is found, then the serverless backend is deleted.
-func mergeAlternativeBackends(ing *extensions.Ingress, upstreams map[string]*ingress.Backend,
+func mergeAlternativeBackends(ing *ingress.Ingress, upstreams map[string]*ingress.Backend,
 	servers map[string]*ingress.Server) {
 
 	// merge catch-all alternative backends
@@ -1183,7 +1166,7 @@ func mergeAlternativeBackends(ing *extensions.Ingress, upstreams map[string]*ing
 
 // extractTLSSecretName returns the name of the Secret containing a SSL
 // certificate for the given host name, or an empty string.
-func extractTLSSecretName(host string, ing *extensions.Ingress,
+func extractTLSSecretName(host string, ing *ingress.Ingress,
 	getLocalSSLCert func(string) (*ingress.SSLCert, error)) string {
 
 	if ing == nil {

internal/ingress/controller/controller_test.go

@@ -20,9 +20,10 @@ import (
 	"crypto/x509"
 	"crypto/x509/pkix"
 	"encoding/asn1"
-	"k8s.io/apimachinery/pkg/util/intstr"
 	"testing"
 
+	"k8s.io/apimachinery/pkg/util/intstr"
+
 	extensions "k8s.io/api/extensions/v1beta1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/ingress-nginx/internal/ingress"
@@ -30,31 +31,33 @@ import (
 
 func TestMergeAlternativeBackends(t *testing.T) {
 	testCases := map[string]struct {
-		ingress                   *extensions.Ingress
+		ingress                   *ingress.Ingress
 		upstreams                 map[string]*ingress.Backend
 		servers                   map[string]*ingress.Server
 		expNumAlternativeBackends int
 		expNumLocations           int
 	}{
 		"alternative backend has no server and embeds into matching real backend": {
-			&extensions.Ingress{
-				ObjectMeta: metav1.ObjectMeta{
-					Namespace: "example",
-				},
-				Spec: extensions.IngressSpec{
-					Rules: []extensions.IngressRule{
-						{
-							Host: "example.com",
-							IngressRuleValue: extensions.IngressRuleValue{
-								HTTP: &extensions.HTTPIngressRuleValue{
-									Paths: []extensions.HTTPIngressPath{
-										{
-											Path: "/",
-											Backend: extensions.IngressBackend{
-												ServiceName: "http-svc-canary",
-												ServicePort: intstr.IntOrString{
-													Type:   intstr.Int,
-													IntVal: 80,
+			&ingress.Ingress{
+				Ingress: extensions.Ingress{
+					ObjectMeta: metav1.ObjectMeta{
+						Namespace: "example",
+					},
+					Spec: extensions.IngressSpec{
+						Rules: []extensions.IngressRule{
+							{
+								Host: "example.com",
+								IngressRuleValue: extensions.IngressRuleValue{
+									HTTP: &extensions.HTTPIngressRuleValue{
+										Paths: []extensions.HTTPIngressPath{
+											{
+												Path: "/",
+												Backend: extensions.IngressBackend{
+													ServiceName: "http-svc-canary",
+													ServicePort: intstr.IntOrString{
+														Type:   intstr.Int,
+														IntVal: 80,
+													},
 												},
 											},
 										},
@@ -93,43 +96,45 @@ func TestMergeAlternativeBackends(t *testing.T) {
 			1,
 		},
 		"merging a alternative backend matches with the correct host": {
-			&extensions.Ingress{
-				ObjectMeta: metav1.ObjectMeta{
-					Namespace: "example",
-				},
-				Spec: extensions.IngressSpec{
-					Rules: []extensions.IngressRule{
-						{
-							Host: "foo.bar",
-							IngressRuleValue: extensions.IngressRuleValue{
-								HTTP: &extensions.HTTPIngressRuleValue{
-									Paths: []extensions.HTTPIngressPath{
-										{
-											Path: "/",
-											Backend: extensions.IngressBackend{
-												ServiceName: "foo-http-svc-canary",
-												ServicePort: intstr.IntOrString{
-													Type:   intstr.Int,
-													IntVal: 80,
+			&ingress.Ingress{
+				Ingress: extensions.Ingress{
+					ObjectMeta: metav1.ObjectMeta{
+						Namespace: "example",
+					},
+					Spec: extensions.IngressSpec{
+						Rules: []extensions.IngressRule{
+							{
+								Host: "foo.bar",
+								IngressRuleValue: extensions.IngressRuleValue{
+									HTTP: &extensions.HTTPIngressRuleValue{
+										Paths: []extensions.HTTPIngressPath{
+											{
+												Path: "/",
+												Backend: extensions.IngressBackend{
+													ServiceName: "foo-http-svc-canary",
+													ServicePort: intstr.IntOrString{
+														Type:   intstr.Int,
+														IntVal: 80,
+													},
 												},
 											},
 										},
 									},
 								},
 							},
-						},
-						{
-							Host: "example.com",
-							IngressRuleValue: extensions.IngressRuleValue{
-								HTTP: &extensions.HTTPIngressRuleValue{
-									Paths: []extensions.HTTPIngressPath{
-										{
-											Path: "/",
-											Backend: extensions.IngressBackend{
-												ServiceName: "http-svc-canary",
-												ServicePort: intstr.IntOrString{
-													Type:   intstr.Int,
-													IntVal: 80,
+							{
+								Host: "example.com",
+								IngressRuleValue: extensions.IngressRuleValue{
+									HTTP: &extensions.HTTPIngressRuleValue{
+										Paths: []extensions.HTTPIngressPath{
+											{
+												Path: "/",
+												Backend: extensions.IngressBackend{
+													ServiceName: "http-svc-canary",
+													ServicePort: intstr.IntOrString{
+														Type:   intstr.Int,
+														IntVal: 80,
+													},
 												},
 											},
 										},
@@ -209,7 +214,7 @@ func TestMergeAlternativeBackends(t *testing.T) {
 func TestExtractTLSSecretName(t *testing.T) {
 	testCases := map[string]struct {
 		host    string
-		ingress *extensions.Ingress
+		ingress *ingress.Ingress
 		fn      func(string) (*ingress.SSLCert, error)
 		expName string
 	}{
@@ -223,7 +228,7 @@ func TestExtractTLSSecretName(t *testing.T) {
 		},
 		"empty ingress": {
 			"foo.bar",
-			&extensions.Ingress{},
+			&ingress.Ingress{},
 			func(string) (*ingress.SSLCert, error) {
 				return nil, nil
 			},
@@ -231,17 +236,19 @@ func TestExtractTLSSecretName(t *testing.T) {
 		},
 		"ingress tls, nil secret": {
 			"foo.bar",
-			&extensions.Ingress{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "test",
-				},
-				Spec: extensions.IngressSpec{
-					TLS: []extensions.IngressTLS{
-						{SecretName: "demo"},
+			&ingress.Ingress{
+				Ingress: extensions.Ingress{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "test",
 					},
-					Rules: []extensions.IngressRule{
-						{
-							Host: "foo.bar",
+					Spec: extensions.IngressSpec{
+						TLS: []extensions.IngressTLS{
+							{SecretName: "demo"},
+						},
+						Rules: []extensions.IngressRule{
+							{
+								Host: "foo.bar",
+							},
 						},
 					},
 				},
@@ -253,17 +260,19 @@ func TestExtractTLSSecretName(t *testing.T) {
 		},
 		"ingress tls, no host, matching cert cn": {
 			"foo.bar",
-			&extensions.Ingress{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "test",
-				},
-				Spec: extensions.IngressSpec{
-					TLS: []extensions.IngressTLS{
-						{SecretName: "demo"},
+			&ingress.Ingress{
+				Ingress: extensions.Ingress{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "test",
 					},
-					Rules: []extensions.IngressRule{
-						{
-							Host: "foo.bar",
+					Spec: extensions.IngressSpec{
+						TLS: []extensions.IngressTLS{
+							{SecretName: "demo"},
+						},
+						Rules: []extensions.IngressRule{
+							{
+								Host: "foo.bar",
+							},
 						},
 					},
 				},
@@ -277,19 +286,21 @@ func TestExtractTLSSecretName(t *testing.T) {
 		},
 		"ingress tls, no host, wildcard cert with matching cn": {
 			"foo.bar",
-			&extensions.Ingress{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "test",
-				},
-				Spec: extensions.IngressSpec{
-					TLS: []extensions.IngressTLS{
-						{
-							SecretName: "demo",
-						},
+			&ingress.Ingress{
+				Ingress: extensions.Ingress{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "test",
 					},
-					Rules: []extensions.IngressRule{
-						{
-							Host: "test.foo.bar",
+					Spec: extensions.IngressSpec{
+						TLS: []extensions.IngressTLS{
+							{
+								SecretName: "demo",
+							},
+						},
+						Rules: []extensions.IngressRule{
+							{
+								Host: "test.foo.bar",
+							},
 						},
 					},
 				},
@@ -303,20 +314,22 @@ func TestExtractTLSSecretName(t *testing.T) {
 		},
 		"ingress tls, hosts, matching cert cn": {
 			"foo.bar",
-			&extensions.Ingress{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "test",
-				},
-				Spec: extensions.IngressSpec{
-					TLS: []extensions.IngressTLS{
-						{
-							Hosts:      []string{"foo.bar", "example.com"},
-							SecretName: "demo",
-						},
+			&ingress.Ingress{
+				Ingress: extensions.Ingress{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "test",
 					},
-					Rules: []extensions.IngressRule{
-						{
-							Host: "foo.bar",
+					Spec: extensions.IngressSpec{
+						TLS: []extensions.IngressTLS{
+							{
+								Hosts:      []string{"foo.bar", "example.com"},
+								SecretName: "demo",
+							},
+						},
+						Rules: []extensions.IngressRule{
+							{
+								Host: "foo.bar",
+							},
 						},
 					},
 				},

internal/ingress/controller/store/store.go

@@ -74,10 +74,7 @@ type Storer interface {
 	GetIngress(key string) (*extensions.Ingress, error)
 
 	// ListIngresses returns a list of all Ingresses in the store.
-	ListIngresses() []*extensions.Ingress
-
-	// GetIngressAnnotations returns the parsed annotations of an Ingress matching key.
-	GetIngressAnnotations(key string) (*annotations.Ingress, error)
+	ListIngresses() []*ingress.Ingress
 
 	// GetLocalSSLCert returns the local copy of a SSLCert
 	GetLocalSSLCert(name string) (*ingress.SSLCert, error)
@@ -644,15 +641,22 @@ func (s k8sStore) GetIngress(key string) (*extensions.Ingress, error) {
 }
 
 // ListIngresses returns the list of Ingresses
-func (s k8sStore) ListIngresses() []*extensions.Ingress {
+func (s k8sStore) ListIngresses() []*ingress.Ingress {
 	// filter ingress rules
-	var ingresses []*extensions.Ingress
+	var ingresses []*ingress.Ingress
 	for _, item := range s.listers.Ingress.List() {
 		ing := item.(*extensions.Ingress)
 		if !class.IsValid(ing) {
 			continue
 		}
 
+		ingKey := k8s.MetaNamespaceKey(ing)
+		anns, err := s.getIngressAnnotations(ingKey)
+		if err != nil {
+			glog.Errorf("Error getting Ingress annotations %q: %v", ingKey, err)
+
+		}
+
 		for ri, rule := range ing.Spec.Rules {
 			if rule.HTTP == nil {
 				continue
@@ -665,14 +669,17 @@ func (s k8sStore) ListIngresses() []*extensions.Ingress {
 			}
 		}
 
-		ingresses = append(ingresses, ing)
+		ingresses = append(ingresses, &ingress.Ingress{
+			Ingress:           *ing,
+			ParsedAnnotations: anns,
+		})
 	}
 
 	return ingresses
 }
 
-// GetIngressAnnotations returns the parsed annotations of an Ingress matching key.
-func (s k8sStore) GetIngressAnnotations(key string) (*annotations.Ingress, error) {
+// getIngressAnnotations returns the parsed annotations of an Ingress matching key.
+func (s k8sStore) getIngressAnnotations(key string) (*annotations.Ingress, error) {
 	ia, err := s.listers.IngressAnnotation.ByKey(key)
 	if err != nil {
 		return &annotations.Ingress{}, err

internal/ingress/controller/store/store_test.go

@@ -730,7 +730,8 @@ func newStore(t *testing.T) *k8sStore {
 	return &k8sStore{
 		listers: &Lister{
 			// add more listers if needed
-			Ingress: IngressLister{cache.NewStore(cache.MetaNamespaceKeyFunc)},
+			Ingress:           IngressLister{cache.NewStore(cache.MetaNamespaceKeyFunc)},
+			IngressAnnotation: IngressAnnotationsLister{cache.NewStore(cache.DeletionHandlingMetaNamespaceKeyFunc)},
 		},
 		sslStore:         NewSSLCertTracker(),
 		filesystem:       fs,

internal/ingress/controller/template/template.go

@@ -34,7 +34,6 @@ import (
 	"github.com/golang/glog"
 	"github.com/pkg/errors"
 
-	extensions "k8s.io/api/extensions/v1beta1"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/ingress-nginx/internal/file"
 	"k8s.io/ingress-nginx/internal/ingress"
@@ -796,9 +795,9 @@ type ingressInformation struct {
 }
 
 func getIngressInformation(i, p interface{}) *ingressInformation {
-	ing, ok := i.(*extensions.Ingress)
+	ing, ok := i.(*ingress.Ingress)
 	if !ok {
-		glog.Errorf("expected an '*extensions.Ingress' type but %T was returned", i)
+		glog.Errorf("expected an '*ingress.Ingress' type but %T was returned", i)
 		return &ingressInformation{}
 	}
 

internal/ingress/status/status.go

@@ -30,7 +30,6 @@ import (
 
 	pool "gopkg.in/go-playground/pool.v3"
 	apiv1 "k8s.io/api/core/v1"
-	extensions "k8s.io/api/extensions/v1beta1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/util/wait"
@@ -41,6 +40,7 @@ import (
 	"k8s.io/client-go/tools/record"
 	"k8s.io/kubernetes/pkg/kubelet/util/sliceutils"
 
+	"k8s.io/ingress-nginx/internal/ingress"
 	"k8s.io/ingress-nginx/internal/k8s"
 	"k8s.io/ingress-nginx/internal/task"
 )
@@ -57,7 +57,7 @@ type Sync interface {
 
 type ingressLister interface {
 	// ListIngresses returns the list of Ingresses
-	ListIngresses() []*extensions.Ingress
+	ListIngresses() []*ingress.Ingress
 }
 
 // Config ...
@@ -371,7 +371,7 @@ func (s *statusSync) updateStatus(newIngressPoint []apiv1.LoadBalancerIngress) {
 	batch.WaitAll()
 }
 
-func runUpdate(ing *extensions.Ingress, status []apiv1.LoadBalancerIngress,
+func runUpdate(ing *ingress.Ingress, status []apiv1.LoadBalancerIngress,
 	client clientset.Interface) pool.WorkFunc {
 	return func(wu pool.WorkUnit) (interface{}, error) {
 		if wu.IsCancelled() {

internal/ingress/status/status_test.go

@@ -26,6 +26,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	testclient "k8s.io/client-go/kubernetes/fake"
 
+	"k8s.io/ingress-nginx/internal/ingress"
 	"k8s.io/ingress-nginx/internal/ingress/annotations/class"
 	"k8s.io/ingress-nginx/internal/k8s"
 	"k8s.io/ingress-nginx/internal/task"
@@ -231,25 +232,27 @@ func buildExtensionsIngresses() []extensions.Ingress {
 type testIngressLister struct {
 }
 
-func (til *testIngressLister) ListIngresses() []*extensions.Ingress {
-	var ingresses []*extensions.Ingress
-	ingresses = append(ingresses, &extensions.Ingress{
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      "foo_ingress_non_01",
-			Namespace: apiv1.NamespaceDefault,
-		}})
+func (til *testIngressLister) ListIngresses() []*ingress.Ingress {
+	var ingresses []*ingress.Ingress
+	ingresses = append(ingresses, &ingress.Ingress{
+		Ingress: extensions.Ingress{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "foo_ingress_non_01",
+				Namespace: apiv1.NamespaceDefault,
+			}}})
 
-	ingresses = append(ingresses, &extensions.Ingress{
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      "foo_ingress_1",
-			Namespace: apiv1.NamespaceDefault,
-		},
-		Status: extensions.IngressStatus{
-			LoadBalancer: apiv1.LoadBalancerStatus{
-				Ingress: buildLoadBalancerIngressByIP(),
+	ingresses = append(ingresses, &ingress.Ingress{
+		Ingress: extensions.Ingress{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "foo_ingress_1",
+				Namespace: apiv1.NamespaceDefault,
 			},
-		},
-	})
+			Status: extensions.IngressStatus{
+				LoadBalancer: apiv1.LoadBalancerStatus{
+					Ingress: buildLoadBalancerIngressByIP(),
+				},
+			},
+		}})
 
 	return ingresses
 }

internal/ingress/types.go

@@ -20,6 +20,7 @@ import (
 	apiv1 "k8s.io/api/core/v1"
 	extensions "k8s.io/api/extensions/v1beta1"
 	"k8s.io/apimachinery/pkg/util/intstr"
+	"k8s.io/ingress-nginx/internal/ingress/annotations"
 	"k8s.io/ingress-nginx/internal/ingress/annotations/modsecurity"
 
 	"k8s.io/ingress-nginx/internal/ingress/annotations/auth"
@@ -209,7 +210,7 @@ type Location struct {
 	// uses the default backend.
 	IsDefBackend bool `json:"isDefBackend"`
 	// Ingress returns the ingress from which this location was generated
-	Ingress *extensions.Ingress `json:"ingress"`
+	Ingress *Ingress `json:"ingress"`
 	// Backend describes the name of the backend to use.
 	Backend string `json:"backend"`
 	// Service describes the referenced services from the ingress
@@ -331,3 +332,9 @@ type ProxyProtocol struct {
 	Decode bool `json:"decode"`
 	Encode bool `json:"encode"`
 }
+
+// Ingress holds the definition of an Ingress plus its annotations
+type Ingress struct {
+	extensions.Ingress
+	ParsedAnnotations *annotations.Ingress
+}