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/systemd: provide a typed interface to Exec directives #154154

Closed
wants to merge 3 commits into from
Closed

nixos/systemd: provide a typed interface to Exec directives #154154

wants to merge 3 commits into from

Conversation

pennae
Copy link
Contributor

@pennae pennae commented Jan 9, 2022

Motivation for this change

implements #154123. only very lightly tested so far. documentation also still needs updating/writing.

@roberth @NixOS/systemd

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.

@pennae pennae requested a review from dasJ as a code owner January 9, 2022 15:35
@pennae pennae marked this pull request as draft January 9, 2022 15:35
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Jan 9, 2022
@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 Jan 9, 2022
nixos/tests/systemd-escaping.nix Outdated Show resolved Hide resolved
nixos/tests/systemd-escaping.nix Outdated Show resolved Hide resolved
nixos/lib/systemd-unit-options.nix Outdated Show resolved Hide resolved
nixos/lib/utils.nix Outdated Show resolved Hide resolved
nixos/lib/utils.nix Outdated Show resolved Hide resolved
nixos/lib/systemd-unit-options.nix Outdated Show resolved Hide resolved
nixos/lib/systemd-unit-options.nix Show resolved Hide resolved
nixos/lib/systemd-unit-options.nix Outdated Show resolved Hide resolved
@aanderse
Copy link
Member

aanderse commented Jan 9, 2022

Some abstractions are good. Some aren't. I think script, preStart, postStart, and environment are pretty simple to understand and almost directly map to systemd declarations - because they don't do much. These declarations also keep us pretty close to upstream units, to such a degree that we can often just add systemd.packages = [ pkgs.foo ]; and be done with it.

When we start adding complex abstractions we make ourselves even more alien to the rest of the gnu/linux ecosystem and their developers. We increase the already incredibly steep learning curve with ecosystem specific knowledge that isn't transferable at all.

This is ok, but we have to ask ourselves "is it worth it?" every time we do. So I ask: is this abstraction worth it? Or are we better off taking another route?

@pennae
Copy link
Contributor Author

pennae commented Jan 10, 2022

an alternative solution is to add an analog of escapeShellArgs for systemd exec directives (#154113). we don't care much which one ends up being chosen, as long as there's a way for people to avoid making the same mistake over and over again. :)

@roberth
Copy link
Member

roberth commented Jan 10, 2022

I'd like to differentiate this feature from abstractions as those are commonly talked about. Whereas the feature is an abstraction in the sense of creating an indirection, it is not an abstraction in the sense of eliding "irrelevant" aspects, or encapsulating details. Much of the (correct) criticism of abstraction revolves around the latter parts.

Some abstractions are good. Some aren't.

Yes, and even at the same time. NixOS is great, because it doesn't force you into either one.
For example, you can enable our "abstracted" services, like services.postgresql, or define your own, using systemd.services.

Similarly, this added interface doesn't require you to avoid serviceConfig. You can still define services that way and stay visibly close to an upstream unit file.

On the other hand, when that is not a requirement, you can make use of well-defined, predictable options with consistent searchable documentation.

Consider a backend team developing custom company services on a Nix stack. Hopefully they've picked up on actual DevOps, so they'll have someone on the team who's familiar at least with the relevance of those symbols, etc, but the others should be able to interact easily with the config file as well. For them, a cryptic DSL like systemd's Exec lines only gets in the way.

When we start adding complex abstractions we make ourselves even more alien to the rest of the gnu/linux ecosystem and their developers.

Good thing this one isn't complex. Not even much of an abstraction; just a sensible format for defining service commands.
I find it hard to imagine that someone would like to express important security properties using otherwise meaningless symbols like + or !, so it would seem that of instead of alienation, we should see appreciation.

So I ask: is this abstraction worth it?

In my view, absolutely.

I appreciate you keeping us alert to this.

@roberth roberth requested a review from infinisil March 12, 2022 10:23
@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Sep 21, 2022
@roberth roberth mentioned this pull request Nov 29, 2023
13 tasks
@RaitoBezarius
Copy link
Member

I'm planning to adopt, with @pennae permission, this PR and push it to the finish line in the next days/weeks.

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 13, 2024
@pennae
Copy link
Contributor Author

pennae commented Jan 13, 2024

I'm planning to adopt, with @pennae permission, this PR and push it to the finish line in the next days/weeks.

absolutely!

@wegank wegank added 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 2.status: merge conflict This PR has merge conflicts with the target branch labels Mar 19, 2024
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
@pennae pennae closed this by deleting the head repository Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: user experience 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants