network: support interface binding on Android #1897
Conversation
goaway
force-pushed
the
ms/android-addr-binding
branch
from
2adb4c9
to
c3ff7b0
Compare
goaway
force-pushed
the
ms/android-addr-binding
branch
from
1eca4c1
to
65b96d7
Compare
|
Depends on envoyproxy/envoy#18769. |
| * This is a "synthetic" socket option implementation, which sets the source IP/port of a socket | ||
| * using a provided IP address (and maybe port) during bind. | ||
| * | ||
| * Based on the OriginalSrcSocketOption extension. |
There was a problem hiding this comment.
Is this exactly the same? Or are there any key differences to call out?
If it's the same, would it be worthwhile to try to reuse the upstream one?
There was a problem hiding this comment.
It is the same, and I almost did just that. However, the original source code is part of a larger extension that appears to be intended to allow Envoy to transparently pretend to be the origin of the connection. The socket option just happened to allow us to bind an arbitrary local address, which is what we wanted.
My concern about re-using this upstream code was that the SocketOption piece of it could change in support of the different upstream intent. If it were to change, it could break us in a way that stopped the feature from working or worse, could lead to runtime failures. If this occurred, there's a real risk that such failures wouldn't be detected until they hit production.
The risk of hard-to-detect runtime failures, and the fact that we'd be pulling in a larger extension with an unrelated purpose, swayed me towards just including the code we need. This also enables us to modify it to better suit our purpose as needed.
There was a problem hiding this comment.
I think perhaps ideally we'd lift this up to a generic socket option (so it's not in extension code) and have EM tests that it works the right way for EM's use case so we catch any regression
That said I'm not gonna block on this, if you think it's easier to copy this I would just do that (and maybe note that at the time of writing it's the same as the OriginalSrc one)
There was a problem hiding this comment.
What still gives me pause is that I'm not sure this is so much a case of shared inherent behavior, so much as shared incidental behavior.
My preference right now is to move forward, but I'm open to continuing the discussion.
Signed-off-by: Mike Schore <mike.schore@gmail.com>
Signed-off-by: Mike Schore <mike.schore@gmail.com>
Signed-off-by: Mike Schore <mike.schore@gmail.com>
|
Thanks @snowp! |
Description: Both v4_interfaces and v6_interfaces were using V4 interfaces likely due to a typo. This is a follow-up to #1897. Risk Level: Low, just fixing a log. Signed-off-by: JP Simard <jp@jpsim.com>
Description: Both v4_interfaces and v6_interfaces were using V4 interfaces likely due to a typo. This is a follow-up to #1897. Risk Level: Low, just fixing a log. Signed-off-by: JP Simard <jp@jpsim.com>
Description:
SO_BINDTODEVICEwill generally only be useable by an Android app running as root, so the present interface-binding approach on Android is mostly a non-starter. Instead, this change binds the interface indirectly by finding the local IP of the interface and setting that as the source IP of the socket.To do so, it passes a "synthetic" socket option through to the internal connection API. When applied, instead of setting an actual socket option on the socket, this object sets the source IP property on the connection before bind() is called.
Risk Level: Moderate
Testing: Local & On Device
Signed-off-by: Mike Schore mike.schore@gmail.com