[WIP] systemd.network: module fixes #40573
Conversation
The previously allowed values were simply wrong. Use the one that are used. From systemd.network(5): > Enables DHCPv4 and/or DHCPv6 client support. Accepts "yes", "no", "ipv4", or "ipv6". Defaults to "no".
This is similar to VLAN=, MACVLAN=, VXLAN= and Tunnel=, which live in their own attr.
his is similar to VLAN=, MACVLAN=, VXLAN= and Tunnel=, which live in their own attr.
| @@ -96,7 +96,7 @@ let | |||
| "Description" "DHCP" "DHCPServer" "IPForward" "IPMasquerade" "IPv4LL" "IPv4LLRoute" | |||
| "LLMNR" "MulticastDNS" "Domains" "Bridge" "Bond" "IPv6PrivacyExtensions" | |||
| ]) | |||
| (assertValueOneOf "DHCP" ["both" "none" "v4" "v6"]) | |||
There was a problem hiding this comment.
Nitpick about the commit message:
It's that those were wrong. They were simply not being documented anymore. As per systemd policy you do not (immediately) remove those old options but instead drop them from the documentation ;-)
https://github.com/systemd/systemd/blob/master/src/network/networkd-network.c#L832-L840
There was a problem hiding this comment.
ah, wasn't aware they are still being parsed correctly. Will update the commit message.
| @@ -94,7 +94,7 @@ let | |||
| checkNetwork = checkUnitConfig "Network" [ | |||
| (assertOnlyFields [ | |||
| "Description" "DHCP" "DHCPServer" "IPForward" "IPMasquerade" "IPv4LL" "IPv4LLRoute" | |||
| "LLMNR" "MulticastDNS" "Domains" "Bridge" "Bond" "IPv6PrivacyExtensions" | |||
There was a problem hiding this comment.
So this breaks "old" configurations?
Shouldn't we have a change log entry for that? Or even better support (time limited) backwards compatibility?
There was a problem hiding this comment.
yes, it breaks old configurations that uses those attributes before.
As it's currently not the default for network management, plus the module is already pretty huge, I'd be more in favor of breaking/changing this for unstable/18.09 (with a changelog entry), than introducing a compability shim that blows up the code even more.
| @@ -94,7 +94,7 @@ let | |||
| checkNetwork = checkUnitConfig "Network" [ | |||
| (assertOnlyFields [ | |||
| "Description" "DHCP" "DHCPServer" "IPForward" "IPMasquerade" "IPv4LL" "IPv4LLRoute" | |||
| "LLMNR" "MulticastDNS" "Domains" "Bond" "IPv6PrivacyExtensions" | |||
There was a problem hiding this comment.
So this breaks "old" configurations?
Shouldn't we have a change log entry for that? Or even better support (time limited) backwards compatibility?
| @@ -484,6 +484,16 @@ let | |||
| ''; | |||
| }; | |||
|
|
|||
| vrf = mkOption { | |||
There was a problem hiding this comment.
Did you even test this?
Last time I checked after implementing this in networkd you also had to have a matching .netdev file with Kind=vrf. Currently we are not allowing Kind=vrf. See https://github.com/flokli/nixpkgs/blob/69c091096769ca2ce001beb1072ff0a25812be4e/nixos/modules/system/boot/networkd.nix#L33
There was a problem hiding this comment.
This PR is WIP and only contains code for the .network files currently.
Adding a Kind=vrf netdev via systemd.network.netdevs.<name>.vrfConfig is currently missing, as I wanted to get some comments early in the run.
You can replicate the same via systemd.network.netdevs.<name>.extraConfig for now.
|
cc @fpletz |
|
Thank you for importing my fixes =) |
|
Superseded by #44233 (merged) |
Motivation for this change
This PR fixes some wrong assertions currently in the
systemd.networkmodule, adds support for some not-yet-supported options, and moves some other options around to make everything them more consistent with the rest.Contains the DHCP values fix from #35841, but nothing from the controversial "disable IPv6 usecase".
I'd love to get some feedback from people using networkd, in concert with the nixos module
/cc @andir @Mic92 @netixx
Things done
build-use-sandboxinnix.confon non-NixOS)nix-shell -p nox --run "nox-review wip"./result/bin/)