Bug 235006 - net-mgmt/netdata run as user not recognized in the config file
Summary: net-mgmt/netdata run as user not recognized in the config file
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Matthias Andree
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-01-16 19:42 UTC by Dries Michiels
Modified: 2020-04-04 17:27 UTC (History)
2 users (show)

See Also:
mmokhi: maintainer-feedback+


Attachments
netdata rc script overhaul checks user name consistency and kills zombie slaves (2.46 KB, patch)
2020-04-01 16:35 UTC, Matthias Andree
no flags Details | Diff
v2 of netdata rc script overhaul checks user name consistency and kills zombie slaves (3.58 KB, patch)
2020-04-04 09:23 UTC, Matthias Andree
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dries Michiels 2019-01-16 19:42:34 UTC
When specifying the following option in the netdata config file:

[global]
        run as user = root

The daemon wont actually run as root. It needs to be set in rc.conf as netdata_user="root".

Is there a way to support setting this from the config file?
Comment 1 Dries Michiels 2019-01-20 11:06:46 UTC
As a side solution, if we could make it possible to alter the PID file location from a variable in rc.conf, this would already be a nice addition. 

: ${netdata_enable="NO"}
: ${netdata_user="netdata"}
: ${netdata_pid="/var/db/netdata/${name}.pid"}

pidfile = ${netdata_pid}
Comment 2 Mahdi Mokhtari freebsd_committer freebsd_triage 2019-04-06 11:48:40 UTC
I assume it's already landed in ports r496470
If so please tell me so I can close it.

Thanks.
Comment 3 Dries Michiels 2019-04-06 12:04:44 UTC
Hi, this issue is still relevant. Although I'm not sure how we can fix this.
Because the rc script uses daemon and does not probe for the "run as user" in "netdata.conf". If you want to run netdata as another user you have to set the netdata_user variable in rc.conf atm.
Comment 4 Matthias Andree freebsd_committer 2020-04-01 16:35:34 UTC
Created attachment 212936 [details]
netdata rc script overhaul checks user name consistency and kills zombie slaves

Please test if the attached improvement to the rcfile addresses the issues.
Comment 5 Dries Michiels 2020-04-03 11:14:10 UTC
Hi, I just tested this but I was a bit surprised, so when we set "run as user = root" in netdata.conf and set netdata_user="root" in rc.conf it works. But I would expect that only setting "run as user = root" in netdata.conf would be sufficient, but that now fails with:

"/usr/local/etc/rc.d/netdata: ERROR: /usr/local/etc/netdata/netdata.conf sets 'run as user = root', but rc.conf* sets 'netdata]'. Both must be consistent.
/usr/local/etc/rc.d/netdata: WARNING: failed precmd routine for netdata"

So my question is; was this the intend? The user still has to modify rc.conf anyway? Or can we extract the user from netdata.conf and use it for as netdata_user in the rc script unconditionally? It should only throw an error when both are explicitly set and do not match (eg, rc.conf = root, netdata.conf = netdata, or the other way around), but setting either one should be fine I think?

PS: Thanks for doing this! The "run as user" is extracted correctly with SED.
Comment 6 Dries Michiels 2020-04-03 11:15:43 UTC
Oops, it seems that it does not work when both are set to root.

/usr/local/etc/rc.d/netdata: ERROR: /usr/local/etc/netdata/netdata.conf sets 'run as user = root', but rc.conf* sets 'root]'. Both must be consistent.
/usr/local/etc/rc.d/netdata: WARNING: failed precmd routine for netdata

My entries: 
rc.conf:
netdata_user="root"

netdata.conf:
run as user = root
Comment 7 Matthias Andree freebsd_committer 2020-04-03 12:05:35 UTC
Hi Dries,

thanks for testing this, I will rework and test this based on your comments, sorry it doesn't work as intended. I will upload a new version later.
Comment 8 Matthias Andree freebsd_committer 2020-04-04 09:23:11 UTC
Created attachment 213049 [details]
v2 of netdata rc script overhaul checks user name consistency and kills zombie slaves

Hi Dries,

this is a revised version, which should fix the user consistency issues, and also fixes the stop_cmd, which wasn't working properly in the previous patch version.

This patch is standalone, meaning you need to un-apply the previous patch first (patch -R) and then apply this one.

Could you test again, including stopping? Thank you in advance.
Comment 9 Matthias Andree freebsd_committer 2020-04-04 09:59:27 UTC
Mahdi, I've taken the PR from you because of a maintainer timeout. 
If you want to take it back and have the time to deal with it, anytime.
Comment 10 Dries Michiels 2020-04-04 15:08:33 UTC
Looking good!!!! 

I have played around with it for long enough that I can say that it works very nicely. One small issue I discovered when changing all sort of settings is the following. Lets say that you have been running netdata as root "run as user = root", the permission on the pid are as such: 

-rw-r--r--  1 root     wheel     6 Apr  4 16:56 netdata.pid

When now stopping netdata followed by changing the user in netdata.conf to: "run as user = netdata". And starting netdata again yields the following pid-file ownership (it does not get overwritten with netdata owned pid).

-rw-r--r--  1 root     wheel     6 Apr  4 16:56 netdata.pid

When trying to stop it, it sais the following: 
[/var/db/netdata]$ service netdata stop
netdata not running?

Only way to fix was to start netdata with a netdata owned 
PID file or no PID file present so that it get freshly created with correct ownership. This is a real corner case though and nobody should bump into this. Could you verify reproducibility on your machine Mandree? Thanks for working on this!

If we think its better to avoid, we can delete the PID after shutdown.
A code snippet from Emby-server that does this:

stop_postcmd=%%RC_NAME%%_postcmd
	%%RC_NAME%%_postcmd()
	{
	        rm -f ${%%RC_NAME%%_pid}
	}
Comment 11 Matthias Andree freebsd_committer 2020-04-04 15:36:46 UTC
Dries, thanks for thoroughly testing again and catching the PID file cleanup issue that I missed.  I confirm your finding, but for netdata, the solution can be simpler:

#...
stop_postcmd() {
        rm -f "${netdata_pid}"
}
#...
stop_postcmd=stop_postcmd

The latter is because the rc.subr tests for variable names, not functions. This nicely removes the stale pid file on stop.
Comment 12 commit-hook freebsd_committer 2020-04-04 15:42:37 UTC
A commit references this bug:

Author: mandree
Date: Sat Apr  4 15:41:27 UTC 2020
New revision: 530685
URL: https://svnweb.freebsd.org/changeset/ports/530685

Log:
  net-mgmt/netdata rc script overhaul, fix termination, user config

  This script overhaul does the following:
  - Read "run as user" from the netdata configuration file,
    and use that to override the default user "netdata", in case
    it is not set in /etc/rc.conf* and friends.
  - Kill all children of the PID in the netdata_pid file, too,
    because 1.20.0 would leave some plugin processing lingering.
  - Timeout the termination after (configurable) 30 seconds and
    issue SIGKILL
  - Cleanup the netdata_pid file after stop, so that a subsequent
    start with a less privileged user (say, start as root, stop,
    start as netdata) will work properly.
  - Document all variables, including the all-new netdata_stop_maxwait,
    in the header of the script.
  - Quote parameter expansions where appropriate.

  PR:		235006
  Reported by:	Dries Michiels <driesm.michiels@gmail.com>
  Reviewed by:	Dries Michiels <driesm.michiels@gmail.com>
  Approved by:	maintainer timeout (mmohki@, ~ 1 year)

Changes:
  head/net-mgmt/netdata/Makefile
  head/net-mgmt/netdata/files/netdata.in
Comment 13 commit-hook freebsd_committer 2020-04-04 17:27:50 UTC
A commit references this bug:

Author: mandree
Date: Sat Apr  4 17:26:36 UTC 2020
New revision: 530693
URL: https://svnweb.freebsd.org/changeset/ports/530693

Log:
  net-mgmt/netdata: rc script fixes for borderline cases, and use err[1]/warn.

  PR:		235006
  Reported by:	Mateusz Piotrowski (0mp@) [1]

Changes:
  head/net-mgmt/netdata/Makefile
  head/net-mgmt/netdata/files/netdata.in