Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix panic on multiple non-matching canary #3839

Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion internal/ingress/controller/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really an error? Maybe warn here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just tell me. I can change it in a bit!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's make it a warning and fix the typo in the message:

alternative backend %s has already been removed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

continue
}

merged := false

server, ok := servers[rule.Host]
Expand Down
24 changes: 24 additions & 0 deletions test/e2e/annotations/canary.go
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Copy link
Member

@ElvinEfendi ElvinEfendi Mar 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about

It("does not crash when canary ingress has multiple paths to the same non-matching backend", ...)

without Context

I'd implement this as a unit test, but not feeling strongly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tried to keep up with the structure of the rest of the file. I can remove the Context if preferred.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can remove the Context if preferred.

yes, please

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

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"))
})
})
})
})
26 changes: 26 additions & 0 deletions test/e2e/framework/framework.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down