-
-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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/packetbeat: Add basic module for packetbeat #94862
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.
I have left some comments I hope you find useful. Please don't hesitate to ask if you need any clarification or help on points mentioned.
mkdir -p ${cfg.stateDir}/data | ||
mkdir -p ${cfg.stateDir}/logs | ||
''; | ||
serviceConfig = { |
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.
Please do not run as root
if not required. Maybe DynamicUser
would be appropriate.
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 is completely copied from the official unit, packetbeat needs to run as root because it captures packets from interfaces. I dunno if CAP_NET_ADMIN might be enough instead of having root, but this is how the official systemd unit does it.
''; | ||
}; | ||
|
||
configFlows = mkOption { |
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.
If this is just yaml
does it even need is open option? RFC42 has been promoting a settings
option which I think would benefit this module greatly.
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.
It is just yaml, this was a quick way of getting the module usable. My plan was to first get something usable, then refactor it to be smart too.
''; | ||
}; | ||
|
||
configProtocols = mkOption { |
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.
Would this benefit from a structured type? Maybe type = with types; attrsOf (either (bool (listOf port)));
.
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.
Not impossible, its purely yaml it needs to generate, so a structured type would probably fit. As said in other comments, this was a quick way to getting it usable.
let | ||
cfg = config.services.packetbeat; | ||
|
||
packetbeatYml = pkgs.writeText "packetbeat.yml" '' |
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.
See comment below about a settings
option which could replace this.
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.
I will look into that RFC, I've added the configFile option, that short-circuits the config options in the module, and lets the user handle the configuration file as they please.
83033ce
to
1d33bd3
Compare
A bit of a rough sketch but what about something like this? |
wantedBy = [ "multi-user.target" ]; | ||
preStart = '' | ||
mkdir -p ${cfg.stateDir}/data | ||
mkdir -p ${cfg.stateDir}/logs |
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.
Is it possible to log to stdout
instead so journald
can pick this up?
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.
There is an option to log to stderr (-e) that could be used in ExecStart
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.
There is an option to log to stderr that should make it possible for journald to pick it up (-e).
Well, I've merged your suggestion and fixed it up a bit, however I cannot get it to evaluate. It would seem like there is something wrong with the lib.formats.yaml.generate call, I get the following error message: |
@lejonet I'll try to get back to this in the next few days... |
I'm sorry this took me so long to circle back to. I've pushed another copy which actually builds now... here. Maybe @infinisil wants to take a quick look over this module and offer some helpful advice in general, and specifically for the 2 problematic lines:
|
I had to correct the type from |
Those freeform module options look great! Thanks for taking a look at this module @infinisil. |
Sweet, I'll hopefully have some time this weekend to look at this and merge your changes, so that I can do the testruns of the module. Just as a curious question, what is the actual difference between either and oneOf? Like, semantically, it would seem like they're the same, because both of them choose exactly one of a list of options, but in this case, I guess the implementations might differ in such a way that oneOf works with the freeform module suggestion and either doesn't? |
|
Ah that makes sense, I honestly thought that either was that shorthand, then I know |
I'm getting a weird issue that I hope that you guys can help with, my nix-fu is way too rusty and basic to find the cause of this. With the module in the current state and the following config:
The error I get when I try to deploy the config is:
The traceback isn't all to useful for me, but maybe it is for you, I think its whining about something with the ExecStart value, but that is supposed to be a string, isnt' it?
|
It has not been used by KDE for many years and depends on umaintained libraries we want to drop (Qt4 and Gamin).
They have been broken and disabled for ages and now the dependencies are being removed.
It is insecure and broken. Follow NixOS#93398
it is insecure and broken
fixes at least 17 CVEs also adds test to package
previously it was using the legacy one
ncspot: 0.2.1 -> 0.2.2
plasma5: 5.17.5 -> 5.18.5
doppler: 3.10.1 -> 3.10.3
Motivation for this change
Basic module to enable configuring and enabling packetbeat to send
packet data to elasticsearch. Modeled after the journalbeat service.
We have the packetbeat package in nixpkgs for some time, but no module, so I decided to make a basic one. I decided to make the configuration for flows and protocols their separate config values, so that if all you want to modify is general settings and outputs, you don't have to copy the entire default config and maintain it yourself.
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)