From 20efd1046dfc36c02d94d972911f476ceac08d4a Mon Sep 17 00:00:00 2001 From: Shaun Edwards Date: Thu, 21 Jan 2016 13:25:43 -0800 Subject: [PATCH] EMC Isilon Driver Support For NFS Read-Only Share Added support for read-only shares to the NFS protocol in the Isilon driver. Fixed parameters in the exceptions. Change-Id: Idf8b8772e30d55667317de9f50117968d34ee877 Implements: blueprint emc-isilon-driver-readonly-share --- ...hare_back_ends_feature_support_mapping.rst | 2 +- .../drivers/emc/plugins/isilon/isilon.py | 71 ++++++---- .../drivers/emc/plugins/isilon/test_isilon.py | 122 +++++++++++++++--- 3 files changed, 157 insertions(+), 38 deletions(-) diff --git a/doc/source/devref/share_back_ends_feature_support_mapping.rst b/doc/source/devref/share_back_ends_feature_support_mapping.rst index ae944961b4..6640821f2b 100644 --- a/doc/source/devref/share_back_ends_feature_support_mapping.rst +++ b/doc/source/devref/share_back_ends_feature_support_mapping.rst @@ -82,7 +82,7 @@ Mapping of share drivers and share access rules support +----------------------------------------+--------------+----------------+------------+--------------+----------------+------------+ | EMC VNX | NFS (J) | CIFS (J) | \- | NFS (L) | CIFS (L) | \- | +----------------------------------------+--------------+----------------+------------+--------------+----------------+------------+ -| EMC Isilon | NFS,CIFS (K) | \- | \- | \- | \- | \- | +| EMC Isilon | NFS,CIFS (K) | \- | \- | NFS (M) | \- | \- | +----------------------------------------+--------------+----------------+------------+--------------+----------------+------------+ | Red Hat GlusterFS | NFS (J) | \- | \- | \- | \- | \- | +----------------------------------------+--------------+----------------+------------+--------------+----------------+------------+ diff --git a/manila/share/drivers/emc/plugins/isilon/isilon.py b/manila/share/drivers/emc/plugins/isilon/isilon.py index 6af05c60ec..18bdefab5c 100644 --- a/manila/share/drivers/emc/plugins/isilon/isilon.py +++ b/manila/share/drivers/emc/plugins/isilon/isilon.py @@ -23,6 +23,7 @@ from oslo_log import log from oslo_utils import units import six +from manila.common import constants as const from manila import exception from manila.i18n import _, _LW from manila.share.drivers.emc.plugins import base @@ -69,7 +70,7 @@ class IsilonStorageConnection(base.StorageConnection): message = (_('Unsupported share protocol: %(proto)s.') % {'proto': share['share_proto']}) LOG.error(message) - raise exception.InvalidShare(message=message) + raise exception.InvalidShare(reason=message) # apply directory quota based on share size max_share_size = share['size'] * units.Gi @@ -102,7 +103,7 @@ class IsilonStorageConnection(base.StorageConnection): _('The requested NFS share "%(share)s" was not created.') % {'share': share['name']}) LOG.error(message) - raise exception.ShareBackendException(message=message) + raise exception.ShareBackendException(msg=message) location = '{0}:{1}'.format(self._server, container_path) return location @@ -131,7 +132,7 @@ class IsilonStorageConnection(base.StorageConnection): message = (_('Unsupported share type: %(type)s.') % {'type': share['share_proto']}) LOG.error(message) - raise exception.InvalidShare(message=message) + raise exception.InvalidShare(reason=message) def _delete_nfs_share(self, share): """Is called to remove nfs share.""" @@ -148,7 +149,7 @@ class IsilonStorageConnection(base.StorageConnection): if not export_deleted: message = _('Error deleting NFS share: %s') % share['name'] LOG.error(message) - raise exception.ShareBackendException(message=message) + raise exception.ShareBackendException(msg=message) def _delete_cifs_share(self, share): """Is called to remove CIFS share.""" @@ -162,7 +163,7 @@ class IsilonStorageConnection(base.StorageConnection): if not share_deleted: message = _('Error deleting CIFS share: %s') % share['name'] LOG.error(message) - raise exception.ShareBackendException(message=message) + raise exception.ShareBackendException(msg=message) def delete_snapshot(self, context, snapshot, share_server): """Is called to remove snapshot.""" @@ -184,45 +185,57 @@ class IsilonStorageConnection(base.StorageConnection): if access['access_type'] != 'ip': message = _('Only ip access type allowed.') LOG.error(message) - raise exception.ShareBackendException(message=message) + raise exception.ShareBackendException(msg=message) access_ip = access['access_to'] if share['share_proto'] == 'NFS': export_path = self._get_container_path(share) - self._nfs_allow_access(access_ip, export_path) + self._nfs_allow_access( + access_ip, export_path, access['access_level']) elif share['share_proto'] == 'CIFS': - self._cifs_allow_access(access_ip, share) + self._cifs_allow_access( + access_ip, share, access['access_level']) else: message = _( 'Unsupported share protocol: %s. Only "NFS" and ' '"CIFS" are currently supported share protocols.') % share[ 'share_proto'] LOG.error(message) - raise exception.InvalidShare(message=message) + raise exception.InvalidShare(reason=message) - def _nfs_allow_access(self, access_ip, export_path): + def _nfs_allow_access(self, access_ip, export_path, access_level): """Allow access to nfs share.""" share_id = self._isilon_api.lookup_nfs_export(export_path) + share_access_group = 'clients' + if access_level == const.ACCESS_LEVEL_RO: + share_access_group = 'read_only_clients' + # Get current allowed clients - export = self._isilon_api.get_nfs_export(share_id) - current_clients = export['clients'] + export = self._get_existing_nfs_export(share_id) + current_clients = export[share_access_group] # Format of ips could be '10.0.0.2', or '10.0.0.2, 10.0.0.0/24' ips = list() ips.append(access_ip) ips.extend(current_clients) - export_params = {"clients": ips} + export_params = {share_access_group: ips} url = '{0}/platform/1/protocols/nfs/exports/{1}'.format( self._server_url, share_id) resp = self._isilon_api.request('PUT', url, data=export_params) resp.raise_for_status() - def _cifs_allow_access(self, ip, share): + def _cifs_allow_access(self, ip, share, access_level): """Allow access to cifs share.""" + if access_level == const.ACCESS_LEVEL_RO: + message = _('Only RW Access allowed for CIFS Protocol when using ' + 'the "ip" access type.') + LOG.error(message) + raise exception.InvalidShareAccess(reason=message) + allowed_ip = 'allow:' + ip smb_share = self._isilon_api.lookup_smb_share(share['name']) host_acl = smb_share['host_acl'] @@ -241,14 +254,19 @@ class IsilonStorageConnection(base.StorageConnection): return denied_ip = access['access_to'] + access_level = access['access_level'] if share['share_proto'] == 'NFS': - self._nfs_deny_access(denied_ip, share) + self._nfs_deny_access(denied_ip, share, access_level) elif share['share_proto'] == 'CIFS': self._cifs_deny_access(denied_ip, share) - def _nfs_deny_access(self, denied_ip, share): + def _nfs_deny_access(self, denied_ip, share, access_level): """Deny access to nfs share.""" + share_access_group = 'clients' + if access_level == const.ACCESS_LEVEL_RO: + share_access_group = 'read_only_clients' + # Get list of currently allowed client ips export_id = self._isilon_api.lookup_nfs_export( self._get_container_path(share)) @@ -256,27 +274,36 @@ class IsilonStorageConnection(base.StorageConnection): message = _('Share %s should have been created, but was not ' 'found.') % share['name'] LOG.error(message) - raise exception.ShareBackendException(message=message) - clients = self._get_nfs_ip_access_list(export_id) + raise exception.ShareBackendException(msg=message) + export = self._get_existing_nfs_export(export_id) + try: + clients = export[share_access_group] + except KeyError: + message = (_('Export %(export_name)s should have contained the ' + 'JSON key %(json_key)s, but this key was not found.') + % {'export_name': share['name'], + 'json_key': share_access_group}) + LOG.error(message) + raise exception.ShareBackendException(msg=message) allowed_ips = set(clients) if allowed_ips.__contains__(denied_ip): allowed_ips.remove(denied_ip) - data = {"clients": list(allowed_ips)} + data = {share_access_group: list(allowed_ips)} url = ('{0}/platform/1/protocols/nfs/exports/{1}' .format(self._server_url, six.text_type(export_id))) r = self._isilon_api.request('PUT', url, data=data) r.raise_for_status() - def _get_nfs_ip_access_list(self, export_id): + def _get_existing_nfs_export(self, export_id): export = self._isilon_api.get_nfs_export(export_id) if export is None: message = _('NFS share with export id %d should have been ' 'created, but was not found.') % export_id LOG.error(message) - raise exception.ShareBackendException(message=message) + raise exception.ShareBackendException(msg=message) - return export["clients"] + return export def _cifs_deny_access(self, denied_ip, share): """Deny access to cifs share.""" diff --git a/manila/tests/share/drivers/emc/plugins/isilon/test_isilon.py b/manila/tests/share/drivers/emc/plugins/isilon/test_isilon.py index a4122aaa33..8177ba1c63 100644 --- a/manila/tests/share/drivers/emc/plugins/isilon/test_isilon.py +++ b/manila/tests/share/drivers/emc/plugins/isilon/test_isilon.py @@ -16,7 +16,9 @@ import mock from oslo_log import log from oslo_utils import units +import six +from manila.common import constants as const from manila import exception from manila.share.drivers.emc.plugins.isilon import isilon from manila import test @@ -72,11 +74,13 @@ class IsilonTest(test.TestCase): def test_allow_access_single_ip_nfs(self): # setup share = {'name': self.SHARE_NAME, 'share_proto': 'NFS'} - access = {'access_type': 'ip', 'access_to': '10.1.1.10'} + access = {'access_type': 'ip', 'access_to': '10.1.1.10', + 'access_level': const.ACCESS_LEVEL_RW} share_server = None fake_export_id = 1 self._mock_isilon_api.lookup_nfs_export.return_value = fake_export_id - self._mock_isilon_api.get_nfs_export.return_value = {'clients': []} + self._mock_isilon_api.get_nfs_export.return_value = { + 'clients': []} self.assertFalse(self._mock_isilon_api.request.called) # call method under test @@ -90,7 +94,59 @@ class IsilonTest(test.TestCase): self._mock_isilon_api.request.assert_called_once_with( 'PUT', expected_url, data=expected_data) - def test_deny_access_ip_nfs(self): + def test_allow_access_with_nfs_readonly(self): + # setup + share = {'name': self.SHARE_NAME, 'share_proto': 'NFS'} + access = {'access_type': 'ip', 'access_to': '10.1.1.10', + 'access_level': const.ACCESS_LEVEL_RO} + fake_export_id = 70 + self._mock_isilon_api.lookup_nfs_export.return_value = fake_export_id + self._mock_isilon_api.get_nfs_export.return_value = { + 'read_only_clients': []} + self.assertFalse(self._mock_isilon_api.request.called) + + self.storage_connection.allow_access( + self.mock_context, share, access, None) + + # verify expected REST API call is executed + expected_url = (self.API_URL + '/platform/1/protocols/nfs/exports/' + + six.text_type(fake_export_id)) + expected_data = {'read_only_clients': ['10.1.1.10']} + self._mock_isilon_api.request.assert_called_once_with( + 'PUT', expected_url, data=expected_data) + + def test_allow_access_with_nfs_readwrite(self): + # setup + share = {'name': self.SHARE_NAME, 'share_proto': 'NFS'} + access = {'access_type': 'ip', 'access_to': '10.1.1.10', + 'access_level': const.ACCESS_LEVEL_RW} + fake_export_id = 70 + self._mock_isilon_api.lookup_nfs_export.return_value = fake_export_id + self._mock_isilon_api.get_nfs_export.return_value = { + 'clients': []} + self.assertFalse(self._mock_isilon_api.request.called) + + self.storage_connection.allow_access( + self.mock_context, share, access, None) + + # verify expected REST API call is executed + expected_url = (self.API_URL + '/platform/1/protocols/nfs/exports/' + + six.text_type(fake_export_id)) + expected_data = {'clients': ['10.1.1.10']} + self._mock_isilon_api.request.assert_called_once_with( + 'PUT', expected_url, data=expected_data) + + def test_allow_access_with_cifs_ip_readonly(self): + # Note: Driver does not currently support readonly access for "ip" type + share = {'name': self.SHARE_NAME, 'share_proto': 'CIFS'} + access = {'access_type': 'ip', 'access_to': '10.1.1.10', + 'access_level': const.ACCESS_LEVEL_RO} + + self.assertRaises( + exception.InvalidShareAccess, self.storage_connection.allow_access, + self.mock_context, share, access, None) + + def test_deny_access__ip_nfs_readwrite(self): """Verifies that an IP will be remove from a whitelist.""" fake_export_id = 1 self._mock_isilon_api.lookup_nfs_export.return_value = fake_export_id @@ -100,7 +156,8 @@ class IsilonTest(test.TestCase): 'clients': [ip_addr]} share = {'name': self.SHARE_NAME, 'share_proto': 'NFS'} - access = {'access_type': 'ip', 'access_to': ip_addr} + access = {'access_type': 'ip', 'access_to': ip_addr, + 'access_level': const.ACCESS_LEVEL_RW} share_server = None # call method under test @@ -116,6 +173,32 @@ class IsilonTest(test.TestCase): 'PUT', expected_url, data=expected_data ) + def test_deny_access__nfs_ip_readonly(self): + fake_export_id = 1 + self._mock_isilon_api.lookup_nfs_export.return_value = fake_export_id + # simulate an IP added to the whitelist + ip_addr = '10.0.0.4' + self._mock_isilon_api.get_nfs_export.return_value = { + 'read_only_clients': [ip_addr]} + + share = {'name': self.SHARE_NAME, 'share_proto': 'NFS'} + access = {'access_type': 'ip', 'access_to': ip_addr, + 'access_level': const.ACCESS_LEVEL_RO} + share_server = None + + # call method under test + self.assertFalse(self._mock_isilon_api.request.called) + self.storage_connection.deny_access(self.mock_context, share, access, + share_server) + + # verify that a call is made to remove an existing IP from the list + expected_url = (self.API_URL + '/platform/1/protocols/nfs/exports/' + + six.text_type(fake_export_id)) + expected_data = {'read_only_clients': []} + self._mock_isilon_api.request.assert_called_once_with( + 'PUT', expected_url, data=expected_data + ) + def test_deny_access_ip_cifs(self): """Verifies that an IP will be remove from a whitelist. @@ -131,7 +214,8 @@ class IsilonTest(test.TestCase): self.assertFalse(self._mock_isilon_api.request.called) # call method under test - access = {'access_type': 'ip', 'access_to': ip_addr} + access = {'access_type': 'ip', 'access_to': ip_addr, + 'access_level': const.ACCESS_LEVEL_RW} share_server = None self.storage_connection.deny_access(self.mock_context, share, access, share_server) @@ -153,7 +237,8 @@ class IsilonTest(test.TestCase): def test_deny_access_invalid_share_protocol(self): share = {'name': self.SHARE_NAME, 'share_proto': 'FOO'} - access = {'access_type': 'ip', 'access_to': '10.0.0.1'} + access = {'access_type': 'ip', 'access_to': '10.0.0.1', + 'access_level': const.ACCESS_LEVEL_RW} # This operation should return silently self.storage_connection.deny_access( @@ -161,7 +246,8 @@ class IsilonTest(test.TestCase): def test_deny_access_nfs_export_does_not_exist(self): share = {'name': self.SHARE_NAME, 'share_proto': 'NFS'} - access = {'access_type': 'ip', 'access_to': '10.0.0.1'} + access = {'access_type': 'ip', 'access_to': '10.0.0.1', + 'access_level': const.ACCESS_LEVEL_RW} self._mock_isilon_api.lookup_nfs_export.return_value = 1 self._mock_isilon_api.get_nfs_export.return_value = None @@ -173,7 +259,8 @@ class IsilonTest(test.TestCase): def test_deny_access_nfs_share_does_not_exist(self): share = {'name': self.SHARE_NAME, 'share_proto': 'NFS'} - access = {'access_type': 'ip', 'access_to': '10.0.0.1'} + access = {'access_type': 'ip', 'access_to': '10.0.0.1', + 'access_level': const.ACCESS_LEVEL_RW} self._mock_isilon_api.lookup_nfs_export.return_value = None self.assertRaises( @@ -194,17 +281,20 @@ class IsilonTest(test.TestCase): new_allowed_ip = '10.7.7.8' self._mock_isilon_api.lookup_nfs_export.return_value = fake_export_id existing_ips = ['10.0.0.1', '10.1.1.1', '10.0.0.2'] - export_json = {'clients': existing_ips} + export_json = { + 'clients': existing_ips, + 'access_level': const.ACCESS_LEVEL_RW, + } self._mock_isilon_api.get_nfs_export.return_value = export_json self.assertFalse(self._mock_isilon_api.request.called) # call method under test share = {'name': self.SHARE_NAME, 'share_proto': 'NFS'} - access = {'access_type': 'ip', 'access_to': new_allowed_ip} + access = {'access_type': 'ip', 'access_to': new_allowed_ip, + 'access_level': const.ACCESS_LEVEL_RW} share_server = None - self.storage_connection.allow_access(self.mock_context, share, - access, - share_server) + self.storage_connection.allow_access( + self.mock_context, share, access, share_server) # verify access rule is applied expected_url = (self.API_URL + '/platform/1/protocols/nfs/exports/' + @@ -239,7 +329,8 @@ class IsilonTest(test.TestCase): # call method under test share = {'name': share_name, 'share_proto': 'CIFS'} - access = {'access_type': 'ip', 'access_to': new_allowed_ip} + access = {'access_type': 'ip', 'access_to': new_allowed_ip, + 'access_level': const.ACCESS_LEVEL_RW} share_server = None self.storage_connection.allow_access(self.mock_context, share, access, @@ -265,7 +356,8 @@ class IsilonTest(test.TestCase): share_name = self.SHARE_NAME share = {'name': share_name, 'share_proto': 'CIFS'} allow_ip = '10.1.1.10' - access = {'access_type': 'ip', 'access_to': allow_ip} + access = {'access_type': 'ip', 'access_to': allow_ip, + 'access_level': const.ACCESS_LEVEL_RW} share_server = None self._mock_isilon_api.lookup_smb_share.return_value = { 'name': share_name, 'host_acl': []}