connection handler: support multiple addresses in listener by soulxu · Pull Request #21521 · envoyproxy/envoy

soulxu · 2022-06-01T04:37:15Z

Commit Message: connection handler: support multiple addresses in listener
Additional Description:
This PR adds new interface listenSocketFactories() to the ListenerConfig. Since when the Listener supports multiple addresses, there will be multiple socket factories.

The ConnectionHandlerImpl will create an active listener for each socket in the listener, and manipulate all the active listeners belonging to the same listener together.

For the full picture, please reference #19367, this PR is primarily separated from this commit 7fa7666

Risk Level: high
Testing: unittest
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: no
Part of Fixes: #11184

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

Signed-off-by: He Jie Xu <hejie.xu@intel.com> quic: remove useless constructor Signed-off-by: He Jie Xu <hejie.xu@intel.com> udp: remove useless constructor Signed-off-by: He Jie Xu <hejie.xu@intel.com>

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu · 2022-06-01T04:42:21Z

@adisuissa @ggreenway @mattklein123 I changed the ListenerConfig's interface, then it touch more files. I can separate that out also if you think that is better for review.

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu · 2022-06-06T10:13:00Z

/retest

repokitteh-read-only · 2022-06-06T10:13:04Z

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

🐱

Caused by: a #21521 (comment) was created by @soulxu.

see: more, trace.

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu · 2022-06-09T10:35:09Z

/retest

repokitteh-read-only · 2022-06-09T10:35:13Z

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

🐱

Caused by: a #21521 (comment) was created by @soulxu.

see: more, trace.

mattklein123

LGTM with small comment comment, thanks.

/wait

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

adisuissa

Overall LGTM, thanks!
Left a couple of small comments

mattklein123

LGTM thanks, will defer to @adisuissa for final review and merge.

adisuissa

Thanks for working on this. Mostly small nits.

adisuissa · 2022-06-16T19:27:04Z

    Network::FilterChainFactory& filterChainFactory() override { return parent_; }
    Network::ListenSocketFactory& listenSocketFactory() override {
-      return *parent_.socket_factory_;
+      return *parent_.socket_factories_[0];


Should we have an assert here that the number of socket factories is 1?

yes, let me add one

adisuissa · 2022-06-16T19:29:46Z

-  auto details = std::make_shared<ActiveListenerDetails>();
+  auto details = std::make_unique<ActiveListenerDetails>();
  if (config.internalListenerConfig().has_value()) {
+    auto pre_address_details = std::make_shared<PerAddressActiveListenerDetails>();


Where is this being used?

Emm...it should be something I forget to remove when refactor the code

adisuissa · 2022-06-17T02:13:53Z

+    // TODO(soulxu): support multiple internal addresses in listener in the future.
+    ASSERT(config.listenSocketFactories().size() == 1);


Does this assert true also before the enclosing if scope?
Seeing the else if statement on line 73 suggests that this is true, but not sure.

This is for the multiple addresses doesn't support internal address. In the late PR, the API will reject multiple internal addresses, so I have the assertion here.

adisuissa · 2022-06-17T02:14:44Z

-          tcp_listener_map_by_address_.insert_or_assign(v4_compatible_addr->asStringView(),
-                                                        details);
+  for (auto& per_address_details : details->per_address_details_list_) {
+    // This map only store the new listener.


Suggested change

// This map only store the new listener.

// This map only stores the new listener.

adisuissa · 2022-06-17T02:17:22Z

+    } else if (absl::holds_alternative<std::reference_wrapper<Network::InternalListener>>(
+                   per_address_details->typed_listener_)) {
+      internal_listener_map_by_address_.insert_or_assign(
+          per_address_details->address_->asStringView(), per_address_details);


Note that the use of pre_address_details and per_address_details is somewhat tricky and makes the code harder to read, so if one can use a completely different name for one of these it would be better.

oops, pre_address_details should be a typo, sorry about that. I will use per_address_details for item name. But let me know if this isn't what you pointed to.

adisuissa · 2022-06-17T02:18:56Z

-            // both "::FFFF:<ipv4-addr>" with ipv4_compat and "<ipv4-addr>" isn't valid case,
-            // remove the v4 compatible addr item directly.
-            tcp_listener_map_by_address_.erase(v4_compatible_addr->asStringView());
+    for (auto& per_address_details : listener_iter->second->per_address_details_list_) {


nit: can per_address_details be const?

yes, it can be, let me update

adisuissa · 2022-06-17T02:21:09Z

+
+    using ListenerMethodFn = std::function<void(Network::ConnectionHandler::ActiveListener&)>;
+
+    void invokeListenerMethod(ListenerMethodFn fn) {


Please add some comments to public methods (helps when looking with an IDE and trying to understand what this is doing)

adisuissa · 2022-06-17T02:25:13Z

-        if (v4_compatible_addr != nullptr) {
-          tcp_listener_map_by_address_.insert_or_assign(v4_compatible_addr->asStringView(),
-                                                        details);
+  for (auto& per_address_details : details->per_address_details_list_) {


I may have missed it, but is there a test that adds more than one listener?
Maybe this is planned as part of the next PRs.

Sorry, I forget to cherry-pick that commit. Let me append that to this PR

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

adisuissa

Overall LGTM.
CI says admin.h coverage needs to be improved.

See if you may be able to improve connection_handler_impl.cc coverage. This may be deferred for a later PR.

/wait

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu · 2022-06-18T00:06:02Z

Thanks!

Overall LGTM. CI says admin.h coverage needs to be improved.

This is probably due to we adding one more assertion in the code, https://github.com/envoyproxy/envoy/pull/21521/files#diff-bdab5025c9978c2b063cc88f0ec2a1b7bf2efe9f24bd8a1ed1761d22cf27ddb5L392

I think there is no dependence on listenSocketFactory() method for admin API anymore, that is why the coverage becomes lower.

I will try to raise the coverage back after listenerSocketFactory method is gone https://github.com/envoyproxy/envoy/pull/21521/files#diff-0ac04ef31c616358d8234bd473469c1a233ecf37947a13475cce764e7f6c43ee

See if you may be able to improve connection_handler_impl.cc coverage. This may be deferred for a later PR.

I added two more tests for Internal Listener and Tcp listener in-place update with runtime flag envoy.reloadable_features.udp_listener_updates_filter_chain_in_place as false

May I ask where I can find the coverage report link? although I can modify the URL to get different PR, but I guess there should be an entry I didn't know.
I only find the main branch link https://storage.googleapis.com/envoy-postsubmit/main/coverage/index.html

adisuissa · 2022-06-18T02:52:57Z

May I ask where I can find the coverage report link? although I can modify the URL to get different PR, but I guess there should be an entry I didn't know. I only find the main branch link https://storage.googleapis.com/envoy-postsubmit/main/coverage/index.html

Using the azure-pipelines. See more details in doc.

adisuissa

LGTM, thanks!

soulxu · 2022-06-18T05:54:38Z

May I ask where I can find the coverage report link? although I can modify the URL to get different PR, but I guess there should be an entry I didn't know. I only find the main branch link https://storage.googleapis.com/envoy-postsubmit/main/coverage/index.html

Using the azure-pipelines. See more details in doc.

Thanks!

…y#21521) Signed-off-by: He Jie Xu <hejie.xu@intel.com> Signed-off-by: Amila Senadheera <amila.15@cse.mrt.ac.lk>

soulxu added 8 commits June 1, 2022 01:28

Enable to create ActiveTcpListener through the socket

039b440

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

Create UDP listener through the socket

52b6994

Signed-off-by: He Jie Xu <hejie.xu@intel.com> quic: remove useless constructor Signed-off-by: He Jie Xu <hejie.xu@intel.com> udp: remove useless constructor Signed-off-by: He Jie Xu <hejie.xu@intel.com>

ListenerImpl: add listenSocketFactories interface

1677455

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

ConnectionHandlerImpl: enable create multiple active listener

f6b64a9

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

fix test

7c9c661

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

correct comments

38bf82f

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

correct the variable name

5cc2bad

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

fix comments

375e492

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu requested review from alyssawilk and ggreenway as code owners June 1, 2022 04:37

fix clang-tidy

f977351

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

KBaichoo assigned adisuissa Jun 1, 2022

mattklein123 self-assigned this Jun 2, 2022

update coverage

c3feb80

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu commented Jun 2, 2022

View reviewed changes

Comment thread test/per_file_coverage.sh Outdated

soulxu added 3 commits June 6, 2022 01:16

Merge branch 'main' into multi_addresses_conn_handler

b7f7798

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

fix format

d95aefb

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

fix active internal listener test

5c96078

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu requested a review from adisuissa as a code owner June 6, 2022 06:48

Merge branch 'main' into multi_addresses_conn_handler

6859e07

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

mattklein123 requested changes Jun 14, 2022

View reviewed changes

Comment thread test/per_file_coverage.sh Outdated

repokitteh-read-only Bot added the waiting label Jun 14, 2022

mattklein123 reviewed Jun 14, 2022

View reviewed changes

Comment thread envoy/network/listener.h

add comments

31f2ffe

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

repokitteh-read-only Bot removed the waiting label Jun 15, 2022

adisuissa reviewed Jun 15, 2022

View reviewed changes

Comment thread source/server/listener_impl.cc

Comment thread source/server/listener_impl.h

mattklein123 previously approved these changes Jun 15, 2022

View reviewed changes

mattklein123 removed their assignment Jun 15, 2022

adisuissa reviewed Jun 17, 2022

View reviewed changes

soulxu added 3 commits June 17, 2022 05:58

Address comments

1b07b79

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

ConnectionHandlerImpl: Add test case for multiple addreses

b005722

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

Address comments

52d4681

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu dismissed mattklein123’s stale review via 52d4681 June 17, 2022 07:04

adisuissa reviewed Jun 17, 2022

View reviewed changes

repokitteh-read-only Bot added the waiting label Jun 17, 2022

soulxu added 3 commits June 17, 2022 23:27

fix coverage

289179d

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

Add more test for inplace update

2bb45fe

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

fix format

5ceed88

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

repokitteh-read-only Bot removed the waiting label Jun 17, 2022

adisuissa approved these changes Jun 18, 2022

View reviewed changes

adisuissa merged commit d1d7d2d into envoyproxy:main Jun 18, 2022

		// TODO(soulxu): support multiple internal addresses in listener in the future.
		ASSERT(config.listenSocketFactories().size() == 1);

	// This map only store the new listener.
	// This map only stores the new listener.


		using ListenerMethodFn = std::function<void(Network::ConnectionHandler::ActiveListener&)>;

		void invokeListenerMethod(ListenerMethodFn fn) {

Conversation

soulxu commented Jun 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soulxu commented Jun 1, 2022

Uh oh!

Uh oh!

soulxu commented Jun 6, 2022

Uh oh!

repokitteh-read-only Bot commented Jun 6, 2022

Uh oh!

soulxu commented Jun 9, 2022

Uh oh!

repokitteh-read-only Bot commented Jun 9, 2022

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mattklein123 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!

Choose a reason for hiding this comment

Uh oh!

soulxu Jun 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soulxu Jun 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

soulxu commented Jun 18, 2022

Uh oh!

adisuissa commented Jun 18, 2022

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

soulxu commented Jun 18, 2022

Uh oh!

Reviewers

Assignees

Labels

soulxu commented Jun 1, 2022 •

edited

Loading

soulxu Jun 17, 2022 •

edited

Loading

soulxu Jun 17, 2022 •

edited

Loading