From 435377f47fde8832f9c8eeb79674efff9470302c Mon Sep 17 00:00:00 2001 From: qianyong Date: Thu, 29 Aug 2019 14:32:43 +0800 Subject: [PATCH] Fix panic on multiple ingress mess up upstream is primary or not --- internal/ingress/controller/controller.go | 18 +- .../ingress/controller/controller_test.go | 277 +++++++++++++++++- 2 files changed, 286 insertions(+), 9 deletions(-) diff --git a/internal/ingress/controller/controller.go b/internal/ingress/controller/controller.go index b33bf15944..5383752b6e 100644 --- a/internal/ingress/controller/controller.go +++ b/internal/ingress/controller/controller.go @@ -1227,9 +1227,16 @@ func mergeAlternativeBackends(ing *ingress.Ingress, upstreams map[string]*ingres } else { merged := false + altEqualsPri := false for _, loc := range servers[defServerName].Locations { priUps := upstreams[loc.Backend] + altEqualsPri = altUps.Name == priUps.Name + if altEqualsPri { + klog.Warningf("alternative upstream %s in Ingress %s/%s is primary upstream in Other Ingress for location %s%s!", + altUps.Name, ing.Namespace, ing.Name, servers[defServerName].Hostname, loc.Path) + break + } if canMergeBackend(priUps, altUps) { klog.V(2).Infof("matching backend %v found for alternative backend %v", @@ -1239,7 +1246,7 @@ func mergeAlternativeBackends(ing *ingress.Ingress, upstreams map[string]*ingres } } - if !merged { + if !altEqualsPri && !merged { klog.Warningf("unable to find real backend for alternative backend %v. Deleting.", altUps.Name) delete(upstreams, altUps.Name) } @@ -1258,6 +1265,7 @@ func mergeAlternativeBackends(ing *ingress.Ingress, upstreams map[string]*ingres } merged := false + altEqualsPri := false server, ok := servers[rule.Host] if !ok { @@ -1271,6 +1279,12 @@ func mergeAlternativeBackends(ing *ingress.Ingress, upstreams map[string]*ingres // find matching paths for _, loc := range server.Locations { priUps := upstreams[loc.Backend] + altEqualsPri = altUps.Name == priUps.Name + if altEqualsPri { + klog.Warningf("alternative upstream %s in Ingress %s/%s is primary upstream in Other Ingress for location %s%s!", + altUps.Name, ing.Namespace, ing.Name, server.Hostname, loc.Path) + break + } if canMergeBackend(priUps, altUps) && loc.Path == path.Path { klog.V(2).Infof("matching backend %v found for alternative backend %v", @@ -1280,7 +1294,7 @@ func mergeAlternativeBackends(ing *ingress.Ingress, upstreams map[string]*ingres } } - if !merged { + if !altEqualsPri && !merged { klog.Warningf("unable to find real backend for alternative backend %v. Deleting.", altUps.Name) delete(upstreams, altUps.Name) } diff --git a/internal/ingress/controller/controller_test.go b/internal/ingress/controller/controller_test.go index 60d9f3b2b0..d984d4b029 100644 --- a/internal/ingress/controller/controller_test.go +++ b/internal/ingress/controller/controller_test.go @@ -818,7 +818,7 @@ func TestGetBackendServers(t *testing.T) { testCases := []struct { Ingresses []*ingress.Ingress - Validate func(servers []*ingress.Server) + Validate func(upstreams []*ingress.Backend, servers []*ingress.Server) }{ { Ingresses: []*ingress.Ingress{ @@ -843,7 +843,7 @@ func TestGetBackendServers(t *testing.T) { }, }, }, - Validate: func(servers []*ingress.Server) { + Validate: func(upstreams []*ingress.Backend, servers []*ingress.Server) { if len(servers) != 1 { t.Errorf("servers count should be 1, got %d", len(servers)) return @@ -905,7 +905,7 @@ func TestGetBackendServers(t *testing.T) { }, }, }, - Validate: func(servers []*ingress.Server) { + Validate: func(upstreams []*ingress.Backend, servers []*ingress.Server) { if len(servers) != 1 { t.Errorf("servers count should be 1, got %d", len(servers)) return @@ -962,7 +962,7 @@ func TestGetBackendServers(t *testing.T) { }, }, }, - Validate: func(servers []*ingress.Server) { + Validate: func(upstreams []*ingress.Backend, servers []*ingress.Server) { if len(servers) != 1 { t.Errorf("servers count should be 1, got %d", len(servers)) return @@ -1056,7 +1056,7 @@ func TestGetBackendServers(t *testing.T) { }, }, }, - Validate: func(servers []*ingress.Server) { + Validate: func(upstreams []*ingress.Backend, servers []*ingress.Server) { if len(servers) != 2 { t.Errorf("servers count should be 2, got %d", len(servers)) return @@ -1084,11 +1084,274 @@ func TestGetBackendServers(t *testing.T) { } }, }, + { + Ingresses: []*ingress.Ingress{ + { + Ingress: networking.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Name: "example-a", + Namespace: "example", + }, + Spec: networking.IngressSpec{ + Rules: []networking.IngressRule{ + { + Host: "example.com", + IngressRuleValue: networking.IngressRuleValue{ + HTTP: &networking.HTTPIngressRuleValue{ + Paths: []networking.HTTPIngressPath{ + { + Path: "/a", + Backend: networking.IngressBackend{ + ServiceName: "http-svc-1", + ServicePort: intstr.IntOrString{ + Type: intstr.Int, + IntVal: 80, + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + ParsedAnnotations: &annotations.Ingress{ + Canary: canary.Config{ + Enabled: false, + }, + }, + }, + { + Ingress: networking.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Name: "example-a-canary", + Namespace: "example", + }, + Spec: networking.IngressSpec{ + Rules: []networking.IngressRule{ + { + Host: "example.com", + IngressRuleValue: networking.IngressRuleValue{ + HTTP: &networking.HTTPIngressRuleValue{ + Paths: []networking.HTTPIngressPath{ + { + Path: "/a", + Backend: networking.IngressBackend{ + ServiceName: "http-svc-2", + ServicePort: intstr.IntOrString{ + Type: intstr.Int, + IntVal: 80, + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + ParsedAnnotations: &annotations.Ingress{ + Canary: canary.Config{ + Enabled: true, + }, + }, + }, + { + Ingress: networking.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Name: "example-b", + Namespace: "example", + }, + Spec: networking.IngressSpec{ + Rules: []networking.IngressRule{ + { + Host: "example.com", + IngressRuleValue: networking.IngressRuleValue{ + HTTP: &networking.HTTPIngressRuleValue{ + Paths: []networking.HTTPIngressPath{ + { + Path: "/b", + Backend: networking.IngressBackend{ + ServiceName: "http-svc-2", + ServicePort: intstr.IntOrString{ + Type: intstr.Int, + IntVal: 80, + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + ParsedAnnotations: &annotations.Ingress{ + Canary: canary.Config{ + Enabled: false, + }, + }, + }, + { + Ingress: networking.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Name: "example-b-canary", + Namespace: "example", + }, + Spec: networking.IngressSpec{ + Rules: []networking.IngressRule{ + { + Host: "example.com", + IngressRuleValue: networking.IngressRuleValue{ + HTTP: &networking.HTTPIngressRuleValue{ + Paths: []networking.HTTPIngressPath{ + { + Path: "/b", + Backend: networking.IngressBackend{ + ServiceName: "http-svc-1", + ServicePort: intstr.IntOrString{ + Type: intstr.Int, + IntVal: 80, + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + ParsedAnnotations: &annotations.Ingress{ + Canary: canary.Config{ + Enabled: true, + }, + }, + }, + { + Ingress: networking.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Name: "example-c", + Namespace: "example", + }, + Spec: networking.IngressSpec{ + Rules: []networking.IngressRule{ + { + Host: "example.com", + IngressRuleValue: networking.IngressRuleValue{ + HTTP: &networking.HTTPIngressRuleValue{ + Paths: []networking.HTTPIngressPath{ + { + Path: "/c", + Backend: networking.IngressBackend{ + ServiceName: "http-svc-1", + ServicePort: intstr.IntOrString{ + Type: intstr.Int, + IntVal: 80, + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + ParsedAnnotations: &annotations.Ingress{ + Canary: canary.Config{ + Enabled: false, + }, + }, + }, + { + Ingress: networking.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Name: "example-c-canary", + Namespace: "example", + }, + Spec: networking.IngressSpec{ + Rules: []networking.IngressRule{ + { + Host: "example.com", + IngressRuleValue: networking.IngressRuleValue{ + HTTP: &networking.HTTPIngressRuleValue{ + Paths: []networking.HTTPIngressPath{ + { + Path: "/c", + Backend: networking.IngressBackend{ + ServiceName: "http-svc-2", + ServicePort: intstr.IntOrString{ + Type: intstr.Int, + IntVal: 80, + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + ParsedAnnotations: &annotations.Ingress{ + Canary: canary.Config{ + Enabled: true, + }, + }, + }, + }, + Validate: func(upstreams []*ingress.Backend, 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-1-80" || s.Locations[1].Backend != "example-http-svc-1-80" || s.Locations[2].Backend != "example-http-svc-1-80" { + t.Errorf("all location backend should be 'example-http-svc-1-80'") + } + + if len(upstreams) != 3 { + t.Errorf("upstreams count should be 3, got %d", len(upstreams)) + return + } + + if upstreams[0].Name != "example-http-svc-1-80" { + t.Errorf("example-http-svc-1-80 should be frist upstream, got %s", upstreams[0].Name) + return + } + if upstreams[0].NoServer { + t.Errorf("'example-http-svc-1-80' should be primary upstream, got as alternative upstream") + } + if len(upstreams[0].AlternativeBackends) != 1 || upstreams[0].AlternativeBackends[0] != "example-http-svc-2-80" { + t.Errorf("example-http-svc-2-80 should be alternative upstream for 'example-http-svc-1-80'") + } + }, + }, } for _, testCase := range testCases { - _, servers := ctl.getBackendServers(testCase.Ingresses) - testCase.Validate(servers) + upstreams, servers := ctl.getBackendServers(testCase.Ingresses) + testCase.Validate(upstreams, servers) } }