From 5983eee1c35ababa1b6e2bad347cf98bd79b226f Mon Sep 17 00:00:00 2001 From: Rodrigo Barbieri Date: Wed, 1 Jul 2020 18:20:15 -0300 Subject: [PATCH] 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 (cherry picked from commit 8ebeafcbbafb700052f3abfc1f7ba004269a92e8) (cherry picked from commit 5358c996b40bbb0fd749ec182ff3ef67f5d71c16) (cherry picked from commit f3cdc27563766b2f49a1d88500f32c8d86838c96) --- cinder/backup/api.py | 5 ++-- cinder/db/api.py | 4 +-- cinder/db/sqlalchemy/api.py | 4 +-- cinder/objects/backup.py | 6 +++-- cinder/tests/unit/backup/test_backup.py | 25 +++++++++++-------- cinder/tests/unit/objects/test_backup.py | 8 +++--- cinder/tests/unit/test_db_api.py | 12 ++++++--- ...ross-project-incremental-backup-error.yaml | 6 +++++ 8 files changed, 45 insertions(+), 25 deletions(-) create mode 100644 releasenotes/notes/bug-1869746-cross-project-incremental-backup-error.yaml diff --git a/cinder/backup/api.py b/cinder/backup/api.py index b6cc0192bde..aa33fa17dcb 100644 --- a/cinder/backup/api.py +++ b/cinder/backup/api.py @@ -247,8 +247,9 @@ class API(base.Base): # incremental backup. latest_backup = None if incremental: - backups = objects.BackupList.get_all_by_volume(context.elevated(), - volume_id) + backups = objects.BackupList.get_all_by_volume( + 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 diff --git a/cinder/db/api.py b/cinder/db/api.py index 0d31ebf4505..b53b5713403 100644 --- a/cinder/db/api.py +++ b/cinder/db/api.py @@ -1225,9 +1225,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) diff --git a/cinder/db/sqlalchemy/api.py b/cinder/db/sqlalchemy/api.py index b3a573835e9..3a48f9bfacc 100644 --- a/cinder/db/sqlalchemy/api.py +++ b/cinder/db/sqlalchemy/api.py @@ -5184,9 +5184,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: diff --git a/cinder/objects/backup.py b/cinder/objects/backup.py index f2cadfc860d..cd7196a1fbf 100644 --- a/cinder/objects/backup.py +++ b/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): - backups = db.backup_get_all_by_volume(context, volume_id, filters) + def get_all_by_volume( + 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) diff --git a/cinder/tests/unit/backup/test_backup.py b/cinder/tests/unit/backup/test_backup.py index ffd0c335e8f..1866850af04 100644 --- a/cinder/tests/unit/backup/test_backup.py +++ b/cinder/tests/unit/backup/test_backup.py @@ -123,7 +123,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 @@ -131,8 +132,8 @@ class BaseBackupTest(test.TestCase): vol = {} vol['size'] = size vol['host'] = host - vol['user_id'] = str(uuid.uuid4()) - vol['project_id'] = str(uuid.uuid4()) + vol['user_id'] = fake.USER_ID + vol['project_id'] = project_id or fake.PROJECT_ID vol['status'] = status vol['display_name'] = display_name vol['display_description'] = display_description @@ -2154,19 +2155,17 @@ 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.backup.rpcapi.BackupAPI.create_backup') @mock.patch.object(api.API, '_get_available_backup_service_host', return_value='fake_host') + @mock.patch('cinder.objects.BackupList.get_all_by_volume') @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, - mock_create, mock_get_backups): - mock_get_backups.return_value = [v2_fakes.fake_backup('fake-1')] - backups = objects.BackupList.get_all_by_volume(self.ctxt, - fake.VOLUME_ID) + self, mock_reserve, mock_rollback, mock_get_backups, + mock_host): + backups = mock.Mock() backups.objects = [] + mock_get_backups.return_value = backups is_incremental = True self.ctxt.user_id = 'fake_user' self.ctxt.project_id = 'fake_project' @@ -2174,7 +2173,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, @@ -2182,6 +2182,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')]) diff --git a/cinder/tests/unit/objects/test_backup.py b/cinder/tests/unit/objects/test_backup.py index 0fb756ef17d..af5a67fec0e 100644 --- a/cinder/tests/unit/objects/test_backup.py +++ b/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, - fake.VOLUME_ID) + backups = objects.BackupList.get_all_by_volume( + 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]) diff --git a/cinder/tests/unit/test_db_api.py b/cinder/tests/unit/test_db_api.py index a6f8178d670..852c5ea9afa 100644 --- a/cinder/tests/unit/test_db_api.py +++ b/cinder/tests/unit/test_db_api.py @@ -2698,15 +2698,21 @@ class DBAPIBackupTestCase(BaseTest): {'fake_key': 'fake'}) self._assertEqualListsOfObjects([], byproj) - def test_backup_get_all_by_volume(self): - byvol = db.backup_get_all_by_volume(self.ctxt, - self.created[1]['volume_id']) + @mock.patch.object(sqlalchemy_api, 'authorize_project_context') + def test_backup_get_all_by_volume(self, mock_authorize): + 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, diff --git a/releasenotes/notes/bug-1869746-cross-project-incremental-backup-error.yaml b/releasenotes/notes/bug-1869746-cross-project-incremental-backup-error.yaml new file mode 100644 index 00000000000..c3fe4c00879 --- /dev/null +++ b/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.