It would be hard that let every filter handle the filter name/custom name key searching by itself.

IMO, it would be better that using Http::StreamFilterCallbacks as a proxy and provides filters a method by the Http::StreamFilterCallbacks to get route config.
The filters need to use the Http::StreamFilterCallbacks's method rather than the method provided by the Route.

It means that the HTTP filter should get route config by the way as following code.

  // Http::StreamDecoderFilter
  Http::FilterHeadersStatus decodeHeaders(Http::RequestHeaderMap&, bool) override {
    auto route_config = decode_callback_->mostSpecificPerFilterConfig();
    return Http::FilterHeadersStatus::Continue;
  }

How?

First, we need make the custom name/filter name available for the Http::StreamFilterCallbacks. We can add new method to Http::StreamFilterBase to provide these name.

class StreamFilterBase {
public:
  /**
   * Get custom filter name of current filter instance. This name typically should be the
   * name of HttpFilter proto configuration.
   */
  virtual absl::string_view customName() const { return {}; }

  /**
   * Get filter name of current filter instance.
   */
  virtual absl::string_view filterName() const { return {}; }
};

Second, we can add method mostSpecificPerFilterConfig and traversePerFilterConfig to the Http::StreamFilterCallbacks.

class StreamFilterCallbacks {
  /**
   * This is a helper to get the route's per-filter config if it exists, otherwise the virtual
   * host's. Or nullptr if none of them exist.
   */
  virtual const RouteSpecificFilterConfig* mostSpecificPerFilterConfig() const PURE;

  /**
   * Fold all the available per route filter configs, invoking the callback with each config (if
   * it is present). Iteration of the configs is in order of specificity. That means that the
   * callback will be called first for a config on a Virtual host, then a route, and finally a route
   * entry (weighted cluster). If a config is not present, the callback will not be invoked.
   */
  virtual void traversePerFilterConfig(std::function<void(const Router::RouteSpecificFilterConfig&)> cb) const PURE;
};

Third, update FilterFactoryCb to provide custom filter name. Then when we create the filter instance, we can set the custom name to the filter.

/**
 * This function is used to wrap the creation of an HTTP filter chain for new streams as they
 * come in. Filter factories create the function at configuration initialization time, and then
 * they are used at runtime.
 * @param custom_name custom name of the filter that created by this factory.
 * @param callbacks supplies the callbacks for the stream to install filters to. Typically the
 * function will install a single filter, but it's technically possibly to install more than one
 * if desired.
 */
using FilterFactoryCb = std::function<void(absl::string_view custom_name, FilterChainFactoryCallbacks& callbacks)>;

Http::FilterFactoryCb FilterFactory::createFilterFactoryFromProtoTyped(const FakePb&, const std::string& stats_prefix,
    Server::Configuration::FactoryContext& context) {
  return [config](absl::string_view custom_name, Http::FilterChainFactoryCallbacks& callbacks) -> void {
    auto fitler = std::make_shared<CorsFilter>(config);
    filter->setNames(custom_name, name());
    callbacks.addStreamFilter(std::move(filter));
  };
}

Fouth, upate every filter to use new mostSpecificPerFilterConfig and traversePerFilterConfig provided by the Http::StreamFilterCallbacks.

cc @mattklein123 Could you take a quick look to this simple proposal?

Looks reasonable to me. One question I have is whether we have to expose the custom name method. The filter manager should have that info, and only share it with the callbacks implementation, without adding another public method.

Looks reasonable to me. One question I have is whether we have to expose the custom name method. The filter manager should have that info, and only share it with the callbacks implementation, without adding another public method.

Thanks for you comment. But in the current implementation, the filter instance is added to filter chain by the callback as following. So when we create the filter chain, it's hard to make the custom name available directly without change the callback or add new another public method.

class FilterChainFactoryCallbacks {
  virtual void addStreamDecoderFilter(Http::StreamDecoderFilterSharedPtr filter) PURE;
};

So when we create the filter chain, it's hard to make the custom name available directly without change the callback or add new another public method.

As long as you are working in this area, can you please fix all debug lines like this one:

To print out the name instead of the pointer address of the filter? This will make debugging multiple filters so much easier. I actually started to do this at one point but gave up because it would require a new public method which I think this solves.

But since we all agree that the current configuration is too heavy, I will respect the community's opinion.

👌 I will update all these log line.

This is outdated.

Inherited relationships before this PR of HTTP stream filter.

graph TD
  BA(StreamFilterBase)
  SD(StreamDecoderFilter)
  SE(StreamEncoderFilter)
  SF(StreamFilter)
  PD(PassthroughDecoderFilter)
  PE(PassthroughEncoderFilter)
  PF(PassthroughFilter)

  BA --> SD
  BA --> SE
  SD --virtual--> PD
  SD --virtual--> SF
  SE --virtual--> SF
  SE --virtual--> PE
  PD --> PF
  SF --> PF
  PE --> PF

Inherited relationships after this PR of HTTP stream filter:

graph TD
  BA(StreamFilterBase)
  SD(StreamDecoderFilter)
  SE(StreamEncoderFilter)
  SF(StreamFilter)
  PD(PassthroughDecoderFilter)
  PE(PassthroughEncoderFilter)
  PF(PassthroughFilter)

  BA --virtual--> SD
  BA --virtual--> SE
  SD --> PD
  SD --> SF
  SE --> SF
  SE --> PE
  SF --> PF

I like this way. Just one more comment about whether we can put the all the name to the FilterConfigProvider. Then we needn't that NamedHttpFilterFactoryCb wrapper.

It's ok for me. But I am just not sure if we need to expose this canonical name for every type extension. Until for now, the canonical name is only used by the HTTP filters to get route config. And we keep it for the backwards compatibility. So I am not sure if it's worth it to update the APIs of ExtensionConfigProvider.

But I'm not against this either. I can update it if the community wishes to do so.

cc @mattklein123 cc @soulxu

I like this way. Just one more comment about whether we can put the all the name to the FilterConfigProvider. Then we needn't that NamedHttpFilterFactoryCb wrapper.

It's ok for me. But I am just not sure if we need to expose this canonical name for every type extension. Until for now, the canonical name is only used by the HTTP filters to get route config. And we keep it for the backwards compatibility. So I am not sure if it's worth it to update the APIs of ExtensionConfigProvider.

Since I saw the FilterConfigProvider already holds the custom name,https://github.com/envoyproxy/envoy/pull/21525/files#diff-9ffd7a166e0bd6afee8cbe95c75544a26060e9d2bc814ba3309fa4c04d8860ecR738
so I thought the FilterConfigProvider can hold the canonical name also

I like this way. Just one more comment about whether we can put the all the name to the FilterConfigProvider. Then we needn't that NamedHttpFilterFactoryCb wrapper.

It's ok for me. But I am just not sure if we need to expose this canonical name for every type extension. Until for now, the canonical name is only used by the HTTP filters to get route config. And we keep it for the backwards compatibility. So I am not sure if it's worth it to update the APIs of ExtensionConfigProvider.

Since I saw the FilterConfigProvider already holds the custom name,https://github.com/envoyproxy/envoy/pull/21525/files#diff-9ffd7a166e0bd6afee8cbe95c75544a26060e9d2bc814ba3309fa4c04d8860ecR738 so I thought the FilterConfigProvider can hold the canonical name also

custom name is used by the ECDS, but canonical name is unnecessary for most of extention config providers.

Any way, it ok for me to udpate it or not. So waiting for some more suggessions from matt.

/retest

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

/retest

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

/retest

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit