-
-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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/prometheus: Harden systemd service #162784
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on x84_64-linux without issues.
For reference: - ./nixos/modules/services/monitoring/grafana.nix - https://salsa.debian.org/go-team/packages/prometheus/-/blob/80192f1fe3e4b2b3a1816b4d2c4a628809acccbe/debian/service - https://github.com/archlinux/svntogit-packages/blob/5894b9b77a63f8d1aad434e190217ba5f4ba40d4/trunk/prometheus.service I have omitted the Limit* as they do not appear to be commonly used in NixOS, and, per `man systemd.exec`, are less preferred vs. cgroup limits.
3f8c503
to
26ca4d1
Compare
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/1042 |
@ofborg test prometheus |
@winterqt Hi! Thanks for kicking off the tests. In case you missed it, they all passed so wondering if there are any other blockers for this or if you have any other comments. Thanks! |
@amarshall I was waiting on the other reviewers, hopefully the activity will remind them 🙂 These changes do look good at a glance, though, so if nobody takes a look at it I will take a closer look. |
@amarshall, @Ma27 I'm having immediate issues with prometheus being killed for saying |
Curious, when does it do that? But if that's causing issues, would you mind filing a PR? :) |
This is an issue with go 1.19: #197443 |
Meh... will file a PR for that. |
See the discussion below the original PR[1] and NixOS#197443 for more context. I guess I missed that upon review because the branch was too old and I cherry-picked the commit onto my deployment branch which is based on 22.05. Sorry for that! [1] NixOS#162784 (comment)
See the discussion below the original PR[1] and NixOS#197443 for more context. I guess I missed that upon review because the branch was too old and I cherry-picked the commit onto my deployment branch which is based on 22.05. Sorry for that! [1] NixOS#162784 (comment)
Motivation for this change
Services should be hardened when possible.
For reference:
I have omitted the Limit* as they do not appear to be commonly used in
NixOS, and, per
man systemd.exec
, are less preferred vs. cgrouplimits.
Things done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)nixos/doc/manual/md-to-db.sh
to update generated release notes