-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Compress OPA response & accept compressed input #5696
Compress OPA response & accept compressed input #5696
Conversation
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
eb07d30
to
da62d5c
Compare
This is great! We'll take a closer look next week. I wonder if this is something to consider? |
@anderseknert thank you |
Setting status to |
I'm planning to push the change this week, worst case, next one. |
Thanks for the update, @AdrianArnautu 👍 |
543ae95
to
285fce5
Compare
@anderseknert I've pushed the latest changes, this PR is ready for the review. Let me know if changes are needed, either naming/structure, features or something I've grossly overlooked. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great contribution! Code looks good to me, and a really extensive set of tests 👏
Only a few questions, really. Perhaps @johanfylling or @ashutosh-narkar could take a look as well, but this is a great improvement 👍
285fce5
to
e1a6c76
Compare
I've done the changes and rebased the branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! Some minor questions from my side 👇
2bf6b5d
to
74394a8
Compare
Ready for review @anderseknert & @srenatus |
74394a8
to
9597f39
Compare
9597f39
to
f9839d6
Compare
efbcc7e
to
840946e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that's an impressive first contribution.
@@ -73,8 +74,26 @@ func (h *LoggingHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { | |||
bs, r.Body, err = readBody(r.Body) | |||
} | |||
if err == nil { | |||
fields["req_body"] = string(bs) | |||
} else { | |||
if gzipReceived(r.Header) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's somewhat unfortunate that we have to decompress what we've ourselves compressed before just for logging it. I wonder if there's a different way of chaining handlers possible that would let us avoid that...
@AdrianArnautu thanks for this contribution. Can you please update the commit message explaining why the changes were made? Here are some guidelines. We can then merge this. |
840946e
to
911e794
Compare
…ed response and request body. It is available for the following REST API endpoints: - GET & POST HTTP methods on /v0/data & /v1/data endpoints - POST HTTP method on /v1/compile endpoint HTTP clients can optionally: - send 'Accept-Encoding: gzip' header and expect a gzip compressed body and a Content-Encoding: gzip response header. The server will send the content encoded as gzip only after a threshold defined by server.encoding.gzip.min_length (default value is 1024). If the size is below the threshold, the body is not compressed - send 'Content-Encoding: gzip' header and a gzip compressed body and expect the server to correctly interpret the request Fixes open-policy-agent#5310 Signed-off-by: aarnautu <[email protected]>
911e794
to
727b33e
Compare
@anderseknert , @srenatus & @ashutosh-narkar - thank you. |
Thank YOU @AdrianArnautu 👏 |
This change allows the HTTP clients to consume and send gzip compressed response and request body.
It is available for the following REST API endpoints:
GET
&POST
HTTP methods on/v0/data
&/v1/data
endpointsPOST
HTTP method on/v1/compile
endpoint.HTTP clients can optionally:
Accept-Encoding: gzip
header and expect a gzip compressed body and aContent-Encoding: gzip
response header. The server will send the content encoded as gzip only after a threshold defined byserver.gzip_min_length
(defaulted on 1024 bytes). If the size is below the threshold, the body is not compressedContent-Encoding: gzip
header and a gzip compressed body and expect the server to correctly interpret the requestReducing the network load in our setup is the reason for this change.
High level implementation details
Accept-Encoding: gzip
request header and responds with a gzip body. The implementation is independent of gorilla/mux HTTP routerserver.gzip_min_length
configurationContent-Encoding: gzip
request header and is decompressing the input before consuming itserver.gzip_min_length
server.gzip_compression_level
defaulted to value9
(best compression). See the gzip libraryFixes #5310
Signed-off-by: Adrian Arnautu [email protected]