Merge pull request #3446 from Shopify/add_tests_merge_canary

add more testing for mergeAlternativeBackends

internal/ingress/controller/controller.go

@@ -1115,6 +1115,24 @@ func (n *NGINXController) createServers(data []*ingress.Ingress,
 	return servers
 }
 
+func canMergeBackend(primary *ingress.Backend, alternative *ingress.Backend) bool {
+	return primary.Name != alternative.Name && !primary.NoServer
+}
+
+// Performs the merge action and checks to ensure that one two alternative backends do not merge into each other
+func mergeAlternativeBackend(priUps *ingress.Backend, altUps *ingress.Backend) bool {
+	if priUps.NoServer {
+		glog.Warningf("unable to merge alternative backend %v into primary backend %v because %v is a primary backend",
+			altUps.Name, priUps.Name, priUps.Name)
+		return false
+	}
+
+	priUps.AlternativeBackends =
+		append(priUps.AlternativeBackends, altUps.Name)
+
+	return true
+}
+
 // Compares an Ingress of a potential alternative backend's rules with each existing server and finds matching host + path pairs.
 // 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.
@@ -1126,49 +1144,59 @@ func mergeAlternativeBackends(ing *ingress.Ingress, upstreams map[string]*ingres
 	if ing.Spec.Backend != nil {
 		upsName := upstreamName(ing.Namespace, ing.Spec.Backend.ServiceName, ing.Spec.Backend.ServicePort)
 
-		ups := upstreams[upsName]
+		altUps := upstreams[upsName]
 
-		for _, defLoc := range servers[defServerName].Locations {
+		merged := false
-			if !upstreams[defLoc.Backend].NoServer {
+
+		for _, loc := range servers[defServerName].Locations {
+			priUps := upstreams[loc.Backend]
+
+			if canMergeBackend(priUps, altUps) {
 				glog.Infof("matching backend %v found for alternative backend %v",
-					upstreams[defLoc.Backend].Name, ups.Name)
+					priUps.Name, altUps.Name)
 
-				upstreams[defLoc.Backend].AlternativeBackends =
+				merged = mergeAlternativeBackend(priUps, altUps)
-					append(upstreams[defLoc.Backend].AlternativeBackends, ups.Name)
 			}
 		}
+
+		if !merged {
+			glog.Warningf("unable to find real backend for alternative backend %v. Deleting.", altUps.Name)
+			delete(upstreams, altUps.Name)
+		}
 	}
 
 	for _, rule := range ing.Spec.Rules {
 		for _, path := range rule.HTTP.Paths {
 			upsName := upstreamName(ing.Namespace, path.Backend.ServiceName, path.Backend.ServicePort)
 
-			ups := upstreams[upsName]
+			altUps := upstreams[upsName]
 
 			merged := false
 
-			server := servers[rule.Host]
+			server, ok := servers[rule.Host]
+			if !ok {
+				glog.Errorf("cannot merge alternative backend %s into hostname %s that does not exist",
+					altUps.Name,
+					rule.Host)
 
-			// find matching paths
-			for _, location := range server.Locations {
-				if location.Backend == defUpstreamName {
 				continue
 			}
 
-				if location.Path == path.Path && !upstreams[location.Backend].NoServer {
+			// find matching paths
+			for _, loc := range server.Locations {
+				priUps := upstreams[loc.Backend]
+
+				if canMergeBackend(priUps, altUps) && loc.Path == path.Path {
 					glog.Infof("matching backend %v found for alternative backend %v",
-						upstreams[location.Backend].Name, ups.Name)
+						priUps.Name, altUps.Name)
 
-					upstreams[location.Backend].AlternativeBackends =
+					merged = mergeAlternativeBackend(priUps, altUps)
-						append(upstreams[location.Backend].AlternativeBackends, ups.Name)
-
-					merged = true
 				}
 			}
 
 			if !merged {
-				glog.Warningf("unable to find real backend for alternative backend %v. Deleting.", ups.Name)
+				glog.Warningf("unable to find real backend for alternative backend %v. Deleting.", altUps.Name)
-				delete(upstreams, ups.Name)
+				delete(upstreams, altUps.Name)
 			}
 		}
 	}

internal/ingress/controller/controller_test.go

@@ -20,13 +20,12 @@ import (
 	"crypto/x509"
 	"crypto/x509/pkix"
 	"encoding/asn1"
-	"testing"
-
-	"k8s.io/apimachinery/pkg/util/intstr"
 
 	extensions "k8s.io/api/extensions/v1beta1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/ingress-nginx/internal/ingress"
+	"testing"
 )
 
 func TestMergeAlternativeBackends(t *testing.T) {
@@ -34,8 +33,8 @@ func TestMergeAlternativeBackends(t *testing.T) {
 		ingress      *ingress.Ingress
 		upstreams    map[string]*ingress.Backend
 		servers      map[string]*ingress.Server
-		expNumAlternativeBackends int
+		expUpstreams map[string]*ingress.Backend
-		expNumLocations           int
+		expServers   map[string]*ingress.Server
 	}{
 		"alternative backend has no server and embeds into matching real backend": {
 			&ingress.Ingress{
@@ -92,10 +91,33 @@ func TestMergeAlternativeBackends(t *testing.T) {
 					},
 				},
 			},
-			1,
+			map[string]*ingress.Backend{
-			1,
+				"example-http-svc-80": {
+					Name:                "example-http-svc-80",
+					NoServer:            false,
+					AlternativeBackends: []string{"example-http-svc-canary-80"},
 				},
-		"merging a alternative backend matches with the correct host": {
+				"example-http-svc-canary-80": {
+					Name:     "example-http-svc-canary-80",
+					NoServer: true,
+					TrafficShapingPolicy: ingress.TrafficShapingPolicy{
+						Weight: 20,
+					},
+				},
+			},
+			map[string]*ingress.Server{
+				"example.com": {
+					Hostname: "example.com",
+					Locations: []*ingress.Location{
+						{
+							Path:    "/",
+							Backend: "example-http-svc-80",
+						},
+					},
+				},
+			},
+		},
+		"alternative backend merges with the correct real backend when multiple are present": {
 			&ingress.Ingress{
 				Ingress: extensions.Ingress{
 					ObjectMeta: metav1.ObjectMeta{
@@ -160,6 +182,7 @@ func TestMergeAlternativeBackends(t *testing.T) {
 				"example-http-svc-80": {
 					Name:                "example-http-svc-80",
 					NoServer:            false,
+					AlternativeBackends: []string{"example-http-svc-canary-80"},
 				},
 				"example-http-svc-canary-80": {
 					Name:     "example-http-svc-canary-80",
@@ -189,8 +212,206 @@ func TestMergeAlternativeBackends(t *testing.T) {
 					},
 				},
 			},
-			1,
+			map[string]*ingress.Backend{
-			1,
+				"example-foo-http-svc-80": {
+					Name:                "example-foo-http-svc-80",
+					NoServer:            false,
+					AlternativeBackends: []string{"example-foo-http-svc-canary-80"},
+				},
+				"example-foo-http-svc-canary-80": {
+					Name:     "example-foo-http-svc-canary-80",
+					NoServer: true,
+					TrafficShapingPolicy: ingress.TrafficShapingPolicy{
+						Weight: 20,
+					},
+				},
+				"example-http-svc-80": {
+					Name:                "example-http-svc-80",
+					NoServer:            false,
+					AlternativeBackends: []string{"example-http-svc-canary-80"},
+				},
+				"example-http-svc-canary-80": {
+					Name:     "example-http-svc-canary-80",
+					NoServer: true,
+					TrafficShapingPolicy: ingress.TrafficShapingPolicy{
+						Weight: 20,
+					},
+				},
+			},
+			map[string]*ingress.Server{
+				"example.com": {
+					Hostname: "example.com",
+					Locations: []*ingress.Location{
+						{
+							Path:    "/",
+							Backend: "example-http-svc-80",
+						},
+					},
+				},
+			},
+		},
+		"alternative backend does not merge into itself": {
+			&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,
+													},
+												},
+											},
+										},
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+			map[string]*ingress.Backend{
+				"example-http-svc-canary-80": {
+					Name:     "example-http-svc-canary-80",
+					NoServer: true,
+					TrafficShapingPolicy: ingress.TrafficShapingPolicy{
+						Weight: 20,
+					},
+				},
+			},
+			map[string]*ingress.Server{},
+			map[string]*ingress.Backend{},
+			map[string]*ingress.Server{},
+		},
+		"catch-all alternative backend has no server and embeds into matching real backend": {
+			&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,
+							},
+						},
+					},
+				},
+			},
+			map[string]*ingress.Backend{
+				"example-http-svc-80": {
+					Name:     "example-http-svc-80",
+					NoServer: false,
+				},
+				"example-http-svc-canary-80": {
+					Name:     "example-http-svc-canary-80",
+					NoServer: true,
+					TrafficShapingPolicy: ingress.TrafficShapingPolicy{
+						Weight: 20,
+					},
+				},
+			},
+			map[string]*ingress.Server{
+				"_": {
+					Hostname: "_",
+					Locations: []*ingress.Location{
+						{
+							Path:    "/",
+							Backend: "example-http-svc-80",
+						},
+					},
+				},
+			},
+			map[string]*ingress.Backend{
+				"example-http-svc-80": {
+					Name:                "example-http-svc-80",
+					NoServer:            false,
+					AlternativeBackends: []string{"example-http-svc-canary-80"},
+				},
+				"example-http-svc-canary-80": {
+					Name:     "example-http-svc-canary-80",
+					NoServer: true,
+					TrafficShapingPolicy: ingress.TrafficShapingPolicy{
+						Weight: 20,
+					},
+				},
+			},
+			map[string]*ingress.Server{
+				"_": {
+					Hostname: "_",
+					Locations: []*ingress.Location{
+						{
+							Path:    "/",
+							Backend: "example-http-svc-80",
+						},
+					},
+				},
+			},
+		},
+		"catch-all alternative backend does not merge into itself": {
+			&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,
+							},
+						},
+					},
+				},
+			},
+			map[string]*ingress.Backend{
+				"upstream-default-backend": {
+					Name:     "upstream-default-backend",
+					NoServer: false,
+				},
+				"example-http-svc-canary-80": {
+					Name:     "example-http-svc-canary-80",
+					NoServer: true,
+					TrafficShapingPolicy: ingress.TrafficShapingPolicy{
+						Weight: 20,
+					},
+				},
+			},
+			map[string]*ingress.Server{
+				"_": {
+					Hostname: "_",
+					Locations: []*ingress.Location{
+						{
+							Path:    "/",
+							Backend: "upstream-default-backend",
+						},
+					},
+				},
+			},
+			map[string]*ingress.Backend{},
+			map[string]*ingress.Server{
+				"_": {
+					Hostname: "_",
+					Locations: []*ingress.Location{
+						{
+							Path:    "/",
+							Backend: "upstream-default-backend",
+						},
+					},
+				},
+			},
 		},
 	}
 
@@ -198,14 +419,28 @@ func TestMergeAlternativeBackends(t *testing.T) {
 		t.Run(title, func(t *testing.T) {
 			mergeAlternativeBackends(tc.ingress, tc.upstreams, tc.servers)
 
-			numAlternativeBackends := len(tc.upstreams["example-http-svc-80"].AlternativeBackends)
+			for upsName, expUpstream := range tc.expUpstreams {
-			if numAlternativeBackends != tc.expNumAlternativeBackends {
+				actualUpstream, ok := tc.upstreams[upsName]
-				t.Errorf("expected %d alternative backends (got %d)", tc.expNumAlternativeBackends, numAlternativeBackends)
+				if !ok {
+					t.Errorf("expected upstream %s to exist but it did not", upsName)
 				}
 
-			numLocations := len(tc.servers["example.com"].Locations)
+				if !actualUpstream.Equal(expUpstream) {
-			if numLocations != tc.expNumLocations {
+					t.Logf("actual upstream %s alternative backends: %s", actualUpstream.Name, actualUpstream.AlternativeBackends)
-				t.Errorf("expected %d locations (got %d)", tc.expNumLocations, numLocations)
+					t.Logf("expected upstream %s alternative backends: %s", expUpstream.Name, expUpstream.AlternativeBackends)
+					t.Errorf("upstream %s was not equal to what was expected: ", upsName)
+				}
+			}
+
+			for serverName, expServer := range tc.expServers {
+				actualServer, ok := tc.servers[serverName]
+				if !ok {
+					t.Errorf("expected server %s to exist but it did not", serverName)
+				}
+
+				if !actualServer.Equal(expServer) {
+					t.Errorf("server %s was not equal to what was expected", serverName)
+				}
 			}
 		})
 	}