fix(kyber): resolve sudo not found in enableIpForwarding activation - #1440
Conversation
Use sudo discovery pattern (checking PATH, /run/wrappers/bin/sudo, /usr/bin/sudo, doas) consistent with the tailscale module, instead of bare `sudo` which is not on PATH during home-manager activation. Closes the `command not found: sudo` error on `make nix-switch`.
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughSummary by CodeRabbit
WalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Mesa DescriptionTL;DRFixes What changed?
Description generated by Mesa. Update settings |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a more robust mechanism for handling elevated commands within the Home Manager activation script for Tailscale IP forwarding. It adds detection logic for sudo or doas and a run_root_cmd helper function to manage privilege escalation and dry runs. The review feedback suggests simplifying the command discovery logic using a loop for better maintainability and recommends using absolute Nix store paths for utilities like sysctl and tee to ensure reliability across different environments.
| SUDO_CMD="" | ||
| if command -v sudo >/dev/null 2>&1; then | ||
| SUDO_CMD="sudo" | ||
| elif [ -x /run/wrappers/bin/sudo ]; then | ||
| SUDO_CMD="/run/wrappers/bin/sudo" | ||
| elif [ -x /usr/bin/sudo ]; then | ||
| SUDO_CMD="/usr/bin/sudo" | ||
| elif command -v doas >/dev/null 2>&1; then | ||
| SUDO_CMD="doas" | ||
| elif [ -x /usr/bin/doas ]; then | ||
| SUDO_CMD="/usr/bin/doas" | ||
| elif [ "$(id -u)" -ne 0 ]; then | ||
| echo "IP forwarding requires root privileges, but sudo/doas is not available." >&2 | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
The logic for discovering an elevated command helper (sudo/doas) is quite verbose and duplicated from the tailscale module. Using a loop improves maintainability and readability. Additionally, to further resolve the PATH issues that prompted this PR, consider using absolute Nix store paths for other utilities like sysctl (e.g., ${pkgs.procps}/bin/sysctl) and tee (e.g., ${pkgs.coreutils}/bin/tee), as is done with rage and gnupg elsewhere in this file.
SUDO_CMD=""
for cmd in "sudo" "/run/wrappers/bin/sudo" "/usr/bin/sudo" "doas" "/usr/bin/doas"; do
if command -v "$cmd" >/dev/null 2>&1 || [ -x "$cmd" ]; then
SUDO_CMD="$cmd"
break
fi
done
if [ -z "$SUDO_CMD" ] && [ "$(id -u)" -ne 0 ]; then
echo "IP forwarding requires root privileges, but sudo/doas is not available." >&2
exit 1
fi
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes the enableIpForwarding home-manager activation hook on kyber so it can reliably run privileged commands during activation (when sudo isn’t on PATH).
Changes:
- Add discovery logic for an elevated command helper (
sudoviacommand -v, wrapper paths, ordoas) - Route privileged operations through a
run_root_cmdhelper withDRY_RUN_CMDsupport
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| elif [ "$(id -u)" -ne 0 ]; then | ||
| echo "IP forwarding requires root privileges, but sudo/doas is not available." >&2 | ||
| exit 1 | ||
| fi | ||
|
|
||
| run_root_cmd() { | ||
| if [ -n "$SUDO_CMD" ]; then | ||
| ''${DRY_RUN_CMD:-} "$SUDO_CMD" "$@" | ||
| else | ||
| ''${DRY_RUN_CMD:-} "$@" |
| if [ "$(cat /proc/sys/net/ipv4/ip_forward)" != "1" ]; then | ||
| echo "Enabling IP forwarding for Tailscale exit node..." | ||
| sudo sysctl -w net.ipv4.ip_forward=1 | ||
| sudo sysctl -w net.ipv6.conf.all.forwarding=1 | ||
| run_root_cmd sysctl -w net.ipv4.ip_forward=1 | ||
| run_root_cmd sysctl -w net.ipv6.conf.all.forwarding=1 | ||
| fi |
Summary
enableIpForwardinghome-manager activation hook used baresudowhich is not on PATH during activation, causingcommand not foundonmake nix-switchcommand -v sudo,/run/wrappers/bin/sudo,/usr/bin/sudo,doas)DRY_RUN_CMDsupport consistent with other activation hooksTest plan
make nix-switchon kyber - should no longer fail withsudo: command not foundcat /proc/sys/net/ipv4/ip_forward)Summary by cubic
Fixes the kyber Home Manager activation error where
enableIpForwardingusedsudothat wasn’t on PATH, breakingmake nix-switch. Adds a robust root command helper andDRY_RUN_CMDsupport so IP forwarding is applied reliably.sudowith discovery ofsudo(PATH,/run/wrappers/bin/sudo,/usr/bin/sudo) ordoas; fail if neither is available and not root, matching the tailscale module pattern.run_root_cmdwrapper that honorsDRY_RUN_CMDand use it forsysctlandtee.Written for commit 327d5fd. Summary will update on new commits.