access log: change the factory context of log filter to generic one #34157
Conversation
Signed-off-by: wbpcode <wbphub@live.com>
There was a problem hiding this comment.
If I understand this correctly, it narrows the scope of FactoryContext callbacks available for access logs, in exchange for being able to create access logs from other locations (i.e. ones that don't have listener scope)
Looks like the functions "lost" are listenerScope, getTransportSocketFactoryContext, drainDecision and listenerInfo.
So on the whole I'd find that a fine trade-off except we don't know what downstream users are using. I did a quick peek of internal code and I think we actually use some of these fields internally and narrowing the scope would break our loggers.
cc @jmarantz to sanity check as it's his product's code
/wait-any
| @@ -54,7 +54,7 @@ bool ComparisonFilter::compareAgainstValue(uint64_t lhs) const { | |||
| } | |||
|
|
|||
| FilterPtr FilterFactory::fromProto(const envoy::config::accesslog::v3::AccessLogFilter& config, | |||
| Server::Configuration::FactoryContext& context) { | |||
| Server::Configuration::GenericFactoryContext& context) { | |||
There was a problem hiding this comment.
+1: will need listenerInfo for our-use case.
There was a problem hiding this comment.
Before #31012, how your code handle this case? At that time, we only have CommonFactoryContext for log filters.
(But I have no opinion to this PR, it completely up to you. :)
Commit Message: access log: change the factory context of log filter to generic one
Additional Description:
See slack discussion: https://envoyproxy.slack.com/archives/C78HA81DH/p1715658162300489
NOTE: I am not sure is that OK to merge this to main. It's harmless (or senseless?) to the community extensions because all community log filters have no
FactoryContextrequirement. (We useFactoryContextin previous implementation just for simplication.)This change could let the third party developers to create log filters with only
ServerFactoryContext. But will affect the third party developers who create a custom log filter which requireFactoryContext. But considering the fact that before #31012, onlyCommonFactoryContext(it was removed) is provided to the log factory. So I guess this change is OK? Anyway, let the community to make the decision.Risk Level: low.
Testing: n/a.
Docs Changes: n/a.
Release Notes: n/a.
Platform Specific Features: n/a.