listener: support multiple addresses #19367

soulxu · 2021-12-31T03:59:00Z

Commit Message: listener: support multiple addresses
Additional Description:

This PR adds the ability of the listener to support multiple listening addresses.

The new field addresses was added to Listener API. The additional listening addresses can be added. Each address should be unique, and all the addresses should be the same protocol. The Envoy Internal address won't support this implementation since there is no use case for now, but it can be added in the future.

The ListenerImpl, ListenerManagerImpl, and ConnectionHandlerImpl will be aware of the multiple addresses. The ListenerImpl will contains multiple addresses and the corresponding socket factories. ListenerManagerImpl will be aware of the multiple addresses when adding or updating the listener. ConnectionHandlerImpl is responsible for creating the ActiveListener for each address.

The ActiveListener for UDP and TCP won't be aware of the multiple addresses, each ActiveListener will listen to a single address as before. We also can consider one ActiveListener to serve multiple addresses, which will need more change. If this is needed, we can do this in the future.

The ListenerImpl will create ConnectionBalancer and UdpWorkerRouter for each address. The ActiveTcpListener and ActiveUdpListener will reference to the address-specific balancer and worker router.

Also, the admin API /listeners is changed to support returning of multiple addresses.

Risk Level: high
Testing: unit tests and integration tests added
Docs Changes: API doc
Release Notes: new feature
Platform Specific Features: no
Fixes #11184

repokitteh-read-only · 2021-12-31T03:59:03Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #19367 was opened by soulxu.

see: more, trace.

repokitteh-read-only · 2021-12-31T03:59:08Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @adisuissa
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #19367 was opened by soulxu.

see: more, trace.

soulxu · 2021-12-31T04:00:11Z

cc @mattklein123

This is a very rough PoC yet. Maybe it is ok for a high-level review. Appreciate any comments and suggestions!

anyway, I will update it in 2022!

wbpcode · 2022-01-04T11:36:02Z

When the ListenerImpl has multiple addresses, ListenSocketFactory will be created for each address. And ConnectionHandlerImpl will create ActiveListener for each address also

can we shared same ActiveListener across multiple addresses? 🤔

soulxu · 2022-01-05T09:30:10Z

When the ListenerImpl has multiple addresses, ListenSocketFactory will be created for each address. And ConnectionHandlerImpl will create ActiveListener for each address also

can we shared same ActiveListener across multiple addresses? 🤔

Most of the things should be able to share except the stats, Each address should have its own stats, which managed by ActiveListener.

ggreenway · 2022-01-05T16:52:36Z

Each address should have its own stats, which managed by ActiveListener

Consider how this interacts with stat_prefix on the listener. We may want to allow configuring a stat_prefix for each address.

soulxu · 2022-01-05T23:58:51Z

Each address should have its own stats, which managed by ActiveListener

Consider how this interacts with stat_prefix on the listener. We may want to allow configuring a stat_prefix for each address.

got it, thanks!

api/envoy/config/listener/v3/listener.proto

source/server/connection_handler_impl.h

source/server/filter_chain_manager_impl.h

soulxu · 2022-01-24T06:25:25Z

/wait

Still work-in-progress

soulxu · 2022-01-29T08:02:11Z

/wait

still WIP

soulxu · 2022-01-31T03:57:58Z

/wait

still working on the tests

soulxu · 2022-02-14T12:09:04Z

/wait

The major code was finished, and should passed all the existing tests. Need to add new tests for new functionality.

soulxu · 2022-02-14T15:27:54Z

/wait

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

soulxu · 2022-05-18T10:54:41Z

/retest

repokitteh-read-only · 2022-05-18T10:54:45Z

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

🐱

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

see: more, trace.

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

soulxu · 2022-05-18T13:55:13Z

/retest

repokitteh-read-only · 2022-05-18T13:55:17Z

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

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

see: more, trace.

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

soulxu · 2022-05-18T22:37:21Z

api/envoy/config/listener/v3/listener.proto

@@ -107,8 +107,15 @@ message Listener {
  // that is governed by the bind rules of the OS. E.g., multiple listeners can listen on port 0 on
  // Linux as the actual port will be allocated by the OS.
  // Required unless *api_listener* or *listener_specifier* is populated.
+  // TODO (soulxu): deprecated the `address` field after `addresses` implemented.


I didn't mark the old field as deprecated now since it will required to modify a lot of example config and test to not use the depcreated field, that will increase this PR size again and making the review harder. But let me know if we have better procedure for this case.

I suggest splitting this PR, to a few PRs.
You can start by adding the addresses field, mark in not-implemented-hide, and gradually add additional parts of the implementation.
At the end, there should be a well-defined behavior of what happens when both fields (address and addresses) are provided.

Thanks! got it. I will mark it as not-implemented-hide, and separate the API change out.

For other parts, also want to hear others' opinions if they are happy to separate the PR, since before people may want to see the full picture. I'm ok with both ways, if we want to separate the whole thing, I will work on it.

@mattklein123 @ggreenway

Given that literally every envoy configuration uses these fields, we'll want to support both the old and the new forever. And I'd adopt the semantic that it's a config load error if both of them are set. So it logically turns into a oneof between the two, but without modifying the old proto field.

As far as separating the PR or doing it all at once, I'm fine either way. I like seeing how the config will all be put together in one shot, but if the PR gets too big for that I'm fine splitting it. It may be worth defining the entire proto, but just not writing all the code yet, which should be fine if it's not-implemented-hide.

Given that literally every envoy configuration uses these fields, we'll want to support both the old and the new forever. And I'd adopt the semantic that it's a config load error if both of them are set. So it logically turns into a oneof between the two, but without modifying the old proto field.

I tend to agree with this, but from previous experience, when a configuration server supports both "old" and "new" Envoy clients, and sends them the same config, then the "new" clients will reject it.
One thing we can do is have a runtime guard against the rejection of configs that include both fields, and eventually remove that check.
If oneof is the desired goal, consider adding oneof_promotion tag.

As far as separating the PR or doing it all at once, I'm fine either way. I like seeing how the config will all be put together in one shot, but if the PR gets too big for that I'm fine splitting it. It may be worth defining the entire proto, but just not writing all the code yet, which should be fine if it's not-implemented-hide.

If possible, I think that it makes sense to break this to multiple smaller PRs (not only API vs non-API) to make it more reviewable. That said, if the PR includes mostly new tests, then it should be ok.

I tend to agree with this, but from previous experience, when a configuration server supports both "old" and "new" Envoy clients, and sends them the same config, then the "new" clients will reject it.

In this case, I think it could be as simple as if there is only one address, config server populates the old field. If there are multiple addresses, config server populates the new field. If config server doesn't know if the client supports the new field, it can do the old behavior of completely duplicate listeners (filter chains etc) except with a different address.

I tend to agree with this, but from previous experience, when a configuration server supports both "old" and "new" Envoy clients, and sends them the same config, then the "new" clients will reject it. One thing we can do is have a runtime guard against the rejection of configs that include both fields, and eventually remove that check. If oneof is the desired goal, consider adding oneof_promotion tag.

I also have thing about keeping the old address field and adding a new one called additional_addresses. I haven't too much difference with one_of way. Only one thing is if the user doesn't specify the stat_prefix, then we fallback to use address field as stat_prefix. For the oneof case, if only the addresses specified, then choose the first one as the stat_prefix. The additional_addresses way has a little better sematic here.

As far as separating the PR or doing it all at once, I'm fine either way. I like seeing how the config will all be put together in one shot, but if the PR gets too big for that I'm fine splitting it. It may be worth defining the entire proto, but just not writing all the code yet, which should be fine if it's not-implemented-hide.

If possible, I think that it makes sense to break this to multiple smaller PRs (not only API vs non-API) to make it more reviewable. That said, if the PR includes mostly new tests, then it should be ok.

Let me see what the separate PR looks like. I found I can't change ListenerImpl and ListenerManagerImpl separately, since our unit-tests test them together. And we can live this PR here for people who want to see the whole picture.

Let me see what the separate PR looks like. I found I can't change ListenerImpl and ListenerManagerImpl separately, since our unit-tests test them together. And we can live this PR here for people who want to see the whole picture.

Just to clarify, breaking to multiple PRs is not a must, and in some cases it may make sense to have everything in one PR.

I should be able to separate the PR as the commits have in the PR (maybe merge a few of them together) https://github.com/envoyproxy/envoy/pull/19367/commits, I checked each of major commit has unittest covered. I will merge those cleanup and format change commits in the bottom to the top commits. The last PR should be for the integration tests. Let me know if you guys happy with that.

soulxu · 2022-05-18T22:38:46Z

api/envoy/admin/v3/listeners.proto

  // to listen on port 0, then this address has the port that was allocated by the OS.
-  config.core.v3.Address local_address = 2;
+  repeated config.core.v3.Address local_addresses = 2;


Do we have backward-compatibility rule for the admin API?

yes, this is a breaking change, and will not be approved.

soulxu · 2022-05-18T22:40:14Z

@mattklein123 I think this PR is ready for review. I updated the description for this PR #19367 (comment) hope that can help the review. Thanks for your review in advance!

/assign @mattklein123

adisuissa

Thanks for adding multiple addresses!
Left a few API comments.

adisuissa · 2022-05-19T01:49:16Z

api/envoy/admin/v2alpha/listeners.proto

@@ -26,7 +26,7 @@ message ListenerStatus {
  // Name of the listener
  string name = 1;

-  // The actual local address that the listener is listening on. If a listener was configured
+  // The actual local addresses that the listener is listening on. If a listener was configured


I think this v2 should not be modified.

yea, I modify the wrong file and then forget to revert back.

adisuissa · 2022-05-19T01:51:18Z

api/envoy/admin/v3/listeners.proto

  // to listen on port 0, then this address has the port that was allocated by the OS.
-  config.core.v3.Address local_address = 2;
+  repeated config.core.v3.Address local_addresses = 2;


yes, this is a breaking change, and will not be approved.

adisuissa · 2022-05-19T01:53:10Z

api/envoy/config/listener/v3/listener.proto

@@ -107,8 +107,15 @@ message Listener {
  // that is governed by the bind rules of the OS. E.g., multiple listeners can listen on port 0 on
  // Linux as the actual port will be allocated by the OS.
  // Required unless *api_listener* or *listener_specifier* is populated.
+  // TODO (soulxu): deprecated the `address` field after `addresses` implemented.


I suggest splitting this PR, to a few PRs.
You can start by adding the addresses field, mark in not-implemented-hide, and gradually add additional parts of the implementation.
At the end, there should be a well-defined behavior of what happens when both fields (address and addresses) are provided.

adisuissa · 2022-05-19T01:54:16Z

api/envoy/config/listener/v3/listener.proto

+  // A list of addresses that the listener should listen on. In general, the address must be unique, though
+  // that is governed by the bind rules of the OS. E.g., multiple listeners can listen on port 0 on
+  // Linux as the actual port will be allocated by the OS. For multiple addresses in single listener,
+  // all addresses are the same protocol, and multiple internal addresses don't support now.


Suggested change

// all addresses are the same protocol, and multiple internal addresses don't support now.

// all addresses use the same protocol, and multiple internal addresses are not yet supported.

soulxu · 2022-05-19T02:26:43Z

@adisuissa thanks for the review!

mattklein123 · 2022-05-25T22:37:42Z

Is this PR ready for full review? Also, this is very big. Is there any way to split this into smaller chunks? Thank you.

/wait-any

soulxu · 2022-05-26T00:18:03Z

Is this PR ready for full review? Also, this is very big. Is there any way to split this into smaller chunks? Thank you.

Yes, this is ready for a full review. Now we have two votes on the split PR, then let me split it. Also, the API change already separated out #21391, appreciate the review and feedback!

github-actions · 2022-06-25T20:01:08Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

github-actions · 2022-07-02T20:01:24Z

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

repokitteh-read-only bot added the api label Dec 31, 2021

repokitteh-read-only bot assigned adisuissa Dec 31, 2021

ycai-aspen reviewed Jan 6, 2022

View reviewed changes

api/envoy/config/listener/v3/listener.proto Outdated Show resolved Hide resolved

source/server/connection_handler_impl.h Outdated Show resolved Hide resolved

source/server/filter_chain_manager_impl.h Outdated Show resolved Hide resolved

danzh2010 mentioned this pull request Jan 6, 2022

Support multiple addresses per listener #11184

Closed

soulxu mentioned this pull request Jan 13, 2022

listener: filter chain unified matchers #18871

Closed

soulxu force-pushed the multiple_addresses_for_listener branch from 1b2bf35 to eb34df1 Compare January 24, 2022 06:24

soulxu changed the title ~~[POC] listener: support multiple addresses~~ [WIP] listener: support multiple addresses Jan 24, 2022

repokitteh-read-only bot added the waiting label Jan 24, 2022

soulxu force-pushed the multiple_addresses_for_listener branch from eb34df1 to adbb989 Compare January 29, 2022 08:01

repokitteh-read-only bot removed the waiting label Jan 29, 2022

repokitteh-read-only bot added waiting and removed waiting labels Jan 29, 2022

repokitteh-read-only bot added the waiting label Jan 31, 2022

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

repokitteh-read-only bot added waiting and removed waiting labels Feb 14, 2022

repokitteh-read-only bot added the waiting label Feb 14, 2022

repokitteh-read-only bot removed the waiting label Feb 24, 2022

soulxu added 2 commits May 18, 2022 10:27

Merge branch 'main' into multiple_addresses_for_listener

c5409dd

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

quic integration: running all the clients

5b5577f

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

Fix clang-tidy

0921df1

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

soulxu changed the title ~~[WIP] listener: support multiple addresses~~ listener: support multiple addresses May 18, 2022

Add comment

871eb36

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

soulxu commented May 18, 2022

View reviewed changes

soulxu marked this pull request as ready for review May 18, 2022 22:38

soulxu requested a review from htuch as a code owner May 18, 2022 22:38

repokitteh-read-only bot assigned mattklein123 May 18, 2022

adisuissa reviewed May 19, 2022

View reviewed changes

soulxu mentioned this pull request May 20, 2022

Listener: the API change for multiple addresses support #21391

Merged

repokitteh-read-only bot added the waiting:any label May 25, 2022

repokitteh-read-only bot removed the waiting:any label May 26, 2022

mattklein123 added the waiting label May 26, 2022

soulxu mentioned this pull request Jun 1, 2022

connection handler: support multiple addresses in listener #21521

Merged

soulxu mentioned this pull request Jun 9, 2022

Upstream part of smarter way to pick endpoints for dual-stack IPv4/v6 support #21640

Closed

soulxu mentioned this pull request Jun 20, 2022

listener: enable connection balancer to support multiple addresses listener #21774

Merged

github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Jun 25, 2022

github-actions bot closed this Jul 2, 2022

	// all addresses are the same protocol, and multiple internal addresses don't support now.
	// all addresses use the same protocol, and multiple internal addresses are not yet supported.

listener: support multiple addresses #19367

listener: support multiple addresses #19367

Uh oh!

Conversation

soulxu commented Dec 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Dec 31, 2021

Uh oh!

repokitteh-read-only bot commented Dec 31, 2021

Uh oh!

soulxu commented Dec 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wbpcode commented Jan 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soulxu commented Jan 5, 2022

Uh oh!

ggreenway commented Jan 5, 2022

Uh oh!

soulxu commented Jan 5, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

soulxu commented Jan 24, 2022

Uh oh!

soulxu commented Jan 29, 2022

Uh oh!

soulxu commented Jan 31, 2022

Uh oh!

soulxu commented Feb 14, 2022

Uh oh!

soulxu commented Feb 14, 2022

Uh oh!

soulxu commented May 18, 2022

Uh oh!

repokitteh-read-only bot commented May 18, 2022

Uh oh!

soulxu commented May 18, 2022

Uh oh!

repokitteh-read-only bot commented May 18, 2022

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 May 19, 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!

soulxu May 20, 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!

soulxu commented May 18, 2022

Uh oh!

adisuissa left a comment

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

soulxu commented Dec 31, 2021 •

edited

Loading

soulxu commented Dec 31, 2021 •

edited

Loading

wbpcode commented Jan 4, 2022 •

edited

Loading

soulxu May 19, 2022 •

edited

Loading

soulxu May 20, 2022 •

edited

Loading