Skip to content

Make all config options mandatory#541

Merged
frenck merged 8 commits into
hassio-addons:mainfrom
lmagyar:pr-mandatory-options
Feb 5, 2026
Merged

Make all config options mandatory#541
frenck merged 8 commits into
hassio-addons:mainfrom
lmagyar:pr-mandatory-options

Conversation

@lmagyar

@lmagyar lmagyar commented Sep 4, 2025

Copy link
Copy Markdown
Collaborator

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:

  • undefined = use routes from the HA managed/supported interfaces
  • empty list [] = disabled

I've resolved it with a default special value (documented):

options:
  advertise_routes:
    - local_subnets
schema:
  advertise_routes:
    - "match(^(local.subnets|...)$)"

Related Issues

fixes #572

Summary by CodeRabbit

  • New Features

    • Auto-advertise local subnets via a local_subnets sentinel; clearer warning when none found.
  • Changes

    • Several networking/routing flags now require explicit opt-in; defaults made explicit (features no longer implicitly enabled).
    • Validation tightened for ports, tags, login server and log level.
    • share_homeassistant uses only the configured HTTPS port.
  • Documentation

    • Rewritten configuration guidance with step‑by‑step navigation, key expiry advice, and standardized default-value wording.

@coderabbitai

coderabbitai Bot commented Sep 4, 2025

Copy link
Copy Markdown

Walkthrough

Moves many Tailscale behaviors from implicit defaults to explicit opt-in, tightens the add-on config schema (many fields made required/stricter), introduces a local_subnets sentinel for route advertisement, and updates runtime scripts, service gating, subnet-route tooling, docs and translations to act on explicit configuration values.

Changes

Cohort / File(s) Summary
Documentation
tailscale/DOCS.md
Rewrote configuration guidance into step-by-step UI instructions, added [tailscale_machines] anchor, standardized default wording and port phrasing.
Config schema & defaults
tailscale/config.yaml
Added top-level options mapping; converted many optional fields to required/stricter types/patterns; tightened regexes/allowed values; added local_subnets sentinel support in advertise_routes.
Post-start flags & exit-node
tailscale/rootfs/etc/s6-overlay/s6-rc.d/post-tailscaled/run
Flag handling changed to require explicit true for many options (accept_dns, accept_routes, advertise_*, snat/stateful, taildrop, userspace_networking); always passes --login-server; exit-node and advertise_routes handling tightened and supports local_subnets.
Daemon TUN setup
tailscale/rootfs/etc/s6-overlay/s6-rc.d/tailscaled/run
--tun=userspace-networking is added only when userspace_networking is explicitly true (removed prior implicit default-on path).
Stage2 service gating
tailscale/rootfs/etc/s6-overlay/scripts/stage2_hook.sh
Service removal/enablement decisions (MagicDNS proxies, protect-subnets, forwarding, mss-clamping, taildrop, share-homeassistant) now rely on explicit config values rather than implicit/default branches; advertise_routes logic uses local_subnets sentinel.
Subnet advertisement tooling
tailscale/rootfs/usr/bin/subnet-routes
Advertised mode always processes advertise_routes; local_subnets triggers local subnet discovery (subnet-routes local) and warns if none; address collection/filtering improved; duplicates deduped.
Share Home Assistant service
tailscale/rootfs/etc/s6-overlay/s6-rc.d/share-homeassistant/run
Removed fallback default HTTPS port argument — only configured share_on_port passed.
DNS init & proxies
tailscale/rootfs/etc/s6-overlay/s6-rc.d/init-magicdns-proxies-upstream-list/run, .../magicdns-ingress-proxy/run
Gated login_server resolution and blacklist/whitelist behavior on explicit accept_dns true/false rather than treating unset as true.
Translations
tailscale/translations/en.yaml
Standardized default phrasing, documented advertise_routes default (mentions local_subnets), added log_level and login_server entries.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Sys as s6/System
  participant Post as post-tailscaled
  participant TS as tailscaled
  participant CFG as config.yaml
  participant LOG as Logger

  Note over CFG: Flags must be explicit true/false
  Sys->>TS: start tailscaled
  TS-->>Sys: ready
  Sys->>Post: run post-tailscaled
  Post->>CFG: read flags (accept_dns, accept_routes, advertise_*, userspace_networking, snat/stateful, tags)
  alt flag == true
    Post->>TS: append --<flag>=true
  else
    Post->>TS: append --<flag>=false
  end
  Post->>TS: append --login-server=<url> (always)
  alt advertise_exit_node == true and exit_node set
    Post->>LOG: warn about exit-node conflict
  end
Loading
sequenceDiagram
  autonumber
  participant Caller as Caller
  participant SR as subnet-routes
  participant CFG as config
  participant IF as NetworkInterfaces
  participant LOG as Logger

  Caller->>SR: invoke mode ("advertised" or other)
  alt mode = "advertised"
    SR->>CFG: read advertise_routes
    alt contains "local_subnets"
      SR->>IF: run `subnet-routes local` -> collect local subnets
      alt none found
        SR->>LOG: warn "There are no local subnets to advertise!"
      end
    end
    SR->>SR: merge explicit CIDRs from advertise_routes (skip sentinel)
  else
    SR->>IF: collect interface IPs and extract networks
  end
  SR->>SR: deduplicate & sort routes
  SR-->>Caller: emit routes
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

  • hassio-addons/app-tailscale#616 — modifies DNS/userspace_networking conditionals and init/stage2 scripts; closely overlaps gating changes here.
  • hassio-addons/app-tailscale#455 — touches MagicDNS and advertise_routes/userspace behavior and stage2_hook.sh; overlaps on config/flag handling.
  • hassio-addons/app-tailscale#618 — adjusts login_server deduction/accept_dns gating in init scripts; related to login_server handling changes.

Suggested reviewers

  • frenck

Poem

I hopped through configs, tidy and bright,
Defaults now chosen by clearer light.
Sentinels fetch subnets, neat and small,
Flags stand up straight — no more guessing at all.
A rabbit munches logs and gives a cheer 🥕🐇

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main objective of the PR: converting previously optional configuration options to mandatory fields.
Linked Issues check ✅ Passed The PR successfully addresses issue #572 by making configuration options mandatory with explicit defaults, ensuring UI-behavior consistency and preventing default toggles.
Out of Scope Changes check ✅ Passed All changes are directly related to making config options mandatory and aligning defaults with actual behavior, with no unrelated modifications detected.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@frenck frenck added the refactor Improvement of existing code, not introducing new features. label Sep 14, 2025
@lmagyar lmagyar force-pushed the pr-mandatory-options branch from 0270a14 to d2acb2c Compare September 14, 2025 21:51
@lmagyar lmagyar marked this pull request as ready for review September 14, 2025 21:54

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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::/64
tailscale/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

📥 Commits

Reviewing files that changed from the base of the PR and between 8a30992 and d2acb2c.

📒 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/run
  • tailscale/rootfs/etc/s6-overlay/s6-rc.d/share-homeassistant/run
  • tailscale/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 literal local.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 true

Verified: config.yaml contains userspace_networking: true (line 45) and the schema lists userspace_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 negatives

comm 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.

Comment thread tailscale/config.yaml
Comment thread tailscale/DOCS.md Outdated
Comment thread tailscale/rootfs/etc/s6-overlay/scripts/stage2_hook.sh
Comment thread tailscale/rootfs/etc/s6-overlay/scripts/stage2_hook.sh
Comment thread tailscale/translations/en.yaml Outdated
@lmagyar lmagyar marked this pull request as draft September 15, 2025 00:14
@lmagyar

lmagyar commented Sep 17, 2025

Copy link
Copy Markdown
Collaborator Author

I've reviewed all the AI reviews, it's ready for human review.

@lmagyar lmagyar marked this pull request as ready for review September 17, 2025 23:07
@github-actions

Copy link
Copy Markdown

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 github-actions Bot added stale There has not been activity on this issue or PR for quite some time. and removed stale There has not been activity on this issue or PR for quite some time. labels Oct 18, 2025
@lmagyar lmagyar added the no-stale This issue or PR is exempted from the stable bot. label Oct 22, 2025
@frenck frenck requested a review from Copilot October 27, 2025 05:22

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_node which remains optional)
  • Introduced local_subnets as a special value for advertise_routes to 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.

Comment thread tailscale/rootfs/etc/s6-overlay/scripts/stage2_hook.sh Outdated
Comment thread tailscale/rootfs/etc/s6-overlay/s6-rc.d/post-tailscaled/run Outdated
Comment thread tailscale/config.yaml Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
tailscale/rootfs/usr/bin/subnet-routes (1)

27-41: Consider using appendarray for defensive coding.

The logic correctly handles the local_subnets sentinel value. However, at line 32, readarray -t routes overwrites the routes array rather than appending to it. If the configuration contains multiple local_subnets entries (though unlikely), each subsequent occurrence would discard the previous results.

Consider applying this diff to use the existing appendarray helper:

       if [[ "${address}" =~ ^local_subnets$ ]]; then
         # Handle special value, collect local subnets
-        readarray -t routes < <(subnet-routes local)
+        appendarray routes < <(subnet-routes local)
         if (( 0 == ${#routes[@]} )); then

This makes the code more defensive by appending rather than overwriting. Any resulting duplicates will be removed by the deduplication at line 73.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7dedb85 and e1fae32.

📒 Files selected for processing (3)
  • tailscale/config.yaml (1 hunks)
  • tailscale/rootfs/etc/s6-overlay/scripts/stage2_hook.sh (2 hunks)
  • tailscale/rootfs/usr/bin/subnet-routes (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • tailscale/rootfs/etc/s6-overlay/scripts/stage2_hook.sh
  • tailscale/config.yaml
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: lmagyar
Repo: hassio-addons/addon-tailscale PR: 541
File: tailscale/config.yaml:30-37
Timestamp: 2025-09-16T23:47:20.987Z
Learning: In the hassio-addons/addon-tailscale project, the advertise_routes configuration uses `local_subnets` (with underscore) as the default value, and the runtime regex patterns use `local.subnets` (with dot) which correctly matches the underscore variant because '.' in regex matches any single character including '_'.
Learnt from: lmagyar
Repo: hassio-addons/addon-tailscale PR: 509
File: tailscale/config.yaml:0-0
Timestamp: 2025-09-17T23:53:19.467Z
Learning: In the Tailscale add-on config.yaml, there is an intentional naming mismatch between map entries and schema field names: map entries use actual Home Assistant directory names (like `homeassistant_config`, `all_addon_configs`) while schema fields use user-friendly names (like `config`, `addon_configs`). This allows correct directory mapping while presenting cleaner option names in the UI.
📚 Learning: 2025-09-16T23:47:20.987Z
Learnt from: lmagyar
Repo: hassio-addons/addon-tailscale PR: 541
File: tailscale/config.yaml:30-37
Timestamp: 2025-09-16T23:47:20.987Z
Learning: In the hassio-addons/addon-tailscale project, the advertise_routes configuration uses `local_subnets` (with underscore) as the default value, and the runtime regex patterns use `local.subnets` (with dot) which correctly matches the underscore variant because '.' in regex matches any single character including '_'.

Applied to files:

  • tailscale/rootfs/usr/bin/subnet-routes
📚 Learning: 2025-11-06T10:55:55.062Z
Learnt from: lmagyar
Repo: hassio-addons/addon-tailscale PR: 566
File: tailscale/rootfs/usr/bin/subnet-routes:1-2
Timestamp: 2025-11-06T10:55:55.062Z
Learning: In the hassio-addons/addon-tailscale project, utility scripts in `/usr/bin/` (like `subnet-routes`, `protect-subnet-routes`, `unprotect-subnet-routes`) are invoked from s6-overlay service scripts or NetworkManager dispatchers that already use `#!/command/with-contenv bashio`. These utilities inherit the environment from their parent processes and therefore use `#!/usr/bin/env bashio` rather than `#!/command/with-contenv bashio`.

Applied to files:

  • tailscale/rootfs/usr/bin/subnet-routes
📚 Learning: 2025-10-29T20:47:02.818Z
Learnt from: lmagyar
Repo: hassio-addons/addon-tailscale PR: 562
File: tailscale/rootfs/etc/s6-overlay/s6-rc.d/post-tailscaled/run:120-128
Timestamp: 2025-10-29T20:47:02.818Z
Learning: In the hassio-addons/addon-tailscale project, when checking for IPv6 addresses in bash scripts, use the regex format `if [[ "${variable}" =~ .*:.* ]]; then` for consistency with other IPv6 tests in the codebase.

Applied to files:

  • tailscale/rootfs/usr/bin/subnet-routes
🔇 Additional comments (2)
tailscale/rootfs/usr/bin/subnet-routes (2)

33-36: LGTM!

The warning appropriately alerts users when local_subnets is configured but no local subnets are available, and clearly states that a restart is required after fixing the underlying issue.


50-50: LGTM!

The comment updates improve clarity by explicitly stating what is being processed: "from addresses" at line 50 and "from routes" at line 72.

Also applies to: 72-72

@lmagyar

lmagyar commented Nov 11, 2025

Copy link
Copy Markdown
Collaborator Author

Consider using appendarray for defensive coding.

No. This is fine. After that for, the routes array is an empty list or contains the subnet routes once (even if the user added the "subnet_routes" value multiple times. Later, we add routes from the address array to it, then filter out duplicates. So appendarray is unnecessary, but using it at least would not break the code.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e1fae32 and 647cde2.

📒 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)
🚧 Files skipped from review as they are similar to previous changes (6)
  • tailscale/rootfs/etc/s6-overlay/s6-rc.d/share-homeassistant/run
  • tailscale/rootfs/etc/s6-overlay/s6-rc.d/post-tailscaled/run
  • tailscale/rootfs/usr/bin/subnet-routes
  • tailscale/DOCS.md
  • tailscale/rootfs/etc/s6-overlay/scripts/stage2_hook.sh
  • tailscale/translations/en.yaml
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: lmagyar
Repo: hassio-addons/addon-tailscale PR: 541
File: tailscale/config.yaml:30-37
Timestamp: 2025-09-16T23:47:20.987Z
Learning: In the hassio-addons/addon-tailscale project, the advertise_routes configuration uses `local_subnets` (with underscore) as the default value, and the runtime regex patterns use `local.subnets` (with dot) which correctly matches the underscore variant because '.' in regex matches any single character including '_'.
📚 Learning: 2025-09-17T23:53:19.467Z
Learnt from: lmagyar
Repo: hassio-addons/addon-tailscale PR: 509
File: tailscale/config.yaml:0-0
Timestamp: 2025-09-17T23:53:19.467Z
Learning: In the Tailscale add-on config.yaml, there is an intentional naming mismatch between map entries and schema field names: map entries use actual Home Assistant directory names (like `homeassistant_config`, `all_addon_configs`) while schema fields use user-friendly names (like `config`, `addon_configs`). This allows correct directory mapping while presenting cleaner option names in the UI.

Applied to files:

  • tailscale/config.yaml
📚 Learning: 2025-09-16T23:47:20.987Z
Learnt from: lmagyar
Repo: hassio-addons/addon-tailscale PR: 541
File: tailscale/config.yaml:30-37
Timestamp: 2025-09-16T23:47:20.987Z
Learning: In the hassio-addons/addon-tailscale project, the advertise_routes configuration uses `local_subnets` (with underscore) as the default value, and the runtime regex patterns use `local.subnets` (with dot) which correctly matches the underscore variant because '.' in regex matches any single character including '_'.

Applied to files:

  • tailscale/config.yaml
📚 Learning: 2025-10-29T20:47:02.818Z
Learnt from: lmagyar
Repo: hassio-addons/addon-tailscale PR: 562
File: tailscale/rootfs/etc/s6-overlay/s6-rc.d/post-tailscaled/run:120-128
Timestamp: 2025-10-29T20:47:02.818Z
Learning: In the hassio-addons/addon-tailscale project, when checking for IPv6 addresses in bash scripts, use the regex format `if [[ "${variable}" =~ .*:.* ]]; then` for consistency with other IPv6 tests in the codebase.

Applied to files:

  • tailscale/config.yaml
📚 Learning: 2024-11-15T17:38:56.576Z
Learnt from: lmagyar
Repo: hassio-addons/addon-tailscale PR: 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/config.yaml
  • tailscale/rootfs/etc/s6-overlay/s6-rc.d/tailscaled/run
📚 Learning: 2025-11-06T10:55:55.062Z
Learnt from: lmagyar
Repo: hassio-addons/addon-tailscale PR: 566
File: tailscale/rootfs/usr/bin/subnet-routes:1-2
Timestamp: 2025-11-06T10:55:55.062Z
Learning: In the hassio-addons/addon-tailscale project, utility scripts in `/usr/bin/` (like `subnet-routes`, `protect-subnet-routes`, `unprotect-subnet-routes`) are invoked from s6-overlay service scripts or NetworkManager dispatchers that already use `#!/command/with-contenv bashio`. These utilities inherit the environment from their parent processes and therefore use `#!/usr/bin/env bashio` rather than `#!/command/with-contenv bashio`.

Applied to files:

  • tailscale/config.yaml
  • tailscale/rootfs/etc/s6-overlay/s6-rc.d/tailscaled/run
📚 Learning: 2025-09-16T23:47:20.987Z
Learnt from: lmagyar
Repo: hassio-addons/addon-tailscale PR: 541
File: tailscale/config.yaml:30-37
Timestamp: 2025-09-16T23:47:20.987Z
Learning: In regex patterns, '.' matches any single character, so `local.subnets` in a regex would match `local_subnets`, `localXsubnets`, etc. The user lmagyar correctly pointed this out when I mistakenly identified it as a literal string mismatch.

Applied to files:

  • tailscale/config.yaml
📚 Learning: 2025-09-17T21:52:47.107Z
Learnt from: lmagyar
Repo: hassio-addons/addon-tailscale PR: 541
File: tailscale/rootfs/etc/s6-overlay/scripts/stage2_hook.sh:68-72
Timestamp: 2025-09-17T21:52:47.107Z
Learning: I made a critical regex error by claiming that the pattern "local.subnets" wouldn't match "local_subnets". In regex, '.' matches any single character including '_', so this pattern works correctly. The user lmagyar had to correct this same basic regex misunderstanding twice.

Applied to files:

  • tailscale/config.yaml
🔇 Additional comments (3)
tailscale/config.yaml (2)

31-32: Verify that advertise_exit_node and advertise_connector should default to true.

Based on issue #572, "settings that require explicit action in the Tailscale admin UI and are not enabled by default in the stock client should be disabled by default in this add-on."

Both exit nodes and connectors require explicit setup and approval in the Tailscale admin console, and are not enabled by default in the stock Tailscale client. Setting these to true by default appears to contradict the stated objective of this PR.

Please verify with the project maintainers whether these should default to false instead:

- advertise_exit_node: true
+ advertise_exit_node: false
- advertise_connector: true
+ advertise_connector: false

44-61: Schema updates correctly enforce mandatory configuration.

The schema changes properly make all fields required (except exit_node on line 51), which aligns with the PR objective. The consistency between the options defaults and schema definitions is maintained.

tailscale/rootfs/etc/s6-overlay/s6-rc.d/tailscaled/run (1)

26-29: Explicit true check correctly implements mandatory config behavior.

The updated condition properly checks for an explicit true value in the configuration, which aligns with the PR's shift from implicit defaults to explicit mandatory configuration. The comment accurately describes the behavior.

Comment thread tailscale/config.yaml

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In `@tailscale/config.yaml`:
- Line 32: The default for the Tailscale config currently sets
advertise_exit_node: true; change this default to false by updating the
advertise_exit_node setting in the Tailscale configuration (the
advertise_exit_node key) so exit-node advertising is disabled by default and
must be explicitly enabled by the user or admin via the Tailscale console.
- Line 33: The default for the app connector flag is incorrect: change the
advertise_connector setting from true to false in the configuration so the addon
does not advertise as an app connector by default; update any related defaults
or schema that reference advertise_connector and adjust tests or examples that
assume the old true default to use false or explicit opt-in instead.

Comment thread tailscale/config.yaml
Comment thread tailscale/config.yaml
@lmagyar lmagyar force-pushed the pr-mandatory-options branch from b266b6e to 6902134 Compare February 3, 2026 15:25
lmagyar and others added 4 commits February 3, 2026 21:59
@lmagyar lmagyar force-pushed the pr-mandatory-options branch from 6902134 to a220dba Compare February 3, 2026 21:01

@frenck frenck left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's go! 🚀

../Frenck

                       

Blogging my personal ramblings at frenck.dev

@frenck frenck merged commit 1536ffd into hassio-addons:main Feb 5, 2026
13 checks passed
@lmagyar lmagyar deleted the pr-mandatory-options branch February 5, 2026 15:21
@github-actions github-actions Bot locked and limited conversation to collaborators Feb 7, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

no-stale This issue or PR is exempted from the stable bot. refactor Improvement of existing code, not introducing new features.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Settings UI doesn't reflect settings _behaviour_.

3 participants