Fix MagicDNS incompatibility with Home Assistant's DNS #455
Conversation
|
Warning Rate limit exceeded@lmagyar has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 29 minutes and 33 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request introduces comprehensive changes to the Tailscale Home Assistant Community Add-on, focusing on DNS configuration and documentation improvements. The modifications include updating the documentation with clearer explanations of DNS settings, adding a new AppArmor profile, installing dnsmasq, and enhancing the S6 overlay scripts to manage DNS-related services more effectively. The changes aim to provide better control and understanding of DNS configuration within the Tailscale add-on. Changes
Sequence DiagramsequenceDiagram
participant User
participant Addon
participant Tailscale
participant DNSService
User->>Addon: Configure DNS settings
Addon->>Tailscale: Apply DNS configuration
Tailscale->>DNSService: Initialize DNS handling
DNSService-->>Tailscale: Confirm DNS settings
Tailscale-->>Addon: DNS configuration complete
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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.
|
1 similar comment
✅ 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: 0
🧹 Nitpick comments (1)
tailscale/DOCS.md (1)
438-469: Fix minor formatting and grammar issues in advanced DNS configuration.The advanced DNS configuration section has some minor issues:
Apply these corrections:
- Line 444: Remove comma before "because"
- Line 450: Remove duplicate "the"
- Line 458: Change "eg." to "e.g."
- Line 465: Remove comma before "because"
🧰 Tools
🪛 LanguageTool
[uncategorized] ~438-~438: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...sing Tailscale DNS to resolve everything and you have your own DNS (like AdGuard) _o...(COMMA_COMPOUND_SENTENCE)
[formatting] ~444-~444: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...s global nameserver on the admin console, because you want to redirect all DNS queries wi...(COMMA_BEFORE_BECAUSE)
[duplication] ~450-~450: Possible typo: you repeated a word.
Context: ...f your tailnet that are configured on the the admin console above. This will preven...(ENGLISH_WORD_REPEAT_RULE)
[uncategorized] ~455-~455: The abbreviation “e.g.” (= for example) requires two periods.
Context: ...gure your DNS as the only DNS server (eg. IPv4: 127.0.0.1, IPv6: ::1). - In your...(E_G)
[uncategorized] ~457-~457: Possible missing comma found.
Context: ...IPv4: 127.0.0.1, IPv6: ::1). - In your DNS configure Tailscale DNS for your tailne...(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~458-~458: The abbreviation “e.g.” (= for example) requires two periods.
Context: ...ailnet domain as upstream DNS server (eg. in case of AdGuard `[/tail1234.ts.net/]...(E_G)
[uncategorized] ~461-~461: The abbreviation “e.g.” (= for example) requires two periods.
Context: ... can configure your normal DNS servers (eg. 192.168.1.1 or 1.1.1.1) at the second o...(E_G)
[formatting] ~465-~465: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...n Home Assistant's network configuration, because whenaccept_dnsoption is disabled, T...(COMMA_BEFORE_BECAUSE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
tailscale/DOCS.md(4 hunks)tailscale/Dockerfile(1 hunks)tailscale/apparmor.txt(1 hunks)tailscale/config.yaml(1 hunks)tailscale/rootfs/etc/s6-overlay/s6-rc.d/dnsmasq/finish(1 hunks)tailscale/rootfs/etc/s6-overlay/s6-rc.d/dnsmasq/run(1 hunks)tailscale/rootfs/etc/s6-overlay/s6-rc.d/dnsmasq/type(1 hunks)tailscale/rootfs/etc/s6-overlay/s6-rc.d/tailscaled/run(2 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 (1)
- tailscale/rootfs/etc/s6-overlay/s6-rc.d/dnsmasq/type
🧰 Additional context used
🪛 LanguageTool
tailscale/DOCS.md
[uncategorized] ~91-~91: Possible missing comma found.
Context: ...le_dns] of the admin console. For more information see the "DNS" section of this documenta...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~368-~368: The abbreviation “i.e.” (= that is) requires two periods.
Context: ...n name instead of only the device name, ie. `ping some-tailnet-device.tail1234.ts.n...
(I_E)
[grammar] ~385-~385: It appears that a pronoun is missing.
Context: ... in the add-on, is that Tailscale's DNS when can't resolve a query, instead of retur...
(WHERE_MD_VB)
[uncategorized] ~404-~404: The abbreviation “e.g.” (= for example) requires two periods.
Context: ...0::53). Move your normal DNS servers (eg. 192.168.1.1 or 1.1.1.1) to lower positi...
(E_G)
[uncategorized] ~429-~429: The abbreviation “e.g.” (= for example) requires two periods.
Context: ... can configure your normal DNS servers (eg. 192.168.1.1 or 1.1.1.1) at the second o...
(E_G)
[style] ~434-~434: The adverb ‘also’ is commonly used to connect clauses and isn’t usually used at the end of a phrase or before a conjunction. Consider replacing it with a more formal alternative.
Context: ... DNS will resolve non-tailnet addresses also. Whether you have your own DNS (like Ad...
(ALSO_AS_WELL)
[uncategorized] ~438-~438: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...sing Tailscale DNS to resolve everything and you have your own DNS (like AdGuard) _o...
(COMMA_COMPOUND_SENTENCE)
[formatting] ~444-~444: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...s global nameserver on the admin console, because you want to redirect all DNS queries wi...
(COMMA_BEFORE_BECAUSE)
[duplication] ~450-~450: Possible typo: you repeated a word.
Context: ...f your tailnet that are configured on the the admin console above. This will preven...
(ENGLISH_WORD_REPEAT_RULE)
[uncategorized] ~455-~455: The abbreviation “e.g.” (= for example) requires two periods.
Context: ...gure your DNS as the only DNS server (eg. IPv4: 127.0.0.1, IPv6: ::1). - In your...
(E_G)
[uncategorized] ~457-~457: Possible missing comma found.
Context: ...IPv4: 127.0.0.1, IPv6: ::1). - In your DNS configure Tailscale DNS for your tailne...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~458-~458: The abbreviation “e.g.” (= for example) requires two periods.
Context: ...ailnet domain as upstream DNS server (eg. in case of AdGuard `[/tail1234.ts.net/]...
(E_G)
[uncategorized] ~461-~461: The abbreviation “e.g.” (= for example) requires two periods.
Context: ... can configure your normal DNS servers (eg. 192.168.1.1 or 1.1.1.1) at the second o...
(E_G)
[formatting] ~465-~465: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...n Home Assistant's network configuration, because when accept_dns option is disabled, T...
(COMMA_BEFORE_BECAUSE)
🔇 Additional comments (13)
tailscale/rootfs/etc/s6-overlay/s6-rc.d/dnsmasq/run (2)
8-8: LGTM! Good choice of IP address.The chosen IP address (127.52.52.52) is within the loopback range but uses an uncommon subnet to avoid conflicts with other services.
12-16: LGTM! Well-configured dummy DNS setup.The dnsmasq configuration is secure and properly isolated:
- Binds only to specific interface
- Runs in foreground for proper supervision
- Refuses all queries as intended
- Uses port 53 as required for resolv.conf
tailscale/rootfs/etc/s6-overlay/s6-rc.d/dnsmasq/finish (1)
15-25: LGTM! Robust exit handling implementation.The script properly manages service shutdown with appropriate exit code handling:
- Preserves signal-based exits (code 256)
- Special handling for SIGTERM (signal 15)
- Maintains container exit codes
tailscale/rootfs/etc/s6-overlay/s6-rc.d/tailscaled/run (2)
38-40: LGTM! Good DNS loop prevention strategy.The implementation correctly prevents DNS loops by isolating tailscaled's DNS configuration from the system.
52-62: Verify mount namespace isolation.While the mount namespace isolation is a good approach, ensure that:
- The mount namespace is properly cleaned up on service restart
- The file permissions are preserved during the move operation
tailscale/config.yaml (1)
24-24: Verify the necessity of SYS_ADMIN privilege.While SYS_ADMIN is required for mount namespace operations, it's a powerful capability. Consider if a more limited capability like CAP_SYS_CHROOT would suffice for the mount --bind operation.
✅ Verification successful
SYS_ADMIN capability is required for Tailscale's core functionality
The capability is necessary for network namespace operations and interface management, which are essential for Tailscale's VPN functionality. CAP_SYS_CHROOT would not be sufficient as Tailscale requires full network namespace management capabilities.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the mount operation works with CAP_SYS_CHROOT # Note: This is a theoretical check as the actual test would require runtime capabilities capsh --print | grep cap_sys_adminLength of output: 132
Script:
#!/bin/bash # Search for mount-related operations and SYS_ADMIN references echo "=== Searching for mount operations ===" rg -i "mount" --type yaml --type go -C 2 echo -e "\n=== Searching for SYS_ADMIN references ===" rg -i "sys_admin" --type yaml --type go -C 2 echo -e "\n=== Checking README and docs ===" fd "README|\.md$" -t f -X catLength of output: 30735
tailscale/apparmor.txt (2)
3-9: LGTM! AppArmor profile correctly configured with necessary flags and base abstractions.The profile is properly set up with:
attach_disconnectedflag for handling disconnected pathsmediate_deletedflag for mediating deleted files- Required signal capabilities for process management
43-47: Verify mount capabilities for DNS fix.The mount capabilities and rules look correct for the MagicDNS fix:
sys_admincapability for mount operations- Root filesystem mount with
rprivatefor proper isolation- Bind mount for tailscaled resolver configuration
tailscale/rootfs/etc/s6-overlay/scripts/stage2_hook.sh (1)
8-15: LGTM! Correct service management logic for dnsmasq.The conditional block properly handles dnsmasq service based on configuration:
- Disables when userspace networking is enabled
- Disables when DNS acceptance is disabled
- Correctly removes both service and dependency files
tailscale/Dockerfile (1)
13-13: Verify dnsmasq package version for security vulnerabilities.The dnsmasq package version 2.90-r3 is added as a dependency.
tailscale/translations/en.yaml (1)
6-7: LGTM! Clear and accurate description of accept_dns option.The updated description correctly explains:
- Purpose of accepting DNS settings from tailnet
- Configuration location in admin console
tailscale/DOCS.md (2)
348-394: LGTM! Comprehensive explanation of DNS functionality.Excellent documentation of:
- DNS server functionality at 100.100.100.100
- MagicDNS components and limitations
- DNS resolution behavior
- Loop prevention mechanism
🧰 Tools
🪛 LanguageTool
[uncategorized] ~368-~368: The abbreviation “i.e.” (= that is) requires two periods.
Context: ...n name instead of only the device name, ie. `ping some-tailnet-device.tail1234.ts.n...(I_E)
[grammar] ~385-~385: It appears that a pronoun is missing.
Context: ... in the add-on, is that Tailscale's DNS when can't resolve a query, instead of retur...(WHERE_MD_VB)
397-415: LGTM! Clear instructions for basic DNS configuration.Well-documented scenario for resolving only tailnet devices:
- Correct DNS server configuration
- Proper order of DNS servers
- Fallback behavior
🧰 Tools
🪛 LanguageTool
[uncategorized] ~404-~404: The abbreviation “e.g.” (= for example) requires two periods.
Context: ...0::53). Move your normal DNS servers (eg. 192.168.1.1 or 1.1.1.1) to lower positi...(E_G)
Proposed Changes
This was a permanent returning issue.
Problem:
Not solutions:
Solution:
dns: resolver: forward: no upstream resolvers set, returning SERVFAILPositive side effects:
Negative side effects:
ping tailnet-device-namedoesn't work, onlyping tailnet-device-name.tailxxxx.ts.net, so without domain, only local.hass.io domain works, but this is the default HA behavior, so I think it is fineRelated Issues
Summary by CodeRabbit
Documentation
accept_dnsoption behavior and its role in DNS settingsNew Features
dnsmasqpackage to improve DNS handlingConfiguration
SYS_ADMINprivilege to expand service capabilitiesBug Fixes