Skip to content
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

Is there any rule about about wellknown package? #552

Closed
zirain opened this issue Mar 31, 2022 · 17 comments
Closed

Is there any rule about about wellknown package? #552

zirain opened this issue Mar 31, 2022 · 17 comments

Comments

@zirain
Copy link
Member

zirain commented Mar 31, 2022

Hi folks
Is there any rule that should be followed when someone want add new filter names in wellknown package?

@alecholmez
Copy link
Contributor

Please just follow the existing naming schemes in the file. If you wish to open a PR please do I will review

@zirain
Copy link
Member Author

zirain commented Apr 6, 2022

thanks for your response.

@zirain zirain closed this as completed Apr 6, 2022
@jpeach
Copy link
Contributor

jpeach commented Apr 6, 2022

@zirain You should not need well-known names any more. I think there are a couple of historical cases where the filter name matters, but in general you can use whatever name you want (something that's meaningful to your control plane) and Envoy selects the filter based on the type of the config protobuf.

@shashankram
Copy link

@zirain You should not need well-known names any more. I think there are a couple of historical cases where the filter name matters, but in general you can use whatever name you want (something that's meaningful to your control plane) and Envoy selects the filter based on the type of the config protobuf.

@jpeach since when was this change introduced in Envoy? Previously, we could simply specify the filter name without the typed config spec in the Go types, so curious.

@sunjayBhatia
Copy link
Member

I believe Envoy 1.22: https://www.envoyproxy.io/docs/envoy/v1.22.2/version_history/v1.22.0#deprecated

@shashankram
Copy link

I believe Envoy 1.22: https://www.envoyproxy.io/docs/envoy/v1.22.2/version_history/v1.22.0#deprecated

Thanks @sunjayBhatia. Is there a place where we can see the list of filters and their corresponding types?
For e.g., the http router filter uses the type type.googleapis.com/envoy.extensions.filters.http.router.v3.Router. I'd like to see the types for other filters as well, such as HTTP RBAC.

Is it also safe to discontinue the use of the wellknown names for filters prior to v1.22?

@sunjayBhatia
Copy link
Member

for xDS v3 and above I think all the type urls match well to package names so you can follow those to find the types

e.g. this page has all the http filters and I usually use the type url/package to correlate to get the go package from this repo: https://www.envoyproxy.io/docs/envoy/v1.22.2/api-v3/config/filter/http/http

@sunjayBhatia
Copy link
Member

release notes imply the filter name usage was "long deprecated" but I haven't found what release that was started in, I think it has generally been the case you can/should use the types for quite a while if not forever

@shashankram
Copy link

Do the non-http filters (e.g. tcp_proxy) still rely on the wellknown names?

@sunjayBhatia
Copy link
Member

you should use the type in the config for non HTTP filters as well, rather than solely the name

@jpeach
Copy link
Contributor

jpeach commented Jun 17, 2022

@jpeach since when was this change introduced in Envoy?

I guess some time prior to 2020, maybe 1.12-ish?

xref #293

@shashankram
Copy link

you should use the type in the config for non HTTP filters as well, rather than solely the name

But does it matter what name you use for non-HTTP filters? For e.g., do we need to use wellknown.TCPProxy for the tcp_proxy filter or can it be any name?

I came across envoyproxy/envoy@8cb6862 which seems to only affect extension filters, so wasn't sure.

@shashankram
Copy link

you should use the type in the config for non HTTP filters as well, rather than solely the name

But does it matter what name you use for non-HTTP filters? For e.g., do we need to use wellknown.TCPProxy for the tcp_proxy filter or can it be any name?

I came across envoyproxy/envoy@8cb6862 which seems to only affect extension filters, so wasn't sure.

For e.g., the Envoy doc suggests to use the filter name for the OriginalDestination ListenerFilter:
This filter should be configured with the name envoy.filters.listener.original_dst.

But does the name actually matter?

@jpeach, @sunjayBhatia

@sunjayBhatia
Copy link
Member

no, the name doesn't really matter, it is used just for annotation, just verified by changing the httpconnectionmanager filter with a non-type url name and everything is fine

@shashankram
Copy link

no, the name doesn't really matter, it is used just for annotation, just verified by changing the httpconnectionmanager filter with a non-type url name and everything is fine

Thanks for clarifying. I guess it makes sense to document these clearly in the Envoy docs. I'll open an issue there.

@shashankram
Copy link

@sunjayBhatia it seems like the name still matters for some filters, see envoyproxy/envoy#21759 (comment)

@sunjayBhatia
Copy link
Member

ah, well that is a shame it hasn't been fixed everywhere, good find @shashankram

shashankram added a commit to shashankram/osm that referenced this issue Jun 21, 2022
- Updates Envoy to its latest available version
  (v1.22.2 for Linux, v1.22.1 for Windows).
  The latest version includes the latest released
  security fix.
  We could not update Envoy previously due to a blocking bug:
  envoyproxy/envoy#20113

- Updates filter names to custom names as wellknown
  names are deprecated in Envoy (with 1 exception
  for the http.rbac filter). Envoy will use the
  TypeURL in the proto to determine which filter
  to use instead. Wellknown names are not required
  and using them is confusing because not all filters
  are defined in the legacy wellknown pkg (e.g.
  http.local_ratelimit).
  See:
  envoyproxy/envoy#21759
  envoyproxy/envoy#21763
  envoyproxy/go-control-plane#293
  envoyproxy/go-control-plane#552

- Uses the distroless image as the alpine image has been
  discontinued: envoyproxy/envoy#21758

- Updates tests to use custom filter names

- Adds `proto_types.go` to aid dynamic proto resolution
  for typed configs using `any.Any()`. This helps resolve
  protos where dynamic resolution is necessary.

- Updated Prometheus' ConfigMap to reflect changes to
  Envoy metrics prefixes

Signed-off-by: Shashank Ram <[email protected]>
shashankram added a commit to openservicemesh/osm that referenced this issue Jun 22, 2022
- Updates Envoy to its latest available version
  (v1.22.2 for Linux, v1.22.1 for Windows).
  The latest version includes the latest released
  security fix.
  We could not update Envoy previously due to a blocking bug:
  envoyproxy/envoy#20113

- Updates filter names to custom names as wellknown
  names are deprecated in Envoy (with 1 exception
  for the http.rbac filter). Envoy will use the
  TypeURL in the proto to determine which filter
  to use instead. Wellknown names are not required
  and using them is confusing because not all filters
  are defined in the legacy wellknown pkg (e.g.
  http.local_ratelimit).
  See:
  envoyproxy/envoy#21759
  envoyproxy/envoy#21763
  envoyproxy/go-control-plane#293
  envoyproxy/go-control-plane#552

- Uses the distroless image as the alpine image has been
  discontinued: envoyproxy/envoy#21758

- Updates tests to use custom filter names

- Adds `proto_types.go` to aid dynamic proto resolution
  for typed configs using `any.Any()`. This helps resolve
  protos where dynamic resolution is necessary.

- Updated Prometheus' ConfigMap to reflect changes to
  Envoy metrics prefixes

Signed-off-by: Shashank Ram <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants