h3/quic upstream connection: set local interface name on upstream connection #20644
Conversation
|
Hi @KfreeZ, welcome and thank you for your contribution. We will try to review your Pull Request as quickly as possible. In the meantime, please take a look at the contribution guidelines if you have not done so already. |
| const auto quicConn = static_cast<EnvoyQuicClientConnection*>(quicConnection()); | ||
| if (quicConn != nullptr) { | ||
| quicConn->connectionSocket()->connectionInfoProvider().maybeSetInterfaceName( | ||
| quicConn->connectionSocket()->ioHandle()); |
There was a problem hiding this comment.
Setting the interface name in the session layer doesn't seem like the right place. It should be done by the connection layer if possible.
Further thinking, since we set the interface name both for TCP and QUIC now, the TCP is done by #19758. at
So it would be great to set the interface name in one place.
This also can avoid some bugs due to copying the connection around.
I guess we only can get the interface name after bind the socket (correct me if I'm wrong), so one place what I'm thinking is in the SocketImpl::bind method, set the interface name after the bind.
Also SocketImpl is where we create connectionInfoProvider, it seems the right place to consider.
envoy/source/common/network/socket_impl.cc
Lines 25 to 26 in e85654e
There was a problem hiding this comment.
One can get network interfaces from kernel at any time but to pick one that matches the connection's local address which is retrieved via getsockname() we need to call maybeSetInterfaceName() on a bound socket.
However, a client socket doesn't necessarily always bind() before write(). For TCP, if the local address is not specified, kernel will pick the right network interface and a port for the connection. For QUIC, right now QUIC client connection always calls bind() because we will pick a network interface according to the remote address type if the local address is not provided via getLocalAddress(), see createConnectionSocket() in source/common/quic/envoy_quic_utils.cc. So it is infeasible to unify the place for TCP and QUIC to call maybeSetInterfaceName(). For QUIC, the most feasible place is after bind() in createConnectionSocket() if you want a quick fix.
Admittedly QUIC client socket creation should align with TCP, but even in that case, QUIC doesn't need to wait till handshake finish to call maybeSetInterfaceName() but after the connection receives the first packet. To do so, you will need to rewrite createConnectionSocket() and override some QuicConnection packet processing callbacks. Feel free to chose either approach.
BTW, thanks for contributing to Envoy H3!
There was a problem hiding this comment.
thanks for your comments, will look into it, your guide is very improtant for me as a beginer for H3 and envoy, apperciated.
There was a problem hiding this comment.
However, a client socket doesn't necessarily always bind() before write(). For TCP, if the local address is not specified, kernel will pick the right network interface and a port for the connection.
Thanks! good to learn this.
|
Plz fix the DCO and other CIs. |
|
It looks like there are still problems with CI. Please fix. |
|
/wait |
Signed-off-by: kfreez <kfreec@gmail.com>
Fix #20039 This provides a few cleanup for the example docker (compose) recipes - rationalize shared images - add hashed pip requirements - remove debug logging - remove superfluous docker compose config There are a couple of docker recipes separated out for flask and a traceable envoy service. This should also make runing the examples slightly faster/more efficient with docker resources as there is better sharing between them - we can probably rationalize this a little further for some of the other images/containers used in the sandboxes. Fixing the pip requirements should, moving forward, make the pip deps much less likely to break on release branches, and brings them into dependabot checking. I removed the debug logging as i think these are designed to be beginner examples and for ci brevity Signed-off-by: Ryan Northey <ryan@synca.io> Signed-off-by: kfreez <kfreec@gmail.com>
…eam connection Signed-off-by: kfreez <kfreec@gmail.com>
Signed-off-by: kfreez <kfreec@gmail.com>
repokitteh-read-only
bot
added
the
deps
|
CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to |
Signed-off-by: kfreez <kfreec@gmail.com>
|
get messing after pushing new commit/merge and DCO fixing, will open a new PR. |
Commit Message: when QUIC client complete TLS. invoke syscall to get the interface name
Additional Description: this is a following work for #19336
Risk Level: midium
Testing: yes
Docs Changes: no
Release Notes: no
Platform Specific Features: no
[Optional Runtime guard:]
[Optional Fixes #Issue] #20209
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]