Fix snappy/framed-snappy encoding/decoding #12911
Conversation
|
Hey @skandragon, I took the liberty of continuing your PR just because we're blocked in developing the prometheusremotewritereceiver. I hope that's okay! |
Signed-off-by: Arthur Silva Sens <[email protected]>
Codecov ReportAttention: Patch coverage is
❌ Your patch check has failed because the patch coverage (80.37%) is below the target coverage (95.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #12911 +/- ##
==========================================
- Coverage 91.65% 91.61% -0.05%
==========================================
Files 499 499
Lines 27426 27492 +66
==========================================
+ Hits 25138 25186 +48
- Misses 1809 1822 +13
- Partials 479 484 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Arthur Silva Sens <[email protected]>
| change_type: bug_fix | ||
|
|
||
| # The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver) | ||
| component: confighttp |
There was a problem hiding this comment.
Should we say that we are changing both?
| component: confighttp | |
| component: confighttp and configcompression |
| subtext: | | ||
| The collector used the Snappy compression type of "framed" to handle the HTTP | ||
| content-encoding "snappy". However, this encoding is typically used to indicate | ||
| the "block" compression variant of "snappy". This change allows the collector to: | ||
| - When receiving a request with encoding 'snappy', the server endpoints will peek | ||
| at the first bytes of the payload to determine if it is "framed" or "block" snappy, | ||
| and will decompress accordingly. This is a backwards-compatible change. | ||
|
|
||
| If the feature-gate "confighttp.framedSnappy" is enabled, you'll see new behavior for both client and server: | ||
| - Client compression type "snappy" will now compress to the "block" variant of snappy | ||
| instead of "framed". Client compression type "x-snappy-framed" will now compress to the "framed" variant of snappy. | ||
| - Servers will accept both "snappy" and "x-snappy-framed" as valid content-encodings. |
There was a problem hiding this comment.
Maybe would be nice to add these details here, or just add the new compression type: x-snappy-framed
| // snappyHandler returns an io.ReadCloser that auto-detects the snappy format. | ||
| // This is necessary because the collector previously used "content-encoding: snappy" | ||
| // but decompressed and compressed the payloads using the snappy framing format. | ||
| // However, "content-encoding: snappy" is uses the block format, and "x-snappy-framed" | ||
| // is the framing format. This handler is a (hopefully temporary) hack to | ||
| // make this work in a backwards-compatible way. |
There was a problem hiding this comment.
On the original issue, the contributors shared those references:
https://github.com/google/snappy/blob/main/framing_format.txt
https://pkg.go.dev/github.com/golang/snappy#pkg-overview
Maybe add them here on the function comment?
| If the feature-gate "confighttp.framedSnappy" is enabled, you'll see new behavior for both client and server: | ||
| - Client compression type "snappy" will now compress to the "block" variant of snappy | ||
| instead of "framed". Client compression type "x-snappy-framed" will now compress to the "framed" variant of snappy. | ||
| - Servers will accept both "snappy" and "x-snappy-framed" as valid content-encodings. |
There was a problem hiding this comment.
I think this is not clear about when gate is disabled (default).
| if enableFramedSnappy.IsEnabled() { | ||
| //nolint:unparam // Ignoring the linter request to remove error return since it needs to match the method signature | ||
| availableDecoders["x-snappy-framed"] = func(body io.ReadCloser) (io.ReadCloser, error) { | ||
| // Lazy Reading content to improve memory efficiency | ||
| return &compressReadCloser{ | ||
| Reader: snappy.NewReader(body), | ||
| orig: body, | ||
| }, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't think it would cause problems either way, but is there a need to put support for the x-snappy-framed format behind the gate?
| @@ -141,6 +200,7 @@ func httpContentDecompressor(h http.Handler, maxRequestBodySize int64, eh func(w | |||
| errHandler = eh | |||
| } | |||
|
|
|||
| availableDecoders := availableDecoders() | |||
There was a problem hiding this comment.
Instead of rebuilding the map every time we setup a server, I think we could probably put the code of availableDecoders() into an init() function. I don't expect server creation to be a performance bottleneck for anyone though, so I don't think it's a big concern.
Description
This is an alternative PR to #12825.
I'm taking all the commits from that PR and adding the feature gate on the client side, as requested by reviews. The server behavior of peeking into the initial bytes to identify the encoding is kept :)
If you've reviewed the previous PR, you can just review the latest commit!
Link to tracking issue
Fixes #10584