From 205ef44283fdcdc0e46200d4533c3e5b015e911c Mon Sep 17 00:00:00 2001 From: Chris Yang Date: Tue, 5 Dec 2017 18:53:10 +0800 Subject: [PATCH] QNAP Manila driver: Access rule setting is override by the later rule setting. When user try to add access rule1 and access rule2, share will first set to the rule1 and then override by the rule2. We expect that share should apply both access rule1 and access rule2. Change-Id: Id77cffb5efe4388e3b66aa85fc89cf6f51d5bd98 Closes-Bug: #1736370 --- manila/share/drivers/qnap/api.py | 51 ++++- manila/share/drivers/qnap/qnap.py | 199 ++++++++++++------ manila/tests/share/drivers/qnap/fakes.py | 23 +- manila/tests/share/drivers/qnap/test_api.py | 106 +++++++++- manila/tests/share/drivers/qnap/test_qnap.py | 145 +++++++++---- ...access-rule-override-1b79b70ae48ad9e6.yaml | 5 + 6 files changed, 413 insertions(+), 116 deletions(-) create mode 100644 releasenotes/notes/bug-1736370-qnap-fix-access-rule-override-1b79b70ae48ad9e6.yaml diff --git a/manila/share/drivers/qnap/api.py b/manila/share/drivers/qnap/api.py index 7d70208568..8a8a355e43 100644 --- a/manila/share/drivers/qnap/api.py +++ b/manila/share/drivers/qnap/api.py @@ -177,7 +177,10 @@ class QnapAPIExecutor(object): for key in params: value = params[key] if value is not None: - sanitized_params[key] = six.text_type(value) + if isinstance(value, list): + sanitized_params[key] = [six.text_type(v) for v in value] + else: + sanitized_params[key] = six.text_type(value) return sanitized_params @_connection_checker @@ -580,6 +583,52 @@ class QnapAPIExecutor(object): if root.find('result').text < '0': raise exception.ShareBackendException(msg=MSG_UNEXPECT_RESP) + @_connection_checker + def edit_host(self, hostname, ipv4_list): + """Execute edit_host API.""" + params = { + 'module': 'hosts', + 'func': 'apply_sethost', + 'name': hostname, + 'ipaddr_v4': ipv4_list, + 'sid': self.sid, + } + sanitized_params = self._sanitize_params(params) + + # urlencode with True parameter to parse ipv4_list + sanitized_params = urllib.parse.urlencode(sanitized_params, True) + url = ('/cgi-bin/accessrights/accessrightsRequest.cgi?%s' % + sanitized_params) + + res_details = self._execute_and_get_response_details(self.ip, url) + root = ET.fromstring(res_details['data']) + if root.find('authPassed').text == '0': + raise exception.ShareBackendException(msg=MSG_SESSION_EXPIRED) + if root.find('result').text < '0': + raise exception.ShareBackendException(msg=MSG_UNEXPECT_RESP) + + @_connection_checker + def delete_host(self, hostname): + """Execute delete_host API.""" + params = { + 'module': 'hosts', + 'func': 'apply_delhost', + 'host_name': hostname, + 'sid': self.sid, + } + sanitized_params = self._sanitize_params(params) + + sanitized_params = urllib.parse.urlencode(sanitized_params) + url = ('/cgi-bin/accessrights/accessrightsRequest.cgi?%s' % + sanitized_params) + + res_details = self._execute_and_get_response_details(self.ip, url) + root = ET.fromstring(res_details['data']) + if root.find('authPassed').text == '0': + raise exception.ShareBackendException(msg=MSG_SESSION_EXPIRED) + if root.find('result').text < '0': + raise exception.ShareBackendException(msg=MSG_UNEXPECT_RESP) + @_connection_checker def set_nfs_access(self, sharename, access, host_name): """Execute set_nfs_access API.""" diff --git a/manila/share/drivers/qnap/qnap.py b/manila/share/drivers/qnap/qnap.py index fedde37c10..ec4ab1b3eb 100644 --- a/manila/share/drivers/qnap/qnap.py +++ b/manila/share/drivers/qnap/qnap.py @@ -16,7 +16,9 @@ Share driver for QNAP Storage. This driver supports QNAP Storage for NFS. """ +import datetime import re +import time from oslo_config import cfg from oslo_log import log as logging @@ -62,9 +64,11 @@ class QnapShareDriver(driver.ShareDriver): Version history: 1.0.0 - Initial driver (Only NFS) 1.0.1 - Add support for QES fw 1.1.4. + 1.0.2 - Fix bug #1736370, QNAP Manila driver: Access rule setting is + override by the another access rule. """ - DRIVER_VERSION = '1.0.1' + DRIVER_VERSION = '1.0.2' def __init__(self, *args, **kwargs): """Initialize QnapShareDriver.""" @@ -193,6 +197,16 @@ class QnapShareDriver(driver.ShareDriver): {'ifx': infix, 'time': timeutils.utcnow().strftime('%Y%m%d%H%M%S%f')}) + def _gen_host_name(self, vol_name_timestamp, access_level): + # host_name will be manila-{vol_name_timestamp}-ro or + # manila-{vol_name_timestamp}-rw + return 'manila-{}-{}'.format(vol_name_timestamp, access_level) + + def _get_timestamp_from_vol_name(self, vol_name): + vol_name_split = vol_name.split('-') + dt = datetime.datetime.strptime(vol_name_split[2], '%Y%m%d%H%M%S%f') + return int(time.mktime(dt.timetuple())) + def _get_location_path(self, share_name, share_proto, ip, vol_id): if share_proto == 'NFS': vol = self.api_executor.get_specific_volinfo(vol_id) @@ -503,30 +517,31 @@ class QnapShareDriver(driver.ShareDriver): self.configuration.qnap_share_ip, create_volID) - def _get_manila_hostIPv4s(self, hostlist): - host_dict_IPs = [] - if hostlist is None: - return host_dict_IPs - for host in hostlist: - # Check host alias name with prefix "manila-hst-" to verify this - # host is created/managed by Manila or not. - if (re.match("^manila-hst-[0-9]+$", host.find('name').text) - is not None): - LOG.debug('host netaddrs text: %s', host.find('netaddrs').text) - if host.find('netaddrs').text is not None: - # Because Manila supports only IPv4 now, check "netaddrs" - # have "ipv4" tag to verify this host is created/managed - # by Manila or not. - if host.find('netaddrs/ipv4').text is not None: - host_dict = { - 'index': host.find('index').text, - 'hostid': host.find('hostid').text, - 'name': host.find('name').text, - 'netaddrs': host.find('netaddrs').find('ipv4').text - } - host_dict_IPs.append(host_dict) - return host_dict_IPs + def _get_vol_host(self, host_list, vol_name_timestamp): + vol_host_list = [] + if host_list is None: + return vol_host_list + for host in host_list: + # Check host alias name with prefix "manila-{vol_name_timestamp}" + # to find the host of this manila share. + LOG.debug('_get_vol_host name:%s', host.find('name').text) + # Because driver supports only IPv4 now, check "netaddrs" + # have "ipv4" tag to get address. + if re.match("^manila-{}".format(vol_name_timestamp), + host.find('name').text): + host_dict = { + 'index': host.find('index').text, + 'hostid': host.find('hostid').text, + 'name': host.find('name').text, + 'ipv4': [], + } + for ipv4 in host.findall('netaddrs/ipv4'): + host_dict['ipv4'].append(ipv4.text) + vol_host_list.append(host_dict) + LOG.debug('_get_vol_host vol_host_list:%s', vol_host_list) + return vol_host_list + @utils.synchronized('qnap-update_access') def update_access(self, context, share, access_rules, add_rules, delete_rules, share_server=None): if not (add_rules or delete_rules): @@ -540,6 +555,15 @@ class QnapShareDriver(driver.ShareDriver): # Clear all current ACLs self.api_executor.set_nfs_access(volName, 2, "all") + vol_name_timestamp = self._get_timestamp_from_vol_name(volName) + host_list = self.api_executor.get_host_list() + LOG.debug('host_list:%s', host_list) + vol_host_list = self._get_vol_host(host_list, vol_name_timestamp) + # If host already exist, delete the host + if len(vol_host_list) > 0: + for vol_host in vol_host_list: + self.api_executor.delete_host(vol_host['name']) + # Add each one through all rules. for access in access_rules: self._allow_access(context, share, access, share_server) @@ -556,45 +580,71 @@ class QnapShareDriver(driver.ShareDriver): access_type = access['access_type'] access_level = access['access_level'] access_to = access['access_to'] + LOG.debug('share_proto: %(share_proto)s ' + 'access_type: %(access_type)s' + 'access_level: %(access_level)s' + 'access_to: %(access_to)s', + {'share_proto': share_proto, + 'access_type': access_type, + 'access_level': access_level, + 'access_to': access_to}) self._check_share_access(share_proto, access_type) - hostlist = self.api_executor.get_host_list() - host_dict_IPs = self._get_manila_hostIPv4s(hostlist) - LOG.debug('host_dict_IPs: %s', host_dict_IPs) - if len(host_dict_IPs) == 0: - host_name = self._gen_random_name("host") + vol_name = self.private_storage.get(share['id'], 'volName') + vol_name_timestamp = self._get_timestamp_from_vol_name(vol_name) + host_name = self._gen_host_name(vol_name_timestamp, access_level) + + host_list = self.api_executor.get_host_list() + LOG.debug('vol_name: %(vol_name)s ' + 'access_level: %(access_level)s ' + 'host_name: %(host_name)s ' + 'host_list: %(host_list)s ', + {'vol_name': vol_name, + 'access_level': access_level, + 'host_name': host_name, + 'host_list': host_list}) + filter_host_list = self._get_vol_host(host_list, vol_name_timestamp) + if len(filter_host_list) == 0: + # if host does not exist, create a host for the share self.api_executor.add_host(host_name, access_to) + elif (len(filter_host_list) == 1 and + filter_host_list[0]['name'] == host_name): + # if the host exist, and this host is for the same access right, + # add ip to the host. + ipv4_list = filter_host_list[0]['ipv4'] + if access_to not in ipv4_list: + ipv4_list.append(access_to) + LOG.debug('vol_host["ipv4"]: %s', filter_host_list[0]['ipv4']) + LOG.debug('ipv4_list: %s', ipv4_list) + self.api_executor.edit_host(host_name, ipv4_list) else: - for host in host_dict_IPs: - LOG.debug('host[netaddrs]: %s', host['netaddrs']) - LOG.debug('access_to: %s', access_to) - if host['netaddrs'] == access_to: - LOG.debug('in match ip') - host_name = host['name'] - break - if host is host_dict_IPs[-1]: - host_name = self._gen_random_name("host") - self.api_executor.add_host(host_name, access_to) - - volName = self.private_storage.get(share['id'], 'volName') - LOG.debug('volName: %(volName)s for share: %(share)s', - {'volName': volName, 'share': share['id']}) - - LOG.debug('access_level: %(access)s for share: %(share)s', - {'access': access_level, 'share': share['id']}) - LOG.debug('host_name: %(host)s for share: %(share)s', - {'host': host_name, 'share': share['id']}) - if access_level == constants.ACCESS_LEVEL_RO: - self.api_executor.set_nfs_access(volName, 1, host_name) - elif access_level == constants.ACCESS_LEVEL_RW: - self.api_executor.set_nfs_access(volName, 0, host_name) + # Until now, share of QNAP NAS can only apply one access level for + # all ips. "rw" for some ips and "ro" for else is not allowed. + support_level = (constants.ACCESS_LEVEL_RW if + access_level == constants.ACCESS_LEVEL_RO + else constants.ACCESS_LEVEL_RO) + reason = _('Share only supports one access ' + 'level: %s') % support_level + LOG.error(reason) + raise exception.InvalidShareAccess(reason=reason) + access = 1 if access_level == constants.ACCESS_LEVEL_RO else 0 + self.api_executor.set_nfs_access(vol_name, access, host_name) def _deny_access(self, context, share, access, share_server=None): """Deny access to the share.""" share_proto = share['share_proto'] access_type = access['access_type'] + access_level = access['access_level'] access_to = access['access_to'] + LOG.debug('share_proto: %(share_proto)s ' + 'access_type: %(access_type)s' + 'access_level: %(access_level)s' + 'access_to: %(access_to)s', + {'share_proto': share_proto, + 'access_type': access_type, + 'access_level': access_level, + 'access_to': access_to}) try: self._check_share_access(share_proto, access_type) @@ -602,23 +652,34 @@ class QnapShareDriver(driver.ShareDriver): LOG.warning('The denied rule is invalid and does not exist.') return - hostlist = self.api_executor.get_host_list() - host_dict_IPs = self._get_manila_hostIPv4s(hostlist) - LOG.debug('host_dict_IPs: %s', host_dict_IPs) - if len(host_dict_IPs) == 0: - return - else: - for host in host_dict_IPs: - if (host['netaddrs'] == access_to): - host_name = host['name'] - break - if (host is host_dict_IPs[-1]): - return - - volName = self.private_storage.get(share['id'], 'volName') - LOG.debug('volName: %s', volName) - - self.api_executor.set_nfs_access(volName, 2, host_name) + vol_name = self.private_storage.get(share['id'], 'volName') + vol_name_timestamp = self._get_timestamp_from_vol_name(vol_name) + host_name = self._gen_host_name(vol_name_timestamp, access_level) + host_list = self.api_executor.get_host_list() + LOG.debug('vol_name: %(vol_name)s ' + 'access_level: %(access_level)s ' + 'host_name: %(host_name)s ' + 'host_list: %(host_list)s ', + {'vol_name': vol_name, + 'access_level': access_level, + 'host_name': host_name, + 'host_list': host_list}) + filter_host_list = self._get_vol_host(host_list, vol_name_timestamp) + # if share already have host, remove ip from host + for vol_host in filter_host_list: + if vol_host['name'] == host_name: + ipv4_list = vol_host['ipv4'] + if access_to in ipv4_list: + ipv4_list.remove(access_to) + LOG.debug('vol_host["ipv4"]: %s', vol_host['ipv4']) + LOG.debug('ipv4_list: %s', ipv4_list) + if len(ipv4_list) == 0: # if list empty, remove the host + self.api_executor.set_nfs_access( + vol_name, 2, host_name) + self.api_executor.delete_host(host_name) + else: + self.api_executor.edit_host(host_name, ipv4_list) + break def _check_share_access(self, share_proto, access_type): if share_proto == 'NFS' and access_type != 'ip': diff --git a/manila/tests/share/drivers/qnap/fakes.py b/manila/tests/share/drivers/qnap/fakes.py index 22b514d1f0..90fa364136 100644 --- a/manila/tests/share/drivers/qnap/fakes.py +++ b/manila/tests/share/drivers/qnap/fakes.py @@ -192,7 +192,7 @@ FAKE_RES_DETAIL_DATA_GET_HOST_LIST = """ - + @@ -287,6 +287,15 @@ FAKE_RES_DETAIL_DATA_GET_HOST_LIST_API = """ """ +FAKE_RES_DETAIL_DATA_GET_NO_HOST_LIST_API = """ + + + + + + + """ + FAKE_RES_DETAIL_DATA_CREATE_SNAPSHOT = """ @@ -546,7 +555,7 @@ class FakeDeleteSnapshotResponseShareNotExist(object): class FakeGetHostListResponse(object): - """Fake pool info response.""" + """Fake host info response.""" status = 'fackStatus' @@ -555,6 +564,16 @@ class FakeGetHostListResponse(object): return FAKE_RES_DETAIL_DATA_GET_HOST_LIST_API +class FakeGetNoHostListResponse(object): + """Fake host info response.""" + + status = 'fackStatus' + + def read(self): + """Mock response.read.""" + return FAKE_RES_DETAIL_DATA_GET_NO_HOST_LIST_API + + class FakeAuthPassFailResponse(object): """Fake pool info response.""" diff --git a/manila/tests/share/drivers/qnap/test_api.py b/manila/tests/share/drivers/qnap/test_api.py index 693c7e038c..19c404a86c 100644 --- a/manila/tests/share/drivers/qnap/test_api.py +++ b/manila/tests/share/drivers/qnap/test_api.py @@ -91,14 +91,17 @@ class QnapAPITestCase(QnapShareDriverBaseTestCase): host='QnapShareDriver', size=10) - def _sanitize_params(self, params): + def _sanitize_params(self, params, doseq=False): sanitized_params = {} for key in params: value = params[key] if value is not None: - sanitized_params[key] = six.text_type(value) + if isinstance(value, list): + sanitized_params[key] = [six.text_type(v) for v in value] + else: + sanitized_params[key] = six.text_type(value) - sanitized_params = urllib.parse.urlencode(sanitized_params) + sanitized_params = urllib.parse.urlencode(sanitized_params, doseq) return sanitized_params @ddt.data('fake_share_name', 'fakeLabel') @@ -493,14 +496,16 @@ class QnapAPITestCase(QnapShareDriverBaseTestCase): expected_call_list, mock_http_connection.return_value.request.call_args_list) - def test_get_host_list(self): + @ddt.data(fakes.FakeGetHostListResponse(), + fakes.FakeGetNoHostListResponse()) + def test_get_host_list(self, fakeGetHostListResponse): """Test get host list api.""" mock_http_connection = six.moves.http_client.HTTPConnection mock_http_connection.return_value.getresponse.side_effect = [ fakes.FakeLoginResponse(), fakes.FakeGetBasicInfoResponseEs_1_1_3(), fakes.FakeLoginResponse(), - fakes.FakeGetHostListResponse()] + fakeGetHostListResponse] self._do_setup('http://1.2.3.4:8080', '1.2.3.4', 'admin', 'qnapadmin', 'Storage Pool 1') @@ -560,8 +565,8 @@ class QnapAPITestCase(QnapShareDriverBaseTestCase): expected_call_list, mock_http_connection.return_value.request.call_args_list) - def test_set_nfs_access(self): - """Test get host list api.""" + def test_edit_host(self): + """Test edit host api.""" mock_http_connection = six.moves.http_client.HTTPConnection mock_http_connection.return_value.getresponse.side_effect = [ fakes.FakeLoginResponse(), @@ -569,6 +574,75 @@ class QnapAPITestCase(QnapShareDriverBaseTestCase): fakes.FakeLoginResponse(), fakes.FakeGetHostListResponse()] + self._do_setup('http://1.2.3.4:8080', '1.2.3.4', 'admin', + 'qnapadmin', 'Storage Pool 1') + self.driver.api_executor.edit_host( + 'fakeHostName', ['fakeIpV4']) + + fake_params = { + 'module': 'hosts', + 'func': 'apply_sethost', + 'name': 'fakeHostName', + 'ipaddr_v4': ['fakeIpV4'], + 'sid': 'fakeSid', + } + sanitized_params = self._sanitize_params(fake_params, doseq=True) + fake_url = ( + ('/cgi-bin/accessrights/accessrightsRequest.cgi?%s') % + sanitized_params) + + expected_call_list = [ + mock.call('GET', self.login_url), + mock.call('GET', self.get_basic_info_url), + mock.call('GET', self.login_url), + mock.call('GET', fake_url)] + self.assertEqual( + expected_call_list, + mock_http_connection.return_value.request.call_args_list) + + def test_delete_host(self): + """Test delete host api.""" + mock_http_connection = six.moves.http_client.HTTPConnection + mock_http_connection.return_value.getresponse.side_effect = [ + fakes.FakeLoginResponse(), + fakes.FakeGetBasicInfoResponseEs_1_1_3(), + fakes.FakeLoginResponse(), + fakes.FakeGetHostListResponse()] + + self._do_setup('http://1.2.3.4:8080', '1.2.3.4', 'admin', + 'qnapadmin', 'Storage Pool 1') + self.driver.api_executor.delete_host('fakeHostName') + + fake_params = { + 'module': 'hosts', + 'func': 'apply_delhost', + 'host_name': 'fakeHostName', + 'sid': 'fakeSid', + } + sanitized_params = self._sanitize_params(fake_params) + fake_url = ( + ('/cgi-bin/accessrights/accessrightsRequest.cgi?%s') % + sanitized_params) + + expected_call_list = [ + mock.call('GET', self.login_url), + mock.call('GET', self.get_basic_info_url), + mock.call('GET', self.login_url), + mock.call('GET', fake_url)] + self.assertEqual( + expected_call_list, + mock_http_connection.return_value.request.call_args_list) + + @ddt.data(fakes.FakeGetHostListResponse()) + def test_set_nfs_access(self, fakeGetHostListResponse): + """Test get host list api.""" + mock_http_connection = six.moves.http_client.HTTPConnection + mock_http_connection.return_value.getresponse.side_effect = [ + fakes.FakeLoginResponse(), + fakes.FakeGetBasicInfoResponseEs_1_1_3(), + fakes.FakeLoginResponse(), + fakeGetHostListResponse] + self._do_setup('http://1.2.3.4:8080', '1.2.3.4', 'admin', 'qnapadmin', 'Storage Pool 1') self.driver.api_executor.set_nfs_access( @@ -749,6 +823,24 @@ class QnapAPITestCase(QnapShareDriverBaseTestCase): 'ipv4': 'fakeIpV4'}, fakes.FakeAuthPassFailResponse(), fakes.FakeGetBasicInfoResponseEs_1_1_3()], + ['self.driver.api_executor.edit_host', + {'hostname': 'fakeHostName', + 'ipv4_list': 'fakeIpV4List'}, + fakes.FakeResultNegativeResponse(), + fakes.FakeGetBasicInfoResponseEs_1_1_3()], + ['self.driver.api_executor.edit_host', + {'hostname': 'fakeHostName', + 'ipv4_list': 'fakeIpV4List'}, + fakes.FakeAuthPassFailResponse(), + fakes.FakeGetBasicInfoResponseEs_1_1_3()], + ['self.driver.api_executor.delete_host', + {'hostname': 'fakeHostName'}, + fakes.FakeResultNegativeResponse(), + fakes.FakeGetBasicInfoResponseEs_1_1_3()], + ['self.driver.api_executor.delete_host', + {'hostname': 'fakeHostName'}, + fakes.FakeAuthPassFailResponse(), + fakes.FakeGetBasicInfoResponseEs_1_1_3()], ['self.driver.api_executor.get_host_list', {}, fakes.FakeResultNegativeResponse(), diff --git a/manila/tests/share/drivers/qnap/test_qnap.py b/manila/tests/share/drivers/qnap/test_qnap.py index 41b81f3da5..e5a861fc4f 100644 --- a/manila/tests/share/drivers/qnap/test_qnap.py +++ b/manila/tests/share/drivers/qnap/test_qnap.py @@ -721,15 +721,22 @@ class QnapShareDriverTestCase(QnapShareDriverBaseTestCase): snapshot=fake_snapshot, share_server=None) + @mock.patch.object(qnap.QnapShareDriver, '_get_timestamp_from_vol_name') @mock.patch.object(qnap.QnapShareDriver, '_allow_access') + @ddt.data('fakeHostName', 'fakeHostNameNotMatch') def test_update_access_allow_access( - self, mock_allow_access): + self, fakeHostName, mock_allow_access, + mock_get_timestamp_from_vol_name): """Test update access with allow access rules.""" mock_private_storage = mock.Mock() mock_private_storage.get.return_value = 'fakeVolName' mock_api_executor = qnap.QnapShareDriver._create_api_executor + mock_api_executor.return_value.get_host_list.return_value = ( + self.get_host_list_return_value()) mock_api_executor.return_value.set_nfs_access.return_value = None + mock_api_executor.return_value.delete_host.return_value = None mock_allow_access.return_value = None + mock_get_timestamp_from_vol_name.return_value = fakeHostName self._do_setup('http://1.2.3.4:8080', '1.2.3.4', 'admin', 'qnapadmin', 'Storage Pool 1', @@ -1044,7 +1051,7 @@ class QnapShareDriverTestCase(QnapShareDriverBaseTestCase): mock_api_return.get_specific_poolinfo.assert_called_once_with( self.driver.configuration.qnap_poolname) - def test_get_manila_host_ipv4s(self): + def test_get_vol_host(self): """Test get manila host IPV4s.""" mock_private_storage = mock.Mock() @@ -1059,28 +1066,31 @@ class QnapShareDriverTestCase(QnapShareDriverBaseTestCase): 'index': host.find('index').text, 'hostid': host.find('hostid').text, 'name': host.find('name').text, - 'netaddrs': host.find('netaddrs').find('ipv4').text + 'ipv4': [host.find('netaddrs').find('ipv4').text] } expect_host_dict_ips.append(host_dict) self.assertEqual( - expect_host_dict_ips, self.driver._get_manila_hostIPv4s( - host_list)) + expect_host_dict_ips, self.driver._get_vol_host( + host_list, 'fakeHostName')) - @mock.patch.object(qnap.QnapShareDriver, '_gen_random_name') + @mock.patch.object(qnap.QnapShareDriver, '_gen_host_name') + @mock.patch.object(qnap.QnapShareDriver, '_get_timestamp_from_vol_name') @mock.patch.object(qnap.QnapShareDriver, '_check_share_access') def test_allow_access_ro( self, mock_check_share_access, - mock_gen_random_name): + mock_get_timestamp_from_vol_name, + mock_gen_host_name): """Test allow_access with access type ro.""" fake_access = fakes.AccessClass('fakeAccessType', 'ro', 'fakeIp') mock_private_storage = mock.Mock() + mock_private_storage.get.return_value = 'fakeVolName' mock_api_executor = qnap.QnapShareDriver._create_api_executor - mock_api_executor.return_value.get_host_list.return_value = ( - self.get_host_list_return_value()) - mock_gen_random_name.return_value = 'fakeHostName' + mock_api_executor.return_value.get_host_list.return_value = [] + mock_get_timestamp_from_vol_name.return_value = 'fakeHostName' + mock_gen_host_name.return_value = 'manila-fakeHostName-ro' mock_api_executor.return_value.add_host.return_value = None mock_api_executor.return_value.set_nfs_access.return_value = None @@ -1092,14 +1102,17 @@ class QnapShareDriverTestCase(QnapShareDriverBaseTestCase): mock_check_share_access.assert_called_once_with( 'NFS', 'fakeAccessType') - mock_gen_random_name.assert_called_once_with('host') mock_api_executor.return_value.add_host.assert_called_once_with( - 'fakeHostName', 'fakeIp') + 'manila-fakeHostName-ro', 'fakeIp') + @mock.patch.object(qnap.QnapShareDriver, '_gen_host_name') + @mock.patch.object(qnap.QnapShareDriver, '_get_timestamp_from_vol_name') @mock.patch.object(qnap.QnapShareDriver, '_check_share_access') def test_allow_access_ro_with_hostlist( self, - mock_check_share_access): + mock_check_share_access, + mock_get_timestamp_from_vol_name, + mock_gen_host_name): """Test allow_access_ro_with_hostlist.""" host_dict_ips = [] for host in self.get_host_list_return_value(): @@ -1108,18 +1121,21 @@ class QnapShareDriverTestCase(QnapShareDriverBaseTestCase): 'index': host.find('index').text, 'hostid': host.find('hostid').text, 'name': host.find('name').text, - 'netaddrs': host.find('netaddrs').find('ipv4').text} + 'ipv4': [host.find('netaddrs').find('ipv4').text]} host_dict_ips.append(host_dict) for host in host_dict_ips: - fake_access_to = host['netaddrs'] + fake_access_to = host['ipv4'] fake_access = fakes.AccessClass( 'fakeAccessType', 'ro', fake_access_to) mock_private_storage = mock.Mock() + mock_private_storage.get.return_value = 'fakeVolName' mock_api_executor = qnap.QnapShareDriver._create_api_executor mock_api_executor.return_value.get_host_list.return_value = ( self.get_host_list_return_value()) + mock_get_timestamp_from_vol_name.return_value = 'fakeHostName' + mock_gen_host_name.return_value = 'manila-fakeHostName' mock_api_executor.return_value.set_nfs_access.return_value = None self._do_setup('http://1.2.3.4:8080', '1.2.3.4', 'admin', @@ -1131,20 +1147,64 @@ class QnapShareDriverTestCase(QnapShareDriverBaseTestCase): mock_check_share_access.assert_called_once_with( 'NFS', 'fakeAccessType') - @mock.patch.object(qnap.QnapShareDriver, '_gen_random_name') + @mock.patch.object(qnap.QnapShareDriver, '_gen_host_name') + @mock.patch.object(qnap.QnapShareDriver, '_get_timestamp_from_vol_name') + @mock.patch.object(qnap.QnapShareDriver, '_check_share_access') + def test_allow_access_rw_with_hostlist_invalid_access( + self, + mock_check_share_access, + mock_get_timestamp_from_vol_name, + mock_gen_host_name): + """Test allow_access_rw_invalid_access.""" + host_dict_ips = [] + for host in self.get_host_list_return_value(): + if host.find('netaddrs/ipv4').text is not None: + host_dict = { + 'index': host.find('index').text, + 'hostid': host.find('hostid').text, + 'name': host.find('name').text, + 'ipv4': [host.find('netaddrs').find('ipv4').text]} + host_dict_ips.append(host_dict) + + for host in host_dict_ips: + fake_access_to = host['ipv4'] + fake_access = fakes.AccessClass( + 'fakeAccessType', 'rw', fake_access_to) + + mock_private_storage = mock.Mock() + mock_private_storage.get.return_value = 'fakeVolName' + mock_api_executor = qnap.QnapShareDriver._create_api_executor + mock_api_executor.return_value.get_host_list.return_value = ( + self.get_host_list_return_value()) + mock_get_timestamp_from_vol_name.return_value = 'fakeHostName' + mock_gen_host_name.return_value = 'manila-fakeHostName-rw' + + self._do_setup('http://1.2.3.4:8080', '1.2.3.4', 'admin', + 'qnapadmin', 'Storage Pool 1', + private_storage=mock_private_storage) + + self.assertRaises( + exception.InvalidShareAccess, + self.driver._allow_access, + context='context', + share=self.share, + access=fake_access, + share_server=None) + + @mock.patch.object(qnap.QnapShareDriver, '_get_timestamp_from_vol_name') @mock.patch.object(qnap.QnapShareDriver, '_check_share_access') def test_allow_access_rw( self, mock_check_share_access, - mock_gen_random_name): + mock_get_timestamp_from_vol_name): """Test allow_access with access type rw.""" fake_access = fakes.AccessClass('fakeAccessType', 'rw', 'fakeIp') mock_private_storage = mock.Mock() + mock_private_storage.get.return_value = 'fakeVolName' mock_api_executor = qnap.QnapShareDriver._create_api_executor - mock_api_executor.return_value.get_host_list.return_value = ( - self.get_host_list_return_value()) - mock_gen_random_name.return_value = 'fakeHostName' + mock_api_executor.return_value.get_host_list.return_value = [] + mock_get_timestamp_from_vol_name.return_value = 'fakeHostName' mock_api_executor.return_value.add_host.return_value = None mock_api_executor.return_value.set_nfs_access.return_value = None @@ -1156,44 +1216,50 @@ class QnapShareDriverTestCase(QnapShareDriverBaseTestCase): mock_check_share_access.assert_called_once_with( 'NFS', 'fakeAccessType') - mock_gen_random_name.assert_called_once_with('host') mock_api_executor.return_value.add_host.assert_called_once_with( - 'fakeHostName', 'fakeIp') + 'manila-fakeHostName-rw', 'fakeIp') - @mock.patch.object(qnap.QnapShareDriver, '_get_manila_hostIPv4s') - @mock.patch.object(qnap.QnapShareDriver, '_gen_random_name') + @mock.patch.object(qnap.QnapShareDriver, '_gen_host_name') @mock.patch.object(qnap.QnapShareDriver, '_check_share_access') - def test_allow_access_without_hostlist( + def test_allow_access_ro_without_hostlist( self, mock_check_share_access, - mock_gen_random_name, - mock_get_manila_hostipv4s): + mock_gen_host_name): """Test allow access without host list.""" fake_access = fakes.AccessClass('fakeAccessType', 'ro', 'fakeIp') mock_private_storage = mock.Mock() + mock_api_executor = qnap.QnapShareDriver._create_api_executor mock_api_executor.return_value.get_host_list.return_value = None - mock_gen_random_name.return_value = 'fakeHostName' + mock_gen_host_name.return_value = 'fakeHostName' mock_api_executor.return_value.add_host.return_value = None mock_api_executor.return_value.set_nfs_access.return_value = None self._do_setup('http://1.2.3.4:8080', '1.2.3.4', 'admin', 'qnapadmin', 'Storage Pool 1', private_storage=mock_private_storage) + share_name = self.driver._gen_random_name('share') + mock_private_storage.get.return_value = share_name self.driver._allow_access( 'context', self.share, fake_access, share_server=None) mock_check_share_access.assert_called_once_with( 'NFS', 'fakeAccessType') - mock_gen_random_name.assert_called_once_with('host') mock_api_executor.return_value.add_host.assert_called_once_with( 'fakeHostName', 'fakeIp') + @mock.patch.object(qnap.QnapShareDriver, '_get_vol_host') + @mock.patch.object(qnap.QnapShareDriver, '_gen_host_name') + @mock.patch.object(qnap.QnapShareDriver, '_get_timestamp_from_vol_name') @mock.patch.object(qnap.QnapShareDriver, '_check_share_access') def test_deny_access_with_hostlist( self, - mock_check_share_access): + mock_check_share_access, + mock_get_timestamp_from_vol_name, + mock_gen_host_name, + mock_get_vol_host): + """Test deny access.""" host_dict_ips = [] for host in self.get_host_list_return_value(): @@ -1202,11 +1268,11 @@ class QnapShareDriverTestCase(QnapShareDriverBaseTestCase): 'index': host.find('index').text, 'hostid': host.find('hostid').text, 'name': host.find('name').text, - 'netaddrs': host.find('netaddrs').find('ipv4').text} + 'ipv4': [host.find('netaddrs').find('ipv4').text]} host_dict_ips.append(host_dict) for host in host_dict_ips: - fake_access_to = host['netaddrs'] + fake_access_to = host['ipv4'][0] fake_access = fakes.AccessClass('fakeAccessType', 'ro', fake_access_to) mock_private_storage = mock.Mock() @@ -1216,6 +1282,9 @@ class QnapShareDriverTestCase(QnapShareDriverBaseTestCase): qnap.QnapShareDriver._create_api_executor.return_value) mock_api_return.get_host_list.return_value = ( self.get_host_list_return_value()) + mock_get_timestamp_from_vol_name.return_value = 'fakeTimeStamp' + mock_gen_host_name.return_value = 'manila-fakeHostName' + mock_get_vol_host.return_value = host_dict_ips mock_api_return.add_host.return_value = None mock_api_return.set_nfs_access.return_value = None @@ -1227,13 +1296,13 @@ class QnapShareDriverTestCase(QnapShareDriverBaseTestCase): mock_check_share_access.assert_called_once_with( 'NFS', 'fakeAccessType') - mock_api_return.set_nfs_access.assert_called_once_with( - 'vol_name', 2, 'manila-hst-123') + @mock.patch.object(qnap.QnapShareDriver, '_get_timestamp_from_vol_name') @mock.patch.object(qnap.QnapShareDriver, '_check_share_access') def test_deny_access_with_hostlist_not_equel_access_to( self, - mock_check_share_access): + mock_check_share_access, + mock_get_timestamp_from_vol_name): """Test deny access.""" fake_access = fakes.AccessClass('fakeAccessType', 'ro', 'fakeIp') @@ -1254,18 +1323,20 @@ class QnapShareDriverTestCase(QnapShareDriverBaseTestCase): mock_check_share_access.assert_called_once_with( 'NFS', 'fakeAccessType') - @mock.patch.object(qnap.QnapShareDriver, '_get_manila_hostIPv4s') + @mock.patch.object(qnap.QnapShareDriver, '_get_timestamp_from_vol_name') @mock.patch.object(qnap.QnapShareDriver, '_check_share_access') def test_deny_access_without_hostlist( self, mock_check_share_access, - mock_get_manila_hostipv4s): + mock_get_timestamp_from_vol_name): """Test deny access without hostlist.""" fake_access = fakes.AccessClass('fakeAccessType', 'ro', 'fakeIp') mock_private_storage = mock.Mock() + mock_private_storage.get.return_value = 'fakeVolName' mock_api_executor = qnap.QnapShareDriver._create_api_executor mock_api_executor.return_value.get_host_list.return_value = None + mock_get_timestamp_from_vol_name.return_value = 'fakeHostName' mock_api_executor.return_value.add_host.return_value = None mock_api_executor.return_value.set_nfs_access.return_value = None diff --git a/releasenotes/notes/bug-1736370-qnap-fix-access-rule-override-1b79b70ae48ad9e6.yaml b/releasenotes/notes/bug-1736370-qnap-fix-access-rule-override-1b79b70ae48ad9e6.yaml new file mode 100644 index 0000000000..6b0a55be74 --- /dev/null +++ b/releasenotes/notes/bug-1736370-qnap-fix-access-rule-override-1b79b70ae48ad9e6.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - | + Fixed the QNAP driver that the access rule setting is overridden by the later + access rule setting.