Merge "Lock shares when creating a locked access rule"

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:

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
 ~~~~~~~~~~~~~~~~~~~~~~~

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 "
+            msg = _("Failed to lock the %(action)s of the %(resource_type)s "
-                    "%(rule)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'])
+                        context, access_deletion_lock['id'])
-                raise_lock_failed(access, constants.RESOURCE_ACTION_SHOW)
+                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,

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):
 

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']
             )

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'}}))
+                         mock.Mock(return_value=expected_access))
-        self.mock_object(resource_locks.API, 'create')
+        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 = []
+        self.controller._create_access_locks.assert_called_once_with(
-        if lock_deletion:
+            context, access, lock_deletion=lock_deletion,
-            restrict_calls.append(
+            lock_visibility=lock_visibility, lock_reason=lock_reason
-                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)
 
     def test_allow_access_with_network_id(self):
         share_network = db_utils.create_share_network()

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()

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 <https://bugs.launchpad.net/manila/+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 <https://bugs.launchpad.net/manila/+bug/2075967>`_.