From 8a40e82ffb710469797f9effff2908b1a0b6fad5 Mon Sep 17 00:00:00 2001 From: Per Bernhardt Date: Mon, 4 Mar 2019 11:20:54 +0000 Subject: [PATCH 1/3] Fix panic on multiple non-matching canary --- internal/ingress/controller/controller.go | 7 +++++- test/e2e/annotations/canary.go | 24 +++++++++++++++++++++ test/e2e/framework/framework.go | 26 +++++++++++++++++++++++ 3 files changed, 56 insertions(+), 1 deletion(-) diff --git a/internal/ingress/controller/controller.go b/internal/ingress/controller/controller.go index 728e8b3a6e..d02bee72b1 100644 --- a/internal/ingress/controller/controller.go +++ b/internal/ingress/controller/controller.go @@ -1162,7 +1162,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 @@ -1224,6 +1224,11 @@ func mergeAlternativeBackends(ing *ingress.Ingress, upstreams map[string]*ingres altUps := upstreams[upsName] + if altUps == nil { + klog.Errorf("alternative backend %s has already be removed", upsName) + continue + } + merged := false server, ok := servers[rule.Host] diff --git a/test/e2e/annotations/canary.go b/test/e2e/annotations/canary.go index 1c9aa9bac9..bcab689be4 100644 --- a/test/e2e/annotations/canary.go +++ b/test/e2e/annotations/canary.go @@ -758,4 +758,28 @@ var _ = framework.IngressNginxDescribe("Annotations - canary", func() { }) }) }) + + Context("Single canary Ingress with multiple paths to same backend", func() { + It("should work", 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")) + }) + }) + }) }) diff --git a/test/e2e/framework/framework.go b/test/e2e/framework/framework.go index cbdb454efb..04fa62704b 100644 --- a/test/e2e/framework/framework.go +++ b/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{ From c995e13249801d52b8dd0bedfd1a4e52cb4a8992 Mon Sep 17 00:00:00 2001 From: Per Bernhardt Date: Tue, 12 Mar 2019 13:52:52 +0100 Subject: [PATCH 2/3] Improve text, error level, tests... --- internal/ingress/controller/controller.go | 29 +++++++++------- test/e2e/annotations/canary.go | 41 +++++++++++------------ 2 files changed, 36 insertions(+), 34 deletions(-) diff --git a/internal/ingress/controller/controller.go b/internal/ingress/controller/controller.go index d02bee72b1..6f5b46cf60 100644 --- a/internal/ingress/controller/controller.go +++ b/internal/ingress/controller/controller.go @@ -1199,22 +1199,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 { - priUps := upstreams[loc.Backend] + merged := false - if canMergeBackend(priUps, altUps) { - klog.V(2).Infof("matching backend %v found for alternative backend %v", - priUps.Name, altUps.Name) + for _, loc := range servers[defServerName].Locations { + priUps := upstreams[loc.Backend] - 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 { - klog.Warningf("unable to find real backend for alternative backend %v. Deleting.", altUps.Name) - delete(upstreams, altUps.Name) + if !merged { + klog.Warningf("unable to find real backend for alternative backend %v. Deleting.", altUps.Name) + delete(upstreams, altUps.Name) + } } } @@ -1225,7 +1230,7 @@ func mergeAlternativeBackends(ing *ingress.Ingress, upstreams map[string]*ingres altUps := upstreams[upsName] if altUps == nil { - klog.Errorf("alternative backend %s has already be removed", upsName) + klog.Warningf("alternative backend %s has already been removed", upsName) continue } diff --git a/test/e2e/annotations/canary.go b/test/e2e/annotations/canary.go index bcab689be4..7a9a237c94 100644 --- a/test/e2e/annotations/canary.go +++ b/test/e2e/annotations/canary.go @@ -759,27 +759,24 @@ var _ = framework.IngressNginxDescribe("Annotations - canary", func() { }) }) - Context("Single canary Ingress with multiple paths to same backend", func() { - It("should work", 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")) - }) - }) + 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")) + }) }) }) From ee13a653628c4f7d9ebdf8531263542c11d7342f Mon Sep 17 00:00:00 2001 From: Per Bernhardt Date: Tue, 12 Mar 2019 16:34:46 +0100 Subject: [PATCH 3/3] Force travis rebuild