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

Added client_max_body_size to authPath location #813

Merged

Conversation

olvesh
Copy link
Contributor

@olvesh olvesh commented Jun 2, 2017

Seems like nginx denies the request because it would be over the max body size,
even if proxy_pass_request_body is off.

This fixes #811

Not sure if it is straight forward to build tests for this. Hope this is ok as it is. For me this fixed my issues at least. Built a docker image and swapped the official one for one with this fix and it works.

Seems like nginx denies the request because it would be over the max body size,
event if `proxy_pass_request_body` is `off`.

This fixes 811
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 2, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 46.476% when pulling d4600a8 on olvesh:externalauth_client_max_body_size_811 into 4c868cf on kubernetes:master.

@aledbf
Copy link
Member

aledbf commented Jun 5, 2017

@olvesh I think we need to fix the real issue here, i.e. why the auth block is passing the body.

@aledbf
Copy link
Member

aledbf commented Jun 5, 2017

@olvesh can you test moving this line 383 to line 349 (before the auth section) and remove the line you added?

I think the issue here is that we need to accept the request first, that works because we are setting the max size before the proxy_pass directive but we need to move the configuration before the auth_request.

@olvesh
Copy link
Contributor Author

olvesh commented Jun 6, 2017

Tried your suggestion and it does not work. Still this error in log file:

2017/06/06 13:49:34 [error] 25#25: *29 client intended to send too large body: 8964178 bytes, client: 82.134.26.130, server: rundeck.example.com, request: "POST /project/CI-env/importArchive HTTP/2.0", subrequest: "/_external-auth-Lw", host: "rundeck.example.com", referrer: "https://rundeck.example.com/project/CI-env/configure"
2017/06/06 13:49:34 [error] 25#25: *29 auth request unexpected status: 413, client: 82.134.26.130, server: rundeck.example.com, request: "POST /project/CI-env/importArchive HTTP/2.0", host: "rundeck.example.com", referrer: "https://rundeck.example.com/project/CI-env/configure"

Relevant config result here: https://gist.github.com/olvesh/42bf8a6769e6fc41d07bfb9a6c710fc6

Nginx preemptively stops the request since it knows it will be to big given that it is a sub request. I think the logic here is that since it is an internal call Nginx can see that it will exceed the max body size, but it does not know that send_body is off thus denying the subrequest.

It could be put outside of the location element, in the server group instead, but then I think more refactoring is needed.

@aledbf
Copy link
Member

aledbf commented Jun 6, 2017

It could be put outside of the location element, in the server group instead, but then I think more refactoring is needed.

We cannot do that because it removes the feature of custom body size per path

@aledbf aledbf self-assigned this Jun 6, 2017
@aledbf
Copy link
Member

aledbf commented Jun 6, 2017

@olvesh thanks!

@aledbf
Copy link
Member

aledbf commented Jun 6, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 6, 2017
@aledbf aledbf merged commit dd354bf into kubernetes:master Jun 6, 2017
@aledbf
Copy link
Member

aledbf commented Jun 6, 2017

@olvesh I need to check if the auth_request is passing the body but I think this PR improves the current situation

@olvesh
Copy link
Contributor Author

olvesh commented Jun 6, 2017

@aledbf Thanks!
I tried to see if the size of the post on the internal sub-request appeared, but couldn't see that the request was logged, even the setting log_subrequest on; is present.

@olvesh olvesh deleted the externalauth_client_max_body_size_811 branch June 6, 2017 15:16
@olvesh
Copy link
Contributor Author

olvesh commented Jun 6, 2017

@aledbf btw, when will you (someone) cut a new beta of the ingress controller? Would like to drop the local snowflake we have now :-)

@aledbf
Copy link
Member

aledbf commented Jun 6, 2017

@olvesh after completing this list https://github.com/kubernetes/ingress/projects/3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

external auth - proxy_pass_request_body off + big bodies give 500/413
5 participants