Fix hot restart bug due to passed an fd without bind#8426
Fix hot restart bug due to passed an fd without bind#8426mattklein123 merged 5 commits intoenvoyproxy:masterfrom
Conversation
Signed-off-by: tianqian.zyf <tianqian.zyf@alibaba-inc.com>
9959bb9 to
911b541
Compare
mattklein123
left a comment
There was a problem hiding this comment.
Looks good a at a high level, thanks. Some small comments.
/wait
| : Network::Utility::UDP_SCHEME; | ||
| const std::string addr = absl::StrCat(scheme, address->asString()); | ||
|
|
||
| if (!bind_to_port && socket_type == Network::Address::SocketType::Stream) { |
There was a problem hiding this comment.
Is this needed? Won't this just return -1 below once you check for not binding in the parent?
There was a problem hiding this comment.
Here is the need to determine whether the bind, if there is no bind socket to the parent process to obtain the socket, may get the already bind socket, in addition, there is no way to handle the traffic without the bind socket, we can not go to the parent process to obtain the socket, directly create it.
| Network::Address::InstanceConstSharedPtr addr = | ||
| Network::Utility::resolveUrl(request.pass_listen_socket().address()); | ||
| for (const auto& listener : server_->listenerManager().listeners()) { | ||
| if (listener.get().socket().socketType() == Network::Address::SocketType::Stream && |
There was a problem hiding this comment.
Pedantically, do we need the stream type check? Isn't just checking for bind to port sufficient?
There was a problem hiding this comment.
Yes, you are right, I understand it wrong, udp socket does not have bind to port, there is no way to handle traffic., so we just need to judge whether bind to port is enough.
Signed-off-by: tianqian.zyf <tianqian.zyf@alibaba-inc.com>
cae0fd1 to
6347e39
Compare
Signed-off-by: tianqian.zyf <tianqian.zyf@alibaba-inc.com>
|
/retest |
|
🔨 rebuilding |
|
@zyfjeff friendly request to please not force push. It makes reviewing more difficult. Thank you! |
mattklein123
left a comment
There was a problem hiding this comment.
Thanks, this makes sense to me. One test request .Thank you!
/wait
| } else { | ||
| return std::make_shared<Network::UdpListenSocket>(std::move(io_handle), address, options); | ||
|
|
||
| if (bind_to_port) { |
There was a problem hiding this comment.
Can we add a listener manager test for this case also?
There was a problem hiding this comment.
Yes, the idea of my test case is to test when the bind_to_port is equal to false, not to get the socket from the hot restart, but to create the socket directly.
Signed-off-by: tianqian.zyf <tianqian.zyf@alibaba-inc.com>
mattklein123
left a comment
There was a problem hiding this comment.
Thanks, LGTM with some small comments.
/wait
| Network::Address::SocketType socket_type, | ||
| const Network::Socket::OptionsSharedPtr& options, | ||
| bool bind_to_port) -> Network::SocketSharedPtr { | ||
| // When bind_to_port is equal to false, create socket fd directly, and do not get socket |
There was a problem hiding this comment.
Can you add an explicit expectation with Times(0) to the mock hot restarter to make sure we never call it?
| class ListenerManagerImplTest : public testing::Test { | ||
| protected: | ||
| ListenerManagerImplTest() : api_(Api::createApiForTest()) { | ||
| ListenerManagerImplTest() : api_(Api::createApiForTest()), real_listener_factory_(server_) { |
There was a problem hiding this comment.
Can you just define this in the test where you use it for now as we don't use it anywhere else?
Signed-off-by: tianqian.zyf <tianqian.zyf@alibaba-inc.com>
|
/azp run envoy-linux |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Signed-off-by: tianqian.zyf <tianqian.zyf@alibaba-inc.com>
Signed-off-by: tianqian.zyf <tianqian.zyf@alibaba-inc.com>
When the 15001 listener appears at the same time, one of them will bind the socket and listening, and the other will not bind. In this case, the hot restart gets the socket of the previous process and may get the socket that is not listening.
Description: Fix hot restart bug due to passed an fd without bind
Risk Level: low
Testing: Existing tests and new tests
Docs Changes: N/A
Release Notes: N/A
[Optional Fixes #Issue] #8427