quic: Connect client UDP sockets before usage by abeyad · Pull Request #35652 · envoyproxy/envoy

abeyad · 2024-08-09T20:54:44Z

This change does a few things:

Calls connect() on the QUIC UDP sockets, which allows the connection to receive async ICMP error messages on recvmsg calls throughout the lifetime of the connection.
Does not call bind() on a QUIC client UDP socket unless the local address is specified.
If the socket is already connected, call writev/send instead of sendmsg. Connected sockets should not specify a peer address and hence, writev/send is more appropriate.
Previously, when EnvoyQuicClientConnection did preferred server address probing, it specified the existing remote address, not the new preferred server address. This still worked because the sendmsg call took in the actual destination IP address. However, when calling connect(), this caused the socket to connect to the wrong address. This issue is fixed in this PR by creating a socket connection to the preferred server address.
Uses the loopback address for the local address, if one isn't specified, as it is only needed in createConnectionSocket to get an IoHandle. This avoids having to make the more expensive getifaddrs call, as mentioned in Network::Utility::getLocalAddress performance is bad in multi-threads #35137.
The connect() behavior is guarded by the runtime guard envoy.reloadable_features.quic_connect_client_udp_sockets, defaulted to true.

This change does a few things: 1. Calls `connect()` on the QUIC UDP sockets, which allows the socket to know if the peer address is routable earlier. 2. Does not call `bind()` on a QUIC client UDP socket. It's incorrect to do so. 3. If the socket is already connected, call writev/send instead of sendmsg. Connected sockets should not specify a peer address and hence, writev/send is more appropriate. Signed-off-by: Ali Beyad <abeyad@google.com>

source/common/quic/envoy_quic_utils.cc

danzh2010 · 2024-08-09T21:32:47Z

Calls connect() on the QUIC UDP sockets, which allows the socket to know if the peer address is routable earlier.

Not really, we saw connect() succeeded but following write cause "no route to host". The reason we want to connect() here is that connected UDP socket will surface async socket error received from ICMP messages.

Signed-off-by: Ali Beyad <abeyad@google.com>

abeyad · 2024-08-19T22:51:41Z

/assign @danzh2010

Signed-off-by: Ali Beyad <abeyad@google.com>

adisuissa · 2024-08-20T14:02:03Z

Alyssa is OOO this week. Assigning Ryan who has more QuIC context.
/assign @RyanTheOptimist

danzh2010

Thanks for working on this! Just a few nits!

source/common/quic/envoy_quic_utils.cc

source/common/quic/envoy_quic_client_connection.cc

source/common/quic/envoy_quic_utils.h

Signed-off-by: Ali Beyad <abeyad@google.com>

abeyad · 2024-08-22T22:51:33Z

@danzh2010 I pushed up new changes that I believe fixes all the tests. PTAL!

Signed-off-by: Ali Beyad <abeyad@google.com>

source/common/quic/envoy_quic_utils.cc

test/common/quic/envoy_quic_client_session_test.cc

Signed-off-by: Ali Beyad <abeyad@google.com>

abeyad

I also fixed some more broken tests. PTAL, thanks!

source/common/quic/envoy_quic_client_connection.cc

abeyad · 2024-08-23T21:02:14Z

/retest

Signed-off-by: Ali Beyad <abeyad@google.com>

abeyad · 2024-08-24T01:28:26Z

/retest

Signed-off-by: Ali Beyad <abeyad@google.com>

abeyad

Addressed yesterday's comments, thanks!

source/common/quic/envoy_quic_utils.cc

test/common/quic/envoy_quic_client_session_test.cc

Signed-off-by: Ali Beyad <abeyad@google.com>

abeyad · 2024-08-27T00:50:05Z

/retest

abeyad · 2024-08-27T17:45:06Z

@danzh2010 friendly ping, thanks!

test/common/quic/envoy_quic_client_session_test.cc

source/common/http/http3/conn_pool.cc

RyanTheOptimist

Out of curiosity, have we tried to use EM with this PR to make sure it works as expected?

Also, should we add a runtime guard since this changes behavior.

envoy/network/io_handle.h

source/common/network/utility.cc

source/common/quic/envoy_quic_client_connection.cc

RyanTheOptimist · 2024-08-27T19:49:41Z

source/common/quic/envoy_quic_utils.cc

+      Network::Socket::Type::Datagram,
+      // Use the loopback address if `local_addr` is null, to pass in the socket interface used to
+      // create the IoHandle, without having to make the more expensive `getifaddrs` call.
+      local_addr ? local_addr : getLoopbackAddress(peer_addr->ip()->version()), peer_addr,


I'm curious how this local address is used. In the server case, we need a local address 'cause that's what we listen. But in the normal POSIX client socket case, a caller typically does not specify a local address. Do you understand how this is used?

Unfortunately, the local address is needed so we can get a SocketInterface instance because it is needed to create an IoHandle instance. If we don't supply an address here, we segfault in creating the IoHandle in the SocketImpl constructor. Dan and I discussed this and we think the setup of SocketImpl isn't correct - we should be able to use a SocketInterface independent of an Address instance. But that's a change for a separate PR.

I see. That makes sense, but it makes me nervous. Have we tried running this PR against an external server (using HTTP/3)?

It should only affect upstream traffic. And if we hide it behind a runtime feature, it should be fine.

we have tested it on a mobile app connecting to a H/3 server, but for added safety, I added a runtime guard.

Excellent. Can you also mention that in the PR description and update the release notes?

Release notes was updated in https://github.com/envoyproxy/envoy/pull/35652/files#diff-6f9c718224c533c13c2c0ba1d5abaab86be9d0cc73808749c77934e9f9b0d5d0. Just updated the PR description.

source/common/quic/envoy_quic_utils.cc

Signed-off-by: Ali Beyad <abeyad@google.com>

abeyad · 2024-08-28T04:52:23Z

/retest

abeyad · 2024-08-28T13:44:37Z

/retest

danzh2010 · 2024-08-28T15:44:29Z

Calls connect() on the QUIC UDP sockets, which allows the connection to know if the socket is not routable upon the first recvmsg call via an async ICMP message.

"No route to host" can be reported synchronously during connect() as we observed in the tests. But we want connected UDP socket overall because it surfaces any async errors (from ICMP messages) via following recvmsg through out the connection life time.

RyanTheOptimist

Looking good!

RyanTheOptimist · 2024-08-28T16:06:10Z

source/common/quic/envoy_quic_utils.cc

+      Network::Socket::Type::Datagram,
+      // Use the loopback address if `local_addr` is null, to pass in the socket interface used to
+      // create the IoHandle, without having to make the more expensive `getifaddrs` call.
+      local_addr ? local_addr : getLoopbackAddress(peer_addr->ip()->version()), peer_addr,


I see. That makes sense, but it makes me nervous. Have we tried running this PR against an external server (using HTTP/3)?

abeyad · 2024-08-28T17:25:30Z

Calls connect() on the QUIC UDP sockets, which allows the connection to know if the socket is not routable upon the first recvmsg call via an async ICMP message.

"No route to host" can be reported synchronously during connect() as we observed in the tests. But we want connected UDP socket overall because it surfaces any async errors (from ICMP messages) via following recvmsg through out the connection life time.

Updated the PR description

Signed-off-by: Ali Beyad <abeyad@google.com>

repokitteh-read-only · 2024-08-28T19:55:04Z

CC @envoyproxy/runtime-guard-changes: FYI only for changes made to (source/common/runtime/runtime_features.cc).

🐱

Caused by: #35652 was synchronize by abeyad.

see: more, trace.

Signed-off-by: Ali Beyad <abeyad@google.com>

abeyad · 2024-08-29T13:36:18Z

/retest

danzh2010

Thanks for working on this!

abeyad · 2024-08-30T15:46:42Z

Thanks for the great reviews, @danzh2010 and @RyanTheOptimist !

abeyad requested review from kyessenov and soulxu as code owners August 9, 2024 20:54

abeyad marked this pull request as draft August 9, 2024 20:55

danzh2010 reviewed Aug 9, 2024

View reviewed changes

source/common/quic/envoy_quic_utils.cc Outdated Show resolved Hide resolved

source/common/quic/envoy_quic_utils.cc Outdated Show resolved Hide resolved

abeyad added 5 commits August 13, 2024 14:22

sendmsg

42e0e03

Signed-off-by: Ali Beyad <abeyad@google.com>

debugging

d07c44b

Signed-off-by: Ali Beyad <abeyad@google.com>

fix test

132bc4f

Signed-off-by: Ali Beyad <abeyad@google.com>

test

9fc7c01

Signed-off-by: Ali Beyad <abeyad@google.com>

fix

bdb9a1a

Signed-off-by: Ali Beyad <abeyad@google.com>

abeyad marked this pull request as ready for review August 19, 2024 22:51

repokitteh-read-only bot assigned danzh2010 Aug 19, 2024

abeyad changed the title ~~udp: Connect QUIC sockets before usage~~ quic: Connect UDP sockets before usage Aug 19, 2024

abeyad changed the title ~~quic: Connect UDP sockets before usage~~ quic: Connect client UDP sockets before usage Aug 19, 2024

fix

75b2d67

Signed-off-by: Ali Beyad <abeyad@google.com>

repokitteh-read-only bot assigned RyanTheOptimist Aug 20, 2024

danzh2010 reviewed Aug 20, 2024

View reviewed changes

source/common/quic/envoy_quic_utils.cc Outdated Show resolved Hide resolved

source/common/quic/envoy_quic_client_connection.cc Show resolved Hide resolved

source/common/quic/envoy_quic_utils.h Outdated Show resolved Hide resolved

abeyad added 2 commits August 20, 2024 14:59

comments

0a867bb

Signed-off-by: Ali Beyad <abeyad@google.com>

more fixes

46792b5

Signed-off-by: Ali Beyad <abeyad@google.com>

c

2190a6b

Signed-off-by: Ali Beyad <abeyad@google.com>

danzh2010 reviewed Aug 23, 2024

View reviewed changes

source/common/quic/envoy_quic_utils.cc Show resolved Hide resolved

test/common/quic/envoy_quic_client_session_test.cc Outdated Show resolved Hide resolved

test/common/quic/envoy_quic_client_session_test.cc Outdated Show resolved Hide resolved

abeyad added 3 commits August 23, 2024 18:44

fixes

e267fed

Signed-off-by: Ali Beyad <abeyad@google.com>

another

c7deb81

Signed-off-by: Ali Beyad <abeyad@google.com>

more

792f9c0

Signed-off-by: Ali Beyad <abeyad@google.com>

abeyad requested a review from mattklein123 as a code owner August 23, 2024 19:03

abeyad commented Aug 23, 2024

View reviewed changes

source/common/quic/envoy_quic_client_connection.cc Show resolved Hide resolved

Merge remote-tracking branch 'upstream/main' into udp_sockets

03e6f17

Signed-off-by: Ali Beyad <abeyad@google.com>

abeyad added 2 commits August 24, 2024 15:51

utils

f3893e7

Signed-off-by: Ali Beyad <abeyad@google.com>

udplistensocket

3af46d9

Signed-off-by: Ali Beyad <abeyad@google.com>

abeyad commented Aug 24, 2024

View reviewed changes

source/common/quic/envoy_quic_utils.cc Show resolved Hide resolved

test/common/quic/envoy_quic_client_session_test.cc Outdated Show resolved Hide resolved

fix

9ab784a

Signed-off-by: Ali Beyad <abeyad@google.com>

Merge remote-tracking branch 'upstream/main' into udp_sockets

a516dc5

danzh2010 reviewed Aug 27, 2024

View reviewed changes

test/common/quic/envoy_quic_client_session_test.cc Show resolved Hide resolved

source/common/http/http3/conn_pool.cc Show resolved Hide resolved

RyanTheOptimist reviewed Aug 27, 2024

View reviewed changes

abeyad added 2 commits August 27, 2024 20:04

add test

599930b

Signed-off-by: Ali Beyad <abeyad@google.com>

address comments

9d8f3cf

Signed-off-by: Ali Beyad <abeyad@google.com>

RyanTheOptimist reviewed Aug 28, 2024

View reviewed changes

address comments

a2de295

Signed-off-by: Ali Beyad <abeyad@google.com>

existing behavior if flag is off

2ee309e

Signed-off-by: Ali Beyad <abeyad@google.com>

RyanTheOptimist approved these changes Aug 28, 2024

View reviewed changes

danzh2010 approved these changes Aug 30, 2024

View reviewed changes

RyanTheOptimist merged commit 4ab5cfa into envoyproxy:main Aug 30, 2024

abeyad mentioned this pull request Aug 30, 2024

Network::Utility::getLocalAddress performance is bad in multi-threads #35137

Closed

phlax mentioned this pull request Sep 3, 2024

MetricsServiceIntegrationTest.BasicFlow (coverage) flakey #35945

Closed

abeyad mentioned this pull request Sep 4, 2024

quic: add QuicNetworkConnectivityObserver and EnvoyQuicNetworkObserverRegistry interfaces #35912

Merged

Conversation

abeyad commented Aug 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danzh2010 commented Aug 9, 2024

Uh oh!

abeyad commented Aug 19, 2024

Uh oh!

adisuissa commented Aug 20, 2024

Uh oh!

danzh2010 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

abeyad commented Aug 22, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

abeyad left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

abeyad commented Aug 23, 2024

Uh oh!

abeyad commented Aug 24, 2024

Uh oh!

abeyad left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

abeyad commented Aug 27, 2024

Uh oh!

abeyad commented Aug 27, 2024

Uh oh!

Uh oh!

Uh oh!

RyanTheOptimist left a comment

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!

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!

abeyad commented Aug 28, 2024

Uh oh!

abeyad commented Aug 28, 2024

Uh oh!

danzh2010 commented Aug 28, 2024

Uh oh!

RyanTheOptimist left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abeyad commented Aug 28, 2024

Uh oh!

repokitteh-read-only bot commented Aug 28, 2024

Uh oh!

abeyad commented Aug 29, 2024

abeyad commented Aug 9, 2024 •

edited

Loading