From a74c03499e22698a3c9cb682c331a1762d5b9835 Mon Sep 17 00:00:00 2001 From: Qixiang Wan Date: Feb 18 2020 06:28:02 +0000 Subject: [PATCH 1/2] Add async build view An async build can be requested via '/api/{api_version}/async-builds' with POST method, the requested data will be validated by server and then a message will be published with topic 'async.manual.build'. --- diff --git a/freshmaker/models.py b/freshmaker/models.py index 09059e2..e143b73 100644 --- a/freshmaker/models.py +++ b/freshmaker/models.py @@ -45,7 +45,8 @@ from freshmaker.events import ( BodhiUpdateCompleteStableEvent, KojiTaskStateChangeEvent, BrewSignRPMEvent, ErrataAdvisoryRPMsSignedEvent, BrewContainerTaskStateChangeEvent, ErrataAdvisoryStateChangedEvent, FreshmakerManualRebuildEvent, - ODCSComposeStateChangeEvent, ManualRebuildWithAdvisoryEvent) + ODCSComposeStateChangeEvent, ManualRebuildWithAdvisoryEvent, + FreshmakerAsyncManualBuildEvent) EVENT_TYPES = { MBSModuleStateChangeEvent: 0, @@ -62,6 +63,7 @@ EVENT_TYPES = { FreshmakerManualRebuildEvent: 11, ODCSComposeStateChangeEvent: 12, ManualRebuildWithAdvisoryEvent: 13, + FreshmakerAsyncManualBuildEvent: 14, } INVERSE_EVENT_TYPES = {v: k for k, v in EVENT_TYPES.items()} diff --git a/freshmaker/parsers/internal/async_manual_build.py b/freshmaker/parsers/internal/async_manual_build.py index 8fcc1ba..37b89ae 100644 --- a/freshmaker/parsers/internal/async_manual_build.py +++ b/freshmaker/parsers/internal/async_manual_build.py @@ -19,6 +19,7 @@ # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE # SOFTWARE. +import time from freshmaker.parsers import BaseParser from freshmaker.events import FreshmakerAsyncManualBuildEvent @@ -32,14 +33,25 @@ class FreshmakerAsyncManualbuildParser(BaseParser): def can_parse(self, topic, msg): return any([topic.endswith(s) for s in self.topic_suffixes]) + def parse_post_data(self, data): + """ + Method shared between Frontend and Backend to parse the POST data + of async build JSON and generate the BaseEvent representation + of the rebuild request. + + :param dict data: Dict generated from JSON from HTTP POST or parsed + from the UMB message sent from Frontend to Backend. + """ + msg_id = data.get('msg_id', "async_build_%s" % (str(time.time()))) + + event = FreshmakerAsyncManualBuildEvent( + msg_id, data.get('dist_git_branch'), data.get('container_images', []), + freshmaker_event_id=data.get('freshmaker_event_id'), + brew_target=data.get('brew_target'), + dry_run=data.get('dry_run', False)) + + return event + def parse(self, topic, msg): inner_msg = msg['msg'] - - return FreshmakerAsyncManualBuildEvent( - inner_msg['msg_id'], - inner_msg['dist_git_branch'], - inner_msg['container_images'], - freshmaker_event_id=inner_msg.get('freshmaker_event_id'), - brew_target=inner_msg.get('brew_target'), - dry_run=inner_msg.get('dry_run'), - ) + return self.parse_post_data(inner_msg) diff --git a/freshmaker/views.py b/freshmaker/views.py index cb642f0..3f1c3da 100644 --- a/freshmaker/views.py +++ b/freshmaker/views.py @@ -34,12 +34,14 @@ from freshmaker import db from freshmaker import conf from freshmaker import version from freshmaker import log +from freshmaker import events from freshmaker.api_utils import filter_artifact_builds from freshmaker.api_utils import filter_events from freshmaker.api_utils import json_error from freshmaker.api_utils import pagination_metadata from freshmaker.auth import login_required, requires_roles, require_scopes, user_has_role from freshmaker.parsers.internal.manual_rebuild import FreshmakerManualRebuildParser +from freshmaker.parsers.internal.async_manual_build import FreshmakerAsyncManualbuildParser from freshmaker.monitor import ( monitor_api, freshmaker_build_api_latency, freshmaker_event_api_latency) from freshmaker.image_verifier import ImageVerifier @@ -127,6 +129,14 @@ api_v1 = { } }, }, + 'async_builds': { + 'async_build': { + 'url': '/api/1/async-builds/', + 'options': { + 'methods': ['POST'], + } + }, + }, 'about': { 'about': { 'url': '/api/1/about/', @@ -348,6 +358,79 @@ class EventAPI(MethodView): return jsonify(event.json()), 200 +def _validate_rebuild_request(request): + """ + Perform basic data validation against the rebuild request + + :param request: Flask request object. + :return: If validation fails, returns JSON serialized flask.Response with + error code and messages, otherwise returns None. + """ + data = request.get_json(force=True) + + for key in ('errata_id', 'freshmaker_event_id'): + if data.get(key) and not isinstance(data[key], int): + return json_error(400, 'Bad Request', f'"{key}" must be an integer.') + + if data.get('freshmaker_event_id'): + event = models.Event.get_by_event_id(db.session, data.get('freshmaker_event_id')) + if not event: + return json_error( + 400, 'Bad Request', 'The provided "freshmaker_event_id" is invalid.', + ) + + for key in ('dist_git_branch', 'brew_target'): + if data.get(key) and not isinstance(data[key], str): + return json_error(400, 'Bad Request', f'"{key}" must be a string.') + + container_images = data.get('container_images', []) + if ( + not isinstance(container_images, list) or + any(not isinstance(image, str) for image in container_images) + ): + return json_error( + 400, 'Bad Request', '"container_images" must be an array of strings.', + ) + + if not isinstance(data.get('dry_run', False), bool): + return json_error(400, 'Bad Request', '"dry_run" must be a boolean.') + + return None + + +def _create_rebuild_event_from_request(db_session, parser, request): + """ + Create a rebuild event by parsing the request data + + :param db_session: SQLAlchemy database session object. + :param parser: Freshmaker parser object. + :param request: Flask request object. + :return: Event object. + """ + data = request.get_json(force=True) + event = parser.parse_post_data(data) + + # Store the event into database, so it gets the ID which we can return + # to client sending this POST request. The client can then use the ID + # to check for the event status. + db_event = models.Event.get_or_create_from_event(db_session, event) + db_event.requester = g.user.username + db_event.requested_rebuilds = " ".join(event.container_images) + if hasattr(event, 'requester_metadata_json') and event.requester_metadata_json: + db_event.requester_metadata = json.dumps(event.requester_metadata_json) + if data.get('freshmaker_event_id'): + dependent_event = models.Event.get_by_event_id( + db_session, data.get('freshmaker_event_id'), + ) + if dependent_event: + dependency = db_event.add_event_dependency(db_session, dependent_event) + if not dependency: + log.warn('Dependency between {} and {} could not be added!'.format( + event.freshmaker_event_id, dependent_event.id)) + db_session.commit() + return db_event + + class BuildAPI(MethodView): @freshmaker_build_api_latency.time() def get(self, id): @@ -406,23 +489,11 @@ class BuildAPI(MethodView): :statuscode 200: A new event was created. :statuscode 400: The provided input is invalid. """ - data = request.get_json(force=True) - for key in ('errata_id', 'freshmaker_event_id'): - if data.get(key) and not isinstance(data[key], int): - return json_error(400, 'Bad Request', f'"{key}" must be an integer.') - - container_images = data.get('container_images', []) - if ( - not isinstance(container_images, list) or - any(not isinstance(image, str) for image in container_images) - ): - return json_error( - 400, 'Bad Request', '"container_images" must be an array of strings.', - ) - - if not isinstance(data.get('dry_run', False), bool): - return json_error(400, 'Bad Request', '"dry_run" must be a boolean.') + error = _validate_rebuild_request(request) + if error is not None: + return error + data = request.get_json(force=True) if not data.get('errata_id') and not data.get('freshmaker_event_id'): return json_error( 400, @@ -435,11 +506,14 @@ class BuildAPI(MethodView): dependent_event = models.Event.get_by_event_id( db.session, data.get('freshmaker_event_id'), ) - if not dependent_event: + # requesting a CVE rebuild, the event can not be an async build event which + # is for non-CVE only + async_build_event_type = models.EVENT_TYPES[events.FreshmakerAsyncManualBuildEvent] + if dependent_event.event_type_id == async_build_event_type: return json_error( - 400, 'Bad Request', 'The provided "freshmaker_event_id" is invalid.', + 400, 'Bad Request', 'The event (id={}) is an async build event, ' + 'can not be used for this build.'.format(data.get('freshmaker_event_id')), ) - if not data.get('errata_id'): data['errata_id'] = int(dependent_event.search_key) elif int(dependent_event.search_key) != data['errata_id']: @@ -454,33 +528,104 @@ class BuildAPI(MethodView): # event based on the data. Currently it generates just # ManualRebuildWithAdvisoryEvent. parser = FreshmakerManualRebuildParser() - event = parser.parse_post_data(data) - - # Store the event into database, so it gets the ID which we can return - # to client sending this POST request. The client can then use the ID - # to check for the event status. - db_event = models.Event.get_or_create_from_event(db.session, event) - db_event.requester = g.user.username - db_event.requested_rebuilds = " ".join(event.container_images) - if event.requester_metadata_json: - db_event.requester_metadata = json.dumps(event.requester_metadata_json) - if dependent_event: - dependency = db_event.add_event_dependency(db.session, dependent_event) - if not dependency: - log.warn('Dependency between {} and {} could not be added!'.format( - event.freshmaker_event_id, dependent_event.id)) - db.session.commit() + db_event = _create_rebuild_event_from_request(db.session, parser, request) # Forward the POST data (including the msg_id of the database event we # added to DB) to backend using UMB messaging. Backend will then # re-generate the event and start handling it. - data["msg_id"] = event.msg_id + data["msg_id"] = db_event.message_id messaging.publish("manual.rebuild", data) # Return back the JSON representation of Event to client. return jsonify(db_event.json()), 200 +class AsyncBuildAPI(MethodView): + @login_required + @require_scopes('submit-build') + @requires_roles(['admin', 'freshmaker_async_rebuilders']) + def post(self): + """ + Trigger Freshmaker async rebuild (a.k.a non-CVE rebuild). The request + must be :mimetype:`application/json`. + + Returns the newly created Freshmaker event as JSON. + + **Sample request**: + + .. sourcecode:: http + + POST /api/1/async-builds HTTP/1.1 + Accept: application/json + Content-Type: application/json + + { + "dist_git_branch": "master", + "container_images": ["foo-1-1"] + } + + :jsonparam string dist_git_branch: The name of the branch in dist-git + to build the container images from. This is a mandatory field. + :jsonparam list container_images: A list of images to rebuild. They + might be sharing a parent-child relationship which are then rebuilt + by Freshmaker in the right order. For example, if images A is parent + image of B, which is parent image of C, and container_images is + [A, B, C], Freshmaker will make sure to rebuild all three images, + in the correct order. It is however possible also to rebuild images + completely unrelated to each other. This is a mandatory field. + :jsonparam bool dry_run: When True, the Event will be handled in + the DRY_RUN mode. + :jsonparam bool freshmaker_event_id: When set, it defines the event + which will be used as the dependant event. Successfull builds from + this event will be reused in the newly created event instead of + building all the artifacts from scratch. The event should refer + to an async rebuild event. + :jsonparam string brew_target: The name of the Brew target. While + requesting an async rebuild, it should be the same for all the images + in the list of container_images. This parameter is optional, with + default value will be pulled from the previous buildContainer task. + :statuscode 200: A new event was created. + :statuscode 400: The provided input is invalid. + """ + error = _validate_rebuild_request(request) + if error is not None: + return error + + data = request.get_json(force=True) + if not all([data.get('dist_git_branch'), data.get('container_images')]): + return json_error( + 400, + 'Bad Request', + '"dist_git_branch" and "container_images" are required in the request ' + 'for async builds', + ) + + dependent_event = None + if data.get('freshmaker_event_id'): + dependent_event = models.Event.get_by_event_id( + db.session, data.get('freshmaker_event_id'), + ) + async_build_event_type = models.EVENT_TYPES[events.FreshmakerAsyncManualBuildEvent] + if dependent_event.event_type_id != async_build_event_type: + return json_error( + 400, 'Bad Request', 'The event (id={}) is not an async build ' + 'event.'.format(data.get('freshmaker_event_id')), + ) + + # parse the POST data and generate FreshmakerAsyncManualBuildEvent + parser = FreshmakerAsyncManualbuildParser() + db_event = _create_rebuild_event_from_request(db.session, parser, request) + + # Forward the POST data (including the msg_id of the database event we + # added to DB) to backend using UMB messaging. Backend will then + # re-generate the event and start handling it. + data["msg_id"] = db_event.message_id + messaging.publish("async.manual.build", data) + + # Return back the JSON representation of Event to client. + return jsonify(db_event.json()), 200 + + class AboutAPI(MethodView): def get(self): json = {'version': version} @@ -583,6 +728,7 @@ class VerifyImageRepositoryAPI(MethodView): API_V1_MAPPING = { 'events': EventAPI, 'builds': BuildAPI, + 'async_builds': AsyncBuildAPI, 'event_types': EventTypeAPI, 'build_types': BuildTypeAPI, 'build_states': BuildStateAPI, diff --git a/tests/test_views.py b/tests/test_views.py index bc7802b..83ffed7 100644 --- a/tests/test_views.py +++ b/tests/test_views.py @@ -874,6 +874,175 @@ class TestManualTriggerRebuild(ViewBaseTest): self.assertEqual(resp.status_code, 400) self.assertEqual(resp.json['message'], '"dry_run" must be a boolean.') + def test_manual_rebuild_with_async_event(self): + models.Event.create( + db.session, '2017-00000000-0000-0000-0000-000000000003', '123', + events.FreshmakerAsyncManualBuildEvent + ) + db.session.commit() + with patch('freshmaker.models.datetime') as datetime_patch: + datetime_patch.utcnow.return_value = datetime.datetime(2017, 8, 21, 13, 42, 20) + + payload = { + 'container_images': ['foo-1-1', 'bar-1-1'], + 'freshmaker_event_id': 1, + } + with self.test_request_context(user='root'): + resp = self.client.post( + '/api/1/builds/', + data=json.dumps(payload), + content_type='application/json', + ) + self.assertEqual(resp.status_code, 400) + self.assertEqual( + resp.json['message'], + 'The event (id=1) is an async build event, can not be used for this build.') + + +class TestAsyncBuild(ViewBaseTest): + def setUp(self): + super(TestAsyncBuild, self).setUp() + self.client = app.test_client() + + @patch('freshmaker.messaging.publish') + @patch('freshmaker.parsers.internal.async_manual_build.time.time') + def test_async_build(self, time, publish): + time.return_value = 123 + with patch('freshmaker.models.datetime') as datetime_patch: + datetime_patch.utcnow.return_value = datetime.datetime(2017, 8, 21, 13, 42, 20) + + payload = { + 'dist_git_branch': 'master', + 'container_images': ['foo-1-1', 'bar-1-1'] + } + with self.test_request_context(user='root'): + resp = self.client.post( + '/api/1/async-builds/', + data=json.dumps(payload), + content_type='application/json', + ) + data = json.loads(resp.get_data(as_text=True)) + + self.assertEqual(data, { + u'builds': [], + u'depending_events': [], + u'depends_on_events': [], + u'dry_run': False, + u'event_type_id': 14, + u'id': 1, + u'message_id': 'async_build_123', + u'requested_rebuilds': ['foo-1-1', 'bar-1-1'], + u'requester': 'root', + u'requester_metadata': {}, + u'search_key': 'async_build_123', + u'state': 0, + u'state_name': 'INITIALIZED', + u'state_reason': None, + u'time_created': '2017-08-21T13:42:20Z', + u'time_done': None, + u'url': '/api/1/events/1'}) + + publish.assert_called_once_with( + 'async.manual.build', + { + 'msg_id': 'async_build_123', + 'dist_git_branch': 'master', + 'container_images': ['foo-1-1', 'bar-1-1'] + }) + + @patch('freshmaker.messaging.publish') + @patch('freshmaker.parsers.internal.async_manual_build.time.time') + def test_async_build_dry_run(self, time, publish): + time.return_value = 123 + + payload = { + 'dist_git_branch': 'master', + 'container_images': ['foo-1-1', 'bar-1-1'], + 'dry_run': True + } + with self.test_request_context(user='root'): + resp = self.client.post( + '/api/1/async-builds/', json=payload, content_type='application/json') + + data = json.loads(resp.get_data(as_text=True)) + + self.assertEqual(data['dry_run'], True) + publish.assert_called_once_with( + 'async.manual.build', + { + 'msg_id': 'async_build_123', + 'dist_git_branch': 'master', + 'container_images': ['foo-1-1', 'bar-1-1'], + 'dry_run': True, + }) + + def test_async_build_with_non_async_event(self): + models.Event.create( + db.session, '2017-00000000-0000-0000-0000-000000000003', '123', events.TestingEvent, + ) + db.session.commit() + with patch('freshmaker.models.datetime') as datetime_patch: + datetime_patch.utcnow.return_value = datetime.datetime(2017, 8, 21, 13, 42, 20) + + payload = { + 'dist_git_branch': 'master', + 'container_images': ['foo-1-1', 'bar-1-1'], + 'freshmaker_event_id': 1, + } + with self.test_request_context(user='root'): + resp = self.client.post( + '/api/1/async-builds/', + data=json.dumps(payload), + content_type='application/json', + ) + self.assertEqual(resp.status_code, 400) + self.assertEqual(resp.json['message'], 'The event (id=1) is not an async build event.') + + def test_async_build_invalid_dist_git_branch(self): + payload = {'dist_git_branch': 123} + with self.test_request_context(user='root'): + resp = self.client.post( + '/api/1/async-builds/', json=payload, content_type='application/json') + + self.assertEqual(resp.status_code, 400) + self.assertEqual(resp.json['message'], '"dist_git_branch" must be a string.') + + def test_async_build_invalid_type_freshmaker_event_id(self): + payload = {'freshmaker_event_id': '123'} + with self.test_request_context(user='root'): + resp = self.client.post( + '/api/1/async-builds/', json=payload, content_type='application/json') + + self.assertEqual(resp.status_code, 400) + self.assertEqual(resp.json['message'], '"freshmaker_event_id" must be an integer.') + + def test_async_build_invalid_type_container_images(self): + payload = {'container_images': '123'} + with self.test_request_context(user='root'): + resp = self.client.post( + '/api/1/async-builds/', json=payload, content_type='application/json') + + self.assertEqual(resp.status_code, 400) + self.assertEqual(resp.json['message'], '"container_images" must be an array of strings.') + + def test_async_build_invalid_type_brew_target(self): + payload = {'brew_target': 123} + with self.test_request_context(user='root'): + resp = self.client.post( + '/api/1/async-builds/', json=payload, content_type='application/json') + + self.assertEqual(resp.status_code, 400) + self.assertEqual(resp.json['message'], '"brew_target" must be a string.') + + def test_async_build_invalid_type_dry_run(self): + payload = {'dry_run': '123'} + with self.test_request_context(user='root'): + resp = self.client.post( + '/api/1/async-builds/', json=payload, content_type='application/json') + + self.assertEqual(resp.status_code, 400) + self.assertEqual(resp.json['message'], '"dry_run" must be a boolean.') + class TestPatchAPI(ViewBaseTest): def test_patch_event_cancel(self): From ee5d2f9f2b7ca019d389fe021d548d73c015f6ba Mon Sep 17 00:00:00 2001 From: Qixiang Wan Date: Feb 18 2020 06:28:07 +0000 Subject: [PATCH 2/2] Fix authentication failure in testing Prior to version 0.5, flask_login get user_id from session['user_id'], and then in 0.5, it's changed to session['_user_id'], so we need to set both in testing for compatibility. --- diff --git a/tests/test_views.py b/tests/test_views.py index 83ffed7..9b59d8a 100644 --- a/tests/test_views.py +++ b/tests/test_views.py @@ -89,7 +89,12 @@ class ViewBaseTest(helpers.ModelsTestCase): else: flask.g.groups = [] with self.client.session_transaction() as sess: + # prior to version 0.5, flask_login gets user_id from + # session['user_id'], and then in version 0.5, it's + # changed to get from session['_user_id'], so we set + # both here to make it work for both old and new versions sess['user_id'] = user + sess['_user_id'] = user sess['_fresh'] = True oidc_scopes = oidc_scopes if oidc_scopes else []