Fix a bug with Canary becoming main server

internal/ingress/controller/controller.go

@@ -940,8 +940,13 @@ func (n *NGINXController) createServers(data []*ingress.Ingress,
 		// default upstream name
 		un := du.Name
 
+		if anns.Canary.Enabled {
+			klog.V(2).Infof("Ingress %v is marked as Canary, ignoring", ingKey)
+			continue
+		}
+
 		if ing.Spec.Backend != nil {
-			defUpstream := fmt.Sprintf("%v-%v-%v", ing.Namespace, ing.Spec.Backend.ServiceName, ing.Spec.Backend.ServicePort.String())
+			defUpstream := upstreamName(ing.Namespace, ing.Spec.Backend.ServiceName, ing.Spec.Backend.ServicePort)
 
 			if backendUpstream, ok := upstreams[defUpstream]; ok {
 				// use backend specified in Ingress as the default backend for all its rules
@@ -1016,6 +1021,11 @@ func (n *NGINXController) createServers(data []*ingress.Ingress,
 		ingKey := k8s.MetaNamespaceKey(ing)
 		anns := ing.ParsedAnnotations
 
+		if anns.Canary.Enabled {
+			klog.V(2).Infof("Ingress %v is marked as Canary, ignoring", ingKey)
+			continue
+		}
+
 		for _, rule := range ing.Spec.Rules {
 			host := rule.Host
 			if host == "" {

internal/ingress/controller/controller_test.go

@@ -20,12 +20,24 @@ import (
 	"crypto/x509"
 	"crypto/x509/pkix"
 	"encoding/asn1"
+	"fmt"
+	"time"
 
+	"testing"
+
+	"github.com/eapache/channels"
+	"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"
-	"testing"
+	"k8s.io/ingress-nginx/internal/ingress/annotations"
+	"k8s.io/ingress-nginx/internal/ingress/annotations/canary"
+	ngx_config "k8s.io/ingress-nginx/internal/ingress/controller/config"
+	"k8s.io/ingress-nginx/internal/ingress/controller/store"
+	"k8s.io/ingress-nginx/internal/k8s"
 )
 
 func TestMergeAlternativeBackends(t *testing.T) {
@@ -586,6 +598,337 @@ func TestExtractTLSSecretName(t *testing.T) {
 	}
 }
 
+func TestGetBackendServers(t *testing.T) {
+	ctl := newNGINXController(t)
+
+	testCases := []struct {
+		Ingresses []*ingress.Ingress
+		Validate  func(servers []*ingress.Server)
+	}{
+		{
+			Ingresses: []*ingress.Ingress{
+				{
+					Ingress: extensions.Ingress{
+						ObjectMeta: metav1.ObjectMeta{
+							Namespace: "example",
+						},
+						Spec: extensions.IngressSpec{
+							Backend: &extensions.IngressBackend{
+								ServiceName: "http-svc-canary",
+								ServicePort: intstr.IntOrString{
+									IntVal: 80,
+								},
+							},
+						},
+					},
+					ParsedAnnotations: &annotations.Ingress{
+						Canary: canary.Config{
+							Enabled: true,
+						},
+					},
+				},
+			},
+			Validate: func(servers []*ingress.Server) {
+				if len(servers) != 1 {
+					t.Errorf("servers count should be 1, got %d", len(servers))
+					return
+				}
+
+				s := servers[0]
+				if s.Hostname != "_" {
+					t.Errorf("server hostname should be '_', got '%s'", s.Hostname)
+				}
+				if !s.Locations[0].IsDefBackend {
+					t.Errorf("server location 0 should be default backend")
+				}
+
+				if s.Locations[0].Backend != defUpstreamName {
+					t.Errorf("location backend should be '%s', got '%s'", defUpstreamName, s.Locations[0].Backend)
+				}
+			},
+		},
+		{
+			Ingresses: []*ingress.Ingress{
+				{
+					Ingress: extensions.Ingress{
+						ObjectMeta: metav1.ObjectMeta{
+							Namespace: "example",
+						},
+						Spec: extensions.IngressSpec{
+							Backend: &extensions.IngressBackend{
+								ServiceName: "http-svc-canary",
+								ServicePort: intstr.IntOrString{
+									IntVal: 80,
+								},
+							},
+						},
+					},
+					ParsedAnnotations: &annotations.Ingress{
+						Canary: canary.Config{
+							Enabled: true,
+						},
+					},
+				},
+				{
+					Ingress: extensions.Ingress{
+						ObjectMeta: metav1.ObjectMeta{
+							Namespace: "example",
+						},
+						Spec: extensions.IngressSpec{
+							Backend: &extensions.IngressBackend{
+								ServiceName: "http-svc",
+								ServicePort: intstr.IntOrString{
+									IntVal: 80,
+								},
+							},
+						},
+					},
+					ParsedAnnotations: &annotations.Ingress{
+						Canary: canary.Config{
+							Enabled: false,
+						},
+					},
+				},
+			},
+			Validate: func(servers []*ingress.Server) {
+				if len(servers) != 1 {
+					t.Errorf("servers count should be 1, got %d", len(servers))
+					return
+				}
+
+				s := servers[0]
+				if s.Hostname != "_" {
+					t.Errorf("server hostname should be '_', got '%s'", s.Hostname)
+				}
+				if s.Locations[0].IsDefBackend {
+					t.Errorf("server location 0 should not be default backend")
+				}
+
+				if s.Locations[0].Backend != "example-http-svc-80" {
+					t.Errorf("location backend should be 'example-http-svc-80', got '%s'", s.Locations[0].Backend)
+				}
+			},
+		},
+		{
+			Ingresses: []*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,
+														},
+													},
+												},
+											},
+										},
+									},
+								},
+							},
+						},
+					},
+					ParsedAnnotations: &annotations.Ingress{
+						Canary: canary.Config{
+							Enabled: true,
+						},
+					},
+				},
+			},
+			Validate: func(servers []*ingress.Server) {
+				if len(servers) != 1 {
+					t.Errorf("servers count should be 1, got %d", len(servers))
+					return
+				}
+
+				s := servers[0]
+				if s.Hostname != "_" {
+					t.Errorf("server hostname should be '_', got '%s'", s.Hostname)
+				}
+				if !s.Locations[0].IsDefBackend {
+					t.Errorf("server location 0 should be default backend")
+				}
+
+				if s.Locations[0].Backend != defUpstreamName {
+					t.Errorf("location backend should be '%s', got '%s'", defUpstreamName, s.Locations[0].Backend)
+				}
+			},
+		},
+		{
+			Ingresses: []*ingress.Ingress{
+				{
+					Ingress: extensions.Ingress{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "example",
+							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",
+														ServicePort: intstr.IntOrString{
+															Type:   intstr.Int,
+															IntVal: 80,
+														},
+													},
+												},
+											},
+										},
+									},
+								},
+							},
+						},
+					},
+					ParsedAnnotations: &annotations.Ingress{
+						Canary: canary.Config{
+							Enabled: false,
+						},
+					},
+				},
+				{
+					Ingress: extensions.Ingress{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "example-canary",
+							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,
+														},
+													},
+												},
+											},
+										},
+									},
+								},
+							},
+						},
+					},
+					ParsedAnnotations: &annotations.Ingress{
+						Canary: canary.Config{
+							Enabled: true,
+						},
+					},
+				},
+			},
+			Validate: func(servers []*ingress.Server) {
+				if len(servers) != 2 {
+					t.Errorf("servers count should be 2, got %d", len(servers))
+					return
+				}
+
+				s := servers[0]
+				if s.Hostname != "_" {
+					t.Errorf("server hostname should be '_', got '%s'", s.Hostname)
+				}
+				if !s.Locations[0].IsDefBackend {
+					t.Errorf("server location 0 should be default backend")
+				}
+
+				if s.Locations[0].Backend != defUpstreamName {
+					t.Errorf("location backend should be '%s', got '%s'", defUpstreamName, s.Locations[0].Backend)
+				}
+
+				s = servers[1]
+				if s.Hostname != "example.com" {
+					t.Errorf("server hostname should be 'example.com', got '%s'", s.Hostname)
+				}
+
+				if s.Locations[0].Backend != "example-http-svc-80" {
+					t.Errorf("location backend should be 'example-http-svc-80', got '%s'", s.Locations[0].Backend)
+				}
+			},
+		},
+	}
+
+	for _, testCase := range testCases {
+		_, servers := ctl.getBackendServers(testCase.Ingresses)
+		testCase.Validate(servers)
+	}
+}
+
+func newNGINXController(t *testing.T) *NGINXController {
+	ns := v1.NamespaceDefault
+	pod := &k8s.PodInfo{
+		Name:      "testpod",
+		Namespace: ns,
+		Labels: map[string]string{
+			"pod-template-hash": "1234",
+		},
+	}
+
+	clientSet := fake.NewSimpleClientset()
+	configMap := &v1.ConfigMap{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:     "config",
+			SelfLink: fmt.Sprintf("/api/v1/namespaces/%s/configmaps/config", ns),
+		},
+	}
+	_, err := clientSet.CoreV1().ConfigMaps(ns).Create(configMap)
+	if err != nil {
+		t.Fatalf("error creating the configuration map: %v", err)
+	}
+
+	fs, err := file.NewFakeFS()
+	if err != nil {
+		t.Fatalf("error: %v", err)
+	}
+
+	storer := store.New(true,
+		ns,
+		fmt.Sprintf("%v/config", ns),
+		fmt.Sprintf("%v/tcp", ns),
+		fmt.Sprintf("%v/udp", ns),
+		"",
+		10*time.Minute,
+		clientSet,
+		fs,
+		channels.NewRingChannel(10),
+		false,
+		pod)
+
+	config := &Configuration{
+		ListenPorts: &ngx_config.ListenPorts{
+			Default: 80,
+		},
+	}
+
+	return &NGINXController{
+		store: storer,
+		cfg:   config,
+	}
+}
+
 var oidExtensionSubjectAltName = asn1.ObjectIdentifier{2, 5, 29, 17}
 
 func fakeX509Cert(dnsNames []string) *x509.Certificate {

test/e2e/annotations/canary.go

@@ -192,4 +192,54 @@ var _ = framework.IngressNginxDescribe("Annotations - canary", func() {
 			Expect(body).ShouldNot(ContainSubstring("http-svc-canary"))
 		})
 	})
+
+	Context("Single canary Ingress", func() {
+		It("should not use canary as a catch-all server", func() {
+			host := "foo"
+			canaryIngName := fmt.Sprintf("%v-canary", host)
+			annotations := map[string]string{
+				"nginx.ingress.kubernetes.io/canary":           "true",
+				"nginx.ingress.kubernetes.io/canary-by-header": "CanaryByHeader",
+			}
+
+			ing := framework.NewSingleCatchAllIngress(canaryIngName, f.IngressController.Namespace, "http-svc-canary", 80, &annotations)
+			f.EnsureIngress(ing)
+
+			ing = framework.NewSingleCatchAllIngress(host, f.IngressController.Namespace, "http-svc", 80, nil)
+			f.EnsureIngress(ing)
+
+			f.WaitForNginxServer("_",
+				func(server string) bool {
+					upstreamName := fmt.Sprintf(`set $proxy_upstream_name "%s-%s-%s";`, f.IngressController.Namespace, "http-svc", "80")
+					canaryUpstreamName := fmt.Sprintf(`set $proxy_upstream_name "%s-%s-%s";`, f.IngressController.Namespace, "http-svc-canary", "80")
+					return Expect(server).Should(ContainSubstring(`set $ingress_name "`+host+`";`)) &&
+						Expect(server).ShouldNot(ContainSubstring(`set $proxy_upstream_name "upstream-default-backend";`)) &&
+						Expect(server).ShouldNot(ContainSubstring(canaryUpstreamName)) &&
+						Expect(server).Should(ContainSubstring(upstreamName))
+				})
+		})
+
+		It("should not use canary with domain as a server", func() {
+			host := "foo"
+			canaryIngName := fmt.Sprintf("%v-canary", host)
+			annotations := map[string]string{
+				"nginx.ingress.kubernetes.io/canary":           "true",
+				"nginx.ingress.kubernetes.io/canary-by-header": "CanaryByHeader",
+			}
+
+			ing := framework.NewSingleIngress(canaryIngName, "/", host, f.IngressController.Namespace, "http-svc-canary", 80, &annotations)
+			f.EnsureIngress(ing)
+
+			otherHost := "bar"
+			ing = framework.NewSingleIngress(otherHost, "/", otherHost, f.IngressController.Namespace, "http-svc", 80, nil)
+			f.EnsureIngress(ing)
+
+			time.Sleep(waitForLuaSync)
+
+			f.WaitForNginxConfiguration(func(cfg string) bool {
+				return Expect(cfg).Should(ContainSubstring("server_name "+otherHost)) &&
+					Expect(cfg).ShouldNot(ContainSubstring("server_name "+host))
+			})
+		})
+	})
 })

test/e2e/framework/framework.go

@@ -354,38 +354,28 @@ func UpdateDeployment(kubeClientSet kubernetes.Interface, namespace string, name
 
 // NewSingleIngressWithTLS creates a simple ingress rule with TLS spec included
 func NewSingleIngressWithTLS(name, path, host, ns, service string, port int, annotations *map[string]string) *extensions.Ingress {
-	return newSingleIngress(name, path, host, ns, service, port, annotations, true)
+	return newSingleIngressWithRules(name, path, host, ns, service, port, annotations, true)
 }
 
 // NewSingleIngress creates a simple ingress rule
 func NewSingleIngress(name, path, host, ns, service string, port int, annotations *map[string]string) *extensions.Ingress {
-	return newSingleIngress(name, path, host, ns, service, port, annotations, false)
+	return newSingleIngressWithRules(name, path, host, ns, service, port, annotations, false)
 }
 
-func newSingleIngress(name, path, host, ns, service string, port int, annotations *map[string]string, withTLS bool) *extensions.Ingress {
+func newSingleIngressWithRules(name, path, host, ns, service string, port int, annotations *map[string]string, withTLS bool) *extensions.Ingress {
-	if annotations == nil {
-		annotations = &map[string]string{}
-	}
 
-	ing := &extensions.Ingress{
+	spec := extensions.IngressSpec{
-		ObjectMeta: metav1.ObjectMeta{
+		Rules: []extensions.IngressRule{
-			Name:        name,
+			{
-			Namespace:   ns,
+				Host: host,
-			Annotations: *annotations,
+				IngressRuleValue: extensions.IngressRuleValue{
-		},
+					HTTP: &extensions.HTTPIngressRuleValue{
-		Spec: extensions.IngressSpec{
+						Paths: []extensions.HTTPIngressPath{
-			Rules: []extensions.IngressRule{
+							{
-				{
+								Path: path,
-					Host: host,
+								Backend: extensions.IngressBackend{
-					IngressRuleValue: extensions.IngressRuleValue{
+									ServiceName: service,
-						HTTP: &extensions.HTTPIngressRuleValue{
+									ServicePort: intstr.FromInt(port),
-							Paths: []extensions.HTTPIngressPath{
-								{
-									Path: path,
-									Backend: extensions.IngressBackend{
-										ServiceName: service,
-										ServicePort: intstr.FromInt(port),
-									},
 								},
 							},
 						},
@@ -396,7 +386,7 @@ func newSingleIngress(name, path, host, ns, service string, port int, annotation
 	}
 
 	if withTLS {
-		ing.Spec.TLS = []extensions.IngressTLS{
+		spec.TLS = []extensions.IngressTLS{
 			{
 				Hosts:      []string{host},
 				SecretName: host,
@@ -404,5 +394,33 @@ func newSingleIngress(name, path, host, ns, service string, port int, annotation
 		}
 	}
 
+	return newSingleIngress(name, ns, annotations, spec)
+}
+
+// NewSingleCatchAllIngress creates a simple ingress with a catch-all backend
+func NewSingleCatchAllIngress(name, ns, service string, port int, annotations *map[string]string) *extensions.Ingress {
+	spec := extensions.IngressSpec{
+		Backend: &extensions.IngressBackend{
+			ServiceName: service,
+			ServicePort: intstr.FromInt(port),
+		},
+	}
+	return newSingleIngress(name, ns, annotations, spec)
+}
+
+func newSingleIngress(name, ns string, annotations *map[string]string, spec extensions.IngressSpec) *extensions.Ingress {
+	if annotations == nil {
+		annotations = &map[string]string{}
+	}
+
+	ing := &extensions.Ingress{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:        name,
+			Namespace:   ns,
+			Annotations: *annotations,
+		},
+		Spec: spec,
+	}
+
 	return ing
 }