openstack/os-brick
Code Issues Proposed changes

Fix encryption symlink issues

Browse Source 
This patch fixes 2 issues related to the symlinks, or lack of, that
connectors' connect_volume methods return.

Some connectors always return the block device instead of a symlink for
encrypted volumes, and other connectors return a symlink that is owned
by the system's udev rules.  Both cases are problematic

Returning the real device can prevent the encryptor connect_volume to
complete successfully, and in other cases (such as nvmeof) it completes,
but on the connector's disconnect volume it will leave the device behind
(i.e., /dev/nvme0n1) preventing new connections that would use that same
device name.

Returning a symlink owned by the system's udev rules means that they can
be reclaimed by those rules at any time.  This can happen with
cryptsetup, because when it creates a new mapping it triggers udev rules
for the device that can reclaim the symlink after os-brick has replaced
it.

This patch creates a couple of decorators to facilitate this for all
connectors. These decorators transform the paths so that the callers
gets the expected symlink, but the connector doesn't need to worry about
it and will always see the value it returns regardless of what symlink
the caller gets.

From this moment onwards we use our own custom symlink that starts with
"/dev/disk/by-id/os-brick".

The patch fixes bugs in other connectors (such as the RBD local
connection), but since there are no open bugs they have not been
reported.

Closes-Bug: #1964379
Closes-Bug: #1967790
Change-Id: Ie373ab050dcc0a35c749d9a53b6cf5ca060bcb58
This commit is contained in:
Gorka Eguileor 2022-04-04 20:01:39 +02:00
parent 6ca48e79ba
commit 1583a5038d
17 changed files with 572 additions and 185 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
os_brick/initiator/connectors/fibre_channel.py
View File

@ -169,6 +169,7 @@ class FibreChannelConnector(base.BaseLinuxConnector):
@utils.trace
@synchronized('extend_volume', external=True)
@utils.connect_volume_undo_prepare_result
def extend_volume(self, connection_properties):
"""Update the local kernel's size information.
@ -189,6 +190,7 @@ class FibreChannelConnector(base.BaseLinuxConnector):
raise exception.VolumePathsNotFound()
@utils.trace
@utils.connect_volume_prepare_result
@synchronized('connect_volume', external=True)
def connect_volume(self, connection_properties):
"""Attach the volume to instance_name.
@ -315,6 +317,7 @@ class FibreChannelConnector(base.BaseLinuxConnector):
@utils.trace
@synchronized('connect_volume', external=True)
@utils.connect_volume_undo_prepare_result(unlink_after=True)
def disconnect_volume(self, connection_properties, device_info,
force=False, ignore_errors=False):
"""Detach the volume from instance_name.
@ -356,8 +359,7 @@ class FibreChannelConnector(base.BaseLinuxConnector):
# There may have been more than 1 device mounted
# by the kernel for this volume. We have to remove
# all of them
path_used = self._linuxscsi.get_dev_path(connection_properties,
device_info)
path_used = utils.get_dev_path(connection_properties, device_info)
# NOTE: Due to bug #1897787 device_info may have a real path for some
# single paths instead of a symlink as it should have, so it'll only
# be a multipath if it was a symlink (not real path) and it wasn't a

39
os_brick/initiator/connectors/iscsi.py
View File

@ -475,6 +475,7 @@ class ISCSIConnector(base.BaseLinuxConnector, base_iscsi.BaseISCSIConnector):
@utils.trace
@synchronized('extend_volume', external=True)
@utils.connect_volume_undo_prepare_result
def extend_volume(self, connection_properties: dict):
"""Update the local kernel's size information.
@ -497,6 +498,7 @@ class ISCSIConnector(base.BaseLinuxConnector, base_iscsi.BaseISCSIConnector):
raise exception.VolumePathsNotFound()
@utils.trace
@utils.connect_volume_prepare_result
@synchronized('connect_volume', external=True)
def connect_volume(self, connection_properties: dict):
"""Attach the volume to instance_name.
@ -523,41 +525,8 @@ class ISCSIConnector(base.BaseLinuxConnector, base_iscsi.BaseISCSIConnector):
with excutils.save_and_reraise_exception():
self._cleanup_connection(connection_properties, force=True)
@utils.retry((exception.VolumeDeviceNotFound))
def _get_device_link(self, wwn, device, mpath):
# These are the default symlinks that should always be there
if mpath:
symlink = '/dev/disk/by-id/dm-uuid-mpath-' + mpath
else:
symlink = '/dev/disk/by-id/scsi-' + wwn
# If default symlinks are not there just search for anything that links
# to our device. In my experience this will return the last added link
# first, so if we are going to succeed this should be fast.
if not os.path.realpath(symlink) == device:
links_path = '/dev/disk/by-id/'
for symlink in os.listdir(links_path):
symlink = links_path + symlink
if os.path.realpath(symlink) == device:
break
else:
# Raising this will trigger the next retry
raise exception.VolumeDeviceNotFound(device='/dev/disk/by-id')
return symlink
def _get_connect_result(self, con_props, wwn, devices_names, mpath=None):
device = '/dev/' + (mpath or devices_names[0])
# NOTE(geguileo): This is only necessary because of the current
# encryption flow that requires that connect_volume returns a symlink
# because first we do the volume attach, then the libvirt config is
# generated using the path returned by the atach, and then we do the
# encryption attach, which is forced to preserve the path that was used
# in the libvirt config. If we fix that flow in OS-brick, Nova, and
# Cinder we can remove this and just return the real path.
if con_props.get('encrypted'):
device = self._get_device_link(wwn, device, mpath)
result = {'type': 'block', 'scsi_wwn': wwn, 'path': device}
if mpath:
result['multipath_id'] = wwn
@ -863,6 +832,7 @@ class ISCSIConnector(base.BaseLinuxConnector, base_iscsi.BaseISCSIConnector):
@utils.trace
@synchronized('connect_volume', external=True)
@utils.connect_volume_undo_prepare_result(unlink_after=True)
def disconnect_volume(self, connection_properties, device_info,
force=False, ignore_errors=False):
"""Detach the volume from instance_name.
@ -928,8 +898,7 @@ class ISCSIConnector(base.BaseLinuxConnector, base_iscsi.BaseISCSIConnector):
for remove, __ in devices_map.values():
remove_devices.update(remove)
path_used = self._linuxscsi.get_dev_path(connection_properties,
device_info)
path_used = utils.get_dev_path(connection_properties, device_info)
was_multipath = (path_used.startswith('/dev/dm-') or
'mpath' in path_used)
multipath_name = self._linuxscsi.remove_connection(

3
os_brick/initiator/connectors/lightos.py
View File

@ -256,6 +256,7 @@ class LightOSConnector(base.BaseLinuxConnector):
return None
@utils.trace
@utils.connect_volume_prepare_result
@synchronized('volume_op')
def connect_volume(self, connection_properties):
"""Discover and attach the volume.
@ -292,6 +293,7 @@ class LightOSConnector(base.BaseLinuxConnector):
@utils.trace
@synchronized('volume_op')
@utils.connect_volume_undo_prepare_result(unlink_after=True)
def disconnect_volume(self, connection_properties, device_info,
force=False, ignore_errors=False):
"""Disconnect a volume from the local host.
@ -334,6 +336,7 @@ class LightOSConnector(base.BaseLinuxConnector):
@utils.trace
@synchronized('volume_op')
@utils.connect_volume_undo_prepare_result
def extend_volume(self, connection_properties):
uuid = connection_properties['uuid']
new_size = self._get_size_by_uuid(uuid)

3
os_brick/initiator/connectors/nvmeof.py
View File

@ -307,6 +307,7 @@ class NVMeOFConnector(base.BaseLinuxConnector):
return self._is_nvme_available(nvme_name)
@utils.trace
@utils.connect_volume_prepare_result
@synchronized('connect_volume', external=True)
def connect_volume(self, connection_properties):
"""Discover and attach the volume.
@ -359,6 +360,7 @@ class NVMeOFConnector(base.BaseLinuxConnector):
@utils.trace
@synchronized('connect_volume', external=True)
@utils.connect_volume_undo_prepare_result(unlink_after=True)
def disconnect_volume(self, connection_properties, device_info,
force=False, ignore_errors=False):
"""Flush the volume.
@ -403,6 +405,7 @@ class NVMeOFConnector(base.BaseLinuxConnector):
@utils.trace
@synchronized('extend_volume', external=True)
@utils.connect_volume_undo_prepare_result
def extend_volume(self, connection_properties):
"""Update the local kernel's size information.

3
os_brick/initiator/connectors/rbd.py
View File

@ -227,6 +227,7 @@ class RBDConnector(base_rbd.RBDConnectorMixin, base.BaseLinuxConnector):
return res
@utils.trace
@utils.connect_volume_prepare_result
def connect_volume(self, connection_properties):
"""Connect to a volume.
@ -305,6 +306,7 @@ class RBDConnector(base_rbd.RBDConnectorMixin, base.BaseLinuxConnector):
return None
@utils.trace
@utils.connect_volume_undo_prepare_result(unlink_after=True)
def disconnect_volume(self, connection_properties, device_info,
force=False, ignore_errors=False):
"""Disconnect a volume.
@ -368,6 +370,7 @@ class RBDConnector(base_rbd.RBDConnectorMixin, base.BaseLinuxConnector):
# For backward compatibility ignore run_as_root param with handles
return self._check_valid_device(path)
@utils.connect_volume_undo_prepare_result
def extend_volume(self, connection_properties):
"""Refresh local volume view and return current size in bytes."""
# Nothing to do, RBD attached volumes are automatically refreshed, but

3
os_brick/initiator/connectors/scaleio.py
View File

@ -348,6 +348,7 @@ class ScaleIOConnector(base.BaseLinuxConnector):
return device_info
@utils.trace
@utils.connect_volume_prepare_result
@lockutils.synchronized('scaleio', 'scaleio-', external=True)
def connect_volume(self, connection_properties):
"""Connect the volume.
@ -463,6 +464,7 @@ class ScaleIOConnector(base.BaseLinuxConnector):
@utils.trace
@lockutils.synchronized('scaleio', 'scaleio-', external=True)
@utils.connect_volume_undo_prepare_result(unlink_after=True)
def disconnect_volume(self, connection_properties, device_info,
force=False, ignore_errors=False):
"""Disconnect the ScaleIO volume.
@ -526,6 +528,7 @@ class ScaleIOConnector(base.BaseLinuxConnector):
LOG.error(msg)
raise exception.BrickException(message=msg)
@utils.connect_volume_undo_prepare_result
def extend_volume(self, connection_properties):
"""Update the local kernel's size information.

4
os_brick/initiator/connectors/storpool.py
View File

@ -21,6 +21,7 @@ from oslo_utils import importutils
from os_brick import exception
from os_brick.initiator.connectors import base
from os_brick import utils
LOG = logging.getLogger(__name__)
@ -50,6 +51,7 @@ class StorPoolConnector(base.BaseLinuxConnector):
"""The StorPool connector properties."""
return {}
@utils.connect_volume_prepare_result
def connect_volume(self, connection_properties):
"""Connect to a volume.
@ -85,6 +87,7 @@ class StorPoolConnector(base.BaseLinuxConnector):
self._attach.sync(req_id, None)
return {'type': 'block', 'path': '/dev/storpool/' + volume}
@utils.connect_volume_undo_prepare_result(unlink_after=True)
def disconnect_volume(self, connection_properties, device_info,
force=False, ignore_errors=False):
"""Disconnect a volume from the local host.
@ -195,6 +198,7 @@ class StorPoolConnector(base.BaseLinuxConnector):
else:
return None
@utils.connect_volume_undo_prepare_result
def extend_volume(self, connection_properties):
"""Update the attached volume's size.

23
os_brick/initiator/initiator_connector.py
View File

@ -126,6 +126,11 @@ class InitiatorConnector(executor.Executor, metaclass=abc.ABCMeta):
whenever there's a connection between the host's HBA and the storage
array's target ports.
Encrypted volumes have some peculiar requirements on the path that must
be returned, so it is recommended to decorate the method with the
os_brick.utils.connect_volume_prepare_result to ensure that the right
device path is returned to the caller.
:param connection_properties: The dictionary that describes all
of the target volume attributes.
:type connection_properties: dict
@ -141,6 +146,15 @@ class InitiatorConnector(executor.Executor, metaclass=abc.ABCMeta):
The connection_properties are the same as from connect_volume.
The device_info is returned from connect_volume.
If the connector's connect_volume is decorated with
os_brick.utils.connect_volume_prepare_result then the path will
have been changed by the decorator if the volume was encrypted, so if
we need to have the original path that the connector returned instead
of the modified one (for example to identify the WWN from the symlink)
then we should use the
os_brick.utils.connect_volume_undo_prepare_result decorator with the
unlink_after=True parameter.
:param connection_properties: The dictionary that describes all
of the target volume attributes.
:type connection_properties: dict
@ -187,6 +201,15 @@ class InitiatorConnector(executor.Executor, metaclass=abc.ABCMeta):
system. The new volume size in bytes will be returned.
If there is a failure to update, then None will be returned.
If the connector's connect_volume is decorated with
os_brick.utils.connect_volume_prepare_result then the path will
have been changed by the decorator if the volume was encrypted, so if
this method uses the original path as a shortcut to know which device
to extend (instead of using the other connection information) then it
should use the os_brick.utils.connect_volume_undo_prepare_result
decorator on this method so that it gets the original path instead of
the modified symlink one.
:param connection_properties: The volume connection properties.
:returns: new size of the volume.
"""

8
os_brick/initiator/linuxscsi.py
View File

@ -254,14 +254,6 @@ class LinuxSCSI(executor.Executor):
return dm
return None
@staticmethod
def get_dev_path(connection_properties, device_info):
"""Determine what path was used by Nova/Cinder to access volume."""
if device_info and device_info.get('path'):
return device_info.get('path')
return connection_properties.get('device_path') or ''
@staticmethod
def requires_flush(path, path_used, was_multipath):
"""Check if a device needs to be flushed when detaching.

17
os_brick/privileged/rootwrap.py
View File

@ -216,8 +216,25 @@ def unlink_root(*links, **kwargs):
raise_at_end = kwargs.get('raise_at_end', False)
exc = exception.ExceptionChainer()
catch_exception = no_errors or raise_at_end
LOG.debug('Unlinking %s', links)
for link in links:
with exc.context(catch_exception, 'Unlink failed for %s', link):
os.unlink(link)
if not no_errors and raise_at_end and exc:
raise exc
@privileged.default.entrypoint
def link_root(target, link_name, force=True):
"""Create a symbolic link with sys admin privileges.
This method behaves like the "ln -s" command, including the force parameter
where it will replace the link_name file even if it's not a symlink.
"""
LOG.debug('Linking %s -> %s', link_name, target)
if force:
try:
os.remove(link_name)
except FileNotFoundError:
pass
os.symlink(target, link_name)

2
os_brick/tests/initiator/connectors/test_fibre_channel.py
View File

@ -740,7 +740,7 @@ class FibreChannelConnectorTestCase(test_connector.ConnectorTestCase):
@ddt.unpack
@mock.patch('os_brick.initiator.linuxscsi.LinuxSCSI.remove_scsi_device')
@mock.patch('os_brick.initiator.linuxscsi.LinuxSCSI.requires_flush')
@mock.patch('os_brick.initiator.linuxscsi.LinuxSCSI.get_dev_path')
@mock.patch('os_brick.utils.get_dev_path')
def test__remove_devices(self, path_used, was_multipath, get_dev_path_mock,
flush_mock, remove_mock):
get_dev_path_mock.return_value = path_used

117
os_brick/tests/initiator/connectors/test_iscsi.py
View File

@ -551,13 +551,17 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase):
@mock.patch.object(iscsi.ISCSIConnector, '_cleanup_connection')
def test_disconnect_volume(self, cleanup_mock):
res = self.connector.disconnect_volume(mock.sentinel.con_props,
conn_props = {'target_portal': '198.72.124.185:3260',
'target_iqn': 'iqn.2010-10.org.openstack:volume-uuid',
'target_lun': 0,
'device_path': '/dev/sda'}
res = self.connector.disconnect_volume(conn_props,
mock.sentinel.dev_info,
mock.sentinel.Force,
mock.sentinel.ignore_errors)
self.assertEqual(cleanup_mock.return_value, res)
cleanup_mock.assert_called_once_with(
mock.sentinel.con_props,
conn_props,
force=mock.sentinel.Force,
ignore_errors=mock.sentinel.ignore_errors,
device_info=mock.sentinel.dev_info,
@ -677,7 +681,7 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase):
('/dev/dm-11', True),
('/dev/disk/by-id/dm-uuid-mpath-MPATH', True))
@ddt.unpack
@mock.patch.object(linuxscsi.LinuxSCSI, 'get_dev_path')
@mock.patch('os_brick.utils.get_dev_path')
@mock.patch.object(iscsi.ISCSIConnector, '_disconnect_connection')
@mock.patch.object(iscsi.ISCSIConnector, '_get_connection_devices')
@mock.patch.object(linuxscsi.LinuxSCSI, 'flush_multipath_device')
@ -1641,17 +1645,14 @@ Setting up iSCSI targets: unused
scan_mock.assert_not_called()
dev_name_mock.assert_called_once_with(mock.sentinel.session, hctl)
@mock.patch.object(iscsi.ISCSIConnector, '_get_device_link')
def test__get_connect_result(self, get_link_mock):
def test__get_connect_result(self):
props = self.CON_PROPS.copy()
props['encrypted'] = False
res = self.connector._get_connect_result(props, 'wwn', ['sda', 'sdb'])
expected = {'type': 'block', 'scsi_wwn': 'wwn', 'path': '/dev/sda'}
self.assertDictEqual(expected, res)
get_link_mock.assert_not_called()
@mock.patch.object(iscsi.ISCSIConnector, '_get_device_link')
def test__get_connect_result_mpath(self, get_link_mock):
def test__get_connect_result_mpath(self):
props = self.CON_PROPS.copy()
props['encrypted'] = False
res = self.connector._get_connect_result(props, 'wwn', ['sda', 'sdb'],
@ -1659,106 +1660,6 @@ Setting up iSCSI targets: unused
expected = {'type': 'block', 'scsi_wwn': 'wwn', 'path': '/dev/mpath',
'multipath_id': 'wwn'}
self.assertDictEqual(expected, res)
get_link_mock.assert_not_called()
@mock.patch.object(iscsi.ISCSIConnector, '_get_device_link',
return_value='/dev/disk/by-id/scsi-wwn')
def test__get_connect_result_encrypted(self, get_link_mock):
props = self.CON_PROPS.copy()
props['encrypted'] = True
res = self.connector._get_connect_result(props, 'wwn', ['sda', 'sdb'])
expected = {'type': 'block', 'scsi_wwn': 'wwn',
'path': get_link_mock.return_value}
self.assertDictEqual(expected, res)
get_link_mock.assert_called_once_with('wwn', '/dev/sda', None)
@mock.patch('os.path.realpath', return_value='/dev/sda')
def test__get_device_link(self, realpath_mock):
symlink = '/dev/disk/by-id/scsi-wwn'
res = self.connector._get_device_link('wwn', '/dev/sda', None)
self.assertEqual(symlink, res)
realpath_mock.assert_called_once_with(symlink)
@mock.patch('os.path.realpath', return_value='/dev/dm-0')
def test__get_device_link_multipath(self, realpath_mock):
symlink = '/dev/disk/by-id/dm-uuid-mpath-wwn'
res = self.connector._get_device_link('wwn', '/dev/dm-0', 'wwn')
self.assertEqual(symlink, res)
realpath_mock.assert_called_once_with(symlink)
@mock.patch('os.path.realpath', side_effect=('/dev/sdz', '/dev/sdy',
'/dev/sda', '/dev/sdx'))
@mock.patch('os.listdir', return_value=['dm-...', 'scsi-wwn', 'scsi-...'])
def test__get_device_link_check_links(self, listdir_mock, realpath_mock):
res = self.connector._get_device_link('wwn', '/dev/sda', None)
self.assertEqual(res, '/dev/disk/by-id/scsi-wwn')
listdir_mock.assert_called_once_with('/dev/disk/by-id/')
realpath_mock.assert_has_calls([
mock.call('/dev/disk/by-id/scsi-wwn'),
mock.call('/dev/disk/by-id/dm-...'),
mock.call('/dev/disk/by-id/scsi-wwn')])
@mock.patch('os_brick.utils._time_sleep')
@mock.patch('os.path.realpath', return_value='/dev/sdz')
@mock.patch('os.listdir', return_value=['dm-...', 'scsi-...'])
def test__get_device_link_not_found(self, listdir_mock, realpath_mock,
mock_time):
self.assertRaises(exception.VolumeDeviceNotFound,
self.connector._get_device_link,
'wwn', '/dev/sda', None)
listdir_mock.assert_has_calls(3 * [mock.call('/dev/disk/by-id/')])
self.assertEqual(3, listdir_mock.call_count)
realpath_mock.assert_has_calls(
3 * [mock.call('/dev/disk/by-id/scsi-wwn'),
mock.call('/dev/disk/by-id/dm-...'),
mock.call('/dev/disk/by-id/scsi-...')])
self.assertEqual(9, realpath_mock.call_count)
@mock.patch('os_brick.utils._time_sleep')
@mock.patch('os.path.realpath')
@mock.patch('os.listdir', return_value=['dm-...', 'scsi-...'])
def test__get_device_link_symlink_found_after_retry(self, mock_listdir,
mock_realpath,
mock_time):
# Return the expected realpath on the third retry
mock_realpath.side_effect = [
None, None, None, None, None, None, '/dev/sda']
# Assert that VolumeDeviceNotFound isn't raised
self.connector._get_device_link('wwn', '/dev/sda', None)
# Assert that listdir and realpath have been called correctly
mock_listdir.assert_has_calls(2 * [mock.call('/dev/disk/by-id/')])
self.assertEqual(2, mock_listdir.call_count)
mock_realpath.assert_has_calls(
2 * [mock.call('/dev/disk/by-id/scsi-wwn'),
mock.call('/dev/disk/by-id/dm-...'),
mock.call('/dev/disk/by-id/scsi-...')]
+ [mock.call('/dev/disk/by-id/scsi-wwn')])
self.assertEqual(7, mock_realpath.call_count)
@mock.patch('os_brick.utils._time_sleep')
@mock.patch('os.path.realpath')
@mock.patch('os.listdir', return_value=['dm-...', 'scsi-...'])
def test__get_device_link_symlink_found_after_retry_by_listdir(
self, mock_listdir, mock_realpath, mock_time):
# Return the expected realpath on the second retry while looping over
# the devices returned by listdir
mock_realpath.side_effect = [
None, None, None, None, None, '/dev/sda']
# Assert that VolumeDeviceNotFound isn't raised
self.connector._get_device_link('wwn', '/dev/sda', None)
# Assert that listdir and realpath have been called correctly
mock_listdir.assert_has_calls(2 * [mock.call('/dev/disk/by-id/')])
self.assertEqual(2, mock_listdir.call_count)
mock_realpath.assert_has_calls(
2 * [mock.call('/dev/disk/by-id/scsi-wwn'),
mock.call('/dev/disk/by-id/dm-...'),
mock.call('/dev/disk/by-id/scsi-...')])
self.assertEqual(6, mock_realpath.call_count)
@mock.patch.object(iscsi.ISCSIConnector, '_run_iscsiadm_bare')
def test_get_node_startup_values(self, run_iscsiadm_bare_mock):

30
os_brick/tests/initiator/test_linuxscsi.py
View File

@ -1231,36 +1231,6 @@ loop0 0"""
self.linuxscsi.multipath_del_path('/dev/sda')
self.assertEqual(['multipathd del path /dev/sda'], self.cmds)
@ddt.data({'con_props': {}, 'dev_info': {'path': mock.sentinel.path}},
{'con_props': None, 'dev_info': {'path': mock.sentinel.path}},
{'con_props': {'device_path': mock.sentinel.device_path},
'dev_info': {'path': mock.sentinel.path}})
@ddt.unpack
def test_get_dev_path_device_info(self, con_props, dev_info):
self.assertEqual(mock.sentinel.path,
self.linuxscsi.get_dev_path(con_props, dev_info))
@ddt.data({'con_props': {'device_path': mock.sentinel.device_path},
'dev_info': {'path': None}},
{'con_props': {'device_path': mock.sentinel.device_path},
'dev_info': {'path': ''}},
{'con_props': {'device_path': mock.sentinel.device_path},
'dev_info': {}},
{'con_props': {'device_path': mock.sentinel.device_path},
'dev_info': None})
@ddt.unpack
def test_get_dev_path_conn_props(self, con_props, dev_info):
self.assertEqual(mock.sentinel.device_path,
self.linuxscsi.get_dev_path(con_props, dev_info))
@ddt.data({'con_props': {'device_path': ''}, 'dev_info': {'path': None}},
{'con_props': {'device_path': None}, 'dev_info': {'path': ''}},
{'con_props': {}, 'dev_info': {}},
{'con_props': {}, 'dev_info': None})
@ddt.unpack
def test_get_dev_path_no_path(self, con_props, dev_info):
self.assertEqual('', self.linuxscsi.get_dev_path(con_props, dev_info))
@ddt.data(('/dev/sda', '/dev/sda', False, True, None),
# This checks that we ignore the was_multipath parameter if it
# doesn't make sense (because the used path is the one we are

36
os_brick/tests/privileged/test_rootwrap.py
View File

@ -12,6 +12,7 @@
from unittest import mock
import ddt
from oslo_concurrency import processutils as putils
from os_brick import exception
@ -20,6 +21,7 @@ from os_brick.privileged import rootwrap as priv_rootwrap
from os_brick.tests import base
@ddt.ddt
class PrivRootwrapTestCase(base.TestCase):
def setUp(self):
super(PrivRootwrapTestCase, self).setUp()
@ -155,3 +157,37 @@ class PrivRootwrapTestCase(base.TestCase):
*links, raise_at_end=True)
unlink_mock.assert_has_calls([mock.call(links[0]),
mock.call(links[1])])
@mock.patch.object(priv_rootwrap.unlink_root.privsep_entrypoint,
'client_mode', False)
@mock.patch('os.symlink')
@mock.patch('os.remove')
def test_link_root_no_force(self, mock_remove, mock_link):
priv_rootwrap.link_root(mock.sentinel.target, mock.sentinel.link_name,
force=False)
mock_remove.assert_not_called()
mock_link.assert_called_once_with(mock.sentinel.target,
mock.sentinel.link_name)
@ddt.data(None, FileNotFoundError)
@mock.patch.object(priv_rootwrap.unlink_root.privsep_entrypoint,
'client_mode', False)
@mock.patch('os.symlink')
@mock.patch('os.remove')
def test_link_root_force(self, remove_effect, mock_remove, mock_link):
mock_remove.side_effect = remove_effect
priv_rootwrap.link_root(mock.sentinel.target, mock.sentinel.link_name)
mock_remove.assert_called_once_with(mock.sentinel.link_name)
mock_link.assert_called_once_with(mock.sentinel.target,
mock.sentinel.link_name)
@mock.patch.object(priv_rootwrap.unlink_root.privsep_entrypoint,
'client_mode', False)
@mock.patch('os.symlink')
@mock.patch('os.remove', side_effect=IndexError) # Non not found error
def test_link_root_force_fail(self, mock_remove, mock_link):
self.assertRaises(IndexError,
priv_rootwrap.link_root,
mock.sentinel.target, mock.sentinel.link_name)
mock_remove.assert_called_once_with(mock.sentinel.link_name)
mock_link.assert_not_called()

277
os_brick/tests/test_utils.py
View File

@ -13,9 +13,12 @@
# under the License.
import functools
import io
import time
from unittest import mock
import ddt
from os_brick import exception
from os_brick.tests import base
from os_brick import utils
@ -313,3 +316,277 @@ class LogTracingTestCase(base.TestCase):
self.assertEqual(2, mock_log.debug.call_count)
self.assertIn("'auth_password': '***'",
str(mock_log.debug.call_args_list[0]))
@ddt.ddt
class GetDevPathTestCase(base.TestCase):
"""Test the get_dev_path method."""
@ddt.data({'con_props': {}, 'dev_info': {'path': '/dev/sda'}},
{'con_props': {}, 'dev_info': {'path': b'/dev/sda'}},
{'con_props': None, 'dev_info': {'path': '/dev/sda'}},
{'con_props': None, 'dev_info': {'path': b'/dev/sda'}},
{'con_props': {'device_path': b'/dev/sdb'},
'dev_info': {'path': '/dev/sda'}},
{'con_props': {'device_path': '/dev/sdb'},
'dev_info': {'path': b'/dev/sda'}})
@ddt.unpack
def test_get_dev_path_device_info(self, con_props, dev_info):
self.assertEqual('/dev/sda', utils.get_dev_path(con_props, dev_info))
@ddt.data({'con_props': {'device_path': '/dev/sda'},
'dev_info': {'path': None}},
{'con_props': {'device_path': b'/dev/sda'},
'dev_info': {'path': None}},
{'con_props': {'device_path': '/dev/sda'},
'dev_info': {'path': ''}},
{'con_props': {'device_path': b'/dev/sda'},
'dev_info': {'path': ''}},
{'con_props': {'device_path': '/dev/sda'},
'dev_info': {}},
{'con_props': {'device_path': b'/dev/sda'},
'dev_info': {}},
{'con_props': {'device_path': '/dev/sda'},
'dev_info': None},
{'con_props': {'device_path': b'/dev/sda'},
'dev_info': None})
@ddt.unpack
def test_get_dev_path_conn_props(self, con_props, dev_info):
self.assertEqual('/dev/sda', utils.get_dev_path(con_props, dev_info))
@ddt.data({'con_props': {'device_path': ''}, 'dev_info': {'path': None}},
{'con_props': {'device_path': None}, 'dev_info': {'path': ''}},
{'con_props': {}, 'dev_info': {}},
{'con_props': {}, 'dev_info': None})
@ddt.unpack
def test_get_dev_path_no_path(self, con_props, dev_info):
self.assertEqual('', utils.get_dev_path(con_props, dev_info))
@ddt.ddt
class ConnectionPropertiesDecoratorsTestCase(base.TestCase):
def test__symlink_name_from_device_path(self):
"""Get symlink for non replicated device."""
dev_name = '/dev/nvme0n1'
res = utils._symlink_name_from_device_path(dev_name)
self.assertEqual('/dev/disk/by-id/os-brick+dev+nvme0n1', res)
def test__symlink_name_from_device_path_raid(self):
"""Get symlink for replicated device."""
dev_name = '/dev/md/alias'
res = utils._symlink_name_from_device_path(dev_name)
self.assertEqual('/dev/disk/by-id/os-brick+dev+md+alias', res)
def test__device_path_from_symlink(self):
"""Get device name for non replicated symlink."""
symlink = '/dev/disk/by-id/os-brick+dev+nvme0n1'
res = utils._device_path_from_symlink(symlink)
self.assertEqual('/dev/nvme0n1', res)
def test__device_path_from_symlink_raid(self):
"""Get device name for replicated symlink."""
symlink = '/dev/disk/by-id/os-brick+dev+md+alias'
res = utils._device_path_from_symlink(symlink)
self.assertEqual('/dev/md/alias', res)
@ddt.data(({}, {'type': 'block', 'path': '/dev/sda'}),
({'encrypted': False}, {'type': 'block', 'path': '/dev/sda'}),
({'encrypted': False}, {'type': 'block', 'path': b'/dev/sda'}),
({'encrypted': True}, {'type': 'block', 'path': io.StringIO()}))
@ddt.unpack
@mock.patch('os_brick.utils._symlink_name_from_device_path')
@mock.patch('os.path.realpath')
@mock.patch('os_brick.privileged.rootwrap.link_root')
def test_connect_volume_prepare_result_non_encrypted(
self, conn_props, result, mock_link, mock_path, mock_get_symlink):
"""Test decorator for non encrypted devices or non host devices."""
testing_self = mock.Mock()
testing_self.connect_volume.return_value = result
func = utils.connect_volume_prepare_result(testing_self.connect_volume)
res = func(testing_self, conn_props)
self.assertEqual(testing_self.connect_volume.return_value, res)
testing_self.connect_volume.assert_called_once_with(testing_self,
conn_props)
mock_path.assert_not_called()
mock_get_symlink.assert_not_called()
mock_link.assert_not_called()
@ddt.data('/dev/md/alias', b'/dev/md/alias')
@mock.patch('os_brick.utils._symlink_name_from_device_path')
@mock.patch('os.path.realpath')
@mock.patch('os_brick.privileged.rootwrap.link_root')
def test_connect_volume_prepare_result_encrypted(
self, connector_path, mock_link, mock_path, mock_get_symlink):
"""Test decorator for encrypted device."""
real_device = '/dev/md-6'
expected_symlink = '/dev/disk/by-id/os-brick_dev_md_alias'
mock_path.return_value = real_device
mock_get_symlink.return_value = expected_symlink
testing_self = mock.Mock()
testing_self.connect_volume.return_value = {'type': 'block',
'path': connector_path}
conn_props = {'encrypted': True}
func = utils.connect_volume_prepare_result(testing_self.connect_volume)
res = func(testing_self, conn_props)
self.assertEqual({'type': 'block', 'path': expected_symlink}, res)
testing_self.connect_volume.assert_called_once_with(testing_self,
conn_props)
expected_connector_path = utils.convert_str(connector_path)
mock_get_symlink.assert_called_once_with(expected_connector_path)
mock_link.assert_called_once_with(real_device, expected_symlink,
force=True)
@ddt.data({}, {'encrypted': False}, {'encrypted': True})
@mock.patch('os_brick.utils._symlink_name_from_device_path')
@mock.patch('os.path.realpath')
@mock.patch('os_brick.privileged.rootwrap.link_root')
def test_connect_volume_prepare_result_connect_fail(
self, conn_props, mock_link, mock_path, mock_get_symlink):
"""Test decorator when decorated function fails."""
testing_self = mock.Mock()
testing_self.connect_volume.side_effect = ValueError
func = utils.connect_volume_prepare_result(testing_self.connect_volume)
self.assertRaises(ValueError, func, testing_self, conn_props)
mock_link.assert_not_called()
mock_path.assert_not_called()
mock_get_symlink.assert_not_called()
@mock.patch('os_brick.utils._symlink_name_from_device_path')
@mock.patch('os.path.realpath')
@mock.patch('os_brick.privileged.rootwrap.link_root')
def test_connect_volume_prepare_result_symlink_fail(
self, mock_link, mock_path, mock_get_symlink):
"""Test decorator for encrypted device failing on the symlink."""
real_device = '/dev/md-6'
connector_path = '/dev/md/alias'
expected_symlink = '/dev/disk/by-id/os-brick_dev_md_alias'
mock_path.return_value = real_device
mock_get_symlink.return_value = expected_symlink
testing_self = mock.Mock()
connect_result = {'type': 'block', 'path': connector_path}
mock_link.side_effect = ValueError
testing_self.connect_volume.return_value = connect_result
conn_props = {'encrypted': True}
func = utils.connect_volume_prepare_result(testing_self.connect_volume)
self.assertRaises(ValueError, func, testing_self, conn_props)
testing_self.connect_volume.assert_called_once_with(testing_self,
conn_props)
mock_get_symlink.assert_called_once_with(connector_path)
mock_link.assert_called_once_with(real_device, expected_symlink,
force=True)
testing_self.disconnect_volume.assert_called_once_with(
connect_result, force=True, ignore_errors=True)
@ddt.data(({'device_path': '/dev/md/alias'}, {}),
({'device_path': '/dev/md/alias', 'encrypted': False}, None),
({'device_path': '/dev/md/alias'}, {'path': '/dev/md/alias'}),
({'device_path': '/dev/md/alias', 'encrypted': False},
{'path': '/dev/md/alias'}),
({'device_path': io.StringIO(), 'encrypted': True}, None),
({'device_path': '/dev/disk/by-id/wwn-...', 'encrypted': True},
None))
@ddt.unpack
@mock.patch('os_brick.utils._device_path_from_symlink')
@mock.patch('os_brick.privileged.rootwrap.unlink_root')
def test_connect_volume_undo_prepare_result_non_custom_link(
outer_self, conn_props, dev_info, mock_unlink, mock_dev_path):
class Test(object):
@utils.connect_volume_undo_prepare_result(unlink_after=True)
def disconnect_volume(self, connection_properties, device_info,
force=False, ignore_errors=False):
outer_self.assertEqual(conn_props, connection_properties)
outer_self.assertEqual(dev_info, device_info)
return 'disconnect_volume'
@utils.connect_volume_undo_prepare_result
def extend_volume(self, connection_properties):
outer_self.assertEqual(conn_props, connection_properties)
return 'extend_volume'
path = conn_props['device_path']
mock_dev_path.return_value = path
t = Test()
res = t.disconnect_volume(conn_props, dev_info)
outer_self.assertEqual('disconnect_volume', res)
res = t.extend_volume(conn_props)
outer_self.assertEqual('extend_volume', res)
if conn_props.get('encrypted'):
outer_self.assertEqual(2, mock_dev_path.call_count)
mock_dev_path.assert_has_calls((mock.call(path), mock.call(path)))
else:
mock_dev_path.assert_not_called()
mock_unlink.assert_not_called()
@mock.patch('os_brick.utils._device_path_from_symlink')
@mock.patch('os_brick.privileged.rootwrap.unlink_root')
def test_connect_volume_undo_prepare_result_encrypted_disconnect(
outer_self, mock_unlink, mock_dev_path):
connector_path = '/dev/md/alias'
mock_dev_path.return_value = connector_path
symlink_path = '/dev/disk/by-id/os-brick_dev_md_alias'
mock_unlink.side_effect = ValueError
class Test(object):
@utils.connect_volume_undo_prepare_result(unlink_after=True)
def disconnect_volume(self, connection_properties, device_info,
force=False, ignore_errors=False):
outer_self.assertEqual(connector_path,
connection_properties['device_path'])
outer_self.assertEqual(connector_path,
device_info['path'])
return 'disconnect_volume'
conn_props = {'target_portal': '198.72.124.185:3260',
'target_iqn': 'iqn.2010-10.org.openstack:volume-uuid',
'target_lun': 0,
'encrypted': True,
'device_path': symlink_path}
dev_info = {'type': 'block', 'path': symlink_path}
t = Test()
res = t.disconnect_volume(conn_props, dev_info)
outer_self.assertEqual('disconnect_volume', res)
mock_dev_path.assert_called_once_with(symlink_path)
mock_unlink.assert_called_once_with(symlink_path)
@mock.patch('os_brick.utils._device_path_from_symlink')
@mock.patch('os_brick.privileged.rootwrap.unlink_root')
def test_connect_volume_undo_prepare_result_encrypted_extend(
outer_self, mock_unlink, mock_dev_path):
connector_path = '/dev/md/alias'
mock_dev_path.return_value = connector_path
symlink_path = '/dev/disk/by-id/os-brick_dev_md_alias'
mock_unlink.side_effect = ValueError
class Test(object):
@utils.connect_volume_undo_prepare_result
def extend_volume(self, connection_properties):
outer_self.assertEqual(connector_path,
connection_properties['device_path'])
return 'extend_volume'
conn_props = {'target_portal': '198.72.124.185:3260',
'target_iqn': 'iqn.2010-10.org.openstack:volume-uuid',
'target_lun': 0,
'encrypted': True,
'device_path': symlink_path}
t = Test()
res = t.extend_volume(conn_props)
outer_self.assertEqual('extend_volume', res)
mock_dev_path.assert_called_once_with(symlink_path)
mock_unlink.assert_not_called()

174
os_brick/utils.py
View File

@ -15,8 +15,9 @@
import functools
import inspect
import logging as py_logging
import os
import time
from typing import Callable, Tuple, Type, Union # noqa: H301
from typing import Any, Callable, Tuple, Type, Union # noqa: H301
from oslo_concurrency import processutils
from oslo_log import log as logging
@ -24,6 +25,10 @@ from oslo_utils import strutils
from os_brick.i18n import _
from os_brick.privileged import nvmeof as priv_nvme
from os_brick.privileged import rootwrap as priv_rootwrap
CUSTOM_LINK_PREFIX = '/dev/disk/by-id/os-brick'
_time_sleep = time.sleep
@ -221,3 +226,170 @@ def get_host_nqn():
except Exception:
host_nqn = None
return host_nqn
def _symlink_name_from_device_path(device_path):
"""Generate symlink absolute path for encrypted devices.
The symlink's basename will contain the original device name so we can
reconstruct it afterwards on disconnect.
Being able to restore the original device name may be important for some
connectors, because the system may have multiple devices for the same
connection information (for example if a controller came back to life after
having network issues and an auto scan presented the device) and if we
reuse an existing symlink created by udev we wouldn't know which one was
actually used.
The symlink will be created under the /dev/disk/by-id directory and will
prefix the name with os-brick- and then continue with the full device path
that was passed (replacing '/' with '+')
"""
# Convert / into + that is unlikely used by devices or symlinks (cryptsetup
# is not happy if we use · in the symlink)
encoded_device = device_path.replace('/', '+')
return CUSTOM_LINK_PREFIX + encoded_device
def _device_path_from_symlink(symlink):
"""Get the original encrypted device path from the device symlink.
This is the reverse operation of the one performed by the
_symlink_name_from_device_path method.
"""
if symlink and symlink.startswith(CUSTOM_LINK_PREFIX):
ending = symlink[len(CUSTOM_LINK_PREFIX):]
return ending.replace('+', '/')
return symlink
def connect_volume_prepare_result(
func: Callable[[Any, dict], dict]) -> Callable[[Any, dict], dict]:
"""Decorator to prepare the result of connect_volume for encrypted volumes.
WARNING: This decorator must be **before** any connect_volume locking
because it may call disconnect_volume.
Encryptor drivers expect a symlink that they "own", so that they can modify
it as they want.
The current flow is like this:
- connect_volume connector call
- libvirt config is generated by Nova using returned path
- connect_volume encryptor call => Replaces the original path
For encrypted volumes the decorator modifies the "path" value for the
returned dictionary.
Unencrypted volumes will be left unchanged.
There are special connectors that return a file descriptor instead of a
path depending on the parameters. In those cases the result will also be
left untouched.
If a connector relies on the path that has been used they can use the
connect_volume_undo_prepare_result decorator to get the value changed back
the original path.
"""
@functools.wraps(func)
def change_encrypted(self, connection_properties):
res = func(self, connection_properties)
# Decode if path is bytes, otherwise leave it as it is
device_path = convert_str(res['path'])
# There are connectors that sometimes return file descriptors (rbd)
if (connection_properties.get('encrypted') and
isinstance(device_path, str)):
symlink = _symlink_name_from_device_path(device_path)
try:
priv_rootwrap.link_root(os.path.realpath(device_path),
symlink,
force=True)
res['path'] = symlink
except Exception as exc:
LOG.debug('Failed to create symlink, cleaning connection: %s',
exc)
self.disconnect_volume(res, force=True, ignore_errors=True)
raise
return res
return change_encrypted
def get_dev_path(connection_properties, device_info):
"""Return the device that was returned when connecting a volume."""
if device_info and device_info.get('path'):
res = device_info['path']
else:
res = connection_properties.get('device_path') or ''
# Decode if path is bytes, otherwise leave it as it is
return convert_str(res)
def connect_volume_undo_prepare_result(f=None, unlink_after=False):
"""Decorator that returns the device path to how it was originally.
WARNING: This decorator must be **the first** decorator of the method to
get the actual method signature during introspection.
Undo changes made to the device path of encrypted volumes done by the
connect_volume_prepare_result decorator.
That way the connector will always get back the same device path that it
returned.
Examples of connector methods that may want to use this are
disconnect_volume and extend_volume.
It can optionally delete the symlink on successful completion, required for
disconnect_volume method.
@connect_volume_undo_prepare_result(unlink_after=True)
def disconnect_volume(...):
@connect_volume_undo_prepare_result
def extend_volume(...):
"""
def decorator(func):
@functools.wraps(func)
def change_encrypted(*args, **kwargs):
# May receive only connection_properties or also device_info params
call_args = inspect.getcallargs(func, *args, **kwargs)
conn_props = call_args['connection_properties']
custom_symlink = False
if conn_props.get('encrypted'):
dev_info = call_args.get('device_info')
symlink = get_dev_path(conn_props, dev_info)
devpath = _device_path_from_symlink(symlink)
# Symlink can be a file descriptor, which we don't touch, same
# for old symlinks where the path is the same
if isinstance(symlink, str) and symlink != devpath:
custom_symlink = True
# Don't modify the caller's dictionaries
call_args['connection_properties'] = conn_props.copy()
call_args['connection_properties']['device_path'] = devpath
# Same for the device info dictionary
if dev_info:
dev_info = call_args['device_info'] = dev_info.copy()
dev_info['path'] = devpath
res = func(**call_args)
# Clean symlink only when asked (usually on disconnect)
if custom_symlink and unlink_after:
try:
priv_rootwrap.unlink_root(symlink)
except Exception:
LOG.warning('Failed to remove encrypted custom symlink %s',
symlink)
return res
return change_encrypted
if f:
return decorator(f)
return decorator

12
releasenotes/notes/encryption-a642889a82ff9207.yaml Normal file
View File

@ -0,0 +1,12 @@
---
fixes:
- |
NVMe-oF connector `bug #1964379
<https://bugs.launchpad.net/os-brick/+bug/1964379>`_: Fixed using non LUKS
v1 encrypted volumes, as once one of such volumes is disconnected from a
host all successive NVMe-oF attachments would fail.
- |
`Bug #1967790
<https://bugs.launchpad.net/os-brick/+bug/1967790>`_: Fixed encryptor's
connect_volume returns and the symlink is pointing to the raw block device
instead of to the decrypted device mapper device.