network: support conditionally binding active alt interface#1834
network: support conditionally binding active alt interface#1834
Conversation
goaway
changed the title
goaway
force-pushed
the
ms/interface-binding-2
branch
from
14f3cef to
605344a
Compare
goaway
force-pushed
the
ms/interface-binding-3
branch
2 times, most recently
from
e3e1761 to
0d2add8
Compare
Signed-off-by: Mike Schore <mike.schore@gmail.com>
Signed-off-by: Mike Schore <mike.schore@gmail.com>
goaway
force-pushed
the
ms/interface-binding-3
branch
from
0d2add8 to
e77667d
Compare
| if (network == ENVOY_NET_WLAN) { | ||
| auto interfaces = | ||
| enumerateInterfaces(family, IFF_UP | IFF_MULTICAST, IFF_LOOPBACK | IFF_POINTOPOINT); | ||
| return interfaces.size() > 0 ? interfaces[0] : ""; |
There was a problem hiding this comment.
does it matter if we send the first available interface? is there any heuristic for preference?
There was a problem hiding this comment.
| return interfaces.size() > 0 ? interfaces[0] : ""; | |
| return interfaces.size() > 0 ? interfaces.front() : ""; |
There was a problem hiding this comment.
There's no documentation I'm aware that asserts any meaning with respect to order, and absent that, a slight bias towards whatever matches first seems reasonable.
There was a problem hiding this comment.
I don't really have a strong opinion with respect to [0] vs front(), but the former seems perfectly clear to me, and subjectively, better indicates to me that I'm picking an element from a vector. May I ask your basis for preferring front()?
| [[maybe_unused]] unsigned int reject_flags) { | ||
| std::vector<std::string> names{}; | ||
|
|
||
| #ifdef SUPPORTS_GETIFADDRS |
There was a problem hiding this comment.
note: I am adding full support for getifaddrs in Envoy here envoyproxy/envoy#18513. So we will be able to get rid of the ifdef.
Non-blocking of course! I can update this code once that lands :)
There was a problem hiding this comment.
Sure, we can revisit. I actually think this still might be needed here though, but since this is non-blocking, let's discuss after.
| TEST_F(ConfiguratorTest, EnumerateInterfacesFiltersByFlags) { | ||
| // Select loopback. | ||
| auto loopbacks = configurator_->enumerateInterfaces(AF_INET, IFF_LOOPBACK, 0); | ||
| EXPECT_EQ(loopbacks[0].rfind("lo", 0), 0); |
There was a problem hiding this comment.
| EXPECT_EQ(loopbacks[0].rfind("lo", 0), 0); | |
| EXPECT_TRUE(absl::StrContains(loopbacks.front(), "lo")) |
There was a problem hiding this comment.
Again, I don't have a strong preference here, but I think the existing check is a little more specific. This suggestion would match an interface named "hello_world", for instance.
| // Reject loopback. | ||
| auto nonloopbacks = configurator_->enumerateInterfaces(AF_INET, 0, IFF_LOOPBACK); | ||
| for (const auto& interface : nonloopbacks) { | ||
| EXPECT_NE(interface.rfind("lo", 0), 0); |
There was a problem hiding this comment.
| EXPECT_NE(interface.rfind("lo", 0), 0); | |
| EXPECT_FALSE(absl::StrContains(interface, "lo")); |
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Generally LGTM, thanks. Will take a further look once @junr03 comments are addressed.
Signed-off-by: Mike Schore <mike.schore@gmail.com>
5 participants
Description: Adds utility support for filtering for a viable WIFI or cellular interface to select as a potential alternate to the OS-supplied default, and an accessor for the platform-specific socket options to bind to it.
Risk Level: Low
Testing: Added unit test
Signed-off-by: Mike Schore mike.schore@gmail.com