Add a new HappyEyeballsConnectionImpl class by RyanTheOptimist · Pull Request #17371 · envoyproxy/envoy

RyanTheOptimist · 2021-07-15T19:56:10Z

Add a new HappyEyeballsConnectionImpl class

This is an implementation of Network::ClientConnection which is constructed with a list of IP address instead of a single address. The class will potentially create an underlying connection for each address and use the first connection which actually succeeds.

Additional Description: N/A
Risk Level: None - Unused code
Testing: Unit tests
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A

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

RyanTheOptimist · 2021-07-15T19:56:44Z

/assign @RenjieTang

source/common/network/happy_eyeballs_connection_impl.h

source/common/network/happy_eyeballs_connection_impl.cc

source/common/network/happy_eyeballs_connection_impl.h

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

RyanTheOptimist · 2021-07-16T17:40:01Z

/assign @alyssawilk

alyssawilk

Huzzah for happy eyeballs! I a high level first pass but I think it's going to result in some churn so I'll do a more thorough pass after those changes land.
/wait

source/common/network/happy_eyeballs_connection_impl.h

source/common/network/happy_eyeballs_connection_impl.cc

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

source/common/network/happy_eyeballs_connection_impl.cc

test/common/network/happy_eyeballs_connection_impl_test.cc

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

alyssawilk

:-)

RyanTheOptimist · 2021-07-27T21:13:35Z

/retest

repokitteh-read-only · 2021-07-27T21:13:38Z

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

🐱

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

see: more, trace.

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

snowp

Thanks this looks great overall, just a few comments mostly around comments :)

snowp · 2021-07-23T15:51:59Z

source/common/network/happy_eyeballs_connection_impl.h

+  bool startSecureTransport() override;
+  absl::optional<std::chrono::milliseconds> lastRoundTripTime() const override;
+
+  // Simple getters which always delegate to the first connection.


First connection that we attempted to create? Or "first" connection still active (first in some list?)?

First connection in connection_. I've tweaked the comment accordingly.

snowp · 2021-07-23T15:52:49Z

source/common/network/happy_eyeballs_connection_impl.h

+ * underlying connection. Before the connection is established, however
+ * their behavior depends on their semantics. For anything which can result


Before the connection is established, however their behavior depends on their semantics. reads a tad bit awkward, maybe we can rephrase it or just change the punctuation?

Oh, good point. I think it's missing a comma after "however", but I've tweaked it slightly to make it less awkward (I hope!).

source/common/network/happy_eyeballs_connection_impl.cc

snowp · 2021-07-29T14:06:35Z

source/common/network/happy_eyeballs_connection_impl.cc

+  for (auto it = connections_.begin(); it != connections_.end();) {
+    if (it->get() == &(wrapper->connection())) {
+      (*it)->close(ConnectionCloseType::NoFlush);
+      it = connections_.erase(it);
+    } else {
+      ++it;
+    }
+  }


How come we remove the connection callbacks up higher in setUpFinalConnection when we close connections but not here?

So this method, cleanupWrapperAndConnection() is only called from onEvent(). The first thing that onEvent() does it remove the wrapper connection callbacks from the connection. So that means that by the time we're in the body of cleanupWrapperAndConnection() the callbacks have already been removed. Does that sound right?

I think that sounds right. Anything we can do to make this clearer in the code? From reading this I was wondering if this was a bug

Maybe extract function with a bool param to decide if we should remove the callbacks or just a comment somewhere?

Thanks for digging into this. Yeah, I agree this is not awesome so I'm happy to make it better... Just not quite sure what that should look like.

One thing we could do would be to move the current call to removeConnectionCallbacks() from the opt of onEvent() to the top of cleanupWrapperAndConnection(). Then we can call removeConnectionCallbacks for all connections in setUpFinalConnection(). I think this ends up with a net readability win since the remove calls are closer to the other related logic?

I've done this is the most recent commit. WDYT?

source/common/network/happy_eyeballs_connection_impl.cc

snowp · 2021-07-29T14:09:33Z

source/common/network/happy_eyeballs_connection_impl.h

+  // Called by the wrapper when the wrapped connection is above the write buffer
+  // high water mark.
+  void onAboveWriteBufferHighWatermark(ConnectionCallbacksWrapper* wrapper);
+
+  // Called by the wrapper when the wrapped connection is above the write buffer
+  // high water mark.
+  void onBelowWriteBufferLowWatermark(ConnectionCallbacksWrapper* wrapper);


These have NOT_REACHED macros in the impl, so are these not actually called at all? Comments seem a bit misleading

Fair question. I wasn't quite sure where to put this. Basically since we never write data to the connection while the wrapper is still set as the connection callbacks, it should be impossible for this method to be called. But it is reachable via the code on line 107. That being said, I've removed these methods and added the NOT_REACHED to the wrapper methods, along with a comment. Does that help?

snowp · 2021-07-29T14:11:54Z

source/common/upstream/upstream_impl.h

  static Network::ClientConnectionPtr
  createConnection(Event::Dispatcher& dispatcher, const ClusterInfo& cluster,
                   const Network::Address::InstanceConstSharedPtr& address,
+                   const std::vector<Network::Address::InstanceConstSharedPtr>& address_list,


What is the distinction between the address param and the address_list param? Could they be consolidated?

So this is a bit of a work in progress. We have another piece of work to actually populate this in production and I suspect we'll end up tweaking the API here, but I'm not sure. I'd be inclined to punt the consolidate to the follow up PR.

That being said, the idea here is that traditionally Envoy creates a connection to a single IP, of course. The Envoy HostImpl object has a single address which is used when creating a connection. This address comes from the host resolution which happened before the HostImpl was created. We're going to change this so that host resolution optionally ends up providing the full list of addresses to the HostImpl. We'd also like to run v4 (A records) and v6 (AAAA records) resolution in parallel and delivering the results as they arrive. This seems likely to require a tweak to the API involved, but we'll get to that when we get there :)

Ok, so to your specific question... Today a host always has an address and sometimes has an addressList (in the tests which call setAddressList). We could have this method simply take an address list and treat the first entry as the main address. The downside is that this would require an extra vector allocation in the case where the HostImpl itself has no address list. I don't love this idea but it's not the end of the world. Happy to do it if you think it's best?

It sounds a bit hairy so let's save that until we want to properly support a list of addresses, thanks for explaining!

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

RyanTheOptimist

Thanks for the review!

RyanTheOptimist · 2021-07-30T16:35:14Z

source/common/network/happy_eyeballs_connection_impl.h

+ * underlying connection. Before the connection is established, however
+ * their behavior depends on their semantics. For anything which can result


Oh, good point. I think it's missing a comma after "however", but I've tweaked it slightly to make it less awkward (I hope!).

RyanTheOptimist · 2021-07-30T16:36:08Z

source/common/network/happy_eyeballs_connection_impl.h

+  bool startSecureTransport() override;
+  absl::optional<std::chrono::milliseconds> lastRoundTripTime() const override;
+
+  // Simple getters which always delegate to the first connection.


First connection in connection_. I've tweaked the comment accordingly.

RyanTheOptimist · 2021-07-30T16:42:57Z

source/common/network/happy_eyeballs_connection_impl.h

+  // Called by the wrapper when the wrapped connection is above the write buffer
+  // high water mark.
+  void onAboveWriteBufferHighWatermark(ConnectionCallbacksWrapper* wrapper);
+
+  // Called by the wrapper when the wrapped connection is above the write buffer
+  // high water mark.
+  void onBelowWriteBufferLowWatermark(ConnectionCallbacksWrapper* wrapper);


Fair question. I wasn't quite sure where to put this. Basically since we never write data to the connection while the wrapper is still set as the connection callbacks, it should be impossible for this method to be called. But it is reachable via the code on line 107. That being said, I've removed these methods and added the NOT_REACHED to the wrapper methods, along with a comment. Does that help?

source/common/network/happy_eyeballs_connection_impl.cc

RyanTheOptimist · 2021-07-30T16:55:08Z

source/common/network/happy_eyeballs_connection_impl.cc

+  for (auto it = connections_.begin(); it != connections_.end();) {
+    if (it->get() == &(wrapper->connection())) {
+      (*it)->close(ConnectionCloseType::NoFlush);
+      it = connections_.erase(it);
+    } else {
+      ++it;
+    }
+  }


So this method, cleanupWrapperAndConnection() is only called from onEvent(). The first thing that onEvent() does it remove the wrapper connection callbacks from the connection. So that means that by the time we're in the body of cleanupWrapperAndConnection() the callbacks have already been removed. Does that sound right?

source/common/network/happy_eyeballs_connection_impl.cc

RyanTheOptimist · 2021-07-30T17:19:52Z

source/common/upstream/upstream_impl.h

  static Network::ClientConnectionPtr
  createConnection(Event::Dispatcher& dispatcher, const ClusterInfo& cluster,
                   const Network::Address::InstanceConstSharedPtr& address,
+                   const std::vector<Network::Address::InstanceConstSharedPtr>& address_list,


So this is a bit of a work in progress. We have another piece of work to actually populate this in production and I suspect we'll end up tweaking the API here, but I'm not sure. I'd be inclined to punt the consolidate to the follow up PR.

That being said, the idea here is that traditionally Envoy creates a connection to a single IP, of course. The Envoy HostImpl object has a single address which is used when creating a connection. This address comes from the host resolution which happened before the HostImpl was created. We're going to change this so that host resolution optionally ends up providing the full list of addresses to the HostImpl. We'd also like to run v4 (A records) and v6 (AAAA records) resolution in parallel and delivering the results as they arrive. This seems likely to require a tweak to the API involved, but we'll get to that when we get there :)

Ok, so to your specific question... Today a host always has an address and sometimes has an addressList (in the tests which call setAddressList). We could have this method simply take an address list and treat the first entry as the main address. The downside is that this would require an extra vector allocation in the case where the HostImpl itself has no address list. I don't love this idea but it's not the end of the world. Happy to do it if you think it's best?

snowp

Thanks, at this point the main thing I'm missing is making it clear about when we remove the connection callbacks

source/common/network/happy_eyeballs_connection_impl.cc

snowp · 2021-08-02T21:02:01Z

source/common/network/happy_eyeballs_connection_impl.cc

+  for (auto it = connections_.begin(); it != connections_.end();) {
+    if (it->get() == &(wrapper->connection())) {
+      (*it)->close(ConnectionCloseType::NoFlush);
+      it = connections_.erase(it);
+    } else {
+      ++it;
+    }
+  }


I think that sounds right. Anything we can do to make this clearer in the code? From reading this I was wondering if this was a bug

Maybe extract function with a bool param to decide if we should remove the callbacks or just a comment somewhere?

source/common/network/happy_eyeballs_connection_impl.cc

snowp · 2021-08-02T21:03:32Z

source/common/upstream/upstream_impl.h

  static Network::ClientConnectionPtr
  createConnection(Event::Dispatcher& dispatcher, const ClusterInfo& cluster,
                   const Network::Address::InstanceConstSharedPtr& address,
+                   const std::vector<Network::Address::InstanceConstSharedPtr>& address_list,


It sounds a bit hairy so let's save that until we want to properly support a list of addresses, thanks for explaining!

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

RyanTheOptimist

Thanks for the insightful comments.

RyanTheOptimist · 2021-08-02T22:19:41Z

source/common/network/happy_eyeballs_connection_impl.cc

+  for (auto it = connections_.begin(); it != connections_.end();) {
+    if (it->get() == &(wrapper->connection())) {
+      (*it)->close(ConnectionCloseType::NoFlush);
+      it = connections_.erase(it);
+    } else {
+      ++it;
+    }
+  }


Thanks for digging into this. Yeah, I agree this is not awesome so I'm happy to make it better... Just not quite sure what that should look like.

One thing we could do would be to move the current call to removeConnectionCallbacks() from the opt of onEvent() to the top of cleanupWrapperAndConnection(). Then we can call removeConnectionCallbacks for all connections in setUpFinalConnection(). I think this ends up with a net readability win since the remove calls are closer to the other related logic?

I've done this is the most recent commit. WDYT?

RyanTheOptimist · 2021-08-03T01:58:47Z

/retest

repokitteh-read-only · 2021-08-03T01:58:50Z

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

🐱

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

see: more, trace.

alyssawilk · 2021-08-03T15:58:20Z

sorry for coverage you may need to pull main

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

snowp

Thanks!

…bridge-stream * upstream/main: stream info: add upstream connection id (envoyproxy#17512) runtime: removing require_ocsp_response_for_must_staple_certs (envoyproxy#17541) tooling: Fixes/cleanups/tests for `rst_checks.py` (envoyproxy#17589) Add a new HappyEyeballsConnectionImpl class (envoyproxy#17371) hcm/listener_manager: remove deprecated v2 config handling. (envoyproxy#17586) tools: simplify bazel deps query (envoyproxy#17599) tooling: Move tempdir/cleanup to base runner class (envoyproxy#17590) tools: fix missing format strings (envoyproxy#17600) tooling: Add `@envoy` prefix in `envoy_py_` macros (envoyproxy#17588) stream info: add attempt count for HTTP requests and TCP proxy (envoyproxy#17583) Signed-off-by: Garrett Bourg <bourg@squareup.com>

Add a new HappyEyeballsConnectionImpl class This is an implementation of Network::ClientConnection which is constructed with a list of IP address instead of a single address. The class will potentially create an underlying connection for each address and use the first connection which actually succeeds. Additional Description: N/A Risk Level: None - Unused code Testing: Unit tests Docs Changes: N/A Release Notes: N/A Platform Specific Features: N/A Signed-off-by: Ryan Hamilton <rch@google.com>

RyanTheOptimist added 20 commits July 13, 2021 20:22

Add a new HappyEyeballsConnectionImpl

0e8c581

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

Cleanup

2d0bcdc

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

WIP

7d9f86b

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

More WIP

6ee6483

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

more wip

70d5cce

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

WIP

608cde5

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

unit tests pass

3650ba1

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

More tests

d0e5230

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

All unit tests

cf2a080

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

Cleanup

ae4b120

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

Cleanup

866cbd1

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

All but close()!

fa98c2b

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

Close tests

f7c14b6

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

setBufferLimits

eca1009

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

setBufferLimits

e6f2c19

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

Cleanup

d038acb

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

Working

3b7a5b1

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

Cleanup

0ac5d91

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

Format

9f6ac88

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

tidy

9a5acf3

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

repokitteh-read-only bot assigned RenjieTang Jul 15, 2021

RenjieTang reviewed Jul 15, 2021

View reviewed changes

RyanTheOptimist added 3 commits July 16, 2021 02:06

Comments from Renjie

afe4c6a

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

OptRef

935ce6b

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

Spelling

12cf110

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

RenjieTang previously approved these changes Jul 16, 2021

View reviewed changes

repokitteh-read-only bot assigned alyssawilk Jul 16, 2021

alyssawilk reviewed Jul 19, 2021

View reviewed changes

RyanTheOptimist added 2 commits July 27, 2021 20:22

Format

ae43032

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

went -> when

47e34e4

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

alyssawilk reviewed Jul 27, 2021

View reviewed changes

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

alyssawilk reviewed Jul 27, 2021

View reviewed changes

test/common/network/happy_eyeballs_connection_impl_test.cc Show resolved Hide resolved

Better comments

73cce3b

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

alyssawilk previously approved these changes Jul 27, 2021

View reviewed changes

Grammar! Yarrrrrr!

9b0ad9e

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

RyanTheOptimist dismissed alyssawilk’s stale review via 9b0ad9e July 27, 2021 20:49

alyssawilk previously approved these changes Jul 27, 2021

View reviewed changes

Merge branch 'main' into HappyEyeballs

34458f3

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

snowp reviewed Jul 29, 2021

View reviewed changes

RyanTheOptimist added 2 commits July 30, 2021 17:20

Fix comments from snow

88ba996

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

Spelling

52aee63

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

RyanTheOptimist dismissed alyssawilk’s stale review via 52aee63 July 30, 2021 17:22

RyanTheOptimist commented Jul 30, 2021

View reviewed changes

snowp reviewed Aug 2, 2021

View reviewed changes

Better location for removeConnectionCallbacks

811309a

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

RyanTheOptimist commented Aug 2, 2021

View reviewed changes

Merge branch 'main' into HappyEyeballs

c061de4

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

snowp approved these changes Aug 4, 2021

View reviewed changes

alyssawilk merged commit a19a9e0 into envoyproxy:main Aug 5, 2021

alyssawilk mentioned this pull request Oct 4, 2021

Happy eyeballs support for Dynamic Forward Proxy (and possibly others) #16309

Closed

		* underlying connection. Before the connection is established, however
		* their behavior depends on their semantics. For anything which can result

Conversation

RyanTheOptimist commented Jul 15, 2021

Uh oh!

RyanTheOptimist commented Jul 15, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RyanTheOptimist commented Jul 16, 2021

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

RyanTheOptimist commented Jul 27, 2021

Uh oh!

repokitteh-read-only bot commented Jul 27, 2021

Uh oh!

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

Uh oh!

Uh oh!

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!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!