-
Notifications
You must be signed in to change notification settings - Fork 5.3k
router: new methods for stream filter callback to get route per filter config #21525
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
Changes from all commits
1d14813
9a93e04
7e9af36
f7bf240
aed108a
a5b8da3
7064a02
834002f
99bd886
ab1dfb5
18000df
14def65
eff8e67
2d23437
fc2d687
76fb97a
af2b15a
ac8adf2
d831041
d19f087
95a9870
0e90132
d3ce0d9
68df359
05919dc
58ad9dc
c6cfaf7
0239eff
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -164,9 +164,17 @@ class DynamicFilterConfigProviderImpl : public DynamicFilterConfigProviderImplBa | |||||||||||||||||||||||||
| ThreadLocal::TypedSlot<ThreadLocalConfig> tls_; | ||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // Struct of canonical filter name and HTTP stream filter factory callback. | ||||||||||||||||||||||||||
| struct NamedHttpFilterFactoryCb { | ||||||||||||||||||||||||||
| // Canonical filter name. | ||||||||||||||||||||||||||
| std::string name; | ||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it possible to put this name to the FilterConfigProvider also?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 to merge this in some way. I'm not familiar with this code but it seems like there is a bunch of duplication with the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have just tried to add a method to expose this name in the Because it's supported to used different type filters by the ECDS, so the canonical filter name of current provider may be changed when the filter config is updateing by the ECDS. We must make the canonical filter name thread local and update it just as we update factory function in the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO, it would be better to add some restrictions here. For example, only multiple version APIs of same type filter can be used here.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I guess even though there can be multiple extension type URLs, they should pointed to the same factory, or they should point to the same canonical filter name? envoy/source/extensions/filters/network/http_connection_manager/config.cc Lines 674 to 682 in 154a7a8
Then you can get envoy/source/extensions/filters/network/http_connection_manager/config.cc Lines 684 to 686 in 154a7a8
@adisuissa probably knows the usecase of multiple extension type urls.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It seems that there is no any restriction for now. It would be great if we can add this restriction. But it would be a break change.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea, it seems like this is the best we have now. Thanks!
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kyessenov Can probably shed some light on multiple extensions type-urls |
||||||||||||||||||||||||||
| // Factory function used to create filter instances. | ||||||||||||||||||||||||||
| Http::FilterFactoryCb factory_cb; | ||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // Implementation of a HTTP dynamic filter config provider. | ||||||||||||||||||||||||||
| class HttpDynamicFilterConfigProviderImpl | ||||||||||||||||||||||||||
| : public DynamicFilterConfigProviderImpl<Http::FilterFactoryCb> { | ||||||||||||||||||||||||||
| : public DynamicFilterConfigProviderImpl<NamedHttpFilterFactoryCb> { | ||||||||||||||||||||||||||
| public: | ||||||||||||||||||||||||||
| HttpDynamicFilterConfigProviderImpl( | ||||||||||||||||||||||||||
| FilterConfigSubscriptionSharedPtr& subscription, | ||||||||||||||||||||||||||
|
|
@@ -190,10 +198,12 @@ class HttpDynamicFilterConfigProviderImpl | |||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| private: | ||||||||||||||||||||||||||
| Http::FilterFactoryCb instantiateFilterFactory(const Protobuf::Message& message) const override { | ||||||||||||||||||||||||||
| NamedHttpFilterFactoryCb | ||||||||||||||||||||||||||
| instantiateFilterFactory(const Protobuf::Message& message) const override { | ||||||||||||||||||||||||||
| auto* factory = Registry::FactoryRegistry<Server::Configuration::NamedHttpFilterConfigFactory>:: | ||||||||||||||||||||||||||
| getFactoryByType(message.GetTypeName()); | ||||||||||||||||||||||||||
| return factory->createFilterFactoryFromProto(message, getStatPrefix(), factory_context_); | ||||||||||||||||||||||||||
| return {factory->name(), | ||||||||||||||||||||||||||
| factory->createFilterFactoryFromProto(message, getStatPrefix(), factory_context_)}; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| Server::Configuration::FactoryContext& factory_context_; | ||||||||||||||||||||||||||
|
|
@@ -495,7 +505,7 @@ class FilterConfigProviderManagerImpl : public FilterConfigProviderManagerImplBa | |||||||||||||||||||||||||
| // HTTP filter | ||||||||||||||||||||||||||
| class HttpFilterConfigProviderManagerImpl | ||||||||||||||||||||||||||
| : public FilterConfigProviderManagerImpl< | ||||||||||||||||||||||||||
| Server::Configuration::NamedHttpFilterConfigFactory, Http::FilterFactoryCb, | ||||||||||||||||||||||||||
| Server::Configuration::NamedHttpFilterConfigFactory, NamedHttpFilterFactoryCb, | ||||||||||||||||||||||||||
| Server::Configuration::FactoryContext, HttpDynamicFilterConfigProviderImpl> { | ||||||||||||||||||||||||||
| public: | ||||||||||||||||||||||||||
| absl::string_view statPrefix() const override { return "http_filter."; } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.