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

@@ -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."""

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):
+    def _get_vol_host(self, host_list, vol_name_timestamp):
-        host_dict_IPs = []
+        vol_host_list = []
-        if hostlist is None:
+        if host_list is None:
-            return host_dict_IPs
+            return vol_host_list
-        for host in hostlist:
+        for host in host_list:
-            # Check host alias name with prefix "manila-hst-" to verify this
+            # Check host alias name with prefix "manila-{vol_name_timestamp}"
-            # host is created/managed by Manila or not.
+            # to find the host of this manila share.
-            if (re.match("^manila-hst-[0-9]+$", host.find('name').text)
+            LOG.debug('_get_vol_host name:%s', host.find('name').text)
-               is not None):
+            # Because driver supports only IPv4 now, check "netaddrs"
-                LOG.debug('host netaddrs text: %s', host.find('netaddrs').text)
+            # have "ipv4" tag to get address.
-                if host.find('netaddrs').text is not None:
+            if re.match("^manila-{}".format(vol_name_timestamp),
-                    # Because Manila supports only IPv4 now, check "netaddrs"
+                        host.find('name').text):
-                    # have "ipv4" tag to verify this host is created/managed
+                host_dict = {
-                    # by Manila or not.
+                    'index': host.find('index').text,
-                    if host.find('netaddrs/ipv4').text is not None:
+                    'hostid': host.find('hostid').text,
-                        host_dict = {
+                    'name': host.find('name').text,
-                            'index': host.find('index').text,
+                    'ipv4': [],
-                            'hostid': host.find('hostid').text,
+                }
-                            'name': host.find('name').text,
+                for ipv4 in host.findall('netaddrs/ipv4'):
-                            'netaddrs': host.find('netaddrs').find('ipv4').text
+                    host_dict['ipv4'].append(ipv4.text)
-                        }
+                vol_host_list.append(host_dict)
-                        host_dict_IPs.append(host_dict)
+        LOG.debug('_get_vol_host vol_host_list:%s', vol_host_list)
-        return host_dict_IPs
+        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()
+        vol_name = self.private_storage.get(share['id'], 'volName')
-        host_dict_IPs = self._get_manila_hostIPv4s(hostlist)
+        vol_name_timestamp = self._get_timestamp_from_vol_name(vol_name)
-        LOG.debug('host_dict_IPs: %s', host_dict_IPs)
+        host_name = self._gen_host_name(vol_name_timestamp, access_level)
-        if len(host_dict_IPs) == 0:
+
-            host_name = self._gen_random_name("host")
+        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:
+            # Until now, share of QNAP NAS can only apply one access level for
-                LOG.debug('host[netaddrs]: %s', host['netaddrs'])
+            # all ips. "rw" for some ips and "ro" for else is not allowed.
-                LOG.debug('access_to: %s', access_to)
+            support_level = (constants.ACCESS_LEVEL_RW if
-                if host['netaddrs'] == access_to:
+                             access_level == constants.ACCESS_LEVEL_RO
-                    LOG.debug('in match ip')
+                             else constants.ACCESS_LEVEL_RO)
-                    host_name = host['name']
+            reason = _('Share only supports one access '
-                    break
+                       'level: %s') % support_level
-                if host is host_dict_IPs[-1]:
+            LOG.error(reason)
-                    host_name = self._gen_random_name("host")
+            raise exception.InvalidShareAccess(reason=reason)
-                    self.api_executor.add_host(host_name, access_to)
+        access = 1 if access_level == constants.ACCESS_LEVEL_RO else 0
-
+        self.api_executor.set_nfs_access(vol_name, access, host_name)
-        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)
 
     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()
+        vol_name = self.private_storage.get(share['id'], 'volName')
-        host_dict_IPs = self._get_manila_hostIPv4s(hostlist)
+        vol_name_timestamp = self._get_timestamp_from_vol_name(vol_name)
-        LOG.debug('host_dict_IPs: %s', host_dict_IPs)
+        host_name = self._gen_host_name(vol_name_timestamp, access_level)
-        if len(host_dict_IPs) == 0:
+        host_list = self.api_executor.get_host_list()
-            return
+        LOG.debug('vol_name: %(vol_name)s '
-        else:
+                  'access_level: %(access_level)s '
-            for host in host_dict_IPs:
+                  'host_name: %(host_name)s '
-                if (host['netaddrs'] == access_to):
+                  'host_list: %(host_list)s ',
-                    host_name = host['name']
+                  {'vol_name': vol_name,
-                    break
+                   'access_level': access_level,
-                if (host is host_dict_IPs[-1]):
+                   'host_name': host_name,
-                    return
+                   'host_list': host_list})
-
+        filter_host_list = self._get_vol_host(host_list, vol_name_timestamp)
-        volName = self.private_storage.get(share['id'], 'volName')
+        # if share already have host, remove ip from host
-        LOG.debug('volName: %s', volName)
+        for vol_host in filter_host_list:
-
+            if vol_host['name'] == host_name:
-        self.api_executor.set_nfs_access(volName, 2, 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':

manila/tests/share/drivers/qnap/fakes.py

@@ -192,7 +192,7 @@ FAKE_RES_DETAIL_DATA_GET_HOST_LIST = """
             <host>
                 <index><![CDATA[fakeHostIndex]]></index>
                 <hostid><![CDATA[fakeHostId]]></hostid>
-                <name><![CDATA[manila-hst-123]]></name>
+                <name><![CDATA[manila-fakeHostName]]></name>
                 <netaddrs>
                     <ipv4>
                         <![CDATA[fakeIp]]>
@@ -287,6 +287,15 @@ FAKE_RES_DETAIL_DATA_GET_HOST_LIST_API = """
         <result><![CDATA[0]]></result>
     </QDocRoot>"""
 
+FAKE_RES_DETAIL_DATA_GET_NO_HOST_LIST_API = """
+    <QDocRoot version="1.0">
+        <authPassed><![CDATA[1]]></authPassed>
+        <ES_RET_CODE><![CDATA[1]]></ES_RET_CODE>
+        <content>
+        </content>
+        <result><![CDATA[0]]></result>
+    </QDocRoot>"""
+
 FAKE_RES_DETAIL_DATA_CREATE_SNAPSHOT = """
     <QDocRoot version="1.0">
         <authPassed><![CDATA[1]]></authPassed>
@@ -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."""
 

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):
+    def test_edit_host(self):
-        """Test get host list api."""
+        """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(),

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(
+            expect_host_dict_ips, self.driver._get_vol_host(
-                host_list))
+                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 = (
+        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_random_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 = (
+        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_random_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_host_name')
-    @mock.patch.object(qnap.QnapShareDriver, '_gen_random_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_gen_host_name):
-            mock_get_manila_hostipv4s):
         """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
 

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.