Add support for gzip content-encoding in HTTP client #8731
Conversation
- Toggled via `--tls_enable_gzip` default off - Send headers indicating gzip support and decompress gzipped responses - Clean up existing gzip compression functoin and add decompression function - Update tests for roundtrip compression and integration testing gzip with an HTTP server
directionless
left a comment
There was a problem hiding this comment.
What about using gzip for the uploads?
Also, this feels like something we should enable by default, and thus that we should build the flag names to be more reasonable when we roll that out.
| CLI_FLAG(bool, | ||
| tls_enable_gzip, | ||
| false, | ||
| "Enable gzip compression for HTTP responses"); |
There was a problem hiding this comment.
If I imagine us in a year, this feels wrong. This feels like the sort of thing we should always have enabled, and perhaps a flag to force disabling it for broken http servers.
So I think we should name it in a way that makes that clear. (eg: I think we'll want to default this to enabled in the future)
There was a problem hiding this comment.
Yes I agree that we probably want to make it default at some point, but I definitely want to keep it off by default at first. Perhaps just swap this to tls_disable_gzip and flip the default for now?
There was a problem hiding this comment.
Yah -- I think default false for a release is correct. But I think we should name it so we don't end up telling people to use tls_enable_gzip=false
so renaming to tls_disable_gzip makes sense
|
For requests from the osquery client, I think we may want another mechanism for enabling and disabling. Perhaps the server needs to return an I would love to pursue adding that. Ideally we merge this before the coming release so that we can start testing it and continue to expand the gzip capabilities. |
Why would be a different option? |
Because the client can send an It seems like some state needs to be maintained on the client indicating whether the server can receive gzip. When I think about that, I lean towards the server being able to return a configuration option that indicates it can receive that encoding. |
|
Alternatively, I'd be okay with calling the flag |
|
We discussed further in Slack. Now updated per the discussion. |
|
|
||
| `--tls_accept_gzip=true` | ||
|
|
||
| (Default false) Enable the HTTP client to process gzip responses from the server. Will turn on sending an `Accept-Encoding: gzip` header. In the future this flag may default to true, or be removed entirely with the feature always on. |
There was a problem hiding this comment.
Removing a flag would be a breaking change, right? (Because starting osquery with an invalid flag would crash). Maybe the flag can be deprecated+hidden but not removed?
There was a problem hiding this comment.
Yah. Toggling the default would be safe, and I assume Zach meant "remove the functionality attached"
There was a problem hiding this comment.
Yeah if we really removed it I think we would need to do it in a major version or just log a warning without crashing.
| return output; | ||
| } | ||
|
|
||
| std::string decompressString(const std::string& data) { |
There was a problem hiding this comment.
Could we return an error/status? Or maybe just add VLOG(1)s on the places that there's an error and the empty string is returned. (Thinking about troubleshooting.)
lucasmrod
left a comment
There was a problem hiding this comment.
LGTM! Left a few comments.
--tls_accept_gzipdefault off (may switch to default on in future releases)