Listener ECDS: Adding config provider manager implemantion for listener filter by yanjunxiang-google · Pull Request #20560 · envoyproxy/envoy

yanjunxiang-google · 2022-03-28T20:59:01Z

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:

The class definition of ListenerFilterConfigProviderManagerImpl.
Moving getMessage() method to the base class to avoid duplicated code.
Unit test cases for ListenerFilterConfigProviderManagerImpl.

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:]

…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>

yanjunxiang-google · 2022-03-28T21:02:49Z

/assign @adisuissa @kyessenov @yanavlasov

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

envoy/filter/config_provider_manager.h

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

adisuissa

Thanks for working on this, mostly small things.

envoy/filter/config_provider_manager.h

source/common/filter/config_discovery_impl.cc

source/extensions/filters/listener/tls_inspector/BUILD

test/common/filter/config_discovery_impl_test.cc

source/common/filter/config_discovery_impl.h

kyessenov · 2022-03-29T20:51:14Z

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.

yanjunxiang-google · 2022-03-29T22:49:51Z

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>

source/common/filter/config_discovery_impl.h

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>

yanjunxiang-google · 2022-04-04T13:22:29Z

/retest

repokitteh-read-only · 2022-04-04T13:22:33Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #20560 (comment) was created by @yanjunxiang-google.

see: more, trace.

source/common/filter/config_discovery_impl.cc

yanjunxiang-google · 2022-04-05T00:07:36Z

/assign @htuch

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

yanjunxiang-google · 2022-04-19T20:10:42Z

@adisuissa @kyessenov PTAL

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

adisuissa

Thanks, overall LGTM.
@soulxu can you take another look.

adisuissa · 2022-04-22T04:38:54Z

source/common/filter/config_discovery_impl.h

 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,


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.

source/common/filter/config_discovery_impl.h

soulxu · 2022-04-22T08:01:45Z

Thanks, overall LGTM. @soulxu can you take another look.

overall LGTM also! except for the listener filter matcher #20560 (comment)

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

soulxu · 2022-04-25T01:45:21Z

@yanjunxiang-google thanks! that is perfect. Then this is LGTM now.

adisuissa

Thanks, LGTM modulo test nits.
@htuch can you PTAL?

test/common/filter/config_discovery_impl_test.cc

test/integration/filters/test_listener_filter.cc

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

yanjunxiang-google · 2022-04-26T15:34:37Z

@htuch @kyessenov PTAL

adisuissa

LGTM, left a small nit.

adisuissa · 2022-05-02T03:20:30Z

source/common/filter/config_discovery_impl.cc

-      Config::Utility::getFactoryByType<Server::Configuration::NamedHttpFilterConfigFactory>(
-          proto_config);
-  if (default_factory == nullptr) {
+void FilterConfigProviderManagerImplBase::validateProtoConfigDefaultFactory(


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.

Thanks for the comments.

This method throw exception. Envoy CI scripts doesn't allow throw exception in .h files.

htuch

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.

adisuissa

LGTM, thanks!

adisuissa · 2022-05-02T14:31:23Z

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.

Matt volunteered to do a final pass, once @kyessenov LGTM's this PR.

yanjunxiang-google · 2022-05-02T14:57:19Z

/assign @mattklein123

repokitteh-read-only · 2022-05-02T14:57:23Z

neither of Thanks, for, help, reviewing! can be assigned to this issue.

🐱

Caused by: a #20560 (comment) was created by @yanjunxiang-google.

see: more, trace.

yanjunxiang-google · 2022-05-02T15:15:02Z

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.

Matt volunteered to do a final pass, once @kyessenov LGTM's this PR.

Thanks @mattklein123 for help reviewing!

yanjunxiang-google · 2022-05-02T17:28:44Z

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

ProdListenerComponentFactory::createListenerFilterFactoryList_(

). The hooking to the listener manager and corresponding code, also the listener filter ECDS integration test, will be done in the coming PR.

…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>

yanjunxiang-google requested review from alyssawilk and mattklein123 as code owners March 28, 2022 20:59

yanjunxiang-google mentioned this pull request Mar 28, 2022

ecds: add support for listener filters #20049

Closed

repokitteh-read-only bot assigned adisuissa, kyessenov and yanavlasov Mar 28, 2022

fixing format error

c97c715

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

kyessenov reviewed Mar 29, 2022

View reviewed changes

envoy/filter/config_provider_manager.h Outdated Show resolved Hide resolved

switching to use tls_inspector listener filter in the test

c473f29

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

yanjunxiang-google requested review from KBaichoo and ggreenway as code owners March 29, 2022 15:31

yanjunxiang-google changed the title ~~Listener ECDS: Adding FilterConfigProviderManagerImpl class implemantion for listener filter~~ Listener ECDS: Adding config provider manager implemantion for listener filter Mar 29, 2022

adisuissa reviewed Mar 29, 2022

View reviewed changes

addressing comments

9b50205

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

soulxu reviewed Mar 30, 2022

View reviewed changes

source/common/filter/config_discovery_impl.h Outdated Show resolved Hide resolved

alyssawilk mentioned this pull request Mar 30, 2022

forward fix extensions being depended on by core code/tests #20589

Closed

yanjunxiang-google added 6 commits March 30, 2022 22:34

fixing test code

7829692

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

Merge branch 'main' of https://github.com/envoyproxy/envoy into ecds_…

5df2374

…listener Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

adding UDP listener filter support also refactoring ECDS tests

e7a217b

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

fixing format

39f434b

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

removing unused validate.proto

fe77b12

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

fixing CI error

4fd9312

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

lambdai reviewed Apr 4, 2022

View reviewed changes

source/common/filter/config_discovery_impl.cc Outdated Show resolved Hide resolved

addressing comments, moving statPrefix out of the template parameters

56fb733

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

repokitteh-read-only bot removed the waiting label Apr 14, 2022

addressing comments

1a8dd74

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

yanjunxiang-google added 2 commits April 21, 2022 02:34

clean up test code

99a590a

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

fix format

7dfbd75

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

adisuissa reviewed Apr 22, 2022

View reviewed changes

soulxu suggested changes Apr 22, 2022

View reviewed changes

source/common/filter/config_discovery_impl.h Show resolved Hide resolved

yanjunxiang-google added 4 commits April 24, 2022 17:20

addressing comments

8931395

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

adding a TODO comments

743de2a

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

fixing CI error

ad043b8

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

reduce duplicated code

105153a

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

adisuissa reviewed Apr 25, 2022

View reviewed changes

test/common/filter/config_discovery_impl_test.cc Outdated Show resolved Hide resolved

test/common/filter/config_discovery_impl_test.cc Outdated Show resolved Hide resolved

test/integration/filters/test_listener_filter.cc Outdated Show resolved Hide resolved

yanjunxiang-google added 2 commits April 25, 2022 15:01

addressing comments

eeff8c0

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

addressing comments

2de5f43

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>

adisuissa reviewed May 2, 2022

View reviewed changes

htuch reviewed May 2, 2022

View reviewed changes

adisuissa approved these changes May 2, 2022

View reviewed changes

mattklein123 self-assigned this May 2, 2022

mattklein123 approved these changes May 2, 2022

View reviewed changes

mattklein123 merged commit 2f7f14b into envoyproxy:main May 2, 2022

Conversation

yanjunxiang-google commented Mar 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yanjunxiang-google commented Mar 28, 2022

Uh oh!

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kyessenov commented Mar 29, 2022

Uh oh!

yanjunxiang-google commented Mar 29, 2022

Uh oh!

Uh oh!

yanjunxiang-google commented Apr 4, 2022

Uh oh!

repokitteh-read-only bot commented Apr 4, 2022

Uh oh!

Uh oh!

yanjunxiang-google commented Apr 5, 2022

Uh oh!

yanjunxiang-google commented Apr 19, 2022

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

adisuissa Apr 22, 2022

Choose a reason for hiding this comment

Uh oh!

yanjunxiang-google Apr 24, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

soulxu commented Apr 22, 2022

Uh oh!

soulxu commented Apr 25, 2022

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yanjunxiang-google commented Apr 26, 2022

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

adisuissa May 2, 2022

Choose a reason for hiding this comment

Uh oh!

yanjunxiang-google May 2, 2022

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

adisuissa commented May 2, 2022

Uh oh!

yanjunxiang-google commented May 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented May 2, 2022

Uh oh!

yanjunxiang-google commented May 2, 2022

Uh oh!

yanjunxiang-google commented May 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yanjunxiang-google commented Mar 28, 2022 •

edited

Loading

yanjunxiang-google commented May 2, 2022 •

edited

Loading

yanjunxiang-google commented May 2, 2022 •

edited

Loading