Merge lp:~freyes/charm-helpers/lp1670223 into lp:charm-helpers

[Felipe Reyes]

Verify if nagios user exists before attempting to run a check

There are cases where a charm may generate its configuration before
nagios-nrpe-server has been installed and attempting to run a check will
cause a failure due that /var/lib/nagios does not exist.

[Paul Gear (paulgear)]

@freyes, is there a reason you're gating this on the user existing rather than the directory existing? It seems to me that just ensuring the directory exists regardless of nagios-nrpe-server's install state would be a more direct solution (assuming that's the only dependency between add_init_service_checks() and the nrpe package).

review: Needs Information

[Ryan Beisner (1chb1n)]

Agree @paulgear, was about to comment similarly. ie. Check that the dir exists before attempting to open a file for write within the dir.

[Ryan Beisner (1chb1n)]

review: Needs Information

[Felipe Reyes (freyes)]

I chose to check the nagios use over the directory only because I was worried about the permissions, but now you mentioned, the cronjob runs as root just like the hook, so checking os.path.isdir('/var/lib/nagios') will do the trick as well and it's more explicit what it's needed.

I'll do the change.

[lp:~freyes/charm-helpers/lp1670223]

710. By Felipe Reyes on 2017-03-08

Check if /var/lib/nagios exists

Instead of checking if the nagios user exists, this patch checks if
/var/lib/nagios directory is available, it makes the check more appropriate
for what it's needed to run the check

711. By Felipe Reyes on 2017-03-08

Updated comments to reflect the new logic of the check

[Felipe Reyes (freyes)]

done, ready for review

[Stuart Bishop (stub)]

Looks good.

review: Approve

=== modified file 'charmhelpers/contrib/charmsupport/nrpe.py'
--- charmhelpers/contrib/charmsupport/nrpe.py	2017-03-02 07:53:12 +0000
+++ charmhelpers/contrib/charmsupport/nrpe.py	2017-03-08 12:44:27 +0000
@@ -227,6 +227,7 @@
     nagios_logdir = '/var/log/nagios'
     nagios_exportdir = '/var/lib/nagios/export'
     nrpe_confdir = '/etc/nagios/nrpe.d'
+    homedir = '/var/lib/nagios'  # home dir provided by nagios-nrpe-server
 
     def __init__(self, hostname=None, primary=True):
         super(NRPE, self).__init__()
@@ -369,7 +370,7 @@
             )
         elif os.path.exists(sysv_init):
             cronpath = '/etc/cron.d/nagios-service-check-%s' % svc
-            checkpath = '/var/lib/nagios/service-check-%s.txt' % svc
+            checkpath = '%s/service-check-%s.txt' % (nrpe.homedir, svc)
             croncmd = (
                 '/usr/local/lib/nagios/plugins/check_exit_status.pl '
                 '-s /etc/init.d/%s status' % svc
@@ -383,7 +384,9 @@
                 description='service check {%s}' % unit_name,
                 check_cmd='check_status_file.py -f %s' % checkpath,
             )
-            if immediate_check:
+            # if /var/lib/nagios doesn't exist open(checkpath, 'w') will fail
+            # (LP: #1670223).
+            if immediate_check and os.path.isdir(nrpe.homedir):
                 f = open(checkpath, 'w')
                 subprocess.call(
                     croncmd.split(),

=== modified file 'tests/contrib/charmsupport/test_nrpe.py'
--- tests/contrib/charmsupport/test_nrpe.py	2017-03-02 08:18:34 +0000
+++ tests/contrib/charmsupport/test_nrpe.py	2017-03-08 12:44:27 +0000
@@ -3,7 +3,7 @@
 import subprocess
 
 from testtools import TestCase
-from mock import patch, call
+from mock import patch, call, MagicMock
 
 from charmhelpers.contrib.charmsupport import nrpe
 from charmhelpers.core import host
@@ -295,7 +295,8 @@
         self.patched['relations_of_type'].return_value = []
         self.assertEqual(nrpe.get_nagios_unit_name(), 'testunit')
 
-    def test_add_init_service_checks(self):
+    @patch.object(os.path, 'isdir')
+    def test_add_init_service_checks(self, mock_isdir):
         def _exists(init_file):
             files = ['/etc/init/apache2.conf',
                      '/usr/lib/nagios/plugins/check_upstart_job',
@@ -309,11 +310,14 @@
 
         self.patched['exists'].side_effect = _exists
 
-        # Test without systemd
+        # Test without systemd and /var/lib/nagios does not exist
         self.patched['init_is_systemd'].return_value = False
+        mock_isdir.return_value = False
         bill = nrpe.NRPE()
         services = ['apache2', 'haproxy']
         nrpe.add_init_service_checks(bill, services, 'testunit')
+        mock_isdir.assert_called_with('/var/lib/nagios')
+        self.patched['call'].assert_not_called()
         expect_cmds = {
             'apache2': '/usr/lib/nagios/plugins/check_upstart_job apache2',
             'haproxy': '/usr/lib/nagios/plugins/check_status_file.py -f '
@@ -324,6 +328,19 @@
         self.assertEqual(bill.checks[1].shortname, 'haproxy')
         self.assertEqual(bill.checks[1].check_cmd, expect_cmds['haproxy'])
 
+        # without systemd and /var/lib/nagios does exist
+        mock_isdir.return_value = True
+        f = MagicMock()
+        self.patched['open'].return_value = f
+        bill = nrpe.NRPE()
+        services = ['apache2', 'haproxy']
+        nrpe.add_init_service_checks(bill, services, 'testunit')
+        mock_isdir.assert_called_with('/var/lib/nagios')
+        self.patched['call'].assert_called_with(
+            ['/usr/local/lib/nagios/plugins/check_exit_status.pl', '-s',
+             '/etc/init.d/haproxy', 'status'], stdout=f,
+            stderr=subprocess.STDOUT)
+
         # Test with systemd
         self.patched['init_is_systemd'].return_value = True
         nrpe.add_init_service_checks(bill, services, 'testunit')