Skip to content
Merged
2 changes: 2 additions & 0 deletions contrib/vcl/source/vcl_io_handle.h
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,8 @@ class VclIoHandle : public Envoy::Network::IoHandle,
void resetFileEvents() override;
IoHandlePtr duplicate() override;

absl::optional<std::string> interfaceName() override { return absl::nullopt; }

void cb(uint32_t events) { cb_(events); }
void setCb(Event::FileReadyCb cb) { cb_ = cb; }
void updateEvents(uint32_t events);
Expand Down
5 changes: 5 additions & 0 deletions envoy/network/io_handle.h
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,11 @@ class IoHandle {
* returned.
*/
virtual absl::optional<std::chrono::milliseconds> lastRoundTripTime() PURE;

/**
* @return the interface name for the socket, if the OS supports it. Otherwise, absl::nullopt.
*/
virtual absl::optional<std::string> interfaceName() PURE;
};

using IoHandlePtr = std::unique_ptr<IoHandle>;
Expand Down
49 changes: 49 additions & 0 deletions source/common/network/io_socket_handle_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -576,5 +576,54 @@ absl::optional<std::chrono::milliseconds> IoSocketHandleImpl::lastRoundTripTime(
return std::chrono::duration_cast<std::chrono::milliseconds>(info.tcpi_rtt);
}

absl::optional<std::string> IoSocketHandleImpl::interfaceName() {
auto& os_syscalls_singleton = Api::OsSysCallsSingleton::get();
if (!os_syscalls_singleton.supportsGetifaddrs()) {
return absl::nullopt;
}

Address::InstanceConstSharedPtr socket_address = localAddress();
if (!socket_address || socket_address->type() != Address::Type::Ip) {
return absl::nullopt;
}

Api::InterfaceAddressVector interface_addresses{};
const Api::SysCallIntResult rc = os_syscalls_singleton.getifaddrs(interface_addresses);
RELEASE_ASSERT(!rc.return_value_, fmt::format("getiffaddrs error: {}", rc.errno_));

absl::optional<std::string> selected_interface_name{};
for (const auto& interface_address : interface_addresses) {
if (!interface_address.interface_addr_) {
continue;
}

if (socket_address->ip()->version() == interface_address.interface_addr_->ip()->version()) {
// Compare address _without port_.
// TODO: create common addressAsStringWithoutPort method to simplify code here.
absl::uint128 socket_address_value;
absl::uint128 interface_address_value;
switch (socket_address->ip()->version()) {
case Address::IpVersion::v4:
socket_address_value = socket_address->ip()->ipv4()->address();
interface_address_value = interface_address.interface_addr_->ip()->ipv4()->address();
break;
case Address::IpVersion::v6:
socket_address_value = socket_address->ip()->ipv6()->address();
interface_address_value = interface_address.interface_addr_->ip()->ipv6()->address();
break;
default:
ENVOY_BUG(false, fmt::format("unexpected IP family {}", socket_address->ip()->version()));
}

if (socket_address_value == interface_address_value) {
selected_interface_name = interface_address.interface_name_;
break;
}
}
}

return selected_interface_name;
}

} // namespace Network
} // namespace Envoy
1 change: 1 addition & 0 deletions source/common/network/io_socket_handle_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ class IoSocketHandleImpl : public IoHandle, protected Logger::Loggable<Logger::I

Api::SysCallIntResult shutdown(int how) override;
absl::optional<std::chrono::milliseconds> lastRoundTripTime() override;
absl::optional<std::string> interfaceName() override;

protected:
// Converts a SysCallSizeResult to IoCallUint64Result.
Expand Down
1 change: 1 addition & 0 deletions source/common/quic/quic_io_handle_wrapper.h
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ class QuicIoHandleWrapper : public Network::IoHandle {
void activateFileEvents(uint32_t events) override { io_handle_.activateFileEvents(events); }
void enableFileEvents(uint32_t events) override { io_handle_.enableFileEvents(events); }
void resetFileEvents() override { return io_handle_.resetFileEvents(); };
absl::optional<std::string> interfaceName() override { return io_handle_.interfaceName(); }

Api::SysCallIntResult shutdown(int how) override { return io_handle_.shutdown(how); }
absl::optional<std::chrono::milliseconds> lastRoundTripTime() override { return {}; }
Expand Down
1 change: 1 addition & 0 deletions source/extensions/io_socket/user_space/io_handle_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ class IoHandleImpl final : public Network::IoHandle,

Api::SysCallIntResult shutdown(int how) override;
absl::optional<std::chrono::milliseconds> lastRoundTripTime() override { return absl::nullopt; }
absl::optional<std::string> interfaceName() override { return absl::nullopt; }
Copy link
Copy Markdown
Member Author

@junr03 junr03 Dec 9, 2021

Choose a reason for hiding this comment

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

@florincoras @rojkov I believe the same implementation that I wrote (above) for the IoSocketHandleImpl would work here. Do you think it would be worth it to move the implementation to the virtual class? Given that the implementation is based on localAddress() and the os syscalls available it doesn't seem like it would necessarily need to be outside of the virtual class.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The virtual class is supposed to be just an interface enforcing concrete implementations at compile-time like this trivial stub. Otherwise there's a chance of calling a suboptimal implementation (which e.g. checks os syscall availability first). And in this particular case it could be even an error if something called a user space handle's interfaceName().

But I agree the implementation should be shared somehow where it makes sense. In #19082 I'll need the same code. Even more to that - I need the same implementation for localAddress() and peerAddress(). Maybe utility functions would suffice?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Agreed with @rojkov. It won't also work for vcl because the interfaces are in another user space process (vpp), so not available via syscalls.

The idea might not be too bright but one could argue that there should've been a syscall to get a socket's localAddress(), peerAddress() and the interfaceName for a localAddress(), so one option would be to add these utility functions to syscall interfaces. The downside however is that we'll pollute those with non-posix apis.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@rojkov @florincoras thanks for your thoughtful responses. It makes sense to me. I also realize how little experience I have of this line of work. If you dont think that the current addition to the interface is egregious, do you all mind if I implement here as is and let you all handle this when/if you tackle moving localAddress() and peerAddress around?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't, I'm fine with it as it is for now.


void setWatermarks(uint32_t watermark) { pending_received_data_.setWatermarks(watermark); }
void onBelowLowWatermark() {
Expand Down
1 change: 1 addition & 0 deletions test/common/network/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -404,6 +404,7 @@ envoy_cc_test(
"//source/common/network:address_lib",
"//test/mocks/api:api_mocks",
"//test/test_common:threadsafe_singleton_injector_lib",
"//test/test_common:utility_lib",
],
)

Expand Down
77 changes: 76 additions & 1 deletion test/common/network/io_socket_handle_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include "test/test_common/environment.h"
#include "test/test_common/network_utility.h"
#include "test/test_common/threadsafe_singleton_injector.h"
#include "test/test_common/utility.h"

#include "gmock/gmock.h"
#include "gtest/gtest.h"
Expand All @@ -23,7 +24,7 @@ namespace Envoy {
namespace Network {
namespace {

TEST(IoSocketHandleImplTest, TestIoSocketError) {
TEST(IoSocketHandleImpl, TestIoSocketError) {
EXPECT_DEBUG_DEATH(IoSocketError(SOCKET_ERROR_AGAIN),
".*assert failure: .* Details: Didn't use getIoSocketEagainInstance.*");
EXPECT_EQ(errorDetails(SOCKET_ERROR_AGAIN),
Expand Down Expand Up @@ -98,6 +99,80 @@ TEST(IoSocketHandleImpl, LastRoundTripTimeReturnsRttIfSuccessful) {
EXPECT_THAT(io_handle.lastRoundTripTime(),
Eq(std::chrono::duration_cast<std::chrono::milliseconds>(rtt)));
}

TEST(IoSocketHandleImpl, InterfaceNameWithPipe) {
std::string path = TestEnvironment::unixDomainSocketPath("foo.sock");

const mode_t mode = 0777;
Address::PipeInstance pipe(path, mode);
Address::InstanceConstSharedPtr address = std::make_shared<Address::PipeInstance>(pipe);
SocketImpl socket(Socket::Type::Stream, address, nullptr, {});

EXPECT_TRUE(socket.ioHandle().isOpen()) << pipe.asString();

Api::SysCallIntResult result = socket.bind(address);
ASSERT_EQ(result.return_value_, 0);

EXPECT_FALSE(socket.ioHandle().interfaceName().has_value());
}

TEST(IoSocketHandleImpl, ExplicitDoesNotSupportGetifaddrs) {

auto socket = std::make_shared<Network::Test::TcpListenSocketImmediateListen>(
Network::Test::getCanonicalLoopbackAddress(Address::IpVersion::v4));

NiceMock<Api::MockOsSysCalls> os_sys_calls;
TestThreadsafeSingletonInjector<Api::OsSysCallsImpl> os_calls(&os_sys_calls);

EXPECT_CALL(os_sys_calls, supportsGetifaddrs()).WillOnce(Return(false));
const auto maybe_interface_name = socket->ioHandle().interfaceName();
EXPECT_FALSE(maybe_interface_name.has_value());
}

TEST(IoSocketHandleImpl, NullptrIfaddrs) {
auto& os_syscalls_singleton = Api::OsSysCallsSingleton::get();
auto socket = std::make_shared<Network::Test::TcpListenSocketImmediateListen>(
Network::Test::getCanonicalLoopbackAddress(Address::IpVersion::v4));

NiceMock<Api::MockOsSysCalls> os_sys_calls;
TestThreadsafeSingletonInjector<Api::OsSysCallsImpl> os_calls(&os_sys_calls);

EXPECT_CALL(os_sys_calls, supportsGetifaddrs()).WillRepeatedly(Return(true));
EXPECT_CALL(os_sys_calls, getsockname(_, _, _))
.WillOnce(
Invoke([&](os_fd_t sockfd, sockaddr* addr, socklen_t* addrlen) -> Api::SysCallIntResult {
os_syscalls_singleton.getsockname(sockfd, addr, addrlen);
return {0, 0};
}));
EXPECT_CALL(os_sys_calls, getifaddrs(_))
.WillOnce(Invoke([&](Api::InterfaceAddressVector&) -> Api::SysCallIntResult {
return {0, 0};
}));

const auto maybe_interface_name = socket->ioHandle().interfaceName();
EXPECT_FALSE(maybe_interface_name.has_value());
}

class IoSocketHandleImplTest : public testing::TestWithParam<Network::Address::IpVersion> {};
INSTANTIATE_TEST_SUITE_P(IpVersions, IoSocketHandleImplTest,
testing::ValuesIn({Network::Address::IpVersion::v4,
Network::Address::IpVersion::v6}),
TestUtility::ipTestParamsToString);

TEST_P(IoSocketHandleImplTest, InterfaceNameForLoopback) {
auto socket = std::make_shared<Network::Test::TcpListenSocketImmediateListen>(
Network::Test::getCanonicalLoopbackAddress(GetParam()));

const auto maybe_interface_name = socket->ioHandle().interfaceName();

if (Api::OsSysCallsSingleton::get().supportsGetifaddrs()) {
EXPECT_TRUE(maybe_interface_name.has_value());
EXPECT_TRUE(absl::StrContains(maybe_interface_name.value(), "lo"));
} else {
EXPECT_FALSE(maybe_interface_name.has_value());
}
}

} // namespace
} // namespace Network
} // namespace Envoy
2 changes: 2 additions & 0 deletions test/extensions/io_socket/user_space/io_handle_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@ class IoHandleImplTest : public testing::Test {
absl::FixedArray<char> buf_;
};

TEST_F(IoHandleImplTest, InterfaceName) { ASSERT_FALSE(io_handle_->interfaceName().has_value()); }

// Test recv side effects.
TEST_F(IoHandleImplTest, BasicRecv) {
Buffer::OwnedImpl buf_to_write("0123456789");
Expand Down
1 change: 1 addition & 0 deletions test/mocks/network/io_handle.h
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ class MockIoHandle : public IoHandle {
MOCK_METHOD(absl::optional<std::chrono::milliseconds>, lastRoundTripTime, ());
MOCK_METHOD(Api::SysCallIntResult, ioctl,
(unsigned long, void*, unsigned long, void*, unsigned long, unsigned long*));
MOCK_METHOD(absl::optional<std::string>, interfaceName, ());
};

} // namespace Network
Expand Down