nncp: 7.7.0 -> 8.0.2, add NixOS module #146011
Conversation
There already exists a package "hjson" but it's written in Python.
github-actions
bot
added
6.topic: nixos
|
Great work! My comments:
|
|
@woffs thanks. The configuration mangling and merging was working for me yesterday but now I'm getting similar errors. Maybe it breaks after a certain amount of neighbour complexity. Its a bit unusual that the conversion is happening in an activation script and not as part of a systemd unit, were an error might be more obvious. I think it needs to be thought out some more. a1dfbf6 doesn't need to be part this PR, but found myself copy-pasting from documentation from the u9fs module when I should just look at the systemd socket options themselves. Also, systemd is not passing everything that |
|
Maybe you want to change nccp to nncp in the topic :-) |
ehmry
changed the title
This update introduces an encrypted packet format that is incompatible with releases older than 8.0.0.
Add a module that generates a global NNCP configuration and adds a TCP synchronization service.
|
@woffs I tweaked the configuration generator and the error has gone away for me (and I fixed the topic, thanks). |
|
I'm not sure what the "user" should be for the nncp-daemon. My thought was that if there is a spool directory that can be modified by both a service and normal users then a non-dynamic user is appropriate, but systemd complains that using |
| spool = mkDefault "/var/spool/nncp"; | ||
| log = mkDefault "/var/spool/nncp/log"; |
There was a problem hiding this comment.
Can we not user directories favoured by systemd instead? /var/lib/nncp and /var/log/nncp? This gives us a few benefits when integrating with systemd.
There was a problem hiding this comment.
This is a traditional spool directory and I would not move it. It would contradict the spirit of NNCP as a UUCP replacement as well as the documentation.
| whether <literal>autotoss</literal> is enabled and which | ||
| commands are enabled for remote execution. | ||
| ''; | ||
| default = "nobody"; |
There was a problem hiding this comment.
As already mentioned nobody/nogroup shouldn't be used.
| group = mkOption { | ||
| type = types.str; | ||
| default = "nncp"; | ||
| description = '' | ||
| UNIX group that users must be in to use <literal>nncp</literal>. | ||
| ''; | ||
| }; |
There was a problem hiding this comment.
Does this option make the module easier to use for the user or make the module more complicated to understand/maintain?
I would suggest that maybe just using nncp as a hard coded group might be better. If I'm incorrect (I don't know anything about nncp, so very possible!) then we should consider following the common pattern in NixOS and only provision the group if the sysadmin leaves the default value of nncp:
users.groups = optionalAttrs (programCfg.group == "nncp") {
nncp = { };
}There was a problem hiding this comment.
I would prefer to hardcode this group, but I would also like to allocate a uid:gid pair, this is a traditional unix style service and we still support those.
| with lib; | ||
|
|
||
| let | ||
| statefulCfgFile = "/etc/nncp.secrets"; |
There was a problem hiding this comment.
Whenever I see stateful and /etc I wonder if there is a different way to do things - a way that better represents nix philosophies.
Maybe we should create a secretsFile option with type = with types; nullOr path; which defaults to null. If the sysadmin provides a file then we use it, otherwise we generate our own and store it in a stateful directory, like /var/lib/nncp/config. If we pair this with LoadCredential I think we have a reasonable chance at running this service with DynamicUser = true;.
There was a problem hiding this comment.
This is complicated because /etc/nncp.hjson must contain private keys and configuration, but nodes can only connnect to each other when the public key is known. It may be better to use a configuration directory.
| "f ${programCfg.settings.log} 0770 root ${programCfg.group}" | ||
| ]; | ||
|
|
||
| system.activationScripts.nncp = '' |
There was a problem hiding this comment.
With my above comments on adding a secretsFile I think we might be able to entirely avoid using an activation script here, which would be ideal.
|
I think we have problems with configuring NNCP that should be dicussed upstream before releasing a NixOS module. Package update moved to #151071. |
Motivation for this change
The NNCP package is out of date and there is no NixOS module.
Some other things:
listenStreamsoption description.Things done
sandbox = trueset 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/)