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 bb09e29cd0..f888a471e8 100644 --- a/manila/common/constants.py +++ b/manila/common/constants.py @@ -295,6 +295,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 431f974359..0b80963b44 100644 --- a/manila/db/sqlalchemy/api.py +++ b/manila/db/sqlalchemy/api.py @@ -3205,6 +3205,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 `_.