-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Listener ECDS: Adding config provider manager implemantion for listener filter #20560
Listener ECDS: Adding config provider manager implemantion for listener filter #20560
Conversation
…ion for Listener filter. This is the 3rd PR to support listener ECDS. It includes: 1) The class definition of ListenerFilterConfigProviderManagerImpl. 2) Moving getMessage() method to the base class to avoid duplicated code. 3) Unit test cases for ListenerFilterConfigProviderManagerImpl. Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
/assign @adisuissa @kyessenov @yanavlasov |
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
Signed-off-by: Yanjun Xiang <yanjunxiang@google.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.
Thanks for working on this, mostly small things.
For something like this, an xDS integration test would be useful. There is one for HTTP, so it can be copied and adapted fairly easy. |
Yes, that's planned. This PR just added a ListenerFilterConfigManagerImpl definition. It's not used any where. In the planned next PR, I will be creating an object of it in class ListenerManagerImpl, and using it to create listener filter factories, and build listener filter chain. In that PR, I will be extending the current test/integration/extension_discovery_integration_test.cc to test listener ECDS functionality end-to-end. |
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
…listener Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
/retest |
Retrying Azure Pipelines: |
/assign @htuch |
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
@adisuissa @kyessenov PTAL |
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
Signed-off-by: Yanjun Xiang <yanjunxiang@google.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.
Thanks, overall LGTM.
@soulxu can you take another look.
@@ -29,6 +29,13 @@ class FilterConfigSubscription; | |||
|
|||
using FilterConfigSubscriptionSharedPtr = std::shared_ptr<FilterConfigSubscription>; | |||
|
|||
// These helper functions throw exceptions. Thus can not be defined in .h files. | |||
void validateProtoConfigDefaultFactoryHelper(const bool null_default_factory, |
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.
Try having functions which are scoped inside a class or are in an empty namespace.
The helper functions should probably be either private or protected functions in one of the classes below.
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.
ACK
overall LGTM also! except for the listener filter matcher #20560 (comment) |
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
@yanjunxiang-google thanks! that is perfect. Then this is LGTM now. |
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.
Thanks, LGTM modulo test nits.
@htuch can you PTAL?
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
@htuch @kyessenov PTAL |
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.
LGTM, left a small nit.
Config::Utility::getFactoryByType<Server::Configuration::NamedHttpFilterConfigFactory>( | ||
proto_config); | ||
if (default_factory == nullptr) { | ||
void FilterConfigProviderManagerImplBase::validateProtoConfigDefaultFactory( |
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.
IIUC both this method and validateProtoConfigTypeUrl
are only called once.
Is there a plan to call them in other places in later PRs? If not, then we should just inline their contents in the call place.
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.
Thanks for the comments.
This method throw exception. Envoy CI scripts doesn't allow throw exception in .h files.
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.
LGTM. I think @kyessenov can do final pass/merge (now as a maintainer). Generally I'd also ask for a non-Googler, but this is mostly an internal change and consistent with previous work on ECDS, so won't insist on that unless someone feels strongly in favor.
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.
LGTM, thanks!
Matt volunteered to do a final pass, once @kyessenov LGTM's this PR. |
/assign @mattklein123 |
neither of Thanks, for, help, reviewing! can be assigned to this issue. |
Thanks @mattklein123 for help reviewing! |
For reviewers: This PR added the building blocks to support listener filter ECDS. But it is not hooked into listener manager code yet (e.g. envoy/source/server/listener_manager_impl.cc Line 112 in aa19a78
|
…er filter (envoyproxy#20560) This is the 3rd PR to support listener ECDS. It includes: 1) The class definition of ListenerFilterConfigProviderManagerImpl. 2) Moving getMessage() method to the base class to avoid duplicated code. 3) Unit test cases for ListenerFilterConfigProviderManagerImpl. Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
Problem description:
Listener ECDS: Adding FilterConfigProviderManagerImpl class implemantion for listener filter.
This is the 3rd PR to support listener filter ECDS as in issue: #20049. It includes:
Build:
passed
Testing:
passed
Release Notes:
N/A
Issues: ecds: add support for listener filters #20049
Signed-off-by: Yanjun Xiang yanjunxiang@google.com
Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]