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

usbip: fix default snippetIpAddress command #522

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

terlar
Copy link
Contributor

@terlar terlar commented Jul 26, 2024

This fixes some bugs that I discovered in the default command.

The nix string escaping was wrong so the \ got lost and when I tested this the command had a trailing space that made it fail still.

Also this didn't work when I was using network-mode mirrored since it includes metric in the output:

$ ip route list
default via 192.168.1.1 dev eth0 proto kernel metric 35
192.168.1.0/24 dev eth0 proto kernel scope link metric 291
192.168.1.1 dev eth0 proto kernel scope link metric 35

This also adds a target so you can batch restart the services. This is handy when having multiple YubiKeys and or switching between VPN. Perhaps there could even be some auto-restart thing. Will have to think about if that is a hack or not.

@terlar
Copy link
Contributor Author

terlar commented Jul 26, 2024

Seems it is still brittle when connected to VPN with multiple routes or any other case where you have multiple routes. This solves it for me:

"$(ip route list | sed -nE 's/(default)? via (.*) dev .*/\\2/p' | head -n1)";

@terlar
Copy link
Contributor Author

terlar commented Jul 26, 2024

Networking mode NAT

$ wslinfo --networking-mode
nat
$ ip route list
default via 172.17.176.1 dev eth0 proto kernel
172.17.176.0/20 dev eth0 proto kernel scope link src 172.17.181.191
$ ip route list | sed -nE 's/(default)? via (.*) dev .*/\2/p' | head -n1
172.17.176.1

Networking mode mirrored

$ wslinfo --networking-mode
mirrored
$ ip route list
default via 192.168.1.1 dev eth0 proto kernel metric 35
192.168.1.0/24 dev eth0 proto kernel scope link metric 291
192.168.1.1 dev eth0 proto kernel scope link metric 35
$ ip route list | sed -nE 's/(default)? via (.*) dev .*/\2/p' | head -n1
192.168.1.1

Networking mode mirrored + wslvpnkit

$ wslinfo --networking-mode
mirrored
$ ip route list
default via 192.168.127.1 dev wsltap
<-- SNIP VERY LONG LIST OF ROUTES (all on eth0) -->
$ ip route list | sed -nE 's/(default)? via (.*) dev .*/\2/p' | head -n1
192.168.127.1

@terlar
Copy link
Contributor Author

terlar commented Jul 26, 2024

Is there any reason not to always grab the default route?

@terlar terlar force-pushed the fix/usbip-snippetIpAddress-default branch from ec9a1aa to 5fdf3c2 Compare July 26, 2024 22:45
@@ -21,7 +21,7 @@ in

snippetIpAddress = lib.mkOption {
type = lib.types.str;
default = "$(ip route list | sed -nE 's/(default)? via (.*) dev eth0 proto kernel/\2/p')";
default = "$(ip route list | sed -nE 's/(default)? via (.*) dev .*/\\2/p' | head -n1)";
Copy link
Member

Choose a reason for hiding this comment

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

I think we want to keep the eth0 and so on and just add the wildcard after it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about the head -n1, otherwise you risk getting multiple matches, which won't work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any reason to keep it at eth0 by the way? If not wsl-vpnkit would work out of the box.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SuperSandro2000 Pinging in case you didn't see my follow-up questions.

Copy link
Member

Choose a reason for hiding this comment

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

What about the head -n1, otherwise you risk getting multiple matches, which won't work

yeah, lets add that.

Any reason to keep it at eth0 by the way? If not wsl-vpnkit would work out of the box.

Thats the default interface WSL uses.

Copy link
Contributor Author

@terlar terlar Oct 3, 2024

Choose a reason for hiding this comment

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

@SuperSandro2000 I adjusted it per your request. I am still not sure what the drawback is to not specify eth0 as it will work in all the cases (both when on eth0 or when using wsl-vpnkit which is not using eth0). Instead of when you do specify it, those users won't have it working out of the box, but will have to adjust this configuration (which is not a big deal, but still).

The nix string escaping was wrong so the `\` got lost.

I also discovered that this fails if having multiple routes. This should
make it more robust and default to the first match (which is usually the
default route).
@terlar terlar force-pushed the fix/usbip-snippetIpAddress-default branch from 5fdf3c2 to 5d68316 Compare October 3, 2024 21:14
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.

2 participants