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 <ces.eduardo98@gmail.com>

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 "
-                    "%(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,

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

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

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

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>`_.