Add network interface settings for mDNS/LLMNR #5520
Conversation
|
Warning Rate limit exceeded@agners has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 27 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 (2)
📝 WalkthroughWalkthroughThe pull request introduces comprehensive support for Multicast DNS (mDNS) and Link-Local Multicast Name Resolution (LLMNR) configurations across multiple files in the supervisor network management system. The changes add new attributes, constants, and methods to enable configuration and retrieval of mDNS and LLMNR settings for network interfaces. This enhancement allows more granular control over local network service discovery by providing options to turn these services off, enable resolution, or enable announcement. Changes
Sequence DiagramsequenceDiagram
participant API as Network API
participant Configuration as Host Configuration
participant DBus as DBus Network Setting
API->>Configuration: Request Interface Details
Configuration-->>API: Return Interface with mDNS/LLMNR Settings
API->>DBus: Update Network Interface
DBus->>DBus: Map mDNS/LLMNR Settings
DBus-->>API: Confirm Configuration Update
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
supervisor/host/const.py (1)
75-80: Enhance enum documentation with mode descriptions.The enum documentation could be more helpful by describing what each mode does:
class MulticastDnsMode(StrEnum): - """Multicast DNS (MDNS/LLMNR) mode.""" + """Multicast DNS (MDNS/LLMNR) mode. + + OFF: Disable mDNS/LLMNR completely + RESOLVE: Enable only name resolution + ANNOUNCE: Enable both name resolution and service announcements + """supervisor/dbus/network/setting/generate.py (1)
137-144: Add error handling for invalid modes.The mapping function should handle invalid mode values gracefully.
def _map_mdns_setting(mode: MulticastDnsMode | None) -> int: + """Map MulticastDnsMode to integer value. + + Args: + mode: The MulticastDnsMode to map, or None for default + Returns: + Integer value representing the mode + Raises: + ValueError: If an invalid mode is provided + """ mapping = { MulticastDnsMode.OFF: MulticastDnsValue.OFF, MulticastDnsMode.RESOLVE: MulticastDnsValue.RESOLVE, MulticastDnsMode.ANNOUNCE: MulticastDnsValue.ANNOUNCE, } + if mode is not None and mode not in mapping: + raise ValueError(f"Invalid MulticastDnsMode: {mode}") return int(mapping[mode] if mode else MulticastDnsValue.ANNOUNCE)supervisor/host/configuration.py (1)
280-288: Add error handling and documentation to _map_nm_multicast_dns.The method needs error handling for invalid modes and proper documentation.
@staticmethod def _map_nm_multicast_dns(mode: int | None) -> MulticastDnsMode | None: + """Map integer mode to MulticastDnsMode. + + Args: + mode: Integer value representing the mode, or None + Returns: + Corresponding MulticastDnsMode value, or None if mode is None + Raises: + ValueError: If an invalid mode value is provided + """ + if mode is None: + return None + mapping = { MulticastDnsValue.OFF: MulticastDnsMode.OFF, MulticastDnsValue.RESOLVE: MulticastDnsMode.RESOLVE, MulticastDnsValue.ANNOUNCE: MulticastDnsMode.ANNOUNCE, } + if mode not in mapping: + raise ValueError(f"Invalid multicast DNS mode value: {mode}") return mapping[mode]supervisor/const.py (1)
Line range hint
1-1: Consider adding documentation for the new mDNS/LLMNR settings.To improve maintainability and user experience, consider:
- Adding docstrings to explain the purpose and usage of the new mDNS/LLMNR settings.
- Updating the API documentation to describe the new configuration options.
- Including examples of common configuration scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
supervisor/api/network.py(5 hunks)supervisor/const.py(2 hunks)supervisor/dbus/const.py(1 hunks)supervisor/dbus/network/configuration.py(1 hunks)supervisor/dbus/network/setting/__init__.py(1 hunks)supervisor/dbus/network/setting/generate.py(3 hunks)supervisor/host/configuration.py(5 hunks)supervisor/host/const.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
supervisor/const.py (6)
Pattern */**(html|markdown|md): - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
Pattern */**(html|markdown|md): - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.
Pattern */**(html|markdown|md): - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
Pattern */**(html|markdown|md): - Use sentence-style capitalization also in headings.
Pattern */**(html|markdown|md): do not comment on HTML used for icons
Pattern */**(html|markdown|md): Avoid flagging inline HTML for embedding videos in future reviews for this repository.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Run tests Python 3.12.8
- GitHub Check: Build armv7 supervisor
- GitHub Check: Build armhf supervisor
- GitHub Check: Build aarch64 supervisor
🔇 Additional comments (5)
supervisor/dbus/network/setting/__init__.py (1)
222-223: LGTM! The new mDNS and LLMNR attributes are correctly integrated.The implementation follows the existing pattern and properly extracts the attributes from the connection data.
supervisor/api/network.py (2)
96-97: LGTM! Schema validation is correctly implemented.The schema properly uses
MulticastDnsModefor type coercion of both mDNS and LLMNR fields.
240-243: LGTM! Update logic correctly handles the new attributes.The implementation properly updates the interface with the new mDNS and LLMNR settings.
supervisor/dbus/const.py (1)
299-305: LGTM! The MulticastDnsValue enum is well-defined.The enum values are properly ordered from least to most permissive (OFF → RESOLVE → ANNOUNCE) and follow Python's enum best practices.
supervisor/const.py (1)
231-231: LGTM! The new attributes follow the naming convention.The attributes are correctly placed in alphabetical order and follow the existing naming pattern.
Also applies to: 241-241
| if inet.settings and inet.settings.connection: | ||
| mdns = inet.settings.connection.mdns | ||
| lldmp = inet.settings.connection.lldmp | ||
| else: | ||
| mdns = None | ||
| lldmp = None |
There was a problem hiding this comment.
Fix typo in variable name: 'lldmp' should be 'llmnr'.
The variable name 'lldmp' should be 'llmnr' for consistency.
- lldmp = inet.settings.connection.lldmp
+ llmnr = inet.settings.connection.llmnr
- lldmp = None
+ llmnr = NoneCommittable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
The bot is right, this should be corrected as well.
There was a problem hiding this comment.
@sairon Yes, I found that the typo appears in multiple places. The correction should be made in:
-
supervisor/host/configuration.py:- Change
lldmp = inet.settings.connection.lldmptollmnr = inet.settings.connection.llmnr - Change
lldmp = Nonetollmnr = None - Change
Interface._map_nm_multicast_dns(lldmp)toInterface._map_nm_multicast_dns(llmnr)
- Change
-
supervisor/dbus/const.py:- Update the docstring from
"""Connection MulticastDNS (mdns/lldmp) values."""to"""Connection MulticastDNS (mdns/llmnr) values."""
- Update the docstring from
This will ensure consistency throughout the codebase by using the correct terminology (LLMNR - Link-Local Multicast Name Resolution) everywhere.
🔗 Analysis chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all occurrences of 'lldmp' in the codebase
rg "lldmp" -l
Length of output: 71
Script:
#!/bin/bash
echo "=== supervisor/host/configuration.py ==="
rg "lldmp" -A 2 supervisor/host/configuration.py
echo -e "\n=== supervisor/dbus/const.py ==="
rg "lldmp" -A 2 supervisor/dbus/const.py
Length of output: 536
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Proposed change
Currently there is an issue with mDNS announcement enabled by default on all interfaces.
See #5435 and home-assistant/operating-system#1163
This PR is a second take to fix this issue.
First attempt #5455 was using heuristic logic to enable mDNS annoucement only for primary interface.
After some discusison with @agners we decided that's not an unacceptable solution, because it would break mDNS for people without installed mDNS reflector on router.
This PR take a different route to solve this issue:
announce.Type of change
Additional information
Checklist
ruff format supervisor tests)If API endpoints or add-on configuration are added/changed:
Summary by CodeRabbit
Release Notes
New Features
Improvements