feat(matter_server): allow configuring custom primary network interface#4623
feat(matter_server): allow configuring custom primary network interface#4623txptr wants to merge 4 commits into
Conversation
![home-assistant[bot]](https://avatars.githubusercontent.com/in/97978?s=60&v=4){kind=small}
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
home-assistant
Bot
added
cla-recheck
cla-signed
and removed
cla-recheck
cla-needed
labels
|
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 (1)
📝 WalkthroughWalkthroughThis PR adds an optional add-on config ChangesPrimary Network Interface Configuration
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (1)
matter_server/translations/en.yaml (1)
68-70:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winThe description documents non-functional behavior.
The translation promises that users can "manually specify" the primary network interface, but the runtime script does not read or use the
matter_server_primary_interfaceconfiguration value (see issue raised inconfig.yaml). Until the runtime implementation is added, this description misleads users about the option's functionality.🤖 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 `@matter_server/translations/en.yaml` around lines 68 - 70, The translation for matter_server_primary_interface claims a manual override but the runtime does not read or use that config key; update the translation text to avoid promising functionality that doesn't exist (e.g., remove "Manually specify" and indicate it's reserved/unused) OR implement the runtime support that reads matter_server_primary_interface and applies it when selecting the primary interface; locate the setting by the symbol matter_server_primary_interface and either change its description in en.yaml to reflect current behavior or add code in the runtime (configuration parsing and interface selection) to honor that key.
🤖 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.
Duplicate comments:
In `@matter_server/translations/en.yaml`:
- Around line 68-70: The translation for matter_server_primary_interface claims
a manual override but the runtime does not read or use that config key; update
the translation text to avoid promising functionality that doesn't exist (e.g.,
remove "Manually specify" and indicate it's reserved/unused) OR implement the
runtime support that reads matter_server_primary_interface and applies it when
selecting the primary interface; locate the setting by the symbol
matter_server_primary_interface and either change its description in en.yaml to
reflect current behavior or add code in the runtime (configuration parsing and
interface selection) to honor that key.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 33f15d78-4aaf-4118-9146-da232d3b3035
📒 Files selected for processing (2)
matter_server/config.yamlmatter_server/translations/en.yaml
There was a problem hiding this comment.
🧹 Nitpick comments (1)
matter_server/rootfs/etc/s6-overlay/s6-rc.d/matter-server/run (1)
144-151: ⚡ Quick winConsider quoting variables instead of disabling shellcheck warnings.
Lines 145 and 151 use unquoted
${primary_interface}with shellcheck SC2086 explicitly disabled. While the practical risk is low (network interface names don't contain spaces or glob characters), following bash best practices by quoting variables improves code maintainability and prevents potential future issues.♻️ Proposed fix: Quote the variables
- # shellcheck disable=SC2086 - if [ -z ${primary_interface} ]; then + if [ -z "${primary_interface}" ]; then bashio::log.warning 'Trying fallback method to determine primary interface' primary_interface="$(ip --json route show default | jq --raw-output '.[0].dev')" fi - # shellcheck disable=SC2086 - if [ -z ${primary_interface} ] || [ ${primary_interface} == "null" ]; then + if [ -z "${primary_interface}" ] || [ "${primary_interface}" == "null" ]; then bashio::exit.nok "No primary network interface found!" 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 `@matter_server/rootfs/etc/s6-overlay/s6-rc.d/matter-server/run` around lines 144 - 151, The checks and tests around the primary_interface variable disable SC2086 instead of quoting; update the run script to quote variable expansions for primary_interface (e.g., replace unquoted uses in the if tests that currently read [ -z ${primary_interface} ] and [ ${primary_interface} == "null" ] with quoted expansions like [ -z "${primary_interface}" ] and [ "${primary_interface}" = "null" ] or the bash equivalent), and ensure any other references in this block use "${primary_interface}" so ShellCheck SC2086 can be removed safely; keep the same logic (fallback ip/jq assignment to primary_interface and subsequent null check) but use quoted comparisons.
🤖 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 `@matter_server/rootfs/etc/s6-overlay/s6-rc.d/matter-server/run`:
- Around line 144-151: The checks and tests around the primary_interface
variable disable SC2086 instead of quoting; update the run script to quote
variable expansions for primary_interface (e.g., replace unquoted uses in the if
tests that currently read [ -z ${primary_interface} ] and [ ${primary_interface}
== "null" ] with quoted expansions like [ -z "${primary_interface}" ] and [
"${primary_interface}" = "null" ] or the bash equivalent), and ensure any other
references in this block use "${primary_interface}" so ShellCheck SC2086 can be
removed safely; keep the same logic (fallback ip/jq assignment to
primary_interface and subsequent null check) but use quoted comparisons.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e4f0def9-64ad-437d-adbc-209cf3b97f71
📒 Files selected for processing (1)
matter_server/rootfs/etc/s6-overlay/s6-rc.d/matter-server/run
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 `@matter_server/rootfs/etc/s6-overlay/s6-rc.d/matter-server/run`:
- Around line 144-149: The fallback detection is only run when primary_interface
is empty, so if Supervisor sets primary_interface to the literal string "null"
the fallback is skipped and the later abort triggers; change the first guard so
it treats "null" the same as empty (i.e., update the if that checks
primary_interface before running the fallback to check for -z OR equality to
"null"), leaving the later abort check intact and keeping the fallback
assignment (primary_interface="$(ip --json route show default | jq --raw-output
'.[0].dev')") in the same block.
🪄 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: 5728d18e-4a8f-4a35-8bbd-61dd49873fc2
📒 Files selected for processing (1)
matter_server/rootfs/etc/s6-overlay/s6-rc.d/matter-server/run
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
At least for the Python Matter Server this is actually not true, see also #4070 (comment). From my perspective this flag does not make sense for this and the other reasons I brought forward in that PR. As for the matter.js server I am not sure. But if we add this flag, I'd definitely do it imminent promotion of matter.js to default (#4642). |
|
Yeah matter.js works a bit different here. For us this is basically the main "MDNS listen and send interface". so yes it is basically also used to handle link local scoping but mainly because we should only get announces from that interface, so yes here it makes more sense. @txptr we plan the relewae of the server 9.0 for next week and I would like to plan this in for after this. So it would be great if you could then next week after merging the 9.0 PR then add changelog for a 9.0.1 |
|
@Apollon77 thank you. I will definitely do this. |
|
@txptr what I would prefer is if we'd have a generic network interface isolation based on macvlan or ipvlan for apps which run on host network. This idea exists since a while, see home-assistant/architecture#1034. I created a prototype today, and at least in my testing it worked fine with Matter over Thread devices (see home-assistant/architecture#1034 (comment)). Would this suite your use case as well? |
|
Thank you all for taking time for my pr and your insightful feedback. I completely agree that the proposed larger overhaul - introducing native macvlan or isolated network support directly into the Supervisor - is conceptually the most elegant and robust long-term solution. It would fundamentally solve mDNS, multicast, and routing issues for complex addons across the entire ecosystem. However, since implementing such a core architectural change in the Supervisor will understandably take considerable time to specify, develop, and thoroughly test, I would love to propose my current PR as a pragmatic, low-risk bridge solution (quick win). My patch:
Once the grand network reform for the Supervisor is ready and rolled out, the matter_server_primary_interface configuration flag can easily be deprecated and removed. I am happy to adjust my PR based on whichever direction the core team decides to prioritize, but I believe delivering this relief to users today would be highly beneficial while we work towards the ideal architecture. You decide :-) |
3 participants
Description
This PR introduces an optional configuration option
matter_server_primary_interfaceto the Matter Server add-on. This allows users to manually specify the primary network interface used by the Matter Server (e.g.,enp0s19), rather than relying solely on the Supervisor's automatic detection.Motivation & Context
In complex network environments (such as setups with multiple network interfaces, untagged/tagged VLANs, or multiple gateways), the automatic network interface detection by the Home Assistant Supervisor can sometimes select the wrong interface for the Matter Server.
Currently, trying to append
--primary-interfaceviamatter_server_argsresults in duplicate arguments where the automatically detected one takes precedence, making it impossible for users to override it. This PR solves the issue by offering a dedicated, clean UI configuration field.How Has This Been Tested?
ip link show) detects the missing interface, logs a proper warning/error, and safely falls back to the system default instead of crashing the add-on loop.Checklist
Summary by CodeRabbit
New Features
Bug Fixes / Behavior