Add DuplicateAddressDetection settings for systemd-networkd (LP: #1959190) #525
Conversation
* implement configuration key duplicate-address-detection * documentation of the parameter * adding a few tests
slyon
added
community
slyon
changed the title
There was a problem hiding this comment.
Hello @sanecz Thank you very much for your high quality contribution to Netplan and for tackling this long-standing feature request in LP#1959190!
This PR is already pretty good, as you adopted many of the required parts of Netplan to make it work, like:
- networkd.c backend renderer
- netplan.c YAML emitter
- unit tests
- Python bindings
- Fuzzer schemas
- passes all CI checks
As a non-blocking requirement, we should also consider adding:
- nm.c (NetworkManager) backend renderer
- Integration test(s), e.g. in
tests/integration/ethernets.py
Before we get into the implementation details, I'd like to talk about the new parsing logic and interfaces that we will expose to our users. And also the data structures for storage. I don't think the DAD-per-address is the optimal solution here. Also, we should probably choose something more specific than a string (char*) to store the data internally.
Generally, I like the duplicate-address-detection naming. There's also the notion of "IPv4 Address Conflict Detection" (ACD) [RFC 5227], but I have the impression that "Duplicate Address Detection" (DAD) is used commonly across IPv4 & IPv6 these days. See:
- systemd-networkd: networkd: address add support to configure flags systemd/systemd#4201 & network: introduce DAD for static IPV4 address systemd/systemd#14102
- NetworkManager: ipv4.dad-timeout & ipv6.dad-timeout
Besides that, I think the new duplicate-address-detection setting should move to the per-NetDef/interface level. As an "ipv6" or "both" setting doesn't make sense for a specific IPv4 address and vice versa. Furthermore, NetworkManager apparently only allows defining this per-connection, not per-address via its ipv4/6.dad-timeout settings.
So I'd suggest the following schema:
ethernets:
eth0:
addresses: [169.254.0.13/16]
dhcp6: true
duplicate-address-detection: [ipv4-ll, ipv6] # could be [ipv4], [ipv6], [ipv4, ipv6], or the empty list "[]"The default could be implicitly represented as [ipv4-ll, ipv6], as that is what networkd uses today.
What do you think about those suggestions?
I'll also ask for feedback from our architect @vorlonofportland about this.
| const mapping_entry_handler address_option_handlers[] = { | ||
| {"lifetime", YAML_SCALAR_NODE, {.generic=handle_address_option_lifetime}, addr_option_offset(lifetime)}, | ||
| {"label", YAML_SCALAR_NODE, {.generic=handle_address_option_label}, addr_option_offset(label)}, | ||
| {"duplicate-address-detection", YAML_SCALAR_NODE, {.generic=handle_address_option_duplicate_address_detection}, addr_option_offset(duplicate_address_detection)}, |
There was a problem hiding this comment.
I think this setting should move to COMMON_LINK_HANDLERS.
For network-manager, we can only control it on a per ipv4/ipv6 level:
- https://www.networkmanager.dev/docs/api/1.32.8/settings-ipv4.html
- https://www.networkmanager.dev/docs/api/1.32.8/settings-ipv6.html
Also, when defining it directly on a IPv4 address only none or ipv4 makes sense (boolean effectively, maybe "ipv4-ll" would be a special case), while when defining it directly on a IPv6 address, only none (false) or ipv6(true) makes sense. So I think it should be a per NetDef/interface setting, that way it could also describe dynamic addresses, like DHCP or IPv6RA.
| g_ascii_strcasecmp(scalar(node), "none") != 0) { | ||
| return yaml_error(npp, node, error, "invalid duplicate-address-detection value '%s'", scalar(node)); | ||
| } | ||
| return handle_generic_str(npp, node, npp->current.addr_options, data, error); |
There was a problem hiding this comment.
I think we should rather store this in a guint and represent it as an enum... Maybe something along those lines?
typedef enum {
NETPLAN_DAD_UNSET = 0,
NETPLAN_DAD_BOTH = 1<<0,
NETPLAN_DAD_NONE = 1<<1,
NETPLAN_DAD_IPV4LL = 1<<2,
NETPLAN_DAD_IPV4 = 1<<3,
NETPLAN_DAD_IPV6 = 1<<4,
NETPLAN_DAD_MAX_,
} NetplanDadFlag;The current (networkd) default could then be reflected as NETPLAN_DAD_IPV4LL | NETPLAN_DAD_IPV6
More flags could be added as needed in the future and the backend renderers (networkd.c / nm.c) would be able to handle only the flags that they actually understand individually, writing out the corresponding per-address or per-connection settings.
| @@ -422,6 +422,12 @@ Match devices by MAC when setting options like: `wakeonlan` or `*-offload`. | |||
| > In addition to the addresses themselves one can specify configuration | |||
| > parameters as mappings. Current supported options are: | |||
|
|
|||
| - **`duplicate-address-detection`** (scalar) | |||
There was a problem hiding this comment.
I like the duplicate-address-detection naming. There's also the notion of "IPv4 Address Conflict Detection" (ACD) [RFC 5227], but I have the impression that "Duplicate Address Detection" (DAD) is used commonly across IPV4 & IPv6 these days.
See
| > Configure the duplicate address detection (DAD). Valid options | ||
| > are `ipv4`, `ipv6`, `both` or `none`. Currently supported on the | ||
| > networkd back end only. |
There was a problem hiding this comment.
This is currently a direct copy of the networkd settings. I'd rather like to represent this as a sequence/list of flags, though. Maybe on the global level instead of per-address.
e.g.
ethernets:
eth0:
addresses: [169.254.0.13/16]
dhcp6: true
duplicate-address-detection: [ipv4-ll, ipv6]The current default could be represented as [ipv4-ll, ipv6], we could have "both" [ipv4, ipv6] or "none" []. ipv4 should probably include ipv4-ll implicitly.
On a per-IP level it does not alway make sense, or is there any use case for this? E.g:
ethernets:
eth0:
addresses:
- 169.254.0.13/16:
duplicate-address-detection: "ipv6"
dhcp6: trueThere was a problem hiding this comment.
On a per-IP level it does not alway make sense, or is there any use case for this? E.g: [...]
I don't think this it makes sense too to have this configuration
But thinking of weird corner cases, I don't know why someone would do that, but it still is a valid configuration for networkd, that cannot be reproduced with nm afaik.
ethernets:
eth0:
addresses:
- 172.16.0.1/24:
duplicate-address-detection: "ipv4"
- 169.254.0.13/16:
duplicate-address-detection: "none"
dhcp6: true
|
We might also consider enabling this feature by default, similarly as it was done in NetworkManager by Fedora 40: https://fedoraproject.org/wiki/Changes/Enable_IPv4_Address_Conflict_Detection E.g. defaulting to |
| @@ -422,6 +422,12 @@ Match devices by MAC when setting options like: `wakeonlan` or `*-offload`. | |||
| > In addition to the addresses themselves one can specify configuration | |||
| > parameters as mappings. Current supported options are: | |||
|
|
|||
| - **`duplicate-address-detection`** (scalar) | |||
There was a problem hiding this comment.
nitpick (non-blocking):
| - **`duplicate-address-detection`** (scalar) | |
| - **`duplicate-address-detection`** (scalar) – since 1.2 |
|
Hi @slyon Thank you for your feedback, really appreciate it !
For the NetworkManager render, as I do not have much knowledge on it, I'll take a little time to explore and experiment configurations for it.
Sure I'll take a look and implement it.
Agreed, as you mentioned on a later comment, an enum would do perfectly the job.
I entirely agree.
It seems that systemd-networkd and NetworkManager do really differs on the way of configuring it, as networkd mentions DAD configuration only on the address section option and there is only one address declared by section, and not much more, while with networkmanager as you previously mentioned, to enable the dad per interfaces it is enabled by setting the dad-timeout to a value > 0 for ipv4 and ipv6. So it's not easy to think about a one-fit-both configuration. [2] This also means that another (optional) configuration key only available for nm should exists to set the timeout instead of a fixed value. The networkd configuration does not seem to have an equivalence. (I think only the number of probes sent can be changed)
True, this schema seems clearer than setting directly the networkd keywords, especially if nm is supported too !
On purpose I didn't set a default value to use the default behavior of the network backend thus avoiding issues for some configurations too (and some people that might ask why it takes few seconds longer to up the network too). -- Howerver, as mentioned on [1], if netplan does coherence check, we can manage to check the family of the ip and set the only proper configuration keys per address/interface or throw an error or a warning. About the setting [2], I'm still not sure what could be the best idea:
I'm looking forward to hearing your ideas ! |
As mentioned, I see this as non-blocking. It's fine to focus on systemd-networkd for now. The implementation for NetworkManager could then be a follow-up PR.
Thanks!
IMO coherence checks should happen on the highest layer (e.g. inside Netplan), we don't want our users to dig down into the stack, to understand that they described an invalid configuration. We should make it obvious from the very start.
Sure, but I think we could cover this with the schema I suggested above, e.g.:
Yes, unfortunately there are many such nuanced difference between networking backends. With Netplan we try unify them in a best effort approach. For this DAD case, I could think of setting NM's
Thanks! I've also got +1 from Steve, our architect, about this (after some back channel discussions). So let's go with that.
I agree. Let's try not to change any default for now. I'd suggest going with
Right, there's a balance to strike here. With the proposed |
|
@sanecz Do the above comments clarify the next steps for you? Should there be any specific issues, or blocks, feel free to ask about it in here! Using the recommended schema of |
|
Hello @slyon, yes it is clear! Thank you :) I'm on it but not much time recently :/ Great news for approval of the schema! |
Description
Adding DuplicateAddressDetection parameter to be able to configure the DAD as by default it is enabled for ipv6 and ipv4ll.
It should kinda close LP#1959190 as it would provide a way to configure it, but not setting any default value to avoid breaking network configurations.
Checklist
make checksuccessfully.make check-coverage).