IoHandle: add interfaceName() method by junr03 · Pull Request #18531 · envoyproxy/envoy

junr03 · 2021-10-08T16:35:40Z

Commit Message: IoHandle - add interfaceName() method.
Additional Description: returns the interface name for the underlying socket if the implementation and OS allows. This is done by matching the sockets local address to the address of interfaces as returned by getifaddrs.
Risk Level: low
Testing: pending UT

Signed-off-by: Jose Nino jnino@lyft.com

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 · 2021-10-08T16:36:32Z

@mattklein123 @goaway I am figuring out how to test this reliably on CI. I think using the loopback interface and address might be the best route. Will update ASAP.

I mainly wanted to get out quick so @goaway could comment on if this is the direction he wanted this piece of work to go on.

source/common/network/io_socket_handle_impl.cc

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 · 2021-10-08T18:38:09Z

@goaway @mattklein123 this is ready for review!

Signed-off-by: Jose Nino <jnino@lyft.com>

mattklein123

Thanks LGTM. Not sure if you want to merge the other PR first or fix up that PR based on this one?

mattklein123 · 2021-10-08T21:57:51Z

source/common/network/io_socket_handle_impl.cc


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


Will this change based on the other other PR?

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

The windows failure can also be solved with ^.

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 · 2021-10-11T13:45:10Z

test/common/network/io_socket_handle_impl_test.cc

+  EXPECT_FALSE(maybe_interface_name.has_value());
+}
+
+TEST(IoSocketHandleImpl, InterfaceNameForLoopbackExplicitV6) {


The test below was not showing coverage for v6. Testing here explicitly.

mattklein123

Thanks, LGTM.

junr03 · 2021-10-11T22:43:36Z

/retest

repokitteh-read-only · 2021-10-11T22:43:40Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #18531 (comment) was created by @junr03.

see: more, trace.

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 · 2021-12-09T00:58:30Z

source/extensions/io_socket/user_space/io_handle_impl.h


  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; }


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

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?

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.

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

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

Signed-off-by: Jose Nino <jnino@lyft.com>

source/common/network/io_socket_handle_impl.cc

goaway · 2021-12-15T10:25:09Z

Overall this looks good from an EM perspective. Left one nit comment. Thanks @junr03!

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 · 2021-12-15T20:28:22Z

@rojkov do you mind doing a pass on this?

junr03 · 2021-12-16T21:19:37Z

/retest

repokitteh-read-only · 2021-12-16T21:19:40Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #18531 (comment) was created by @junr03.

see: more, trace.

rojkov

This looks good! I added a couple of questions to clarify.

test/common/network/io_socket_handle_impl_test.cc

source/common/network/io_socket_handle_impl.cc

Signed-off-by: Jose Nino <jnino@lyft.com>

rojkov

Thanks! LGTM

Was reading through envoyproxy#18531 and seeing the socket address being looked up for each interface confused me a bit, so let's do it only once. Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

Commit Message: IoHandle - add interfaceName() method. Additional Description: returns the interface name for the underlying socket if the implementation and OS allows. This is done by matching the sockets local address to the address of interfaces as returned by getifaddrs. Risk Level: low Testing: pending UT Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: Josh Perry <josh.perry@mx.com>

Jose Nino added 2 commits October 7, 2021 22:03

wip

27851a9

Signed-off-by: Jose Nino <jnino@lyft.com>

update

0cd8429

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 requested a review from antoniovicente as a code owner October 8, 2021 16:35

junr03 assigned goaway and mattklein123 Oct 8, 2021

junr03 commented Oct 8, 2021

View reviewed changes

source/common/network/io_socket_handle_impl.cc Outdated Show resolved Hide resolved

Jose Nino added 2 commits October 8, 2021 10:55

done

d21195d

Signed-off-by: Jose Nino <jnino@lyft.com>

one more test

be38d7b

Signed-off-by: Jose Nino <jnino@lyft.com>

Jose Nino added 3 commits October 8, 2021 13:05

linux

f2827f1

Signed-off-by: Jose Nino <jnino@lyft.com>

namespace

ce6f9ab

Signed-off-by: Jose Nino <jnino@lyft.com>

pipe test

2dafddd

Signed-off-by: Jose Nino <jnino@lyft.com>

mattklein123 previously approved these changes Oct 8, 2021

View reviewed changes

Jose Nino added 3 commits October 8, 2021 15:16

windows

c7f4d19

Signed-off-by: Jose Nino <jnino@lyft.com>

Merge branch 'main' into interface-name

4243b7b

Signed-off-by: Jose Nino <jnino@lyft.com>

updated

7372a08

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 dismissed mattklein123’s stale review via 7372a08 October 9, 2021 19:21

Jose Nino added 3 commits October 10, 2021 19:14

more tests

1e2d2c3

Signed-off-by: Jose Nino <jnino@lyft.com>

mocks

5948098

Signed-off-by: Jose Nino <jnino@lyft.com>

nicemock

5ea99e6

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 commented Oct 11, 2021

View reviewed changes

mattklein123 previously approved these changes Oct 11, 2021

View reviewed changes

Jose Nino added 2 commits October 12, 2021 08:43

more tests

98b6896

Signed-off-by: Jose Nino <jnino@lyft.com>

spelling

09939e0

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 dismissed mattklein123’s stale review via 09939e0 October 12, 2021 13:53

Merge branch 'main' into interface-name

7bac749

Signed-off-by: Jose Nino <jnino@lyft.com>

mattklein123 previously approved these changes Oct 12, 2021

View reviewed changes

github-actions bot closed this Nov 19, 2021

mattklein123 deleted the interface-name branch November 19, 2021 23:02

junr03 restored the interface-name branch December 8, 2021 18:01

Jose Nino added 2 commits December 8, 2021 10:15

Merge branch 'main' into interface-name

c6175b6

Signed-off-by: Jose Nino <jnino@lyft.com>

update

6edc2ff

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 reopened this Dec 8, 2021

junr03 dismissed mattklein123’s stale review via 6edc2ff December 8, 2021 18:39

repokitteh-read-only bot removed the waiting label Dec 8, 2021

Merge branch 'main' into interface-name

8f3464c

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 removed the stale stalebot believes this issue/PR has not been touched recently label Dec 9, 2021

nullopt contrib

6ed79e2

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 requested a review from rojkov as a code owner December 9, 2021 00:55

junr03 commented Dec 9, 2021

View reviewed changes

Jose Nino added 2 commits December 9, 2021 12:51

test

89c9f15

Signed-off-by: Jose Nino <jnino@lyft.com>

fmt

40e3e1e

Signed-off-by: Jose Nino <jnino@lyft.com>

goaway reviewed Dec 15, 2021

View reviewed changes

source/common/network/io_socket_handle_impl.cc Outdated Show resolved Hide resolved

comment

58cb9e4

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 assigned rojkov Dec 15, 2021

rojkov reviewed Dec 17, 2021

View reviewed changes

test/common/network/io_socket_handle_impl_test.cc Show resolved Hide resolved

source/common/network/io_socket_handle_impl.cc Outdated Show resolved Hide resolved

comments

75ed9b7

Signed-off-by: Jose Nino <jnino@lyft.com>

rojkov approved these changes Dec 20, 2021

View reviewed changes

junr03 merged commit 6957172 into main Dec 20, 2021

rgs1 mentioned this pull request Jan 7, 2022

IoHandle: simplify interfaceName() #19448

Closed

junr03 deleted the interface-name branch January 11, 2022 18:41

Conversation

junr03 commented Oct 8, 2021

Uh oh!

junr03 commented Oct 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

junr03 commented Oct 8, 2021

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

junr03 commented Oct 11, 2021

Uh oh!

repokitteh-read-only bot commented Oct 11, 2021

Uh oh!

junr03 Dec 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

goaway commented Dec 15, 2021

Uh oh!

junr03 commented Dec 15, 2021

Uh oh!

junr03 commented Dec 16, 2021

Uh oh!

repokitteh-read-only bot commented Dec 16, 2021

Uh oh!

rojkov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rojkov left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

junr03 commented Oct 8, 2021 •

edited

Loading

junr03 Dec 9, 2021 •

edited

Loading