Make all config options mandatory #541
Conversation
WalkthroughShifts many Tailscale defaults to explicit opt-in, tightens the config schema (required booleans, stricter patterns, new options/defaults), introduces local.subnets sentinel for route advertisement, changes runtime scripts to rely on explicit values, and rewrites docs/translations to reflect the new explicit semantics. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant S6 as s6
participant PT as post-tailscaled/run
participant TS as tailscaled
participant CFG as config.yaml
participant LOG as Logger
Note over CFG: Booleans now explicit required values
S6->>TS: start tailscaled
TS-->>S6: ready
S6->>PT: run post-tailscaled
PT->>CFG: read flags (accept_dns, accept_routes, advertise_*, userspace_networking...)
alt flag == true
PT->>TS: append --<flag>=true
else flag == false
PT->>TS: append --<flag>=false
end
PT->>TS: append --login-server=<url> (unconditional)
alt advertise_exit_node=true and exit_node set
PT->>LOG: warn exit-node conflict
end
sequenceDiagram
autonumber
participant SR as subnet-routes
participant CFG as config
participant IF as Interfaces
participant IPC as ipcalc
participant LOG as Logger
SR->>CFG: mode arg ("advertised" or not)
alt mode = "advertised"
SR->>CFG: read advertise_routes
alt contains "local.subnets"
SR->>IF: collect local subnets
alt none
SR->>LOG: warn "There are no local subnets to advertise!"
end
end
SR->>SR: merge explicit CIDRs
else
SR->>IF: collect IP addresses
SR->>IPC: extract networks from addresses
end
SR->>SR: deduplicate routes (sort -u)
SR-->>Caller: emit routes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
frenck
added
the
refactor
lmagyar
force-pushed
the
pr-mandatory-options
branch
from
0270a14 to
d2acb2c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tailscale/rootfs/usr/bin/subnet-routes (1)
50-58: Exclude loopback addresses to avoid advertising 127.0.0.0/8 or ::1/128.Current filters skip link-local but not loopback. With local.subnets, this could generate bogus routes.
Apply this diff:
# Extract routes from addresses for address in "${addresses[@]}"; do if bashio::var.has_value "${address}"; then + # Skip loopback addresses + if [[ "${address}" == 127.* ]] || [[ "${address:0:3}" == "::1" ]]; then + continue + fi # Skip local link addresses if [[ "${address:0:6}" == "fe80::" ]] || [[ "${address:0:8}" == "169.254." ]]; then continue fi
🧹 Nitpick comments (2)
tailscale/DOCS.md (1)
66-86: Consider showing the local.subnets sentinel in the sample YAML.Helps users discover the new default/shortcut.
Apply this diff:
advertise_routes: - - 192.168.1.0/24 + - local.subnets # Advertise all local subnets discovered on supported interfaces + - 192.168.1.0/24 - fd12:3456:abcd::/64tailscale/rootfs/etc/s6-overlay/s6-rc.d/share-homeassistant/run (1)
15-19: Gracefully handle share_homeassistant=disabled in case gating fails.If stage removal ever regresses, this service would crash-loop on “disabled”. Early no-op is safer.
Apply this diff:
# Validate share_homeassistant value -if ! bashio::config.equals 'share_homeassistant' 'serve' && \ - ! bashio::config.equals 'share_homeassistant' 'funnel' -then - bashio::exit.nok "Invalid value '$(bashio::config 'share_homeassistant')' for share_homeassistant. Must be either 'serve' or 'funnel'" -fi +if bashio::config.equals 'share_homeassistant' 'disabled'; then + bashio::log.info "share_homeassistant=disabled; skipping Serve/Funnel setup." + exit 0 +fi +if ! bashio::config.equals 'share_homeassistant' 'serve' && \ + ! bashio::config.equals 'share_homeassistant' 'funnel'; then + bashio::exit.nok "Invalid value '$(bashio::config 'share_homeassistant')' for share_homeassistant. Must be 'serve', 'funnel', or 'disabled'." +fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
tailscale/DOCS.md(12 hunks)tailscale/config.yaml(1 hunks)tailscale/rootfs/etc/s6-overlay/s6-rc.d/post-tailscaled/run(4 hunks)tailscale/rootfs/etc/s6-overlay/s6-rc.d/share-homeassistant/run(1 hunks)tailscale/rootfs/etc/s6-overlay/s6-rc.d/tailscaled/run(1 hunks)tailscale/rootfs/etc/s6-overlay/scripts/stage2_hook.sh(2 hunks)tailscale/rootfs/usr/bin/subnet-routes(3 hunks)tailscale/translations/en.yaml(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2024-11-15T17:38:56.576Z
Learnt from: lmagyar
PR: hassio-addons/addon-tailscale#419
File: tailscale/rootfs/command/with-contenv-merge:8-12
Timestamp: 2024-11-15T17:38:56.576Z
Learning: The script `with-contenv-merge` in `tailscale/rootfs/command/` is directly copied from s6 and should not be modified.
Applied to files:
tailscale/rootfs/etc/s6-overlay/s6-rc.d/tailscaled/runtailscale/rootfs/etc/s6-overlay/s6-rc.d/share-homeassistant/runtailscale/rootfs/etc/s6-overlay/s6-rc.d/post-tailscaled/run
📚 Learning: 2024-10-09T10:32:14.110Z
Learnt from: mikz
PR: hassio-addons/addon-tailscale#407
File: tailscale/rootfs/etc/s6-overlay/s6-rc.d/init-nginx/run:24-29
Timestamp: 2024-10-09T10:32:14.110Z
Learning: In the Tailscale add-on for Home Assistant, the `tailscale/rootfs` directory is copied into the container's root directory via the Dockerfile, ensuring that files like `/etc/nginx/templates/homeassistant.gtpl` are present at runtime.
Applied to files:
tailscale/DOCS.md
📚 Learning: 2025-06-11T20:37:01.152Z
Learnt from: lmagyar
PR: hassio-addons/addon-tailscale#455
File: tailscale/rootfs/usr/bin/magicdns-ingress-proxy-forwarding:100-106
Timestamp: 2025-06-11T20:37:01.152Z
Learning: In the Home Assistant Tailscale add-on, DNATing DNS packets to port 0 (e.g. `--to-destination 127.0.0.1:0`) is an intentional technique to drop traffic. iptables accepts port 0, rewrites the destination port to 0, and the packet is rejected by the stack, effectively discarding it. This is valid and should not be flagged as an error in future reviews.
Applied to files:
tailscale/DOCS.md
🔇 Additional comments (19)
tailscale/translations/en.yaml (1)
8-8: LGTM on standardizing the “enabled/disabled by default” phrasing.Consistent with the “mandatory config” direction and clearer for users.
Also applies to: 14-14, 21-21, 29-29, 63-63, 70-70, 78-78, 85-85, 96-96, 104-104
tailscale/DOCS.md (1)
294-294: LGTM: default port wording.Matches the tightened schema and the run script using the configured port.
tailscale/rootfs/usr/bin/subnet-routes (2)
72-74: LGTM: unique the route list.Ensures stable, deduplicated output for downstream consumers.
27-41: Approve — schema already allows literallocal.subnets
config.yaml advertise_routes pattern includes the literal "local.subnets" (line 52); no schema change required.tailscale/rootfs/etc/s6-overlay/s6-rc.d/share-homeassistant/run (1)
72-72: LGTM: use configured HTTPS port.
Uses the schema-constrained value instead of hardcoding 443.
Stage gating verified: stage2 hook removes /etc/s6-overlay/s6-rc.d/user/contents.d/share-homeassistant when share_homeassistant is 'disabled'.tailscale/rootfs/etc/s6-overlay/s6-rc.d/tailscaled/run (1)
26-29: LGTM — only enable userspace networking when explicitly true; default verified trueVerified: config.yaml contains
userspace_networking: true(line 45) and the schema listsuserspace_networking: bool(line 63).tailscale/rootfs/etc/s6-overlay/scripts/stage2_hook.sh (4)
62-66: LGTM: gating for protect-subnets matches the new explicit semantics.
75-77: LGTM: forwarding removal correctly keys off userspace networking.
80-82: LGTM: mss-clamping removal aligns with userspace networking constraints.
90-92: LGTM: share-homeassistant only removed when explicitly disabled.tailscale/rootfs/etc/s6-overlay/s6-rc.d/post-tailscaled/run (8)
19-25: LGTM: explicit accept-dns/accept-routes flags with explicit false when not enabled.Also applies to: 26-32
34-38: LGTM: exit-node conflict guard and explicit advertise-exit-node toggling are correct.Also applies to: 40-46
57-63: LGTM: advertise-connector is now explicit; good.
65-67: LGTM: login_server always passed; consistent with required schema default.
83-85: LGTM: tags join is safe; empty list yields empty string.
170-178: LGTM: userspace networking notice gated on explicit true.
68-74: No action needed — bundled Tailscale (v1.88.1) supports these flags.
tailscale/Dockerfile sets ARG TAILSCALE_VERSION="v1.88.1"; --stateful-filtering was added in v1.66.0 and --snat-subnet-routes is available, so these flags are supported and startup should not fail.
150-156: Sort both comm inputs to avoid false negativescomm requires both inputs to be sorted; sort the local routes before comparing.
File: tailscale/rootfs/etc/s6-overlay/s6-rc.d/post-tailscaled/run (around lines 150-156)
- comm -1 -2 \ - <(subnet-routes local) \ - <(/opt/tailscale status --json --peers=true --self=false \ + comm -1 -2 \ + <(subnet-routes local | sort -u) \ + <(/opt/tailscale status --json --peers=true --self=false \ | jq -rc '.Peer[] | select(has("PrimaryRoutes")) | .PrimaryRoutes[]' \ | sort -u))Run on a device with multiple peers to confirm expected warnings.
tailscale/config.yaml (1)
47-63: LGTM: booleans now required; patterns and enums look consistent with script expectations.
|
I've reviewed all the AI reviews, it's ready for human review. |
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. Thank you for your contributions. |
github-actions
bot
added
stale
There was a problem hiding this comment.
Pull Request Overview
This PR makes configuration options mandatory rather than optional, standardizing the add-on's behavior by requiring explicit user choices for all settings. The change removes ambiguity about default behaviors and ensures consistent configuration across installations.
Key Changes:
- Made all configuration options mandatory with explicit defaults (except
exit_nodewhich remains optional) - Introduced
local_subnetsas a special value foradvertise_routesto auto-discover and advertise local subnet routes - Updated documentation to reflect that defaults are now explicitly set rather than implicitly applied when options are unset
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tailscale/config.yaml | Added options section with default values for all configuration parameters and removed ? optional markers from schema definitions |
| tailscale/translations/en.yaml | Updated configuration descriptions to replace "When not set, this option is..." with "This option is..." for clarity |
| tailscale/DOCS.md | Enhanced documentation with step-by-step UI instructions for enabling features and updated default value wording |
| tailscale/rootfs/etc/s6-overlay/scripts/stage2_hook.sh | Simplified conditional logic by removing checks for undefined configuration values |
| tailscale/rootfs/etc/s6-overlay/s6-rc.d/tailscaled/run | Removed checks for undefined userspace_networking configuration |
| tailscale/rootfs/etc/s6-overlay/s6-rc.d/post-tailscaled/run | Simplified conditionals by removing undefined value checks and updated tags configuration handling |
| tailscale/rootfs/etc/s6-overlay/s6-rc.d/share-homeassistant/run | Removed default value fallback for share_on_port |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # If advertise_routes is configured, do not wait for the local network to be ready to collect subnet information | ||
| if bashio::config.exists "advertise_routes"; | ||
| # If local subnets are not configured in advertise_routes, do not wait for the local network to be ready to collect subnet information | ||
| if ! bashio::config "advertise_routes" | grep -Eq "^local.subnets$"; |
There was a problem hiding this comment.
The regex pattern ^local.subnets$ uses a literal dot which matches any character. It should be ^local_subnets$ to match the exact string 'local_subnets' as defined in the configuration.
| if ! bashio::config "advertise_routes" | grep -Eq "^local.subnets$"; | |
| if ! bashio::config "advertise_routes" | grep -Eq "^local_subnets$"; |
|
|
||
| # Get configured tags | ||
| tags=$(bashio::config "tags//[] | join(\",\")" "") | ||
| tags=$(bashio::config "tags | join(\",\")" "") |
There was a problem hiding this comment.
Removed the //[] fallback operator which handled the case when tags is not an array. Since tags now defaults to an empty array [] in config.yaml, this change appears safe. However, the second argument \"\" is now redundant since tags will always be an array and never undefined.
| tags=$(bashio::config "tags | join(\",\")" "") | |
| tags=$(bashio::config "tags | join(\",\")") |
| advertise_connector: bool | ||
| advertise_routes: | ||
| - "match(^(((25[0-5]|(2[0-4]|1\\d|[1-9]?)\\d)\\.){3}(25[0-5]|(2[0-4]|1\\d|[1-9]?)\\d)\\/(3[0-2]|[12]?\\d)|[a-fA-F\\d.:]+:[a-fA-F\\d.:]+\\/(12[0-8]|(1[01]|[1-9]?)\\d))$)?" | ||
| - "match(^(?:local.subnets|(?:(?:25[0-5]|(?:2[0-4]|1\\d|[1-9]?)\\d)\\.){3}(?:25[0-5]|(?:2[0-4]|1\\d|[1-9]?)\\d)\\/(?:3[0-2]|[12]?\\d)|[a-fA-F\\d.:]+:[a-fA-F\\d.:]+\\/(?:12[0-8]|(?:1[01]|[1-9]?)\\d))$)" |
There was a problem hiding this comment.
The regex pattern local.subnets uses a literal dot which matches any character. It should be local_subnets to match the exact string as used in the default configuration and documentation.
| - "match(^(?:local.subnets|(?:(?:25[0-5]|(?:2[0-4]|1\\d|[1-9]?)\\d)\\.){3}(?:25[0-5]|(?:2[0-4]|1\\d|[1-9]?)\\d)\\/(?:3[0-2]|[12]?\\d)|[a-fA-F\\d.:]+:[a-fA-F\\d.:]+\\/(?:12[0-8]|(?:1[01]|[1-9]?)\\d))$)" | |
| - "match(^(?:local_subnets|(?:(?:25[0-5]|(?:2[0-4]|1\\d|[1-9]?)\\d)\\.){3}(?:25[0-5]|(?:2[0-4]|1\\d|[1-9]?)\\d)\\/(?:3[0-2]|[12]?\\d)|[a-fA-F\\d.:]+:[a-fA-F\\d.:]+\\/(?:12[0-8]|(?:1[01]|[1-9]?)\\d))$)" |
2 participants
Proposed Changes
This is a follow up after #463.
I've made all the optional config options mandatory, except exit node, becasue that is really an optional "string".
1 option was problematic: advertise_routes
Where:
I've resolved it with a default special value (documented):
Related Issues
Summary by CodeRabbit
New Features
Changes
Documentation