Fix request buffering for HTTP/2.0

[PidgeyBE]

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.
Fixes #7418

Kong 3.1.1 has nginx > 1.9.14, so the check is not needed any more.

Checklist

• The Pull Request has tests
• There's an entry in the CHANGELOG
• There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE

Issue reference

Fix #7418

[CLAassistant]

All committers have signed the CLA.

[bungle]

Thanks @PidgeyBE! This is nice!

[kikito]

@PidgeyBE how are you testing that this works? Could you perhaps add a test for this on the PR?

[PidgeyBE]

@PidgeyBE how are you testing that this works? Could you perhaps add a test for this on the PR?

@kikito
I started investigating this issue because we had a bug in our software where kubernetes was killing Kong (because it was using too much disk) when we were uploading big (10GB) files.

Google Chrome uses HTTP/2.0 by default, so although we instructed Kong to turn off request buffering on that route, it didn't help.
After implementing the changes of this MR, we could see in the logs that request buffering was indeed off and the Kong pod was not killed anymore for big uploads.

I'm currently on holiday with no PC access, but I can add a test next week! Would be nice if you could link me to the best place to add this test!

[PidgeyBE]

@kikito I checked the code and assume testing happens here https://github.com/Kong/kong/blob/master/spec/02-integration/05-proxy/27-unbuffered_spec.lua .
It seems however these tests are using lua-resty-http https://github.com/ledgetech/lua-resty-http#features (https://github.com/Kong/kong/blob/master/spec/helpers.lua#L654), which only supports HTTP 1.0 and 1.1.
So I'm not sure how to proceed... :/

[dndx]

@PidgeyBE Could you enable the option with your patch and manually test this with a HTTP/2 compliant client (like curl --http2-prior-knowledge) to see if this feature actually works?

[PidgeyBE]

@dndx Yes, that's how I've tested it, in addition to testing via Google Chrome, which also defaults to HTTP/2.0. Do you want me to repeat the test and make a video somehow?

[StarlightIbuki]

@dndx Yes, that's how I've tested it, in addition to testing via Google Chrome, which also defaults to HTTP/2.0. Do you want me to repeat the test and make a video somehow?

Generally, we encourage adding automated tests for every PR. If possible, we should let CI run the test for every new coming PRs after the fix is applied.

[PidgeyBE]

@dndx Yes, that's how I've tested it, in addition to testing via Google Chrome, which also defaults to HTTP/2.0. Do you want me to repeat the test and make a video somehow?

Generally, we encourage adding automated tests for every PR. If possible, we should let CI run the test for every new coming PRs after the fix is applied.

@StarlightIbuki Yes of course! The issue is the current test framework does not support HTTP/2.0. So the only thing I can test in CI is if HTTP/1.1 still works. The feature I adapted already had tests for HTTP/1.1 and these still work.

[StarlightIbuki]

@dndx Yes, that's how I've tested it, in addition to testing via Google Chrome, which also defaults to HTTP/2.0. Do you want me to repeat the test and make a video somehow?

Generally, we encourage adding automated tests for every PR. If possible, we should let CI run the test for every new coming PRs after the fix is applied.

@StarlightIbuki Yes of course! The issue is the current test framework does not support HTTP/2.0. So the only thing I can test in CI is if HTTP/1.1 still works. The feature I adapted already had tests for HTTP/1.1 and these still work.

Thank you for reminding me of this. We will be discussing http2 support for test.

[bungle]

@dndx Yes, that's how I've tested it, in addition to testing via Google Chrome, which also defaults to HTTP/2.0. Do you want me to repeat the test and make a video somehow?

Generally, we encourage adding automated tests for every PR. If possible, we should let CI run the test for every new coming PRs after the fix is applied.

@StarlightIbuki Yes of course! The issue is the current test framework does not support HTTP/2.0. So the only thing I can test in CI is if HTTP/1.1 still works. The feature I adapted already had tests for HTTP/1.1 and these still work.

Thank you for reminding me of this. We will be discussing http2 support for test.

There is http client capable of http2 in our test dependencies:
https://github.com/Kong/kong/blob/master/Makefile#L6

That is the http 0.4. There is even a test helper based on it with similar interface as http client:
https://github.com/Kong/kong/blob/master/spec/helpers.lua#L1014-L1057

You can see it in action here:
https://github.com/Kong/kong/blob/master/spec/02-integration/08-status_api/01-core_routes_spec.lua#L104-L145

@PidgeyBE could you try that?

[PidgeyBE]

@dndx Yes, that's how I've tested it, in addition to testing via Google Chrome, which also defaults to HTTP/2.0. Do you want me to repeat the test and make a video somehow?

Generally, we encourage adding automated tests for every PR. If possible, we should let CI run the test for every new coming PRs after the fix is applied.

@StarlightIbuki Yes of course! The issue is the current test framework does not support HTTP/2.0. So the only thing I can test in CI is if HTTP/1.1 still works. The feature I adapted already had tests for HTTP/1.1 and these still work.

Thank you for reminding me of this. We will be discussing http2 support for test.

There is http client capable of http2 in our test dependencies: https://github.com/Kong/kong/blob/master/Makefile#L6

That is the http 0.4. There is even a test helper based on it with similar interface as http client: https://github.com/Kong/kong/blob/master/spec/helpers.lua#L1014-L1057

You can see it in action here: https://github.com/Kong/kong/blob/master/spec/02-integration/08-status_api/01-core_routes_spec.lua#L104-L145

@PidgeyBE could you try that?

@bungle Yes sure!
Thanks for the pointers! I'm quite new in the Lua ecosystem so I'm struggling a bit with setting up a local environment where I can run make test-integration.
I've tried running everything from source but I'm afraid I screwed up my computer so hard that it's hard to recover from.
Currently stuck at:

 make test
starting make in kong
fatal: No names found, cannot describe anything.
/usr/bin/env: ‘resty’: Permission denied
make: *** [Makefile:184: test] Error 126

As alternative I tried https://github.com/Kong/kong-build-tools; and followed all steps, but it's also failing at different points. (e.g. prebuild docker images not available, missing flags for buildx, etc.. )
Am I missing some some obvious way to easily run the tests on my local machine? 😬

//edit: I've found out about .devcontainer and in this environment I can at least run make test-integration. However, most tests seem to fail because FATAL: database "kong_tests" does not exist. So also this way of working seems to be not really well supported :/
//edit2: Seems the necessary env vars (like KONG_TEST_PG_DATABASE) are missing in the .devcontainer setup
//edit3: I can finally run bin/busted -v --exclude-tags=flaky,ipv6,cassandra -p unbuffered_spec on my own laptop 🎉 After putting this in comment:

--set_log_level = require("resty.kong.log").set_log_level

because I got an error

Error: ./kong/cmd/start.lua:101: nginx: [error] init_by_lua error: ./kong/runloop/handler.lua:108: module 'resty.kong.log' not found:No LuaRocks module found for resty.kong.log 

[PidgeyBE]

@bungle I'm trying to add tests for HTTP/2.0.
I'm making baby steps progress. I can send requests over HTTP/2.0 via the pointers you gave me.
I can send a body rstring.to_hex(random.bytes(100)) but if I send a body rstring.to_hex(random.bytes(1000)), the request times out... The issue is unrelated to the changes I did, cause I'm facing the same issues on master as well...
Do you have any idea what could be going wrong? :/
I've put the code changes for the tests here https://github.com/PidgeyBE/kong/pull/1/files

[dndx]

@PidgeyBE the http Lua crate is not very stable, request timing out could very likely be the Nginx process crashed.

You can check the output of dmesg to verify this. In addition, I would not mind if we use external process like curl for sending HTTP/2 requests. You just need to find a way to wrap this in a Lua helper.

[PidgeyBE]

@PidgeyBE the http Lua crate is not very stable, request timing out could very likely be the Nginx process crashed.

You can check the output of dmesg to verify this. In addition, I would not mind if we use external process like curl for sending HTTP/2 requests. You just need to find a way to wrap this in a Lua helper.

@dndx Ok 😬
Do you think https://github.com/Lua-cURL/Lua-cURLv3 is a stable crate?

[PidgeyBE]

Dear @dndx @bungle
I've refactored the tests a bit in https://github.com/PidgeyBE/kong/pull/1/files
I've added lua-curl as a dependency to be able to use Curl in the tests.

The tests I made work for both HTTP/1.1 and HTTP/2.0 and I can run them locally, with success.

local versions_to_test = {"CURL_HTTP_VERSION_1_1", "CURL_HTTP_VERSION_2_0"}
for _, version in pairs(versions_to_test) do
  local http_version = HTTP_VERSIONS[version]
  for _, strategy in helpers.each_strategy() do
    ...

If I run the same tests on master branch (where the fix this PR is about is not present), the tests for HTTP/2.0 fail (as expected).

If you guys have some early feedback or approve these tests, let me know! 🙏

[PidgeyBE]

Alright, I've merged the tests! 👌
So all should be good now!

[hbagdi]

@bungle @dndx Could you review and merge this one? Big deal patch!

some of the ci things still bothers me, I will try something on my own

[bungle]

@PidgeyBE one last ask, can you rebase this to latest master, and le's run the test suite once more (I'll hit run in a morning my time). I think I am going to merge this tomorrow. I tried to tests working with http, but it seems the it will stuck around here:
https://github.com/daurnimator/lua-http/blob/master/http/h2_stream.lua#L1222

I also tried the https://github.com/tokers/lua-resty-http2 with a slightly better success, but couldn't yet figure out how to write the code so that I can send more than 64kB (seems to require lower level APIs for that). So both kinda dead-ends and your solution using Lua cURL feels the best.

[PidgeyBE]

@PidgeyBE one last ask, can you rebase this to latest master, and le's run the test suite once more (I'll hit run in a morning my time). I think I am going to merge this tomorrow. I tried to tests working with http, but it seems the it will stuck around here: https://github.com/daurnimator/lua-http/blob/master/http/h2_stream.lua#L1222

I also tried the https://github.com/tokers/lua-resty-http2 with a slightly better success, but couldn't yet figure out how to write the code so that I can send more than 64kB (seems to require lower level APIs for that). So both kinda dead-ends and your solution using Lua cURL feels the best.

Indeed! I more or less traveled the same path 👍