From b29a0e553f44266fbc544f5cb95cf89f6cfd39db Mon Sep 17 00:00:00 2001 From: Alyson Rosa Date: Tue, 6 Sep 2016 16:00:29 -0300 Subject: [PATCH] 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 | 54 ++++++- manila/share/drivers/hitachi/hsp/rest.py | 12 +- .../share/drivers/hitachi/hsp/test_driver.py | 146 ++++++++++++++++-- .../share/drivers/hitachi/hsp/test_rest.py | 14 +- ...es-for-managed-share-f28a26ffc980f6fb.yaml | 6 + 5 files changed, 201 insertions(+), 31 deletions(-) create mode 100644 releasenotes/notes/rules-for-managed-share-f28a26ffc980f6fb.yaml diff --git a/manila/share/drivers/hitachi/hsp/driver.py b/manila/share/drivers/hitachi/hsp/driver.py index 45ab824e8a..3c9be9fcd6 100644 --- a/manila/share/drivers/hitachi/hsp/driver.py +++ b/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( - hsp_share_id, rule['access_to'], - (rule['access_level'] == constants.ACCESS_LEVEL_RW)) + try: + self.hsp.add_access_rule( + 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 = [] diff --git a/manila/share/drivers/hitachi/hsp/rest.py b/manila/share/drivers/hitachi/hsp/rest.py index 748e06ea14..632b4f5af3 100644 --- a/manila/share/drivers/hitachi/hsp/rest.py +++ b/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.") - args = resp_json['id'] - raise exception.HSPBackendException(msg=msg % args) + msg = _("HSP job %(id)s failed. %(reason)s") + job_id = resp_json['id'] + 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'] diff --git a/manila/tests/share/drivers/hitachi/hsp/test_driver.py b/manila/tests/share/drivers/hitachi/hsp/test_driver.py index 99beb3c918..6813c69853 100644 --- a/manila/tests/share/drivers/hitachi/hsp/test_driver.py +++ b/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( - fakes.share['id']) + rest.HSPRestBackend.get_access_rules.assert_has_calls([ + 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, [], [], - delete_rules) + self.assertRaises(exception.HSPBackendException, + 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_file_system", - mock.Mock()) + self.mock_object(rest.HSPRestBackend, "delete_share") + self.mock_object(rest.HSPRestBackend, "delete_file_system") + 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( diff --git a/manila/tests/share/drivers/hitachi/hsp/test_rest.py b/manila/tests/share/drivers/hitachi/hsp/test_rest.py index a8b02d11d5..5798957995 100644 --- a/manila/tests/share/drivers/hitachi/hsp/test_rest.py +++ b/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', }] } diff --git a/releasenotes/notes/rules-for-managed-share-f28a26ffc980f6fb.yaml b/releasenotes/notes/rules-for-managed-share-f28a26ffc980f6fb.yaml new file mode 100644 index 0000000000..41df4245a6 --- /dev/null +++ b/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.