Forward incoming tailnet connections to the host's primary interface #454
Conversation
WalkthroughThe pull request introduces a new configuration option Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
tailscale/rootfs/etc/s6-overlay/s6-rc.d/forwarding/finish (1)
22-24: Make warning messages more specific.The warning messages should indicate whether it's IPv4 or IPv6 forwarding removal that failed.
- bashio::log.warning "Removing forwarding is unsuccessful" + bashio::log.warning "Failed to remove IPv4 forwarding rule from ${from_address_ipv4} to ${to_address}"- bashio::log.warning "Removing forwarding is unsuccessful" + bashio::log.warning "Failed to remove IPv6 forwarding rule from ${from_address_ipv6} to ${to_address}"Also applies to: 32-34
tailscale/rootfs/etc/s6-overlay/s6-rc.d/forwarding/run (2)
31-35: Add error message for ipcalc failures.When
ipcalcfails, it would be helpful to log the reason.- if ! ipinfo="$(/usr/bin/ipcalc --json "${address}")"; then + if ! ipinfo="$(/usr/bin/ipcalc --json "${address}" 2>&1)"; then + bashio::log.debug "Address ${address} is not valid: ${ipinfo}" return 1 fi
44-48: Consider handling multiple addresses more robustly.The current implementation takes the first valid address but doesn't log skipped addresses. Adding debug logging would help troubleshooting.
for address in "$(bashio::network.ipv4_address)"; do + bashio::log.debug "Checking IPv4 address: ${address}" if to_address_ipv4=$(get_forwardable_address "${address}"); then + bashio::log.debug "Using IPv4 address: ${to_address_ipv4}" break + else + bashio::log.debug "Skipping non-forwardable IPv4 address: ${address}" fi doneAlso applies to: 51-55
tailscale/DOCS.md (1)
166-168: Minor style improvement needed in the documentation.The abbreviation "ie." should be written as "i.e." (that is) with two periods.
-Home Assistant (ie. not on all interfaces), are not accessible directly from the +Home Assistant (i.e., not on all interfaces), are not accessible directly from the🧰 Tools
🪛 LanguageTool
[uncategorized] ~167-~167: The abbreviation “i.e.” (= that is) requires two periods.
Context: ...e interfaces managed by Home Assistant (ie. not on all interfaces), are not accessi...(I_E)
[style] ~167-~167: Consider using “inaccessible” to avoid wordiness.
Context: ...istant (ie. not on all interfaces), are not accessible directly from the tailnet when userspac...(NOT_ABLE_PREMIUM)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
tailscale/DOCS.md(2 hunks)tailscale/config.yaml(1 hunks)tailscale/rootfs/etc/s6-overlay/s6-rc.d/forwarding/down(1 hunks)tailscale/rootfs/etc/s6-overlay/s6-rc.d/forwarding/finish(1 hunks)tailscale/rootfs/etc/s6-overlay/s6-rc.d/forwarding/run(1 hunks)tailscale/rootfs/etc/s6-overlay/s6-rc.d/forwarding/type(1 hunks)tailscale/rootfs/etc/s6-overlay/s6-rc.d/forwarding/up(1 hunks)tailscale/rootfs/etc/s6-overlay/scripts/stage2_hook.sh(1 hunks)tailscale/translations/en.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- tailscale/rootfs/etc/s6-overlay/s6-rc.d/forwarding/down
- tailscale/rootfs/etc/s6-overlay/s6-rc.d/forwarding/up
🧰 Additional context used
🪛 LanguageTool
tailscale/DOCS.md
[uncategorized] ~167-~167: The abbreviation “i.e.” (= that is) requires two periods.
Context: ...e interfaces managed by Home Assistant (ie. not on all interfaces), are not accessi...
(I_E)
[style] ~167-~167: Consider using “inaccessible” to avoid wordiness.
Context: ...istant (ie. not on all interfaces), are not accessible directly from the tailnet when userspac...
(NOT_ABLE_PREMIUM)
🔇 Additional comments (5)
tailscale/rootfs/etc/s6-overlay/s6-rc.d/forwarding/type (1)
1-1: LGTM! Appropriate service type chosen.The
oneshotservice type is correct for this use case as the forwarding setup should run once at startup.tailscale/config.yaml (1)
37-37: LGTM! Schema addition follows existing patterns.The new
forward_to_hostfield is properly defined as an optional boolean, maintaining consistency with other similar configuration options.tailscale/rootfs/etc/s6-overlay/scripts/stage2_hook.sh (1)
17-23: LGTM! Well-structured service management logic.The new conditional block follows the established pattern for service management and correctly handles the configuration checks for the new
forward_to_hostfeature.tailscale/translations/en.yaml (1)
37-42: LGTM! Clear and consistent translation entry.The translation entry for
forward_to_hostis well-documented and follows the established format.tailscale/DOCS.md (1)
151-175: LGTM! Comprehensive documentation of the new feature.The documentation thoroughly explains the feature's purpose, behavior, and limitations. The notes about Tailscale's serve/funnel features and hairpinning limitations are particularly helpful for users.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~167-~167: The abbreviation “i.e.” (= that is) requires two periods.
Context: ...e interfaces managed by Home Assistant (ie. not on all interfaces), are not accessi...(I_E)
[style] ~167-~167: Consider using “inaccessible” to avoid wordiness.
Context: ...istant (ie. not on all interfaces), are not accessible directly from the tailnet when userspac...(NOT_ABLE_PREMIUM)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tailscale/rootfs/etc/s6-overlay/s6-rc.d/forwarding/finish (1)
18-29: LGTM! Consider improving error handling.The function correctly removes forwarding rules. However, consider capturing and logging the specific error message when rule removal fails.
- if ! ${cmd} -t nat -D PREROUTING -d ${from_address} -j DNAT --to-destination ${to_address}; then - bashio::log.warning "Removing forwarding is unsuccessful (${ip_version})" + if ! error=$(${cmd} -t nat -D PREROUTING -d ${from_address} -j DNAT --to-destination ${to_address} 2>&1); then + bashio::log.warning "Failed to remove ${ip_version} forwarding: ${error}"tailscale/rootfs/etc/s6-overlay/s6-rc.d/forwarding/run (2)
12-37: Enhance error messages for better debugging.The function correctly validates addresses but could provide more specific error messages.
if [[ "${address}" =~ .*:.* ]]; then if [[ $(</proc/sys/net/ipv6/conf/all/forwarding) -eq 0 ]]; then + bashio::log.debug "IPv6 forwarding is disabled" return 1 fi else if [[ $(</proc/sys/net/ipv4/ip_forward) -eq 0 ]]; then + bashio::log.debug "IPv4 forwarding is disabled" return 1 fi fi
41-53: Add success logging for better visibility.Consider adding a log message when a valid address is found.
if ! bashio::var.equals "$(bashio::network.${ip_version}_method)" "disabled"; then for address in "$(bashio::network.${ip_version}_address)"; do if get_forwardable_address "${address}"; then + bashio::log.debug "Found valid ${ip_version} address: ${address}" break else bashio::log.debug "Skipping non-forwardable ${ip_version} address: ${address}" fi done fitailscale/DOCS.md (1)
151-174: Improve documentation clarity and conciseness.The documentation is comprehensive but could be more concise. Consider:
- Combining the first two notes about service accessibility
- Making the hairpinning note more prominent as it's a key limitation
-**Note:** Without forwarding, services running only on the interfaces managed by -Home Assistant (i.e. not on all interfaces), are not accessible directly from -the tailnet when userspace networking is disabled. - -**Note:** Tailscale's serve and funnel features have priority over this plain -port forwarding, those connections won't be forwarded directly to the host. +**Note:** Without forwarding, services running only on Home Assistant-managed +interfaces are inaccessible from the tailnet when userspace networking is disabled. +Tailscale's serve and funnel features take priority over this forwarding. -**Note:** Hairpinning is not implemented, do not test forwarding by accessing -the host, from itself, through the tailscale0 interface. +**Important:** Hairpinning is not implemented. Do not test forwarding by accessing +the host from itself through the tailscale0 interface.🧰 Tools
🪛 LanguageTool
[style] ~167-~167: Consider using “inaccessible” to avoid wordiness.
Context: ...stant (i.e. not on all interfaces), are not accessible directly from the tailnet when userspac...(NOT_ABLE_PREMIUM)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tailscale/DOCS.md(2 hunks)tailscale/rootfs/etc/s6-overlay/s6-rc.d/forwarding/finish(1 hunks)tailscale/rootfs/etc/s6-overlay/s6-rc.d/forwarding/run(1 hunks)
🧰 Additional context used
🪛 LanguageTool
tailscale/DOCS.md
[style] ~167-~167: Consider using “inaccessible” to avoid wordiness.
Context: ...stant (i.e. not on all interfaces), are not accessible directly from the tailnet when userspac...
(NOT_ABLE_PREMIUM)
🔇 Additional comments (3)
tailscale/rootfs/etc/s6-overlay/s6-rc.d/forwarding/finish (1)
32-38: LGTM! Error handling implemented as suggested.The implementation correctly handles and logs failures when retrieving Tailscale IP addresses.
tailscale/rootfs/etc/s6-overlay/s6-rc.d/forwarding/run (2)
56-77: LGTM! Well-structured implementation.The function correctly:
- Validates input parameters
- Checks for existing rules
- Uses append (-A) for less intrusive rule addition
79-92: LGTM! Consistent implementation with finish script.The main section properly handles both IPv4 and IPv6 address retrieval and forwarding setup.
Proposed Changes
TLDR: Without this, users have to enable subnet routing only to access the host, really different user experience compared to the general Tailscale client (and when there are multiple 192.168.1.x subnets, more trouble).
This is identical what Tailscale's docker image does (here). Though compared to Tailscale's docker image, this PR is using only -A append, that is less intrusive than -I insert.
When userspace networking is enabled, tailscaled automatically forwards incoming tailnet connections to localhost. Not in TS docs, only an issue comment (here), but tested and true. Without this, there would be no way to access anything on the host.
When userspace networking is disabled, tailscaled doesn't do anything, but in a container, services not running on all interfaces will not be accessible on tailscale0 interface, and by default they are running only on the HA managed interfaces.
I've added a config switch to be able to disable this forwarding in case it interferes with some complex networking setup.
UPDATE: Tested with real subnet routing, rPI3, rPI4, HA OS VM, userspace_networking enabled/disabled, snat_subnet_routes enabled/disables, stateful_filtering enabled/disabled, it didn't interfere with these settings.
Related Issues
Summary by CodeRabbit
New Features
forward_to_hostfor Tailscale add-on.Documentation