From f51f4d385296231052a54a8e0eecbe836f983dfb Mon Sep 17 00:00:00 2001 From: Mitsuhiro Tanino Date: Fri, 25 Sep 2015 18:09:59 -0400 Subject: [PATCH] Obtain target authentication from database same as LIO target Currently, tgt, iet and cxt obtain user and password for iSCSI target by analyzing configuration file. However this information is already stored in DB and LIO obtains these authentication from provider_auth in DB. This way is simple and robust instead of analyzing configuration file directly. This patch proposes these two changes: - Change the way to obtain authentication from configuration file to DB at _get_target_chap_auth(). - Move _get_target_chap_auth() into iscsi.py and inherit the method at tgt, iet and cxt target because they can use same implementation to get authentication from DB. Co-Authored-By: Anish Bhatt Change-Id: I5188ce5855d206c513f72e01f010175490ec89b2 Partial-Bug: #1499795 --- .../unit/targets/test_base_iscsi_driver.py | 8 ++++ cinder/tests/unit/targets/test_cxt_driver.py | 39 ------------------- cinder/tests/unit/targets/test_iet_driver.py | 26 ------------- cinder/tests/unit/targets/test_lio_driver.py | 8 ---- cinder/tests/unit/targets/test_tgt_driver.py | 33 ---------------- cinder/tests/unit/test_volume.py | 12 +++++- cinder/volume/targets/cxt.py | 28 ------------- cinder/volume/targets/fake.py | 3 -- cinder/volume/targets/iet.py | 36 ----------------- cinder/volume/targets/iscsi.py | 16 ++++++-- cinder/volume/targets/lio.py | 12 ------ cinder/volume/targets/tgt.py | 29 -------------- 12 files changed, 30 insertions(+), 220 deletions(-) diff --git a/cinder/tests/unit/targets/test_base_iscsi_driver.py b/cinder/tests/unit/targets/test_base_iscsi_driver.py index 5ab0ddc0b14..267997229ba 100644 --- a/cinder/tests/unit/targets/test_base_iscsi_driver.py +++ b/cinder/tests/unit/targets/test_base_iscsi_driver.py @@ -33,6 +33,8 @@ class TestBaseISCSITargetDriver(tf.TargetDriverFixture): super(TestBaseISCSITargetDriver, self).setUp() self.target = fake.FakeTarget(root_helper=utils.get_root_helper(), configuration=self.configuration) + self.target.db = mock.MagicMock( + volume_get=lambda x, y: {'provider_auth': 'CHAP otzL 234Z'}) def test_abc_methods_not_present_fails(self): configuration = conf.Configuration(cfg.StrOpt('iscsi_target_prefix', @@ -154,3 +156,9 @@ class TestBaseISCSITargetDriver(tf.TargetDriverFixture): location = self.target._iscsi_location('portal', 1, 'target', 2, ['portal2']) self.assertEqual('portal:3260;portal2:3260,1 target 2', location) + + def test_get_target_chap_auth(self): + ctxt = context.get_admin_context() + self.assertEqual(('otzL', '234Z'), + self.target._get_target_chap_auth(ctxt, + self.test_vol)) diff --git a/cinder/tests/unit/targets/test_cxt_driver.py b/cinder/tests/unit/targets/test_cxt_driver.py index 9ae93219230..13456ac4368 100644 --- a/cinder/tests/unit/targets/test_cxt_driver.py +++ b/cinder/tests/unit/targets/test_cxt_driver.py @@ -13,11 +13,9 @@ # License for the specific language governing permissions and limitations # under the License. -import contextlib import os import mock -import six from cinder import context from cinder.tests.unit.targets import targets_fixture as tf @@ -55,43 +53,6 @@ class TestCxtAdmDriver(tf.TargetDriverFixture): ) self.assertTrue(m_exec.called) - def test_get_target_chap_auth(self): - tmp_file = six.StringIO() - tmp_file.write( - 'target:\n' - ' TargetName=iqn.2010-10.org.openstack:volume-%(id)s\n' - ' TargetDevice=/dev/%(vg)s/volume-%(id)s\n' - ' PortalGroup=1@10.9.8.7:3260\n' - ' AuthMethod=CHAP\n' - ' Auth_CHAP_Policy=Oneway\n' - ' Auth_CHAP_Initiator="otzL":"234Z"\n' % - {'id': self.VOLUME_ID, 'vg': self.VG} - ) - tmp_file.seek(0) - - expected = ('otzL', '234Z') - with mock.patch('six.moves.builtins.open') as mock_open: - ctx = context.get_admin_context() - mock_open.return_value = contextlib.closing(tmp_file) - self.assertEqual(expected, - self.target._get_target_chap_auth(ctx, - self.test_vol)) - self.assertTrue(mock_open.called) - - def test_get_target_chap_auth_negative(self): - with mock.patch('six.moves.builtins.open') as mock_open: - e = IOError() - e.errno = 123 - mock_open.side_effect = e - ctxt = context.get_admin_context() - self.assertRaises(IOError, - self.target._get_target_chap_auth, - ctxt, self.test_vol) - mock_open.side_effect = ZeroDivisionError() - self.assertRaises(ZeroDivisionError, - self.target._get_target_chap_auth, - ctxt, self.test_vol) - @mock.patch('cinder.volume.targets.cxt.CxtAdm._get_target', return_value=1) @mock.patch('cinder.utils.execute') diff --git a/cinder/tests/unit/targets/test_iet_driver.py b/cinder/tests/unit/targets/test_iet_driver.py index 92e8a4ea377..04b749e9f82 100644 --- a/cinder/tests/unit/targets/test_iet_driver.py +++ b/cinder/tests/unit/targets/test_iet_driver.py @@ -51,32 +51,6 @@ class TestIetAdmDriver(tf.TargetDriverFixture): self.target._get_target, '') - @mock.patch('os.path.exists', return_value=True) - @mock.patch('cinder.utils.temporary_chown') - def test_get_target_chap_auth(self, mock_chown, mock_exists): - tmp_file = six.StringIO() - tmp_file.write( - 'Target iqn.2010-10.org.openstack:volume-83c2e877-feed-46be-8435-77884fe55b45\n' # noqa - ' IncomingUser otzLy2UYbYfnP4zXLG5z 234Zweo38VGBBvrpK9nt\n' - ' Lun 0 Path=/dev/stack-volumes-lvmdriver-1/volume-83c2e877-feed-46be-8435-77884fe55b45,Type=fileio\n' # noqa - ) - tmp_file.seek(0) - expected = ('otzLy2UYbYfnP4zXLG5z', '234Zweo38VGBBvrpK9nt') - with mock.patch('__builtin__.open') as mock_open: - ictx = context.get_admin_context() - mock_open.return_value = contextlib.closing(tmp_file) - self.assertEqual(expected, - self.target._get_target_chap_auth(ictx, - self.test_vol)) - self.assertTrue(mock_open.called) - - # Test the failure case: Failed to handle the config file - mock_open.side_effect = StandardError() - self.assertRaises(StandardError, - self.target._get_target_chap_auth, - ictx, - self.test_vol) - @mock.patch('cinder.volume.targets.iet.IetAdm._get_target', return_value=0) @mock.patch('cinder.utils.execute') diff --git a/cinder/tests/unit/targets/test_lio_driver.py b/cinder/tests/unit/targets/test_lio_driver.py index bf933e6b75d..68b63e115bb 100644 --- a/cinder/tests/unit/targets/test_lio_driver.py +++ b/cinder/tests/unit/targets/test_lio_driver.py @@ -28,8 +28,6 @@ class TestLioAdmDriver(tf.TargetDriverFixture): with mock.patch.object(lio.LioAdm, '_verify_rtstool'): self.target = lio.LioAdm(root_helper=utils.get_root_helper(), configuration=self.configuration) - self.target.db = mock.MagicMock( - volume_get=lambda x, y: {'provider_auth': 'IncomingUser foo bar'}) @mock.patch.object(lio.LioAdm, '_execute', side_effect=lio.LioAdm._execute) @mock.patch.object(lio.LioAdm, '_persist_configuration') @@ -57,12 +55,6 @@ class TestLioAdmDriver(tf.TargetDriverFixture): self.assertEqual(expected, self.target._get_target_and_lun(ctxt, self.testvol)) - def test_get_target_chap_auth(self): - ctxt = context.get_admin_context() - self.assertEqual(('foo', 'bar'), - self.target._get_target_chap_auth(ctxt, - self.test_vol)) - @mock.patch.object(lio.LioAdm, '_execute', side_effect=lio.LioAdm._execute) @mock.patch.object(lio.LioAdm, '_persist_configuration') @mock.patch('cinder.utils.execute') diff --git a/cinder/tests/unit/targets/test_tgt_driver.py b/cinder/tests/unit/targets/test_tgt_driver.py index f103cc72e5d..65be5e7f21d 100644 --- a/cinder/tests/unit/targets/test_tgt_driver.py +++ b/cinder/tests/unit/targets/test_tgt_driver.py @@ -142,39 +142,6 @@ class TestTgtAdmDriver(tf.TargetDriverFixture): self.assertEqual(expected, self.target._get_target_and_lun(ctxt, self.testvol)) - def test_get_target_chap_auth(self): - persist_file =\ - '\n'\ - ' backing-store %(bspath)s\n'\ - ' driver iscsi\n'\ - ' incominguser otzL 234Z\n'\ - ' write-cache on\n'\ - '' % {'id': self.VOLUME_ID, - 'bspath': self.testvol_path} - with open(os.path.join(self.fake_volumes_dir, - self.test_vol.split(':')[1]), - 'w') as tmp_file: - tmp_file.write(persist_file) - ctxt = context.get_admin_context() - expected = ('otzL', '234Z') - self.assertEqual(expected, - self.target._get_target_chap_auth(ctxt, - self.test_vol)) - - def test_get_target_chap_auth_negative(self): - with mock.patch('six.moves.builtins.open') as mock_open: - e = IOError() - e.errno = 123 - mock_open.side_effect = e - ctxt = context.get_admin_context() - self.assertRaises(IOError, - self.target._get_target_chap_auth, - ctxt, self.test_vol) - mock_open.side_effect = ZeroDivisionError() - self.assertRaises(ZeroDivisionError, - self.target._get_target_chap_auth, - ctxt, self.test_vol) - def test_create_iscsi_target(self): with mock.patch('cinder.utils.execute', return_value=('', '')),\ mock.patch.object(self.target, '_get_target', diff --git a/cinder/tests/unit/test_volume.py b/cinder/tests/unit/test_volume.py index 227b1b53042..9358b2c213c 100644 --- a/cinder/tests/unit/test_volume.py +++ b/cinder/tests/unit/test_volume.py @@ -2014,13 +2014,16 @@ class VolumeTestCase(BaseVolumeTestCase): image_id='fake_id', source_volume='fake_id') + @mock.patch.object(cinder.volume.targets.iscsi.ISCSITarget, + '_get_target_chap_auth') @mock.patch.object(db, 'volume_admin_metadata_get') @mock.patch.object(db, 'volume_get') @mock.patch.object(db, 'volume_update') def test_initialize_connection_fetchqos(self, _mock_volume_update, _mock_volume_get, - _mock_volume_admin_metadata_get): + _mock_volume_admin_metadata_get, + mock_get_target): """Make sure initialize_connection returns correct information.""" _fake_admin_meta = {'fake-key': 'fake-value'} _fake_volume = {'volume_type_id': 'fake_type_id', @@ -2046,6 +2049,7 @@ class VolumeTestCase(BaseVolumeTestCase): 'initialize_connection') as driver_init: type_qos.return_value = dict(qos_specs=qos_values) driver_init.return_value = {'data': {}} + mock_get_target.return_value = None qos_specs_expected = {'key1': 'value1', 'key2': 'value2'} # initialize_connection() passes qos_specs that is designated to @@ -2098,6 +2102,8 @@ class VolumeTestCase(BaseVolumeTestCase): 'fake_volume_id', connector) + @mock.patch.object(cinder.volume.targets.iscsi.ISCSITarget, + '_get_target_chap_auth') @mock.patch.object(db, 'volume_admin_metadata_get') @mock.patch.object(db, 'volume_update') @mock.patch.object(db, 'volume_get') @@ -2109,7 +2115,8 @@ class VolumeTestCase(BaseVolumeTestCase): mock_driver_init, mock_volume_get, mock_volume_update, - mock_metadata_get): + mock_metadata_get, + mock_get_target): fake_admin_meta = {'fake-key': 'fake-value'} fake_volume = {'volume_type_id': None, @@ -2122,6 +2129,7 @@ class VolumeTestCase(BaseVolumeTestCase): mock_volume_get.return_value = fake_volume mock_volume_update.return_value = fake_volume + mock_get_target.return_value = None connector = {'ip': 'IP', 'initiator': 'INITIATOR'} mock_driver_init.return_value = { 'driver_volume_type': 'iscsi', diff --git a/cinder/volume/targets/cxt.py b/cinder/volume/targets/cxt.py index 8ab161a701b..9a4b4c4cfe0 100644 --- a/cinder/volume/targets/cxt.py +++ b/cinder/volume/targets/cxt.py @@ -15,7 +15,6 @@ import os -import re from oslo_concurrency import processutils as putils from oslo_log import log as logging @@ -88,33 +87,6 @@ class CxtAdm(iscsi.ISCSITarget): iscsi_target = 1 return iscsi_target, lun - def _get_target_chap_auth(self, context, name): - volumes_dir = self._get_volumes_dir() - vol_id = name.split(':')[1] - volume_path = os.path.join(volumes_dir, vol_id) - - try: - with open(volume_path, 'r') as f: - volume_conf = f.read() - except IOError as e_fnf: - LOG.debug('Failed to open config for %(vol_id)s: %(e)s', - {'vol_id': vol_id, 'e': e_fnf}) - # We don't run on anything non-linux - if e_fnf.errno == 2: - return None - else: - raise - except Exception as e_vol: - LOG.error(_LE('Failed to open config for %(vol_id)s: %(e)s'), - {'vol_id': vol_id, 'e': e_vol}) - raise - - m = re.search('Auth_CHAP_Initiator="(\w+)":"(\w+)"', volume_conf) - if m: - return (m.group(1), m.group(2)) - LOG.debug('Failed to find CHAP auth from config for %s', vol_id) - return None - @staticmethod def _get_portal(ip, port=None): # ipv6 addresses use [ip]:port format, ipv4 use ip:port diff --git a/cinder/volume/targets/fake.py b/cinder/volume/targets/fake.py index 1d89afb0830..17883dd79f6 100644 --- a/cinder/volume/targets/fake.py +++ b/cinder/volume/targets/fake.py @@ -22,9 +22,6 @@ class FakeTarget(iscsi.ISCSITarget): def _get_target_and_lun(self, context, volume): return(0, 0) - def _get_target_chap_auth(self, context, iscsi_name): - pass - def create_iscsi_target(self, name, tid, lun, path, chap_auth, **kwargs): pass diff --git a/cinder/volume/targets/iet.py b/cinder/volume/targets/iet.py index 93e79bf7c90..abd5da05c21 100644 --- a/cinder/volume/targets/iet.py +++ b/cinder/volume/targets/iet.py @@ -80,42 +80,6 @@ class IetAdm(iscsi.ISCSITarget): return iscsi_target, lun - def _get_target_chap_auth(self, context, name): - - vol_id = name.split(':')[1] - if os.path.exists(self.iet_conf): - try: - with utils.temporary_chown(self.iet_conf): - with open(self.iet_conf, 'r') as f: - iet_conf_text = f.readlines() - except Exception: - # If we fail to handle config file, raise exception here to - # prevent unexpected behavior during subsequent operations. - LOG.exception(_LE("Failed to open config for %s."), vol_id) - raise - - target_found = False - for line in iet_conf_text: - if target_found: - m = re.search('(\w+) (\w+) (\w+)', line) - if m: - return (m.group(2), m.group(3)) - else: - LOG.debug("Failed to find CHAP auth from config " - "for %s", vol_id) - return None - elif name in line: - target_found = True - else: - # Missing config file is unxepected sisuation. But we will create - # new config file during create_iscsi_target(). Just we warn the - # operator here. - LOG.warning(_LW("Failed to find CHAP auth from config for " - "%(vol_id)s. Config file %(conf)s does not " - "exist."), - {'vol_id': vol_id, 'conf': self.iet_conf}) - return None - def create_iscsi_target(self, name, tid, lun, path, chap_auth=None, **kwargs): diff --git a/cinder/volume/targets/iscsi.py b/cinder/volume/targets/iscsi.py index 792c06eec66..60f02f53d87 100644 --- a/cinder/volume/targets/iscsi.py +++ b/cinder/volume/targets/iscsi.py @@ -326,15 +326,23 @@ class ISCSITarget(driver.Target): if tid is None: raise exception.NotFound() + def _get_target_chap_auth(self, context, iscsi_name): + """Get the current chap auth username and password.""" + try: + # 'iscsi_name': 'iqn.2010-10.org.openstack:volume-00000001' + vol_id = iscsi_name.split(':volume-')[1] + volume_info = self.db.volume_get(context, vol_id) + # 'provider_auth': 'CHAP user_id password' + if volume_info['provider_auth']: + return tuple(volume_info['provider_auth'].split(' ', 3)[1:]) + except exception.NotFound: + LOG.debug('Failed to get CHAP auth from DB for %s.', vol_id) + @abc.abstractmethod def _get_target_and_lun(self, context, volume): """Get iscsi target and lun.""" pass - @abc.abstractmethod - def _get_target_chap_auth(self, context, iscsi_name): - pass - @abc.abstractmethod def create_iscsi_target(self, name, tid, lun, path, chap_auth, **kwargs): diff --git a/cinder/volume/targets/lio.py b/cinder/volume/targets/lio.py index 7bd2a5108fc..05346af69bd 100644 --- a/cinder/volume/targets/lio.py +++ b/cinder/volume/targets/lio.py @@ -33,18 +33,6 @@ class LioAdm(iscsi.ISCSITarget): self._verify_rtstool() - def _get_target_chap_auth(self, context, iscsi_name): - """Get the current chap auth username and password.""" - try: - # 'iscsi_name': 'iqn.2010-10.org.openstack:volume-00000001' - vol_id = iscsi_name.split(':volume-')[1] - volume_info = self.db.volume_get(context, vol_id) - # 'provider_auth': 'CHAP user_id password' - if volume_info['provider_auth']: - return tuple(volume_info['provider_auth'].split(' ', 3)[1:]) - except exception.NotFound: - LOG.debug('Failed to get CHAP auth from DB for %s', vol_id) - def _verify_rtstool(self): try: # This call doesn't need locking diff --git a/cinder/volume/targets/tgt.py b/cinder/volume/targets/tgt.py index 5d6ac6132ca..d1e8169992c 100644 --- a/cinder/volume/targets/tgt.py +++ b/cinder/volume/targets/tgt.py @@ -11,7 +11,6 @@ # under the License. import os -import re import textwrap import time @@ -115,34 +114,6 @@ class TgtAdm(iscsi.ISCSITarget): iscsi_target = 0 # NOTE(jdg): Not used by tgtadm return iscsi_target, lun - def _get_target_chap_auth(self, context, iscsi_name): - """Get the current chap auth username and password.""" - volumes_dir = self.volumes_dir - vol_id = iscsi_name.split(':')[1] - volume_path = os.path.join(volumes_dir, vol_id) - - try: - with open(volume_path, 'r') as f: - volume_conf = f.read() - except IOError as e_fnf: - LOG.debug('Failed to open config for Volume %(vol_id)s: %(e)s', - {'vol_id': vol_id, 'e': e_fnf}) - # tgt is linux specific - if e_fnf.errno == 2: - return None - else: - raise - except Exception as e_vol: - LOG.error(_LE('Failed to open config for %(vol_id)s: %(e)s'), - {'vol_id': vol_id, 'e': e_vol}) - raise - - m = re.search('incominguser (\w+) (\w+)', volume_conf) - if m: - return (m.group(1), m.group(2)) - LOG.debug('Failed to find CHAP auth from config for %s', vol_id) - return None - @utils.retry(putils.ProcessExecutionError) def _do_tgt_update(self, name): (out, err) = utils.execute('tgt-admin', '--update', name,