Merge ~mertkirpici/charm-hw-health:lp/1979587 into charm-hw-health:master

[Mert Kirpici]

Close LP #1979587

The ilorest ELF includes several shared objects that are unpacked to the tempdir during runtime. This causes issues in cases where the tempdir(usually /tmp) is mounted with the mount option 'noexec'. e.g. CIS hardening.

The introduced _run_ilorest private wrapper function is introduced to accomodate this by providing the binary with an alternative tempdir that it can use.

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

PASSED: Continuous integration, rev:f6a72e9b0cad7bc46d2ecddfa41d586b862b4c3a
https://jenkins.canonical.com/bootstack/job/lp-charm-hw-health-ci/70/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-unit/310/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/1515/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-hw-health-ci/70//rebuild

review: Approve (continuous-integration)

[Robert Gildein (rgildein)]

LGTM

review: Approve

[Martin Kalcok (martin-kalcok)]

Overall LGTM, thanks. Just one question, is the hardcoded TMP dir guaranteed to exist?

Also one suggestion, rather than asserting that specific command was executed in each "test_version_getter_*" tests, I think I'd be better to assert that the wrapper (`_run_ilorest()`) was called and then have specific unit test for wrapper. It'll be easier in the future to update unit tests in case the wrapper changes internally

[Eric Chen (eric-chen)]

review: Approve

[Erhan Sunar (esunar)]

review: Approve

[Mert Kirpici (mertkirpici)]

Thanks for the reviews everyone!
I want to apply Martin's suggestion and push an update to the tests before merging, so I would appreciate if we wait for that update and then merge this.

> Overall LGTM, thanks. Just one question, is the hardcoded TMP dir guaranteed to exist?

Yes this is part of the previous implementation, we are taking care of this in the install handler.
https://git.launchpad.net/~mertkirpici/charm-hw-health/tree/src/reactive/hw_health.py#n39

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

PASSED: Continuous integration, rev:16f70181a08caab9e015cac327963a005713bfb1
https://jenkins.canonical.com/bootstack/job/lp-charm-hw-health-ci/71/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-unit/311/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/1518/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-hw-health-ci/71//rebuild

review: Approve (continuous-integration)

[Mert Kirpici (mertkirpici)]

pushed the update that implements a new test for the introduced function and adapting the existing tests to match the source code. ci passed. we are ready to merge this.

[Martin Kalcok (martin-kalcok)]

Thanks, +1

review: Approve

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

Change successfully merged at revision 27c38cd9129285de453b12b287fdf818d439d0d1

diff --git a/src/files/ilorest/cron_ilorest.py b/src/files/ilorest/cron_ilorest.py
index 737ae7a..08eb1c7 100755
--- a/src/files/ilorest/cron_ilorest.py
+++ b/src/files/ilorest/cron_ilorest.py
@@ -46,6 +46,7 @@ class CronILOrest:
         """Parse CLI args during instance creation."""
         self.args = self.parse_args(argv)
         self._ilorest_version = None
+        self._ilorest_binary = "/usr/sbin/ilorest"
 
     @property
     def ilorest_version(self):
@@ -60,13 +61,15 @@ class CronILOrest:
         if self._ilorest_version is not None:
             return self._ilorest_version
 
-        version_cmd = ["/usr/sbin/ilorest", "--version"]
+        ilorest_args = ["--version"]
         try:
-            command_output = check_output(version_cmd, universal_newlines=True)
+            command_output = self._run_ilorest(*ilorest_args)
             version_string = command_output.rsplit(maxsplit=1)
             if not version_string:
                 print(
-                    "Command '%s' did not return any value." % version_cmd,
+                    "Command '{}' did not return any value.".format(
+                        [self._ilorest_binary, *ilorest_args]
+                    ),
                     file=sys.stderr,
                 )
                 self._ilorest_version = None
@@ -136,17 +139,32 @@ class CronILOrest:
 
         return self._walk_selector(jsondata, [selector])
 
+    def _run_ilorest(self, *args):
+        """Run ilorest executable and return stdout string.
+
+        The ilorest ELF includes several shared objects that are unpacked
+        to the tempdir during runtime. This causes issues in cases where
+        the tempdir(usually /tmp) is mounted with the mount option 'noexec'.
+        This wrapper function is essentially in place to accomodate this
+        by providing the binary with an alternative tempdir that it can use.
+
+        See: Launchpad #1935032, #1979587
+        """
+        return check_output(
+            [self._ilorest_binary, *args],
+            env={**os.environ, "TMP": "/var/lib/hw-health"},
+            universal_newlines=True,
+        )
+
     def _get_json_ilorest_output(self, selector, version):
-        cmd = ["/usr/sbin/ilorest", "list", "-j", "--selector={}".format(selector)]
+        ilorest_args = ["list", "-j", "--selector={}".format(selector)]
         if int(version[0]) >= 3:
             # ilorest above version 3.0 (included) has '--refresh' flag and
             # should use it to force fetching new data instead of using cached
             # values
-            cmd.append("--refresh")
+            ilorest_args.append("--refresh")
 
-        return check_output(  # LP#1935032 fix for noexec /tmp
-            cmd, env={**os.environ, "TMP": "/var/lib/hw-health"}
-        ).decode("UTF-8")
+        return self._run_ilorest(*ilorest_args)
 
     def _get_health_status_message(self, json_item, crumb_trail=[]):
         desc = json_item["Name"]
diff --git a/src/tests/unit/test_cron_ilorest.py b/src/tests/unit/test_cron_ilorest.py
index 796c76e..d9f29bf 100644
--- a/src/tests/unit/test_cron_ilorest.py
+++ b/src/tests/unit/test_cron_ilorest.py
@@ -50,56 +50,59 @@ class TestCronILOrest(unittest.TestCase):
             errors = cronilorest.check_selector(selector)
             assert errors != []
 
-    @mock.patch.object(cron_ilorest, "check_output")
-    def test_version_getter_success(self, check_output_mock):
+    @mock.patch.object(cron_ilorest.CronILOrest, "_run_ilorest")
+    def test_version_getter_success(self, run_ilorest_mock):
         """Test getting and parsing iloret version string."""
         version_string = "RESTful Interface Tool 3.5.0.0"
         expected_version = "3.5.0.0"
-        check_output_mock.return_value = version_string
+        run_ilorest_mock.return_value = version_string
 
         cronilorest = cron_ilorest.CronILOrest()
 
         self.assertEqual(cronilorest.ilorest_version, expected_version)
+        run_ilorest_mock.assert_called_once_with("--version")
 
     @mock.patch("builtins.print")
-    @mock.patch.object(cron_ilorest, "check_output")
-    def test_version_getter_no_value(self, check_output_mock, print_mock):
+    @mock.patch.object(cron_ilorest.CronILOrest, "_run_ilorest")
+    def test_version_getter_no_value(self, run_ilorest_mock, print_mock):
         """Test handling when ilorest version command does not return value."""
         version_string = ""
         expected_version = None
         expected_msg = (
             "Command '['/usr/sbin/ilorest', '--version']' did not" " return any value."
         )
-        check_output_mock.return_value = version_string
+        run_ilorest_mock.return_value = version_string
 
         cronilorest = cron_ilorest.CronILOrest()
 
         self.assertEqual(cronilorest.ilorest_version, expected_version)
         print_mock.assert_called_once_with(expected_msg, file=sys.stderr)
+        run_ilorest_mock.assert_called_once_with("--version")
 
     @mock.patch("builtins.print")
-    @mock.patch.object(cron_ilorest, "check_output")
-    def test_version_getter_invalid_value(self, check_output_mock, print_mock):
+    @mock.patch.object(cron_ilorest.CronILOrest, "_run_ilorest")
+    def test_version_getter_invalid_value(self, run_ilorest_mock, print_mock):
         """Test handling when ilorest version command returns unexpected value."""
         bad_version = "3.5.0.a"
         version_string = "RESTful Interface Tool " + bad_version
         expected_version = None
         expected_msg = "Failed to parse ilorest version: '%s'" % bad_version
-        check_output_mock.return_value = version_string
+        run_ilorest_mock.return_value = version_string
 
         cronilorest = cron_ilorest.CronILOrest()
 
         self.assertEqual(cronilorest.ilorest_version, expected_version)
         print_mock.assert_called_once_with(expected_msg, file=sys.stderr)
+        run_ilorest_mock.assert_called_once_with("--version")
 
     @mock.patch("builtins.print")
-    @mock.patch.object(cron_ilorest, "check_output")
-    def test_version_getter_cmd_fail(self, check_output_mock, print_mock):
+    @mock.patch.object(cron_ilorest.CronILOrest, "_run_ilorest")
+    def test_version_getter_cmd_fail(self, run_ilorest_mock, print_mock):
         """Test handling when ilorest version command returns non-zero code."""
         error_msg = "Ilorest command failed"
         expected_version = None
         expected_msg = "Failed to get ilorest version: %s" % error_msg
-        check_output_mock.side_effect = subprocess.CalledProcessError(
+        run_ilorest_mock.side_effect = subprocess.CalledProcessError(
             output=error_msg, cmd="/usr/sbin/ilorest --version", returncode=1
         )
 
@@ -107,9 +110,10 @@ class TestCronILOrest(unittest.TestCase):
 
         self.assertEqual(cronilorest.ilorest_version, expected_version)
         print_mock.assert_called_once_with(expected_msg, file=sys.stderr)
+        run_ilorest_mock.assert_called_once_with("--version")
 
-    @mock.patch.object(cron_ilorest, "check_output")
-    def test_get_json_ilorest_output_v3(self, check_output_mock):
+    @mock.patch.object(cron_ilorest.CronILOrest, "_run_ilorest")
+    def test_get_json_ilorest_output_v3(self, run_ilorest_mock):
         """Test that _get_json_ilorest_output runs correct command for version 3.
 
         ilorest tool got new argument `--refresh` in version 3.0.0 that forces
@@ -117,27 +121,24 @@ class TestCronILOrest(unittest.TestCase):
         """
         selector = "Chassis"
         version = "3.0.0"
-        expected_cmd = [
-            "/usr/sbin/ilorest",
+        expected_args = [
             "list",
             "-j",
             "--selector=%s" % selector,
             "--refresh",
         ]
-        expected_output = b"Chassis status"
+        expected_output = "Chassis status"
 
-        check_output_mock.return_value = expected_output
+        run_ilorest_mock.return_value = expected_output
 
         cronilorest = cron_ilorest.CronILOrest()
         result = cronilorest._get_json_ilorest_output(selector, version)
 
-        self.assertEqual(result, expected_output.decode("UTF-8"))
-        check_output_mock.assert_called_once_with(
-            expected_cmd, env={**os.environ, "TMP": "/var/lib/hw-health"}
-        )
+        self.assertEqual(result, expected_output)
+        run_ilorest_mock.assert_called_once_with(*expected_args)
 
-    @mock.patch.object(cron_ilorest, "check_output")
-    def test_get_json_ilorest_output_v2(self, check_output_mock):
+    @mock.patch.object(cron_ilorest.CronILOrest, "_run_ilorest")
+    def test_get_json_ilorest_output_v2(self, run_ilorest_mock):
         """Test that _get_json_ilorest_output runs correct command for version 2.
 
         ilorest tool does not have an argument `--refresh` in versions below 3.0.0
@@ -145,15 +146,24 @@ class TestCronILOrest(unittest.TestCase):
         """
         selector = "Chassis"
         version = "2.5.1"
-        expected_cmd = ["/usr/sbin/ilorest", "list", "-j", "--selector=%s" % selector]
-        expected_output = b"Chassis status"
+        expected_args = ["list", "-j", "--selector=%s" % selector]
+        expected_output = "Chassis status"
 
-        check_output_mock.return_value = expected_output
+        run_ilorest_mock.return_value = expected_output
 
         cronilorest = cron_ilorest.CronILOrest()
         result = cronilorest._get_json_ilorest_output(selector, version)
 
-        self.assertEqual(result, expected_output.decode("UTF-8"))
+        self.assertEqual(result, expected_output)
+        run_ilorest_mock.assert_called_once_with(*expected_args)
+
+    @mock.patch.object(cron_ilorest, "check_output")
+    def test_run_ilorest(self, check_output_mock):
+        args = ["foo", "bar"]
+        cron_ilorest.CronILOrest()._run_ilorest(*args)
+
         check_output_mock.assert_called_once_with(
-            expected_cmd, env={**os.environ, "TMP": "/var/lib/hw-health"}
+            ["/usr/sbin/ilorest", *args],
+            env={**os.environ, "TMP": "/var/lib/hw-health"},
+            universal_newlines=True,
         )
diff --git a/src/tox.ini b/src/tox.ini
index 71e969c..afa9c71 100644
--- a/src/tox.ini
+++ b/src/tox.ini
@@ -26,7 +26,7 @@ passenv =
 [testenv:lint]
 commands =
     flake8
-    black --check --exclude "/(\.eggs|\.git|\.tox|\.venv|\.build|dist|charmhelpers|mod)/" .
+    black --diff --color --check --exclude "/(\.eggs|\.git|\.tox|\.venv|\.build|dist|charmhelpers|mod)/" .
 deps =
     black
     flake8
@@ -73,7 +73,8 @@ commands =
            --cov=files \
            --cov-report=term \
            --cov-report=annotate:report/annotated \
-           --cov-report=html:report/html
+           --cov-report=html:report/html \
+           --cov-report=term-missing
 deps = -r{toxinidir}/tests/unit/requirements.txt
 
 [testenv:func]