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.

Sign up for GitHub

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

Jump to bottom

Systemd stage 1 networkd #169116

Merged
merged 10 commits into from
Apr 21, 2023
Merged

Systemd stage 1 networkd #169116

merged 10 commits into from
Apr 21, 2023

Conversation

ElvishJerricco
Copy link
Contributor

@ElvishJerricco ElvishJerricco commented Apr 17, 2022 •
edited
Loading

Description of changes

Use systemd-networkd for networking in stage 1.

  • Adds a mechanism for users/groups in initrd, since that's required for the systemd-network user/group.
  • Configures networking.interfaces to the same level as scripted initrd.
  • Supports SSH and OpenVPN
  • Adds dbus with socket activation so that networkctl can work.
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@ElvishJerricco ElvishJerricco requested review from a team and dasJ as code owners April 17, 2022 22:10
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: systemd 8.has: module (update) This PR changes an existing module in `nixos/` labels Apr 17, 2022
@ElvishJerricco ElvishJerricco marked this pull request as draft April 17, 2022 22:11
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Apr 17, 2022
@ElvishJerricco ElvishJerricco removed the request for review from ckiee April 17, 2022 22:55
flokli
flokli reviewed Apr 18, 2022
View reviewed changes
Copy link
Contributor

@flokli flokli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite understand why we can't just also ship all .network and .netdev files in the initrd. Did you run into troubles when doing that?

nixos/modules/system/boot/systemd/initrd.nix
@@ -364,7 +356,7 @@ in {

"/etc/systemd/system.conf".text = ''
[Manager]
DefaultEnvironment=PATH=/bin:/sbin ${optionalString (isBool cfg.emergencyAccess && cfg.emergencyAccess) "SYSTEMD_SULOGIN_FORCE=1"}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unrelated to this PR, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sort of? I needed it because sshd requires the root account to be unlocked.

@ElvishJerricco ElvishJerricco mentioned this pull request May 4, 2022
Merged
13 tasks
@ElvishJerricco ElvishJerricco force-pushed the systemd-stage-1-networkd branch from e9ee154 to 88e7633 Compare June 28, 2022 21:23
@ElvishJerricco ElvishJerricco force-pushed the systemd-stage-1-networkd branch from 88e7633 to 7a778fd Compare July 22, 2022 06:34
@github-actions github-actions bot added the 6.topic: kernel The Linux kernel label Jul 22, 2022
SuperSandro2000
SuperSandro2000 reviewed Jul 22, 2022
View reviewed changes
nixos/modules/services/networking/ssh/sshd.nix Outdated
restartTriggers = optionals (!cfg.startWhenNeeded) [
config.environment.etc."ssh/sshd_config".source
];
path = [ cfgc.package pkgs.gawk pkgs.coreutils ];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
path = [ cfgc.package pkgs.gawk pkgs.coreutils ];
path = with pkgs; [ cfgc.package gawk coreutils ];

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather avoid with whenever reasonable. Even in simple examples like this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually would say we should highly recommend it to be used in small lists with repeating prefixes. With my change it is very easy to see with a glance what packages are added but with the extra pkgs. prefix I can barely parse it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On top of the fact that I and many others strongly disagree with that on account of with introducing dynamic scoping that requires logical analysis instead of simpler lexical analysis to understand, this is also not a matter of substance to this PR as the line being commented on is a pre-existing one.

@ElvishJerricco ElvishJerricco force-pushed the systemd-stage-1-networkd branch from 7a778fd to eaae5cb Compare July 22, 2022 10:04
@ElvishJerricco ElvishJerricco force-pushed the systemd-stage-1-networkd branch 2 times, most recently from ba27910 to 1ee4193 Compare July 23, 2022 10:22
@ElvishJerricco ElvishJerricco force-pushed the systemd-stage-1-networkd branch from 1ee4193 to a76cd70 Compare August 3, 2022 11:39
@ElvishJerricco
Copy link
Contributor Author

I've added a warning if no networks are configured, since we don't automatically configure anything like the scripted initrd does.

I've also decided not to mangle sshd.nix to have it serve both stage 1 and 2. Instead I've made a much simpler patch to implement effectively what the scripted initrd does with a simple service.

@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Aug 3, 2022
@ElvishJerricco ElvishJerricco force-pushed the systemd-stage-1-networkd branch from a76cd70 to a0f0c2d Compare August 3, 2022 11:44
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Aug 3, 2022
@ElvishJerricco ElvishJerricco force-pushed the systemd-stage-1-networkd branch from a0f0c2d to 63aaff4 Compare August 3, 2022 17:19
@ElvishJerricco ElvishJerricco force-pushed the systemd-stage-1-networkd branch from 4ce87ec to 47c05f9 Compare April 11, 2023 19:48
@ElvishJerricco
Copy link
Contributor Author

Rebased on nixos-unstable-small, along with the openvpn mkdir /tmp change mentioned above

flokli
flokli reviewed Apr 11, 2023
View reviewed changes
Copy link
Contributor

@flokli flokli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd personally feel more comfortable if the OpenVPN and flush bits would be separate PRs. It might trigger some more discussion, blocking this PR more than necessary.

nixos/modules/system/boot/initrd-openvpn.nix Outdated
@@ -71,6 +78,14 @@ in
boot.initrd.network.postCommands = ''
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we have OpenVPN both as postCommand forking off and as a systemd unit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The network.postCommands part is for scripted stage 1. The systemd stage 1 has been deliberately designed not to implement the boot.initrd.*Commands options, for reasons outlined in the original PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pushed changes adopting the convention of using mkIf (!config.boot.initrd.systemd.enable) for these *Commands options in the ssh and openvpn modules, in order to make it clear what they're for, and to help ensure those options are not used at all when systemd initrd is in use.

@ElvishJerricco ElvishJerricco force-pushed the systemd-stage-1-networkd branch 3 times, most recently from d310410 to e94f56b Compare April 11, 2023 22:13
lilyinstarlight
lilyinstarlight reviewed Apr 12, 2023
View reviewed changes
Copy link
Member

@lilyinstarlight lilyinstarlight left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks pretty fabulous; thank you for this work!

Most of the technical questions I would have had seem to have already been asked or addressed, but I did leave a few review comments

@ElvishJerricco ElvishJerricco force-pushed the systemd-stage-1-networkd branch 2 times, most recently from 06f89d3 to 0ff1545 Compare April 15, 2023 08:04
@ElvishJerricco ElvishJerricco force-pushed the systemd-stage-1-networkd branch from 0ff1545 to 3cb9534 Compare April 17, 2023 20:42
@infinisil infinisil added the significant Novel ideas, large API changes, notable refactorings, issues with RFC potential, etc. label Apr 19, 2023
@flokli flokli merged commit 6b27ed3 into NixOS:master Apr 21, 2023
@colemickens
Copy link
Member

🎊 🚀 I've been excited for this one for a while, thank you to everyone!

@SuperSandro2000 SuperSandro2000 mentioned this pull request Nov 7, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mweinelt mweinelt mweinelt left review comments

@flokli flokli flokli left review comments

@lilyinstarlight lilyinstarlight lilyinstarlight left review comments

@colemickens colemickens colemickens left review comments

@dasJ dasJ dasJ left review comments

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

@mlyxshi mlyxshi mlyxshi left review comments

@gkleen gkleen gkleen left review comments

@oddlama oddlama oddlama approved these changes

@0x4A6F 0x4A6F 0x4A6F requested changes

@phaer phaer phaer approved these changes

@arianvp arianvp Awaiting requested review from arianvp

@andersk andersk Awaiting requested review from andersk

Assignees
No one assigned
Labels
6.topic: kernel The Linux kernel 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: systemd 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 significant Novel ideas, large API changes, notable refactorings, issues with RFC potential, etc.
Projects
systemd in Stage 1
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.