Skip to content
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.

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

net/netns: add Windows support for bind-to-interface-by-route #12552

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dblohm7
Copy link
Member

@dblohm7 dblohm7 commented Jun 19, 2024

This is implemented via GetBestInterfaceEx. Should we encounter errors or fail to resolve a valid, non-Tailscale interface, we fall back to returning the index for the default interface instead.

Fixes #12551

@dblohm7 dblohm7 requested a review from raggi June 19, 2024 23:02
@dblohm7 dblohm7 force-pushed the aaron/win-bind-iface-by-route branch 2 times, most recently from aa2b7dd to c5d1525 Compare June 19, 2024 23:06
Copy link
Member

@raggi raggi left a comment

Choose a reason for hiding this comment

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

This implementation looks good as-is, but a couple of things we might consider:

  • You can get the same result using the BSD sockets API by creating a UDP socket, then calling connect(2) (which won't send a packet). The connect will have a side effect of binding the local socket address to the best interface - so then you can use the local sockaddr API to get that interface index. This strategy is portable, and works on macOS, Windows, BSD's and Linux.
  • I believe both this implementation and the one discussed above may have trouble when an exit node is selected, as they'll return the tailscale interface in that case. Is that covered somewhere by callers of this feature?

@dblohm7 dblohm7 force-pushed the aaron/win-bind-iface-by-route branch from c5d1525 to 2ac9587 Compare June 21, 2024 19:26
@dblohm7
Copy link
Member Author

dblohm7 commented Jun 21, 2024

I'm not sure about your second point; I've pinged @andrew-d about what he knows about the history here.

This is implemented via GetBestInterfaceEx. Should we encounter errors
or fail to resolve a valid, non-Tailscale interface, we fall back to
returning the index for the default interface instead.

Fixes #12551

Signed-off-by: Aaron Klotz <aaron@tailscale.com>
@dblohm7 dblohm7 force-pushed the aaron/win-bind-iface-by-route branch from 2ac9587 to b36c286 Compare June 21, 2024 19:36
@andrew-d
Copy link
Member

  • I believe both this implementation and the one discussed above may have trouble when an exit node is selected, as they'll return the tailscale interface in that case. Is that covered somewhere by callers of this feature?

At least on macOS, the implementation falls back to the default if the "bind to interface by route" logic picks the Tailscale interface, which is how we avoid a routing loop with exit nodes.

In this PR, it looks like we do the same thing here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FR: Windows support for binding to interface by route
3 participants