Skip to content
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.

Sign up for GitHub

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

Jump to bottom

h3/quic upstream connection: set local interface name on upstream connection #20644

Closed
wants to merge 7 commits into from
Closed

h3/quic upstream connection: set local interface name on upstream connection #20644

Show file tree
Hide file tree
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 Apr 3, 2022
83e152c
move the maysetinterface to createsocket
KfreeZ Apr 12, 2022
f9e2887
Merge branch 'envoyproxy:main' into h3IntfNm
KfreeZ Apr 12, 2022
0046626
examples: Cleanups and shared services (#20392)
phlax Mar 23, 2022
da9b410
#20209 h3/quic upstream connection: set local interface name on upstr…
KfreeZ Apr 3, 2022
68ce766
move the maysetinterface to createsocket
KfreeZ Apr 12, 2022
acca167
rebase too add DCO
KfreeZ Apr 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions source/common/quic/envoy_quic_client_session.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,17 @@ void EnvoyQuicClientSession::OnTlsHandshakeComplete() {
streamInfo().upstreamInfo()->upstreamTiming().onUpstreamHandshakeComplete(
dispatcher_.timeSource());

const auto quicConn = static_cast<EnvoyQuicClientConnection*>(quicConnection());
if (quicConn != nullptr) {
quicConn->connectionSocket()->connectionInfoProvider().maybeSetInterfaceName(
quicConn->connectionSocket()->ioHandle());
Copy link
Member

@soulxu soulxu Apr 5, 2022 •
edited
Loading

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

socket_->connectionInfoProvider().maybeSetInterfaceName(ioHandle());

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

Api::SysCallIntResult SocketImpl::bind(Network::Address::InstanceConstSharedPtr address) {

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

connection_info_provider_(
std::make_shared<ConnectionInfoSetterImpl>(local_address, remote_address)) {

cc @mattklein123 @alyssawilk

Copy link
Contributor

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!

Copy link
Author

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const auto maybe_interface_name =
quicConn->connectionSocket()->connectionInfoProvider().interfaceName();
if (maybe_interface_name.has_value()) {
ENVOY_CONN_LOG_EVENT(debug, "conn_interface", "connected on local interface '{}'", *this,
maybe_interface_name.value());
}
}
raiseConnectionEvent(Network::ConnectionEvent::Connected);
}

Expand Down
1 change: 1 addition & 0 deletions test/common/quic/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,7 @@ envoy_cc_test(
"//source/common/quic:envoy_quic_client_session_lib",
"//source/common/quic:envoy_quic_connection_helper_lib",
"//source/extensions/quic/crypto_stream:envoy_quic_crypto_client_stream_lib",
"//test/mocks/api:api_mocks",
"//test/mocks/http:http_mocks",
"//test/mocks/http:stream_decoder_mock",
"//test/mocks/network:network_mocks",
Expand Down
22 changes: 22 additions & 0 deletions test/common/quic/envoy_quic_client_session_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#include "envoy/stats/stats_macros.h"

#include "source/common/api/os_sys_calls_impl.h"
#include "source/common/quic/codec_impl.h"
#include "source/common/quic/envoy_quic_alarm_factory.h"
#include "source/common/quic/envoy_quic_client_connection.h"
Expand All @@ -16,6 +17,7 @@
#include "test/mocks/stats/mocks.h"
#include "test/test_common/logging.h"
#include "test/test_common/simulated_time_system.h"
#include "test/mocks/api/mocks.h"

#include "gmock/gmock.h"
#include "gtest/gtest.h"
Expand All @@ -27,6 +29,7 @@
using testing::_;
using testing::Invoke;
using testing::Return;
using testing::AnyNumber;

namespace Envoy {
namespace Quic {
Expand Down Expand Up @@ -149,6 +152,7 @@ class EnvoyQuicClientSessionTest : public testing::TestWithParam<quic::ParsedQui
Http::Http3::CodecStats stats_;
envoy::config::core::v3::Http3ProtocolOptions http3_options_;
QuicHttpClientConnectionImpl http_connection_;
Api::OsSysCallsImpl os_sys_calls_actual_;
};

INSTANTIATE_TEST_SUITE_P(EnvoyQuicClientSessionTests, EnvoyQuicClientSessionTest,
Expand Down Expand Up @@ -359,5 +363,23 @@ TEST_P(EnvoyQuicClientSessionTest, GetRttAndCwnd) {
quic::kInitialCongestionWindow * quic::kDefaultTCPMSS);
}

TEST_P(EnvoyQuicClientSessionTest, ConnectedAfterHandshake) {
Api::MockOsSysCalls os_sys_calls;
if (quic_version_[0].UsesTls()) {
EXPECT_CALL(network_connection_callbacks_, onEvent(Network::ConnectionEvent::Connected));
EXPECT_CALL(os_sys_calls, supportsGetifaddrs())
.Times(AnyNumber())
.WillRepeatedly(
Invoke([this]() -> bool { return os_sys_calls_actual_.supportsGetifaddrs(); }));
EXPECT_CALL(os_sys_calls, getifaddrs(_))
.Times(AnyNumber())
.WillRepeatedly(
Invoke([this](Api::InterfaceAddressVector& vector) -> Api::SysCallIntResult {
return os_sys_calls_actual_.getifaddrs(vector);
}));
envoy_quic_session_.OnTlsHandshakeComplete();
}
}

} // namespace Quic
} // namespace Envoy