Fix cross-project incremental backups

This patch addresses the scenario where an
incremental backup can be created having a
parent backup that was taken in a different
project. This scenario ultimately leads to
a silent error when creating/deleting the
incremental backup and quotas going out of
sync.

The proposed fix is to narrow the backup search
down to the same project. To achieve this, a
method's signature had to be updated to achieve
the desired optimized behavior of passing the
volume's project_id parameter.

Closes-bug: #1869746
Closes-bug: #1873518

Change-Id: Icb621ff5966133f59d9d43ca2dd9f8e1919b1149

cinder/backup/api.py

@@ -243,8 +243,9 @@ class API(base.Base):
         # incremental backup.
         latest_backup = None
         if incremental:
-            backups = objects.BackupList.get_all_by_volume(context.elevated(),
+            backups = objects.BackupList.get_all_by_volume(
-                                                           volume_id)
+                context, volume_id, volume['project_id'],
+                filters={'project_id': context.project_id})
             if backups.objects:
                 # NOTE(xyang): The 'data_timestamp' field records the time
                 # when the data on the volume was first saved. If it is

cinder/db/api.py

@@ -1232,9 +1232,9 @@ def backup_get_all_by_project(context, project_id, filters=None, marker=None,
                                           sort_dirs=sort_dirs)
 
 
-def backup_get_all_by_volume(context, volume_id, filters=None):
+def backup_get_all_by_volume(context, volume_id, vol_project_id, filters=None):
     """Get all backups belonging to a volume."""
-    return IMPL.backup_get_all_by_volume(context, volume_id,
+    return IMPL.backup_get_all_by_volume(context, volume_id, vol_project_id,
                                          filters=filters)
 
 

cinder/db/sqlalchemy/api.py

@@ -5268,9 +5268,9 @@ def backup_get_all_by_project(context, project_id, filters=None, marker=None,
 
 
 @require_context
-def backup_get_all_by_volume(context, volume_id, filters=None):
+def backup_get_all_by_volume(context, volume_id, vol_project_id, filters=None):
 
-    authorize_project_context(context, volume_id)
+    authorize_project_context(context, vol_project_id)
     if not filters:
         filters = {}
     else:

cinder/objects/backup.py

@@ -261,8 +261,10 @@ class BackupList(base.ObjectListBase, base.CinderObject):
                                   backups, expected_attrs=expected_attrs)
 
     @classmethod
-    def get_all_by_volume(cls, context, volume_id, filters=None):
+    def get_all_by_volume(
-        backups = db.backup_get_all_by_volume(context, volume_id, filters)
+            cls, context, volume_id, vol_project_id, filters=None):
+        backups = db.backup_get_all_by_volume(
+            context, volume_id, vol_project_id, filters)
         expected_attrs = Backup._get_expected_attrs(context)
         return base.obj_make_list(context, cls(context), objects.Backup,
                                   backups, expected_attrs=expected_attrs)

cinder/tests/unit/backup/test_backup.py

@@ -124,7 +124,8 @@ class BaseBackupTest(test.TestCase):
                                 previous_status='available',
                                 size=1,
                                 host='testhost',
-                                encryption_key_id=None):
+                                encryption_key_id=None,
+                                project_id=None):
         """Create a volume entry in the DB.
 
         Return the entry ID
@@ -133,7 +134,7 @@ class BaseBackupTest(test.TestCase):
         vol['size'] = size
         vol['host'] = host
         vol['user_id'] = fake.USER_ID
-        vol['project_id'] = fake.PROJECT_ID
+        vol['project_id'] = project_id or fake.PROJECT_ID
         vol['status'] = status
         vol['display_name'] = display_name
         vol['display_description'] = display_description
@@ -2058,19 +2059,14 @@ class BackupAPITestCase(BaseBackupTest):
             self.ctxt, backups=1, backup_gigabytes=1)
         mock_rollback.assert_called_with(self.ctxt, "fake_reservation")
 
-    @mock.patch('cinder.db.backup_get_all_by_volume')
+    @mock.patch('cinder.objects.BackupList.get_all_by_volume')
-    @mock.patch('cinder.backup.rpcapi.BackupAPI.create_backup')
-    @mock.patch.object(api.API, '_get_available_backup_service_host',
-                       return_value='fake_host')
     @mock.patch.object(quota.QUOTAS, 'rollback')
     @mock.patch.object(quota.QUOTAS, 'reserve')
     def test_create_backup_failed_with_empty_backup_objects(
-            self, mock_reserve, mock_rollback, mock_get_service,
+            self, mock_reserve, mock_rollback, mock_get_backups):
-            mock_create, mock_get_backups):
+        backups = mock.Mock()
-        mock_get_backups.return_value = [v2_fakes.fake_backup('fake-1')]
-        backups = objects.BackupList.get_all_by_volume(self.ctxt,
-                                                       fake.VOLUME_ID)
         backups.objects = []
+        mock_get_backups.return_value = backups
         is_incremental = True
         self.ctxt.user_id = 'fake_user'
         self.ctxt.project_id = 'fake_project'
@@ -2078,7 +2074,8 @@ class BackupAPITestCase(BaseBackupTest):
 
         volume_id = self._create_volume_db_entry(status='available',
                                                  host='testhost#rbd',
-                                                 size=1)
+                                                 size=1,
+                                                 project_id="vol_proj_id")
         self.assertRaises(exception.InvalidBackup,
                           self.api.create,
                           self.ctxt,
@@ -2086,6 +2083,9 @@ class BackupAPITestCase(BaseBackupTest):
                           volume_id, None,
                           incremental=is_incremental)
         mock_rollback.assert_called_with(self.ctxt, "fake_reservation")
+        mock_get_backups.assert_called_once_with(
+            self.ctxt, volume_id, 'vol_proj_id',
+            filters={'project_id': 'fake_project'})
 
     @mock.patch('cinder.db.backup_get_all_by_volume',
                 return_value=[v2_fakes.fake_backup('fake-1')])

cinder/tests/unit/objects/test_backup.py

@@ -290,11 +290,13 @@ class TestBackupList(test_objects.BaseObjectsTestCase):
     @mock.patch('cinder.db.backup_get_all_by_volume',
                 return_value=[fake_backup])
     def test_get_all_by_volume(self, get_all_by_volume):
-        backups = objects.BackupList.get_all_by_volume(self.context,
+        backups = objects.BackupList.get_all_by_volume(
-                                                       fake.VOLUME_ID)
+            self.context, fake.VOLUME_ID, 'fake_proj')
         self.assertEqual(1, len(backups))
         get_all_by_volume.assert_called_once_with(self.context,
-                                                  fake.VOLUME_ID, None)
+                                                  fake.VOLUME_ID,
+                                                  'fake_proj',
+                                                  None)
         TestBackup._compare(self, fake_backup, backups[0])
 
 

cinder/tests/unit/test_db_api.py

@@ -2879,15 +2879,21 @@ class DBAPIBackupTestCase(BaseTest):
                                               {'fake_key': 'fake'})
         self._assertEqualListsOfObjects([], byproj)
 
-    def test_backup_get_all_by_volume(self):
+    @mock.patch.object(sqlalchemy_api, 'authorize_project_context')
-        byvol = db.backup_get_all_by_volume(self.ctxt,
+    def test_backup_get_all_by_volume(self, mock_authorize):
-                                            self.created[1]['volume_id'])
+        byvol = db.backup_get_all_by_volume(
+            self.ctxt, self.created[1]['volume_id'], 'fake_proj')
         self._assertEqualObjects(self.created[1], byvol[0])
 
         byvol = db.backup_get_all_by_volume(self.ctxt,
                                             self.created[1]['volume_id'],
+                                            'fake_proj',
                                             {'fake_key': 'fake'})
         self._assertEqualListsOfObjects([], byvol)
+        mock_authorize.assert_has_calls([
+            mock.call(self.ctxt, 'fake_proj'),
+            mock.call(self.ctxt, 'fake_proj')
+        ])
 
     def test_backup_update_nonexistent(self):
         self.assertRaises(exception.BackupNotFound,

releasenotes/notes/bug-1869746-cross-project-incremental-backup-error.yaml

@@ -0,0 +1,6 @@
+---
+fixes:
+  - |
+    Cinder no longer allows an incremental backup to be
+    created while having the parent backup in another
+    project.