feat: automatically set first compressor with choose_first#7406
feat: automatically set first compressor with choose_first#7406arkodg merged 5 commits intoenvoyproxy:mainfrom
Conversation
2e305d6 to
8a5b844
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7406 +/- ##
==========================================
- Coverage 72.36% 71.21% -1.15%
==========================================
Files 231 274 +43
Lines 34042 34815 +773
==========================================
+ Hits 24633 24793 +160
- Misses 7633 8238 +605
- Partials 1776 1784 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -113,26 +113,40 @@ static_resources: | |||
| envoy.filters.http.compression: | |||
| "@type": type.googleapis.com/envoy.extensions.filters.http.compressor.v3.CompressorPerRoute | |||
| overrides: | |||
| {{- if .PrometheusCompressionRemoveAcceptEncodingHeader }} | |||
There was a problem hiding this comment.
thinking this loud, should we move 19001 to dynamic listener
cc @envoyproxy/gateway-maintainers
2b2606e to
ce5e36f
Compare
|
/retest |
4c6f00a to
587d9a5
Compare
|
/retest |
|
@arkodg Can I get a review please? :-) |
| Type: c.Type, | ||
| ChooseFirst: ptr.Deref(&c.ChooseFirst, false), | ||
| RemoveAcceptEncodingHeader: ptr.Deref(&c.RemoveAcceptEncodingHeader, false), |
There was a problem hiding this comment.
Should we update these set of tests (backendtrafficpolicy-compressor-*.in.yaml) and verify that the Compressor field can also properly handle new ChooseFirst and RemoveAcceptEncodingHeader during merging?
There was a problem hiding this comment.
ChooseFirst and RemoveAcceptEncodingHeader are global, so you cannot just disable them per-route like that example.
There was a problem hiding this comment.
I see, just for my understanding, does that mean we can not do the following?
backendTrafficPolicies:
# Gateway-level policy - Enable Gzip only
- apiVersion: gateway.envoyproxy.io/v1alpha1
kind: BackendTrafficPolicy
metadata:
namespace: envoy-gateway
name: gateway-compressor-policy
spec:
targetRef:
group: gateway.networking.k8s.io
kind: Gateway
name: gateway-1
compressor:
- type: Gzip
chooseFirst: true # enabled at gateway level
gzip: {}
# Route-level policy - Switch to Brotli and Zstd, disable Gzip
- apiVersion: gateway.envoyproxy.io/v1alpha1
kind: BackendTrafficPolicy
metadata:
namespace: default
name: route-compressor-policy-switch-to-brotli-and-zstd
spec:
targetRef:
group: gateway.networking.k8s.io
kind: HTTPRoute
name: httproute-1
compressor:
- type: Gzip
gzip: null # disabled in route level
- type: Brotli
chooseFirst: true # using chooseFirst for Brotli in the route level
brotli: {}
- type: Zstd
zstd: {}
mergeType: JSONMerge|
is this API really needed, can be leverage the list order and set the first value as choose first ? |
Do we want to assume that? |
are there any drawbacks to this approach ? |
27f7480 to
30dccf7
Compare
Just thought about it, not really. We can just assume the first item in the compression list is choosen first. This also allows us to set removeAcceptEncodingHeader per individual route instead of global. |
|
@buroa can we also split up the PR into 2 - one for |
Can do. |
Signed-off-by: Steven Kreitzer <skre@skre.me>
db23c03 to
af23c25
Compare
Signed-off-by: Steven Kreitzer <skre@skre.me>
|
can we add a doc string in the API to highlight that order matters and if multiple values are present, we pick the one first in the list |
7e5adde to
8a52cc4
Compare
Signed-off-by: Steven Kreitzer <skre@skre.me>
8a52cc4 to
c409501
Compare
Signed-off-by: Steven Kreitzer <skre@skre.me>
Signed-off-by: Steven Kreitzer <skre@skre.me>
| compressorProto := compressorPerRouteConfig() | ||
| if compressorAny, err = proto.ToAnyWithValidation(compressorProto); err != nil { | ||
| return err |
There was a problem hiding this comment.
Can we create this outside of the for loop? Is there any advantage to doing it inside?
There was a problem hiding this comment.
Is see, it now takes *ir.Compression
There was a problem hiding this comment.
Yeah, in the next PR it will use that to determine the accept encoding overrides.
|
/retest |
1 similar comment
|
/retest |
This adds a new
chooseFirstto each compressor.When
chooseFirstis true, the compressor is preferred when q-values in Accept-Encoding are equal. If multiple compressor filters set choose_first to true, the last one in the filter chain is chosen.I also cleaned up some code in various places. There was some duplication happening.
chooseFirstis automatically set to true based on the first item in the compressor/compression list.Closes #5796