-
Notifications
You must be signed in to change notification settings - Fork 632
feat: add minContentLength support for compression #7799
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: add minContentLength support for compression #7799
Conversation
Adds support for configuring minimum response size for compression in BackendTrafficPolicy. - Added MinContentLength field with validation (min 30 bytes) - Updated IR and XDS translator - Added tests Fixes envoyproxy#7781 Signed-off-by: Rajat Vig <rvig@etsy.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7799 +/- ##
==========================================
+ Coverage 72.79% 72.83% +0.03%
==========================================
Files 235 235
Lines 35176 35189 +13
==========================================
+ Hits 25606 25629 +23
+ Misses 7754 7747 -7
+ Partials 1816 1813 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
5ebaf3e to
446dcb6
Compare
Signed-off-by: Rajat Vig <rvig@etsy.com>
446dcb6 to
abeaa9f
Compare
|
/retest |
|
Could you also update the backendtrafficpolicy testdata with an example inside |
Signed-off-by: Rajat Vig <rvig@etsy.com>
…n-content-length Signed-off-by: Rajat Vig <rvig@etsy.com>
Signed-off-by: Rajat Vig <rvig@etsy.com>
Signed-off-by: Rajat Vig <rvig@etsy.com>
Signed-off-by: Rajat Vig <rvig@etsy.com>
…n-content-length Signed-off-by: Rajat Vig <rvig@etsy.com>
Signed-off-by: Rajat Vig <rvig@etsy.com>
|
/retest |
arkodg
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
jukie
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.
Thanks!
| route: | ||
| cluster: httproute/default/httproute-1/rule/0 | ||
| upgradeConfigs: | ||
| - upgradeType: websocket |
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.
curious why disappear perFilterConfig for compressor?
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.
found it, we should set compression field name in input file of L39.
@rajatvig
All your code looks good, just in case could you update xds testfile ?
What type of PR is this?
Feature
What this PR does / why we need it:
Adds
minContentLengthfield to BackendTrafficPolicy compression configuration. Allows operators to configure the minimum response size for compression, reducing CPU overhead on small responses.Which issue(s) this PR fixes:
Fixes #7781