Skip to content
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

Netdata version bump and module permission fix #30550

Closed
wants to merge 2 commits into from

Conversation

cransom
Copy link
Contributor

@cransom cransom commented Oct 18, 2017

Motivation for this change

The version bump is minor, but the module changes allows one of the default plugins to properly read data from /proc to report more process stats. I also added a test to verify the setuid capability.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

I'm all ears if there's a better way to pull in a setuid binary. I tried libredirect but it didn't catch execve and there's no option in netdata itself yet to specify multiple directories for plugins so I could just use /run/wrappers/bin/apps.plugin directly.

Casey Ransom added 2 commits October 17, 2017 19:58
apps.plugin requires capabilities for full process monitoring. Add a
setuid and new derivation to include that data as netdata doesn't
support using multiple plugin directories yet.
echo '#!${pkgs.bash}/bin/bash
exec /run/wrappers/bin/apps.plugin $@
' >$out/libexec/netdata/plugins.d/apps.plugin
chmod 755 $out/libexec/netdata/plugins.d/apps.plugin
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using makeWrapper for this would make it a lot nicer

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd agree that makeWrapper would be better, but I ran into some issues because /run/wrappers/bin/ doesn't exist when this is evaluated and it fails to build. I think sandboxing would also cause this to fail because it's out side of the store.

This is quickly becoming moot though. netdata/netdata#2900 should be available in the next release and I can add /run/wrappers/bin/ to the plugin directory list and it will do the right thing without all the wrapping.

type = types.lines;
default = "";
description = "netdata.conf configuration.";
type = types.nullOr types.str;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason not to keep using types.lines here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, no reason. I didn't realize this got flipped. I'll switch it back.

@cransom
Copy link
Contributor Author

cransom commented Oct 20, 2017

There will be a fix upstream landing (probably) in the next release that will make most of the workarounds here obsolete. When that's out, I'll open a new PR.

@cransom cransom closed this Oct 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants