From 6ef648282284492e905ff789668e2753382bbf27 Mon Sep 17 00:00:00 2001 From: Clinton Knight Date: Sat, 6 Feb 2016 16:20:22 -0500 Subject: [PATCH] Implement update_access() in NetApp cDOT drivers This commit replaces the allow_access and deny_access methods in the NetApp cDOT drivers with the new update_access method. Due to how cDOT works, the driver always uses the full update path instead of adding or removing access rules one at a time. The rules are added in an order that guarantees no transient interruptions occur, and the IP addresses for NFS are sorted such that single addresses and smaller subnets take precedence over larger networks. Change-Id: I040690895ca1a60f5964a6d01a799b613f7f6a30 Implements: blueprint netapp-cdot-update-access --- .../netapp/dataontap/client/client_cmode.py | 67 +++++-- .../dataontap/cluster_mode/drv_multi_svm.py | 7 +- .../dataontap/cluster_mode/drv_single_svm.py | 7 +- .../netapp/dataontap/cluster_mode/lib_base.py | 16 +- .../netapp/dataontap/protocols/base.py | 32 +++- .../netapp/dataontap/protocols/cifs_cmode.py | 130 ++++++++----- .../netapp/dataontap/protocols/nfs_cmode.py | 112 +++++++++--- .../drivers/netapp/dataontap/client/fakes.py | 32 ++++ .../dataontap/client/test_client_cmode.py | 67 +++++++ .../dataontap/cluster_mode/test_lib_base.py | 42 +---- .../netapp/dataontap/protocols/fakes.py | 23 +++ .../netapp/dataontap/protocols/test_base.py | 16 ++ .../dataontap/protocols/test_cifs_cmode.py | 171 ++++++++---------- .../dataontap/protocols/test_nfs_cmode.py | 141 ++++++++------- 14 files changed, 563 insertions(+), 300 deletions(-) diff --git a/manila/share/drivers/netapp/dataontap/client/client_cmode.py b/manila/share/drivers/netapp/dataontap/client/client_cmode.py index dfd257bb4d..b5d6e419fe 100644 --- a/manila/share/drivers/netapp/dataontap/client/client_cmode.py +++ b/manila/share/drivers/netapp/dataontap/client/client_cmode.py @@ -1780,6 +1780,37 @@ class NetAppCmodeClient(client_base.NetAppBaseClient): api_args = {'path': share_path, 'share-name': share_name} self.send_request('cifs-share-create', api_args) + @na_utils.trace + def get_cifs_share_access(self, share_name): + api_args = { + 'max-records': 1000, + 'query': { + 'cifs-share-access-control': { + 'share': share_name, + }, + }, + 'desired-attributes': { + 'cifs-share-access-control': { + 'user-or-group': None, + 'permission': None, + }, + }, + } + result = self.send_request('cifs-share-access-control-get-iter', + api_args) + + attributes_list = result.get_child_by_name( + 'attributes-list') or netapp_api.NaElement('none') + + rules = {} + + for rule in attributes_list.get_children(): + user_or_group = rule.get_child_content('user-or-group') + permission = rule.get_child_content('permission') + rules[user_or_group] = permission + + return rules + @na_utils.trace def add_cifs_share_access(self, share_name, user_name, readonly): api_args = { @@ -1789,6 +1820,15 @@ class NetAppCmodeClient(client_base.NetAppBaseClient): } self.send_request('cifs-share-access-control-create', api_args) + @na_utils.trace + def modify_cifs_share_access(self, share_name, user_name, readonly): + api_args = { + 'permission': 'read' if readonly else 'full_control', + 'share': share_name, + 'user-or-group': user_name, + } + self.send_request('cifs-share-access-control-modify', api_args) + @na_utils.trace def remove_cifs_share_access(self, share_name, user_name): api_args = {'user-or-group': user_name, 'share': share_name} @@ -1799,21 +1839,22 @@ class NetAppCmodeClient(client_base.NetAppBaseClient): self.send_request('cifs-share-delete', {'share-name': share_name}) @na_utils.trace - def add_nfs_export_rule(self, policy_name, rule, readonly): - rule_indices = self._get_nfs_export_rule_indices(policy_name, rule) + def add_nfs_export_rule(self, policy_name, client_match, readonly): + rule_indices = self._get_nfs_export_rule_indices(policy_name, + client_match) if not rule_indices: - self._add_nfs_export_rule(policy_name, rule, readonly) + self._add_nfs_export_rule(policy_name, client_match, readonly) else: # Update first rule and delete the rest self._update_nfs_export_rule( - policy_name, rule, readonly, rule_indices.pop(0)) + policy_name, client_match, readonly, rule_indices.pop(0)) self._remove_nfs_export_rules(policy_name, rule_indices) @na_utils.trace - def _add_nfs_export_rule(self, policy_name, rule, readonly): + def _add_nfs_export_rule(self, policy_name, client_match, readonly): api_args = { 'policy-name': policy_name, - 'client-match': rule, + 'client-match': client_match, 'ro-rule': { 'security-flavor': 'sys', }, @@ -1827,11 +1868,12 @@ class NetAppCmodeClient(client_base.NetAppBaseClient): self.send_request('export-rule-create', api_args) @na_utils.trace - def _update_nfs_export_rule(self, policy_name, rule, readonly, rule_index): + def _update_nfs_export_rule(self, policy_name, client_match, readonly, + rule_index): api_args = { 'policy-name': policy_name, 'rule-index': rule_index, - 'client-match': rule, + 'client-match': client_match, 'ro-rule': { 'security-flavor': 'sys' }, @@ -1845,12 +1887,12 @@ class NetAppCmodeClient(client_base.NetAppBaseClient): self.send_request('export-rule-modify', api_args) @na_utils.trace - def _get_nfs_export_rule_indices(self, policy_name, rule): + def _get_nfs_export_rule_indices(self, policy_name, client_match): api_args = { 'query': { 'export-rule-info': { 'policy-name': policy_name, - 'client-match': rule, + 'client-match': client_match, }, }, 'desired-attributes': { @@ -1874,8 +1916,9 @@ class NetAppCmodeClient(client_base.NetAppBaseClient): return [six.text_type(rule_index) for rule_index in rule_indices] @na_utils.trace - def remove_nfs_export_rule(self, policy_name, rule): - rule_indices = self._get_nfs_export_rule_indices(policy_name, rule) + def remove_nfs_export_rule(self, policy_name, client_match): + rule_indices = self._get_nfs_export_rule_indices(policy_name, + client_match) self._remove_nfs_export_rules(policy_name, rule_indices) @na_utils.trace diff --git a/manila/share/drivers/netapp/dataontap/cluster_mode/drv_multi_svm.py b/manila/share/drivers/netapp/dataontap/cluster_mode/drv_multi_svm.py index 0ce4dd0fd6..d9b32d1da1 100644 --- a/manila/share/drivers/netapp/dataontap/cluster_mode/drv_multi_svm.py +++ b/manila/share/drivers/netapp/dataontap/cluster_mode/drv_multi_svm.py @@ -95,11 +95,8 @@ class NetAppCmodeMultiSvmShareDriver(driver.ShareDriver): def unmanage(self, share): raise NotImplementedError - def allow_access(self, context, share, access, **kwargs): - self.library.allow_access(context, share, access, **kwargs) - - def deny_access(self, context, share, access, **kwargs): - self.library.deny_access(context, share, access, **kwargs) + def update_access(self, context, share, access_rules, **kwargs): + self.library.update_access(context, share, access_rules, **kwargs) def _update_share_stats(self, data=None): data = self.library.get_share_stats() diff --git a/manila/share/drivers/netapp/dataontap/cluster_mode/drv_single_svm.py b/manila/share/drivers/netapp/dataontap/cluster_mode/drv_single_svm.py index adc507a01f..21f6ad913e 100644 --- a/manila/share/drivers/netapp/dataontap/cluster_mode/drv_single_svm.py +++ b/manila/share/drivers/netapp/dataontap/cluster_mode/drv_single_svm.py @@ -95,11 +95,8 @@ class NetAppCmodeSingleSvmShareDriver(driver.ShareDriver): def unmanage(self, share): self.library.unmanage(share) - def allow_access(self, context, share, access, **kwargs): - self.library.allow_access(context, share, access, **kwargs) - - def deny_access(self, context, share, access, **kwargs): - self.library.deny_access(context, share, access, **kwargs) + def update_access(self, context, share, access_rules, **kwargs): + self.library.update_access(context, share, access_rules, **kwargs) def _update_share_stats(self, data=None): data = self.library.get_share_stats() diff --git a/manila/share/drivers/netapp/dataontap/cluster_mode/lib_base.py b/manila/share/drivers/netapp/dataontap/cluster_mode/lib_base.py index ad13fe8e62..6062cfedfb 100644 --- a/manila/share/drivers/netapp/dataontap/cluster_mode/lib_base.py +++ b/manila/share/drivers/netapp/dataontap/cluster_mode/lib_base.py @@ -956,22 +956,14 @@ class NetAppCmodeFileStorageLibrary(object): vserver_client.set_volume_size(share_name, new_size) @na_utils.trace - def allow_access(self, context, share, access, share_server=None): - """Allows access to a given NAS storage.""" + def update_access(self, context, share, access_rules, add_rules=None, + delete_rules=None, share_server=None): + """Updates access rules for a share.""" vserver, vserver_client = self._get_vserver(share_server=share_server) share_name = self._get_valid_share_name(share['id']) helper = self._get_helper(share) helper.set_client(vserver_client) - helper.allow_access(context, share, share_name, access) - - @na_utils.trace - def deny_access(self, context, share, access, share_server=None): - """Denies access to a given NAS storage.""" - vserver, vserver_client = self._get_vserver(share_server=share_server) - share_name = self._get_valid_share_name(share['id']) - helper = self._get_helper(share) - helper.set_client(vserver_client) - helper.deny_access(context, share, share_name, access) + helper.update_access(share, share_name, access_rules) def setup_server(self, network_info, metadata=None): raise NotImplementedError() diff --git a/manila/share/drivers/netapp/dataontap/protocols/base.py b/manila/share/drivers/netapp/dataontap/protocols/base.py index 7fad859d1f..fc202fa031 100644 --- a/manila/share/drivers/netapp/dataontap/protocols/base.py +++ b/manila/share/drivers/netapp/dataontap/protocols/base.py @@ -17,6 +17,26 @@ import abc import six +from manila.common import constants +from manila import utils + + +def access_rules_synchronized(f): + """Decorator for synchronizing share access rule modification methods.""" + + def wrapped_func(self, *args, **kwargs): + + # The first argument is always a share, which has an ID + key = "cifs-access-%s" % args[0]['id'] + + @utils.synchronized(key) + def source_func(self, *args, **kwargs): + return f(self, *args, **kwargs) + + return source_func(self, *args, **kwargs) + + return wrapped_func + @six.add_metaclass(abc.ABCMeta) class NetAppBaseHelper(object): @@ -28,6 +48,10 @@ class NetAppBaseHelper(object): def set_client(self, client): self._client = client + def _is_readonly(self, access_level): + """Returns whether an access rule specifies read-only access.""" + return access_level == constants.ACCESS_LEVEL_RO + @abc.abstractmethod def create_share(self, share, share_name, export_addresses): """Creates NAS share.""" @@ -37,12 +61,8 @@ class NetAppBaseHelper(object): """Deletes NAS share.""" @abc.abstractmethod - def allow_access(self, context, share, share_name, access): - """Allows new_rules to a given NAS storage in new_rules.""" - - @abc.abstractmethod - def deny_access(self, context, share, share_name, access): - """Denies new_rules to a given NAS storage in new_rules.""" + def update_access(self, share, share_name, rules): + """Replaces the list of access rules known to the backend storage.""" @abc.abstractmethod def get_target(self, share): diff --git a/manila/share/drivers/netapp/dataontap/protocols/cifs_cmode.py b/manila/share/drivers/netapp/dataontap/protocols/cifs_cmode.py index 6bf546839c..234c7bfab4 100644 --- a/manila/share/drivers/netapp/dataontap/protocols/cifs_cmode.py +++ b/manila/share/drivers/netapp/dataontap/protocols/cifs_cmode.py @@ -12,7 +12,7 @@ # License for the specific language governing permissions and limitations # under the License. """ -NetApp CIFS protocol helper class. +NetApp cDOT CIFS protocol helper class. """ import re @@ -21,8 +21,7 @@ from oslo_log import log from manila.common import constants from manila import exception -from manila.i18n import _, _LE -from manila.share.drivers.netapp.dataontap.client import api as netapp_api +from manila.i18n import _ from manila.share.drivers.netapp.dataontap.protocols import base from manila.share.drivers.netapp import utils as na_utils @@ -31,7 +30,7 @@ LOG = log.getLogger(__name__) class NetAppCmodeCIFSHelper(base.NetAppBaseHelper): - """Netapp specific cluster-mode CIFS sharing driver.""" + """NetApp cDOT CIFS protocol helper class.""" @na_utils.trace def create_share(self, share, share_name, export_addresses): @@ -48,49 +47,96 @@ class NetAppCmodeCIFSHelper(base.NetAppBaseHelper): self._client.remove_cifs_share(share_name) @na_utils.trace - def allow_access(self, context, share, share_name, access): - """Allows access to the CIFS share for a given user.""" - if access['access_type'] != 'user': - msg = _("Cluster Mode supports only 'user' type for share access" - " rules with CIFS protocol.") - raise exception.InvalidShareAccess(reason=msg) + @base.access_rules_synchronized + def update_access(self, share, share_name, rules): + """Replaces the list of access rules known to the backend storage.""" - user_name = access['access_to'] + # Ensure rules are valid + for rule in rules: + self._validate_access_rule(rule) - if access['access_level'] == constants.ACCESS_LEVEL_RW: - readonly = False - elif access['access_level'] == constants.ACCESS_LEVEL_RO: - readonly = True - else: - raise exception.InvalidShareAccessLevel( - level=access['access_level']) + new_rules = {rule['access_to']: rule['access_level'] for rule in rules} - target, share_name = self._get_export_location(share) - try: - self._client.add_cifs_share_access(share_name, - user_name, - readonly) - except netapp_api.NaApiError as e: - if e.code == netapp_api.EDUPLICATEENTRY: - # Duplicate entry, so use specific exception. - raise exception.ShareAccessExists( - access_type=access['access_type'], access=access) - raise e + # Get rules from share + existing_rules = self._get_access_rules(share, share_name) + + # Update rules in an order that will prevent transient disruptions + self._handle_added_rules(share_name, existing_rules, new_rules) + self._handle_ro_to_rw_rules(share_name, existing_rules, new_rules) + self._handle_rw_to_ro_rules(share_name, existing_rules, new_rules) + self._handle_deleted_rules(share_name, existing_rules, new_rules) @na_utils.trace - def deny_access(self, context, share, share_name, access): - """Denies access to the CIFS share for a given user.""" - host_ip, share_name = self._get_export_location(share) - user_name = access['access_to'] - try: - self._client.remove_cifs_share_access(share_name, user_name) - except netapp_api.NaApiError as e: - if e.code == netapp_api.EONTAPI_EINVAL: - LOG.error(_LE("User %s does not exist."), user_name) - elif e.code == netapp_api.EOBJECTNOTFOUND: - LOG.error(_LE("Rule %s does not exist."), user_name) - else: - raise e + def _validate_access_rule(self, rule): + """Checks whether access rule type and level are valid.""" + + if rule['access_type'] != 'user': + msg = _("Clustered Data ONTAP supports only 'user' type for " + "share access rules with CIFS protocol.") + raise exception.InvalidShareAccess(reason=msg) + + if rule['access_level'] not in constants.ACCESS_LEVELS: + raise exception.InvalidShareAccessLevel(level=rule['access_level']) + + @na_utils.trace + def _handle_added_rules(self, share_name, existing_rules, new_rules): + """Updates access rules added between two rule sets.""" + added_rules = { + user_or_group: permission + for user_or_group, permission in new_rules.items() + if user_or_group not in existing_rules + } + + for user_or_group, permission in added_rules.items(): + self._client.add_cifs_share_access( + share_name, user_or_group, self._is_readonly(permission)) + + @na_utils.trace + def _handle_ro_to_rw_rules(self, share_name, existing_rules, new_rules): + """Updates access rules modified (RO-->RW) between two rule sets.""" + modified_rules = { + user_or_group: permission + for user_or_group, permission in new_rules.items() + if (user_or_group in existing_rules and + permission == constants.ACCESS_LEVEL_RW and + existing_rules[user_or_group] != 'full_control') + } + + for user_or_group, permission in modified_rules.items(): + self._client.modify_cifs_share_access( + share_name, user_or_group, self._is_readonly(permission)) + + @na_utils.trace + def _handle_rw_to_ro_rules(self, share_name, existing_rules, new_rules): + """Returns access rules modified (RW-->RO) between two rule sets.""" + modified_rules = { + user_or_group: permission + for user_or_group, permission in new_rules.items() + if (user_or_group in existing_rules and + permission == constants.ACCESS_LEVEL_RO and + existing_rules[user_or_group] != 'read') + } + + for user_or_group, permission in modified_rules.items(): + self._client.modify_cifs_share_access( + share_name, user_or_group, self._is_readonly(permission)) + + @na_utils.trace + def _handle_deleted_rules(self, share_name, existing_rules, new_rules): + """Returns access rules deleted between two rule sets.""" + deleted_rules = { + user_or_group: permission + for user_or_group, permission in existing_rules.items() + if user_or_group not in new_rules + } + + for user_or_group, permission in deleted_rules.items(): + self._client.remove_cifs_share_access(share_name, user_or_group) + + @na_utils.trace + def _get_access_rules(self, share, share_name): + """Returns the list of access rules known to the backend storage.""" + return self._client.get_cifs_share_access(share_name) @na_utils.trace def get_target(self, share): diff --git a/manila/share/drivers/netapp/dataontap/protocols/nfs_cmode.py b/manila/share/drivers/netapp/dataontap/protocols/nfs_cmode.py index b480ec1521..bd939b21a1 100644 --- a/manila/share/drivers/netapp/dataontap/protocols/nfs_cmode.py +++ b/manila/share/drivers/netapp/dataontap/protocols/nfs_cmode.py @@ -12,14 +12,18 @@ # License for the specific language governing permissions and limitations # under the License. """ -NetApp NFS protocol helper class. +NetApp cDOT NFS protocol helper class. """ +import uuid + +import netaddr from oslo_log import log +import six from manila.common import constants from manila import exception -from manila.i18n import _ +from manila.i18n import _, _LI from manila.share.drivers.netapp.dataontap.protocols import base from manila.share.drivers.netapp import utils as na_utils @@ -28,7 +32,7 @@ LOG = log.getLogger(__name__) class NetAppCmodeNFSHelper(base.NetAppBaseHelper): - """Netapp specific cluster-mode NFS sharing driver.""" + """NetApp cDOT NFS protocol helper class.""" @na_utils.trace def create_share(self, share, share_name, export_addresses): @@ -40,6 +44,7 @@ class NetAppCmodeNFSHelper(base.NetAppBaseHelper): for export_address in export_addresses] @na_utils.trace + @base.access_rules_synchronized def delete_share(self, share, share_name): """Deletes NFS share.""" LOG.debug('Deleting NFS export policy for share %s', share['id']) @@ -48,37 +53,87 @@ class NetAppCmodeNFSHelper(base.NetAppBaseHelper): self._client.soft_delete_nfs_export_policy(export_policy_name) @na_utils.trace - def allow_access(self, context, share, share_name, access): - """Allows access to a given NFS share.""" - if access['access_type'] != 'ip': - msg = _("Cluster Mode supports only 'ip' type for share access" - " rules with NFS protocol.") - raise exception.InvalidShareAccess(reason=msg) + @base.access_rules_synchronized + def update_access(self, share, share_name, rules): + """Replaces the list of access rules known to the backend storage.""" + # Ensure rules are valid + for rule in rules: + self._validate_access_rule(rule) + + # Sort rules by ascending network size + new_rules = {rule['access_to']: rule['access_level'] for rule in rules} + addresses = self._get_sorted_access_rule_addresses(new_rules) + + # Ensure current export policy has the name we expect self._ensure_export_policy(share, share_name) export_policy_name = self._get_export_policy_name(share) - rule = access['access_to'] - if access['access_level'] == constants.ACCESS_LEVEL_RW: - readonly = False - elif access['access_level'] == constants.ACCESS_LEVEL_RO: - readonly = True - else: - raise exception.InvalidShareAccessLevel( - level=access['access_level']) + # Make temp policy names so this non-atomic workflow remains resilient + # across process interruptions. + temp_new_export_policy_name = self._get_temp_export_policy_name() + temp_old_export_policy_name = self._get_temp_export_policy_name() - self._client.add_nfs_export_rule(export_policy_name, rule, readonly) + # Create new export policy + self._client.create_nfs_export_policy(temp_new_export_policy_name) + + # Add new rules to new policy + for address in addresses: + self._client.add_nfs_export_rule( + temp_new_export_policy_name, address, + self._is_readonly(new_rules[address])) + + # Rename policy currently in force + LOG.info(_LI('Renaming NFS export policy for share %(share)s to ' + '%(policy)s.') % + {'share': share_name, 'policy': temp_old_export_policy_name}) + self._client.rename_nfs_export_policy(export_policy_name, + temp_old_export_policy_name) + + # Switch share to the new policy + LOG.info(_LI('Setting NFS export policy for share %(share)s to ' + '%(policy)s.') % + {'share': share_name, 'policy': temp_new_export_policy_name}) + self._client.set_nfs_export_policy_for_volume( + share_name, temp_new_export_policy_name) + + # Delete old policy + self._client.soft_delete_nfs_export_policy(temp_old_export_policy_name) + + # Rename new policy to its final name + LOG.info(_LI('Renaming NFS export policy for share %(share)s to ' + '%(policy)s.') % + {'share': share_name, 'policy': export_policy_name}) + self._client.rename_nfs_export_policy(temp_new_export_policy_name, + export_policy_name) @na_utils.trace - def deny_access(self, context, share, share_name, access): - """Denies access to a given NFS share.""" - if access['access_type'] != 'ip': - return + def _validate_access_rule(self, rule): + """Checks whether access rule type and level are valid.""" - self._ensure_export_policy(share, share_name) - export_policy_name = self._get_export_policy_name(share) - rule = access['access_to'] - self._client.remove_nfs_export_rule(export_policy_name, rule) + if rule['access_type'] != 'ip': + msg = _("Clustered Data ONTAP supports only 'ip' type for share " + "access rules with NFS protocol.") + raise exception.InvalidShareAccess(reason=msg) + + if rule['access_level'] not in constants.ACCESS_LEVELS: + raise exception.InvalidShareAccessLevel(level=rule['access_level']) + + @na_utils.trace + def _get_sorted_access_rule_addresses(self, rules): + """Given a dict of access rules, sort by increasing network size.""" + + networks = sorted([self._get_network_object_from_rule(rule) + for rule in rules], reverse=True) + + return [six.text_type(network) for network in networks] + + def _get_network_object_from_rule(self, rule): + """Get most appropriate netaddr object for address or network rule.""" + try: + return netaddr.IPAddress(rule) + except ValueError: + return netaddr.IPNetwork(rule) @na_utils.trace def get_target(self, share): @@ -98,6 +153,11 @@ class NetAppCmodeNFSHelper(base.NetAppBaseHelper): export_location = share['export_location'] or ':' return export_location.rsplit(':', 1) + @staticmethod + def _get_temp_export_policy_name(): + """Builds export policy name for an NFS share.""" + return 'temp_' + six.text_type(uuid.uuid1()).replace('-', '_') + @staticmethod def _get_export_policy_name(share): """Builds export policy name for an NFS share.""" diff --git a/manila/tests/share/drivers/netapp/dataontap/client/fakes.py b/manila/tests/share/drivers/netapp/dataontap/client/fakes.py index 0bd0b801a0..9874fcfaec 100644 --- a/manila/tests/share/drivers/netapp/dataontap/client/fakes.py +++ b/manila/tests/share/drivers/netapp/dataontap/client/fakes.py @@ -1264,6 +1264,38 @@ SNAPSHOT_MULTIDELETE_ERROR_RESPONSE = etree.XML(""" """ % {'volume': SHARE_NAME}) +CIFS_SHARE_ACCESS_CONTROL_GET_ITER = etree.XML(""" + + + + full_control + %(volume)s + Administrator + manila_svm_cifs + + + change + %(volume)s + Administrators + manila_svm_cifs + + + read + %(volume)s + Power Users + manila_svm_cifs + + + no_access + %(volume)s + Users + manila_svm_cifs + + + 4 + +""" % {'volume': SHARE_NAME}) + NFS_EXPORT_RULES = ('10.10.10.10', '10.10.10.20') NFS_EXPORTFS_LIST_RULES_2_NO_RULES_RESPONSE = etree.XML(""" diff --git a/manila/tests/share/drivers/netapp/dataontap/client/test_client_cmode.py b/manila/tests/share/drivers/netapp/dataontap/client/test_client_cmode.py index 78e3201957..1df95c79a9 100644 --- a/manila/tests/share/drivers/netapp/dataontap/client/test_client_cmode.py +++ b/manila/tests/share/drivers/netapp/dataontap/client/test_client_cmode.py @@ -3113,6 +3113,53 @@ class NetAppClientCmodeTestCase(test.TestCase): self.client.send_request.assert_has_calls([ mock.call('cifs-share-create', cifs_share_create_args)]) + def test_get_cifs_share_access(self): + + api_response = netapp_api.NaElement( + fake.CIFS_SHARE_ACCESS_CONTROL_GET_ITER) + self.mock_object(self.client, + 'send_request', + mock.Mock(return_value=api_response)) + + result = self.client.get_cifs_share_access(fake.SHARE_NAME) + + cifs_share_access_control_get_iter_args = { + 'max-records': 1000, + 'query': { + 'cifs-share-access-control': { + 'share': fake.SHARE_NAME, + }, + }, + 'desired-attributes': { + 'cifs-share-access-control': { + 'user-or-group': None, + 'permission': None, + }, + }, + } + self.client.send_request.assert_has_calls([ + mock.call('cifs-share-access-control-get-iter', + cifs_share_access_control_get_iter_args)]) + + expected = { + 'Administrator': 'full_control', + 'Administrators': 'change', + 'Power Users': 'read', + 'Users': 'no_access', + } + self.assertDictEqual(expected, result) + + def test_get_cifs_share_access_not_found(self): + + api_response = netapp_api.NaElement(fake.NO_RECORDS_RESPONSE) + self.mock_object(self.client, + 'send_request', + mock.Mock(return_value=api_response)) + + result = self.client.get_cifs_share_access(fake.SHARE_NAME) + + self.assertEqual({}, result) + @ddt.data(True, False) def test_add_cifs_share_access(self, readonly): @@ -3133,6 +3180,26 @@ class NetAppClientCmodeTestCase(test.TestCase): 'cifs-share-access-control-create', cifs_share_access_control_create_args)]) + @ddt.data(True, False) + def test_modify_cifs_share_access(self, readonly): + + self.mock_object(self.client, 'send_request') + + self.client.modify_cifs_share_access(fake.SHARE_NAME, + fake.USER_NAME, + readonly) + + cifs_share_access_control_modify_args = { + 'permission': 'read' if readonly else 'full_control', + 'share': fake.SHARE_NAME, + 'user-or-group': fake.USER_NAME + } + + self.client.send_request.assert_has_calls([ + mock.call( + 'cifs-share-access-control-modify', + cifs_share_access_control_modify_args)]) + def test_remove_cifs_share_access(self): self.mock_object(self.client, 'send_request') diff --git a/manila/tests/share/drivers/netapp/dataontap/cluster_mode/test_lib_base.py b/manila/tests/share/drivers/netapp/dataontap/cluster_mode/test_lib_base.py index d85436821d..85c8165ce0 100644 --- a/manila/tests/share/drivers/netapp/dataontap/cluster_mode/test_lib_base.py +++ b/manila/tests/share/drivers/netapp/dataontap/cluster_mode/test_lib_base.py @@ -1743,10 +1743,10 @@ class NetAppFileStorageLibraryTestCase(test.TestCase): mock_set_volume_size.assert_called_once_with(fake.SHARE_NAME, new_size) - def test_allow_access(self): + def test_update_access(self): protocol_helper = mock.Mock() - protocol_helper.allow_access.return_value = None + protocol_helper.update_access.return_value = None self.mock_object(self.library, '_get_helper', mock.Mock(return_value=protocol_helper)) @@ -1756,42 +1756,16 @@ class NetAppFileStorageLibraryTestCase(test.TestCase): mock.Mock(return_value=(fake.VSERVER1, vserver_client))) - self.library.allow_access(self.context, - fake.SHARE, - fake.SHARE_ACCESS, - share_server=fake.SHARE_SERVER) + self.library.update_access(self.context, + fake.SHARE, + [fake.SHARE_ACCESS], + share_server=fake.SHARE_SERVER) protocol_helper.set_client.assert_called_once_with(vserver_client) - protocol_helper.allow_access.assert_called_once_with( - self.context, + protocol_helper.update_access.assert_called_once_with( fake.SHARE, fake.SHARE_NAME, - fake.SHARE_ACCESS) - - def test_deny_access(self): - - protocol_helper = mock.Mock() - protocol_helper.deny_access.return_value = None - self.mock_object(self.library, - '_get_helper', - mock.Mock(return_value=protocol_helper)) - vserver_client = mock.Mock() - self.mock_object(self.library, - '_get_vserver', - mock.Mock(return_value=(fake.VSERVER1, - vserver_client))) - - self.library.deny_access(self.context, - fake.SHARE, - fake.SHARE_ACCESS, - share_server=fake.SHARE_SERVER) - - protocol_helper.set_client.assert_called_once_with(vserver_client) - protocol_helper.deny_access.assert_called_once_with( - self.context, - fake.SHARE, - fake.SHARE_NAME, - fake.SHARE_ACCESS) + [fake.SHARE_ACCESS]) def test_setup_server(self): self.assertRaises(NotImplementedError, diff --git a/manila/tests/share/drivers/netapp/dataontap/protocols/fakes.py b/manila/tests/share/drivers/netapp/dataontap/protocols/fakes.py index be18b56150..4b276929d2 100644 --- a/manila/tests/share/drivers/netapp/dataontap/protocols/fakes.py +++ b/manila/tests/share/drivers/netapp/dataontap/protocols/fakes.py @@ -49,3 +49,26 @@ USER_ACCESS = { VOLUME = { 'name': SHARE_NAME, } + +NEW_NFS_RULES = { + '10.10.10.0/30': constants.ACCESS_LEVEL_RW, + '10.10.10.0/24': constants.ACCESS_LEVEL_RO, + '10.10.10.10': constants.ACCESS_LEVEL_RW, + '10.10.20.0/24': constants.ACCESS_LEVEL_RW, + '10.10.20.10': constants.ACCESS_LEVEL_RW, +} + +EXISTING_CIFS_RULES = { + 'user1': constants.ACCESS_LEVEL_RW, + 'user2': constants.ACCESS_LEVEL_RO, + 'user3': constants.ACCESS_LEVEL_RW, + 'user4': constants.ACCESS_LEVEL_RO, +} + +NEW_CIFS_RULES = { + 'user1': constants.ACCESS_LEVEL_RW, + 'user2': constants.ACCESS_LEVEL_RW, + 'user3': constants.ACCESS_LEVEL_RO, + 'user5': constants.ACCESS_LEVEL_RW, + 'user6': constants.ACCESS_LEVEL_RO, +} diff --git a/manila/tests/share/drivers/netapp/dataontap/protocols/test_base.py b/manila/tests/share/drivers/netapp/dataontap/protocols/test_base.py index c977e28c5c..f2df31ae72 100644 --- a/manila/tests/share/drivers/netapp/dataontap/protocols/test_base.py +++ b/manila/tests/share/drivers/netapp/dataontap/protocols/test_base.py @@ -15,10 +15,14 @@ Mock unit tests for the NetApp driver protocols base class module. """ +import ddt + +from manila.common import constants from manila.share.drivers.netapp.dataontap.protocols import nfs_cmode from manila import test +@ddt.ddt class NetAppNASHelperBaseTestCase(test.TestCase): def test_set_client(self): @@ -29,3 +33,15 @@ class NetAppNASHelperBaseTestCase(test.TestCase): helper.set_client('fake_client') self.assertEqual('fake_client', helper._client) + + @ddt.data( + {'level': constants.ACCESS_LEVEL_RW, 'readonly': False}, + {'level': constants.ACCESS_LEVEL_RO, 'readonly': True}) + @ddt.unpack + def test_is_readonly(self, level, readonly): + + helper = nfs_cmode.NetAppCmodeNFSHelper() + + result = helper._is_readonly(level) + + self.assertEqual(readonly, result) diff --git a/manila/tests/share/drivers/netapp/dataontap/protocols/test_cifs_cmode.py b/manila/tests/share/drivers/netapp/dataontap/protocols/test_cifs_cmode.py index 7e952fbc36..c6e0fab161 100644 --- a/manila/tests/share/drivers/netapp/dataontap/protocols/test_cifs_cmode.py +++ b/manila/tests/share/drivers/netapp/dataontap/protocols/test_cifs_cmode.py @@ -23,7 +23,6 @@ from oslo_log import log from manila.common import constants from manila import exception -from manila.share.drivers.netapp.dataontap.client import api as netapp_api from manila.share.drivers.netapp.dataontap.protocols import cifs_cmode from manila import test from manila.tests.share.drivers.netapp.dataontap.protocols \ @@ -83,125 +82,115 @@ class NetAppClusteredCIFSHelperTestCase(test.TestCase): self.mock_client.remove_cifs_share.assert_called_once_with( fake.SHARE_NAME) - def test_allow_access(self): + def test_update_access(self): - self.helper.allow_access(self.mock_context, - fake.CIFS_SHARE, - fake.SHARE_NAME, - fake.USER_ACCESS) + mock_validate_access_rule = self.mock_object(self.helper, + '_validate_access_rule') + mock_get_access_rules = self.mock_object( + self.helper, '_get_access_rules', + mock.Mock(return_value=fake.EXISTING_CIFS_RULES)) + mock_handle_added_rules = self.mock_object(self.helper, + '_handle_added_rules') + mock_handle_ro_to_rw_rules = self.mock_object(self.helper, + '_handle_ro_to_rw_rules') + mock_handle_rw_to_ro_rules = self.mock_object(self.helper, + '_handle_rw_to_ro_rules') + mock_handle_deleted_rules = self.mock_object(self.helper, + '_handle_deleted_rules') - self.mock_client.add_cifs_share_access.assert_called_once_with( - fake.SHARE_NAME, fake.USER_ACCESS['access_to'], False) + self.helper.update_access(fake.CIFS_SHARE, + fake.SHARE_NAME, + [fake.USER_ACCESS]) - def test_allow_access_readonly(self): + new_rules = {'fake_user': constants.ACCESS_LEVEL_RW} + mock_validate_access_rule.assert_called_once_with(fake.USER_ACCESS) + mock_get_access_rules.assert_called_once_with(fake.CIFS_SHARE, + fake.SHARE_NAME) + mock_handle_added_rules.assert_called_once_with( + fake.SHARE_NAME, fake.EXISTING_CIFS_RULES, new_rules) + mock_handle_ro_to_rw_rules.assert_called_once_with( + fake.SHARE_NAME, fake.EXISTING_CIFS_RULES, new_rules) + mock_handle_rw_to_ro_rules.assert_called_once_with( + fake.SHARE_NAME, fake.EXISTING_CIFS_RULES, new_rules) + mock_handle_deleted_rules.assert_called_once_with( + fake.SHARE_NAME, fake.EXISTING_CIFS_RULES, new_rules) - user_access = copy.deepcopy(fake.USER_ACCESS) - user_access['access_level'] = constants.ACCESS_LEVEL_RO + def test_validate_access_rule(self): - self.helper.allow_access(self.mock_context, - fake.CIFS_SHARE, - fake.SHARE_NAME, - user_access) + result = self.helper._validate_access_rule(fake.USER_ACCESS) - self.mock_client.add_cifs_share_access.assert_called_once_with( - fake.SHARE_NAME, fake.USER_ACCESS['access_to'], True) + self.assertIsNone(result) - def test_allow_access_preexisting(self): + def test_validate_access_rule_invalid_type(self): - self.mock_client.add_cifs_share_access.side_effect = ( - netapp_api.NaApiError(code=netapp_api.EDUPLICATEENTRY)) + rule = copy.copy(fake.USER_ACCESS) + rule['access_type'] = 'ip' - self.assertRaises(exception.ShareAccessExists, - self.helper.allow_access, - self.mock_context, - fake.CIFS_SHARE, - fake.SHARE_NAME, - fake.USER_ACCESS) + self.assertRaises(exception.InvalidShareAccess, + self.helper._validate_access_rule, + rule) - def test_allow_access_api_error(self): + def test_validate_access_rule_invalid_level(self): - self.mock_client.add_cifs_share_access.side_effect = ( - netapp_api.NaApiError()) - - self.assertRaises(netapp_api.NaApiError, - self.helper.allow_access, - self.mock_context, - fake.CIFS_SHARE, - fake.SHARE_NAME, - fake.USER_ACCESS) - - def test_allow_access_invalid_level(self): - - user_access = copy.deepcopy(fake.USER_ACCESS) - user_access['access_level'] = 'fake_level' + rule = copy.copy(fake.USER_ACCESS) + rule['access_level'] = 'none' self.assertRaises(exception.InvalidShareAccessLevel, - self.helper.allow_access, - self.mock_context, - fake.NFS_SHARE, - fake.SHARE_NAME, - user_access) + self.helper._validate_access_rule, + rule) - def test_allow_access_invalid_type(self): + def test_handle_added_rules(self): - fake_access = fake.USER_ACCESS.copy() - fake_access['access_type'] = 'group' - self.assertRaises(exception.InvalidShareAccess, - self.helper.allow_access, - self.mock_context, - fake.CIFS_SHARE, - fake.SHARE_NAME, - fake_access) + self.helper._handle_added_rules(fake.SHARE_NAME, + fake.EXISTING_CIFS_RULES, + fake.NEW_CIFS_RULES) - def test_deny_access(self): + self.mock_client.add_cifs_share_access.assert_has_calls([ + mock.call(fake.SHARE_NAME, 'user5', False), + mock.call(fake.SHARE_NAME, 'user6', True), + ], any_order=True) - self.helper.deny_access(self.mock_context, - fake.CIFS_SHARE, - fake.SHARE_NAME, - fake.USER_ACCESS) + def test_handle_ro_to_rw_rules(self): - self.mock_client.remove_cifs_share_access.assert_called_once_with( - fake.SHARE_NAME, fake.USER_ACCESS['access_to']) + self.helper._handle_ro_to_rw_rules(fake.SHARE_NAME, + fake.EXISTING_CIFS_RULES, + fake.NEW_CIFS_RULES) - def test_deny_access_nonexistent_user(self): + self.mock_client.modify_cifs_share_access.assert_has_calls([ + mock.call(fake.SHARE_NAME, 'user2', False) + ]) - self.mock_client.remove_cifs_share_access.side_effect = ( - netapp_api.NaApiError(code=netapp_api.EONTAPI_EINVAL)) + def test_handle_rw_to_ro_rules(self): - self.helper.deny_access(self.mock_context, - fake.CIFS_SHARE, - fake.SHARE_NAME, - fake.USER_ACCESS) + self.helper._handle_rw_to_ro_rules(fake.SHARE_NAME, + fake.EXISTING_CIFS_RULES, + fake.NEW_CIFS_RULES) - self.mock_client.remove_cifs_share_access.assert_called_once_with( - fake.SHARE_NAME, fake.USER_ACCESS['access_to']) - self.assertEqual(1, cifs_cmode.LOG.error.call_count) + self.mock_client.modify_cifs_share_access.assert_has_calls([ + mock.call(fake.SHARE_NAME, 'user3', True) + ]) - def test_deny_access_nonexistent_rule(self): + def test_handle_deleted_rules(self): - self.mock_client.remove_cifs_share_access.side_effect = ( - netapp_api.NaApiError(code=netapp_api.EOBJECTNOTFOUND)) + self.helper._handle_deleted_rules(fake.SHARE_NAME, + fake.EXISTING_CIFS_RULES, + fake.NEW_CIFS_RULES) - self.helper.deny_access(self.mock_context, - fake.CIFS_SHARE, - fake.SHARE_NAME, - fake.USER_ACCESS) + self.mock_client.remove_cifs_share_access.assert_has_calls([ + mock.call(fake.SHARE_NAME, 'user4') + ]) - self.mock_client.remove_cifs_share_access.assert_called_once_with( - fake.SHARE_NAME, fake.USER_ACCESS['access_to']) - self.assertEqual(1, cifs_cmode.LOG.error.call_count) + def test_get_access_rules(self): - def test_deny_access_api_error(self): + self.mock_client.get_cifs_share_access = ( + mock.Mock(return_value='fake_rules')) - self.mock_client.remove_cifs_share_access.side_effect = ( - netapp_api.NaApiError()) + result = self.helper._get_access_rules(fake.CIFS_SHARE, + fake.SHARE_NAME) - self.assertRaises(netapp_api.NaApiError, - self.helper.deny_access, - self.mock_context, - fake.CIFS_SHARE, - fake.SHARE_NAME, - fake.USER_ACCESS) + self.assertEqual('fake_rules', result) + self.mock_client.get_cifs_share_access.assert_called_once_with( + fake.SHARE_NAME) def test_get_target(self): diff --git a/manila/tests/share/drivers/netapp/dataontap/protocols/test_nfs_cmode.py b/manila/tests/share/drivers/netapp/dataontap/protocols/test_nfs_cmode.py index c13508b571..477bfbc3ab 100644 --- a/manila/tests/share/drivers/netapp/dataontap/protocols/test_nfs_cmode.py +++ b/manila/tests/share/drivers/netapp/dataontap/protocols/test_nfs_cmode.py @@ -16,10 +16,12 @@ Mock unit tests for the NetApp driver protocols NFS class module. """ import copy +import uuid +import ddt import mock +import netaddr -from manila.common import constants from manila import exception from manila.share.drivers.netapp.dataontap.protocols import nfs_cmode from manila import test @@ -27,6 +29,7 @@ from manila.tests.share.drivers.netapp.dataontap.protocols \ import fakes as fake +@ddt.ddt class NetAppClusteredNFSHelperTestCase(test.TestCase): def setUp(self): @@ -50,8 +53,8 @@ class NetAppClusteredNFSHelperTestCase(test.TestCase): expected = [':'.join([fake.SHARE_ADDRESS_1, fake.NFS_SHARE_PATH])] self.assertEqual(expected, result) - self.mock_client.clear_nfs_export_policy_for_volume.\ - assert_called_once_with(fake.SHARE_NAME) + (self.mock_client.clear_nfs_export_policy_for_volume. + assert_called_once_with(fake.SHARE_NAME)) self.assertTrue(mock_ensure_export_policy.called) def test_create_share_multiple(self): @@ -82,90 +85,86 @@ class NetAppClusteredNFSHelperTestCase(test.TestCase): self.mock_client.soft_delete_nfs_export_policy.assert_called_once_with( fake.EXPORT_POLICY_NAME) - def test_allow_access(self): + def test_update_access(self): - mock_ensure_export_policy = self.mock_object(self.helper, - '_ensure_export_policy') + self.mock_object(self.helper, '_ensure_export_policy') + self.mock_object(self.helper, + '_get_export_policy_name', + mock.Mock(return_value='fake_export_policy')) + self.mock_object(self.helper, + '_get_temp_export_policy_name', + mock.Mock(side_effect=['fake_new_export_policy', + 'fake_old_export_policy'])) - self.helper.allow_access(self.mock_context, - fake.NFS_SHARE, - fake.SHARE_NAME, - fake.IP_ACCESS) + self.helper.update_access(fake.CIFS_SHARE, + fake.SHARE_NAME, + [fake.IP_ACCESS]) - self.assertTrue(mock_ensure_export_policy.called) + self.mock_client.create_nfs_export_policy.assert_called_once_with( + 'fake_new_export_policy') self.mock_client.add_nfs_export_rule.assert_called_once_with( - fake.EXPORT_POLICY_NAME, fake.CLIENT_ADDRESS_1, False) + 'fake_new_export_policy', fake.CLIENT_ADDRESS_1, False) + (self.mock_client.set_nfs_export_policy_for_volume. + assert_called_once_with(fake.SHARE_NAME, 'fake_new_export_policy')) + (self.mock_client.soft_delete_nfs_export_policy. + assert_called_once_with('fake_old_export_policy')) + self.mock_client.rename_nfs_export_policy.assert_has_calls([ + mock.call('fake_export_policy', 'fake_old_export_policy'), + mock.call('fake_new_export_policy', 'fake_export_policy'), + ]) - def test_allow_access_readonly(self): + def test_validate_access_rule(self): - ip_access = copy.deepcopy(fake.IP_ACCESS) - ip_access['access_level'] = constants.ACCESS_LEVEL_RO + result = self.helper._validate_access_rule(fake.IP_ACCESS) - mock_ensure_export_policy = self.mock_object(self.helper, - '_ensure_export_policy') + self.assertIsNone(result) - self.helper.allow_access(self.mock_context, - fake.NFS_SHARE, - fake.SHARE_NAME, - ip_access) + def test_validate_access_rule_invalid_type(self): - self.assertTrue(mock_ensure_export_policy.called) - self.mock_client.add_nfs_export_rule.assert_called_once_with( - fake.EXPORT_POLICY_NAME, fake.CLIENT_ADDRESS_1, True) - - def test_allow_access_invalid_level(self): - - ip_access = copy.deepcopy(fake.IP_ACCESS) - ip_access['access_level'] = 'fake_level' - - self.assertRaises(exception.InvalidShareAccessLevel, - self.helper.allow_access, - self.mock_context, - fake.NFS_SHARE, - fake.SHARE_NAME, - ip_access) - - def test_allow_access_invalid_type(self): - - ip_access = copy.deepcopy(fake.IP_ACCESS) - ip_access['access_type'] = 'user' + rule = copy.copy(fake.IP_ACCESS) + rule['access_type'] = 'user' self.assertRaises(exception.InvalidShareAccess, - self.helper.allow_access, - self.mock_context, - fake.NFS_SHARE, - fake.SHARE_NAME, - ip_access) + self.helper._validate_access_rule, + rule) - def test_deny_access(self): + def test_validate_access_rule_invalid_level(self): - mock_ensure_export_policy = self.mock_object(self.helper, - '_ensure_export_policy') + rule = copy.copy(fake.IP_ACCESS) + rule['access_level'] = 'none' - self.helper.deny_access(self.mock_context, - fake.NFS_SHARE, - fake.SHARE_NAME, - fake.IP_ACCESS) + self.assertRaises(exception.InvalidShareAccessLevel, + self.helper._validate_access_rule, + rule) - self.assertTrue(mock_ensure_export_policy.called) - self.mock_client.remove_nfs_export_rule.assert_called_once_with( - fake.EXPORT_POLICY_NAME, fake.CLIENT_ADDRESS_1) + def test_get_sorted_access_rule_addresses(self): - def test_deny_access_invalid_type(self): + result = self.helper._get_sorted_access_rule_addresses( + fake.NEW_NFS_RULES) - ip_access = copy.deepcopy(fake.IP_ACCESS) - ip_access['access_type'] = 'user' + expected = [ + '10.10.20.10', + '10.10.20.0/24', + '10.10.10.10', + '10.10.10.0/30', + '10.10.10.0/24', + ] + self.assertEqual(expected, result) - mock_ensure_export_policy = self.mock_object(self.helper, - '_ensure_export_policy') + @ddt.data({'rule': '1.2.3.4', 'out': netaddr.IPAddress('1.2.3.4')}, + {'rule': '1.2.3.4/32', 'out': netaddr.IPNetwork('1.2.3.4/32')}) + @ddt.unpack + def test_get_network_object_from_rule(self, rule, out): - self.helper.deny_access(self.mock_context, - fake.NFS_SHARE, - fake.SHARE_NAME, - ip_access) + result = self.helper._get_network_object_from_rule(rule) - self.assertFalse(mock_ensure_export_policy.called) - self.assertFalse(self.mock_client.remove_nfs_export_rule.called) + self.assertEqual(out, result) + + def test_get_network_object_from_rule_invalid(self): + + self.assertRaises(netaddr.AddrFormatError, + self.helper._get_network_object_from_rule, + 'invalid') def test_get_target(self): @@ -215,6 +214,14 @@ class NetAppClusteredNFSHelperTestCase(test.TestCase): self.assertEqual('', host_ip) self.assertEqual('', export_path) + def test_get_temp_export_policy_name(self): + + self.mock_object(uuid, 'uuid1', mock.Mock(return_value='fake-uuid')) + + result = self.helper._get_temp_export_policy_name() + + self.assertEqual('temp_fake_uuid', result) + def test_get_export_policy_name(self): result = self.helper._get_export_policy_name(fake.NFS_SHARE)