Fix hot restart bug due to passed an fd without bind #8426

zyfjeff · 2019-09-29T07:58:36Z

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

Signed-off-by: tianqian.zyf <[email protected]>

mattklein123

Looks good a at a high level, thanks. Some small comments.

/wait

mattklein123 · 2019-09-30T18:51:31Z

source/server/listener_manager_impl.cc

@@ -166,6 +166,11 @@ Network::SocketSharedPtr ProdListenerComponentFactory::createListenSocket(
                                 ? Network::Utility::TCP_SCHEME
                                 : Network::Utility::UDP_SCHEME;
  const std::string addr = absl::StrCat(scheme, address->asString());
+
+  if (!bind_to_port && socket_type == Network::Address::SocketType::Stream) {


Is this needed? Won't this just return -1 below once you check for not binding in the parent?

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.

mattklein123 · 2019-09-30T18:52:09Z

source/server/hot_restarting_parent.cc

@@ -95,6 +95,11 @@ HotRestartingParent::Internal::getListenSocketsForChild(const HotRestartMessage:
  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 &&


Pedantically, do we need the stream type check? Isn't just checking for bind to port sufficient?

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 <[email protected]>

zyfjeff · 2019-10-01T05:02:35Z

/retest

repokitteh-read-only · 2019-10-01T05:02:39Z

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #8426 (comment) was created by @zyfjeff.

see: more, trace.

mattklein123 · 2019-10-01T17:33:09Z

@zyfjeff friendly request to please not force push. It makes reviewing more difficult. Thank you!

mattklein123

Thanks, this makes sense to me. One test request .Thank you!

/wait

mattklein123 · 2019-10-01T17:35:24Z

source/server/listener_manager_impl.cc

-    } else {
-      return std::make_shared<Network::UdpListenSocket>(std::move(io_handle), address, options);
+
+  if (bind_to_port) {


Can we add a listener manager test for this case also?

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 <[email protected]>

mattklein123

Thanks, LGTM with some small comments.

/wait

mattklein123 · 2019-10-03T17:47:26Z

test/server/listener_manager_impl_test.cc

+                                         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


Can you add an explicit expectation with Times(0) to the mock hot restarter to make sure we never call it?

mattklein123 · 2019-10-03T17:47:45Z

test/server/listener_manager_impl_test.h

@@ -37,7 +37,7 @@ class ListenerHandle {

 class ListenerManagerImplTest : public testing::Test {
 protected:
-  ListenerManagerImplTest() : api_(Api::createApiForTest()) {
+  ListenerManagerImplTest() : api_(Api::createApiForTest()), real_listener_factory_(server_) {


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 <[email protected]>

mattklein123

Awesome, thanks.

mattklein123 · 2019-10-04T03:19:36Z

/azp run envoy-linux

azure-pipelines · 2019-10-04T03:19:45Z

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: tianqian.zyf <[email protected]>

Fix hot restart bug due to passed an fd without bind

911b541

Signed-off-by: tianqian.zyf <[email protected]>

zyfjeff force-pushed the fix-hotrestart-bug branch from 9959bb9 to 911b541 Compare September 29, 2019 07:59

zyfjeff changed the title ~~WIP: Fix hot restart bug due to passed an fd without bind~~ Fix hot restart bug due to passed an fd without bind Sep 29, 2019

dio assigned mattklein123 Sep 30, 2019

mattklein123 requested changes Sep 30, 2019

View reviewed changes

repokitteh-read-only bot added waiting and removed waiting labels Sep 30, 2019

Address code review comments

6347e39

Signed-off-by: tianqian.zyf <[email protected]>

zyfjeff force-pushed the fix-hotrestart-bug branch from cae0fd1 to 6347e39 Compare October 1, 2019 02:42

Fix compile errors

3838f12

Signed-off-by: tianqian.zyf <[email protected]>

mattklein123 requested changes Oct 1, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Oct 1, 2019

Add listener manager test case for bind_to_port=false

a36120a

Signed-off-by: tianqian.zyf <[email protected]>

repokitteh-read-only bot removed the waiting label Oct 2, 2019

mattklein123 requested changes Oct 3, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Oct 3, 2019

Address code reviews comments

435407f

Signed-off-by: tianqian.zyf <[email protected]>

repokitteh-read-only bot removed the waiting label Oct 3, 2019

mattklein123 approved these changes Oct 4, 2019

View reviewed changes

mattklein123 merged commit fcb3cf1 into envoyproxy:master Oct 4, 2019

danzh2010 pushed a commit to danzh2010/envoy that referenced this pull request Oct 4, 2019

Fix hot restart bug due to passed an fd without bind (envoyproxy#8426)

c5b658e

Signed-off-by: tianqian.zyf <[email protected]>

zyfjeff deleted the fix-hotrestart-bug branch October 12, 2019 02:32

nandu-vinodan pushed a commit to nandu-vinodan/envoy that referenced this pull request Oct 17, 2019

Fix hot restart bug due to passed an fd without bind (envoyproxy#8426)

c7c6bb8

Signed-off-by: tianqian.zyf <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix hot restart bug due to passed an fd without bind #8426

Fix hot restart bug due to passed an fd without bind #8426

zyfjeff commented Sep 29, 2019 •

edited

Loading

mattklein123 left a comment

mattklein123 Sep 30, 2019

zyfjeff Oct 1, 2019 •

edited

Loading

mattklein123 Sep 30, 2019

zyfjeff Oct 1, 2019 •

edited

Loading

zyfjeff commented Oct 1, 2019

repokitteh-read-only bot commented Oct 1, 2019

mattklein123 commented Oct 1, 2019

mattklein123 left a comment

mattklein123 Oct 1, 2019

zyfjeff Oct 2, 2019

mattklein123 left a comment

mattklein123 Oct 3, 2019

mattklein123 Oct 3, 2019

mattklein123 left a comment

mattklein123 commented Oct 4, 2019

azure-pipelines bot commented Oct 4, 2019

Fix hot restart bug due to passed an fd without bind #8426

Fix hot restart bug due to passed an fd without bind #8426

Conversation

zyfjeff commented Sep 29, 2019 • edited Loading

mattklein123 left a comment

Choose a reason for hiding this comment

mattklein123 Sep 30, 2019

Choose a reason for hiding this comment

zyfjeff Oct 1, 2019 • edited Loading

Choose a reason for hiding this comment

mattklein123 Sep 30, 2019

Choose a reason for hiding this comment

zyfjeff Oct 1, 2019 • edited Loading

Choose a reason for hiding this comment

zyfjeff commented Oct 1, 2019

repokitteh-read-only bot commented Oct 1, 2019

mattklein123 commented Oct 1, 2019

mattklein123 left a comment

Choose a reason for hiding this comment

mattklein123 Oct 1, 2019

Choose a reason for hiding this comment

zyfjeff Oct 2, 2019

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

mattklein123 Oct 3, 2019

Choose a reason for hiding this comment

mattklein123 Oct 3, 2019

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

mattklein123 commented Oct 4, 2019

azure-pipelines bot commented Oct 4, 2019

zyfjeff commented Sep 29, 2019 •

edited

Loading

zyfjeff Oct 1, 2019 •

edited

Loading

zyfjeff Oct 1, 2019 •

edited

Loading