nixos/networkd: update configuration options #82026
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andir
changed the title
|
Awesome, it was on my todo list! (shame on me: I still use `extraConfig` for that part ><)
One thing though: the systemd configuration parser won't error when encountering some unknown keys. That's kind of an issue when we'll update systemd and a key has been removed/renamed. The module could silently break.
One way to mitigate that problem is to add some nixos tests and check that each networkd directive has indeed been applied to a test network. We started doing that in nixos/tests/systemd-networkd.nix.
Do you plan to add some tests for the RA/DHCPv6 setup? If you don't, could you please wait for me to add some RA-related tests before merging that?
|
flokli
requested changes
Mar 9, 2020
| <para> | ||
| The <literal>systemd-networkd</literal> option | ||
| <literal>systemd.network.networks.<name>.dhcp.CriticalConnection</literal> | ||
| has been removed following upstream systemds deprecation of the same. It is recommended to use |
There was a problem hiding this comment.
Suggested change
| has been removed following upstream systemds deprecation of the same. It is recommended to use | |
| has been removed following upstream systemd's deprecation of the same. It is recommended to use |
| @@ -641,13 +640,13 @@ let | |||
| ''; | |||
| }; | |||
|
|
|||
| dhcpConfig = mkOption { | |||
| dhcpV4Config = mkOption { | |||
There was a problem hiding this comment.
We probably still want to add a mkRemovedOptionModule, and print a short summary of what's in the release notes instead of just dropping the configuration options here.
There was a problem hiding this comment.
I tried adding that but due to the nature of how those work that isn't really feasible here. We have a "wildcard" network name in between and can only really assert if someone tries to access that value.
I tried a few things:
- traversing the
configtree to see if the option is set, that does then trigger the "on access" assertion - traversing the
optiontree (as it is done in the original helper) but that doesn't seem possible for these kinds of situations..
Here is the code I ended up backing out of this changest in case anyone wants to give it a shot:
# systemd.network.networks.*.dhcpConfig has been deprecated in favor of ….dhcpV4Config
# Produce a nice warning message so users know it is gone.
{
assertions = let
#affectedNetworks = attrNames (filterAttrs (network: conf: conf.dhcpConfig.isDefined) options.systemd.network.networks.values);
affectedNetworks = attrNames (filterAttrs (network: conf: conf ? dhcpConfig) config.systemd.network.networks);
in
[
{
assertion = length affectedNetworks == 0;
message = ''
The option definition `systemd.network.networks.*.dhcpConfig` has been deprecated in favor of `systemd.network.networks.*.dhcpV4Config`. Please update your configuration.
The following networks are still using it:
${concatMapStringsSep "\n" (network: " - `systemd.network.networks.\"${network}\"`") affectedNetworks}
'';
}
];
}There was a problem hiding this comment.
For now I just left the "on access" throw in the code as that is the least we could do. I do not really want to make it an alias.
andir
force-pushed
the
systemd-update-networkd-options
branch
from
24683bd
to
dbf5584
Compare
|
@andir can you address the requested changes, and rebase on latest master? |
|
On 00:49 08.03.20, Félix Baylac-Jacqué wrote:
Awesome, it was on my todo list!
One thing though: the systemd configuration parser won't error when
encountering some unknown keys. That's kind of an issue when we'll
update systemd and a key has been renamed. The module could silently
break.
One way to mitigate that problem is to add some nixos tests and check
that each networkd directive has indeed been applied to a test
network. We started doing that in nixos/tests/systemd-networkd.nix.
Do you plan to add some tests for the RA/DHCPv6 setup? If you don't,
could you please wait for me to add some RA-related tests before
merging that?
Yes, I have some extensive test suite locally that tests the whole
IA-NA/IA-PD setup but I do not intend to commit it with this PR. It
needs a bunch of polishing. Will likely happen soon(tm) but not
immediately.
|
andir
force-pushed
the
systemd-update-networkd-options
branch
from
dbf5584
to
26a62ca
Compare
…nection Systemd upstream has deprecated CriticalConnection with v244 in favor of KeepConnection as that seems to be more flexible: The CriticalConnection= setting in .network files is now deprecated, and replaced by a new KeepConfiguration= setting which allows more detailed configuration of the IP configuration to keep in place.
You can now specify option for the `[DHCPv6]` section with `systemd.network.<name>.dhcpV6Config.…`. Previously you could only use the combined legacy DHCP configuration.
This follows upstreams change in documentation. While the `[DHCP]` section might still work it is undocumented and we should probably not be using it anymore. Users can just upgrade to the new option without much hassle. I had to create a bit of custom module deprecation code since the usual approach doesn't support wildcards in the path.
With sytemd v244 we will have support for this option.
andir
force-pushed
the
systemd-update-networkd-options
branch
from
26a62ca
to
00215e5
Compare
|
Rebased and fixed a few typos. Any further requests? @flokli As I stated earlier the tests I plan to add are probably too big to be in scope of this change and will require some discussions while these changes here should not be controversial. |
|
Adding the tests later is fine. Let's merge this 👍 |
flokli
approved these changes
May 1, 2020
10 tasks
andir
added a commit
to andir/nixpkgs
that referenced
this pull request
May 1, 2020
This is a follow-up to the PR NixOS#82026 that contains the promised tests. In this test I am testing if we can properly propagate prefixes received via DHCPv6 PD with the networkd options in our module system. The comments in the test should be sufficient to follow the idea and what is going on.
10 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation for this change
While working on my person home router configuration I started adding a few more options to our systemd-networkd configuration options. The entire area around prefix delegation wasn't really covered. Also upstream just recently added support for
PrefixDelegationHintwhich is a must-have (for me). That flag will become available soon. @NinjaTrappeur is working on the systemd v244 upgrade.Things done
sandboxinnix.confon non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)nix path-info -Sbefore and after)