-
-
Notifications
You must be signed in to change notification settings - Fork 17.7k
nixos/qui: init service #472934
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
base: master
Are you sure you want to change the base?
nixos/qui: init service #472934
Conversation
|
|
||
| serviceConfig = { | ||
| Type = "simple"; | ||
| DynamicUser = true; |
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.
This looks mostly fine to me but I think it might interfere with QUI's ability to use external tools? I'd rather add an option for that. I'll check qui's docs and I'll give a proposal in a bit...
I say this because I've had this annoyance with Flood web UI where the NixOS module doesn't allow mediainfo to work by default. Would be nice if this works from the start here.
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.
Actually, nevermind this. I was misunderstanding the level of sandboxing this applies, turns out this should be fine.
I say LGTM. Further improvements can be made later!
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.
Thanks for the quick review, and packaging qui. Just discovered that I forgot to set a default for the settings option. That's fixed now, and also checked with an additional test.
Not completely sure about the interference with external tools. It's possible to set the programs that are allowed to be executed via externalProgramAllowlist. Which is possible via this module.
The main issue I can think of is that an external programs needs to write files outside of StateDirectory, CacheDirectory, LogsDirectory etc. By setting ReadWritePaths it is still possible however. For "normal" usage I don't think this would be an issue.
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.
Testing the feature i don't think it works at all right now. It starts whatever you give it with sh -c, and it complains about missing it. So i think bash should be added to the service PATH.
{"level":"error","error":"exec: \"sh\": executable file not found in $PATH","program":"test","hash":"007425713928ad60f0c76fd9692a6e136a99d676","command":"[sh -c /etc/profiles/per-user/user/bin/tmux]","time":"2025-12-30T14:56:54+02:00","message":"External program failed to start"}My preference would be an option in the module to soften the hardening to allow it access to outside folders if the user desires it. Something like this:
lessenHardening = mkOption {
type = bool;
default = false;
description = "Allow external tools to write to files outside of the service.";
};I say LGTM. Further improvements can be made later!
but at same time i agree with this, i feel like this is extra functionality that can be improved on later, personally i dont think i would ever use this feature anyway.
On an unrelated note: i don't really like declaring the external tools via the qui GUI. Would be nice if we had an option to declare them in nix. If we had that option it would also simplify externalProgramAllowlist usage since it could be autofilled from whatever you declared in nix. e.g something like this:
externalTools = {
my-notify-tool = writeShellApplication { # The path to this shell application would be added to externalProgramAllowlist and declared as an external tool
name = "my-notify-tool";
runtimeInputs = [
pkgs.ntfy
];
text = ``
ntfy publish mytopic "{hash} {name} sending info about torrent" # This template replacement part probably wouldn't work in this pseudo code example but you will get the general concept
``;
};
};As far as i can tell its not possible to declare external tools in config right now but can always be added to qui.
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.
That's some useful info. I don't use the feature myself, so I didn't know all of this.
Will leave the module like this for the initial merge. If someone comes along that wants to use this feature, possibly me, I might look into it some more.
Wish you all a happy new year!
cbd3d68 to
8e8158f
Compare
|
8e8158f to
28bb1c7
Compare
|
Thanks for your review @BatteredBunny. Just pushed a commit that includes your proposed changes. Any input on the use, and potential issues, of |
|
As discussed in the comment thread the current state of the module is enough for the initial merge. Even though there are some rough edges when trying to run external programs. In a future PR we could look into improving support for the external programs functionality. |
|
|
The newest qui release also has hardlink support. Will need to rework the service a bit to allow for hardlinking. For hardlinking to work, the service needs Will do the following:
|
28bb1c7 to
5837546
Compare
These changes are implemented. Largely reused the hardening settings that are used for nemorosa (#462968). For that service the hardlinks are working. However there I did set Personally I don't use the cross-seeding functionality so I can't check it 100%. If the sandboxing turns out to be too tight it can always be relaxed later. Basic functionality is there at the moment. |
5837546 to
ba32ded
Compare
|
The qui packages has recently been merged to nixpkgs. In this PR a service module is created.
Like with the package the service module is quite similar to the
autobrrmodule.Appreciate any feedback.
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.