nginx/proxy: allow specifying next upstream behaviour #907
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
This change is |
glerchundi
changed the title
|
Lots of work to be done but just to put it on the table and get feedback from you guys. |
|
@glerchundi you need to change the template (no reference int the PR).
No. We need to keep the current defaults and allow the RetryNonIdempotent customization to the user |
|
Also please add the new flag in the section https://github.com/kubernetes/ingress/blob/master/controllers/nginx/configuration.md#allowed-parameters-in-configuration-configmap |
|
@aledbf in case I don't know if the extra logic to remove non_idempotent is worth it or not. Will do tomorrow in case it is. Thanks for being that quick! |
glerchundi
changed the title
|
@aledbf, in the case of
I would choose the second one, WDYT? |
|
@aledbf any chance to review and make it inside the next release? ;)
…On Tue, 27 Jun 2017 at 07:58, Coveralls ***@***.***> wrote:
[image: Coverage Status] <https://:/builds/12145625>
Coverage increased (+0.03%) to 44.271% when pulling *1b3adb5
<1b3adb5>
on glerchundi:master* into *1468fcb
<1468fcb>
on kubernetes:master*.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#907 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACIPlr_sfML1yNvky_UX3rJTprNHzSdhks5sIJnogaJpZM4OF0r8>
.
|
| @@ -439,6 +436,9 @@ http { | |||
| proxy_cookie_domain {{ $location.Proxy.CookieDomain }}; | |||
| proxy_cookie_path {{ $location.Proxy.CookiePath }}; | |||
|
|
|||
| # In case of errors try the next upstream server before returning an error | |||
| proxy_next_upstream {{ $location.Proxy.NextUpstream }}{{ if $cfg.RetryNonIdempotent }} non_idempotent{{ end }}; | |||
There was a problem hiding this comment.
Please create a function in the file template.go in order to avoid non_idempotent duplication (if the user added that option to the default)
Please refactor the template with the function and I will test this asap |
|
@aledbf addressed and squashed. Note: already tested and works as expected. |
| func buildNextUpstream(input interface{}) string { | ||
| nextUpstream, ok := input.(string) | ||
| if !ok { | ||
| glog.Errorf("expected an string type but %T was returned", input) |
There was a problem hiding this comment.
return the default values here (do not continue)
Edit: forget this commenr
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
@glerchundi thanks! |
|
Thanks and keep up the good work!
…On Wed, 28 Jun 2017 at 01:24, Manuel Alejandro de Brito Fontes < ***@***.***> wrote:
Merged #907 <#907>.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#907 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ACIPluTGDxHq6j173zKWEVL94Wccqobtks5sIY8igaJpZM4OF0r8>
.
|
The motivation is that in our use case 504 http response is a valid user facing status code and nginx is retrying to all upstreams because it's inside the list of retryable errors (
proxy_next_upstream):https://github.com/kubernetes/ingress/blob/master/controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl#L220
I didn't tested it yet because docker is retrying some image layers all the time but in case we can accelerate the acceptance-denying/review process here it is.
Should replace
RetryNonIdempotentconfiguration.WDYT?