allow typed_per_filter_config to refer to "custom" HTTP filter names

[mpuncel]

Use case: We'd like to use multiple ext_authz filters configured with different gRPC servers, and would like to be able to toggle them separately via route config. This means we can't just use the canonical filter name, but need to be able to refer to custom names.

#9618 made Envoy able to infer most extension types from the type URL of the config, which frees up users to use the name field to make filter names more descriptive (particularly useful for distinguishing multiple instances of the same filter).

It looks like typed_per_filter_config doesn't yet have this capability and requires that the canonical filter name is used.

I'm not super familiar with the code or filter registration system, but it seems like this could be done by either:

1. keeping a map of filter name to type, and looking up the supplied name in that map to figure out the filter config type
2. use the type URL of the config to infer the filter type
3. both (with a precedence order for applying 1 and 2).

Probably just 1) makes sense? that would ensure that there can't be multiple configs that map to the same filter.

It also might require a config option somewhere backwards compatible? There could be a working config today where a filter has a custom name but the typed_per_filter_config is using the old name.

[lizan]

To be consistent with #9618, I think 2 is the way to go, if the config isn't typed (i.e. google.protobuf.Struct), then it should be inferred from name.

[stale]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or other activity occurs. Thank you for your contributions.

[mpuncel]

Looking at this more closely, I think I misunderstood what needs to happen in my original comment.

It looks like the issue is that the individual HTTP filters (ext_authz used here as an example) get their per route config like this;

  const auto* specific_per_route_config =
      Http::Utility::resolveMostSpecificPerFilterConfig<FilterConfigPerRoute>(
          HttpFilterNames::get().ExtAuthorization, route);

It's using the well-known name there. Knowing this, I think the solution should be to make each filter know its user-specified name from the http_filters list in HCM. Then the filter can try to resolve config using this name instead.

I'm not sure how to handle backwards compatibility for this, since users may have started using user supplied filter names but continue to rely on per route config using the well known filter names, I'm open to suggestions. I'd love to do something like "try to resolve with the user supplied name, and if that is a miss then use the canonical name", but I think that could have strange behavior because for some routes you might hit on one name, and on other routes you might hit on the other. That said, users migrating can probably be aware of this potential behavior and rename all of the typed_per_filter_config keys to avoid this scenario.

Another option could be a new field in either HTTP connection manager or HttpFilter like use_supplied_name_for_per_route_config_resolution: true?

[htuch]

Agree with the above, we should use the opaque filter name in the HTTP filter chain as the key to join with the per-route overrides. I think this makes most sense. @fuqianggao @markdroth

[fuqianggao]

+1. In this case, should Envoy reject a config that has filters with the same filter name and filter type?

[htuch]

I think that would make sense, but it could break a bunch of existing users (maybe?). Would be worth adding a check and runtime enabling for rollout.

[markdroth]

To be clear, I think it should be reject duplicate filter names, not duplicate types -- it's totally fine to have two instances of the same filter under different names.

[markdroth]

Note that when validating the RDS response, we won't necessarily know which LDS response to use to validate the filter instance names, since there could be a new LDS response that has recently been received but has not yet been swapped in. So we probably won't be able to validate that the names in the per-route overrides actually exist in the HCM filter list, but we can still validate that the protobuf types in the values of the typed_per_filter_config fields do map to known filter types.

[markdroth]

Since gRPC will be adding support for HTTP filters in the near future, it would be good to get this implemented in Envoy, so that the semantics are the same between the two xDS clients.

[htuch]

Bumped to priority/high.

[kvn-esque]

Hi, just wondering if this is still high priority, and if someone could comment on the ballpark timeline.

[htuch]

@kev1n11 yes, still priority, PRs welcome!

[aaddati]

Hi @mpuncel! Finally, could you find some workaround? I have a similar case. Thanks! :)

[markdroth]

Note that gRPC has already implemented the semantic described here, and it does not support the old canonical filter names at all, so whatever we do for backward compatibility, we should not break that.

[wbpcode]

cc @mattklein123 I am thinking something like your suggesions.

May be we can add a extenable filed route_config_key_generator or something. It will take the config name, canonical filter name and request headers as the input. A string view key as the ouput. It's will return canonical filter name by default. Then it will not break anything.

Is this a possible solution?

[wbpcode]

cc @markdroth Does the gRPC introduced new API for the semantic?

[markdroth]

No, gRPC has no new API for this. The typed_per_filter_config key is always the custom name from the HttpFilter.name field in the HCM's list of filters.

I suspect the easiest path for backward compatibility here will be to first try to find a custom name match and then fall back to using the old canonical names.

[wbpcode]

🤔 IMO, l prefer explicit configuration than default compatibility.

[markdroth]

The default compatibility approach is essentially the same thing that was done in #9618 to change the meaning of the existing filter name field in the HCM config. If it worked for that, I'm not sure why it wouldn't work here as well.

I really don't think it makes sense to require gRPC to support a new field when we already have the desired functionality here. I think Envoy just needs to change its behavior to work the same way.

[mattklein123]

As long as we don't break existing users I'm OK with a fallback approach. If we are going to break existing users we will have to add some type of new config. There are too many people that would be effected by a breaking change here.

[markdroth]

Yeah, agreed. I don't think this should be a breaking change.

Unless I'm missing something, the only case in which this would be a breaking change is if there is a conflict where someone is using one of the canonical names as the custom name for a different filter impl (e.g., using "envoy.filters.http.ext_authz" as the name for a fault injection filter instead of an ext_authz filter), which seems exceedingly unlikely. Such a config would not have worked at all with Envoy prior to #9618, and I'm not sure why anyone would have switched to that config since then.

If we do need an explicit way to make this configurable, though, then I think we can do it in a way that doesn't break the existing behavior for gRPC. We can use the same approach that was introduced in #14982: we can wrap the value of the typed_per_extension_config entry in a FilterConfig proto, and we can add a new bool key_is_custom_name field to the FilterConfig proto. gRPC will not add support for this field, because we always use the custom names, but Envoy can look at that field to decide what name to match on.

[wbpcode]

I'm not sure why it wouldn't work here as well.

It works. Explicit configuration just my personal preference.

I hope the behavior can be controlled by the users and provides some extendibility here.

Any way, we can make the fallback a default behavior to reduce work of grpc.