Add IPv6 address generation mode & privacy extensions #5892
Conversation
davidrapan
changed the title
📝 WalkthroughWalkthroughThe changes introduce explicit support for IPv6 address generation mode and privacy settings throughout the network configuration stack. New data structures, constants, and enums are added to represent these IPv6-specific parameters. Serialization, deserialization, validation, and API exposure are updated to handle and propagate the new fields, ensuring consistent management and representation of IPv6 configuration. Changes
Sequence Diagram(s)sequenceDiagram
participant API as Network API
participant Host as Host Configuration
participant DBus as DBus Layer
API->>API: Receive IPv6 config (addr_gen_mode, ip6_privacy)
API->>Host: Create Ip6Setting with new fields
Host->>DBus: Map and forward settings (addr_gen_mode, ip6_privacy)
DBus->>DBus: Store as Ip6Properties
DBus-->>Host: Return updated IPv6 config
Host-->>API: Serialize with addr_gen_mode, ip6_privacy
API-->>Client: Expose IPv6 config with new fields
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (6)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 0
🔭 Outside diff range comments (1)
supervisor/api/network.py (1)
241-248:⚠️ Potential issueConstructor arguments are in the wrong order – will corrupt data at runtime
Ip6Settinginherits the field order fromIpSetting(method, address, gateway, nameservers) and then appends the new fields (addr_gen_mode, ip6_privacy).
By passing the arguments positionally asIp6Setting(method, addr_gen_mode, ip6_privacy, address, gateway, nameservers)each value is shifted one slot to the right:
addr_gen_modeis stored inaddressip6_privacyis stored ingateway- the real address list ends up in
nameservers- …
This will break validation, generate garbage DBus payloads, and likely raise exceptions when the supervisor tries to iterate
ipv6setting.address.Fix by using keyword arguments (safer) or the correct positional order.
- interface.ipv6setting = Ip6Setting( - config.get(ATTR_METHOD, InterfaceMethod.STATIC), - config.get(ATTR_ADDR_GEN_MODE, InterfaceAddrGenMode.STABLE_PRIVACY), - config.get(ATTR_IP6_PRIVACY, InterfaceIp6Privacy.ENABLED), - config.get(ATTR_ADDRESS, []), - config.get(ATTR_GATEWAY), - config.get(ATTR_NAMESERVERS, []), - ) + interface.ipv6setting = Ip6Setting( + method=config.get(ATTR_METHOD, InterfaceMethod.STATIC), + address=config.get(ATTR_ADDRESS, []), + gateway=config.get(ATTR_GATEWAY), + nameservers=config.get(ATTR_NAMESERVERS, []), + addr_gen_mode=config.get(ATTR_ADDR_GEN_MODE, InterfaceAddrGenMode.STABLE_PRIVACY), + ip6_privacy=config.get(ATTR_IP6_PRIVACY, InterfaceIp6Privacy.ENABLED), + )
♻️ Duplicate comments (1)
supervisor/api/network.py (1)
310-319: Same argument-ordering bug when creating VLANsThe positional call to
Ip6Settingin VLAN creation has the identical mis-ordering and needs the same fix as above to avoid corrupting the new interface object.
🧹 Nitpick comments (6)
supervisor/host/const.py (1)
18-30: Consider specialising the doc-string for the new enums
InterfaceAddrGenModeandInterfaceIp6Privacyinherit the generic “Configuration of an interface.” description that is already used above. A short, specific sentence (“IPv6 address generation mode”, “IPv6 privacy extension behaviour”) will make the generated reference documentation clearer.supervisor/api/network.py (3)
72-80: Validation schema looks good – tiny readability nit
The_SCHEMA_IPV6_CONFIGcorrectly adds the two new options and type-coerces to the StrEnums.
If you moveATTR_GATEWAY&ATTR_NAMESERVERSbeneath the new fields you added, the IPv4 / IPv6 schemas stay visually aligned which helps readers spot the diff faster.
115-126: Duplication betweenipconfig_structandip6config_struct
The two functions are almost identical except for two extra fields. A tiny helper (_common_ip_struct(...)) or an optional-param approach could remove the code copy-paste and keep future changes in one place.
102-104: Type hint: acceptNoneforipv6setting
_get_ipv6_connection_settings()is called withinterface.ipv6setting, which can legitimately beNone. Declare the parameter asIp6Setting | None(orOptional[Ip6Setting]) to satisfy type-checkers and future maintainers.Also applies to: 158-160
supervisor/dbus/network/setting/generate.py (2)
103-118: Avoid magic integers – use the IntEnum values directly
Hard-coding0 / 1 / 2forCONF_ATTR_IPV6_ADDR_GEN_MODEandCONF_ATTR_IPV6_PRIVACYworks but obscures intent. The correspondingInterfaceAddrGenModeandInterfaceIp6PrivacyIntEnums already define.value, so you can write:ipv6[CONF_ATTR_IPV6_ADDR_GEN_MODE] = Variant("i", ipv6setting.addr_gen_mode.value) ... ipv6[CONF_ATTR_IPV6_PRIVACY] = Variant("i", ipv6setting.ip6_privacy.value)This keeps the mapping in one place and prevents accidental divergence if enum values ever change.
61-63: Signature mismatch: allowNoneforipv4setting/ipv6setting
Both helper functions operate correctly when the setting object isNone, but their annotations specify a non-optional type. Updating the type hints avoids false positives for static analysis tools.-def _get_ipv4_connection_settings(ipv4setting: IpSetting) -> dict: +def _get_ipv4_connection_settings(ipv4setting: IpSetting | None) -> dict: ... -def _get_ipv6_connection_settings(ipv6setting: Ip6Setting) -> dict: +def _get_ipv6_connection_settings(ipv6setting: Ip6Setting | None) -> dict:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (9)
supervisor/api/network.py(8 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(6 hunks)supervisor/dbus/network/setting/generate.py(4 hunks)supervisor/host/configuration.py(6 hunks)supervisor/host/const.py(1 hunks)tests/api/test_network.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
`*/**(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 t...
*/**(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'
supervisor/const.py
`*/**(html|markdown|md)`: - Use bold to mark UI strings. - If "" are used to mark UI strings, replace them by bold.
*/**(html|markdown|md): - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.
supervisor/const.py
`*/**(html|markdown|md)`: - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
*/**(html|markdown|md): - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
supervisor/const.py
`*/**(html|markdown|md)`: - Use sentence-style capitalization also in headings.
*/**(html|markdown|md): - Use sentence-style capitalization also in headings.
supervisor/const.py
`*/**(html|markdown|md)`: do not comment on HTML used for icons
*/**(html|markdown|md): do not comment on HTML used for icons
supervisor/const.py
`*/**(html|markdown|md)`: Avoid flagging inline HTML for embedding videos in future reviews for this repository.
*/**(html|markdown|md): Avoid flagging inline HTML for embedding videos in future reviews for this repository.
supervisor/const.py
🧬 Code Graph Analysis (4)
supervisor/host/const.py (1)
supervisor/dbus/const.py (2)
InterfaceAddrGenMode(213-217)InterfaceIp6Privacy(220-225)
supervisor/dbus/const.py (1)
supervisor/host/const.py (2)
InterfaceAddrGenMode(18-22)InterfaceIp6Privacy(25-30)
supervisor/api/network.py (3)
supervisor/dbus/const.py (3)
InterfaceAddrGenMode(213-217)InterfaceIp6Privacy(220-225)InterfaceMethod(204-210)supervisor/host/const.py (3)
InterfaceAddrGenMode(18-22)InterfaceIp6Privacy(25-30)InterfaceMethod(10-15)supervisor/host/configuration.py (2)
Ip6Setting(59-63)IpConfig(39-45)
supervisor/host/configuration.py (5)
supervisor/dbus/const.py (3)
InterfaceAddrGenMode(213-217)InterfaceIp6Privacy(220-225)InterfaceMethod(204-210)supervisor/host/const.py (6)
InterfaceAddrGenMode(18-22)InterfaceIp6Privacy(25-30)InterfaceMethod(10-15)AuthMethod(41-46)InterfaceType(33-38)WifiMode(49-55)supervisor/dbus/network/interface.py (4)
connection(87-89)connection(92-97)NetworkInterface(27-187)settings(100-102)supervisor/dbus/network/connection.py (1)
NetworkConnection(27-183)supervisor/host/network.py (1)
get(108-113)
🔇 Additional comments (16)
supervisor/const.py (2)
100-100: LGTM! New attribute constant for IPv6 address generation mode.This constant provides a consistent key name for the new IPv6 address generation mode configuration option and follows the existing naming convention in the file.
224-224: LGTM! New attribute constant for IPv6 privacy settings.This constant provides a consistent key name for the new IPv6 privacy settings configuration option and follows the existing naming convention in the file.
supervisor/dbus/network/configuration.py (1)
80-86: LGTM! Well-designed IPv6 properties extension.Good approach to extend the base
IpPropertiesclass with IPv6-specific fields while maintaining the inheritance hierarchy. This allows for IPv6-specific configuration while reusing the common IP properties.tests/api/test_network.py (1)
54-54: LGTM! Test coverage for new IPv6 configuration options.Appropriate test assertions for the new IPv6 address generation mode and privacy settings, ensuring that the API correctly exposes these new configuration options with their expected default values.
Also applies to: 57-57
supervisor/dbus/const.py (1)
213-226: LGTM! Well-structured enums for the new IPv6 configuration options.The new enum classes for IPv6 address generation mode and privacy settings follow the existing pattern in the codebase and are correctly defined as
IntEnumfor DBus integration. The values align with the corresponding string enums insupervisor/host/const.py, providing consistent representation across different layers of the system.supervisor/dbus/network/setting/__init__.py (5)
15-15: New IPv6 Properties import added correctly.The import of
Ip6Propertiesis appropriate for the new IPv6 configuration capabilities being added.
62-63: IPv6 address generation mode and privacy configuration added.These new configuration attributes align with the PR objectives to support custom IPv6 address generation modes.
75-76: IPv6 configuration attributes properly added to ignore fields.Adding these fields to
IPV4_6_IGNORE_FIELDSensures they'll be correctly handled during settings updates.
119-119: Type annotations updated for IPv6 properties.Correctly updating the type annotations from generic
IpPropertiesto specializedIp6Propertiesimproves type safety.Also applies to: 159-159
286-293: IPv6 properties initialization properly includes new settings.The new address generation mode and privacy settings are correctly extracted from the loaded configuration data.
supervisor/host/configuration.py (6)
10-13: NetworkManager IPv6 enums imported with appropriate aliases.Good practice using aliases to avoid naming conflicts between the NetworkManager constants and internal representation.
17-24: Well-structured import of internal enums.The reorganization to explicitly import all required enums enhances code readability.
58-64: Well-defined IPv6 settings dataclass with appropriate defaults.The new
Ip6Settingdataclass properly extendsIpSettingwith IPv6-specific attributes, and includes sensible defaults that match the PR objectives (EUI64 for address generation and ENABLED for privacy).
99-99: Type annotation updated for IPv6 settings.Correctly specifying the more specialized
Ip6Settingtype improves type safety.
138-160: IPv6 settings mapping implementation is thorough.The implementation correctly maps NetworkManager IPv6 configurations to internal representations, including the new address generation mode and privacy settings.
221-241: Comprehensive mapping functions for IPv6 settings.The new static methods provide clean conversions between NetworkManager constants and internal enum representations, with appropriate defaults for handling unknown values:
STABLE_PRIVACYfor address generation modeENABLEDfor IPv6 privacyThis aligns with the PR objectives that mention these as sensible defaults.
davidrapan
force-pushed
the
dev_ipv6_addr-gen-mode
branch
4 times, most recently
from
02c5bec to
9a0010b
Compare
davidrapan
force-pushed
the
dev_ipv6_addr-gen-mode
branch
from
9a0010b to
07c3652
Compare
How is address selection properly implement/what APIs allow to get the correct IP? Do you know how that plays out with e.g. mDNS specifically? How well is address selection in mDNS resonder and similar implementations which use the IP address implemented? E.g. Home Assistant itself announces services using Python Zeroconf, I wonder if it selects addresses correctly. It uses ifaddr to get a list of interfaces and IP addresses from what I remember, but I don't think it does any address selection. Also, there is a suspicion that systems with privacy extensions which run an OpenThread Border Router and announce the TREL services with an IPv6 address which later gets rotated, leading to TREL peers not correctly addressed. There was also the rather uncommon ordering of GUA over ULA over link-local, but it seems that got recently changed. |
It is in RFC 6724 and is simply enforced by a temporary over public rule in kernel address selection or left to the application to bind() to a specific source address (binding to a public one with privacy extensions enabled would violate the RFC).
Both are announced and the source address is a link-local.
This is correct behavior. In cases like this, the kernel address selection is in charge. I'm not sure about OpenThread Border as I've never used it, but I think it will be a similar case to mDNS. Service-specific announcements have very little to do with source address selection. It's all about where the packets are coming from, not what their content is. :) And that should be at least configurable? I guess. BUT, that's why everything presented here is opt-in for those who know what's going on. HA never initiated any connections outside using anything other than a temporary address. If you want me to do any specific tests, let me know. :) |
davidrapan
force-pushed
the
dev_ipv6_addr-gen-mode
branch
from
07c3652 to
c6f1d64
Compare
I am not really worried what the source address of the multicast packet is, it's largely irrelevant for the receiving system. What is relevant is the payload of mDNS packets, which contains This really deviates a bit from that PR, but I try to understand the ramification of enabled privacy extensions for Home Assistant as a whole, and tap your expertise on this topic 😅
I am worried about the remote system here. From what I understand, there is no way for the remote system to tell which address is which.
Ok, yeah I didn't look at the code just yet, but I was assuming so. Can you clarify that in the PR description as well, so that it will be clear in the future what to expect from this change? Also, as you wrote in the description, not adding this to the frontend make sense, at least for now. |
Of course it is, I just wanted to provide "full picture", just in case. 😉
In the case of mDNS? It doesn't matter.
Sure.
👍 |
|
And I do rely heavily on the mDNS. I'm connecting almost everything within my network through |
Hm, ok I see https://datatracker.ietf.org/doc/html/rfc6762#section-10 says 120 seconds for the host records (including A and AAAA). It seems that non-host records have much higher values by default, it was probably what I had in mind. Python Zeroconf seems to also use the RFC values, so we should be fine.
Ok, yeah with the above it doesn't matter because the TTL is lower than the lifetime of the IP address. As long as the client doesn't store/use the address longer than TTL (which probably should be seen as a bug), we should be fine. Related: Do you happen to know how an open TCP connection behaves in face of this? 🤔 |
|
This temporary address is not regenerated that often (RFC says the default TEMP_PREFERRED_LIFETIME is 1 day). + I've never seen it used as a destination address, so I'd have to do some synthetic testing, but my best guess is a simple dc and in case of source address.. I'm using this configuration on every machine sice forever and never noticed anything unusual in terms of connection stability :) |
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
davidrapan
force-pushed
the
dev_ipv6_addr-gen-mode
branch
2 times, most recently
from
a93298e to
babdd8a
Compare
davidrapan
force-pushed
the
dev_ipv6_addr-gen-mode
branch
from
babdd8a to
87589a1
Compare
davidrapan
force-pushed
the
dev_ipv6_addr-gen-mode
branch
from
485f02d to
48a521d
Compare
|
There are some ruff issues still. Ideally also rebase, I merged two PRs. I think then we are good! |
davidrapan
force-pushed
the
dev_ipv6_addr-gen-mode
branch
from
48a521d to
7a236d0
Compare
|
Oh yeah forgot to check ruff after the changes. 😉 What's up w/ that AwesomeVersion in tests (other than test change)? |
davidrapan
force-pushed
the
dev_ipv6_addr-gen-mode
branch
4 times, most recently
from
162d888 to
7a71f2f
Compare
|
Anyway, builds still fails for some reason since the last bump of crypto. :/ When I revert the bump, everything works. |
|
How do you build Supervisor? |
Signed-off-by: David Rapan <[email protected]>
davidrapan
force-pushed
the
dev_ipv6_addr-gen-mode
branch
from
7a71f2f to
bb94f15
Compare
Ah I see, you meant in CI. That is because the wheels bump gets triggered. I wonder though why that is the case, the necessary wheels should be present 🤔 |
|
Yeah, since I'm not home right now, I thought it might be because of that, but what I found strange is why version has such a significance, since I'm not that familiar with all these quality of life "contraptions" yet. :P |
|
Yeah our infrastructure builds wheels for musllinux automatically. However, that wheels build should only trigger if a PR touches Your PR should not touch those files, so no wheels build should be triggered. 🤔 It seems to work now, so I think it's fine. |
|
Yeah, I think I get it: this evaluates to True and shouldn't. 😉 |
|
I don't understand that AwesomeVersion thing. When it's the class it complains it's not valid... but it doesn't like string either... Edit: Ah, now it's a mocking issue, or even better it's a mocking issue all along. :-D |
This fixes the test by since the extended implementation now can read the version of NetworkManager.
Yes, and we have fixture for that. Just updated the test, should be fine now. |
|
Great, thanks! It's annoying that it revealed itself as a mocking issue only after I used comparison to string. :-P |
|
Added another test to validate |
|
Awesome! 🎉 |
Proposed change
Currently default
stable-privacyis probably sufficient for some users, but for the rest of us, there should be an option to easily change this configuration therefore I'm introducing the changes in this PR.Added are two new opt-in configuration items for IPv6:
addr_gen_modeeui64,stable-privacy,default-or-eui64anddefaultip6_privacyenabled,enabledbut prefer public,disabledanddefaultMore 'bout the options here
Configuration of
combines the best of both "worlds" because Privacy Extensions also adds an additional temporary address that is used for all ongoing connections, just like in the "stable-privacy" generation mode, but still has a fixed and deterministic address for all incoming connections.
I've been using this configuration myself since day 1 and it should also be the default in my opinion, but I understand that some may have a different one.
Edit: I just found out that there have been some attempts in the past to bring this configuration to HA:
So hopefully this one will be successful, hah 😆 But I also think it's not necessary to expose this configuration in the frontend.
Type of change
Additional information
--ipv6-addr-gen-modeand--ipv6-privacyoptions forha networkcli#566Checklist
ruff format supervisor tests)If API endpoints or add-on configuration are added/changed:
Summary by CodeRabbit
New Features
Bug Fixes
Tests