-
-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
nixos/mptcp: multipath TCP module #59342
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fi | ||
|
||
|
||
if [ `grep "$DEVICE_IFACE" "$RT_TABLE" | wc -l` -eq 0 ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
grep -c
?
fi | ||
|
||
if [ "$DHCP4_IP_ADDRESS" ]; then | ||
SUBNET=`echo $IP4_ADDRESS_0 | cut -d \ -f 1 | cut -d / -f 2` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extract this code to a function with a meaningful name, because it's duplicated below.
echo "$NUM $DEVICE_IFACE" >> "$RT_TABLE" | ||
fi | ||
|
||
if [ "$DHCP4_IP_ADDRESS" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you forget an operator here?
} | ||
|
||
(mkIf (!config.networking.networkmanager.enable) { | ||
warnings = [ "You have `networkmanager` disabled. Expect things to break." ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spelling (NetworkManager). Why is there only support for NetworkManager?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because that's the only thing I use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't use NetworkManager, but I might switch because of this change.
Is there a reason to not enable this by default for everyone?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, given the dependency on the dispatcher script this should be a failure rather a warning.
|
||
RT_TABLE=/etc/iproute2/rt_tables | ||
|
||
if [ "$DEVICE_IFACE" = "lo" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant quotes around lo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comments, I can definitely improve the hook (copied from the mptcp wiki) and will do but it should already allow you to get started with mptcp (which is why I hurried with the PR :p ).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting PR. I made some minor comments.
|
||
# TODO remove | ||
echo $PATH | ||
env > /tmp/if_up_env |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Insecure use of temporary files. /tmp/if_up_env
could be an attacker controlled symlink.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
He said in the comment that he is going to remove that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still added the comment so it won't be forgotten.
if [ `grep "$DEVICE_IFACE" "$RT_TABLE" | wc -l` -eq 0 ]; then | ||
logger -p user.info -t mptcp "Adding new routing table $DEVICE_IFACE" | ||
NUM=$(wc -l < "$RT_TABLE") | ||
echo "$NUM $DEVICE_IFACE" >> "$RT_TABLE" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this to be created at runtime rather then controlling the whole etc/iproute2/rt_tables
file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One big motivation for these multipath solutions (multipath QUIC/MPTCP/SCTP) is to deal with mobility scenarios so you need to handle dynamic usecases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default /etc/iproute2/rt_tables
does not exists, which would make:
NUM=$(wc -l < /etc/iproute2/rt_tables)
zsh: no such file or directory: /etc/iproute2/rt_tables
fail. If it is an empty file it would also collide with the local
table (number 0)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it needed at all to maintain rt_table
for this to work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be fixed by networking.iproute2.enable = true;
so.
ip rule add from "$DHCP4_IP_ADDRESS" table "$DEVICE_IFACE" | ||
else | ||
# PPP-interface | ||
IPADDR=`echo $IP4_ADDRESS_0 | cut -d \ -f 1 | cut -d / -f 1` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about ipv6?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's IPv6 :D ? I haven't tested with IPv6 (haven't done much IPv6 in general). I can try but if someone wanna step up before or after the PR, I would be really happy.
fi | ||
|
||
if [ -z "$DEVICE_IFACE" ]; then | ||
logger -p user.warn -t mptcp "invalid $DEVICE_IFACE" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is logging to stdout/stderr not better so it would end up in the journald log of network manager?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logger also ends up in journald. I used it because it was in the base example and seemed good enough: like you can set debug levels and a keyword to grep to ("mptcp" here). Now that I am looking to upstream, stdout/stderr may be better.
Upon test failure, I sometimes want to access the VM to check artifacts/rerun tests. This allows to get path to the test driver via: nixosTests.mptcp.driver.outPath
... when qemu.networkingOptions is overriden and thus there is no default SLIRP interface.
Sane defaults for configuration of MPTCP kernel.
Starts a multihomed VM, configure the redundant scheduler with the msh path manager so that all paths are used. We then launch an iperf session. nix-build -A nixosTests.mptcp .
I've spent a fair deal of time looking into the nixos testing infrastructure (also to reuse it in https://github.com/Mic92/nixos-shell) and finally cleaned up the PR. |
@grahamc I would very much like to reproduce the error locally. Looking at ofborg's repo, I ran this script
to no avail (it succeeds). EDIT: got a solution on IRC |
Thank you for your contributions.
|
let me alone bot, I am still on it :) |
I am moving this module and other MPTCP-related software to https://github.com/teto/mptcp-flake. |
Multipath TCP is an IETF extension of TCP that makes use of several TCP connections to speed up transfer/increase reliability/confidentiality. Applications don't have to be changed (yet they can use the new MPTCP API for even better performance), the kernel will use MPTCP when applicable and fall back to TCP otherwise.
I had this in my repo for a while but #59301 convinced me to propose it. It only works with networkmanager for now (you need to update routing tables depending on added/removed interfaces).
Motivation for this change
Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nix-review --run "nix-review wip"
./result/bin/
)nix path-info -S
before and after)