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(proxy): fix request buffering for HTTP/2.0 #10595

Merged
merged 1 commit into from
Mar 31, 2023

Conversation

bungle
Copy link
Member

@bungle bungle commented Mar 31, 2023

Summary

Turning request/response buffering off for HTTP/2.0 is not possible. There is a check that causes buffering only to be controllable for HTTP/1.1.

This was probably done because of an issue in nginx, which was fixed in version 1.9.14 (http://nginx.org/en/docs/http/ngx_http_v2_module.html):

Before version 1.9.14, buffering of a client request body could not be disabled regardless of proxy_request_buffering, fastcgi_request_buffering, uwsgi_request_buffering, and scgi_request_buffering directive values.

Kong now has Nginx > 1.9.14, so the check is not needed any more.

The work was done by @PidgeyBE, thank you very much!

Issues Resolved

Fix #7418
Close #10204

@github-actions github-actions bot added chore Not part of the core functionality of kong, but still needed core/proxy labels Mar 31, 2023
@bungle bungle force-pushed the fix/request-buffering-http2-rebased branch 2 times, most recently from e3c7f52 to c92202c Compare March 31, 2023 08:03
@bungle bungle force-pushed the fix/request-buffering-http2-rebased branch 2 times, most recently from efbffec to 10c20a0 Compare March 31, 2023 08:19
Copy link
Contributor

@hanshuebner hanshuebner left a comment

Choose a reason for hiding this comment

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

Minor nits

kong/runloop/handler.lua Show resolved Hide resolved
spec/02-integration/05-proxy/27-unbuffered_spec.lua Outdated Show resolved Hide resolved
### Summary

Turning request/response buffering off for HTTP/2.0 is not possible.
There is a check that causes buffering only to be controllable for HTTP/1.1.

This was probably done because of an issue in nginx, which was fixed in
version 1.9.14 (http://nginx.org/en/docs/http/ngx_http_v2_module.html):

> Before version 1.9.14, buffering of a client request body could not be
> disabled regardless of [proxy_request_buffering](http://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_request_buffering),
> [fastcgi_request_buffering](http://nginx.org/en/docs/http/ngx_http_fastcgi_module.html#fastcgi_request_buffering),
> [uwsgi_request_buffering](http://nginx.org/en/docs/http/ngx_http_uwsgi_module.html#uwsgi_request_buffering), and
> [scgi_request_buffering](http://nginx.org/en/docs/http/ngx_http_scgi_module.html#scgi_request_buffering) directive values.

Kong now has Nginx > 1.9.14, so the check is not needed any more.

The work was done by @PidgeyBE, thank you very much!

### Issues Resolved

Fix #7418
Close #10204

Signed-off-by: Aapo Talvensaari <aapo.talvensaari@gmail.com>
@bungle bungle force-pushed the fix/request-buffering-http2-rebased branch from 10c20a0 to 9547c53 Compare March 31, 2023 10:59
@hanshuebner hanshuebner merged commit 0354778 into master Mar 31, 2023
@hanshuebner hanshuebner deleted the fix/request-buffering-http2-rebased branch March 31, 2023 11:22
bungle added a commit that referenced this pull request Mar 31, 2023
### Summary

Master is currently red because of #10595 (was green when merged),
this is an attempt to fix it.

Signed-off-by: Aapo Talvensaari <aapo.talvensaari@gmail.com>
hanshuebner pushed a commit that referenced this pull request Mar 31, 2023
### Summary

Master is currently red because of #10595 (was green when merged),
this is an attempt to fix it.

Signed-off-by: Aapo Talvensaari <aapo.talvensaari@gmail.com>
bungle added a commit that referenced this pull request Mar 31, 2023
### Summary

Master is currently red because of #10595 (was green when merged),
this is a second attempt to fix it.

Signed-off-by: Aapo Talvensaari <aapo.talvensaari@gmail.com>
bungle added a commit that referenced this pull request Mar 31, 2023
### Summary

Master is currently red because of #10595 (was green when merged),
this is a second attempt to fix it.

Signed-off-by: Aapo Talvensaari <aapo.talvensaari@gmail.com>
bungle added a commit that referenced this pull request Mar 31, 2023
### Summary

Master is currently red because of #10595 (was green when merged),
this is a final attempt to fix it, but disabling Lua cURL tests,
and removing the CI modifications because of it. We need to get
back on board to get it right.

Signed-off-by: Aapo Talvensaari <aapo.talvensaari@gmail.com>
bungle added a commit that referenced this pull request Mar 31, 2023
### Summary

Master is currently red because of #10595 (was green when merged),
this is a final attempt to fix it, but disabling Lua cURL tests,
and removing the CI modifications because of it. We need to get
back on board to get it right.

Signed-off-by: Aapo Talvensaari <aapo.talvensaari@gmail.com>
@kikito
Copy link
Member

kikito commented Apr 10, 2023

@bungle please cherry-pick into kong-ee

@kikito
Copy link
Member

kikito commented Apr 24, 2023

@bungle ping on this cherry-pick

@kikito
Copy link
Member

kikito commented May 2, 2023

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog chore Not part of the core functionality of kong, but still needed core/proxy size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

a client request body is buffered to a temporary file
4 participants