From 7b050b8932632dd91dbda98d84d418c188140184 Mon Sep 17 00:00:00 2001 From: chen-li Date: Fri, 23 Jan 2015 16:26:42 +0800 Subject: [PATCH] Remove startswith for share_proto check We have explicit definition for supported share protocols. Using "startswith" to search for substring is not a right way. Change-Id: I8f482aea74249efca3bd490d971cf3e944919bb8 --- .../drivers/emc/plugins/vnx/connection.py | 20 ++-- manila/share/drivers/generic.py | 4 +- manila/share/drivers/ibm/gpfs.py | 2 +- manila/share/drivers/netapp/cluster_mode.py | 2 +- manila/share/drivers/zfssa/zfssashare.py | 10 +- manila/tests/db/fakes.py | 2 +- manila/tests/fake_share.py | 4 +- .../tests/share/drivers/emc/test_emc_vnx.py | 101 ++++++++++++++++++ manila/tests/share/drivers/ibm/test_gpfs.py | 10 +- .../share/drivers/netapp/test_cluster_mode.py | 13 ++- manila/tests/share/drivers/test_generic.py | 11 +- 11 files changed, 154 insertions(+), 25 deletions(-) diff --git a/manila/share/drivers/emc/plugins/vnx/connection.py b/manila/share/drivers/emc/plugins/vnx/connection.py index c36c088e3c..5084f8fb0d 100644 --- a/manila/share/drivers/emc/plugins/vnx/connection.py +++ b/manila/share/drivers/emc/plugins/vnx/connection.py @@ -57,11 +57,11 @@ class VNXStorageConnection(driver.StorageConnection): vdm = self.share_server_validation(share_server) self.allocate_container(share_name, size, vdm['id']) - if share['share_proto'].startswith('NFS'): + if share['share_proto'] == 'NFS': location = self._create_nfs_share(share_name, vdm['name'], share_server) - elif share['share_proto'].startswith('CIFS'): + elif share['share_proto'] == 'CIFS': location = self._create_cifs_share(share_name, vdm) else: raise exception.InvalidShare( @@ -156,12 +156,12 @@ class VNXStorageConnection(driver.StorageConnection): vdm_ref = self.share_server_validation(share_server) self.allocate_container_from_snapshot(share, snapshot, vdm_ref) - if share['share_proto'].startswith('NFS'): + if share['share_proto'] == 'NFS': self._create_nfs_share(share_name, vdm_ref['name'], share_server) location = ('%(nfs_if)s:/%(share_name)s' % {'nfs_if': share_server['backend_details']['nfs_if'], 'share_name': share_name}) - elif share['share_proto'].startswith('CIFS'): + elif share['share_proto'] == 'CIFS': location = self._create_cifs_share(share_name, vdm_ref) else: raise exception.InvalidShare( @@ -197,9 +197,9 @@ class VNXStorageConnection(driver.StorageConnection): "Return directly because there is nothing to clean")) return - if share['share_proto'].startswith('NFS'): + if share['share_proto'] == 'NFS': self._delete_nfs_share(share, share_server) - elif share['share_proto'].startswith('CIFS'): + elif share['share_proto'] == 'CIFS': self._delete_cifs_share(share, share_server) else: raise exception.InvalidShare( @@ -345,9 +345,9 @@ class VNXStorageConnection(driver.StorageConnection): def allow_access(self, emc_share_driver, context, share, access, share_server=None): """Allow access to the share.""" - if share['share_proto'].startswith('NFS'): + if share['share_proto'] == 'NFS': self._nfs_allow_access(context, share, access, share_server) - elif share['share_proto'].startswith('CIFS'): + elif share['share_proto'] == 'CIFS': self._cifs_allow_access(context, share, access, share_server) else: raise exception.InvalidShare( @@ -400,9 +400,9 @@ class VNXStorageConnection(driver.StorageConnection): def deny_access(self, emc_share_driver, context, share, access, share_server=None): """Deny access to the share.""" - if share['share_proto'].startswith('NFS'): + if share['share_proto'] == 'NFS': self._nfs_deny_access(share, access, share_server) - elif share['share_proto'].startswith('CIFS'): + elif share['share_proto'] == 'CIFS': self._cifs_deny_access(context, share, access, share_server) else: raise exception.InvalidShare( diff --git a/manila/share/drivers/generic.py b/manila/share/drivers/generic.py index e99030ded8..df2bc0a906 100644 --- a/manila/share/drivers/generic.py +++ b/manila/share/drivers/generic.py @@ -570,9 +570,9 @@ class GenericShareDriver(driver.ExecuteMixin, driver.ShareDriver): access['access_to']) def _get_helper(self, share): - if share['share_proto'].startswith('NFS'): + if share['share_proto'] == 'NFS': return self._helpers['NFS'] - elif share['share_proto'].startswith('CIFS'): + elif share['share_proto'] == 'CIFS': return self._helpers['CIFS'] else: raise exception.InvalidShare(reason='Wrong share type') diff --git a/manila/share/drivers/ibm/gpfs.py b/manila/share/drivers/ibm/gpfs.py index 2c6fada411..29a42932a5 100644 --- a/manila/share/drivers/ibm/gpfs.py +++ b/manila/share/drivers/ibm/gpfs.py @@ -512,7 +512,7 @@ class GPFSShareDriver(driver.ExecuteMixin, driver.GaneshaMixin, super(GPFSShareDriver, self)._update_share_stats(data) def _get_helper(self, share): - if share['share_proto'].startswith('NFS'): + if share['share_proto'] == 'NFS': return self._helpers[self.configuration.gpfs_nfs_server_type] else: msg = (_('Share protocol %s not supported by GPFS driver.'), diff --git a/manila/share/drivers/netapp/cluster_mode.py b/manila/share/drivers/netapp/cluster_mode.py index b9dad3dc69..8ad7368c85 100644 --- a/manila/share/drivers/netapp/cluster_mode.py +++ b/manila/share/drivers/netapp/cluster_mode.py @@ -366,7 +366,7 @@ class NetAppClusteredShareDriver(driver.ShareDriver): raise exception.NetAppException(msg) for proto in self._helpers.keys(): - if share_proto.upper().startswith(proto): + if share_proto == proto: return self._helpers[proto] err_msg = _("Invalid NAS protocol supplied: %s. ") % share_proto diff --git a/manila/share/drivers/zfssa/zfssashare.py b/manila/share/drivers/zfssa/zfssashare.py index 3914a12ef9..b6d08d2a07 100644 --- a/manila/share/drivers/zfssa/zfssashare.py +++ b/manila/share/drivers/zfssa/zfssashare.py @@ -194,7 +194,7 @@ class ZFSSAShareDriver(driver.ShareDriver): arg.update(self.default_args) arg.update({'name': share['id']}) - if share['share_proto'].startswith('CIFS'): + if share['share_proto'] == 'CIFS': arg.update({'sharesmb': 'on'}) LOG.debug("ZFSSAShareDriver.create_share: id=%(name)s, size=%(quota)s", {'name': arg['name'], @@ -240,7 +240,7 @@ class ZFSSAShareDriver(driver.ShareDriver): } arg.update(details) - if share['share_proto'].startswith('CIFS'): + if share['share_proto'] == 'CIFS': arg.update({'sharesmb': 'on'}) self.zfssa.clone_snapshot(lcfg.zfssa_pool, lcfg.zfssa_project, @@ -281,7 +281,7 @@ class ZFSSAShareDriver(driver.ShareDriver): """Allows access to an NFS share for the specified IP.""" LOG.debug("ZFSSAShareDriver.allow_access: share=%s", share['id']) lcfg = self.configuration - if share['share_proto'].startswith('NFS'): + if share['share_proto'] == 'NFS': self.zfssa.allow_access_nfs(lcfg.zfssa_pool, lcfg.zfssa_project, share['id'], @@ -291,12 +291,12 @@ class ZFSSAShareDriver(driver.ShareDriver): """Deny access to an NFS share for the specified IP.""" LOG.debug("ZFSSAShareDriver.deny_access: share=%s", share['id']) lcfg = self.configuration - if share['share_proto'].startswith('NFS'): + if share['share_proto'] == 'NFS': self.zfssa.deny_access_nfs(lcfg.zfssa_pool, lcfg.zfssa_project, share['id'], access) - elif share['share_proto'].startswith('CIFS'): + elif share['share_proto'] == 'CIFS': return def _update_share_stats(self): diff --git a/manila/tests/db/fakes.py b/manila/tests/db/fakes.py index 9173e77dd2..bc0f7f6f09 100644 --- a/manila/tests/db/fakes.py +++ b/manila/tests/db/fakes.py @@ -25,7 +25,7 @@ class FakeModel(object): self.values = values def __getattr__(self, name): - return self.values[name] + return self.values.get(name) def __getitem__(self, key): if key in self.values: diff --git a/manila/tests/fake_share.py b/manila/tests/fake_share.py index b129566dfb..da6cefb29e 100644 --- a/manila/tests/fake_share.py +++ b/manila/tests/fake_share.py @@ -22,7 +22,7 @@ def fake_share(**kwargs): 'id': 'fakeid', 'name': 'fakename', 'size': 1, - 'share_proto': 'NFS', + 'share_proto': 'fake_proto', 'share_network_id': 'fake share network id', 'share_server_id': 'fake share server id', 'export_location': 'fake_location:/fake_share', @@ -38,7 +38,7 @@ def fake_snapshot(**kwargs): 'share_id': 'fakeid', 'name': 'fakesnapshotname', 'share_size': 1, - 'share_proto': 'NFS', + 'share_proto': 'fake_proto', 'export_location': 'fake_location:/fake_share', } snapshot.update(kwargs) diff --git a/manila/tests/share/drivers/emc/test_emc_vnx.py b/manila/tests/share/drivers/emc/test_emc_vnx.py index 12fd07aaf1..0b5b4496b0 100644 --- a/manila/tests/share/drivers/emc/test_emc_vnx.py +++ b/manila/tests/share/drivers/emc/test_emc_vnx.py @@ -12,6 +12,7 @@ # WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the # License for the specific language governing permissions and limitations # under the License. +import ddt import mock from oslo_utils import units @@ -1126,6 +1127,7 @@ class SSHSideEffect(object): return item[1] +@ddt.ddt class EMCShareDriverVNXTestCase(test.TestCase): def setUp(self): super(EMCShareDriverVNXTestCase, self).setUp() @@ -1237,6 +1239,22 @@ class EMCShareDriverVNXTestCase(test.TestCase): helper.XMLAPIConnector.request.assert_has_calls(expected_calls) helper.SSHConnector.run_ssh.assert_has_calls(ssh_calls) + @ddt.data(fake_share.fake_share(), + fake_share.fake_share(share_proto='NFSBOGUS'), + fake_share.fake_share(share_proto='CIFSBOGUS')) + def test_create_share_with_wrong_proto(self, share): + share_server = TD.fake_share_server() + hook = RequestSideEffect() + hook.append(TD.resp_get_vdm()) + hook.append(TD.resp_task_succeed()) + hook.append(TD.resp_task_succeed()) + helper.XMLAPIConnector.request = mock.Mock(side_effect=hook) + sshHook = SSHSideEffect() + sshHook.append(TD.CREATE_NFS_EXPORT_OUT, TD.FAKE_ERROR) + helper.SSHConnector.run_ssh = mock.Mock(side_effect=sshHook) + self.assertRaises(exception.InvalidShare, + self.driver.create_share, None, share, share_server) + def test_create_nfs_share_default(self): share = TD.fake_share(share_proto='NFS') share_server = TD.fake_share_server() @@ -1260,6 +1278,28 @@ class EMCShareDriverVNXTestCase(test.TestCase): self.assertEqual(location, '192.168.1.1:/%s' % share['name'], "NFS export path is incorrect") + @ddt.data(fake_share.fake_share(), + fake_share.fake_share(share_proto='NFSBOGUS'), + fake_share.fake_share(share_proto='CIFSBOGUS')) + def test_delete_share_with_wrong_proto(self, share): + share_server = TD.fake_share_server() + mover_name = share_server['backend_details']['share_server_name'] + path = '/' + share['name'] + sshHook = SSHSideEffect() + sshHook.append(TD.resp_get_nfs_share_by_path(mover_name, + path)) + sshHook.append(TD.resp_delete_nfs_share_success(mover_name)) + helper.SSHConnector.run_ssh = mock.Mock(side_effect=sshHook) + + hook = RequestSideEffect() + hook.append(TD.resp_task_succeed()) + hook.append(TD.resp_get_filesystem(share['name'])) + hook.append(TD.resp_task_succeed()) + helper.XMLAPIConnector.request = mock.Mock(side_effect=hook) + self.assertRaises(exception.InvalidShare, + self.driver.delete_share, + None, share, share_server) + def test_delete_nfs_share_default(self): share = TD.fake_share(share_proto='NFS') share_server = TD.fake_share_server() @@ -1438,6 +1478,23 @@ class EMCShareDriverVNXTestCase(test.TestCase): expected_calls = [mock.call(TD.req_query_snapshot('fakesnapshotname'))] helper.XMLAPIConnector.request.assert_has_calls(expected_calls) + @ddt.data(fake_share.fake_share(), + fake_share.fake_share(share_proto='NFSBOGUS'), + fake_share.fake_share(share_proto='CIFSBOGUS')) + def test_allow_access_with_wrong_proto(self, share): + access = fake_share.fake_access() + share_server = TD.fake_share_server() + mover_name = share_server['backend_details']['share_server_name'] + path = '/' + share['name'] + sshHook = SSHSideEffect() + sshHook.append(TD.resp_get_nfs_share_by_path(mover_name, + path)) + sshHook.append(TD.resp_change_nfs_share_success(mover_name)) + helper.SSHConnector.run_ssh = mock.Mock(side_effect=sshHook) + self.assertRaises(exception.InvalidShare, + self.driver.allow_access, + None, share, access, share_server) + def test_nfs_allow_access(self): share = TD.fake_share(share_proto='NFS') access = TD.fake_access() @@ -1501,6 +1558,24 @@ class EMCShareDriverVNXTestCase(test.TestCase): ] helper.SSHConnector.run_ssh.assert_has_calls(expected_calls) + @ddt.data(fake_share.fake_share(), + fake_share.fake_share(share_proto='NFSBOGUS'), + fake_share.fake_share(share_proto='CIFSBOGUS')) + def test_deny_access_with_wrong_proto(self, share): + access = fake_share.fake_access() + share_server = TD.fake_share_server() + mover_name = share_server['backend_details']['share_server_name'] + path = '/' + share['name'] + sshHook = SSHSideEffect() + sshHook.append(TD.resp_get_nfs_share_by_path(mover_name, + path, + [access['access_to']])) + sshHook.append(TD.resp_change_nfs_share_success(mover_name)) + helper.SSHConnector.run_ssh = mock.Mock(side_effect=sshHook) + self.assertRaises(exception.InvalidShare, + self.driver.deny_access, + None, share, access, share_server) + def test_nfs_deny_access(self): share = TD.fake_share(share_proto='NFS') access = TD.fake_access() @@ -1556,6 +1631,32 @@ class EMCShareDriverVNXTestCase(test.TestCase): manila.db.share_network_get.assert_called_once_with( context, share['share_network_id']) + @ddt.data(fake_share.fake_share(), + fake_share.fake_share(share_proto='NFSBOGUS'), + fake_share.fake_share(share_proto='CIFSBOGUS')) + def test_create_share_from_snapshot_with_wrong_proto(self, share): + snap = fake_share.fake_snapshot() + share_server = TD.fake_share_server() + fake_ckpt = "fake_ckpt" + hook = RequestSideEffect() + hook.append(TD.resp_get_filesystem(share['name'])) + + helper.XMLAPIConnector.request = mock.Mock(side_effect=hook) + sshHook = SSHSideEffect() + sshHook.append(TD.GET_INTERCONNECT_ID_OUT, TD.FAKE_ERROR) + sshHook.append(TD.FAKE_OUTPUT, TD.FAKE_ERROR) + sshHook.append(TD.FAKE_OUTPUT, TD.FAKE_ERROR) + sshHook.append(TD.FAKE_OUTPUT, TD.COPY_CKPT_OUTPUT) + sshHook.append(TD.nas_info_out(snap['name'], fake_ckpt), TD.FAKE_ERROR) + sshHook.append(TD.FAKE_OUTPUT, TD.FAKE_ERROR) + sshHook.append(TD.FAKE_OUTPUT, TD.FAKE_ERROR) + sshHook.append(TD.FAKE_OUTPUT, TD.FAKE_ERROR) + sshHook.append(TD.CREATE_NFS_EXPORT_OUT, TD.FAKE_ERROR) + helper.SSHConnector.run_ssh = mock.Mock(side_effect=sshHook) + self.assertRaises(exception.InvalidShare, + self.driver.create_share_from_snapshot, + None, share, snap, share_server) + def test_create_share_from_snapshot(self): share = TD.fake_share(share_proto='NFS') snap = TD.fake_snapshot() diff --git a/manila/tests/share/drivers/ibm/test_gpfs.py b/manila/tests/share/drivers/ibm/test_gpfs.py index b863009683..4a823b07e2 100644 --- a/manila/tests/share/drivers/ibm/test_gpfs.py +++ b/manila/tests/share/drivers/ibm/test_gpfs.py @@ -17,6 +17,7 @@ import re import socket +import ddt import mock from oslo_config import cfg @@ -33,6 +34,7 @@ from manila import utils CONF = cfg.CONF +@ddt.ddt class GPFSShareDriverTestCase(test.TestCase): """Tests GPFSShareDriver.""" @@ -60,7 +62,7 @@ class GPFSShareDriverTestCase(test.TestCase): self._driver._helpers = { 'KNFS': self._helper_fake } - self.share = fake_share.fake_share() + self.share = fake_share.fake_share(share_proto='NFS') self.server = { 'backend_details': { 'ip': '1.2.3.4', @@ -131,6 +133,12 @@ class GPFSShareDriverTestCase(test.TestCase): ) self.assertEqual(len(self._driver._helpers), 1) + @ddt.data(fake_share.fake_share(), + fake_share.fake_share(share_proto='NFSBOGUS')) + def test__get_helper_with_wrong_proto(self, share): + self.assertRaises(exception.InvalidShare, + self._driver._get_helper, share) + def test_create_share(self): self._helper_fake.create_export.return_value = 'fakelocation' methods = ('_create_share', '_get_share_path') diff --git a/manila/tests/share/drivers/netapp/test_cluster_mode.py b/manila/tests/share/drivers/netapp/test_cluster_mode.py index 1429d1793d..9d8e31f303 100644 --- a/manila/tests/share/drivers/netapp/test_cluster_mode.py +++ b/manila/tests/share/drivers/netapp/test_cluster_mode.py @@ -17,6 +17,7 @@ import copy import hashlib +import ddt import mock from manila import context @@ -30,6 +31,7 @@ from manila.tests import fake_share from manila import utils +@ddt.ddt class NetAppClusteredDrvTestCase(test.TestCase): """Test suite for NetApp Cluster Mode driver.""" @@ -49,7 +51,7 @@ class NetAppClusteredDrvTestCase(test.TestCase): self._vserver_client = mock.Mock() self._vserver_client.send_request = mock.Mock() driver.NetAppApiClient = mock.Mock(return_value=self._vserver_client) - self.share = fake_share.fake_share() + self.share = fake_share.fake_share(share_proto='NFS') self.snapshot = fake_share.fake_snapshot() self.security_service = {'id': 'fake_id', 'domain': 'FAKE', @@ -461,6 +463,15 @@ class NetAppClusteredDrvTestCase(test.TestCase): mock.call('net-interface-create', interface_args), ]) + @ddt.data(fake_share.fake_share(), + fake_share.fake_share(share_proto='NFSBOGUS'), + fake_share.fake_share(share_proto='CIFSBOGUS')) + def test_get_helper_with_wrong_proto(self, share): + self.stubs.Set(self.driver, '_check_licenses', + mock.Mock(return_value=share['share_proto'])) + self.assertRaises(exception.NetAppException, + self.driver._get_helper, share) + def test_enable_nfs(self): self.driver._enable_nfs(self._vserver_client) export_args = { diff --git a/manila/tests/share/drivers/test_generic.py b/manila/tests/share/drivers/test_generic.py index 5ad67a1f53..486d224776 100644 --- a/manila/tests/share/drivers/test_generic.py +++ b/manila/tests/share/drivers/test_generic.py @@ -17,6 +17,7 @@ import os +import ddt import mock from oslo_concurrency import processutils from oslo_config import cfg @@ -39,6 +40,7 @@ from manila import volume CONF = cfg.CONF +@ddt.ddt class GenericShareDriverTestCase(test.TestCase): """Tests GenericShareDriver.""" @@ -87,7 +89,7 @@ class GenericShareDriverTestCase(test.TestCase): 'CIFS': self._helper_cifs, 'NFS': self._helper_nfs, } - self.share = fake_share.fake_share() + self.share = fake_share.fake_share(share_proto='NFS') self.server = { 'instance_id': 'fake_instance_id', 'ip': 'fake_ip', @@ -778,6 +780,13 @@ class GenericShareDriverTestCase(test.TestCase): access['access_type'], access['access_to']) + @ddt.data(fake_share.fake_share(), + fake_share.fake_share(share_proto='NFSBOGUS'), + fake_share.fake_share(share_proto='CIFSBOGUS')) + def test__get_helper_with_wrong_proto(self, share): + self.assertRaises(exception.InvalidShare, + self._driver._get_helper, share) + def test_setup_network(self): sim = self._driver.instance_manager net_info = {'server_id': 'fake',