quiche: implement quic_port_utils by danzh2010 · Pull Request #6488 · envoyproxy/envoy

danzh2010 · 2019-04-05T16:26:12Z

Implement QuicPickUnusedPortOrDie() and QuicRecyclePort() backed by Envoy::Network::Test::findOrCheckFreePort().
Added include_prefix to envoy_cc_test_library argument list and pass it down. So that quiche test only impl's can be included by api header files with relative #include path.

Risk Level: low, not in use
Testing: added new tests in test/extensions/quic_listeners/quiche/platform/quic_platform_test.cc
Part of #2557

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2019-04-05T16:26:32Z

/assign @wu-bin

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2019-04-05T19:06:14Z

/retest

repokitteh-read-only · 2019-04-05T19:06:27Z

🙀 Error while processing event:

evaluation error
error: function http error: Post https://circleci.com/api/v1.1/project/github/envoyproxy/envoy/193731/retry?circle-token=57b4b7966a75db7541e68405ceffb9ffae846ef9: net/http: request canceled (Client.Timeout exceeded while awaiting headers)

🐱

Caused by: a #6488 (comment) was created by @danzh2010.

see: more, trace.

wu-bin · 2019-04-08T13:48:58Z

 )

+envoy_cc_test_library(
+    name = "quic_platform_port_utils_impl_lib",


Is it possible to move this library to //test? That way the source code in this library will be excluded from the coverage ci.

Its header file has to be in the same directory as other impl files unless we want to config the gen rule to change the #include path in its API file which doesn't seem nice to me.

wu-bin · 2019-04-08T13:58:16Z

-  const int rc = ::bind(fd, reinterpret_cast<const sockaddr*>(&ip_.ipv4_.address_),
-                        sizeof(ip_.ipv4_.address_));
-  return {rc, errno};
+  auto& os_syscalls = Api::OsSysCallsSingleton::get();


Is this change related to this PR? Or is it a drive-by fix?

it's adding a mock-able layer of system calls for testing.

wu-bin · 2019-04-08T14:14:10Z

+      return port;
+    }
+  }
+  RELEASE_ASSERT(false, "Failed to pick a port for test.");


How about just ASSERT? I think in dbg mode we also want it to die if it can't pick a unused port.

RELEASE_ASSERT die in both case, but ASSERT doesn't die in opt mode.

Oh, I got it reversed, sorry.

wu-bin · 2019-04-08T14:16:34Z

+  // Only try to get a port for limited times.
+  std::vector<Envoy::Network::Address::IpVersion> supported_versions =
+      Envoy::TestEnvironment::getIpVersionsForTest();
+  for (size_t i = 0; i < 30000; ++i) {


30000 iterations looks very high:) I think O(10) is enough.

In other platforms, we use similar large numbers.

I'd go smaller here as well if for no other reason than I trust CircleCi less than blaze and chrome's test frameworks :-/

reduce to 300

wu-bin · 2019-04-08T14:24:54Z

+  // Fail bind call's to mimic port exhaustion.
+  EXPECT_CALL(os_sys_calls, bind(_, _, _))
+      .WillRepeatedly(Return(Envoy::Api::SysCallIntResult{-1, EADDRINUSE}));
+  EXPECT_DEATH(QuicPickUnusedPortOrDie(), "Failed to pick a port for test.");


In #6393 I've changed

QuicPlatformTest from TEST to TEST_F.

EXPECT_DEATH to EXPECT_DEATH_LOG_TO_STDERR.

Please sync and do the same for the new tests.

Signed-off-by: Dan Zhang <danzh@google.com>

wu-bin

LGTM

danzh2010 · 2019-04-08T18:35:11Z

/retest
/assign @alyssawilk

repokitteh-read-only · 2019-04-08T18:35:21Z

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #6488 (comment) was created by @danzh2010.

see: more, trace.

danzh2010 · 2019-04-08T20:10:16Z

SslIpVersionsClientType/GrpcSslClientIntegrationTest.BasicSslRequestWithClientCert/IPv4_EnvoyGrpc failed coverage test. It shouldn't be related to this PR.

danzh2010 · 2019-04-08T21:45:39Z

/retest

repokitteh-read-only · 2019-04-08T21:45:43Z

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #6488 (comment) was created by @danzh2010.

see: more, trace.

danzh2010 · 2019-04-09T14:46:46Z

/retest

repokitteh-read-only · 2019-04-09T14:46:53Z

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #6488 (comment) was created by @danzh2010.

see: more, trace.

alyssawilk

@htuch can you glance at the bzl changes?

alyssawilk · 2019-04-09T14:31:18Z

+  // Only try to get a port for limited times.
+  std::vector<Envoy::Network::Address::IpVersion> supported_versions =
+      Envoy::TestEnvironment::getIpVersionsForTest();
+  for (size_t i = 0; i < 30000; ++i) {


I'd go smaller here as well if for no other reason than I trust CircleCi less than blaze and chrome's test frameworks :-/

alyssawilk · 2019-04-09T14:40:20Z

+        break;
+      }
+      if (port == 0) {
+        // Just get a port from findOrCheckFreePort(), check its usability in the rest address


rest -> other
(or rest of)

alyssawilk · 2019-04-09T14:48:20Z

+  for (size_t i = 0; i < 30000; ++i) {
+    uint32_t port = 0;
+    bool available = true;
+    for (auto ip_version : supported_versions) {


I should probably know this offhand, but do we need to loop here? If we're listening on ipv6 inaddr any, that covers ipv4 over mapped addresses but I'm not sure if listening on 127.0.0.1:444 and then ::444 would fail on the platforms we support.

but I'm not sure if listening on 127.0.0.1:444 and then ::444 would fail on the platforms we support.
What do you mean?

The returned port of this function should be able to bind to both an v4 and v6 address (in separate test) if test supports both address families. So here we need check if we can create a socket and bind it to an address in both v4 and v6.

I replaced the version loop with checking on single version according to our discussion. Thanks for the suggestion!

alyssawilk · 2019-04-09T16:24:57Z

FWIW if your coverage fail was gRPC I think that's fixed at HEAD, so a master merge may help

Signed-off-by: Dan Zhang <danzh@google.com>

alyssawilk

One last comment request, otherwise LGTM

alyssawilk · 2019-04-09T19:27:23Z

+  std::vector<Envoy::Network::Address::IpVersion> supported_versions =
+      Envoy::TestEnvironment::getIpVersionsForTest();
+  ASSERT(!supported_versions.empty());
+  Envoy::Network::Address::IpVersion ip_version = supported_versions.size() == 1


Maybe a comment as to why this works?

I commented a few lines below inside for loop.

Fair point. I think it would be more useful here, if folks are reading top down though.

Signed-off-by: Dan Zhang <danzh@google.com>

htuch

Build changes look good, but just a comment on the port picking.

htuch · 2019-04-09T22:39:51Z

+    if (addr_port != nullptr && addr_port->ip() != nullptr) {
+      // Find a port.
+      return addr_port->ip()->port();
+    }


@curiouserrandy has also written code like this, so might have comments.

This looks like a lot of hassle to do something the OS will do for you (i.e. pick a port if you bind port 0), and additionally racy as well. Can you give me a sense as to why you're doing it this way as opposed to using bind(..., 0) at point of use?

I also share Harvey's concern about mixing apparently production and apparently test code.

see my reply to Harvey's comment

Ah, gotcha. Pity :-}. Next question: As I read the code (including findOrCheckFreePort) what is being done is that the address passed is being bound with port == 0, the port returned by the kernel is being returned, and the bound socket is being released. Why is that wrapped in a "try 300 times" loop? If a bind with port 0 fails, I'd think there's something badly wrong somewhere, and retrying wouldn't help.

(Meta-comment: I'm still in the "asking questions to understand the code" phase of the review, so if you and Harvey get to closure, don't block on me.)

Ah, gotcha. Pity :-}. Next question: As I read the code (including findOrCheckFreePort) what is being done is that the address passed is being bound with port == 0, the port returned by the kernel is being returned, and the bound socket is being released. Why is that wrapped in a "try 300 times" loop? If a bind with port 0 fails, I'd think there's something badly wrong somewhere, and retrying wouldn't help.
Would temporary running out of port lead to bind failure on port 0? Or some other transient system failure?

Yeah, I think the loop made sense when we were doing v4/v6 checks but now that we've done away with that I suspect we can kill the loop off entirely.

It could be transient but it's functional equivalent to sleep(1)-and-try-again so I think we can skip

htuch · 2019-04-09T22:40:55Z

+
+namespace quic {
+
+int QuicPickUnusedPortOrDieImpl() {


This is test-only, right? I would ideally like to see a better naming or structuring to indicate code is for test vs. prod., since there are different requirements here as far as error handling and robustness. Are we restricted by the QUICHE layout?

Also, why does QUICHE do this? In Envoy, we just bind zero on the server side and then communicate the effective port OOB to the client in integration tests. Could QUICHE do this?

This is test-only, right? I would ideally like to see a better naming or structuring to indicate code is for test vs. prod., since there are different requirements here as far as error handling and robustness. Are we restricted by the QUICHE layout?
Talked with Harvey about a reasonable approach, such test only code can be moved to /test, and be included by a dummy impl.h file in /source.

Also, why does QUICHE do this? In Envoy, we just bind zero on the server side and then communicate the effective port OOB to the client in integration tests. Could QUICHE do this?
Quic e2e tests doesn't have this server->client channel to communicate the port it picks after starting listening.

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2019-04-10T18:47:49Z

Moved the definitions to //test/ PTAL

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2019-04-10T19:54:49Z

/retest

repokitteh-read-only · 2019-04-10T19:54:54Z

🔨 rebuilding ci/circleci: build_image (failed build)
🔨 rebuilding ci/circleci: docker (failed build)
🔨 rebuilding ci/circleci: ipv6_tests (failed build)

🐱

Caused by: a #6488 (comment) was created by @danzh2010.

see: more, trace.

htuch

/wait

htuch · 2019-04-10T19:55:36Z

@@ -0,0 +1,10 @@
+


Nit: superfluous blank line

htuch · 2019-04-10T19:59:13Z

+  // If it supports both v4 and v6, checking availability under v6 with IPV6_V6ONLY
+  // set to false is sufficient because such socket can be used on v4-mapped
+  // v6 address.
+  Envoy::Network::Address::IpVersion ip_version = supported_versions.size() == 1


Nit: const

htuch · 2019-04-10T19:59:31Z

+// consumed or referenced directly by other Envoy code. It serves purely as a
+// porting layer for QUICHE.
+
+#include "test/extensions/quic_listeners/quiche/platform/quic_port_utils_test_impl.h"


Can you add a comment explaining this redirect for posterity? Thanks

Signed-off-by: Dan Zhang <danzh@google.com>

htuch · 2019-04-10T22:10:27Z

Thanks @danzh2010, this is good to merge once CI passes.

danzh2010 · 2019-04-10T22:43:26Z

/retest

repokitteh-read-only · 2019-04-10T22:43:30Z

🔨 rebuilding ci/circleci: ipv6_tests (failed build)

🐱

Caused by: a #6488 (comment) was created by @danzh2010.

see: more, trace.

danzh1989 added 3 commits April 5, 2019 11:54

Implement QuicPortUtils API

7b774ba

Signed-off-by: Dan Zhang <danzh@google.com>

remove debug logging

bff481c

Signed-off-by: Dan Zhang <danzh@google.com>

add select_quiche

01cfb7e

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 requested review from alyssawilk and mattklein123 as code owners April 5, 2019 16:26

repokitteh-read-only Bot assigned wu-bin Apr 5, 2019

fix typo

40d57f4

Signed-off-by: Dan Zhang <danzh@google.com>

wu-bin reviewed Apr 8, 2019

View reviewed changes

danzh1989 added 3 commits April 8, 2019 11:49

Merge branch 'master' into port_util

04d464f

Signed-off-by: Dan Zhang <danzh@google.com>

fix merge

e8deee5

Signed-off-by: Dan Zhang <danzh@google.com>

TEST_F

0d8b1ee

Signed-off-by: Dan Zhang <danzh@google.com>

wu-bin previously approved these changes Apr 8, 2019

View reviewed changes

repokitteh-read-only Bot assigned alyssawilk Apr 8, 2019

alyssawilk reviewed Apr 9, 2019

View reviewed changes

reduce iteration times

2b5e61f

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 dismissed wu-bin’s stale review via 2b5e61f April 9, 2019 17:04

danzh1989 added 2 commits April 9, 2019 13:50

Merge branch 'master' into port_util

9b49f2c

Signed-off-by: Dan Zhang <danzh@google.com>

remove check for v4 and v6

aae1bd7

Signed-off-by: Dan Zhang <danzh@google.com>

alyssawilk reviewed Apr 9, 2019

View reviewed changes

move around comment

a9ae17c

Signed-off-by: Dan Zhang <danzh@google.com>

alyssawilk previously approved these changes Apr 9, 2019

View reviewed changes

htuch reviewed Apr 9, 2019

View reviewed changes

htuch added the waiting:any label Apr 9, 2019

repokitteh-read-only Bot removed the waiting:any label Apr 10, 2019

move to definition to /test

6eac56a

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 dismissed alyssawilk’s stale review via 6eac56a April 10, 2019 18:44

format

06e2d48

Signed-off-by: Dan Zhang <danzh@google.com>

remove loop

bb66ff9

Signed-off-by: Dan Zhang <danzh@google.com>

htuch suggested changes Apr 10, 2019

View reviewed changes

repokitteh-read-only Bot added the waiting label Apr 10, 2019

add const

cf62614

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only Bot removed the waiting label Apr 10, 2019

danzh1989 added 2 commits April 10, 2019 17:06

fix typo

37f18eb

Signed-off-by: Dan Zhang <danzh@google.com>

fix typo again

6177f57

Signed-off-by: Dan Zhang <danzh@google.com>

htuch approved these changes Apr 10, 2019

View reviewed changes

alyssawilk merged commit b81ff99 into envoyproxy:master Apr 11, 2019

Conversation

danzh2010 commented Apr 5, 2019

Uh oh!

danzh2010 commented Apr 5, 2019

Uh oh!

danzh2010 commented Apr 5, 2019

Uh oh!

repokitteh-read-only Bot commented Apr 5, 2019

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!

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!

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!

wu-bin left a comment

Choose a reason for hiding this comment

Uh oh!

danzh2010 commented Apr 8, 2019

Uh oh!

repokitteh-read-only Bot commented Apr 8, 2019

Uh oh!

danzh2010 commented Apr 8, 2019

Uh oh!

danzh2010 commented Apr 8, 2019

Uh oh!

repokitteh-read-only Bot commented Apr 8, 2019

Uh oh!

danzh2010 commented Apr 9, 2019

Uh oh!

repokitteh-read-only Bot commented Apr 9, 2019

Uh oh!

alyssawilk 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

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!

alyssawilk commented Apr 9, 2019

Uh oh!

alyssawilk 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!