From 36f6ec0426dc186654c690503ea2ea2ca5a38ef4 Mon Sep 17 00:00:00 2001 From: Carlos Eduardo Date: Fri, 16 Aug 2024 16:35:58 -0300 Subject: [PATCH] Lock shares when creating a locked access rule Locks for access rules currently only lock the access rule, but in case someone requests the share to be deleted, we will proceed with the requests in case it doesn't have any locks. Now, when creating access rules and locking them against deletion, shares will also be locked to avoid their deletion and getting clients disconnected. For existing access rules, to avoid the share being deleted by someone else, a share lock must be created. The share locks will use a specific lock reason which will be tied to the UUID of the access lock, and we will delete the share logs alongside the access rules locks together. Closes-Bug: #2075967 Change-Id: Ie5204e6098a9338ea721207e077af117793eb255 Signed-off-by: Carlos Eduardo --- .../admin/shared-file-systems-crud-share.rst | 4 +- doc/source/user/create-and-manage-shares.rst | 4 +- manila/api/v1/shares.py | 46 +++++++++++--- manila/common/constants.py | 2 + manila/db/sqlalchemy/api.py | 17 +++++ manila/tests/api/v1/test_shares.py | 63 ++++++++++--------- manila/tests/db/sqlalchemy/test_api.py | 41 ++++++++++++ ...-when-rule-is-locked-9ce9c6914acc1edb.yaml | 12 ++++ 8 files changed, 149 insertions(+), 40 deletions(-) create mode 100644 releasenotes/notes/bug-2075967-lock-shares-deletion-when-rule-is-locked-9ce9c6914acc1edb.yaml diff --git a/doc/source/admin/shared-file-systems-crud-share.rst b/doc/source/admin/shared-file-systems-crud-share.rst index fa893cefec..05d205645c 100644 --- a/doc/source/admin/shared-file-systems-crud-share.rst +++ b/doc/source/admin/shared-file-systems-crud-share.rst @@ -756,7 +756,9 @@ Allow access to the share with ``user`` access type: for creating access rules. A reason (``--lock-reason``) can also be provided. Only the user that placed the lock, system administrators and services will be able to view sensitive fields of, or manipulate such access rules by - virtue of default RBAC. + virtue of default RBAC. In case the deletion of the access rule was locked, + Manila will also place an additional lock on the share, to ensure it will + not be deleted and cause disconnections. To verify that the access rules (ACL) were configured correctly for a share, you list permissions for a share: diff --git a/doc/source/user/create-and-manage-shares.rst b/doc/source/user/create-and-manage-shares.rst index a91b67bdde..bbd9a41602 100644 --- a/doc/source/user/create-and-manage-shares.rst +++ b/doc/source/user/create-and-manage-shares.rst @@ -516,7 +516,9 @@ Grant and revoke share access ``--lock-visibility`` and ``--lock-deletion`` in the Manila OpenStack command for creating access rules. A reason (``--lock-reason``) can also be provided. Only the user that placed the lock, system administrators and services will - be able to manipulate such access rules. + be able to manipulate such access rules. In case the deletion of the access + rule was locked, Manila will also place an additional lock on the share, to + ensure it will not be deleted and cause disconnections. Allow read-write access ~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/manila/api/v1/shares.py b/manila/api/v1/shares.py index 0394770f66..7e5bb7e9a0 100644 --- a/manila/api/v1/shares.py +++ b/manila/api/v1/shares.py @@ -476,29 +476,52 @@ class ShareMixin(object): access['project_id'] = context.project_id access['user_id'] = context.user_id - def raise_lock_failed(access, lock_action): + def raise_lock_failed(resource_id, lock_action, + resource_type='access rule'): word_mapping = { constants.RESOURCE_ACTION_SHOW: 'visibility', constants.RESOURCE_ACTION_DELETE: 'deletion' } - msg = _("Failed to lock the %(action)s of the access rule " - "%(rule)s.") % { + msg = _("Failed to lock the %(action)s of the %(resource_type)s " + "%(resource_id)s.") % { 'action': word_mapping[lock_action], - 'rule': access['id'] + 'resource_id': resource_id, + 'resource_type': resource_type } raise webob.exc.HTTPBadRequest(explanation=msg) - deletion_lock = {} + access_deletion_lock = {} + share_deletion_lock = {} if lock_deletion: try: - deletion_lock = self.resource_locks_api.create( + access_deletion_lock = self.resource_locks_api.create( context, resource_id=access['id'], resource_type='access_rule', resource_action=constants.RESOURCE_ACTION_DELETE, resource=access, lock_reason=lock_reason) except Exception: - raise_lock_failed(access, constants.RESOURCE_ACTION_DELETE) + raise_lock_failed( + access['id'], constants.RESOURCE_ACTION_DELETE + ) + try: + share_lock_reason = ( + constants.SHARE_LOCKED_BY_ACCESS_LOCK_REASON % { + 'lock_id': access_deletion_lock['id'] + } + ) + share_deletion_lock = self.resource_locks_api.create( + context, resource_id=access['share_id'], + resource_type='share', + resource_action=constants.RESOURCE_ACTION_DELETE, + lock_reason=share_lock_reason) + except Exception: + self.resource_locks_api.delete( + context, access_deletion_lock['id']) + raise_lock_failed( + access['share_id'], constants.RESOURCE_ACTION_DELETE, + resource_type='share' + ) if lock_visibility: try: @@ -510,10 +533,13 @@ class ShareMixin(object): except Exception: # If a deletion lock was placed and the visibility wasn't, # we should rollback the deletion lock. - if deletion_lock: + if access_deletion_lock: self.resource_locks_api.delete( - context, deletion_lock['id']) - raise_lock_failed(access, constants.RESOURCE_ACTION_SHOW) + context, access_deletion_lock['id']) + if share_deletion_lock: + self.resource_locks_api.delete( + context, share_deletion_lock['id']) + raise_lock_failed(access['id'], constants.RESOURCE_ACTION_SHOW) @wsgi.Controller.authorize('allow_access') def _allow_access(self, req, id, body, enable_ceph=False, diff --git a/manila/common/constants.py b/manila/common/constants.py index 1aafe41191..c09b3cc978 100644 --- a/manila/common/constants.py +++ b/manila/common/constants.py @@ -293,6 +293,8 @@ DISALLOWED_STATUS_WHEN_LOCKING_ACCESS_RULES = ( ACCESS_STATE_DELETED, ) +SHARE_LOCKED_BY_ACCESS_LOCK_REASON = 'Locked by access lock: %(lock_id)s' + class ExtraSpecs(object): diff --git a/manila/db/sqlalchemy/api.py b/manila/db/sqlalchemy/api.py index 480d96ebed..295c04a6dd 100644 --- a/manila/db/sqlalchemy/api.py +++ b/manila/db/sqlalchemy/api.py @@ -3203,6 +3203,23 @@ def share_instance_access_delete(context, mapping_id): ) if locks: for lock in locks: + if lock['resource_action'] == constants.RESOURCE_ACTION_DELETE: + lock_reason = ( + constants.SHARE_LOCKED_BY_ACCESS_LOCK_REASON % { + 'lock_id': lock['id'] + } + ) + share_filters = { + 'all_projects': True, + 'lock_reason': lock_reason + } + share_locks, _ = resource_lock_get_all( + context.elevated(), filters=share_filters + ) or [] + for share_lock in share_locks: + resource_lock_delete( + context.elevated(), share_lock['id'] + ) resource_lock_delete( context.elevated(), lock['id'] ) diff --git a/manila/tests/api/v1/test_shares.py b/manila/tests/api/v1/test_shares.py index aefdcf2bdb..796eb3f156 100644 --- a/manila/tests/api/v1/test_shares.py +++ b/manila/tests/api/v1/test_shares.py @@ -1127,8 +1127,21 @@ class ShareActionsTest(test.TestCase): 'id': 'fake', 'access_type': 'ip', 'access_to': '127.0.0.1', + 'share_id': 'fake_share_id' } - self.mock_object(resource_locks.API, 'create') + mock_deletion_lock_create = mock.Mock() + lock_id = 'fake_lock_id' + if lock_deletion: + mock_deletion_lock_create = mock.Mock( + side_effect=[ + {'id': lock_id}, + {'id': f'{lock_id}2'}, + {'id': f'{lock_id}3'} + ] + ) + self.mock_object( + resource_locks.API, 'create', mock_deletion_lock_create + ) id = 'fake_share_id' req = fakes.HTTPRequest.blank( @@ -1147,6 +1160,10 @@ class ShareActionsTest(test.TestCase): restrict_calls = [] if lock_deletion: + share_lock_reason = ( + constants.SHARE_LOCKED_BY_ACCESS_LOCK_REASON % + {'lock_id': lock_id} + ) restrict_calls.append( mock.call( context, resource_id=access['id'], @@ -1156,6 +1173,14 @@ class ShareActionsTest(test.TestCase): lock_reason=lock_reason ) ) + restrict_calls.append( + mock.call( + context, resource_id=access['share_id'], + resource_type='share', + resource_action=constants.RESOURCE_ACTION_DELETE, + lock_reason=share_lock_reason + ) + ) if lock_visibility: restrict_calls.append( mock.call( @@ -1245,13 +1270,14 @@ class ShareActionsTest(test.TestCase): @ddt.unpack def test_allow_access_visibility_restrictions(self, lock_visibility, lock_deletion, lock_reason): - access = {'id': 'fake'} + access = {'id': 'fake', 'share_id': 'fake_share_id'} + expected_access = {'access': {'fake_key': 'fake_value'}} self.mock_object(share_api.API, 'allow_access', mock.Mock(return_value=access)) self.mock_object(self.controller._access_view_builder, 'view', - mock.Mock(return_value={'access': {'fake': 'fake'}})) - self.mock_object(resource_locks.API, 'create') + mock.Mock(return_value=expected_access)) + self.mock_object(self.controller, '_create_access_locks') id = 'fake_share_id' body = { @@ -1263,7 +1289,6 @@ class ShareActionsTest(test.TestCase): 'lock_reason': lock_reason } } - expected = {'access': {'fake': 'fake'}} req = fakes.HTTPRequest.blank( '/tenant1/shares/%s/action' % id, version='2.82') context = req.environ['manila.context'] @@ -1274,31 +1299,13 @@ class ShareActionsTest(test.TestCase): req, id, body, lock_visibility=lock_visibility, lock_deletion=lock_deletion, lock_reason=lock_reason) - self.assertEqual(expected, res) + self.assertEqual(expected_access, res) self.mock_policy_check.assert_called_once_with( context, 'share', 'allow_access') - restrict_calls = [] - if lock_deletion: - restrict_calls.append( - mock.call( - context, resource_id=access['id'], - resource_type='access_rule', - resource_action=constants.RESOURCE_ACTION_DELETE, - resource=access, - lock_reason=lock_reason - ) - ) - if lock_visibility: - restrict_calls.append( - mock.call( - context, resource_id=access['id'], - resource_type='access_rule', - resource_action=constants.RESOURCE_ACTION_SHOW, - resource=access, - lock_reason=lock_reason - ) - ) - resource_locks.API.create.assert_has_calls(restrict_calls) + self.controller._create_access_locks.assert_called_once_with( + context, access, lock_deletion=lock_deletion, + lock_visibility=lock_visibility, lock_reason=lock_reason + ) def test_allow_access_with_network_id(self): share_network = db_utils.create_share_network() diff --git a/manila/tests/db/sqlalchemy/test_api.py b/manila/tests/db/sqlalchemy/test_api.py index fa990faa30..409748a36c 100644 --- a/manila/tests/db/sqlalchemy/test_api.py +++ b/manila/tests/db/sqlalchemy/test_api.py @@ -216,6 +216,47 @@ class ShareAccessDatabaseAPITestCase(test.TestCase): self.assertRaises(exception.NotFound, db_api.share_instance_access_get, self.ctxt, access['id'], share['instance']['id']) + def test_share_instance_access_delete_with_locks(self): + share = db_utils.create_share() + access = db_utils.create_access(share_id=share['id'], + metadata={'key1': 'v1'}) + + # create a share and an access lock to ensure they'll be deleted + access_lock = db_utils.create_lock(resource_id=access['id']) + share_lock_reason = ( + constants.SHARE_LOCKED_BY_ACCESS_LOCK_REASON % + {'lock_id': access_lock['id']} + ) + share_lock = db_utils.create_lock( + resource_id=share['id'], lock_reason=share_lock_reason + ) + + # create another share lock, to ensure it won't be deleted + unrelated_share_lock = db_utils.create_lock(resource_id=share['id']) + + instance_access_mapping = db_api.share_instance_access_get( + self.ctxt, access['id'], share.instance['id']) + + db_api.share_instance_access_delete( + self.ctxt, instance_access_mapping['id']) + + rules = db_api.share_access_get_all_for_instance( + self.ctxt, share.instance['id']) + unrelated_share_lock_get = ( + db_api.resource_lock_get(self.ctxt, unrelated_share_lock['id']) + ) + self.assertEqual([], rules) + self.assertEqual(unrelated_share_lock['id'], + unrelated_share_lock_get['id']) + + # ensure the access rules and the locks have been dropped + self.assertRaises(exception.NotFound, db_api.share_instance_access_get, + self.ctxt, access['id'], share['instance']['id']) + self.assertRaises(exception.NotFound, db_api.resource_lock_get, + self.ctxt, access_lock['id']) + self.assertRaises(exception.NotFound, db_api.resource_lock_get, + self.ctxt, share_lock['id']) + def test_one_share_with_two_share_instance_access_delete(self): metadata = {'key2': 'v2', 'key3': 'v3'} share = db_utils.create_share() diff --git a/releasenotes/notes/bug-2075967-lock-shares-deletion-when-rule-is-locked-9ce9c6914acc1edb.yaml b/releasenotes/notes/bug-2075967-lock-shares-deletion-when-rule-is-locked-9ce9c6914acc1edb.yaml new file mode 100644 index 0000000000..261fbd7b1a --- /dev/null +++ b/releasenotes/notes/bug-2075967-lock-shares-deletion-when-rule-is-locked-9ce9c6914acc1edb.yaml @@ -0,0 +1,12 @@ +--- +upgrades: + - | + Creating access rules with a deletion lock will now result in the share + being locked alongside in order to avoid disconnections. For more details, + please refer to + `launchpad bug 2075967 `_. +fixes: + - | + When creating access rules with a deletion lock, the shares will also be + locked to prevent disconnections. For more details, please refer to + `launchpad bug 2075967 `_.