-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
confighttp: Including lz4 as supported compression encoding #11108
confighttp: Including lz4 as supported compression encoding #11108
Conversation
0e22c51
to
623ae28
Compare
Fixes #9128 |
d84a596
to
4c355ac
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #11108 +/- ##
=======================================
Coverage 91.91% 91.92%
=======================================
Files 430 430
Lines 20313 20327 +14
=======================================
+ Hits 18671 18685 +14
Misses 1268 1268
Partials 374 374 ☔ View full report in Codecov by Sentry. |
d068cd4
to
bc5b681
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.
This looks good. I reviewed the library's code as well, and I don't think it would be affected by a situation similar to the CVE we had a couple of months ago. That said, would you be able to do a positive confirmation? The test would be to encode a huge blob that compressed to a tiny payload, and see if the decoding would explode the collector.
12f1972
to
5cf5312
Compare
So this somewhat blew out the original PR intent but, I also noticed that snappy would also be subject to a similar attack considering the steps it would take:
To work around this, I wrote a small wrapper that effectively delays the read until it is actually needed however, since we are no longer reading the entire compressed body, that buffer is also coupled with the compressed reader. This allows the wrapper on close to discard any remaining compressed values and return the result of the original buffer's closed state. |
Turns out I don't actually need the wrapper, I could move the logic into the ServeHTTP method. I will clean up the commit history once the pipeline is happy. |
171bcdc
to
5ea30fc
Compare
e77f66c
to
3f6fb5f
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.
Looks great now!
a27996b
to
34f2030
Compare
2e2827d
to
a4c6dac
Compare
Having trialed out the compression algorithm in a differnt project, the performance impact has greatly reduced the amount of resource overhead with similar compression ratios when compared to flate or gzip.
9e77f44
to
51ddec9
Compare
…emetry#11108) #### Description Adding in support for lz4 compression into the project, I have seen it perform amazing well in other projects that send a lot of traffic and were originally using `gzip` or `flate`. It would be really great to have for environments that are cpu, memory sensitive. #### Link to tracking issue Fixes 9128 #### Testing I've updated the existing test frameworks to include lz4 in their suite #### Documentation I've updated the readmes to include lz4 but I suspect there might be more places to update.
Description
Adding in support for lz4 compression into the project, I have seen it perform amazing well in other projects that send a lot of traffic and were originally using
gzip
orflate
.It would be really great to have for environments that are cpu, memory sensitive.
Link to tracking issue
Fixes 9128
Testing
I've updated the existing test frameworks to include lz4 in their suite
Documentation
I've updated the readmes to include lz4 but I suspect there might be more places to update.