upstream: Implement Happy Eyeballs address sorting by RyanTheOptimist · Pull Request #18906 · envoyproxy/envoy

RyanTheOptimist · 2021-11-04T23:51:07Z

upstream: Implement Happy Eyeballs address sorting with address families interleaved, as per Section 4 of RFC 8305, Happy Eyeballs v2. Sorting as per Section 6 of RFC6724 already happens in ares_getaddrinfo() and is not part of this.

Risk Level: Low - Happy Eyeballs is not yet used
Testing: Unit tests
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A
Signed-off-by: Ryan Hamilton rch@google.com

Signed-off-by: Ryan Hamilton <rch@google.com>

RyanTheOptimist · 2021-11-04T23:51:37Z

/assign @RenjieTang

RenjieTang · 2021-11-04T23:59:57Z

test/common/network/happy_eyeballs_connection_impl_test.cc

+                                                               ip_v6_3, ip_v4_3, ip_v6_4, ip_v4_4};
+  EXPECT_EQ(interleaved3, HappyEyeballsConnectionImpl::sortAddresses(mixed));
+  for (size_t i = 0; i < interleaved3.size(); ++i) {
+    std::cout << i << " " << interleaved3[i]->asString() << "\n";


Whoops! Done.

RenjieTang · 2021-11-05T00:02:00Z

source/common/network/happy_eyeballs_connection_impl.cc

+
+    // Bubble the address up to the current position.
+    for (size_t i = next; i > current; i--) {
+      using std::swap;


Since std::swap is only called once here, would it be cleaner to just call std::swap without the using clause?

Apparently not, as per go/using-std-swap. Who knew?! (Believe it or not, there is a potential behavior difference between the two)

Amazing... Thanks for providing the context!

Signed-off-by: Ryan Hamilton <rch@google.com>

RenjieTang · 2021-11-05T00:12:30Z

source/common/network/happy_eyeballs_connection_impl.cc

+
+    // Bubble the address up to the current position.
+    for (size_t i = next; i > current; i--) {
+      using std::swap;


Amazing... Thanks for providing the context!

RyanTheOptimist · 2021-11-05T00:20:00Z

/assign @junr03

RyanTheOptimist · 2021-11-05T03:27:45Z

/retest

repokitteh-read-only · 2021-11-05T03:27:49Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #18906 (comment) was created by @RyanTheOptimist.

see: more, trace.

source/common/network/happy_eyeballs_connection_impl.cc

davinci26

some drive-by c++ comments. If we commit to using std libraries we could make the code a bit shorter but I don't have a strong opinion. Up to you if you want to change it or not

source/common/network/happy_eyeballs_connection_impl.cc

Signed-off-by: Ryan Hamilton <rch@google.com>

davinci26

LGTM

RyanTheOptimist · 2021-11-09T20:31:57Z

/assign @alyssawilk

junr03 · 2021-11-09T22:08:25Z

source/common/network/happy_eyeballs_connection_impl.cc

    const ConnectionSocket::OptionsSharedPtr options)
-    : id_(ConnectionImpl::next_global_id_++), dispatcher_(dispatcher), address_list_(address_list),
+    : id_(ConnectionImpl::next_global_id_++), dispatcher_(dispatcher),
+      address_list_(sortAddresses(address_list)),


food for thought: do we care about making this behavior configurable? i.e., opt-out via config?

Hm. I'm inclined to say "no", or at least "not unless someone asks for it". Happy Eyeballs itself is already configurable and I can't really imagine why this would be something we'd want to disable if Happy Eyeballs were in use. That being said, if someone ask for it then we could definitely make it configurable. WDYT?

source/common/network/happy_eyeballs_connection_impl.h

junr03 · 2021-11-09T22:10:54Z

source/common/network/happy_eyeballs_connection_impl.h

+  // Returns a new vector containing the contents of |address_list| sorted
+  // with address families interleaved, as per Section 4 of RFC 8305, Happy
+  // Eyeballs v2. It is assumed that The list must already sorted as per
+  // Section 6 of RFC6724, which happens in ares_getaddrinfo().


does the Apple implementation also do this? If not, then we would be breaking this requirement for iOS

Yes, it does, according to what I read online.

maybe comment as such.
is there any way to CHECK on this, in case there are other DNS impls that might not realize the constraint?

I added a comment about Apple DNS. Good idea. As for detecting that this sorting might be missing, I'm not sure how we would do that short of implementing the sorting rules and checking that addr[i] < addr[i+1] but that's non-trivial. I'm open to suggestions though!

Co-authored-by: Jose Ulises Nino Rivera <junr03@users.noreply.github.com> Signed-off-by: Ryan Hamilton <ryan@optimism.cc>

alyssawilk

Awesome, thanks so much for implementing this, and sorting out we don't need to do all those kernel calls!
/wait

alyssawilk · 2021-11-10T18:07:49Z

source/common/network/happy_eyeballs_connection_impl.h

+  // Returns a new vector containing the contents of |address_list| sorted
+  // with address families interleaved, as per Section 4 of RFC 8305, Happy
+  // Eyeballs v2. It is assumed that The list must already sorted as per
+  // Section 6 of RFC6724, which happens in ares_getaddrinfo().


maybe comment as such.
is there any way to CHECK on this, in case there are other DNS impls that might not realize the constraint?

alyssawilk · 2021-11-10T18:08:55Z

source/common/network/happy_eyeballs_connection_impl.cc

+  for (auto current = address_list.begin(); current != address_list.end(); ++current) {
+    // Find the first address with a different address family than the current address.
+    auto it = std::find_if(current, address_list.end(), [&](const auto& val) {
+      return (val->type() != Address::Type::Ip || (*current)->type() != Address::Type::Ip ||


kinda curious, does the spec say what to do with non IP addresses? Should we just remove those if we see any?

The specs refer specifically to IP addresses. I don't think they even consider non-IP addresses. The other address types are Pipe and EnvoyInternal. Are these valid addresses for passing into a Network::ClientConnection? From spelunking down in the code, it kinda looks like we end up in SocketInterfaceImpl::socket which seems to support non-IP addresses so I think we need to keep them in the list, but maybe we don't need them if we're doing Happy Eyeballs?

source/common/network/happy_eyeballs_connection_impl.cc

Signed-off-by: Ryan Hamilton <rch@google.com>

RyanTheOptimist · 2021-11-11T05:33:22Z

/retest

repokitteh-read-only · 2021-11-11T05:33:26Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #18906 (comment) was created by @RyanTheOptimist.

see: more, trace.

RyanTheOptimist · 2021-11-11T14:41:49Z

/retest

repokitteh-read-only · 2021-11-11T14:41:52Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #18906 (comment) was created by @RyanTheOptimist.

see: more, trace.

alyssawilk

Yeah this looks better thanks. Throwing over to @ggreenway for non-google pass

moderation · 2021-11-15T18:37:39Z

Wondering if this PR addresses #18572 and streams results (or was this fixed in #18464) /cc @junr03

RyanTheOptimist · 2021-11-15T18:47:23Z

Wondering if this PR addresses #18572 and streams results (or was this fixed in #18464) /cc @junr03

I don't believe that either this PR nor #18464 address the concern in #18572. That issue is motivated to start a connection attempt as soon as the first DNS response arrives. For example, if we send an A request and an AAAA request we currently wait until both responses have arrived. But we could start an attempt to the first address in the first response. This current PR merely sorts the list of addresses but does not change the timing.

ggreenway

LGTM. One minor grammar nit.

/wait

ggreenway · 2021-11-15T20:02:18Z

source/common/network/happy_eyeballs_connection_impl.h


+  // Returns a new vector containing the contents of |address_list| sorted
+  // with address families interleaved, as per Section 4 of RFC 8305, Happy
+  // Eyeballs v2. It is assumed that the list must already sorted as per


"list must already BE sorted...

Done. PTAL?

Signed-off-by: Ryan Hamilton <rch@google.com>

RyanTheOptimist · 2021-11-16T05:28:11Z

/retest

repokitteh-read-only · 2021-11-16T05:28:15Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #18906 (comment) was created by @RyanTheOptimist.

see: more, trace.

upstream: Implement Happy Eyeballs address sorting

7c4538c

Signed-off-by: Ryan Hamilton <rch@google.com>

repokitteh-read-only bot assigned RenjieTang Nov 4, 2021

RenjieTang reviewed Nov 5, 2021

View reviewed changes

no logging

e03104a

Signed-off-by: Ryan Hamilton <rch@google.com>

RenjieTang previously approved these changes Nov 5, 2021

View reviewed changes

repokitteh-read-only bot assigned junr03 Nov 5, 2021

davinci26 reviewed Nov 8, 2021

View reviewed changes

source/common/network/happy_eyeballs_connection_impl.cc Outdated Show resolved Hide resolved

davinci26 reviewed Nov 8, 2021

View reviewed changes

source/common/network/happy_eyeballs_connection_impl.cc Outdated Show resolved Hide resolved

RyanTheOptimist added 3 commits November 9, 2021 20:01

WIP

ef769a6

Signed-off-by: Ryan Hamilton <rch@google.com>

Cleanup

3f3a4ac

Signed-off-by: Ryan Hamilton <rch@google.com>

Format

0a3f5da

Signed-off-by: Ryan Hamilton <rch@google.com>

RyanTheOptimist dismissed RenjieTang’s stale review via 0a3f5da November 9, 2021 20:12

davinci26 previously approved these changes Nov 9, 2021

View reviewed changes

repokitteh-read-only bot assigned alyssawilk Nov 9, 2021

junr03 reviewed Nov 9, 2021

View reviewed changes

RyanTheOptimist dismissed davinci26’s stale review via 8582c0d November 9, 2021 22:26

Update source/common/network/happy_eyeballs_connection_impl.h

eb86577

Co-authored-by: Jose Ulises Nino Rivera <junr03@users.noreply.github.com> Signed-off-by: Ryan Hamilton <ryan@optimism.cc>

RyanTheOptimist force-pushed the sortAddresses branch from 8582c0d to eb86577 Compare November 9, 2021 22:31

alyssawilk reviewed Nov 10, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Nov 10, 2021

Rewrite sortAddresses to be O(N)

0561593

Signed-off-by: Ryan Hamilton <rch@google.com>

repokitteh-read-only bot removed the waiting label Nov 11, 2021

format

9c84f2b

Signed-off-by: Ryan Hamilton <rch@google.com>

alyssawilk previously approved these changes Nov 11, 2021

View reviewed changes

alyssawilk assigned ggreenway Nov 11, 2021

ggreenway requested changes Nov 15, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Nov 15, 2021

RyanTheOptimist added 2 commits November 16, 2021 01:48

Merge branch 'main' into sortAddresses

8fd62a2

Signed-off-by: Ryan Hamilton <rch@google.com>

typo

545928d

Signed-off-by: Ryan Hamilton <rch@google.com>

RyanTheOptimist dismissed alyssawilk’s stale review via 545928d November 16, 2021 01:50

repokitteh-read-only bot removed the waiting label Nov 16, 2021

ggreenway approved these changes Nov 16, 2021

View reviewed changes

RyanTheOptimist merged commit d981914 into envoyproxy:main Nov 16, 2021

Conversation

RyanTheOptimist commented Nov 4, 2021

Uh oh!

RyanTheOptimist commented Nov 4, 2021

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!

RyanTheOptimist commented Nov 5, 2021

Uh oh!

RyanTheOptimist commented Nov 5, 2021

Uh oh!

repokitteh-read-only bot commented Nov 5, 2021

Uh oh!

Uh oh!

davinci26 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

davinci26 left a comment

Choose a reason for hiding this comment

Uh oh!

RyanTheOptimist commented Nov 9, 2021

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!

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

Uh oh!

RyanTheOptimist commented Nov 11, 2021

Uh oh!

repokitteh-read-only bot commented Nov 11, 2021

Uh oh!

RyanTheOptimist commented Nov 11, 2021

Uh oh!

repokitteh-read-only bot commented Nov 11, 2021

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

moderation commented Nov 15, 2021

Uh oh!

RyanTheOptimist commented Nov 15, 2021

Uh oh!

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