Refactor MagicDNS support to properly handle appconnectors#667
Open
lmagyar wants to merge 19 commits into
Open
Refactor MagicDNS support to properly handle appconnectors#667lmagyar wants to merge 19 commits into
lmagyar wants to merge 19 commits into
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 (3)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughDynamic port allocation replaces fixed port 53 for both egress and ingress DNS proxies; domain filtering switches from static ChangesMagicDNS Proxies & Orchestration Refactor
Sequence Diagram(s)sequenceDiagram
participant IPN as Tailscale IPN
participant Reconf as reconfigurator
participant Config as configure test
participant Egress as egress-proxy
participant Ingress as ingress-proxy
participant FWD as forwarding scripts
loop watch DNS state
IPN->>Reconf: DNS state change
Reconf->>Config: run configure test
alt test unchanged
Reconf->>Reconf: write HEALTHY
else test changed
Reconf->>FWD: remove ingress forwarding
Reconf->>Egress: s6-svc -ruwR
Reconf->>Ingress: create suppress marker
Reconf->>Ingress: s6-svc -ruwR
Reconf->>Ingress: wait file_non_empty /run/dnsmasq_ingress_port
Reconf->>FWD: re-add forwarding with persisted port
Reconf->>FWD: remove drop state
Reconf->>Reconf: delete stored port file
Reconf->>Reconf: write HEALTHY
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
lmagyar
added
the
bugfix
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tailscale/rootfs/usr/bin/magicdns-egress-proxy-forwarding`:
- Around line 42-47: The loop that parses proto and port uses non-POSIX regex
tokens in the grep and sed patterns; update the grep -E pattern to replace \s
with [[:space:]] and \d with [0-9], and replace the non-greedy .*? with greedy
constructs that limit scope (e.g., use [^[:space:]]+ or [^:]+ where appropriate)
so the pattern reliably matches "-p <proto>" and the destination port; likewise
change the sed -nr pattern to capture the protocol and port with
POSIX-compatible groups like ([^[:space:]]+) and ([0-9]+) instead of \S and \d,
keeping the same capture order used by IFS='|' read -r proto to_port.
In `@tailscale/rootfs/usr/bin/magicdns-ingress-proxy-forwarding`:
- Around line 63-68: The grep -E and sed -r patterns in the for loop use PCRE
constructs (.*?, \s, \S, \d) which sed/grep POSIX ERE don't support; update the
grep pattern and the sed substitution to use POSIX ERE classes and greedy
matching (e.g. replace \s with [[:space:]], \S with [^[:space:]] or [[:graph:]]
and \d with [0-9]+, and change .*? to .*). Specifically modify the ${cmd} ... |
{ grep -E "..."; } ... | sed -r 's/^.*?-p\s(\S+).*?:(\d+)$/\1|\2/p' pipeline so
the grep uses [[:space:]] and [0-9]+ and the sed uses -r (or -E) with a pattern
like 's/^.*-p[[:space:]]+([^[:space:]]+).*:([0-9]+)$/\1|\2/p' so that the for
variables loop and the IFS='|' read -r proto to_port correctly extract proto and
to_port.
🪄 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: 0c3c44cb-ea76-4e91-b514-8f55ebf7a674
📒 Files selected for processing (6)
tailscale/rootfs/etc/s6-overlay/s6-rc.d/magicdns-egress-proxy/finishtailscale/rootfs/etc/s6-overlay/s6-rc.d/magicdns-egress-proxy/runtailscale/rootfs/etc/s6-overlay/s6-rc.d/magicdns-ingress-proxy/runtailscale/rootfs/etc/s6-overlay/s6-rc.d/tailscaled/runtailscale/rootfs/usr/bin/magicdns-egress-proxy-forwardingtailscale/rootfs/usr/bin/magicdns-ingress-proxy-forwarding
lmagyar
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tailscale/rootfs/etc/s6-overlay/s6-rc.d/magicdns-egress-proxy/run`:
- Around line 46-55: The loop that searches for a free egress port skips the
upper bound because it exits when ++dnsmasq_egress_port equals
DNSMASQ_EGRESS_DEFAULT_PORT + 1000, so that boundary port is never tested;
modify the exit check in the while loop to only abort after the candidate has
moved past the upper bound (e.g., change the equality check to a greater-than
comparison), keeping references to dnsmasq_egress_port and
DNSMASQ_EGRESS_DEFAULT_PORT inside the same while loop block so the final
boundary port is actually attempted before failing.
- Around line 78-105: Check MAGICDNS_MODE explicitly and fail fast: replace the
current if bashio::var.equals "${MAGICDNS_MODE}" "RESTRICTED" ... else ... fi
pattern with explicit checks for allowed values (e.g., bashio::var.equals
"${MAGICDNS_MODE}" "RESTRICTED" and bashio::var.equals "${MAGICDNS_MODE}"
"UNRESTRICTED"); keep the existing logic that reads
DNSMASQ_BLACK_WHITE_LIST_LOCATION into black_list/white_list (readarray -t) and
populates options+=(...) for servers or NXDOMAIN, but add a final else branch
that logs a clear error including the invalid MAGICDNS_MODE value via
bashio::log.error and terminates with a non-zero exit (e.g., exit 1) to avoid
silently falling back to the wrong behavior.
In `@tailscale/rootfs/etc/s6-overlay/s6-rc.d/magicdns-proxies-reconfigurator/run`:
- Around line 25-34: The probe failure path for configure-magicdns-proxies
currently leaves magicdns_proxies_configuration unset/invalid so the later
change-detection branch can wrongly treat it as "changed" and trigger restarts;
modify the logic in the run loop (inspect the configure-magicdns-proxies call
and the magicdns_proxies_configuration variable usage) so that when
configure-magicdns-proxies test fails you either explicitly set
magicdns_proxies_configuration='unchanged' or skip the restart branch entirely
(only enter the "Restart dnsmasq proxies" block when the probe succeeded and
magicdns_proxies_configuration is a valid non-empty value and not 'unchanged').
Ensure health-state handling with MAGICDNS_PROXIES_RECONFIGURATOR_HEALTH_STATE
remains unchanged.
🪄 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: 802ab648-2ef7-4c57-b53d-3d12fadab999
📒 Files selected for processing (31)
tailscale/rootfs/etc/s6-overlay/s6-rc.d/init-magicdns-ingress-proxy/downtailscale/rootfs/etc/s6-overlay/s6-rc.d/init-magicdns-ingress-proxy/uptailscale/rootfs/etc/s6-overlay/s6-rc.d/init-magicdns-proxies-upstream-list/runtailscale/rootfs/etc/s6-overlay/s6-rc.d/init-magicdns-proxies-upstream-list/uptailscale/rootfs/etc/s6-overlay/s6-rc.d/init-magicdns-proxies/dependencies.d/basetailscale/rootfs/etc/s6-overlay/s6-rc.d/init-magicdns-proxies/downtailscale/rootfs/etc/s6-overlay/s6-rc.d/init-magicdns-proxies/finishtailscale/rootfs/etc/s6-overlay/s6-rc.d/init-magicdns-proxies/runtailscale/rootfs/etc/s6-overlay/s6-rc.d/init-magicdns-proxies/typetailscale/rootfs/etc/s6-overlay/s6-rc.d/init-magicdns-proxies/uptailscale/rootfs/etc/s6-overlay/s6-rc.d/magicdns-egress-proxy/dependencies.d/init-magicdns-proxiestailscale/rootfs/etc/s6-overlay/s6-rc.d/magicdns-egress-proxy/runtailscale/rootfs/etc/s6-overlay/s6-rc.d/magicdns-ingress-proxy/dependencies.d/init-magicdns-proxies-upstream-listtailscale/rootfs/etc/s6-overlay/s6-rc.d/magicdns-ingress-proxy/dependencies.d/magicdns-proxies-configuratortailscale/rootfs/etc/s6-overlay/s6-rc.d/magicdns-ingress-proxy/dependencies.d/post-tailscaledtailscale/rootfs/etc/s6-overlay/s6-rc.d/magicdns-ingress-proxy/finishtailscale/rootfs/etc/s6-overlay/s6-rc.d/magicdns-ingress-proxy/runtailscale/rootfs/etc/s6-overlay/s6-rc.d/magicdns-proxies-configurator/dependencies.d/post-tailscaledtailscale/rootfs/etc/s6-overlay/s6-rc.d/magicdns-proxies-configurator/runtailscale/rootfs/etc/s6-overlay/s6-rc.d/magicdns-proxies-configurator/typetailscale/rootfs/etc/s6-overlay/s6-rc.d/magicdns-proxies-configurator/uptailscale/rootfs/etc/s6-overlay/s6-rc.d/magicdns-proxies-reconfigurator/dependencies.d/magicdns-ingress-proxytailscale/rootfs/etc/s6-overlay/s6-rc.d/magicdns-proxies-reconfigurator/finishtailscale/rootfs/etc/s6-overlay/s6-rc.d/magicdns-proxies-reconfigurator/runtailscale/rootfs/etc/s6-overlay/s6-rc.d/magicdns-proxies-reconfigurator/typetailscale/rootfs/etc/s6-overlay/s6-rc.d/tailscaled/dependencies.d/init-magicdns-ingress-proxytailscale/rootfs/etc/s6-overlay/s6-rc.d/tailscaled/runtailscale/rootfs/etc/s6-overlay/s6-rc.d/user/contents.d/magicdns-proxies-reconfiguratortailscale/rootfs/etc/s6-overlay/scripts/stage2_hook.shtailscale/rootfs/usr/bin/configure-magicdns-proxiestailscale/rootfs/usr/bin/healthcheck
💤 Files with no reviewable changes (4)
- tailscale/rootfs/etc/s6-overlay/s6-rc.d/init-magicdns-ingress-proxy/down
- tailscale/rootfs/etc/s6-overlay/s6-rc.d/init-magicdns-proxies-upstream-list/up
- tailscale/rootfs/etc/s6-overlay/s6-rc.d/init-magicdns-ingress-proxy/up
- tailscale/rootfs/etc/s6-overlay/s6-rc.d/init-magicdns-proxies-upstream-list/run
✅ Files skipped from review due to trivial changes (4)
- tailscale/rootfs/etc/s6-overlay/s6-rc.d/magicdns-proxies-configurator/up
- tailscale/rootfs/etc/s6-overlay/s6-rc.d/magicdns-proxies-reconfigurator/type
- tailscale/rootfs/etc/s6-overlay/s6-rc.d/init-magicdns-proxies/down
- tailscale/rootfs/etc/s6-overlay/s6-rc.d/init-magicdns-proxies/up
🚧 Files skipped from review as they are similar to previous changes (2)
- tailscale/rootfs/etc/s6-overlay/s6-rc.d/tailscaled/run
- tailscale/rootfs/etc/s6-overlay/s6-rc.d/magicdns-ingress-proxy/run
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tailscale/rootfs/etc/s6-overlay/s6-rc.d/magicdns-proxies-reconfigurator/run (1)
37-50: 🏗️ Heavy liftNo error handling in the reconfiguration block leaves the system in an inconsistent state on partial failure.
Every command in the restart sequence (lines 38–50) runs unconditionally with no exit-code checks and no rollback:
- If
magicdns-ingress-proxy-forwarding setup dropfails, the drop rule is absent but forwarding is subsequently removed (line 39) — there is a brief window where queries pass through unconstrained.- If
s6-svc -ruwR /run/service/magicdns-ingress-proxywere to return before the ingress proxy writes the port file (see the companion issue above), the forwarding setup call silently misfires.- If
magicdns-ingress-proxy-forwarding remove drop(line 48) fails after forwarding setup (line 47) also failed, the drop rule lingers but forwarding is absent — DNS is permanently blocked.Because none of these failures update
MAGICDNS_PROXIES_RECONFIGURATOR_HEALTH_STATEtoUNHEALTHY, the healthcheck (context snippet 4) continues to updateLAST_ONLINE_TIMESTAMPeven while DNS is broken.Consider wrapping the reconfiguration sequence in a helper function that tracks success end-to-end and sets
UNHEALTHYif any critical step fails, keeping the SUPPRESS marker in place so that a subsequent loop iteration (after the next config change) can re-attempt cleanly:function reconfigure_proxies() { ... magicdns-ingress-proxy-forwarding setup drop || return 1 magicdns-ingress-proxy-forwarding remove forwarding || return 1 s6-svc -ruwR /run/service/magicdns-egress-proxy || return 1 s6-svc -ruwR /run/service/magicdns-ingress-proxy || return 1 [[ -s "${DNSMASQ_INGRESS_PORT_LOCATION}" ]] || return 1 magicdns-ingress-proxy-forwarding setup forwarding "$(<"${DNSMASQ_INGRESS_PORT_LOCATION}")" || return 1 magicdns-ingress-proxy-forwarding remove drop || return 1 rm -r -f "${MAGICDNS_INGRESS_PROXY_SUPPRESS_FORWARDING_CONFIGURATION_LOCATION}" rm -r -f "${DNSMASQ_INGRESS_PORT_LOCATION}" } if ! reconfigure_proxies; then printf "UNHEALTHY" > /var/run/s6/container_environment/MAGICDNS_PROXIES_RECONFIGURATOR_HEALTH_STATE healthy=false fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tailscale/rootfs/etc/s6-overlay/s6-rc.d/magicdns-proxies-reconfigurator/run` around lines 37 - 50, Wrap the reconfiguration sequence into a helper function (e.g., reconfigure_proxies) that runs each step with exit-code checks and returns non-zero on any failure: run magicdns-ingress-proxy-forwarding setup drop, magicdns-ingress-proxy-forwarding remove forwarding, s6-svc -ruwR for both /run/service/magicdns-egress-proxy and /run/service/magicdns-ingress-proxy, verify DNSMASQ_INGRESS_PORT_LOCATION is present and non-empty before calling magicdns-ingress-proxy-forwarding setup forwarding with its value, then call magicdns-ingress-proxy-forwarding remove drop and only on full success remove the SUPPRESS file and DNSMASQ_INGRESS_PORT_LOCATION; if the helper fails, write "UNHEALTHY" to MAGICDNS_PROXIES_RECONFIGURATOR_HEALTH_STATE and avoid removing the suppress marker so the next loop can retry, ensuring each command (magicdns-ingress-proxy-forwarding, s6-svc) is checked and failure short-circuits the rest.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tailscale/rootfs/etc/s6-overlay/s6-rc.d/magicdns-proxies-reconfigurator/run`:
- Around line 46-50: The restore sequence must guard against a missing/empty
DNSMASQ_INGRESS_PORT_LOCATION and must not remove the drop rule or cleanup the
SUPPRESS marker if forwarding setup cannot run; change the block around
magicdns-ingress-proxy-forwarding so it first tests that
DNSMASQ_INGRESS_PORT_LOCATION exists and is non-empty (e.g. [[ -s
"${DNSMASQ_INGRESS_PORT_LOCATION}" ]]) and only then calls
magicdns-ingress-proxy-forwarding setup forwarding
"$(<"${DNSMASQ_INGRESS_PORT_LOCATION}")"; if the file is missing/empty or the
setup command fails, log an error via bashio::log.error and exit non-zero (or
return) so the subsequent magicdns-ingress-proxy-forwarding remove drop and rm
of
MAGICDNS_INGRESS_PROXY_SUPPRESS_FORWARDING_CONFIGURATION_LOCATION/DNSMASQ_INGRESS_PORT_LOCATION
are not executed; ensure the magicdns-ingress-proxy-forwarding failure is
propagated (do not swallow its exit code).
---
Nitpick comments:
In `@tailscale/rootfs/etc/s6-overlay/s6-rc.d/magicdns-proxies-reconfigurator/run`:
- Around line 37-50: Wrap the reconfiguration sequence into a helper function
(e.g., reconfigure_proxies) that runs each step with exit-code checks and
returns non-zero on any failure: run magicdns-ingress-proxy-forwarding setup
drop, magicdns-ingress-proxy-forwarding remove forwarding, s6-svc -ruwR for both
/run/service/magicdns-egress-proxy and /run/service/magicdns-ingress-proxy,
verify DNSMASQ_INGRESS_PORT_LOCATION is present and non-empty before calling
magicdns-ingress-proxy-forwarding setup forwarding with its value, then call
magicdns-ingress-proxy-forwarding remove drop and only on full success remove
the SUPPRESS file and DNSMASQ_INGRESS_PORT_LOCATION; if the helper fails, write
"UNHEALTHY" to MAGICDNS_PROXIES_RECONFIGURATOR_HEALTH_STATE and avoid removing
the suppress marker so the next loop can retry, ensuring each command
(magicdns-ingress-proxy-forwarding, s6-svc) is checked and failure
short-circuits the rest.
🪄 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: 6dba5b25-d941-431b-912a-54ad8b3b4d42
📒 Files selected for processing (3)
tailscale/rootfs/etc/s6-overlay/s6-rc.d/magicdns-egress-proxy/runtailscale/rootfs/etc/s6-overlay/s6-rc.d/magicdns-ingress-proxy/runtailscale/rootfs/etc/s6-overlay/s6-rc.d/magicdns-proxies-reconfigurator/run
✅ Files skipped from review due to trivial changes (1)
- tailscale/rootfs/etc/s6-overlay/s6-rc.d/magicdns-egress-proxy/run
🚧 Files skipped from review as they are similar to previous changes (1)
- tailscale/rootfs/etc/s6-overlay/s6-rc.d/magicdns-ingress-proxy/run
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Collaborator
Author
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tailscale/rootfs/etc/s6-overlay/s6-rc.d/magicdns-proxies-reconfigurator/run`:
- Around line 74-99: The current logic updates previous_ipn_dns_config before
attempting the probe, which suppresses retries on transient failures; change it
so previous_ipn_dns_config is only set after a successful probe by moving the
assignment into the successful-branch (i.e., inside the else of the
configure-magicdns-proxies test where magicdns_proxies_configuration is
available), keep the existing health-state updates and restart_proxies handling
as-is, and ensure you still compare current_ipn_dns_config vs
previous_ipn_dns_config at the top to gate the probe; referenced symbols:
previous_ipn_dns_config, current_ipn_dns_config, configure-magicdns-proxies,
magicdns_proxies_configuration, restart_proxies.
🪄 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: e5625df6-c623-4eb8-ae41-8542a25aa86d
📒 Files selected for processing (1)
tailscale/rootfs/etc/s6-overlay/s6-rc.d/magicdns-proxies-reconfigurator/run
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note: 2 users reported that it works.
Proposed Changes
The whole internal structure got much clearer and logical:
Related Issues
fixes #661 #666
Summary by CodeRabbit
New Features
Bug Fixes
Chores