-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
wgengine/router: enable ip forwarding on gokrazy #11408
Conversation
67906d5
to
bf262f1
Compare
bf262f1
to
390248c
Compare
The idea is to enable forwarding when we're advertising routes or exit node. I tested that:
It's supposed to be no-op for everything except gokrazy, and if no routes are being advertised. There's no persisting previous state of sysctl but that would be kind of pointless on gokrazy as they're appliances and generally rebooted on every change, keeping only /perm. It was a bit hard to find the right place to inject this in the codebase, hopefully this is good enough or sufficient for Tailscale staff to take this over. |
I know there's a bit of duplication now with sysctls being done three different ways (across the Tailscale repo) after this change, hopefully what I did is the most generic and you can later consider refactoring the rest to use a shared writeSysctl function and move it in an appropriate place in your codebase. I didn't want to take opinion where it should go or refactor the existing usages, keeping this change minimal. I'd imagine you would want to later follow up this by having all of the sysctls try writing file and if that fails fall back to sysctl command, for both read and write. It's not relevant for gokrazy though. |
390248c
to
4d434ea
Compare
LGTM. Couple small comments above. |
Only on Gokrazy, set sysctls to enable IP forwarding so subnet routing and advertised exit node works. Fixes tailscale#11405 Signed-off-by: Joonas Kuorilehto <joneskoo@derbian.fi>
4d434ea
to
b6912e7
Compare
Is this expected to also fix #10918 ? |
I don't know what to expect but based on looking at the description there, I think that configuration should work. Can you test the version in main, e.g. with replace directive in go.mod for each of the tailscale modules:
|
I think this will also be released as part of 1.64 Tailscale release later today, which could make testing easier |
@damdo, can you try it now, with 1.64.0? |
@bradfitz Yes tailscale{,d} 1.64.0 (thanks to this PR I guess), fixes #10918 (exit node issue) for me Proof:
|
@irbekrm the node meant to advertize subnet router/exit-node works as intended.
Also if I ssh and down/up tailscale I don't get the "IP forwarding disabled, ..." warning.
My suspicion is that the check-ip-forwarding logic is executed early when the enableIPForwarding logic for gokrazy hasn't been applied yet. That or there is a race. Things eventually work though. Another separate issue is the other warning "couldn't check system's UDP GRO forwarding configuration". Not sure if they are related. For the record this doesn't go away after a tailscale down/up, contrary to the other warning. |
I had a look at the code - it looks like in your use case the IP forwarding check runs as part of tailscale up whilst the logic to enable forwarding runs when the wgengine router is first set when |
For what it's worth, I only enable subnet router and no exit node and have no issues on Raspberry Pi 3B+. Be sure to check the usual things like accepting the advertised subnets, too. |
@irbekrm here are the logs you asked: https://gist.github.com/damdo/2003f4e196d6af7ebe9c8fc9ffaab7ab |
Thank you for the logs. I cannot pinpoint exactly where |
Only on Gokrazy, set sysctls to enable IP forwarding so subnet routing and advertised exit node works.
Implements #11405