Add bandit and pyright checks to test code

Use tenacity where it is useful to fix pyright and bandit errors.

Signed-off-by: mulhern <[email protected]>

.github/workflows/main.yml

@@ -192,6 +192,7 @@ jobs:
       - name: Install dependencies
         run: >
           dnf install -y
+          bandit
           make
           pylint
           python3-dbus

.github/workflows/support.yml

@@ -39,6 +39,7 @@ jobs:
       matrix:
         include:
           - dependencies: >
+              bandit
               pylint
               python3-dbus-client-gen
               python3-dbus-python-client-gen

.github/workflows/weekly.yml

@@ -17,6 +17,7 @@ jobs:
         include:
           # PYTHON CHECKS ON NEXT FEDORA PYTHON AND PYTHON TOOLS VERSION
           - dependencies: >
+              bandit
               pylint
               python3-dbus-client-gen
               python3-dbus-python-client-gen

Makefile

@@ -432,6 +432,7 @@ clippy: clippy-macros clippy-min clippy-udev-utils clippy-no-ipc clippy-utils cl
 ## Lint Python parts of the source code
 pylint:
 	pylint --disable=invalid-name ./src/bin/utils/stratis-decode-dm
+	bandit ./src/bin/utils/stratis-decode-dm --skip B101
 
 .PHONY:
 	audit

tests/client-dbus/Makefile

@@ -1,9 +1,17 @@
 UNITTEST_OPTS = --verbose
 
+# Ignore bandit B404 errors. Any import of the subprocess module causes this
+# error. We know what we are doing when we import that module and do not
+# need to be warned.
+BANDIT_SKIP = --skip B404,B603,B311
+
 .PHONY: lint
 lint:
 	pylint src/stratisd_client_dbus --ignore=_introspect.py
 	pylint tests --disable=R0801
+	bandit --recursive ./src ${BANDIT_SKIP}
+	bandit --recursive ./tests ${BANDIT_SKIP},B101
+	pyright
 
 .PHONY: startup-tests
 startup-tests:

tests/client-dbus/src/stratisd_client_dbus/_implementation.py

@@ -16,7 +16,7 @@
 """
 
 # isort: STDLIB
-import xml.etree.ElementTree as ET
+import xml.etree.ElementTree as ET  # nosec B405
 
 # isort: FIRSTPARTY
 from dbus_client_gen import managed_object_class, mo_query_builder
@@ -31,9 +31,9 @@
 )
 from ._introspect import SPECS
 
-_POOL_SPEC = ET.fromstring(SPECS[POOL_INTERFACE])
-_FILESYSTEM_SPEC = ET.fromstring(SPECS[FILESYSTEM_INTERFACE])
-_BLOCKDEV_SPEC = ET.fromstring(SPECS[BLOCKDEV_INTERFACE])
+_POOL_SPEC = ET.fromstring(SPECS[POOL_INTERFACE])  # nosec B314
+_FILESYSTEM_SPEC = ET.fromstring(SPECS[FILESYSTEM_INTERFACE])  # nosec B314
+_BLOCKDEV_SPEC = ET.fromstring(SPECS[BLOCKDEV_INTERFACE])  # nosec B314
 
 pools = mo_query_builder(_POOL_SPEC)
 filesystems = mo_query_builder(_FILESYSTEM_SPEC)
@@ -46,11 +46,15 @@
 
 ObjectManager = make_class(
     "ObjectManager",
-    ET.fromstring(SPECS["org.freedesktop.DBus.ObjectManager"]),
+    ET.fromstring(SPECS["org.freedesktop.DBus.ObjectManager"]),  # nosec B314
     TIME_OUT,
 )
-Report = make_class("Report", ET.fromstring(SPECS[REPORT_INTERFACE]), TIME_OUT)
-Manager = make_class("Manager", ET.fromstring(SPECS[MANAGER_INTERFACE]), TIME_OUT)
+Report = make_class(
+    "Report", ET.fromstring(SPECS[REPORT_INTERFACE]), TIME_OUT  # nosec B314
+)
+Manager = make_class(
+    "Manager", ET.fromstring(SPECS[MANAGER_INTERFACE]), TIME_OUT  # nosec B314
+)
 Filesystem = make_class("Filesystem", _FILESYSTEM_SPEC, TIME_OUT)
 Pool = make_class("Pool", _POOL_SPEC, TIME_OUT)
 Blockdev = make_class("Blockdev", _BLOCKDEV_SPEC, TIME_OUT)

tests/client-dbus/tests/udev/_loopback.py

@@ -105,6 +105,8 @@ def create_devices(self, number):
         :return: list of keys for the devices
         :rtype: list of uuid.UUID
         """
+        assert self.dir is not None
+
         tokens = []
 
         first_creation = self.count == 0
@@ -220,6 +222,8 @@ def destroy_all(self):
         Detach all the devices and delete the file(s) and directory!
         :return: None
         """
+        assert self.dir is not None
+
         self.destroy_devices()
         os.rmdir(self.dir)
         self.dir = None

tests/client-dbus/tests/udev/_utils.py

@@ -31,7 +31,14 @@
 import dbus
 import psutil
 import pyudev
-from tenacity import Retrying, retry_if_exception_type, stop_after_delay, wait_fixed
+from tenacity import (
+    RetryError,
+    Retrying,
+    retry_if_exception_type,
+    stop_after_attempt,
+    stop_after_delay,
+    wait_fixed,
+)
 
 # isort: LOCAL
 from stratisd_client_dbus import (
@@ -235,7 +242,7 @@ def settle():
     :return: None
     """
     time.sleep(2)
-    subprocess.check_call(["udevadm", "settle"])
+    subprocess.check_call(["/usr/bin/udevadm", "settle"])
 
 
 def wait_for_udev_count(expected_num):
@@ -288,25 +295,28 @@ def wait_for_udev(fs_type, expected_paths):
     :raises RuntimeError: if unexpected device nodes are found
     """
     expected_devnodes = frozenset((os.path.realpath(x) for x in expected_paths))
-    found_devnodes = None
 
     context = pyudev.Context()
-    end_time = time.time() + 10.0
 
-    while time.time() < end_time and not expected_devnodes == found_devnodes:
-        found_devnodes = frozenset(
+    def find_devnodes():
+        return frozenset(
             [
                 x.device_node
                 for x in context.list_devices(subsystem="block", ID_FS_TYPE=fs_type)
+                if x.device_node is not None
             ]
         )
-        time.sleep(1)
 
-    if expected_devnodes != found_devnodes:
+    try:
+        for attempt in Retrying(wait=wait_fixed(1), stop=stop_after_delay(10)):
+            with attempt:
+                if expected_devnodes == find_devnodes():
+                    break
+    except RetryError as err:
         raise RuntimeError(
             f'Found unexpected devnodes: expected devnodes: {", ".join(expected_devnodes)} '
-            f'!= found_devnodes: {", ".join(found_devnodes)}'
-        )
+            f'!= found_devnodes: {", ".join(find_devnodes())}'
+        ) from err
 
 
 def processes(name):
@@ -538,32 +548,24 @@ def wait_for_pools(self, expected_num, *, name=None):
         :return: list of pool information found
         :rtype: list of (str * MOPool)
         """
-        (count, limit, dbus_err, found_num, known_pools, start_time) = (
-            0,
-            expected_num + 1,
-            None,
-            None,
-            None,
-            time.time(),
-        )
-        while count < limit and not expected_num == found_num:
-            try:
-                known_pools = get_pools(name=name)
-            except dbus.exceptions.DBusException as err:
-                dbus_err = err
-
-            if known_pools is not None:
-                found_num = len(known_pools)
-
-            time.sleep(3)
-            count += 1
-
-        if found_num is None and dbus_err is not None:
+        try:
+            for attempt in Retrying(
+                retry=(retry_if_exception_type(dbus.exceptions.DBusException)),
+                wait=wait_fixed(3),
+                stop=stop_after_attempt(expected_num + 1),
+                reraise=True,
+            ):
+                with attempt:
+                    known_pools = get_pools(name=name)
+                    if len(known_pools) == expected_num:
+                        break
+        except dbus.exceptions.DBusException as err:
             raise RuntimeError(
-                f"After {time.time() - start_time:.2f} seconds, the only "
-                "response is a D-Bus exception"
-            ) from dbus_err
+                "Failed to obtain any information about pools from the D-Bus"
+            ) from err
 
-        self.assertEqual(found_num, expected_num)
+        time.sleep(3)  # Wait for the D-Bus to add an additional pool
+        known_pools = get_pools(name=name)
+        self.assertEqual(len(known_pools), expected_num)
 
         return known_pools

tests/client-dbus/tests/udev/test_predict.py

@@ -94,7 +94,7 @@ def _call_blockdev_size(dev):
     :rtype: str
     """
     with subprocess.Popen(
-        ["blockdev", "--getsize64", dev],
+        ["/usr/sbin/blockdev", "--getsize64", dev],
         stdout=subprocess.PIPE,
     ) as command:
         outs, _ = command.communicate()

tests/client-dbus/tests/udev/test_revert.py

@@ -42,6 +42,9 @@
     settle,
 )
 
+_MOUNT = "/usr/bin/mount"
+_UMOUNT = "/usr/bin/umount"
+
 
 def write_file(mountdir, filename):
     """
@@ -99,7 +102,7 @@ def test_revert(self):  # pylint: disable=too-many-locals
             settle()
 
             filepath = f"/dev/stratis/{pool_name}/{fs_name}"
-            subprocess.check_call(["mount", filepath, mountdir])
+            subprocess.check_call([_MOUNT, filepath, mountdir])
 
             file1 = "file1.txt"
             write_file(mountdir, file1)
@@ -119,7 +122,7 @@ def test_revert(self):  # pylint: disable=too-many-locals
             write_file(mountdir, file2)
 
             Filesystem.Properties.MergeScheduled.Set(get_object(snap_object_path), True)
-            subprocess.check_call(["umount", mountdir])
+            subprocess.check_call([_UMOUNT, mountdir])
 
             self.assertTrue(os.path.exists(f"/dev/stratis/{pool_name}/{snap_name}"))
 
@@ -131,12 +134,12 @@ def test_revert(self):  # pylint: disable=too-many-locals
 
             settle()
 
-            subprocess.check_call(["mount", filepath, mountdir])
+            subprocess.check_call([_MOUNT, filepath, mountdir])
 
             self.read_file(mountdir, file1)
             self.read_file(mountdir, file2)
 
-            subprocess.check_call(["umount", mountdir])
+            subprocess.check_call([_UMOUNT, mountdir])
 
             # Now stop the pool, which should tear down the devices
             (_, return_code, message) = Manager.Methods.StopPool(
@@ -167,14 +170,14 @@ def test_revert(self):  # pylint: disable=too-many-locals
 
             settle()
 
-            subprocess.check_call(["mount", filepath, mountdir])
+            subprocess.check_call([_MOUNT, filepath, mountdir])
 
             self.read_file(mountdir, file1)
 
             self.assertFalse(os.path.exists(os.path.join(mountdir, file2)))
             self.assertFalse(os.path.exists(f"/dev/stratis/{pool_name}/{snap_name}"))
 
-            subprocess.check_call(["umount", mountdir])
+            subprocess.check_call([_UMOUNT, mountdir])
 
     def test_revert_snapshot_chain(self):  # pylint: disable=too-many-locals
         """
@@ -211,7 +214,7 @@ def test_revert_snapshot_chain(self):  # pylint: disable=too-many-locals
             settle()
 
             filepath = f"/dev/stratis/{pool_name}/{fs_name}"
-            subprocess.check_call(["mount", filepath, mountdir])
+            subprocess.check_call([_MOUNT, filepath, mountdir])
 
             file1 = "file1.txt"
             write_file(mountdir, file1)
@@ -233,7 +236,7 @@ def test_revert_snapshot_chain(self):  # pylint: disable=too-many-locals
             Filesystem.Properties.MergeScheduled.Set(
                 get_object(snap_object_path_1), True
             )
-            subprocess.check_call(["umount", mountdir])
+            subprocess.check_call([_UMOUNT, mountdir])
 
             self.assertTrue(os.path.exists(f"/dev/stratis/{pool_name}/{snap_name_1}"))
 
@@ -285,12 +288,12 @@ def test_revert_snapshot_chain(self):  # pylint: disable=too-many-locals
 
             settle()
 
-            subprocess.check_call(["mount", filepath, mountdir])
+            subprocess.check_call([_MOUNT, filepath, mountdir])
 
             self.read_file(mountdir, file1)
 
             self.assertFalse(os.path.exists(os.path.join(mountdir, file2)))
             self.assertFalse(os.path.exists(f"/dev/stratis/{pool_name}/{snap_name_1}"))
             self.assertTrue(os.path.exists(f"/dev/stratis/{pool_name}/{snap_name_2}"))
 
-            subprocess.check_call(["umount", mountdir])
+            subprocess.check_call([_UMOUNT, mountdir])