-
Notifications
You must be signed in to change notification settings - Fork 596
feat: default remove accept encoding header #7562
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
base: main
Are you sure you want to change the base?
feat: default remove accept encoding header #7562
Conversation
06ba285 to
4c395ee
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7562 +/- ##
==========================================
- Coverage 72.33% 72.32% -0.02%
==========================================
Files 232 232
Lines 34109 34116 +7
==========================================
+ Hits 24674 24675 +1
- Misses 7664 7668 +4
- Partials 1771 1773 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
4c395ee to
80e9420
Compare
|
/retest |
b0f55f4 to
d2ef2f5
Compare
|
/retest |
|
rethinking this one, shouldnt this be enabled by default ? |
They pass it directly to the backend, this is not defaulted anywhere. Do we want to default it? That would make it opinionated, but I think if compression is enabled at the Envoy level, we should. |
|
@arkodg I just made this |
9d3f0af to
985c440
Compare
Signed-off-by: Steven Kreitzer <[email protected]>
985c440 to
28e0a06
Compare
Signed-off-by: Steven Kreitzer <[email protected]>
| } | ||
|
|
||
| // Remove accept-encoding header to prevent double compression | ||
| route.RequestHeadersToRemove = append(route.RequestHeadersToRemove, "accept-encoding") |
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.
why aren't we using the setting from the filter
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 doesn't work correctly, at all. If I append apply the removeAcceptEncodingHeader on every compressor (no matter if its on the HCM one or the route overrides), it stops compression all together. If I apply it on one, it works, but it will only remove that header if that compression is picked.
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 filed this in the meantime: envoyproxy/envoy#42160
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, lets wait for the outcome of this before pushing code that needs to be updated again
Signed-off-by: Steven Kreitzer <[email protected]>
d4c0e87 to
d093020
Compare
Continuation of #7406
This removes the
accept-encodingheader from each route if a compressor is present.