Merge ~guoqiao/charm-hw-health:LP1906991-chown-output-files into charm-hw-health:master

[Joe Guo]

ensure cron output dir and files are owned by nagios user

Before, nrpe-server and cron jobs are run by root, so legacy output dir and files are
owned by root. Now the user has changed to nagios, which will get permission errors
while writing to legacy dir and files owned by root.

This patch try to ensure the permission is correct.

LP: #1906991

[Joe Guo (guoqiao)]

NOTE:

This patch is only trying to fix current bug with minimal changes.

There is suggestion to create a subdir for each tool, so we can manage files and permissions in an easier way. e.g.:

/var/lib/nagios/ipmi/ipmi_sensors.out
/var/lib/nagios/ipmi/ipmi_exclude

I will submit another patch for that, after this one if merged.

[Linda Guo (lihuiguo)]

LGTM

review: Approve

[Xav Paice (xavpaice)]

Just a small query, I've not run this through tests as yet.

[Joe Guo (guoqiao)]

hi xav:

Yes, CRONJOB_OUTPUT_DIR is '/var/lib/nagios' and you are right, nrpe charm should chown it, which I already did in another patch.

I added this line just because in testing, I noticed if I don't do so, test will fail. So I add it for safe, just in case nrpe charm is not upgraded yet.

I can remove this line, so this patch will rely on nrpe charm to be upgraded first.

[Joe Guo (guoqiao)]

Hi Xav:

I removed that line and added a comment there.
New review appreciated.

[Xav Paice (xavpaice)]

LGTM

review: Approve

diff --git a/src/lib/hwhealth/tools.py b/src/lib/hwhealth/tools.py
index a3a92fa..2ac85c9 100644
--- a/src/lib/hwhealth/tools.py
+++ b/src/lib/hwhealth/tools.py
@@ -195,6 +195,17 @@ class Tool:
 
         return series in cls.SUPPORTED_SERIES
 
+    def chown(self, path, user="nagios", group="nagios"):
+        if os.path.exists(path):
+            shutil.chown(path, user=user, group=group)
+
+    def get_output_files(self):
+        """Which output files this check tool will generate.
+
+        Override in each subclass of Tool.
+        """
+        return []
+
     def _install_cronjob(self, cron_user="root"):
         assert self._cron_script is not None
 
@@ -238,6 +249,12 @@ class Tool:
             crond_fd.write(cronjob_line)
             hookenv.log("Cronjob configured at {}".format(crond_file), hookenv.DEBUG)
 
+        # ensure for cron output files are owned by nagios
+        # NOTE: to write xxx.out.tmp file, dir CRONJOB_OUTPUT_DIR must also be owned
+        # by nagios, which should already be done in nrpe charm.
+        for filepath in self.get_output_files():
+            self.chown(filepath)
+
         return dst
 
     def _remove_cronjob(self):
@@ -643,6 +660,8 @@ class Ipmi(Tool):
     actual nrpe check, which is imported as a git submodule
     """
 
+    # must match cron_ipmi_sensors.py and check_ipmi.py
+    OUTPUT_FILE = "/var/lib/nagios/ipmi_sensors.out"
     EXCLUDE_FILE = "/var/lib/nagios/ipmi_exclude"
 
     def __init__(self, nrpe_opts=""):
@@ -652,6 +671,12 @@ class Ipmi(Tool):
         super().__init__(cron_script="cron_ipmi_sensors.py", cron_script_args=options)
         self._sudoer_file = "99-check_ipmi_sensor"
 
+    def get_output_files(self):
+        return [
+            self.OUTPUT_FILE,
+            self.EXCLUDE_FILE,
+        ]
+
     def set_exclusions(self, cfg_exclude_lines):
         try:
             excluded_lns = yaml.safe_load(cfg_exclude_lines)