-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Allow Content-Encoding to be unset when CompressionAlgorithms: []string{}
#14132
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
Conversation
Allow unset content encoding to get through when the decompression middleware is disabled fixes open-telemetry#14131
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #14132 +/- ##
==========================================
- Coverage 92.25% 92.24% -0.01%
==========================================
Files 658 658
Lines 41184 41186 +2
==========================================
- Hits 37995 37994 -1
- Misses 2183 2185 +2
- Partials 1006 1007 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@axw can you confirm that your comments have been addressed? |
axw
left a comment
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 good, just a few minor things.
| if len(d.decoders) == 0 { | ||
| return nil, nil // Signal: don't replace r.Body | ||
| } |
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.
Could we just not add the decompressor in the first place, if the configured list is empty?
(Happy for this to be done in a followup.)
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.
I tried to do that, but it was skipping decompression too much sometimes. I am happy to take another stab at it but I’d like to make it a follow-up.
Co-authored-by: Andrew Wilkins <[email protected]>
Co-authored-by: Andrew Wilkins <[email protected]>
axw
left a comment
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, thanks!
Description
When a receiver uses the empty array for CompessionAlgorithm, it tells the middleware to pass any compressed body through to the receiver. This works fine for any body that has a
Conent-Encodingheader set, but when that header is missing, an empty string, orIdentity, it will return a 400 response to the client without passing it along to the receiver.This change bypasses the error when Content-Encoding is unset, empty, or Identity.
Link to tracking issue
Fixes #14131 and open-telemetry/opentelemetry-collector-contrib#44010
Testing
Added some test cases to ensure the missing content-encoding doesn't error out. The test fails with the original code and passes with the minimal change in the
compression.gofile.Documentation