Fix test_delete_locations to be performed by admin
Delete image location operation is allowed to admin only
by default
- https://github.com/openstack/glance/blob/428295969836a2a0bad0ae0593ea1a890e155898/glance/policies/image.py#L150
But tempest test was performing it with non admin creds
- https://github.com/openstack/tempest/blob/4054e13e65f7d11c9097946a36976bf9c6edea45/tempest/api/image/v2/test_images.py#L935
While enabling the Glance RBAC new defaults in devstack (
everything run with RBAC new defaults), this test start failing
with 403 error
- https://review.opendev.org/c/openstack/devstack/+/883601
- https://storage.gra.cloud.ovh.net/v1/AUTH_dcaab5e32b234d56b626f72581e3644c/zuul_opendev_logs_2dd/883601/1/check/nova-ceph-multistore/2dd523c/testr_results.html
It was not failing before because this test was only enabled in
nova-ceph-multistore job which used old policy and old policy allow
everyone to perform this operation. tempest-full-enforce-scope-new-defaults
job which enable new defaults did not run this test to did not capture this
issue.
Change-Id: I6cbf95caf782bde1b11494b13e9d8d39ab9f3080
diff --git a/tempest/api/image/base.py b/tempest/api/image/base.py
index 23e7fd8..11a1e6c 100644
--- a/tempest/api/image/base.py
+++ b/tempest/api/image/base.py
@@ -13,6 +13,7 @@
# under the License.
import io
+import time
from tempest.common import image as common_image
from tempest import config
@@ -22,6 +23,7 @@
import tempest.test
CONF = config.CONF
+BAD_REQUEST_RETRIES = 3
class BaseImageTest(tempest.test.BaseTestCase):
@@ -159,6 +161,82 @@
pass
return stores
+ def _update_image_with_retries(self, image, patch):
+ # NOTE(danms): If glance was unable to fetch the remote image via
+ # HTTP, it will return BadRequest. Because this can be transient in
+ # CI, we try this a few times before we agree that it has failed
+ # for a reason worthy of failing the test.
+ for i in range(BAD_REQUEST_RETRIES):
+ try:
+ self.client.update_image(image, patch)
+ break
+ except exceptions.BadRequest:
+ if i + 1 == BAD_REQUEST_RETRIES:
+ raise
+ else:
+ time.sleep(1)
+
+ def check_set_location(self):
+ image = self.client.create_image(container_format='bare',
+ disk_format='raw')
+
+ # Locations should be empty when there is no data
+ self.assertEqual('queued', image['status'])
+ self.assertEqual([], image['locations'])
+
+ # Add a new location
+ new_loc = {'metadata': {'foo': 'bar'},
+ 'url': CONF.image.http_image}
+ self._update_image_with_retries(image['id'], [
+ dict(add='/locations/-', value=new_loc)])
+
+ # The image should now be active, with one location that looks
+ # like we expect
+ image = self.client.show_image(image['id'])
+ self.assertEqual(1, len(image['locations']),
+ 'Image should have one location but has %i' % (
+ len(image['locations'])))
+ self.assertEqual(new_loc['url'], image['locations'][0]['url'])
+ self.assertEqual('bar', image['locations'][0]['metadata'].get('foo'))
+ if 'direct_url' in image:
+ self.assertEqual(image['direct_url'], image['locations'][0]['url'])
+
+ # If we added the location directly, the image goes straight
+ # to active and no hashing is done
+ self.assertEqual('active', image['status'])
+ self.assertIsNone(None, image['os_hash_algo'])
+ self.assertIsNone(None, image['os_hash_value'])
+
+ return image
+
+ def check_set_multiple_locations(self):
+ image = self.check_set_location()
+
+ new_loc = {'metadata': {'speed': '88mph'},
+ 'url': '%s#new' % CONF.image.http_image}
+ self._update_image_with_retries(image['id'],
+ [dict(add='/locations/-',
+ value=new_loc)])
+
+ # The image should now have two locations and the last one
+ # (locations are ordered) should have the new URL.
+ image = self.client.show_image(image['id'])
+ self.assertEqual(2, len(image['locations']),
+ 'Image should have two locations but has %i' % (
+ len(image['locations'])))
+ self.assertEqual(new_loc['url'], image['locations'][1]['url'])
+
+ # The image should still be active and still have no hashes
+ self.assertEqual('active', image['status'])
+ self.assertIsNone(None, image['os_hash_algo'])
+ self.assertIsNone(None, image['os_hash_value'])
+
+ # The direct_url should still match the first location
+ if 'direct_url' in image:
+ self.assertEqual(image['direct_url'], image['locations'][0]['url'])
+
+ return image
+
class BaseV2MemberImageTest(BaseV2ImageTest):
diff --git a/tempest/api/image/v2/admin/test_images.py b/tempest/api/image/v2/admin/test_images.py
index 733c778..a77b2f2 100644
--- a/tempest/api/image/v2/admin/test_images.py
+++ b/tempest/api/image/v2/admin/test_images.py
@@ -20,6 +20,7 @@
from tempest import config
from tempest.lib.common.utils import data_utils
from tempest.lib import decorators
+from tempest.lib import exceptions as lib_exc
CONF = config.CONF
@@ -120,3 +121,40 @@
self.assertEqual(0, len(failed_stores),
"Failed to copy the following stores: %s" %
str(failed_stores))
+
+
+class ImageLocationsAdminTest(base.BaseV2ImageAdminTest):
+
+ @classmethod
+ def skip_checks(cls):
+ super(ImageLocationsAdminTest, cls).skip_checks()
+ if not CONF.image_feature_enabled.manage_locations:
+ skip_msg = (
+ "%s skipped as show_multiple_locations is not available" % (
+ cls.__name__))
+ raise cls.skipException(skip_msg)
+
+ @decorators.idempotent_id('8a648de4-b745-4c28-a7b5-20de1c3da4d2')
+ def test_delete_locations(self):
+ image = self.check_set_multiple_locations()
+ expected_remaining_loc = image['locations'][1]
+
+ self.admin_client.update_image(image['id'], [
+ dict(remove='/locations/0')])
+
+ # The image should now have only the one location we did not delete
+ image = self.client.show_image(image['id'])
+ self.assertEqual(1, len(image['locations']),
+ 'Image should have one location but has %i' % (
+ len(image['locations'])))
+ self.assertEqual(expected_remaining_loc['url'],
+ image['locations'][0]['url'])
+
+ # The direct_url should now be the last remaining location
+ if 'direct_url' in image:
+ self.assertEqual(image['direct_url'], image['locations'][0]['url'])
+
+ # Removing the last location should be disallowed
+ self.assertRaises(lib_exc.Forbidden,
+ self.admin_client.update_image, image['id'], [
+ dict(remove='/locations/0')])
diff --git a/tempest/api/image/v2/test_images.py b/tempest/api/image/v2/test_images.py
index b723977..fecd5a7 100644
--- a/tempest/api/image/v2/test_images.py
+++ b/tempest/api/image/v2/test_images.py
@@ -16,7 +16,6 @@
import io
import random
-import time
from oslo_log import log as logging
from tempest.api.image import base
@@ -28,7 +27,6 @@
CONF = config.CONF
LOG = logging.getLogger(__name__)
-BAD_REQUEST_RETRIES = 3
class ImportImagesTest(base.BaseV2ImageTest):
@@ -808,89 +806,13 @@
return image
- def _check_set_location(self):
- image = self.client.create_image(container_format='bare',
- disk_format='raw')
-
- # Locations should be empty when there is no data
- self.assertEqual('queued', image['status'])
- self.assertEqual([], image['locations'])
-
- # Add a new location
- new_loc = {'metadata': {'foo': 'bar'},
- 'url': CONF.image.http_image}
- self._update_image_with_retries(image['id'], [
- dict(add='/locations/-', value=new_loc)])
-
- # The image should now be active, with one location that looks
- # like we expect
- image = self.client.show_image(image['id'])
- self.assertEqual(1, len(image['locations']),
- 'Image should have one location but has %i' % (
- len(image['locations'])))
- self.assertEqual(new_loc['url'], image['locations'][0]['url'])
- self.assertEqual('bar', image['locations'][0]['metadata'].get('foo'))
- if 'direct_url' in image:
- self.assertEqual(image['direct_url'], image['locations'][0]['url'])
-
- # If we added the location directly, the image goes straight
- # to active and no hashing is done
- self.assertEqual('active', image['status'])
- self.assertIsNone(None, image['os_hash_algo'])
- self.assertIsNone(None, image['os_hash_value'])
-
- return image
-
@decorators.idempotent_id('37599b8a-d5c0-4590-aee5-73878502be15')
def test_set_location(self):
- self._check_set_location()
-
- def _update_image_with_retries(self, image, patch):
- # NOTE(danms): If glance was unable to fetch the remote image via
- # HTTP, it will return BadRequest. Because this can be transient in
- # CI, we try this a few times before we agree that it has failed
- # for a reason worthy of failing the test.
- for i in range(BAD_REQUEST_RETRIES):
- try:
- self.client.update_image(image, patch)
- break
- except lib_exc.BadRequest:
- if i + 1 == BAD_REQUEST_RETRIES:
- raise
- else:
- time.sleep(1)
-
- def _check_set_multiple_locations(self):
- image = self._check_set_location()
-
- new_loc = {'metadata': {'speed': '88mph'},
- 'url': '%s#new' % CONF.image.http_image}
- self._update_image_with_retries(image['id'],
- [dict(add='/locations/-',
- value=new_loc)])
-
- # The image should now have two locations and the last one
- # (locations are ordered) should have the new URL.
- image = self.client.show_image(image['id'])
- self.assertEqual(2, len(image['locations']),
- 'Image should have two locations but has %i' % (
- len(image['locations'])))
- self.assertEqual(new_loc['url'], image['locations'][1]['url'])
-
- # The image should still be active and still have no hashes
- self.assertEqual('active', image['status'])
- self.assertIsNone(None, image['os_hash_algo'])
- self.assertIsNone(None, image['os_hash_value'])
-
- # The direct_url should still match the first location
- if 'direct_url' in image:
- self.assertEqual(image['direct_url'], image['locations'][0]['url'])
-
- return image
+ self.check_set_location()
@decorators.idempotent_id('bf6e0009-c039-4884-b498-db074caadb10')
def test_replace_location(self):
- image = self._check_set_multiple_locations()
+ image = self.check_set_multiple_locations()
original_locs = image['locations']
# Replacing with the exact thing should work
@@ -927,31 +849,6 @@
len(image['locations'])))
self.assertEqual(original_locs, image['locations'])
- @decorators.idempotent_id('8a648de4-b745-4c28-a7b5-20de1c3da4d2')
- def test_delete_locations(self):
- image = self._check_set_multiple_locations()
- expected_remaining_loc = image['locations'][1]
-
- self.client.update_image(image['id'], [
- dict(remove='/locations/0')])
-
- # The image should now have only the one location we did not delete
- image = self.client.show_image(image['id'])
- self.assertEqual(1, len(image['locations']),
- 'Image should have one location but has %i' % (
- len(image['locations'])))
- self.assertEqual(expected_remaining_loc['url'],
- image['locations'][0]['url'])
-
- # The direct_url should now be the last remaining location
- if 'direct_url' in image:
- self.assertEqual(image['direct_url'], image['locations'][0]['url'])
-
- # Removing the last location should be disallowed
- self.assertRaises(lib_exc.Forbidden,
- self.client.update_image, image['id'], [
- dict(remove='/locations/0')])
-
@decorators.idempotent_id('a9a20396-8399-4b36-909d-564949be098f')
def test_set_location_bad_scheme(self):
image = self.client.create_image(container_format='bare',