Skip to content

IoHandle: add interfaceName() method #18531

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

Merged
junr03 merged 24 commits into from
Dec 20, 2021
Merged

IoHandle: add interfaceName() method #18531

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
27851a9
wip
Oct 8, 2021
0cd8429
update
Oct 8, 2021
d21195d
done
Oct 8, 2021
be38d7b
one more test
Oct 8, 2021
f2827f1
linux
Oct 8, 2021
ce6f9ab
namespace
Oct 8, 2021
2dafddd
pipe test
Oct 8, 2021
c7f4d19
windows
Oct 8, 2021
4243b7b
Merge branch 'main' into interface-name
Oct 9, 2021
7372a08
updated
Oct 9, 2021
1e2d2c3
more tests
Oct 11, 2021
5948098
mocks
Oct 11, 2021
5ea99e6
nicemock
Oct 11, 2021
98b6896
more tests
Oct 12, 2021
09939e0
spelling
Oct 12, 2021
7bac749
Merge branch 'main' into interface-name
Oct 12, 2021
c6175b6
Merge branch 'main' into interface-name
Dec 8, 2021
6edc2ff
update
Dec 8, 2021
8f3464c
Merge branch 'main' into interface-name
Dec 8, 2021
6ed79e2
nullopt contrib
Dec 9, 2021
89c9f15
test
Dec 9, 2021
40e3e1e
fmt
Dec 9, 2021
58cb9e4
comment
Dec 15, 2021
75ed9b7
comments
Dec 17, 2021
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
5 changes: 5 additions & 0 deletions envoy/network/io_handle.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,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
44 changes: 44 additions & 0 deletions source/common/network/io_socket_handle_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -650,5 +650,49 @@ absl::optional<std::chrono::milliseconds> IoSocketHandleImpl::lastRoundTripTime(
return std::chrono::duration_cast<std::chrono::milliseconds>(info.tcpi_rtt);
}

absl::optional<std::string> IoSocketHandleImpl::interfaceName() {
absl::optional<std::string> interface_name{};
#ifdef SUPPORTS_GETIFADDRS

@mattklein123 mattklein123 Oct 8, 2021

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.

Will this change based on the other other PR?

@junr03 junr03 Oct 8, 2021

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.

Yep. This will change to use the other PR (#18513) and prevent the ifdefs and direct syscalls.

@junr03 junr03 Oct 8, 2021

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.

The windows failure can also be solved with ^.

Address::InstanceConstSharedPtr socket_address = localAddress();
if (!socket_address) {
return absl::nullopt;
}

struct ifaddrs* ifaddr;
struct ifaddrs* ifa;

const int rc = getifaddrs(&ifaddr);
RELEASE_ASSERT(!rc, "");

for (ifa = ifaddr; ifa != nullptr; ifa = ifa->ifa_next) {
if (ifa->ifa_addr == nullptr) {
continue;
}

if (ifa->ifa_addr->sa_family == AF_INET || ifa->ifa_addr->sa_family == AF_INET6) {
const struct sockaddr_storage* addr =
reinterpret_cast<const struct sockaddr_storage*>(ifa->ifa_addr);
Address::InstanceConstSharedPtr interface_address = Address::addressFromSockAddrOrThrow(
*addr,
(ifa->ifa_addr->sa_family == AF_INET) ? sizeof(sockaddr_in) : sizeof(sockaddr_in6));

if (!interface_address) {
continue;
}

if (*socket_address == *interface_address) {
Comment thread
junr03 marked this conversation as resolved.
Outdated
interface_name = std::string{ifa->ifa_name};
break;
}
}
}

if (ifaddr) {
freeifaddrs(ifaddr);
}
#endif
return interface_name;
}

} // namespace Network
} // namespace Envoy
1 change: 1 addition & 0 deletions source/common/network/io_socket_handle_impl.h
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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; }

@junr03 junr03 Dec 9, 2021 •
edited
Loading

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.

@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.

@rojkov rojkov Dec 9, 2021

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?

@florincoras florincoras Dec 9, 2021

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.

@junr03 junr03 Dec 9, 2021

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?

@rojkov rojkov Dec 10, 2021

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/mocks/network/io_handle.h
View file
Open in desktop
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