-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
h3/quic upstream connection: set local interface name on upstream connection #20644
Closed
Closed
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
8e55a8f
#20209 h3/quic upstream connection: set local interface name on upstr…
KfreeZ 83e152c
move the maysetinterface to createsocket
KfreeZ f9e2887
Merge branch 'envoyproxy:main' into h3IntfNm
KfreeZ 0046626
examples: Cleanups and shared services (#20392)
phlax da9b410
#20209 h3/quic upstream connection: set local interface name on upstr…
KfreeZ 68ce766
move the maysetinterface to createsocket
KfreeZ acca167
rebase too add DCO
KfreeZ File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
envoy/source/common/network/connection_impl.cc
Line 947 in e85654e
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.envoy/source/common/network/socket_impl.cc
Line 52 in e85654e
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
cc @mattklein123 @alyssawilk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! good to learn this.