Handle requests with very large payload #1772
Conversation
…load it to the memory)
google-cla
bot
added
the
cla: yes
pkg/broker/ingress/handler.go
Outdated
| @@ -47,6 +47,9 @@ const ( | |||
| // TODO(liu-cong) configurable timeout | |||
| decoupleSinkTimeout = 30 * time.Second | |||
|
|
|||
| // Limit for request payload in bytes (100Mb) | |||
| maxRequestBodyBytes = 100000000 | |||
There was a problem hiding this comment.
Let me know if we'd rather have this as an env variable.
There was a problem hiding this comment.
Can you document how this number is picked? Is this the same as the pubsub message size limit?
There was a problem hiding this comment.
Sure, I have updated the description.
|
I recommend also wrapping the body in an io.LimitedReader. |
Some more context on this: I think this is indeed a more reliable approach. I myself considered MaxBytesReader, which is similar. In the context of recently completed perf tuning work (#1600), I wondered if the influence on memory consumption can be assumed to be negligible or whether the memory analysis in #1600 would need to be re-evaluated. Given this and that the malicious use case implies a wider problem (unauthorized access to the Broker first place), I thought it may be fine to keep it with current approach. What do you think? |
|
The following jobs failed:
Automatically retrying due to test flakiness... |
|
The following is the coverage report on the affected files.
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Ectelion, grantr The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Fixes #1754. In particular:
On memory consumption
Memory stress test was re-run several times. Here's is how a typical memory consumption pattern looks like:
In both cases, there is an initial spike varying around 600-800MiB, which then gradually smoothes out. There appears to be little to no impact on memory caused by the MaxBytesReader wrapper. This can also be seen from MaxBytesReader's implementation, which turns out to be lightweight and does not have any notable memory overheads.