From d2522ec5c64722936629f969c091179d502e8c88 Mon Sep 17 00:00:00 2001 From: "James E. Blair" Date: Tue, 17 Dec 2024 15:25:00 -0800 Subject: [PATCH] Add web API image delete endpoints This adds tenant-admin auth protected web API endpoints to delete image builds and uploads. Change-Id: I9d1796c8c74ca094f60896fd3a87988a4cd232b7 --- tests/fixtures/zuul-connections-nodepool.conf | 8 ++ tests/unit/test_web.py | 110 ++++++++++++++++++ zuul/launcher/server.py | 1 + zuul/web/__init__.py | 48 ++++++++ zuul/zk/cache.py | 39 ++++--- zuul/zk/image_registry.py | 32 ++--- zuul/zk/launcher.py | 8 +- 7 files changed, 217 insertions(+), 29 deletions(-) diff --git a/tests/fixtures/zuul-connections-nodepool.conf b/tests/fixtures/zuul-connections-nodepool.conf index a2058ed487..99083d4980 100644 --- a/tests/fixtures/zuul-connections-nodepool.conf +++ b/tests/fixtures/zuul-connections-nodepool.conf @@ -15,6 +15,14 @@ git_user_name=zuul git_dir=/tmp/zuul-test/executor-git load_multiplier=100 +[auth zuul_operator] +driver=HS256 +allow_authz_override=true +realm=zuul.example.com +client_id=zuul.example.com +issuer_id=zuul_operator +secret=NoDanaOnlyZuul + [connection gerrit] driver=gerrit server=review.example.com diff --git a/tests/unit/test_web.py b/tests/unit/test_web.py index a05365ed6d..8d921c31c4 100644 --- a/tests/unit/test_web.py +++ b/tests/unit/test_web.py @@ -1464,6 +1464,116 @@ class TestWebProviders(LauncherBaseTestCase, WebMixin): ] self.assertEqual(expected, data) + @simple_layout('layouts/nodepool-image.yaml', enable_nodepool=True) + @return_data( + 'build-debian-local-image', + 'refs/heads/master', + LauncherBaseTestCase.debian_return_data, + ) + @return_data( + 'build-ubuntu-local-image', + 'refs/heads/master', + LauncherBaseTestCase.ubuntu_return_data, + ) + @mock.patch('zuul.driver.aws.awsendpoint.AwsProviderEndpoint.uploadImage', + return_value="test_external_id") + def test_web_image_delete(self, mock_uploadImage): + self.waitUntilSettled() + self.startWebServer() + self.assertHistory([ + dict(name='build-debian-local-image', result='SUCCESS'), + dict(name='build-ubuntu-local-image', result='SUCCESS'), + ], ordered=False) + + resp = self.get_url('api/tenant/tenant-one/images') + data = resp.json() + self.assertEqual(4, len(data)) + self.assertNotIn('build_artifacts', data[0]) + self.assertEqual(1, len(data[1]['build_artifacts'])) + self.assertEqual(1, len(data[2]['build_artifacts'])) + self.assertEqual(1, len(data[1]['build_artifacts'][0]['uploads'])) + self.assertEqual(1, len(data[2]['build_artifacts'][0]['uploads'])) + art = data[1]['build_artifacts'][0] + # Test that unauthenticated access fails + resp = self.delete_url( + f"api/tenant/tenant-one/image-build-artifact/{art['uuid']}" + ) + self.assertEqual(401, resp.status_code, resp.text) + # Do it again with auth + authz = {'iss': 'zuul_operator', + 'aud': 'zuul.example.com', + 'sub': 'testuser', + 'zuul': { + 'admin': ['tenant-one', ] + }, + 'exp': int(time.time()) + 3600} + token = jwt.encode(authz, key='NoDanaOnlyZuul', + algorithm='HS256') + resp = self.delete_url( + f"api/tenant/tenant-one/image-build-artifact/{art['uuid']}", + headers={'Authorization': 'Bearer %s' % token}) + self.assertEqual(204, resp.status_code, resp.text) + for _ in iterate_timeout(10, "artifact to be deleted"): + resp = self.get_url('api/tenant/tenant-one/images') + data = resp.json() + if 'build_artifacts' not in data[1]: + break + + @simple_layout('layouts/nodepool-image.yaml', enable_nodepool=True) + @return_data( + 'build-debian-local-image', + 'refs/heads/master', + LauncherBaseTestCase.debian_return_data, + ) + @return_data( + 'build-ubuntu-local-image', + 'refs/heads/master', + LauncherBaseTestCase.ubuntu_return_data, + ) + @mock.patch('zuul.driver.aws.awsendpoint.AwsProviderEndpoint.uploadImage', + return_value="test_external_id") + def test_web_upload_delete(self, mock_uploadImage): + self.waitUntilSettled() + self.startWebServer() + self.assertHistory([ + dict(name='build-debian-local-image', result='SUCCESS'), + dict(name='build-ubuntu-local-image', result='SUCCESS'), + ], ordered=False) + + resp = self.get_url('api/tenant/tenant-one/images') + data = resp.json() + self.assertEqual(4, len(data)) + self.assertNotIn('build_artifacts', data[0]) + self.assertEqual(1, len(data[1]['build_artifacts'])) + self.assertEqual(1, len(data[2]['build_artifacts'])) + self.assertEqual(1, len(data[1]['build_artifacts'][0]['uploads'])) + self.assertEqual(1, len(data[2]['build_artifacts'][0]['uploads'])) + upload = data[1]['build_artifacts'][0]['uploads'][0] + # Test that unauthenticated access fails + resp = self.delete_url( + f"api/tenant/tenant-one/image-upload/{upload['uuid']}" + ) + self.assertEqual(401, resp.status_code, resp.text) + # Do it again with auth + authz = {'iss': 'zuul_operator', + 'aud': 'zuul.example.com', + 'sub': 'testuser', + 'zuul': { + 'admin': ['tenant-one', ] + }, + 'exp': int(time.time()) + 3600} + token = jwt.encode(authz, key='NoDanaOnlyZuul', + algorithm='HS256') + resp = self.delete_url( + f"api/tenant/tenant-one/image-upload/{upload['uuid']}", + headers={'Authorization': 'Bearer %s' % token}) + self.assertEqual(204, resp.status_code, resp.text) + for _ in iterate_timeout(30, "artifact to be deleted"): + resp = self.get_url('api/tenant/tenant-one/images') + data = resp.json() + if 'build_artifacts' not in data[1]: + break + class TestWebStatusDisplayBranch(BaseTestWeb): tenant_config_file = 'config/change-queues/main.yaml' diff --git a/zuul/launcher/server.py b/zuul/launcher/server.py index d564dd4c2d..bf834114af 100644 --- a/zuul/launcher/server.py +++ b/zuul/launcher/server.py @@ -321,6 +321,7 @@ class Launcher: ) self.image_upload_registry = ImageUploadRegistry( self.zk_client, + self._imageUpdatedCallback ) self.launcher_thread = threading.Thread( diff --git a/zuul/web/__init__.py b/zuul/web/__init__.py index f9b42f4b0f..f699bb2a68 100755 --- a/zuul/web/__init__.py +++ b/zuul/web/__init__.py @@ -1950,6 +1950,42 @@ class ZuulWebAPI(object): build_artifacts, uploads)) return ret + @cherrypy.expose + @cherrypy.tools.json_out(content_type='application/json; charset=utf-8') + @cherrypy.tools.handle_options() + @cherrypy.tools.check_tenant_auth(require_admin=True) + def image_build_artifact_delete(self, tenant_name, tenant, auth, + artifact_id): + iba = self.zuulweb.image_build_registry.getItem(artifact_id) + self.log.info(f'User {auth.uid} requesting ' + 'image-build-artifact-delete on ' + f'{iba}') + + # We just let the LockException propagate up if we can't lock + # it. + with self.zuulweb.createZKContext(None, self.log) as ctx: + with iba.locked(ctx, blocking=False): + with iba.activeContext(ctx): + iba.state = model.STATE_DELETING + cherrypy.response.status = 204 + + @cherrypy.expose + @cherrypy.tools.json_out(content_type='application/json; charset=utf-8') + @cherrypy.tools.handle_options() + @cherrypy.tools.check_tenant_auth(require_admin=True) + def image_upload_delete(self, tenant_name, tenant, auth, upload_id): + upload = self.zuulweb.image_upload_registry.getItem(upload_id) + self.log.info(f'User {auth.uid} requesting image-upload-delete on ' + f'{upload}') + + # We just let the LockException propagate up if we can't lock + # it. + with self.zuulweb.createZKContext(None, self.log) as ctx: + with upload.locked(ctx, blocking=False): + with upload.activeContext(ctx): + upload.state = model.STATE_DELETING + cherrypy.response.status = 204 + @cherrypy.expose @cherrypy.tools.save_params() @cherrypy.tools.json_out(content_type='application/json; charset=utf-8') @@ -2666,6 +2702,18 @@ class ZuulWeb(object): controller=api, action='pipelines') route_map.connect('api', '/api/tenant/{tenant_name}/images', controller=api, action='images') + route_map.connect('api', + '/api/tenant/{tenant_name}/' + 'image-build-artifact/{artifact_id}', + controller=api, + conditions=dict(method=['DELETE']), + action='image_build_artifact_delete') + route_map.connect('api', + '/api/tenant/{tenant_name}/' + 'image-upload/{upload_id}', + controller=api, + conditions=dict(method=['DELETE']), + action='image_upload_delete') route_map.connect('api', '/api/tenant/{tenant_name}/labels', controller=api, action='labels') route_map.connect('api', '/api/tenant/{tenant_name}/nodes', diff --git a/zuul/zk/cache.py b/zuul/zk/cache.py index 6c7f787fdb..e96ba1223a 100644 --- a/zuul/zk/cache.py +++ b/zuul/zk/cache.py @@ -300,17 +300,16 @@ class ZuulTreeCache(abc.ABC): obj = None if data: # Perform an in-place update of the cached object if possible - old_obj = self._cached_objects.get(key) - if old_obj: - if stat.mzxid <= old_obj._zstat.mzxid: - # Don't update to older data - return - if getattr(old_obj, 'lock', None): - # Don't update a locked object - return - old_obj._updateFromRaw(data, stat, None) + + obj = self._cached_objects.get(key) + if obj: + # Don't update to older data + # Don't update a locked object + if (stat.mzxid > obj._zstat.mzxid and + getattr(obj, 'lock', None) is None): + self.updateFromRaw(obj, key, data, stat) else: - obj = self.objectFromDict(data, stat) + obj = self.objectFromRaw(key, data, stat) self._cached_objects[key] = obj else: try: @@ -370,13 +369,27 @@ class ZuulTreeCache(abc.ABC): return None @abc.abstractmethod - def objectFromDict(self, d, key): + def objectFromRaw(self, key, data, stat): """Construct an object from ZooKeeper data - Given a dictionary of data from ZK and cache key, construct + Given data from ZK and cache key, construct and return an object to insert into the cache. - :param dict d: The dictionary. :param object key: The key as returned by parsePath. + :param dict data: The raw data. + :param Zstat stat: The zstat of the znode. + """ + pass + + @abc.abstractmethod + def updateFromRaw(self, obj, key, data, stat): + """Construct an object from ZooKeeper data + + Update an existing object with new data. + + :param object obj: The old object. + :param object key: The key as returned by parsePath. + :param dict data: The raw data. + :param Zstat stat: The zstat of the znode. """ pass diff --git a/zuul/zk/image_registry.py b/zuul/zk/image_registry.py index 6106379396..146ea2402c 100644 --- a/zuul/zk/image_registry.py +++ b/zuul/zk/image_registry.py @@ -32,19 +32,21 @@ class ImageBuildRegistry(LockableZKObjectCache): ) def postCacheHook(self, event, data, stat, key, obj): - if obj is None: - return exists = key in self._cached_objects - builds = self.builds_by_image_name[obj.canonical_name] if exists: - builds.add(key) + if obj: + builds = self.builds_by_image_name[obj.canonical_name] + builds.add(key) else: - builds.discard(key) + if obj: + builds = self.builds_by_image_name[obj.canonical_name] + builds.discard(key) super().postCacheHook(event, data, stat, key, obj) def getArtifactsForImage(self, image_canonical_name): keys = list(self.builds_by_image_name[image_canonical_name]) - arts = [self._cached_objects[key] for key in keys] + arts = [self._cached_objects.get(key) for key in keys] + arts = [a for a in arts if a is not None] # Sort in a stable order, primarily by timestamp, then format # for identical timestamps. arts = sorted(arts, key=lambda x: x.format) @@ -54,11 +56,11 @@ class ImageBuildRegistry(LockableZKObjectCache): class ImageUploadRegistry(LockableZKObjectCache): - def __init__(self, zk_client): + def __init__(self, zk_client, updated_event=None): self.uploads_by_image_name = collections.defaultdict(set) super().__init__( zk_client, - None, + updated_event, root=ImageUpload.ROOT, items_path=ImageUpload.UPLOADS_PATH, locks_path=ImageUpload.LOCKS_PATH, @@ -66,18 +68,20 @@ class ImageUploadRegistry(LockableZKObjectCache): ) def postCacheHook(self, event, data, stat, key, obj): - if obj is None: - return exists = key in self._cached_objects - uploads = self.uploads_by_image_name[obj.canonical_name] if exists: - uploads.add(key) + if obj: + uploads = self.uploads_by_image_name[obj.canonical_name] + uploads.add(key) else: - uploads.discard(key) + if obj: + uploads = self.uploads_by_image_name[obj.canonical_name] + uploads.discard(key) super().postCacheHook(event, data, stat, key, obj) def getUploadsForImage(self, image_canonical_name): keys = list(self.uploads_by_image_name[image_canonical_name]) - uploads = [self._cached_objects[key] for key in keys] + uploads = [self._cached_objects.get(key) for key in keys] + uploads = [u for u in uploads if u is not None] uploads = sorted(uploads, key=lambda x: x.timestamp) return uploads diff --git a/zuul/zk/launcher.py b/zuul/zk/launcher.py index 7269b10040..35490eefb1 100644 --- a/zuul/zk/launcher.py +++ b/zuul/zk/launcher.py @@ -88,14 +88,18 @@ class LockableZKObjectCache(ZuulTreeCache): if not request: return - if self.updated_event: - self.updated_event() request._set(is_locked=exists) def postCacheHook(self, event, data, stat, key, obj): if self.updated_event: self.updated_event() + def objectFromRaw(self, key, data, zstat): + return self.zkobject_class._fromRaw(data, zstat, None) + + def updateFromRaw(self, obj, key, data, zstat): + obj._updateFromRaw(data, zstat, None) + def objectFromDict(self, d, zstat): return self.zkobject_class._fromRaw(d, zstat, None)