-
Notifications
You must be signed in to change notification settings - Fork 689
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
internal/envoy: use consistent HTTP filter names #6124
Conversation
Use the same names for HTTP filters when they are added to an HTTP connection manager and when they are configured with per-filter config on virtual hosts or routes. Extracts a set of constants for filter names to ensure they remain in sync. This addresses a change in Envoy behavior in Envoy 1.29. See envoyproxy/envoy#29461 for more detail. Signed-off-by: Steve Kriss <stephen.kriss@gmail.com>
@@ -296,7 +309,7 @@ func (b *httpConnectionManagerBuilder) DefaultFilters() *httpConnectionManagerBu | |||
// identified by the TypeURL of each filter. | |||
b.filters = append(b.filters, | |||
&http.HttpFilter{ | |||
Name: "compressor", | |||
Name: CompressorFilterName, |
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 think it should be fine to change the filter names here as they're added to the HCM (making them all match the canonical filter names for consistency), but an alternate option would be to only update the per-filter config within the RouteConfigurations to match these names, without changing them here. Any thoughts?
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #6124 +/- ##
=======================================
Coverage 78.82% 78.82%
=======================================
Files 138 138
Lines 19766 19766
=======================================
Hits 15581 15581
Misses 3878 3878
Partials 307 307
|
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 think it should be ok to use the canonical names rather than match the shortened names in the typed configs even in the HTTP filters/HCM, i dont think we give any guarantees to observers of the config dump that things will be consistently named etc. so I would think its good
Use the same names for HTTP filters when they
are added to an HTTP connection manager and
when they are configured with per-filter config
on virtual hosts or routes. Extracts a set of
constants for filter names to ensure they remain
in sync.
This addresses a change in Envoy behavior in Envoy 1.29.
See envoyproxy/envoy#29461 for
more detail.