Bug 1059281 - Review Request: monitoring-plugins - Host/service/network monitoring program plugins for a variety of different systems
Review Request: monitoring-plugins - Host/service/network monitoring program ...
Status: NEW
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Michael Scherer
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 1054340
  Show dependency treegraph
 
Reported: 2014-01-29 10:03 EST by Sam Kottler
Modified: 2014-12-05 17:53 EST (History)
8 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed:
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)
Comment (69.78 KB, text/plain)
2014-03-20 04:00 EDT, Michael Scherer
no flags Details

  None (edit)
Description Sam Kottler 2014-01-29 10:03:09 EST
Spec URL: http://skottler.fedorapeople.org/reviews/monitoring-plugins/monitoring-plugins.spec
SRPM URL: http://skottler.fedorapeople.org/reviews/monitoring-plugins/monitoring-plugins-1.5-3.fc21.src.rpm
Description: A bundle of more than fifty standard plugins for Icinga, Naemon, Nagios, Shinken, Sensu, and other monitoring applications. Each plugin is a stand-alone command line tool that provides a specific type of check. Typically, your monitoring software runs these plugins to determine the current status of hosts and services on your network.
Fedora Account System Username: skottler

This review is mostly a rename from the nagios-plugins package. The reasoning behind the divergence between the nagios-plugins and monitoring-plugins (this review) packages is available here: https://bugzilla.redhat.com/show_bug.cgi?id=1054340.

Thanks for taking the time to review!
Comment 1 Michael Scherer 2014-01-29 17:28:51 EST
So :
- the list on Requires on 1 single line is a bit too long for my taste, 1 requires per line would likely be better in diff, etc. And it would also be better to have precise requires ( ie, on the specific version )

- the requires on the -log subpackage :
Requires: /bin/egrep

any reason to not use %{_bindir}/ like the rest ?

- description could be enhanced to say this provides more than plugin for nagios 

- %{_libdir}/monitoring/ is unowned

- the various setuid plugin are not a good ideas IMHO, not sure if that requires FESCO approval or something. I would feel better if this was using capacity.

- I see a few tests for the plugins and the lib, yet no %check, is this normal ?

- there is a few tarballs of perl source code, but this is not used for now, just mentioning for documenting it

- why do all plugin provides monitoring-plugins ? ( and not a requires ? )
Comment 2 Sam Kottler 2014-02-12 09:48:18 EST
(In reply to Michael Scherer from comment #1)
> So :
> - the list on Requires on 1 single line is a bit too long for my taste, 1
> requires per line would likely be better in diff, etc. And it would also be
> better to have precise requires ( ie, on the specific version )
> 
> - the requires on the -log subpackage :
> Requires: /bin/egrep
> 
> any reason to not use %{_bindir}/ like the rest ?

Done.

> 
> - description could be enhanced to say this provides more than plugin for
> nagios 

Fixed.

> 
> - %{_libdir}/monitoring/ is unowned

Fixed.

> 
> - the various setuid plugin are not a good ideas IMHO, not sure if that
> requires FESCO approval or something. I would feel better if this was using
> capacity.

I can start working on that upstream. There are lots of other packages that use setuid, though; does this block the review?

> 
> - I see a few tests for the plugins and the lib, yet no %check, is this
> normal ?

The tests require access to different services, like an http server or an ntp peer; they'll never pass inside a buildroot.

> 
> - there is a few tarballs of perl source code, but this is not used for now,
> just mentioning for documenting it
> 
> - why do all plugin provides monitoring-plugins ? ( and not a requires ? )

Fixed.

I've uploaded a new srpm here: http://skottler.fedorapeople.org/monitoring-plugins-1.5-4.fc21.src.rpm. The spec is also updated, but in the same location.
Comment 3 Michael Scherer 2014-02-12 11:01:03 EST
I am gonna ask for setuid, since we had a feature for that ( https://fedoraproject.org/wiki/Features/RemoveSETUID ). I do not think this would block the review, but I prefer to be sure. And to be honest, i do not see that as being hard to fix. Most of them just need the capability for raw socket.

For the tests, if they need something, ok, this would be hard to do.

I will update the review later.
Comment 4 Michael Scherer 2014-02-12 11:01:46 EST
from #fedora-devel

16:55:46|  misc> is setuid packages required to be checked by FESCO or something ( 
                 https://bugzilla.redhat.com/show_bug.cgi?id=1059281 ) ?
16:55:56|  misc> ( as we try to be setuid-free )

16:59:39|  sgallagh> misc: Yes, any setuid root packages have to be approved.
17:00:49|  sgallagh> (of course, setuid non-root is acceptable, since it's non-privileged)

so Fesco time ?
Comment 5 Sam Kottler 2014-02-18 14:58:19 EST
@misc, what's the way forward with this? I've already gotten a _hardened_build and sgallagher wasn't sure about whether or not there's a document about setuid exceptions.
Comment 6 Michael Scherer 2014-02-19 02:44:29 EST
I would ask to Fesco to be sure, but i will in the mean time still continue the review.
Comment 7 Michael Scherer 2014-03-20 04:00:42 EDT
Created attachment 915872 [details]
Comment

(This comment was longer than 65,535 characters and has been moved to an attachment by Red Hat Bugzilla).

Note You need to log in before you can comment on or make changes to this bug.