prometheus-collectd-exporter service: init module #29212
Conversation
|
@bachp, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edolstra, @bjornfor and @offlinehacker to be potential reviewers. |
| }; | ||
|
|
||
| config = mkIf cfg.enable { | ||
| networking.firewall.allowedTCPPorts = [ (optional cfg.openFirewall cfg.port) (optional (cfg.openFirewall && cfg.collectdBinary.enable) cfg.collectdBinary.port) ]; |
There was a problem hiding this comment.
Throws an error if openFirewall is false (default):
error: The option value `networking.firewall.allowedTCPPorts.[definition 1-entry 1]' in `/home/joerg/git/nixpkgs/nixos/modules/services/monitoring/prometheus/collectd-exporter.nix' is not a integer
There was a problem hiding this comment.
@Mic92 Do you know the correct way to enable the second port based on two conditions? I couldn't find any example.
There was a problem hiding this comment.
untested:
networking.firewall.allowedTCPPorts = (optional cfg.openFirewall cfg.port) ++
(optional (cfg.openFirewall && cfg.collectdBinary.enable) cfg.collectdBinary.port);There was a problem hiding this comment.
I will give it a try. I now also figured out why I didn't catch the issue. I have the firewall disabled on my dev machine.
There was a problem hiding this comment.
@Mic92 Your suggestions works. But I was wondering if it would make more sense to have two firewall open rules one for the binary and one for the scrape/json. Maybe people only want to open one.
There was a problem hiding this comment.
I would keep it simple, you can always use networking.firewall.allowedTCPPorts = [ ] yourself, if you want it more granular.
bachp
force-pushed
the
prometheus-collectd
branch
from
fb5f8ae
to
ae44260
Compare
|
Firewall handling fixed |
| unitConfig.Documentation = "https://github.com/prometheus/collectd_exporter"; | ||
| wantedBy = [ "multi-user.target" ]; | ||
| serviceConfig = { | ||
| User = "nobody"; |
There was a problem hiding this comment.
How about using DynamicUser , which we have in systemd since some versions?
There was a problem hiding this comment.
I didn't know about that. I just copied it from the other collectors. A quick tests shows it seems to work. I updated the PR accordingly.
There was a problem hiding this comment.
Seems like this would be the first module in nixpkgs using DynamicUser.
There was a problem hiding this comment.
Many services have persistent data, so that DynamicUser is not applicable. Also DynamicUser is still not well known and new.
Supports JSON and binary (optional) protocol of collectd.
bachp
force-pushed
the
prometheus-collectd
branch
from
ae44260
to
c719804
Compare
|
@Mic92 What's the difference between submodule and just a new attrSet as you used? |
|
The submodule version did not evaluate on my system |
* prometheus-collectd-exporter service: init module Supports JSON and binary (optional) protocol of collectd. * nixos/prometheus-collectd-exporter: submodule is not needed for collectdBinary (cherry picked from commit 334e23d)
Motivation for this change
Allow easy configuration of the collectd exporter
Supports JSON and binary (optional) protocol
of collectd.
Things done
build-use-sandboxinnix.confon non-NixOS)nix-shell -p nox --run "nox-review wip"./result/bin/)