Fix access rules for managed shares in HSP driver

A share managed in HSP could have some access rules that
are not in Manila. Trying to add a rule that already exists
in backend results in an error. Also, rules in backend that
names are not "share_id + ip" can't be deleted.
Additionally, if this share has a rule only in backend and not in
Manila, trying to delete it would fail, because HSP doesn't allow
delete share when it still has rules.

Fix it by adding a check in update access when rules already
exist in backend. Check for access rule name in backend when
denying access. And cleaning all rules from backend before
deleting a share to ensure that it has no children in HSP.

Change-Id: I0c8ee5c47efe22f004692022dd952f301d669b06
Closes-Bug: #1620756

manila/share/drivers/hitachi/hsp/driver.py

@@ -139,6 +139,19 @@ class HitachiHSPDriver(driver.ShareDriver):
                      {'shr': share['id']})
 
         if hsp_share_id:
+            # Clean all rules from share before deleting it
+            current_rules = self.hsp.get_access_rules(hsp_share_id)
+            for rule in current_rules:
+                try:
+                    self.hsp.delete_access_rule(hsp_share_id,
+                                                rule['name'])
+                except exception.HSPBackendException as e:
+                    if 'No matching access rule found.' in e.msg:
+                        LOG.debug("Rule %(rule)s already deleted in "
+                                  "backend.", {'rule': rule['name']})
+                    else:
+                        raise
+
             self.hsp.delete_share(hsp_share_id)
 
         if filesystem_id:
@@ -188,7 +201,8 @@ class HitachiHSPDriver(driver.ShareDriver):
             add_rules = self._get_complement(manila_rules_dict, hsp_rules_dict)
 
             for rule in remove_rules:
-                self.hsp.delete_access_rule(hsp_share_id, rule[0])
+                rule_name = self._get_hsp_rule_name(hsp_share_id, rule[0])
+                self.hsp.delete_access_rule(hsp_share_id, rule_name)
 
             for rule in add_rules:
                 self.hsp.add_access_rule(hsp_share_id, rule[0], rule[1])
@@ -196,20 +210,50 @@ class HitachiHSPDriver(driver.ShareDriver):
             for rule in delete_rules:
                 if rule['access_type'].lower() != 'ip':
                     continue
-                self.hsp.delete_access_rule(hsp_share_id, rule['access_to'])
+
+                # get the real rule name in HSP
+                rule_name = self._get_hsp_rule_name(hsp_share_id,
+                                                    rule['access_to'])
+                try:
+                    self.hsp.delete_access_rule(hsp_share_id,
+                                                rule_name)
+                except exception.HSPBackendException as e:
+                    if 'No matching access rule found.' in e.msg:
+                        LOG.debug("Rule %(rule)s already deleted in "
+                                  "backend.", {'rule': rule['access_to']})
+                    else:
+                        raise
 
             for rule in add_rules:
                 if rule['access_type'].lower() != 'ip':
                     msg = _("Only IP access type currently supported.")
                     raise exception.InvalidShareAccess(reason=msg)
 
-                self.hsp.add_access_rule(
+                try:
-                    hsp_share_id, rule['access_to'],
+                    self.hsp.add_access_rule(
-                    (rule['access_level'] == constants.ACCESS_LEVEL_RW))
+                        hsp_share_id, rule['access_to'],
+                        (rule['access_level'] == constants.ACCESS_LEVEL_RW))
+                except exception.HSPBackendException as e:
+                    if 'Duplicate NFS access rule exists' in e.msg:
+                        LOG.debug("Rule %(rule)s already exists in "
+                                  "backend.", {'rule': rule['access_to']})
+                    else:
+                        raise
 
         LOG.debug("Successfully updated share %(shr)s rules.",
                   {'shr': share['id']})
 
+    def _get_hsp_rule_name(self, share_id, host_to):
+        rule_name = share_id + host_to
+        all_rules = self.hsp.get_access_rules(share_id)
+        for rule in all_rules:
+            # check if this rule has other name in HSP
+            if rule['host-specification'] == host_to:
+                rule_name = rule['name']
+                break
+
+        return rule_name
+
     def _get_complement(self, rules_a, rules_b):
         """Returns the rules of list A that are not on list B"""
         complement = []

manila/share/drivers/hitachi/hsp/rest.py

@@ -161,11 +161,11 @@ class HSPRestBackend(object):
 
         self._send_post(url, payload=json.dumps(payload))
 
-    def delete_access_rule(self, share_id, host_to):
+    def delete_access_rule(self, share_id, rule_name):
         url = "https://%s/hspapi/shares/%s/" % (self.host, share_id)
         payload = {
             "action": "delete-access-rule",
-            "name": share_id + host_to,
+            "name": rule_name,
         }
 
         self._send_post(url, payload=json.dumps(payload))
@@ -198,9 +198,11 @@ class HSPRestBackend(object):
         status = resp_json['properties']['completion-status']
 
         if status == 'ERROR':
-            msg = _("HSP job %s failed.")
+            msg = _("HSP job %(id)s failed. %(reason)s")
-            args = resp_json['id']
+            job_id = resp_json['id']
-            raise exception.HSPBackendException(msg=msg % args)
+            reason = resp_json['properties']['completion-details']
+            raise exception.HSPBackendException(msg=msg % {'id': job_id,
+                                                           'reason': reason})
         elif status != target_status:
             msg = _("Timeout while waiting for job %s to complete.")
             args = resp_json['id']

manila/tests/share/drivers/hitachi/hsp/test_driver.py

@@ -61,7 +61,9 @@ class HitachiHSPTestCase(test.TestCase):
         self._driver.backend_name = "HSP"
         self.mock_log = self.mock_object(driver, 'LOG')
 
-    def test_update_access_add(self):
+    @ddt.data(None, exception.HSPBackendException(
+        message="Duplicate NFS access rule exists."))
+    def test_update_access_add(self, add_rule):
         access = {
             'access_type': 'ip',
             'access_to': '172.24.10.10',
@@ -74,7 +76,8 @@ class HitachiHSPTestCase(test.TestCase):
                          mock.Mock(return_value=fakes.file_system))
         self.mock_object(rest.HSPRestBackend, "get_share",
                          mock.Mock(return_value=fakes.share))
-        self.mock_object(rest.HSPRestBackend, "add_access_rule", mock.Mock())
+        self.mock_object(rest.HSPRestBackend, "add_access_rule", mock.Mock(
+            side_effect=add_rule))
 
         self._driver.update_access('context', self.fake_share_instance, [],
                                    access_list, [])
@@ -89,6 +92,36 @@ class HitachiHSPTestCase(test.TestCase):
             fakes.share['id'], access['access_to'],
             (access['access_level'] == constants.ACCESS_LEVEL_RW))
 
+    def test_update_access_add_exception(self):
+        access = {
+            'access_type': 'ip',
+            'access_to': '172.24.10.10',
+            'access_level': 'rw',
+        }
+
+        access_list = [access]
+
+        self.mock_object(rest.HSPRestBackend, "get_file_system",
+                         mock.Mock(return_value=fakes.file_system))
+        self.mock_object(rest.HSPRestBackend, "get_share",
+                         mock.Mock(return_value=fakes.share))
+        self.mock_object(rest.HSPRestBackend, "add_access_rule",
+                         mock.Mock(side_effect=exception.HSPBackendException(
+                             message="HSP Backend Exception: error adding "
+                                     "rule.")))
+
+        self.assertRaises(exception.HSPBackendException,
+                          self._driver.update_access, 'context',
+                          self.fake_share_instance, [], access_list, [])
+
+        rest.HSPRestBackend.get_file_system.assert_called_once_with(
+            self.fake_share_instance['id'])
+        rest.HSPRestBackend.get_share.assert_called_once_with(
+            fakes.file_system['id'])
+        rest.HSPRestBackend.add_access_rule.assert_called_once_with(
+            fakes.share['id'], access['access_to'],
+            (access['access_level'] == constants.ACCESS_LEVEL_RW))
+
     def test_update_access_recovery(self):
         access1 = {
             'access_type': 'ip',
@@ -108,7 +141,7 @@ class HitachiHSPTestCase(test.TestCase):
         self.mock_object(rest.HSPRestBackend, "get_share",
                          mock.Mock(return_value=fakes.share))
         self.mock_object(rest.HSPRestBackend, "get_access_rules",
-                         mock.Mock(return_value=fakes.hsp_rules))
+                         mock.Mock(side_effect=[fakes.hsp_rules, []]))
         self.mock_object(rest.HSPRestBackend, "delete_access_rule")
         self.mock_object(rest.HSPRestBackend, "add_access_rule")
 
@@ -121,16 +154,56 @@ class HitachiHSPTestCase(test.TestCase):
             self.fake_share_instance['id'])
         rest.HSPRestBackend.get_share.assert_called_once_with(
             fakes.file_system['id'])
-        rest.HSPRestBackend.get_access_rules.assert_called_once_with(
+        rest.HSPRestBackend.get_access_rules.assert_has_calls([
-            fakes.share['id'])
+            mock.call(fakes.share['id'])])
         rest.HSPRestBackend.delete_access_rule.assert_called_once_with(
-            fakes.share['id'], fakes.hsp_rules[0]['host-specification'])
+            fakes.share['id'],
+            fakes.share['id'] + fakes.hsp_rules[0]['host-specification'])
         rest.HSPRestBackend.add_access_rule.assert_has_calls([
             mock.call(fakes.share['id'], access1['access_to'], True),
             mock.call(fakes.share['id'], access2['access_to'], False)
         ], any_order=True)
 
-    def test_update_access_delete(self):
+    @ddt.data(None, exception.HSPBackendException(
+        message="No matching access rule found."))
+    def test_update_access_delete(self, delete_rule):
+        access1 = {
+            'access_type': 'ip',
+            'access_to': '172.24.44.200',
+            'access_level': 'rw',
+        }
+        access2 = {
+            'access_type': 'something',
+            'access_to': '188.100.20.10',
+            'access_level': 'ro',
+        }
+
+        delete_rules = [access1, access2]
+
+        self.mock_object(rest.HSPRestBackend, "get_file_system",
+                         mock.Mock(return_value=fakes.file_system))
+        self.mock_object(rest.HSPRestBackend, "get_share",
+                         mock.Mock(return_value=fakes.share))
+        self.mock_object(rest.HSPRestBackend, "delete_access_rule",
+                         mock.Mock(side_effect=delete_rule))
+        self.mock_object(rest.HSPRestBackend, "get_access_rules",
+                         mock.Mock(return_value=fakes.hsp_rules))
+
+        self._driver.update_access('context', self.fake_share_instance, [], [],
+                                   delete_rules)
+
+        self.assertTrue(self.mock_log.debug.called)
+
+        rest.HSPRestBackend.get_file_system.assert_called_once_with(
+            self.fake_share_instance['id'])
+        rest.HSPRestBackend.get_share.assert_called_once_with(
+            fakes.file_system['id'])
+        rest.HSPRestBackend.delete_access_rule.assert_called_once_with(
+            fakes.share['id'], fakes.hsp_rules[0]['name'])
+        rest.HSPRestBackend.get_access_rules.assert_called_once_with(
+            fakes.share['id'])
+
+    def test_update_access_delete_exception(self):
         access1 = {
             'access_type': 'ip',
             'access_to': '172.24.10.10',
@@ -149,10 +222,15 @@ class HitachiHSPTestCase(test.TestCase):
         self.mock_object(rest.HSPRestBackend, "get_share",
                          mock.Mock(return_value=fakes.share))
         self.mock_object(rest.HSPRestBackend, "delete_access_rule",
-                         mock.Mock())
+                         mock.Mock(side_effect=exception.HSPBackendException(
+                             message="HSP Backend Exception: error deleting "
+                                     "rule.")))
+        self.mock_object(rest.HSPRestBackend, 'get_access_rules',
+                         mock.Mock(return_value=[]))
 
-        self._driver.update_access('context', self.fake_share_instance, [], [],
+        self.assertRaises(exception.HSPBackendException,
-                                   delete_rules)
+                          self._driver.update_access, 'context',
+                          self.fake_share_instance, [], [], delete_rules)
 
         self.assertTrue(self.mock_log.debug.called)
 
@@ -161,7 +239,9 @@ class HitachiHSPTestCase(test.TestCase):
         rest.HSPRestBackend.get_share.assert_called_once_with(
             fakes.file_system['id'])
         rest.HSPRestBackend.delete_access_rule.assert_called_once_with(
-            fakes.share['id'], delete_rules[0]['access_to'])
+            fakes.share['id'], fakes.share['id'] + access1['access_to'])
+        rest.HSPRestBackend.get_access_rules.assert_called_once_with(
+            fakes.share['id'])
 
     @ddt.data(True, False)
     def test_update_access_ip_exception(self, is_recovery):
@@ -260,14 +340,20 @@ class HitachiHSPTestCase(test.TestCase):
                           self._driver.create_share, 'context',
                           fakes.invalid_share)
 
-    def test_delete_share(self):
+    @ddt.data(None, exception.HSPBackendException(
+        message="No matching access rule found."))
+    def test_delete_share(self, delete_rule):
         self.mock_object(rest.HSPRestBackend, "get_file_system",
                          mock.Mock(return_value=fakes.file_system))
         self.mock_object(rest.HSPRestBackend, "get_share",
                          mock.Mock(return_value=fakes.share))
-        self.mock_object(rest.HSPRestBackend, "delete_share", mock.Mock())
+        self.mock_object(rest.HSPRestBackend, "delete_share")
-        self.mock_object(rest.HSPRestBackend, "delete_file_system",
+        self.mock_object(rest.HSPRestBackend, "delete_file_system")
-                         mock.Mock())
+        self.mock_object(rest.HSPRestBackend, "get_access_rules",
+                         mock.Mock(return_value=[fakes.hsp_rules[0]]))
+        self.mock_object(rest.HSPRestBackend, "delete_access_rule", mock.Mock(
+            side_effect=[exception.HSPBackendException(
+                message="No matching access rule found."), delete_rule]))
 
         self._driver.delete_share('context', self.fake_share_instance)
 
@@ -281,6 +367,36 @@ class HitachiHSPTestCase(test.TestCase):
             fakes.share['id'])
         rest.HSPRestBackend.delete_file_system.assert_called_once_with(
             fakes.file_system['id'])
+        rest.HSPRestBackend.get_access_rules.assert_called_once_with(
+            fakes.share['id'])
+        rest.HSPRestBackend.delete_access_rule.assert_called_once_with(
+            fakes.share['id'], fakes.hsp_rules[0]['name'])
+
+    def test_delete_share_rule_exception(self):
+        self.mock_object(rest.HSPRestBackend, "get_file_system",
+                         mock.Mock(return_value=fakes.file_system))
+        self.mock_object(rest.HSPRestBackend, "get_share",
+                         mock.Mock(return_value=fakes.share))
+        self.mock_object(rest.HSPRestBackend, "get_access_rules",
+                         mock.Mock(return_value=[fakes.hsp_rules[0]]))
+        self.mock_object(rest.HSPRestBackend, "delete_access_rule",
+                         mock.Mock(side_effect=exception.HSPBackendException(
+                             message="Internal Server Error.")))
+
+        self.assertRaises(exception.HSPBackendException,
+                          self._driver.delete_share, 'context',
+                          self.fake_share_instance)
+
+        self.assertTrue(self.mock_log.debug.called)
+
+        rest.HSPRestBackend.get_file_system.assert_called_once_with(
+            self.fake_share_instance['id'])
+        rest.HSPRestBackend.get_share.assert_called_once_with(
+            fakes.file_system['id'])
+        rest.HSPRestBackend.get_access_rules.assert_called_once_with(
+            fakes.share['id'])
+        rest.HSPRestBackend.delete_access_rule.assert_called_once_with(
+            fakes.share['id'], fakes.hsp_rules[0]['name'])
 
     def test_delete_share_already_deleted(self):
         self.mock_object(rest.HSPRestBackend, "get_file_system", mock.Mock(

manila/tests/share/drivers/hitachi/hsp/test_rest.py

@@ -237,7 +237,7 @@ class HitachiHSPRestTestCase(test.TestCase):
     def test_delete_share(self):
         url = "https://172.24.47.190/hspapi/shares/%s" % fakes.share['id']
 
-        self.mock_object(rest.HSPRestBackend, "_send_delete", mock.Mock())
+        self.mock_object(rest.HSPRestBackend, "_send_delete")
 
         self._driver.delete_share(fakes.share['id'])
 
@@ -265,13 +265,12 @@ class HitachiHSPRestTestCase(test.TestCase):
         url = "https://172.24.47.190/hspapi/shares/%s/" % fakes.share['id']
         payload = {
             "action": "delete-access-rule",
-            "name": fakes.share['id'] + fakes.access_rule['access_to'],
+            "name": fakes.hsp_rules[0]['name'],
         }
-
         self.mock_object(rest.HSPRestBackend, "_send_post", mock.Mock())
 
         self._driver.delete_access_rule(fakes.share['id'],
-                                        fakes.access_rule['access_to'])
+                                        fakes.hsp_rules[0]['name'])
 
         rest.HSPRestBackend._send_post.assert_called_once_with(
             url, payload=json.dumps(payload))
@@ -314,10 +313,13 @@ class HitachiHSPRestTestCase(test.TestCase):
         url = "fake_job_url"
         json = {
             'id': 'fake_id',
-            'properties': {'completion-status': stat},
+            'properties': {
+                'completion-details': 'Duplicate NFS access rule exists',
+                'completion-status': stat,
+            },
             'messages': [{
                 'id': 'fake_id',
-                'message': 'fake_msg'
+                'message': 'fake_msg',
             }]
         }
 

releasenotes/notes/rules-for-managed-share-f28a26ffc980f6fb.yaml

@@ -0,0 +1,6 @@
+---
+fixes:
+  - Fixed HSP driver not supporting adding rules
+    that exist in backend for managed shares.
+  - Fixed HSP driver not supporting deleting share if
+    it has rules in backend that are not in Manila.