listener: support multiple addresses#19367
Conversation
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
|
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! |
can we shared same |
Most of the things should be able to share except the stats, 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! |
soulxu
force-pushed
the
multiple_addresses_for_listener
branch
from
1b2bf35 to
eb34df1
Compare
soulxu
changed the title
|
/wait Still work-in-progress |
soulxu
force-pushed
the
multiple_addresses_for_listener
branch
from
eb34df1 to
adbb989
Compare
|
/wait still WIP |
|
/wait still working on the tests |
|
/wait The major code was finished, and should passed all the existing tests. Need to add new tests for new functionality. |
|
/wait |
Signed-off-by: He Jie Xu <hejie.xu@intel.com>
Signed-off-by: He Jie Xu <hejie.xu@intel.com>
|
/retest |
|
Retrying Azure Pipelines: |
Signed-off-by: He Jie Xu <hejie.xu@intel.com>
|
/retest |
|
Retrying Azure Pipelines: |
soulxu
changed the title
Signed-off-by: He Jie Xu <hejie.xu@intel.com>
| // 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
oneofbetween 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
oneofis the desired goal, consider addingoneof_promotiontag.
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.
There was a problem hiding this comment.
Let me see what the separate PR looks like. I found I can't change
ListenerImplandListenerManagerImplseparately, 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.
There was a problem hiding this comment.
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.
| // The actual local addresses that the listener is listening on. If a listener was configured | ||
| // 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; |
There was a problem hiding this comment.
Do we have backward-compatibility rule for the admin API?
There was a problem hiding this comment.
yes, this is a breaking change, and will not be approved.
|
@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
left a comment
adisuissa
left a comment
There was a problem hiding this comment.
Thanks for adding multiple addresses!
Left a few API comments.
| 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 |
There was a problem hiding this comment.
I think this v2 should not be modified.
There was a problem hiding this comment.
yea, I modify the wrong file and then forget to revert back.
| // The actual local addresses that the listener is listening on. If a listener was configured | ||
| // 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; |
There was a problem hiding this comment.
yes, this is a breaking change, and will not be approved.
| // 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. |
There was a problem hiding this comment.
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.
| // 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. |
There was a problem hiding this comment.
| // 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. |
|
@adisuissa thanks for the review! |
|
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 |
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! |
|
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
bot
added
the
stale
|
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! |
9 participants
Commit Message: listener: support multiple addresses
Additional Description:
This PR adds the ability of the listener to support multiple listening addresses.
The new field
addresseswas 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, andConnectionHandlerImplwill be aware of the multiple addresses. TheListenerImplwill contains multiple addresses and the corresponding socket factories.ListenerManagerImplwill be aware of the multiple addresses when adding or updating the listener.ConnectionHandlerImplis responsible for creating theActiveListenerfor each address.The
ActiveListenerfor UDP and TCP won't be aware of the multiple addresses, eachActiveListenerwill listen to a single address as before. We also can consider oneActiveListenerto serve multiple addresses, which will need more change. If this is needed, we can do this in the future.The
ListenerImplwill createConnectionBalancerandUdpWorkerRouterfor each address. TheActiveTcpListenerandActiveUdpListenerwill reference to the address-specific balancer and worker router.Also, the admin API
/listenersis 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