nagios-charmers team mailing list archive

[Alok G Singh <alok.singh@xxxxxxxxxxxxx>]

One minor comment inline.

Re the input parameter sanitisation, FTR I think the benefits outweigh the risks. Being able to easily effect changes in the parameters is essential to manage alert fatigue.

Diff comments:

> diff --git a/src/lib/hwhealth/tools.py b/src/lib/hwhealth/tools.py
> index 4b967c2..bc48e7b 100644
> --- a/src/lib/hwhealth/tools.py
> +++ b/src/lib/hwhealth/tools.py
> @@ -101,6 +101,54 @@ class Tool():
>          self._remove_nrpe_plugin()
>          hookenv.log('Removed tool [{}]'.format(self._shortname))
>  
> +    def _install_cronjob(self, cron_user='root'):
> +        assert self._cronjob_script is not None
> +
> +        # Copy the cronjob script to the nagios plugins directory
> +        src = os.path.join(self._files_dir, self._cronjob_script)
> +        dst = shutil.copy(src, self.NRPE_PLUGINS_DIR)
> +        os.chmod(dst, self.CRONJOB_SCRIPT_MODE)
> +        os.chown(dst, uid=self.CRONJOB_SCRIPT_UID, gid=self.CRONJOB_SCRIPT_GID)
> +        hookenv.log(
> +            'Cronjob script [{}] copied to {}'
> +            ''.format(self._cronjob_script, self.NRPE_PLUGINS_DIR),
> +            hookenv.DEBUG
> +        )
> +
> +        cmdline = [dst]
> +        if self._cronjob_script_args and isinstance(self._cronjob_script_args, str):
> +            cmdline.extend(self._cronjob_script_args.split())
> +        else:
> +            # Run it once to generate the temp file, otherwise the nrpe check might
> +            # fail at first. For security reasons, cronjobs that allow parameters
> +            # shared by the user don't run the script as root (check_ipmi allows a
> +            # missing output file)
> +            subprocess.call(cmdline)
> +
> +        # Now generate the cronjob file
> +        cronjob_line = '*/5 * * * * {user} {cmd}\n'.format(user=cron_user, cmd=' '.join(cmdline))

I offer this snippet (from userdir-ldap) which randomises the cron times. 

def cronsplay(string, interval=5):
    offsets = []
    o = binascii.crc_hqx(string, 0) % interval
    while o < 60:
        offsets.append(str(o))
        o += interval
    return ','.join(offsets)

> +        crond_file = os.path.join(self.CROND_DIR, 'hwhealth_{}'.format(self._shortname))
> +        with open(crond_file, 'w') as crond_fd:
> +            crond_fd.write(cronjob_line)
> +            hookenv.log(
> +                'Cronjob configured at {}'.format(crond_file),
> +                hookenv.DEBUG
> +            )
> +        return dst
> +
> +    def _remove_cronjob(self):
> +        assert self._cronjob_script is not None
> +
> +        crond_file = os.path.join(self.CROND_DIR, 'hwhealth_{}'.format(self._shortname))
> +        cronjob_script = os.path.join(self.NRPE_PLUGINS_DIR, self._cronjob_script)
> +        os.remove(crond_file)
> +        os.remove(cronjob_script)
> +        hookenv.log(
> +            'Removed cronjob files [{}, {}]'
> +            ''.format(crond_file, cronjob_script),
> +            hookenv.DEBUG
> +        )
> +
>  
>  class VendorTool(Tool):
>      """A class representing a vendor tool binary that is shipped via resource


-- 
https://code.launchpad.net/~aluria/hw-health-charm/+git/hw-health-charm/+merge/365060
Your team Nagios Charm developers is requested to review the proposed merge of ~aluria/hw-health-charm/+git/hw-health-charm:fix-check-ipmi into hw-health-charm:master.