Clearly document whether filter names matter

[shashankram]

It seems like Envoy does not care about the filter name, but uses the TypeUrl in the config to determine which filter to use.

Relevant commit:

commit 8cb6862fe6099cd8583a64ff037ecdeaf0e939fa
Author: Kuat <kyessenov@users.noreply.github.com>
Date:   Thu Apr 7 06:31:49 2022 -0700

    extensions: strongly prefer type URL lookup (#20397)

Also discussions in the following issues seem to indicate the same:
envoyproxy/go-control-plane#552
envoyproxy/go-control-plane#293

However, there still exists confusion based on the wording in the docs.

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.

This contradicts statements about the filter name not being relevant.

Kindly clarify in the docs whether filter names are relevant or not.

[phlax]

cc @kyessenov

@shashankram my understanding is that this is kinda slowly transitioning and the docs probably need to catch up

i would be happy to follow up or review PRs - i think the issue you raise is important

[kyessenov]

Yes, we need to make a thorough pass to remove the clauses saying "this filter should be configured with name XXX". I want to get #21707 first since it's really important to also say what type URL to use, and that was the reason.

[shashankram]

Yes, we need to make a thorough pass to remove the clauses saying "this filter should be configured with name XXX". I want to get #21707 first since it's really important to also say what type URL to use, and that was the reason.

That's a super useful change, as I have also been wondering if there is a doc for the exact same purpose.

[shashankram]

@phlax @kyessenov so starting v1.22, is it safe to assume filter names are irrelevant? And is this the case for all filters or just extensions?

[kyessenov]

@shashankram The fallback to use type_url was there for a very long time. The only concern is when they are not matching, in which case, v1.22 will use type_url instead of name (and ignore name) as the primary lookup.

[kyessenov]

This works for all typed extensions which is almost everything in Envoy except few exceptions (mostly internal to Envoy and not user facing).

[shashankram]

This works for all typed extensions which is almost everything in Envoy except few exceptions (mostly internal to Envoy and not user facing).

What about types that are not under the extensions category? Do the filters corresponding to the names defined in https://github.com/envoyproxy/go-control-plane/blob/main/pkg/wellknown/wellknown.go need to use a well defined name?

Please document for every filter, regardless of whether is an extension or not, if it needs to be defined with a specific name.

[kyessenov]

Every filter is an extension, and every extension that has a protobuf type is uniquely identified by that protobuf type. The name can be anything as long as you use protobuf type for typed_config. Does that make sense?

[shashankram]

Every filter is an extension, and every extension that has a protobuf type is uniquely identified by that protobuf type. The name can be anything as long as you use protobuf type for typed_config. Does that make sense?

That makes sense, thanks.

[shashankram]

@kyessenov, also in the Envoy config dump I see references to wellknown names in the extensions field.

{
 "configs": [
  {
   "@type": "type.googleapis.com/envoy.admin.v3.BootstrapConfigDump",
   "bootstrap": {
    "node": {
     "id": "79a1995c-1a05-4fba-88be-7e096963b656.sidecar.curl.curl.cluster.local",
     "hidden_envoy_deprecated_build_version": "a17cdcdfad24de101e95716b77549ba689824f25/1.19.3-dev/Clean/RELEASE/BoringSSL",
     "user_agent_name": "envoy",
     "user_agent_build_version": {
      "version": {
       "major_number": 1,
       "minor_number": 19,
       "patch": 3
      },
      "metadata": {
       "build.type": "RELEASE",
       "revision.status": "Clean",
       "build.label": "dev",
       "revision.sha": "a17cdcdfad24de101e95716b77549ba689824f25",
       "ssl.version": "BoringSSL"
      }
     },
     "extensions": [
      {
       "name": "envoy.transport_sockets.alts",
       "category": "envoy.transport_sockets.upstream"
      },
      {
       "name": "envoy.transport_sockets.quic",
       "category": "envoy.transport_sockets.upstream"
      },
      {
       "name": "envoy.transport_sockets.raw_buffer",
       "category": "envoy.transport_sockets.upstream"
      },

Do the names here not matter or are these fallback names?

[shashankram]

@kyessenov @phlax, I tried an experiment where I changed the HTTP RBACPerRoute filter name from envoy.filters.http.rbac to http_rbac_per_route.

I started seeing XDS error logs:

Error adding/updating listener(s) outbound-listener: Didn't find a registered implementation for name: 'http_rbac_per_route'\\ninbound-listener: Didn't find a registered implementation for name: 'http_rbac_per_route'\\n\"\" for nonce 1655500461591137800, last version applied on request 

When I changed the filter name back to the wellnown name envoy.filters.http.rbac, the error went away. I do have a the TypeUrl configured via the Go control plane as:

Name: "envoy.filters.http.rbac",
ConfigType: &xds_hcm.HttpFilter_TypedConfig{
	TypedConfig: &any.Any{
		TypeUrl: "type.googleapis.com/envoy.extensions.filters.http.rbac.v3.RBACPerRoute",
	},
},

So it seems like a wellknown name is still required for certain filters?

[shashankram]

@kyessenov @phlax, I tried an experiment where I changed the HTTP RBACPerRoute filter name from envoy.filters.http.rbac to http_rbac_per_route.

I started seeing XDS error logs:

Error adding/updating listener(s) outbound-listener: Didn't find a registered implementation for name: 'http_rbac_per_route'\\ninbound-listener: Didn't find a registered implementation for name: 'http_rbac_per_route'\\n\"\" for nonce 1655500461591137800, last version applied on request 

When I changed the filter name back to the wellnown name envoy.filters.http.rbac, the error went away. I do have a the TypeUrl configured via the Go control plane as:

Name: "envoy.filters.http.rbac",
ConfigType: &xds_hcm.HttpFilter_TypedConfig{
	TypedConfig: &any.Any{
		TypeUrl: "type.googleapis.com/envoy.extensions.filters.http.rbac.v3.RBACPerRoute",
	},
},

So it seems like a wellknown name is still required for certain filters?

@phlax @kyessenov could you comment whether the RBACPerRoute filter needs to be configured with the filter name envoy.filters.http.rbac in the typed_per_filter_config and http_filter list on the listener? From my testing, specifying a random name here does not work.

[kyessenov]

@shashankram Yes, you are right that we use qualified names for per_filter_config at the moment. That's changing per #12274, but it'll take an effort to transition.

[kyessenov]

@shashankram Actually, your comment is about something else. Per route config is meant to be placed in per_filter_config in routes and virtual hosts. The filter config at the listener level should not be per route config.

[shashankram]

@shashankram Yes, you are right that we use qualified names for per_filter_config at the moment. That's changing per #12274, but it'll take an effort to transition.

@kyessenov this clarifies my concern.

I am referring to the typed_per_filter_config field on the virtual_host and route protos, where a map[name]*any.Any needs to be configured. It looks like qualified names are still necessary here as you highlighted.

I am unsure what you mean in #21759 (comment), but I was conveying that the filter name specified in the typed_per_filter_config for HTTP filters must use a qualified name, while also reference the same qualified name in the http_filters list in the http_connection_manager filter.