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

Chunked HTTP requests broken in 0.67.0 #6904

Closed
David-Wobrock opened this issue Jul 30, 2024 · 2 comments · Fixed by #6906
Closed

Chunked HTTP requests broken in 0.67.0 #6904

David-Wobrock opened this issue Jul 30, 2024 · 2 comments · Fixed by #6906
Labels

Comments

@David-Wobrock
Copy link

David-Wobrock commented Jul 30, 2024

Short description

We identified a regression in 0.67.0 with chunked requests.
More specifically we could bisect it to this commit: c5706ee

When upgrading from 0.66.0 to 0.67.0, an HTTP request like this would work (<=> be allowed),

POST /v1/data/authorization HTTP/1.1
Host: openpolicyagent:8181
User-Agent: Go-http-client/1.1
Transfer-Encoding: chunked
Content-Type: application/json
Accept-Encoding: gzip

{
  "input": {
    "sub": "my-user",
   [...]
}

But now returns:

HTTP/1.1 200 OK
Content-Type: application/json
Vary: Accept-Encoding
Date: Tue, 30 Jul 2024 09:34:00 GMT
Content-Length: 211

{
    "decision_id": "1b31836d-ea2d-4288-b52d-80e43339dd3c",
    "result": {
        "allow": false,
        "reasons": {
            "allow": [],
            "deny": []
        },
        "team": []
    },
    "warning": {
        "code": "api_usage_warning",
        "message": "'input' key missing from the request"
    }
}

The culprit seems to be the Transfer-Encoding: chunked header.

I reckon it's not properly handled by the new code that handles the maximum request body size.

  • OPA version: 0.67.0
  • Any input and any rules should allow to reproduce

Steps To Reproduce

  1. Run a 0.67.0 server locally
  2. Do a cURL request like
curl --location 'http://localhost:8181/v1/data/authorization' \
--header 'Transfer-Encoding: chunked' \
--header 'Content-Type: application/json' \
--data '{
  "input": {
    "sub": "my-user"
  }
}'

Expected behavior

That the body is read properly.

Additional context

We use a Golang service that forwards requests to OPA, and formats the response (to return a 403 on a /v1/data endpoint).
Anyway, in Golang, according to the documentation:

// TransferEncoding lists the transfer encodings from outermost to
// innermost. An empty list denotes the "identity" encoding.
// TransferEncoding can usually be ignored; chunked encoding is
// automatically added and removed as necessary when sending and
// receiving requests.
TransferEncoding []string

With emphasis on chunked encoding is automatically added and removed as necessary :)
Meaning that most Golang clients will probably break when upgrading to 0.67.0.

Our workaround for now is to force a single request and no chunking:

	req.ContentLength = r.ContentLength
	req.TransferEncoding = []string{}
	resp, err := p.Client.Do(req)

which seems to do the trick.

Thanks for this project, and let me know if I can provide further details :)

@ashutosh-narkar
Copy link
Member

Thanks for filing the issue. We'll investigate and provide a fix soon.

@philipaconrad
Copy link
Contributor

Chunked encoding absolutely causes problems for the request-handling changes in #6868. I'll see if I can put together a bugfix PR soon. 🤞

Thank you for your patience, and for filing the issue, @David-Wobrock!

philipaconrad added a commit to philipaconrad/opa that referenced this issue Jul 31, 2024
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 added a commit to philipaconrad/opa that referenced this issue Aug 1, 2024
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 added a commit to philipaconrad/opa that referenced this issue Aug 1, 2024
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>
ashutosh-narkar pushed a commit to ashutosh-narkar/opa that referenced this issue Aug 1, 2024
…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)
ashutosh-narkar pushed a commit that referenced this issue Aug 5, 2024
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants