Fix panic on multiple non-matching canary #3839
Conversation
|
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
k8s-ci-robot
added
cncf-cla: no
perprogramming
force-pushed
the
bugfix/panic-on-multiple-non-matching-canary
branch
from
ede8044 to
8a40e82
Compare
k8s-ci-robot
added
cncf-cla: yes
|
/approve |
k8s-ci-robot
added
the
approved
test/e2e/annotations/canary.go
Outdated
| @@ -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() { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I just tried to keep up with the structure of the rest of the file. I can remove the Context if preferred.
There was a problem hiding this comment.
I can remove the Context if preferred.
yes, please
| @@ -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) | |||
There was a problem hiding this comment.
Is this really an error? Maybe warn here?
There was a problem hiding this comment.
Just tell me. I can change it in a bit!
There was a problem hiding this comment.
let's make it a warning and fix the typo in the message:
alternative backend %s has already been removed
|
I guess the same thing can not happen with https://github.com/kubernetes/ingress-nginx/pull/3839/files#diff-86ac9ff9d75a0c5005c116e766a6127dR1200, right? |
|
I also added a warning for the catch-all block 👍 |
perprogramming
force-pushed
the
bugfix/panic-on-multiple-non-matching-canary
branch
from
f570fd2 to
c995e13
Compare
|
@perprogramming thanks for the patch! /lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aledbf, ElvinEfendi, perprogramming The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
This PR fixes a bug where ingress-controller fails with a panic when trying to remove a non-matching alternative backend for the second time.
Panic occures when you for example apply a canary-ingress with multiple rules/pathes that both select the same backend but can both not be matched to another real backend by host & path.
Which issue this PR fixes: fixes #3838
Special notes for your reviewer:
The PR comes with an e2e test already. I had to add a new e2e-framework factory method, feel free to refactor.