From 0b5880d49790f532d9b5bc502f833d38e48ab6f3 Mon Sep 17 00:00:00 2001 From: Chenxiong Qi Date: Jan 11 2018 02:54:55 +0000 Subject: Skip non-RPM advisory by checking advisory content_types Originally, freshmaker skips non-RPM advisory by checking `.tar.gz` extension in the builds. This way does not work in all cases. This patch checks advisory content_types instead. String "rpm" will be included in content_types if an erratum is a RPM advisory, otherwise skip that advisory. * ErrataAdvisoryRPMsSignedHandler._find_images_to_rebuild is updated. It does not check if the advisory being handled currently is a RPM advisory. This patch ensures only RPM advisory is handled. * ErrataAdvisoryStateChangedHandler.can_handle is updated. Only handle RPM advisory by checking if "rpm" exists in advisory content_types. * ErrataAdvisoryStateChangedEvent has a new attribute `content_types` to hold value of advisory content_types. * Manual trigger endpoint is updated as well. Stop and return error in response if advisory of given errata ID is not a RPM advisory. * Tests are updated and added. Signed-off-by: Chenxiong Qi --- diff --git a/freshmaker/errata.py b/freshmaker/errata.py index 1f14fd4..966fb6e 100644 --- a/freshmaker/errata.py +++ b/freshmaker/errata.py @@ -36,13 +36,15 @@ class ErrataAdvisory(object): Represents Errata advisory. """ - def __init__(self, errata_id, name, state, security_impact=None): + def __init__(self, errata_id, name, state, content_types, + security_impact=None): """ Initializes the ErrataAdvisory instance. """ self.errata_id = errata_id self.name = name self.state = state + self.content_types = content_types self.security_impact = security_impact or "" @@ -116,6 +118,7 @@ class Errata(object): "advisory/%s.json" % str(errata["id"])) advisory = ErrataAdvisory( errata["id"], errata["name"], errata["status"], + extra_data['content_types'], extra_data["security_impact"]) advisories.append(advisory) @@ -140,6 +143,7 @@ class Errata(object): data = self.get_advisory(event.errata_id) advisory = ErrataAdvisory( data["id"], data["advisory_name"], data["status"], + data['content_types'], data["security_impact"]) return [advisory] else: diff --git a/freshmaker/events.py b/freshmaker/events.py index bc9a66d..238920a 100644 --- a/freshmaker/events.py +++ b/freshmaker/events.py @@ -257,10 +257,11 @@ class ErrataAdvisoryStateChangedEvent(BaseEvent): Represents change of Errata Advisory status. """ - def __init__(self, msg_id, errata_id, state): + def __init__(self, msg_id, errata_id, state, content_types): super(ErrataAdvisoryStateChangedEvent, self).__init__(msg_id) self.errata_id = errata_id self.state = state + self.content_types = content_types class ErrataAdvisoryRPMsSignedEvent(BaseEvent): diff --git a/freshmaker/handlers/errata/errata_advisory_rpms_signed.py b/freshmaker/handlers/errata/errata_advisory_rpms_signed.py index 7a9c4ff..f969d40 100644 --- a/freshmaker/handlers/errata/errata_advisory_rpms_signed.py +++ b/freshmaker/handlers/errata/errata_advisory_rpms_signed.py @@ -614,20 +614,14 @@ class ErrataAdvisoryRPMsSignedHandler(ContainerBuildHandler): # containing this package and record those images into database. nvrs = errata.get_builds(errata_id) for nvr in nvrs: - # Container images builds end with ".tar.gz", so do not treat - # them as RPMs here. - if not nvr.endswith(".tar.gz"): - self.log_info( - "Going to find all the container images to rebuild as " - "result of %s update.", nvr) - srpm_name = self._find_build_srpm_name(nvr) - batches = lb.find_images_to_rebuild( - srpm_name, content_sets, - filter_fnc=self._filter_out_not_allowed_builds) - yield batches - else: - self.log_info("Skipping unsupported Errata build type: " - "%s.", nvr) + self.log_info( + "Going to find all the container images to rebuild as " + "result of %s update.", nvr) + srpm_name = self._find_build_srpm_name(nvr) + batches = lb.find_images_to_rebuild( + srpm_name, content_sets, + filter_fnc=self._filter_out_not_allowed_builds) + yield batches def _find_build_srpm_name(self, build_nvr): """Find srpm name from a build""" diff --git a/freshmaker/handlers/errata/errata_advisory_state_changed.py b/freshmaker/handlers/errata/errata_advisory_state_changed.py index 6f0922e..fd4b8e3 100644 --- a/freshmaker/handlers/errata/errata_advisory_state_changed.py +++ b/freshmaker/handlers/errata/errata_advisory_state_changed.py @@ -42,7 +42,14 @@ class ErrataAdvisoryStateChangedHandler(BaseHandler): name = 'ErrataAdvisoryStateChangedHandler' def can_handle(self, event): - return isinstance(event, ErrataAdvisoryStateChangedEvent) + if not isinstance(event, ErrataAdvisoryStateChangedEvent): + return False + + if 'rpm' not in event.content_types: + log.info('Skip non-RPM advisory %s.', event.errata_id) + return False + + return True @fail_event_on_handler_exception def mark_as_released(self, errata_id): diff --git a/freshmaker/handlers/internal/manual_rebuild.py b/freshmaker/handlers/internal/manual_rebuild.py index 9c824e5..d91afc3 100644 --- a/freshmaker/handlers/internal/manual_rebuild.py +++ b/freshmaker/handlers/internal/manual_rebuild.py @@ -65,7 +65,7 @@ class FreshmakerManualRebuildHandler(ContainerBuildHandler): advisory = advisories[0] new_event = ErrataAdvisoryStateChangedEvent( manual_rebuild_event.msg_id + "." + str(advisory.name), - advisory.errata_id, advisory.state) + advisory.errata_id, advisory.state, advisory.content_types) new_event.manual = True msg = ("Generated ErrataAdvisoryStateChangedEvent (%s) for errata: %s" % (manual_rebuild_event.msg_id, manual_rebuild_event.errata_id)) diff --git a/freshmaker/parsers/errata/state_change.py b/freshmaker/parsers/errata/state_change.py index 4ff8756..c3cb72c 100644 --- a/freshmaker/parsers/errata/state_change.py +++ b/freshmaker/parsers/errata/state_change.py @@ -21,6 +21,7 @@ from freshmaker.parsers import BaseParser from freshmaker.events import ErrataAdvisoryStateChangedEvent +from freshmaker.errata import Errata class ErrataAdvisoryStateChangedParser(BaseParser): @@ -40,4 +41,6 @@ class ErrataAdvisoryStateChangedParser(BaseParser): status = inner_msg.get('errata_status') errata_id = int(inner_msg.get('errata_id')) - return ErrataAdvisoryStateChangedEvent(msg_id, errata_id, status) + advisory = Errata().get_advisory(errata_id) + return ErrataAdvisoryStateChangedEvent( + msg_id, errata_id, status, advisory['content_types']) diff --git a/freshmaker/views.py b/freshmaker/views.py index cac6a36..a3be032 100644 --- a/freshmaker/views.py +++ b/freshmaker/views.py @@ -26,14 +26,15 @@ from flask import request, jsonify from flask.views import MethodView from freshmaker import app -from freshmaker import types +from freshmaker import messaging from freshmaker import models -from freshmaker.api_utils import pagination_metadata +from freshmaker import types 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_role, require_scopes -from freshmaker import messaging +from freshmaker.errata import Errata api_v1 = { 'event_types': { @@ -232,6 +233,13 @@ class BuildAPI(MethodView): return json_error( 400, 'Bad Request', 'Missing errata_id in request') + advisory = Errata().get_advisory(data['errata_id']) + if 'rpm' not in advisory['content_types']: + return json_error( + 400, + 'Bad Request', + 'Erratum {} is not a RPM advisory'.format(data['errata_id'])) + messaging.publish("manual.rebuild", data) return jsonify(data), 200 diff --git a/tests/test_brew_sign_rpm_handler.py b/tests/test_brew_sign_rpm_handler.py index 728daa7..6bf1589 100644 --- a/tests/test_brew_sign_rpm_handler.py +++ b/tests/test_brew_sign_rpm_handler.py @@ -55,7 +55,7 @@ class TestBrewSignHandler(unittest.TestCase): Tests that handle method returns ErrataAdvisoryRPMsSignedEvent. """ advisories_from_event.return_value = [ - ErrataAdvisory(123, "RHSA-2017", "REL_PREP")] + ErrataAdvisory(123, "RHSA-2017", "REL_PREP", ["rpm"])] builds_signed.return_value = True event = MagicMock() @@ -78,7 +78,7 @@ class TestBrewSignHandler(unittest.TestCase): Tests that allow_build filters out advisories based on advisory_name. """ advisories_from_event.return_value = [ - ErrataAdvisory(123, "RHBA-2017", "REL_PREP")] + ErrataAdvisory(123, "RHBA-2017", "REL_PREP", ["rpm"])] builds_signed.return_value = False event = MagicMock() @@ -100,7 +100,7 @@ class TestBrewSignHandler(unittest.TestCase): advisory_name. """ advisories_from_event.return_value = [ - ErrataAdvisory(123, "RHSA-2017", "REL_PREP")] + ErrataAdvisory(123, "RHSA-2017", "REL_PREP", ["rpm"])] builds_signed.return_value = False event = MagicMock() @@ -120,7 +120,7 @@ class TestBrewSignHandler(unittest.TestCase): Tests that allow_build filters out advisories based on advisory_name. """ advisories_from_event.return_value = [ - ErrataAdvisory(123, "RHBA-2017", "REL_PREP")] + ErrataAdvisory(123, "RHBA-2017", "REL_PREP", ["rpm"])] builds_signed.return_value = False event = MagicMock() @@ -142,7 +142,7 @@ class TestBrewSignHandler(unittest.TestCase): advisory_name. """ advisories_from_event.return_value = [ - ErrataAdvisory(123, "RHSA-2017", "REL_PREP")] + ErrataAdvisory(123, "RHSA-2017", "REL_PREP", ["rpm"])] builds_signed.return_value = False event = MagicMock() @@ -173,7 +173,7 @@ class TestBrewSignHandler(unittest.TestCase): advisory_security_impact. """ advisories_from_event.return_value = [ - ErrataAdvisory(123, "RHSA-2017", "REL_PREP", "Important")] + ErrataAdvisory(123, "RHSA-2017", "REL_PREP", ["rpm"], "Important")] builds_signed.return_value = False event = MagicMock() @@ -204,7 +204,7 @@ class TestBrewSignHandler(unittest.TestCase): advisory_security_impact. """ advisories_from_event.return_value = [ - ErrataAdvisory(123, "RHSA-2017", "REL_PREP", "None")] + ErrataAdvisory(123, "RHSA-2017", "REL_PREP", ["rpm"], "None")] builds_signed.return_value = False event = MagicMock() @@ -227,7 +227,7 @@ class TestBrewSignHandler(unittest.TestCase): """ builds_signed.return_value = True advisories_from_event.return_value = [ - ErrataAdvisory(123, "RHSA-2017", "REL_PREP")] + ErrataAdvisory(123, "RHSA-2017", "REL_PREP", ["rpm"])] event = MagicMock() event.msg_id = "msg_123" diff --git a/tests/test_errata.py b/tests/test_errata.py index 53fba65..e043834 100644 --- a/tests/test_errata.py +++ b/tests/test_errata.py @@ -73,6 +73,7 @@ class MockedErrataAPI(object): "id": 28484, "advisory_name": "RHSA-2017:28484", "status": "QE", + "content_types": ["rpm"], "security_impact": "Important", "product": { "id": 89 @@ -143,7 +144,7 @@ class TestErrata(unittest.TestCase): def test_advisories_from_event_errata_state_change_event( self, errata_http_get, errata_rest_get): MockedErrataAPI(errata_rest_get, errata_http_get) - event = ErrataAdvisoryStateChangedEvent("msgid", 28484, "SHIPPED_LIVE") + event = ErrataAdvisoryStateChangedEvent("msgid", 28484, "SHIPPED_LIVE", ['rpm']) advisories = self.errata.advisories_from_event(event) self.assertEqual(len(advisories), 1) self.assertEqual(advisories[0].errata_id, 28484) diff --git a/tests/test_errata_advisory_state_changed.py b/tests/test_errata_advisory_state_changed.py index dcd7e32..1057de2 100644 --- a/tests/test_errata_advisory_state_changed.py +++ b/tests/test_errata_advisory_state_changed.py @@ -732,25 +732,6 @@ class TestPrepareYumRepo(unittest.TestCase): "of advisory 123 is the latest build in its candidate tag.")) -class TestFindImagesToRebuild(unittest.TestCase): - - @patch('freshmaker.handlers.errata.errata_advisory_rpms_signed.Errata') - @patch('freshmaker.handlers.errata.errata_advisory_rpms_signed.Pulp') - @patch('freshmaker.handlers.errata.errata_advisory_rpms_signed.LightBlue') - def test_find_images_to_rebuild_non_rpm_content( - self, lb, pulp, errata): - """ - Tests that _find_images_to_rebuild is not called for - non-rpm content. - """ - errata.return_value.get_builds.return_value = set(["httpd-2.4.15-1.f27.tar.gz"]) - - handler = ErrataAdvisoryRPMsSignedHandler() - ret = list(handler._find_images_to_rebuild(12345)) - lb.find_images_to_rebuild.assert_not_called() - self.assertEqual([], ret) - - class TestErrataAdvisoryStateChangedHandler(unittest.TestCase): def setUp(self): @@ -770,8 +751,8 @@ class TestErrataAdvisoryStateChangedHandler(unittest.TestCase): for state in ["REL_PREP", "PUSH_READY", "IN_PUSH", "SHIPPED_LIVE"]: advisories_from_event.return_value = [ - ErrataAdvisory(123, "RHSA-2017", state, "Critical")] - ev = ErrataAdvisoryStateChangedEvent("msg123", 123, state) + ErrataAdvisory(123, "RHSA-2017", state, ["rpm"], "Critical")] + ev = ErrataAdvisoryStateChangedEvent("msg123", 123, state, ['rpm']) ret = handler.handle(ev) self.assertEqual(len(ret), 1) @@ -795,8 +776,8 @@ class TestErrataAdvisoryStateChangedHandler(unittest.TestCase): for state in ["NEW_FILES", "QE", "UNKNOWN"]: advisories_from_event.return_value = [ - ErrataAdvisory(123, "RHSA-2017", state, "Critical")] - ev = ErrataAdvisoryStateChangedEvent("msg123", 123, state) + ErrataAdvisory(123, "RHSA-2017", state, ["rpm"], "Critical")] + ev = ErrataAdvisoryStateChangedEvent("msg123", 123, state, ['rpm']) ret = handler.handle(ev) self.assertEqual(len(ret), 0) @@ -817,8 +798,8 @@ class TestErrataAdvisoryStateChangedHandler(unittest.TestCase): db.session.commit() for state in ["REL_PREP", "PUSH_READY", "IN_PUSH", "SHIPPED_LIVE"]: advisories_from_event.return_value = [ - ErrataAdvisory(123, "RHSA-2017", state, "Critical")] - ev = ErrataAdvisoryStateChangedEvent("msg123", 123, state) + ErrataAdvisory(123, "RHSA-2017", state, ["rpm"], "Critical")] + ev = ErrataAdvisoryStateChangedEvent("msg123", 123, state, ['rpm']) ret = handler.handle(ev) if db_event_state == EventState.FAILED: @@ -833,7 +814,7 @@ class TestErrataAdvisoryStateChangedHandler(unittest.TestCase): handler = ErrataAdvisoryStateChangedHandler() for state in ["REL_PREP", "PUSH_READY", "IN_PUSH", "SHIPPED_LIVE"]: - ev = ErrataAdvisoryStateChangedEvent("msg123", 123, state) + ev = ErrataAdvisoryStateChangedEvent("msg123", 123, state, ['rpm']) ret = handler.handle(ev) self.assertEqual(len(ret), 0) @@ -845,7 +826,7 @@ class TestErrataAdvisoryStateChangedHandler(unittest.TestCase): self.assertEqual(db_event.released, False) - ev = ErrataAdvisoryStateChangedEvent("msg123", 123, "SHIPPED_LIVE") + ev = ErrataAdvisoryStateChangedEvent("msg123", 123, "SHIPPED_LIVE", ["rpm"]) handler = ErrataAdvisoryStateChangedHandler() handler.handle(ev) @@ -859,7 +840,7 @@ class TestErrataAdvisoryStateChangedHandler(unittest.TestCase): db.session.commit() for state in ["NEW_FILES", "QE", "REL_PREP", "PUSH_READY", "IN_PUSH"]: - ev = ErrataAdvisoryStateChangedEvent("msg123", 123, state) + ev = ErrataAdvisoryStateChangedEvent("msg123", 123, state, ['rpm']) handler = ErrataAdvisoryStateChangedHandler() handler.handle(ev) @@ -869,7 +850,7 @@ class TestErrataAdvisoryStateChangedHandler(unittest.TestCase): @patch('freshmaker.errata.Errata.advisories_from_event') def test_mark_as_released_unknown_event(self, advisories_from_event): - ev = ErrataAdvisoryStateChangedEvent("msg123", 123, "SHIPPED_LIVE") + ev = ErrataAdvisoryStateChangedEvent("msg123", 123, "SHIPPED_LIVE", ["rpm"]) handler = ErrataAdvisoryStateChangedHandler() handler.handle(ev) @@ -894,7 +875,7 @@ class TestErrataAdvisoryStateChangedHandler(unittest.TestCase): db.session.commit() event = ErrataAdvisoryStateChangedEvent( - 'msg-id-123', 123456, 'SHIPPED_LIVE') + 'msg-id-123', 123456, 'SHIPPED_LIVE', ['rpm']) handler = ErrataAdvisoryStateChangedHandler() msgs = handler.handle(event) @@ -920,7 +901,7 @@ class TestErrataAdvisoryStateChangedHandler(unittest.TestCase): db.session.commit() event = ErrataAdvisoryStateChangedEvent( - 'msg-id-123', 123456, 'SHIPPED_LIVE') + 'msg-id-123', 123456, 'SHIPPED_LIVE', ['rpm']) event.manual = True handler = ErrataAdvisoryStateChangedHandler() msgs = handler.handle(event) @@ -1359,3 +1340,18 @@ class TestPrepareYumReposForRebuilds(unittest.TestCase): 'http://localhost/repo/3', 'http://localhost/repo/4', ], sorted(urls)) + + +class TestSkipNonRPMAdvisory(unittest.TestCase): + + def test_ensure_to_handle_rpm_adivsory(self): + event = ErrataAdvisoryStateChangedEvent( + 'msg-id-1', 123, 'REL_PREP', ['rpm', 'jar', 'pom']) + handler = ErrataAdvisoryStateChangedHandler() + self.assertTrue(handler.can_handle(event)) + + def test_not_handle_non_rpm_advisory(self): + event = ErrataAdvisoryStateChangedEvent( + 'msg-id-1', 123, 'REL_PREP', ['docker']) + handler = ErrataAdvisoryStateChangedHandler() + self.assertFalse(handler.can_handle(event)) diff --git a/tests/test_freshmaker_manual_rebuild_handler.py b/tests/test_freshmaker_manual_rebuild_handler.py index 0d40ad8..8fa59cd 100644 --- a/tests/test_freshmaker_manual_rebuild_handler.py +++ b/tests/test_freshmaker_manual_rebuild_handler.py @@ -52,7 +52,7 @@ class TestFreshmakerManualRebuildHandler(unittest.TestCase): handler = FreshmakerManualRebuildHandler() advisories_from_event.return_value = [ - ErrataAdvisory(123, "RHSA-2017", "REL_PREP", "Critical")] + ErrataAdvisory(123, "RHSA-2017", "REL_PREP", ["rpm"], "Critical")] ev = FreshmakerManualRebuildEvent("msg123", errata_id=123) ret = handler.handle(ev) diff --git a/tests/test_views.py b/tests/test_views.py index a6dd5f3..6f164df 100644 --- a/tests/test_views.py +++ b/tests/test_views.py @@ -415,13 +415,16 @@ class TestManualTriggerRebuild(unittest.TestCase): self.client = app.test_client() def tearDown(self): - db.session.remove() db.drop_all() db.session.commit() @patch('freshmaker.messaging.publish') - def test_manual_rebuild(self, publish): + @patch('freshmaker.views.Errata') + def test_manual_rebuild(self, Errata, publish): + errata = Errata.return_value + errata.get_advisory.return_value = {'content_types': ['rpm']} + resp = self.client.post('/api/1/builds/', data=json.dumps({'errata_id': 1}), content_type='application/json') @@ -430,6 +433,24 @@ class TestManualTriggerRebuild(unittest.TestCase): self.assertEqual(data["errata_id"], 1) publish.assert_called_once_with('manual.rebuild', {u'errata_id': 1}) + @patch('freshmaker.views.Errata') + def test_not_rebuild_nonrpm_advisory(self, Errata): + errata = Errata.return_value + errata.get_advisory.return_value = {'content_types': ['docker']} + + resp = self.client.post('/api/1/builds/', + data=json.dumps({'errata_id': 1}), + content_type='application/json') + data = json.loads(resp.get_data(as_text=True)) + + self.assertEqual( + { + 'status': 400, + 'error': 'Bad Request', + 'message': 'Erratum 1 is not a RPM advisory' + }, + data) + class TestOpenIDCLogin(ViewBaseTest): """Test that OpenIDC login"""