From c2b7b78d4ff55ea079ea6172241011edc68f5b23 Mon Sep 17 00:00:00 2001 From: Rajat Dhasmana Date: Tue, 5 Mar 2024 03:26:55 +0530 Subject: [PATCH] Use normal credentials for legacy image update When updating legacy images, currenly we use the user's context and elevate priviledges. However, we do not require admin priviledges for the cinder API calls. This patch removes the special case where we elevate priviledges as it wasn't doing anything rather avoiding us to use right credentials and failing to fetch volume in the right location because of wrong credentials. The correct credentials are either the service ones set in glance-api.conf file or the user context credentials, using which the Image-Volume was created. NOTE: When using cinder as glance backend and we want to perform optimized volume upload to image, one thing we should make sure is either using the context or the cinder credentials set in glance-api.conf file, it should match the following details on the cinder side (if we are using internal context to create clone of image-volumes): cinder_store_user_name = context.user_id/cinder_internal_tenant_user_id cinder_store_project_name = context.project_id/cinder_internal_tenant_project_id The cinder_internal_tenant_user_id and cinder_internal_tenant_project_id are set in the [DEFAULT] section of cinder.conf. This issue was first discovered when testing the new location APIs[1] where tempest creates a volume with cinder's internal context and glance uses wrong (user context) credentials to access it and failing with 404 not found. [1] https://review.opendev.org/c/openstack/cinder/+/909847 Closes-Bug: #2056179 Change-Id: I4f27a9800f239da8dbf29f4c028678df1f867664 --- glance_store/_drivers/cinder/store.py | 38 ++++++++++++------- .../unit/cinder/test_multistore_cinder.py | 23 +++++++++-- ...-legacy-image-update-49a149ec267dccb6.yaml | 8 ++++ 3 files changed, 51 insertions(+), 18 deletions(-) create mode 100644 releasenotes/notes/fix-legacy-image-update-49a149ec267dccb6.yaml diff --git a/glance_store/_drivers/cinder/store.py b/glance_store/_drivers/cinder/store.py index ad0f1b16..7a399824 100644 --- a/glance_store/_drivers/cinder/store.py +++ b/glance_store/_drivers/cinder/store.py @@ -555,10 +555,29 @@ class Store(glance_store.driver.Store): If above both conditions doesn't meet, it returns false. """ try: - cinder_client = self.get_cinderclient(context=context, - legacy_update=True) + # We will use either the service credentials defined in + # config file or the user context credentials + cinder_client = self.get_cinderclient(context=context) cinder_volume_type = self.store_conf.cinder_volume_type - volume = cinder_client.volumes.get(volume_id) + # Here we are assuming that the volume is stored in the + # service project or context user's project else this + # will return NotFound exception. + # Ideally we should be using service user's credentials + # defined in the config and the volume should be stored + # in the service (internal) project else we are opening the + # image-volume to modification by users which might lead + # to corruption of image. + try: + volume = cinder_client.volumes.get(volume_id) + except cinder_exception.NotFound: + reason = (_LW("Image-Volume %s not found. If you have " + "upgraded your environment from single store " + "to multi store, transfer all your " + "Image-Volumes from user projects to service " + "project." + % volume_id)) + LOG.warning(reason) + return False if cinder_volume_type and volume.volume_type == cinder_volume_type: return True elif not cinder_volume_type: @@ -584,17 +603,8 @@ class Store(glance_store.driver.Store): for key in ['user_name', 'password', 'project_name', 'auth_address']]) - def get_cinderclient(self, context=None, legacy_update=False, - version='3.0'): - # NOTE: For legacy image update from single store to multiple - # stores we need to use admin context rather than user provided - # credentials - if legacy_update: - user_overriden = False - context = context.elevated() - else: - user_overriden = self.is_user_overriden() - + def get_cinderclient(self, context=None, version='3.0'): + user_overriden = self.is_user_overriden() session = get_cinder_session(self.store_conf) if user_overriden: diff --git a/glance_store/tests/unit/cinder/test_multistore_cinder.py b/glance_store/tests/unit/cinder/test_multistore_cinder.py index f40965bf..e637a28c 100644 --- a/glance_store/tests/unit/cinder/test_multistore_cinder.py +++ b/glance_store/tests/unit/cinder/test_multistore_cinder.py @@ -112,10 +112,12 @@ class TestMultiCinderStore(base.MultiStoreBaseTest, self._test_get_cinderclient_with_ca_certificates(group='cinder1') def test_get_cinderclient_legacy_update(self): - cc = self.store.get_cinderclient(self.fake_admin_context, - legacy_update=True) - self.assertEqual('admin_token', cc.client.auth.token) - self.assertEqual('http://foo/public_url', cc.client.auth.endpoint) + fake_endpoint = 'http://cinder.openstack.example.com/v2/fake_project' + self.config(cinder_endpoint_template=fake_endpoint, group='cinder1') + cc = self.store.get_cinderclient(self.context) + self.assertEqual(self.context.auth_token, + cc.client.auth.token) + self.assertEqual(fake_endpoint, cc.client.auth.endpoint) def test_open_cinder_volume_multipath_enabled(self): self.config(cinder_use_multipath=True, group='cinder1') @@ -221,6 +223,19 @@ class TestMultiCinderStore(base.MultiStoreBaseTest, type_match = self.store.is_image_associated_with_store( self.context, fake_vol_id) self.assertFalse(type_match) + # When the Image-Volume is not found + mocked_cc.return_value.volumes.get = mock.MagicMock( + side_effect=cinder.cinder_exception.NotFound(code=404)) + with mock.patch.object(cinder, 'LOG') as mock_log: + type_match = self.store.is_image_associated_with_store( + self.context, fake_vol_id) + mock_log.warning.assert_called_with( + "Image-Volume %s not found. If you have " + "upgraded your environment from single store " + "to multi store, transfer all your " + "Image-Volumes from user projects to service " + "project." % fake_vol_id) + self.assertFalse(type_match) def test_cinder_get(self): self._test_cinder_get(is_multi_store=True) diff --git a/releasenotes/notes/fix-legacy-image-update-49a149ec267dccb6.yaml b/releasenotes/notes/fix-legacy-image-update-49a149ec267dccb6.yaml new file mode 100644 index 00000000..e9372006 --- /dev/null +++ b/releasenotes/notes/fix-legacy-image-update-49a149ec267dccb6.yaml @@ -0,0 +1,8 @@ +--- +fixes: + - | + `Bug #2056179 `_: + Cinder Store: Fix issue when updating legacy image location. + Previously we only used the user context's credentials to make request + to cinder which we have now updated to use the service credentials + configured in the config file else use the user context's credentials.