-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
access log: change the factory context of log filter to generic one #34157
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: wbpcode <wbphub@live.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1: will need listenerInfo for our-use case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
FactoryContext
requirement. (We useFactoryContext
in 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.