Skip to content

Commit

Permalink
Merge pull request #4506 from ProNic-QY/master
Browse files Browse the repository at this point in the history 
Fix panic on multiple ingress mess up upstream is primary or not
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot authored Sep 7, 2019
2 parents 6fceaf3 + 435377f commit 76e2a5d
Show file tree
Hide file tree
Showing 2 changed files with 286 additions and 9 deletions.
18 changes: 16 additions & 2 deletions internal/ingress/controller/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1230,9 +1230,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",
Expand All @@ -1242,7 +1249,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)
}
Expand All @@ -1261,6 +1268,7 @@ func mergeAlternativeBackends(ing *ingress.Ingress, upstreams map[string]*ingres
}

merged := false
altEqualsPri := false

server, ok := servers[rule.Host]
if !ok {
Expand All @@ -1274,6 +1282,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",
Expand All @@ -1283,7 +1297,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)
}
Expand Down
277 changes: 270 additions & 7 deletions internal/ingress/controller/controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
}
}

Expand Down

0 comments on commit 76e2a5d

Please sign in to comment.