make canary ingresses independent of the order they were applied #3525
Conversation
k8s-ci-robot
added
cncf-cla: yes
| @@ -1115,8 +1123,17 @@ func (n *NGINXController) createServers(data []*ingress.Ingress, | |||
| return servers | |||
| } | |||
|
|
|||
| // OK to merge canary ingresses iff there exists one or more ingresses to potentially merge into | |||
| func safeToMergeCanaryIngresses(ingresses []*ingress.Ingress, canaryIngresses []*ingress.Ingress) bool { | |||
| return len(ingresses)-len(canaryIngresses) > 0 | |||
There was a problem hiding this comment.
IMO this is a bit confusing because main safety is being done in mergeAlternativeBackends function. I'd suggest to call this something like nonCanaryIngressExists
|
|
||
| if safeToMergeCanaryIngresses(ingresses, canaryIngresses) { | ||
| for _, canaryIng := range canaryIngresses { | ||
| mergeAlternativeBackends(canaryIng, upstreams, servers) |
There was a problem hiding this comment.
Since this is done before we set https://kubernetes.github.io/ingress-nginx/user-guide/nginx-configuration/annotations/#default-backend, canary feature won't work when the main backend has no endpoints and Nginx is proxying to custom default backend. IMO this is an expected behaviour. Not the concern of this PR but it would be nice to have an e2e test case where main ingress's backend has no endpoints (replicas: 0) and assert that all requests return 503 regardless of canary cookie/header being set to always.
There was a problem hiding this comment.
That's a good point. I'll add an e2e for this
clandry94
force-pushed
the
order_independence_canary_ann
branch
2 times, most recently
from
429bd99 to
a63d81f
Compare
|
/assign @ElvinEfendi |
clandry94
force-pushed
the
order_independence_canary_ann
branch
10 times, most recently
from
ebff4f4 to
66bf543
Compare
clandry94
force-pushed
the
order_independence_canary_ann
branch
from
66bf543 to
d5840a4
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: clandry94, ElvinEfendi 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 |
k8s-ci-robot
added
the
approved
What this PR does / why we need it:
This PR fixes a problem with the canary ingress feature where canary ingresses depend on the order of which they were applied. It also adds a check that a canary ingress cannot be merged into the default upstream when no other ingresses have been applied.
To make canary ingresses order independent, the
getBackendServersfunction first sets aside canary ingresses as it iterates across them. Then, after servers are created for every real ingress, the a check is done to make sure merging is safe and canary ingresses are iterated over and merged. The current condition for knowing if it is safe to merge canary ingresses is whether thelength of ingresses - length of canary ingresses > 0If true, we know that a real ingress must exist and there is a potential match which can be merged into. This is currently more of a sanity check than a condition for "safeness", but this function can be used to add more conditions for safeness if necessary.Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged): fixes #Special notes for your reviewer: