Add support for advertising Tailscale Services#715
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (6)
WalkthroughAdds a ChangesTailscale Services
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant stage2_hook_sh
participant share_services_run
participant tailscaled
participant TailscaleServe
stage2_hook_sh->>share_services_run: retain service when services are configured
share_services_run->>tailscaled: wait for reachable running backend
share_services_run->>TailscaleServe: synchronize and advertise configured services
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tailscale/rootfs/etc/s6-overlay/scripts/stage2_hook.sh (1)
138-140: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
rm -fto prevent potential startup failures.While the container filesystem is typically recreated on every start, adding the
-fflag ensures the script won't fail and crash the startup process if theshare-servicescontents file is already missing or cannot be removed.🛠️ Proposed fix
if ! bashio::config 'services' | jq -e '. | length > 0' > /dev/null; then - rm /etc/s6-overlay/s6-rc.d/user/contents.d/share-services + rm -f /etc/s6-overlay/s6-rc.d/user/contents.d/share-services fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tailscale/rootfs/etc/s6-overlay/scripts/stage2_hook.sh` around lines 138 - 140, Update the cleanup command in the services configuration check of stage2_hook.sh to use rm -f when removing the share-services contents file, so an already-missing file does not fail startup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tailscale/rootfs/etc/s6-overlay/scripts/stage2_hook.sh`:
- Around line 138-140: Update the cleanup command in the services configuration
check of stage2_hook.sh to use rm -f when removing the share-services contents
file, so an already-missing file does not fail startup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6fec5516-8be6-4b0a-b556-1608b8e2d9ce
📒 Files selected for processing (9)
tailscale/DOCS.mdtailscale/config.yamltailscale/rootfs/etc/s6-overlay/s6-rc.d/share-services/dependencies.d/post-tailscaledtailscale/rootfs/etc/s6-overlay/s6-rc.d/share-services/runtailscale/rootfs/etc/s6-overlay/s6-rc.d/share-services/typetailscale/rootfs/etc/s6-overlay/s6-rc.d/share-services/uptailscale/rootfs/etc/s6-overlay/s6-rc.d/user/contents.d/share-servicestailscale/rootfs/etc/s6-overlay/scripts/stage2_hook.shtailscale/translations/en.yaml
2ca7ee2 to
1575c1d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tailscale/DOCS.md`:
- Around line 342-344: Update the https service description in the Tailscale
documentation to state that tailnet HTTPS certificates and MagicDNS must be
enabled beforehand, while retaining the note that Tailscale provisions the TLS
certificate automatically once those prerequisites are met.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9e896e75-70d3-41e7-acf1-5ba217281b30
📒 Files selected for processing (9)
tailscale/DOCS.mdtailscale/config.yamltailscale/rootfs/etc/s6-overlay/s6-rc.d/share-services/dependencies.d/post-tailscaledtailscale/rootfs/etc/s6-overlay/s6-rc.d/share-services/runtailscale/rootfs/etc/s6-overlay/s6-rc.d/share-services/typetailscale/rootfs/etc/s6-overlay/s6-rc.d/share-services/uptailscale/rootfs/etc/s6-overlay/s6-rc.d/user/contents.d/share-servicestailscale/rootfs/etc/s6-overlay/scripts/stage2_hook.shtailscale/translations/en.yaml
🚧 Files skipped from review as they are similar to previous changes (6)
- tailscale/rootfs/etc/s6-overlay/s6-rc.d/share-services/type
- tailscale/translations/en.yaml
- tailscale/rootfs/etc/s6-overlay/s6-rc.d/share-services/up
- tailscale/rootfs/etc/s6-overlay/scripts/stage2_hook.sh
- tailscale/rootfs/etc/s6-overlay/s6-rc.d/share-services/run
- tailscale/config.yaml
1575c1d to
0b889d6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tailscale/config.yaml`:
- Line 84: Update the target regex in the configuration’s target validation to
enforce the same valid port range, 1–65535, instead of accepting any digits
after 127.0.0.1. Preserve support for both http(s) and tcp schemes and local
ports within the valid range.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b5944bed-00f9-4282-9fd0-f289f39099c0
📒 Files selected for processing (9)
tailscale/DOCS.mdtailscale/config.yamltailscale/rootfs/etc/s6-overlay/s6-rc.d/share-services/dependencies.d/post-tailscaledtailscale/rootfs/etc/s6-overlay/s6-rc.d/share-services/runtailscale/rootfs/etc/s6-overlay/s6-rc.d/share-services/typetailscale/rootfs/etc/s6-overlay/s6-rc.d/share-services/uptailscale/rootfs/etc/s6-overlay/s6-rc.d/user/contents.d/share-servicestailscale/rootfs/etc/s6-overlay/scripts/stage2_hook.shtailscale/translations/en.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
- tailscale/rootfs/etc/s6-overlay/s6-rc.d/share-services/up
- tailscale/rootfs/etc/s6-overlay/scripts/stage2_hook.sh
- tailscale/rootfs/etc/s6-overlay/s6-rc.d/share-services/type
- tailscale/DOCS.md
- tailscale/rootfs/etc/s6-overlay/s6-rc.d/share-services/run
This adds a new `services` configuration option that allows users to advertise local services running on the Home Assistant host as Tailscale Services. A new S6 oneshot service `share-services` reads the configured services and calls `tailscale serve --service=svc:<name>` for each entry. The `svc:` prefix is added automatically; users configure the plain service name. Supported protocols are `http`, `https`, `tcp`, and `tls-terminated-tcp`. Services must be defined and approved in the Tailscale admin console before they can accept traffic. Also updates the app documentation and translations for the new option.
0b889d6 to
3379686
Compare
| @@ -0,0 +1,117 @@ | |||
| #!/command/with-contenv bashio | |||
| # shellcheck shell=bash | |||
| export LOG_FD | |||
There was a problem hiding this comment.
This line is not needed with the new base image with the new bashio. Will be removed in all other places later, with all the temporary workarounds.
| ### Option: `services` | ||
|
|
||
| This option allows you to advertise other local services running on this device | ||
| as [Tailscale Services][tailscale_info_services]. Each service needs a name, a |
There was a problem hiding this comment.
At all other places in DOCS the reference to the TS docs is in a separate line as "More information: ..." This is "important", because the device must to use tags!
|
|
||
| This option allows you to advertise other local services running on this device | ||
| as [Tailscale Services][tailscale_info_services]. Each service needs a name, a | ||
| local target address, a protocol, and a port to expose it on. The `svc:` prefix |
There was a problem hiding this comment.
The advertise_tags option requires the tag: prefix. Maybe we should require svc: here also. What do you think?
There was a problem hiding this comment.
I had the svc: prefix there in the first place, but after configuring it multiple times during testing I felt it annoying. But for parity to the advertise_tags I can revert this change. But honestly I would like the tags also more without the prefix. But that's just a personal preference. If it makes it more clean I'll revert. Just say. ;)
There was a problem hiding this comment.
Agree, we should remove tag: also, in a separate PR. If this PR is accepted, I will make a tag: removing PR. :)
There was a problem hiding this comment.
I would keep it in both places here and do another PR removing them together. So there is no mixed state in between.
| is added automatically by the app; do not include it in the configuration. | ||
|
|
||
| You can use this to expose apps running on your Home Assistant instance, such as | ||
| an audiobookshelf add-on, to your tailnet using a stable MagicDNS name. |
There was a problem hiding this comment.
no add-ons anymore, use apps please
| - name: "match(^[a-zA-Z0-9]([a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?$)" | ||
| target: "match(^(?:https?|tcp)://127\\.0\\.0\\.1:([1-9]\\d{0,3}|[1-5]\\d{4}|6[0-4]\\d{3}|65[0-4]\\d{2}|655[0-2]\\d|6553[0-5])$)" | ||
| protocol: "list(http|https|tcp|tls-terminated-tcp)" | ||
| port: "match(^([1-9]\\d{0,3}|[1-5]\\d{4}|6[0-4]\\d{3}|65[0-4]\\d{2}|655[0-2]\\d|6553[0-5])$)" |
There was a problem hiding this comment.
there is a "port" type, see https://developers.home-assistant.io/docs/apps/configuration#options--schema
| rm /etc/s6-overlay/s6-rc.d/user/contents.d/share-homeassistant | ||
| fi | ||
|
|
||
| # Disable share-services service when no services are configured |
There was a problem hiding this comment.
nope, it always has to start, to be able to remove unused services!!!
|
|
||
| # Disable share-services service when no services are configured | ||
| if ! bashio::config 'services' | jq -e '. | length > 0' > /dev/null; then | ||
| rm -f /etc/s6-overlay/s6-rc.d/user/contents.d/share-services |
There was a problem hiding this comment.
don't follow every advice from an AI, you don't need -f, if it is not there, fail during development :)
| declare -a configured_names=() | ||
| declare -a current_names=() | ||
|
|
||
| # Nothing to do when no services are configured |
There was a problem hiding this comment.
nope, we still have to remove removed services!!!
| # Build a list of configured service names (without the svc: prefix) | ||
| for service in "${configured_services[@]}"; do | ||
| name=$(jq -re '.name' <<< "${service}") | ||
| name="${name#svc:}" |
There was a problem hiding this comment.
do we have svc: prefix there?
There was a problem hiding this comment.
That will be not necessary anymore, when we make svc: mandatory.
There was a problem hiding this comment.
I mean name="${name#svc:}" currently it is not needed, there is no svc: in services.name.
|
|
||
| bashio::log.info "Configured service hosts: ${configured_names[*]}" | ||
|
|
||
| # Wait for tailscaled to be available |
There was a problem hiding this comment.
post-tailscaled already waited for this
| sleep 1 | ||
| done | ||
|
|
||
| while ! /opt/tailscale status --json --peers=false --self=false \ |
There was a problem hiding this comment.
post-tailscaled already waited for this
There was a problem hiding this comment.
Ah, okay that's useful. I haven't caught this.
| fi | ||
|
|
||
| # Build a list of currently configured service names | ||
| readarray -t current_names < <(jq -re '.services | keys[]' <<< "${current_config}" 2>/dev/null || true) |
There was a problem hiding this comment.
why not use bashio::jq() ?
-e and dev/null and ||true, we don't need this, use bashio::jq IMHO
|
|
||
| # Remove services that are no longer configured in this app | ||
| for service_name in "${current_names[@]}"; do | ||
| if [[ "${service_name}" != svc:* ]]; then |
There was a problem hiding this comment.
why do wee need this? just asking! what else can be there?
There was a problem hiding this comment.
It was ment as a safe guard agains when Tailscale should decide to put other things in the .service section then actual services. But you are questioning this correctly. Why should they do it. I'll remove that guard.
| # Compare without the svc: prefix | ||
| if ! printf '%s\n' "${configured_names[@]}" | grep -qx "${service_name#svc:}"; then | ||
| bashio::log.info "Removing stale service host configuration for ${service_name}" | ||
| /opt/tailscale serve drain "${service_name}" >/dev/null 2>&1 || true |
There was a problem hiding this comment.
we should log at least a warning on error (an exit.nok would stop the app, so let it start up, but don't hide the error)
|
|
||
| # Build a list of configured service names (without the svc: prefix) | ||
| for service in "${configured_services[@]}"; do | ||
| name=$(jq -re '.name' <<< "${service}") |
|
|
||
| # Configure and advertise each service | ||
| for service in "${configured_services[@]}"; do | ||
| name=$(jq -re '.name' <<< "${service}") |
| path=$(jq -re '.path // empty' <<< "${service}") | ||
|
|
||
| options=() | ||
| case "${protocol}" in |
There was a problem hiding this comment.
protocol's value is restricted by the config schema, validate only that it is in the accepted list, exit.nok if not (fail during development), then use it directly in a single line
There was a problem hiding this comment.
if ! "${protocol}" = @(http|https|tcp|tls-terminated-tcp); then
exit.nok "Invalid value '${protocol}' for services.protocol"
fi
There was a problem hiding this comment.
I'm not very good in bash. Thanks for the advice.
| ;; | ||
| esac | ||
|
|
||
| if [[ -n "${path}" ]]; then |
| fi | ||
|
|
||
| # Clean any existing configuration for this service before reconfiguring | ||
| /opt/tailscale serve drain "${service_name}" >/dev/null 2>&1 || true |
There was a problem hiding this comment.
at least a warning should be logged when removing, otherwise when the serve below fails during recreation, nobody will know the root cause
| /opt/tailscale serve clear "${service_name}" >/dev/null 2>&1 || true | ||
|
|
||
| bashio::log.info "Advertising service host for ${service_name}" | ||
| if ! /opt/tailscale serve --service="${service_name}" --yes "${options[@]}" "${target}"; then |
There was a problem hiding this comment.
I would add "serve" and "target" to the options array above, this is how it is implemented everywhere else
do we need --yes?
There was a problem hiding this comment.
No it's not anymore. Earlier tailscale serve used to ask interactively when invoking with a service. I tried manually. It's no longer the case. I'll remove it.
There was a problem hiding this comment.
Don't they use it to check if https is enabled and ask whether they should enable it if not? AFAIK they don't do it in a non-interactive cli and just emit an error, but I'm not 100% sure about this. :(
| path=$(jq -re '.path // empty' <<< "${service}") | ||
|
|
||
| options=() | ||
| case "${protocol}" in |
There was a problem hiding this comment.
Another idea. If https or tls-terminated-tcp is used, shouldn't we check once (!!!) that https support is enabled? Like in
https://github.com/hassio-addons/app-tailscale/blob/main/tailscale/rootfs/etc/s6-overlay/s6-rc.d/share-homeassistant/run#L24
Though we can say, that share_homeassistant is for the less experienced users, with more checks. :D
|
And now a general comment. The share_homeassistant option worked similarly a few years ago ("recreating" TS serve/funnel config), and as far as I remember there were complaints, that it comflicted manually configured serve configs, that's why that was changed to a front running serve/funnel with --bg=false. So, when this change is merged, it will delete previously manually configured serve configs for the services. Hmmm, I think we should mark it (potentially) breaking change. The removal is logged, so I think it's fine. |
| name="${name#svc:}" | ||
| service_name="svc:${name}" | ||
| protocol=$(jq -re '.protocol' <<< "${service}") | ||
| port=$(jq -re '.port' <<< "${service}") |
There was a problem hiding this comment.
My last question: can we use the same eg. 443 port here, as the share_homeassistant service uses? Services are running on a virtual tailnet name/IP, so it shouldn't be a problem. I'm asking, because I'm lazy to test it myself. :D
| for service in "${configured_services[@]}"; do | ||
| name=$(jq -re '.name' <<< "${service}") | ||
| name="${name#svc:}" | ||
| configured_names+=("${name}") |
There was a problem hiding this comment.
One more thing, to get configured_names, you can use bashio::config 'services.names' (if I remember correctly), and add some jq or grep to delete the svc: prefixes if needed, so this whole loop can be a one-liner I think.
|
@lmagyar Thanks for all the good feedback. I will work it in over the next week. |
2 participants
Proposed Changes
This adds a new
servicesconfiguration option that allows users to advertise local services running on the Home Assistant host as Tailscale Services.A new S6 oneshot service
share-servicesreads the configured services and callstailscale serve --service=svc:<name>for each entry. Thesvc:prefix is added automatically; users configure the plain service name.Supported protocols are
http,https,tcp, andtls-terminated-tcp. Services must be defined and approved in the Tailscale admin console before they can accept traffic.Also updates the app documentation and translations for the new option.
Related Issues
None. As far as I have looked into the issues.
Summary by CodeRabbit