Merge pull request #3839 from perprogramming/bugfix/panic-on-multiple-non-matching-canary

Fix panic on multiple non-matching canary

internal/ingress/controller/controller.go

@@ -1114,7 +1114,7 @@ func nonCanaryIngressExists(ingresses []*ingress.Ingress, canaryIngresses []*ing
 // 2) primary name is not the default upstream
 // 3) the primary has a server
 func canMergeBackend(primary *ingress.Backend, alternative *ingress.Backend) bool {
-	return primary.Name != alternative.Name && primary.Name != defUpstreamName && !primary.NoServer
+	return alternative != nil && primary.Name != alternative.Name && primary.Name != defUpstreamName && !primary.NoServer
 }
 
 // Performs the merge action and checks to ensure that one two alternative backends do not merge into each other
@@ -1151,22 +1151,27 @@ func mergeAlternativeBackends(ing *ingress.Ingress, upstreams map[string]*ingres
 
 		altUps := upstreams[upsName]
 
-		merged := false
+		if altUps == nil {
+			klog.Warningf("alternative backend %s has already been removed", upsName)
+		} else {
 
-		for _, loc := range servers[defServerName].Locations {
+			merged := false
-			priUps := upstreams[loc.Backend]
 
-			if canMergeBackend(priUps, altUps) {
+			for _, loc := range servers[defServerName].Locations {
-				klog.V(2).Infof("matching backend %v found for alternative backend %v",
+				priUps := upstreams[loc.Backend]
-					priUps.Name, altUps.Name)
 
-				merged = mergeAlternativeBackend(priUps, altUps)
+				if canMergeBackend(priUps, altUps) {
+					klog.V(2).Infof("matching backend %v found for alternative backend %v",
+						priUps.Name, altUps.Name)
+
+					merged = mergeAlternativeBackend(priUps, altUps)
+				}
 			}
-		}
 
-		if !merged {
+			if !merged {
-			klog.Warningf("unable to find real backend for alternative backend %v. Deleting.", altUps.Name)
+				klog.Warningf("unable to find real backend for alternative backend %v. Deleting.", altUps.Name)
-			delete(upstreams, altUps.Name)
+				delete(upstreams, altUps.Name)
+			}
 		}
 	}
 
@@ -1176,6 +1181,11 @@ func mergeAlternativeBackends(ing *ingress.Ingress, upstreams map[string]*ingres
 
 			altUps := upstreams[upsName]
 
+			if altUps == nil {
+				klog.Warningf("alternative backend %s has already been removed", upsName)
+				continue
+			}
+
 			merged := false
 
 			server, ok := servers[rule.Host]

test/e2e/annotations/canary.go

@@ -758,4 +758,25 @@ var _ = framework.IngressNginxDescribe("Annotations - canary", func() {
 			})
 		})
 	})
+
+	It("does not crash when canary ingress has multiple paths to the same non-matching backend", 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",
+		}
+
+		paths := []string{"/foo", "/bar"}
+		ing := framework.NewSingleIngressWithMultiplePaths(canaryIngName, paths, host, f.Namespace, "httpy-svc-canary", 80, &annotations)
+		f.EnsureIngress(ing)
+
+		ing = framework.NewSingleIngress(host, "/", host, f.Namespace, "http-svc", 80, nil)
+		f.EnsureIngress(ing)
+
+		f.WaitForNginxServer(host,
+			func(server string) bool {
+				return Expect(server).Should(ContainSubstring("server_name foo"))
+			})
+	})
 })

test/e2e/framework/framework.go

@@ -367,6 +367,32 @@ func NewSingleIngress(name, path, host, ns, service string, port int, annotation
 	return newSingleIngressWithRules(name, path, host, ns, service, port, annotations, nil)
 }
 
+// NewSingleIngressWithMultiplePaths creates a simple ingress rule with multiple paths
+func NewSingleIngressWithMultiplePaths(name string, paths []string, host, ns, service string, port int, annotations *map[string]string) *extensions.Ingress {
+	spec := extensions.IngressSpec{
+		Rules: []extensions.IngressRule{
+			{
+				Host: host,
+				IngressRuleValue: extensions.IngressRuleValue{
+					HTTP: &extensions.HTTPIngressRuleValue{},
+				},
+			},
+		},
+	}
+
+	for _, path := range paths {
+		spec.Rules[0].IngressRuleValue.HTTP.Paths = append(spec.Rules[0].IngressRuleValue.HTTP.Paths, extensions.HTTPIngressPath{
+			Path: path,
+			Backend: extensions.IngressBackend{
+				ServiceName: service,
+				ServicePort: intstr.FromInt(port),
+			},
+		})
+	}
+
+	return newSingleIngress(name, ns, annotations, spec)
+}
+
 func newSingleIngressWithRules(name, path, host, ns, service string, port int, annotations *map[string]string, tlsHosts []string) *extensions.Ingress {
 
 	spec := extensions.IngressSpec{