config: Consolidate factory category names into common header file#9425
config: Consolidate factory category names into common header file#9425yanavlasov wants to merge 1 commit intoenvoyproxy:masterfrom
Conversation
Prerequisite for PR envoyproxy#9301 Signed-off-by: Yan Avlasov <yavlasov@google.com>
| // Dubbo extension | ||
| const std::string DubboProxyFilters = "dubbo_proxy.filters"; | ||
| const std::string DubboProxyProtocols = "dubbo_proxy.protocols"; | ||
| const std::string DubboProxyRouteMatchers = "dubbo_proxy.route_matchers"; | ||
| const std::string DubboProxySerializers = "dubbo_proxy.serializers"; | ||
|
|
||
| // Thrift extension | ||
| const std::string ThriftProxyFilters = "thrift_proxy.filters"; | ||
| const std::string ThriftProxyProtocols = "thrift_proxy.protocols"; | ||
| const std::string ThriftProxyTransports = "thrift_proxy.transports"; | ||
|
|
||
| // WASM | ||
| const std::string WasmNullVm = "wasm.null_vms"; |
There was a problem hiding this comment.
drive by: is it possible to not include these here as these are only used in extensions? Can they be defined in the extensions themselves?
There was a problem hiding this comment.
The ultimate goal was to have a single point of reference to point folks at for the category field in #9301. That said, this could be done via generated docs or something like that.
What if we define a WasmFactoryPrefix here (same for Dubbo and Thrift), then define the factories inside the extensions?
There was a problem hiding this comment.
In general I think we should try to never reference extensions in the core code, otherwise we have an abstraction leak that is going to bite us eventually. Can we at least spec out the "right" solution here and TODO/issue it?
There was a problem hiding this comment.
I'm still trying to wrap my head around of various changes around this area, so bear with me, please.
Taking a step back it is unclear to me why extensions have this two tiered naming convention. I s the category name used anywhere? It seems to be rather haphazardly named to be machine consumable and configurations use filter names.
It also seems that in V3 we are moving toward using the type URL of configuration proto to uniquely identify the filter.
Would it work if we finish #9391 to have factories report their config proto type URL and add that to the extension list reported by the Node?
There was a problem hiding this comment.
I think for purely documentation purposes I have a simpler solution that would need to be rolled back later. Let me try that.
There was a problem hiding this comment.
The only purpose of the category (AFAIK) is for printing the extensions on startup:
Lines 253 to 256 in 80e649c
It would be really optimal if we could hold the line here and not let extensions leak into the source code any more than they already do (I think there are a few cases which we need to kill).
I don't know the background of this change though, if you think this is required can you give me more context?
There was a problem hiding this comment.
I believe @yanavlasov is going to switch to using generated RST here, so let's close this out for now.
|
Feel free to reopen if we want to do some subset of this PR, thanks. |
Prerequisite for PR #9301
Part of fixing #8332
Risk Level: Low
Testing: Unit Tests
Docs Changes: N/A
Release Notes: N/A
Signed-off-by: Yan Avlasov yavlasov@google.com