RBD: Fix check_valid_device

The check_valid_device method of the RBD connector doesn't work for
locally attached volumes (using krbd) because it only has code for
handles.

This patch adds code to handle the check of locally attached RBD volumes
running the code as root and as the current user.

In order to preserve current behavior we don't change the code for
checking valid handles, which means we are ignoring the run_as_root
parameter.

Closes-Bug: #1884552
Related-Bug: #1883720
Change-Id: Iedf1d4678eeef438ac4f03a61f5f91f3a225c3fd

os_brick/initiator/connectors/rbd.py

@@ -352,13 +352,8 @@ class RBDConnector(base.BaseLinuxConnector):
                     fileutils.delete_if_exists(rbd_handle.rbd_conf)
                     rbd_handle.close()
 
-    def check_valid_device(self, path, run_as_root=True):
-        """Verify an existing RBD handle is connected and valid."""
-        rbd_handle = path
-
-        if rbd_handle is None:
-            return False
-
+    @staticmethod
+    def _check_valid_device(rbd_handle):
         original_offset = rbd_handle.tell()
 
         try:
@@ -372,6 +367,22 @@ class RBDConnector(base.BaseLinuxConnector):
 
         return True
 
+    def check_valid_device(self, path, run_as_root=True):
+        """Verify an existing RBD handle is connected and valid."""
+        if not path:
+            return False
+
+        # We can receive a file handle or a path to a device
+        if isinstance(path, str):
+            if run_as_root:
+                return rbd_privsep.check_valid_path(path)
+            else:
+                with open(path, 'rb') as rbd_handle:
+                    return self._check_valid_device(rbd_handle)
+
+        # For backward compatibility ignore run_as_root param with handles
+        return self._check_valid_device(path)
+
     def extend_volume(self, connection_properties):
         # TODO(walter-boring): is this possible?
         raise NotImplementedError

os_brick/privileged/rbd.py

@@ -50,3 +50,10 @@ def root_create_ceph_conf(monitor_ips, monitor_ports, cluster_name, user,
     get_rbd_class()
     return RBDConnector._create_ceph_conf(monitor_ips, monitor_ports,
                                           cluster_name, user, keyring)
+
+
+@os_brick.privileged.default.entrypoint
+def check_valid_path(path):
+    get_rbd_class()
+    with open(path, 'rb') as rbd_handle:
+        return RBDConnector._check_valid_device(rbd_handle)

os_brick/tests/initiator/connectors/test_rbd.py

@@ -458,3 +458,58 @@ class RBDConnectorTestCase(test_connector.ConnectorTestCase):
             'rbd', 'showmapped', '--format=json', mock.sentinel.rbd_args,
             root_helper=connector._root_helper, run_as_root=True)
         self.assertEqual('/dev/rbd1', res)
+
+    @mock.patch.object(rbd.RBDConnector, '_check_valid_device')
+    @mock.patch('os_brick.privileged.rbd.check_valid_path')
+    @mock.patch.object(rbd, 'open')
+    def test_check_valid_device_handle_no_path(self, mock_open, check_path,
+                                               check_device):
+        connector = rbd.RBDConnector(None)
+        res = connector.check_valid_device(None)
+
+        self.assertFalse(res)
+        mock_open.assert_not_called()
+        check_path.assert_not_called()
+        check_device.assert_not_called()
+
+    @ddt.data(True, False)
+    @mock.patch.object(rbd.RBDConnector, '_check_valid_device')
+    @mock.patch('os_brick.privileged.rbd.check_valid_path')
+    @mock.patch.object(rbd, 'open')
+    def test_check_valid_device_handle(self, run_as_root, mock_open,
+                                       check_path, check_device):
+        connector = rbd.RBDConnector(None)
+        res = connector.check_valid_device(mock.sentinel.handle,
+                                           run_as_root=run_as_root)
+        check_device.assert_called_once_with(mock.sentinel.handle)
+        self.assertIs(check_device.return_value, res)
+        mock_open.assert_not_called()
+        check_path.assert_not_called()
+
+    @mock.patch.object(rbd.RBDConnector, '_check_valid_device')
+    @mock.patch('os_brick.privileged.rbd.check_valid_path')
+    @mock.patch.object(rbd, 'open')
+    def test_check_valid_device_block_root(self, mock_open, check_path,
+                                           check_device):
+        connector = rbd.RBDConnector(None)
+        path = '/dev/rbd0'
+        res = connector.check_valid_device(path, run_as_root=True)
+
+        check_path.assert_called_once_with(path)
+        self.assertEqual(check_path.return_value, res)
+        mock_open.assert_not_called()
+        check_device.assert_not_called()
+
+    @mock.patch.object(rbd.RBDConnector, '_check_valid_device')
+    @mock.patch('os_brick.privileged.rbd.check_valid_path')
+    @mock.patch.object(rbd, 'open')
+    def test_check_valid_device_block_non_root(self, mock_open, check_path,
+                                               check_device):
+        connector = rbd.RBDConnector(None)
+        path = '/dev/rbd0'
+        res = connector.check_valid_device(path, run_as_root=False)
+
+        mock_open.assert_called_once_with(path, 'rb')
+        check_device.assert_called_once_with(mock_open().__enter__())
+        self.assertIs(check_device.return_value, res)
+        check_path.assert_not_called()

os_brick/tests/privileged/test_rbd.py

@@ -65,3 +65,15 @@ class PrivRBDTestCase(base.TestCase):
         mock_connector._create_ceph_conf.assert_called_once_with(
             s.monitor_ips, s.monitor_ports, s.cluster_name, s.user, s.keyring)
         self.assertIs(mock_connector._create_ceph_conf.return_value, res)
+
+    @mock.patch.object(privsep_rbd, 'get_rbd_class')
+    @mock.patch.object(privsep_rbd, 'open')
+    @mock.patch.object(privsep_rbd, 'RBDConnector')
+    def test_check_valid_path(self, mock_connector, mock_open, mock_get_class):
+        res = privsep_rbd.check_valid_path(mock.sentinel.path)
+
+        mock_get_class.assert_called_once_with()
+        mock_open.assert_called_once_with(mock.sentinel.path, 'rb')
+        mock_connector._check_valid_device.assert_called_once_with(
+            mock_open.return_value.__enter__.return_value)
+        self.assertEqual(mock_connector._check_valid_device.return_value, res)

releasenotes/notes/rbd_check_valid_device-2f50c0639adb8e7c.yaml

@@ -0,0 +1,5 @@
+---
+fixes:
+  - |
+    Fix check_valid_path method in the RBD connector for locally attached
+    volumes.  (Bug #1884552)