listener: fixed a bug where the addresses cannot be updated partially #38601
Conversation
Signed-off-by: wangbaiping(wbpcode) <[email protected]>
Signed-off-by: wangbaiping(wbpcode) <[email protected]>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
|
I personally this refactoring will make our future life and maintaining easier. But It would be super appreciated if you can check it to see if I missed something. cc @Shikugawa |
Signed-off-by: wangbaiping(wbpcode) <[email protected]>
Signed-off-by: wangbaiping(wbpcode) <[email protected]>
| // Check if the listener has duplicated address with existing listeners. | ||
|
|
||
| for (const auto& existing_listener : listener_list) { | ||
| if (listener.reusePort() && existing_listener->name() == listener.name()) { |
There was a problem hiding this comment.
I prefer to reject reuse_port == false and the listeners with the same name listen with the duplicated address early as my original PR.
envoy/source/common/listener_manager/listener_manager_impl.cc
Lines 528 to 532 in 50df682
In this implementation, the user can receive only messages that identify "listening address is duplicated". I guess the users may confused about the message, and can't know why this configuration is rejected.
There was a problem hiding this comment.
Oh, But I think this message helps users to understand the root cause. So personally current implementation is LGTM.
envoy/source/common/listener_manager/listener_manager_impl.cc
Lines 1149 to 1150 in f39d4d3
| // Check if the listener has duplicated address with existing listeners. | ||
|
|
||
| for (const auto& existing_listener : listener_list) { | ||
| if (listener.reusePort() && existing_listener->name() == listener.name()) { |
There was a problem hiding this comment.
Oh, But I think this message helps users to understand the root cause. So personally current implementation is LGTM.
envoy/source/common/listener_manager/listener_manager_impl.cc
Lines 1149 to 1150 in f39d4d3
|
/retest |
| @@ -400,6 +400,95 @@ name: bar | |||
| addOrUpdateListener(parseListenerFromV3Yaml(yaml2)); | |||
| } | |||
|
|
|||
| TEST_P(ListenerManagerImplWithRealFiltersTest, AllowAddressesUpdateParitially) { | |||
| const std::string yaml1 = R"EOF( | |||
| name: foo | |||
There was a problem hiding this comment.
Please add a comment saying what part of the listener config is changing so the reader doesn't have to manually diff the two configs in their head.
Co-authored-by: Greg Greenway <[email protected]> Signed-off-by: code <[email protected]>
Signed-off-by: wangbaiping(wbpcode) <[email protected]>
Commit Message: listener: fixed a bug where the addresses cannot be updated partially
Additional Description:
To close #37373. The #37373 was opened for a while and there is some related PR try to fixed it. See #37396 #38530. Thanks for the great contribution from @Shikugawa and @bbassingthwaite.
Their works is great and make sense. But I want a clean patch to fixed all LDS updating problems (not only the #37373). The exist logic and checks are pretty confusing and hard to maintain. So, I think we'd better to do complete refactring/clean up rather than only fix the #37373. So, I created this PR.
If we treat the addresses self and socket options as a whole entity. The rules of LDS updating of same Listener is pretty simple:
The conditional duplicated addresses checking (of different versions of same listener) could cover 2 and 3 easily. If the reuse port is enabled, we can skip the check because we can create new sockets anyway. If the resue port is disabled, we need this check and partially updating will be rejected or if all addresses are different, the update still could be accpeted.
Risk Level: mid. core code change. It would be better to let two senior maintainers to check this PR.
Testing: unit.
Docs Changes: n/a.
Release Notes: added.
Platform Specific Features: n/a.