Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nixos/networkd: update configuration options #82026

Merged
merged 8 commits into from May 1, 2020

Conversation

@andir
Copy link
Member

andir commented Mar 7, 2020

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 PrefixDelegationHint which is a must-have (for me). That flag will become available soon. @NinjaTrappeur is working on the systemd v244 upgrade.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
@andir andir changed the title nixos/systemd: update configuration options nixos/networkd: update configuration options Mar 7, 2020
@Ma27 Ma27 requested review from NinjaTrappeur, fpletz, flokli and Ma27 Mar 7, 2020
@NinjaTrappeur
Copy link
Contributor

NinjaTrappeur commented Mar 8, 2020

@Ma27
Ma27 approved these changes Mar 8, 2020
Copy link
Member

Ma27 left a comment

LGTM 👍

<para>
The <literal>systemd-networkd</literal> option
<literal>systemd.network.networks.&lt;name&gt;.dhcp.CriticalConnection</literal>
has been removed following upstream systemds deprecation of the same. It is recommended to use

This comment has been minimized.

Copy link
@flokli

flokli Mar 9, 2020

Contributor
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
nixos/modules/system/boot/networkd.nix Show resolved Hide resolved
@@ -641,13 +640,13 @@ let
'';
};

dhcpConfig = mkOption {
dhcpV4Config = mkOption {

This comment has been minimized.

Copy link
@flokli

flokli Mar 9, 2020

Contributor

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.

This comment has been minimized.

Copy link
@andir

andir Apr 28, 2020

Author Member

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 config tree to see if the option is set, that does then trigger the "on access" assertion
  • traversing the option tree (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}
      '';
    }
  ];
}

This comment has been minimized.

Copy link
@andir

andir Apr 28, 2020

Author Member

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.

nixos/modules/system/boot/networkd.nix Show resolved Hide resolved
@andir andir force-pushed the andir:systemd-update-networkd-options branch from 24683bd to dbf5584 Apr 14, 2020
@flokli
Copy link
Contributor

flokli commented Apr 27, 2020

@andir can you address the requested changes, and rebase on latest master?

@andir
Copy link
Member Author

andir commented Apr 28, 2020

@andir andir force-pushed the andir:systemd-update-networkd-options branch from dbf5584 to 26a62ca Apr 28, 2020
andir added 8 commits Jan 8, 2020
…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 andir force-pushed the andir:systemd-update-networkd-options branch from 26a62ca to 00215e5 May 1, 2020
@andir
Copy link
Member Author

andir commented May 1, 2020

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.

@flokli
Copy link
Contributor

flokli commented May 1, 2020

Adding the tests later is fine. Let's merge this 👍

@flokli
flokli approved these changes May 1, 2020
@flokli flokli merged commit 0a98d10 into NixOS:master May 1, 2020
14 checks passed
14 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="00215e5"; rev="00215e5bc0f046b0da8af5f5955b13ee8d915d51"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="00215e5"; rev="00215e5bc0f046b0da8af5f5955b13ee8d915d51"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="00215e5"; rev="00215e5bc0f046b0da8af5f5955b13ee8d915d51"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="00215e5"; rev="00215e5bc0f046b0da8af5f5955b13ee8d915d51"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="00215e5"; rev="00215e5bc0f046b0da8af5f5955b13ee8d915d51"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="00215e5"; rev="00215e5bc0f046b0da8af5f5955b13ee8d915d51"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="00215e5"; rev="00215e5bc0f046b0da8af5f5955b13ee8d915d51"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
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.
@andir andir mentioned this pull request May 1, 2020
3 of 10 tasks complete
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.
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.
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.
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.
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.
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.
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.