Bug 175748 - Review Request: cacti
Review Request: cacti
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Aurelien Bompard
David Lawrence
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2005-12-14 11:06 EST by Mike McGrath
Modified: 2007-11-30 17:11 EST (History)
1 user (show)

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


Attachments (Terms of Use)

  None (edit)
Description Mike McGrath 2005-12-14 11:06:48 EST
Spec Url: http://preview.iesabroad.org/~mmcgrath/cacti/cacti.spec
SRPM Url: http://preview.iesabroad.org/~mmcgrath/cacti/cacti-0.8.6g-2.src.rpm
Description:  
Cacti is a complete frontend to RRDTool. It stores all of the
necessary information to create graphs and populate them with
data in a MySQL database. The frontend is completely PHP
driven. Along with being able to maintain graphs, data
sources, and round robin archives in a database, Cacti also
handles the data gathering. There is SNMP support for those
used to creating traffic graphs with MRTG.

Misc: This is my first package and I would like a sponsor.
Comment 1 Aurelien Bompard 2005-12-17 05:24:51 EST
Needs work:
* Use of buildroot is not consistant
  (wiki: PackagingGuidelines#UsingBuildRootOptFlags)
* No downloadable source. Please give the full URL in the Source tag.
* Group Application/System is not valid, it's "Applications/System" (with an s)
* /usr/share/cacti/rra/.placeholder and the log file are chmod 750 because of
the %attr definition in %files. The files should not be executable, so you
should change the lines in %files to :
%dir %attr(0755,cacti,cacti) %{_datadir}/%{name}/log/
%attr(0644,cacti,cacti) %{_datadir}/%{name}/log/*
%dir %attr(0755,cacti,cacti) %{_datadir}/%{name}/rra/
%attr(0644,cacti,cacti) %{_datadir}/%{name}/rra/.placeholder
* Is is necessary to make the log dir non world-readable ? I don't see important
information there. Plus, the log file has to be readable by the webserver for
the "View Log" function to work (in "System Utilities"). In the above example,
I've set the dir 755 and the log file 644.
* This is not FHS complient :
 - move include/config.php in /etc/cacti/config.php
 - move log/ to /var/log/cacti
 - move rra to /var/lib/cacti/rra/
And symlink the files back in /usr/share/cacti
I would also move scripts/ to /var/lib/cacti/scripts, since it is very common
for cacti users to add scripts. What do you think ?
* In %prep, use %setup -q to avoid file listing
* Please add a logrotate file for the log. Example:
/var/log/cacti/cacti.log {
    monthly
    notifempty
    missingok
    compress
    create 0644 cacti cacti
}
* File listed twice: /usr/share/cacti/include/config.php
* Missing dependancy on /sbin/service for %postun (package initscripts)
* /usr/bin should be changed to %{_bindir}
* Why "cp -avx" ? cp -a is enough
* Don't hardcode uid 329 in useradd, it may be already used. Just let useradd
find an available uid
* You need mailx and MTA as Requires(post) because of the mail in %post
* Changed Prereq to Requires(pre)
* No need for php in %pre, you can remove it from Requires(pre)
Comment 2 Mike McGrath 2005-12-17 13:25:05 EST
I don't disagree with any of these.  I'll post the changes soon.  I guess I was
being a little loverly parrinoid about the log files.  Going through my own I
haven't seen anything too important.
Comment 3 Mike McGrath 2005-12-18 00:59:41 EST
I've made these updates.  Couple of issues:

Simply running config.php out of /etc/ does not work as there are includes that
fail, and copying the entire include directory seems inapproperate.  I could
write a patch?  In the meantime I made a link from the config.php to
/etc/cacti/config.php so it can at least be found there. Any thoughts?

I'm also curious about others thoughts on running this as a Cacti user.  Would
it be more approperate to just run it as Apache?

Either way the spec and srpm are both updated and in their original spots:

Spec Url: http://preview.iesabroad.org/~mmcgrath/cacti/cacti.spec
SRPM Url: http://preview.iesabroad.org/~mmcgrath/cacti/cacti-0.8.6g-2.src.rpm
Comment 4 Warren Togami 2005-12-18 01:07:11 EST
When you update a SRPM, please always increment the Release number, do not keep
it the same.
Comment 5 Aurelien Bompard 2005-12-18 10:20:12 EST
> Simply running config.php out of /etc/ does not work as there are includes 
> that fail, and copying the entire include directory seems inapproperate.  I 
> could write a patch?

If it's very easy and obvious, yes, write a patch please. Users should not have
to edit configuration files in /usr/share

> I'm also curious about others thoughts on running this as a Cacti user.  Would
> it be more approperate to just run it as Apache?

You mean the cron job ? I think it's best not to run it as the apache user, so
that the apache user does not have write access to the RRA files in case of
compromise (and there have been such vulnerabilities in cacti's history)

* in %pre : /usr/sbin -> %{_sbindir}
* /var -> %{_localstatedir} in the logrotate file
* mailx and MTA should be in Requires(post)
* /sbin/service should be in Requires(postun)
* Using %doc in %files does the copying of files, so you don't have to do it in
%install

Minor:
* the scripts don't have to be owned by the cacti user, but that's not a problem
(it just spits out more warnings in rpmlint)
* you don't _have_ to use %{__install}, %{__mkdir}, %{__cp} and %{__chmod},
using install, mkdir, etc... is enough. Do what you prefer, it's up to you.
Comment 6 Mike McGrath 2005-12-18 12:23:13 EST
Spec Url: http://preview.iesabroad.org/~mmcgrath/cacti/cacti.spec
SRPM Url: http://preview.iesabroad.org/~mmcgrath/cacti/cacti-0.8.6g-3.noarch.rpm

I've updated many of those changes (including minor).  I ended up taking the
mail out of the RPM all together, I don't remember getting emails from other
packages saying "hey configure me here"

I've taken the database config portion out of the config.php file and placed it
in /etc/cacti/db.php  It seems like nothing else in config.php would need to be
edited for normal use.  

Thanks for all your help on this, I plan on submitting more packages in the
future so feel free to straighten me out on things that are not best practice.
Comment 7 Aurelien Bompard 2005-12-18 13:03:46 EST
> I ended up taking the mail out of the RPM all together, I don't
> remember getting emails from other packages saying "hey configure me here"

Agreed. Seemed "unnatural" to me too.

> I've taken the database config portion out of the config.php file and placed
> it in /etc/cacti/db.php  It seems like nothing else in config.php would need
> to be edited for normal use.

Very nice. There are a few additional config vars for some HTML colors in
config.php, but we can ignore this. Actually, I wish upstream had splitted the
db config out of config.php. Could you submit your patch upstream (with the
/etc/cacti/db.php paths replaced by include/db.php) ?

A few last things :
* remove /sbin/service from Requires (it's already in Requires(postun))
* in %install, first you %__install the .placeholder file, then you empty it
  with echo. Why ? I think that is file is here just to prevent winzip from not
  including the rra dir, so you should not bother about it (I think you can even
  remove it entirely).
Comment 8 Mike McGrath 2005-12-18 16:41:07 EST
These changes are ready, I've submitted an include/db.php patch to the Cacti
team.  Also while looking for their bug system I found 4 patches for 0.8.6g and
have included them in this release.  

Spec Url: http://preview.iesabroad.org/~mmcgrath/cacti/cacti.spec
SRPM Url: http://preview.iesabroad.org/~mmcgrath/cacti/cacti-0.8.6g-4.src.rpm

Comment 9 Aurelien Bompard 2005-12-19 02:54:44 EST
* in the cron job, replace /usr/bin by %{_bindir}
* to make it clear that the patches come from upstream, please give the full url
  (ex:
http://www.cacti.net/downloads/patches/0.8.6g/short_open_tag_parse_error.patch)
  They won't start with "cacti-0.8.6g-", but it is more important to be able to
  verify them against upstream.
* drop the empty %build section
* concerning the apache restart in the scriptlets :
  1. we want to condrestart apache on install, in order for our new cacti.conf
     to be taken into account.
  2. we want to condrestart apache on upgrade, in case we changed cacti.conf
  3. we want to condrestart apache on uninstall, so that apache sees that the
     cacti.conf file has been removed.
  For 1 we need a post scriptlet like this :
%post
if [ $1 == 1 ]; then
  /sbin/service httpd condrestart >/dev/null 2>&1
fi
  We can combine 2 and 3 into a simple %postun scriptlet without tests :
%postun
/sbin/service httpd condrestart >/dev/null 2>&1
  See http://fedoraproject.org/wiki/ScriptletSnippets for more info on which
  tests to run

We are having a larger problem now. SELinux blocks httpd from accessing /var/log
and /var/lib. The RRAs are correctly created by the poller script, but can't be
seen in the web interface. I'm posting in fedora-selinux-list to get advice.
Comment 10 Mike McGrath 2005-12-19 11:08:44 EST
I've got these changes in.  I'll wait to post them until we can figure out what
to do about SELinux.  I'll be monitoring the thread:

https://www.redhat.com/archives/fedora-selinux-list/2005-December/msg00124.html
Comment 11 Mike McGrath 2005-12-21 11:16:27 EST
Spec Url: http://preview.iesabroad.org/~mmcgrath/cacti/cacti.spec
SRPM Url: http://preview.iesabroad.org/~mmcgrath/cacti/cacti-0.8.6g-6.src.rpm

I've made the changes above and used the "quick hack" from the SELinux thread. 
I'm sure there's got to be a more approperate way to do this.  Any ideas anyone?

Comment 12 Mike McGrath 2006-01-04 19:44:10 EST
Welp, selinux and FHS don't always get along :-D  I've moved all the log files
and associated web files to /var/www/cacti/ until we come up with a better way
to do it.

SPEC: http://mmcgrath.net/~mmcgrath/cacti/cacti.spec
SRPM: http://mmcgrath.net/~mmcgrath/cacti/cacti-0.8.6g-7.src.rpm
Comment 13 Mike McGrath 2006-01-17 18:33:25 EST
SPEC: http://mmcgrath.net/~mmcgrath/cacti/cacti.spec
SRPM: http://mmcgrath.net/~mmcgrath/cacti/cacti-0.8.6h-3.src.rpm

Changes:
-Moved log files back to /var/log/cacti
-Moved Scripts back to /var/lib/cacti/scripts
-Moved rra files back to /var/lib/cacti/rra
-Changed logfile to (664,cacti,apache) so the "clear log file" will work
-Upstream had a new version come out
-Upstream already had 4 patches for that version

SELinux suggestion made by Paul Howarth sounds very reasonable:
https://www.redhat.com/archives/fedora-extras-list/2006-January/msg01169.html

If Cacti gets approved I'll submit a bug report to SELinux and try to get it
incorperated into the policy.  I'll use the FC5 modules once FC5 comes out (if
it applies)
Comment 14 Aurelien Bompard 2006-02-04 05:14:28 EST
Sorry for not getting back earlier. In you last spec file, seem to have moved
files back to /usr/share and /var/lib.
In order to avoid the SELinux problem, I think we should keep everything in
/var/www, as you said in comment 12.
Comment 15 Mike McGrath 2006-02-04 10:16:18 EST
I can move it back but do you see any reason Paul's suggestion won't work? 
(From Comment 13)

https://www.redhat.com/archives/fedora-extras-list/2006-January/msg01169.html
Comment 16 Aurelien Bompard 2006-02-06 10:19:01 EST
OK, it looks good. Minor improvements:
- add " || : " at the end of the scriptlets to make sure they always return 0
- the logrotate file contains a "//" in the logfile path
- please add a README.Fedora with something like that :
If you have SELinux enabled, you have to run the following commands :
chcon -R -t httpd_sys_content_t /var/log/cacti/
chcon -R -t httpd_sys_content_t /var/lib/cacti/rra/

This README could be removed when cacti is added into the policy.

If you agree with the above, make the changes and I'll approve it
Comment 17 Mike McGrath 2006-02-06 10:47:19 EST
Sounds good to me, all things added:

%changelog
* Mon Feb 6 2006 Mike McGrath <imlinux@gmail.com> - 0.8.6h-4
- Fixed some scriptlets to always return 0
- Fixed extra '/' in logrotate
- Added README.Fedora

SPEC: http://mmcgrath.net/~mmcgrath/cacti/cacti.spec
SRPM: http://mmcgrath.net/~mmcgrath/cacti/cacti-0.8.6h-4.src.rpm
Comment 18 Aurelien Bompard 2006-02-06 17:38:20 EST
Review for release 4:
* RPM name is OK
* Source cacti-0.8.6h.tar.gz is the same as upstream
* This is the latest version
* Builds fine in mock
* rpmlint looks OK
* File list looks OK
* Works fine
APPROVED

Again, sorry for keeping you waiting.

Please post here the bug number for SELinux integration when you file it.
Comment 19 Mike McGrath 2006-02-08 10:30:26 EST
I've created an SELinux bug: 

https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=180482

Also cacti is now available in devel.  (Soon FC3 and FC4)

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