util+server: Fix bug around chunked request handling. #6906
Conversation
| // For chunked rqeuests (where full size is not known in advance), | ||
| // pass server.decoding.max_length down, using the request context. | ||
| if r.ContentLength < 0 || (r.ContentLength == 0 && r.Body != nil) { | ||
| ctx := util_decoding.AddServerDecodingMaxLen(r.Context(), maxLength) |
There was a problem hiding this comment.
This is where the new context key is inserted. Note that we only add it when we have an "indeterminate body size" case, such as what happens during chunked requests.
There was a problem hiding this comment.
Is this only the case for chunked requests or are there some other use cases? If only true for chunked requests, can we just check the header (ie. 'Transfer-Encoding: chunked')?
Also it would be helpful to add a comment which explain the 2 conditions in the if statement.
| @@ -1713,8 +1713,10 @@ func generateJSONBenchmarkData(k, v int) map[string]interface{} { | |||
| } | |||
|
|
|||
| return map[string]interface{}{ | |||
| "keys": keys, | |||
| "values": values, | |||
| "input": map[string]interface{}{ | |||
There was a problem hiding this comment.
This is a minor refactoring that silences "'input' key missing" warnings from the testcases.
| // incremental read of the body. In this case, we can't be too clever, we | ||
| // just do the best we can with whatever is streamed over to us. | ||
| // Fetch gzip payload size limit from request context. | ||
| if maxLength, ok := decoding.GetServerDecodingMaxLen(r.Context()); ok { |
There was a problem hiding this comment.
This is where we add back the original io.ReadAll code path. It's only used when we have an indeterminate request body length case, otherwise we fall back to the preallocated buffer logic from #6868.
| // For chunked rqeuests (where full size is not known in advance), | ||
| // pass server.decoding.max_length down, using the request context. | ||
| if r.ContentLength < 0 || (r.ContentLength == 0 && r.Body != nil) { | ||
| ctx := util_decoding.AddServerDecodingMaxLen(r.Context(), maxLength) |
There was a problem hiding this comment.
Is this only the case for chunked requests or are there some other use cases? If only true for chunked requests, can we just check the header (ie. 'Transfer-Encoding: chunked')?
Also it would be helpful to add a comment which explain the 2 conditions in the if statement.
philipaconrad
force-pushed
the
philip/fix-issue-6904
branch
from
c4135cb
to
41fa295
Compare
|
Thanks for the review comments @ashutosh-narkar!
Searching in the sources of
I've embellished the original comment to explain the why behind the "magic" check, and have simplified down to just 1x check, because the other conditional check applies only to client requests, where Golang is sending a request, not receiving it. There's a bit of history documented in the sources around why that condition existed, and it dates back to backwards compatibility stuff in Go 1.8. |
This commit fixes a request handling bug introduced in open-policy-agent#6868, which caused OPA to treat all incoming chunked requests as if they had zero-length request bodies. The fix detects cases where the request body size is unknown in the DecodingLimits handler, and propagates a request context key down to the `util.ReadMaybeCompressedBody` function, allowing it to correctly select between using the original `io.ReadAll` style for chunked requests, or the newer preallocated buffers approach (for requests of known size). This change has a small, but barely visible performance impact for large requests (<5% increase in GC pauses for a 1GB request JSON blob), and minimal, if any, effect on RPS under load. Fixes: open-policy-agent#6904 Signed-off-by: Philip Conrad <philip@chariot-chaser.net>
philipaconrad
force-pushed
the
philip/fix-issue-6904
branch
from
41fa295
to
16dee19
Compare
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
…ent#6906) This commit fixes a request handling bug introduced in open-policy-agent#6868, which caused OPA to treat all incoming chunked requests as if they had zero-length request bodies. The fix detects cases where the request body size is unknown in the DecodingLimits handler, and propagates a request context key down to the `util.ReadMaybeCompressedBody` function, allowing it to correctly select between using the original `io.ReadAll` style for chunked requests, or the newer preallocated buffers approach (for requests of known size). This change has a small, but barely visible performance impact for large requests (<5% increase in GC pauses for a 1GB request JSON blob), and minimal, if any, effect on RPS under load. Fixes: open-policy-agent#6904 Signed-off-by: Philip Conrad <philip@chariot-chaser.net> (cherry picked from commit ee9ab0b)
This commit fixes a request handling bug introduced in #6868, which caused OPA to treat all incoming chunked requests as if they had zero-length request bodies. The fix detects cases where the request body size is unknown in the DecodingLimits handler, and propagates a request context key down to the `util.ReadMaybeCompressedBody` function, allowing it to correctly select between using the original `io.ReadAll` style for chunked requests, or the newer preallocated buffers approach (for requests of known size). This change has a small, but barely visible performance impact for large requests (<5% increase in GC pauses for a 1GB request JSON blob), and minimal, if any, effect on RPS under load. Fixes: #6904 Signed-off-by: Philip Conrad <philip@chariot-chaser.net> (cherry picked from commit ee9ab0b)
What are the changes in this PR?
This PR fixes a request handling bug introduced in #6868, which caused OPA to treat all incoming chunked requests as if they had zero-length request bodies.
The fix detects cases where the request body size is unknown in the DecodingLimits handler, and propagates a request context key down to the
util.ReadMaybeCompressedBodyfunction, allowing it to correctly select between using the originalio.ReadAllstyle for chunked requests, or the newer preallocated buffers approach (for requests of known size).This change has a small, but barely visible performance impact for large requests (<5% increase in GC pauses for a 1GB request JSON blob), and minimal, if any, effect on RPS under load.
Fixes: #6904
Notes to assist PR review:
DecodingLimitshandler wrapper inserver/server.goto theutil.ReadMaybeCompressedBodyfunction inutil/read_gzip_body.go. This is used to signal cases where the request body is of indeterminate length, and the body reader should enforce the size limit itself.io.ReadAllcode path inutil.ReadMaybeCompressedBody, but it only triggers now on chunked requests, and is guarded both at the top-level in the DecodingLimits handler with aMaxBytesReader, and at theio.ReadAllcallsite with aLimitReader. This prevents accidentally reading more of the request body than intended.Further notes: