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(grpc-web): err when > client_body_buffer_size #12172

Closed
wants to merge 1 commit into from

Conversation

StarlightIbuki
Copy link
Contributor

Summary

grpc-web uses get_raw_body() to fetch the request body, and does not handle the error when the body exceeds client_body_buffer_size.

Checklist

  • The Pull Request has tests
  • A changelog file has been created under changelog/unreleased/kong or skip-changelog label added on PR if changelog is unnecessary. README.md

Issue reference

Fix FTI-5624

warn("client_body_buffer_size exceeded and reading the request from disk. Please consider increasing the value.")

local file = assert(io_open(body_file, "rb"))
body = assert(file:read("*a"))
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be careful with such blocking file io operations. If the file is large, it will block the whole nginx event loop significantly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's the reason for emitting the warning message. And there's no better solution: we are not using a streaming JSON parser and the pb_unframe.

Copy link
Contributor Author

@StarlightIbuki StarlightIbuki Dec 8, 2023

Choose a reason for hiding this comment

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

A possible solution is to read with limited length and yield after every read, but it increase the memory and CPU usage.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this is a possible approach, or perhaps throwing it to another OS thread to avoid blocking the main worker thread.

Copy link
Member

Choose a reason for hiding this comment

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

A good candidate for:
https://github.com/openresty/lua-nginx-module#ngxrun_worker_thread

But that needs an addition to nginx.conf, such as:

thread_pool default threads=32 max_queue=65536;

@bungle
Copy link
Member

bungle commented Dec 8, 2023

grpc-web uses get_raw_body() to fetch the request body, and does not handle the error when the body exceeds client_body_buffer_size.

This is currently the generic design choice of Kong. Aka plugins and PDK does not read file buffered bodies. I think we should not start making specific plugin related changes, but perhaps allow reading files or continue not to allowl

e.g. should we just enable it with worker thread. Though buffering to file and then reading back to memory feels a bit counter intuitive. Aka why not then just grow the buffers.

@StarlightIbuki
Copy link
Contributor Author

@catbro666 @bungle I agree that this is not an ideal solution. Even if we read from the file, it still uses that amount of memory. I'm confirming the reason why the customer does not want to increase the buffer size.

@catbro666
Copy link
Contributor

catbro666 commented Dec 8, 2023

@catbro666 @bungle I agree that this is not an ideal solution. Even if we read from the file, it still uses that amount of memory. I'm confirming the reason why the customer does not want to increase the buffer size.

These two are still different. The client body buffer will be allocated for every request as long as the whole request body was not pre-read in the header buffer(s), while reading from the file only happens when the client_body_buffer_size is not enough.

@StarlightIbuki
Copy link
Contributor Author

Closing as the customer seems satisfied with the solution.

@StarlightIbuki StarlightIbuki deleted the fix/grpc-web-large-req branch January 21, 2025 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants