From 4ed1b66c03501cd733b15b64d292dbf780ea55fd Mon Sep 17 00:00:00 2001 From: Luiz Carvalho Date: May 20 2020 13:10:41 +0000 Subject: [PATCH 1/2] Try hard to find the repositories for an image An accurate list of repositories is important to ensure the deduplication code works as expected. If Freshmaker has built an image in the past, it may not have been directly published and Lightblue won't list any repositories for it. In such case, try to get a list of repositories from the original NVR. Signed-off-by: Luiz Carvalho WIP: adding intital unit tests Signed-off-by: Luiz Carvalho --- diff --git a/freshmaker/lightblue.py b/freshmaker/lightblue.py index 4a250a8..8f59914 100644 --- a/freshmaker/lightblue.py +++ b/freshmaker/lightblue.py @@ -419,6 +419,29 @@ class ContainerImage(dict): return return rpm_manifest["rpms"] + def get_registry_repositories(self, lb_instance): + if self['repositories']: + return self['repositories'] + + parsed_nvr = kobo.rpmlib.parse_nvr(self.nvr) + + if '.' not in parsed_nvr['release']: + log.debug('There are no repositories for %s', self.nvr) + return [] + + original_release = parsed_nvr['release'].rsplit('.', 1)[0] + parsed_nvr['release'] = original_release + original_nvr = '{name}-{version}-{release}'.format(**parsed_nvr) + log.debug('Finding repositories for %s through %s', self.nvr, original_nvr) + + previous_images = lb_instance.get_images_by_nvrs( + [original_nvr], published=None, include_rpm_manifest=False) + if not previous_images: + log.warning('original_nvr %s not found in Lightblue', original_nvr) + return [] + + return previous_images[0].get_registry_repositories(lb_instance) + class LightBlue(object): """Interface to query lightblue""" @@ -1248,12 +1271,7 @@ class LightBlue(object): for image_id, images in enumerate(to_rebuild): for parent_id, image in enumerate(images): nvr = image.nvr - # Also include the sorted names of repositories in the image group - # to handle the case when different releases of single name-version are - # included in different container repositories. - repository_key = "-".join(sorted([r["repository"] for r in image["repositories"]])) - parsed_nvr = koji.parse_NVR(nvr) - image_group = "%s-%s-%s" % (parsed_nvr["name"], parsed_nvr["version"], repository_key) + image_group = self.describe_image_group(image) if image_group not in image_group_to_nvrs: image_group_to_nvrs[image_group] = [] if nvr not in image_group_to_nvrs[image_group]: @@ -1349,6 +1367,17 @@ class LightBlue(object): return to_rebuild + # Cache to avoid multiple calls. We want one call per nvr, not one per arch + @region.cache_on_arguments(to_str=lambda image: image.nvr) + def describe_image_group(self, image): + # Also include the sorted names of repositories in the image group + # to handle the case when different releases of single name-version are + # included in different container repositories. + repositories = image.get_registry_repositories(self) + repository_key = sorted([r["repository"] for r in repositories]) + parsed_nvr = koji.parse_NVR(image.nvr) + return "%s-%s-%s" % (parsed_nvr["name"], parsed_nvr["version"], repository_key) + def _images_to_rebuild_to_batches(self, to_rebuild, directly_affected_nvrs): """ Creates batches with images as defined by `find_images_to_rebuild` diff --git a/tests/test_lightblue.py b/tests/test_lightblue.py index 9f0b281..9a4ca68 100644 --- a/tests/test_lightblue.py +++ b/tests/test_lightblue.py @@ -1749,6 +1749,107 @@ class TestQueryEntityFromLightBlue(helpers.FreshmakerTestCase): self.assertEqual(len(ret), 1) self.assertIsNotNone(ret[0][0].get("parent")) + @patch("freshmaker.lightblue.LightBlue.get_images_by_nvrs") + @patch('freshmaker.lightblue.LightBlue.find_images_with_packages_from_content_set') + @patch('freshmaker.lightblue.LightBlue.find_parent_images_with_package') + @patch('os.path.exists') + def test_dedupe_dependency_images_with_all_repositories( + self, exists, find_parent_images_with_package, + find_images_with_packages_from_content_set, get_images_by_nvrs): + exists.return_value = True + + vulnerable_srpm_name = 'oh-noes' + vulnerable_srpm_nvr = '{}-1.0-1'.format(vulnerable_srpm_name) + + ubi_image_template = { + "brew": {"package": "ubi8-container", "build": "ubi8-container-8.1-100"}, + "parent_image_builds": {}, + "repository": "containers/ubi8", + "commit": "2b868f757977782367bf624373a5fe3d8e6bacd6", + "repositories": [{"repository": "ubi8"}], + "rpm_manifest": [{ + "rpms": [ + {"srpm_name": vulnerable_srpm_name} + ] + }] + } + + directly_affected_ubi_image = ContainerImage.create(copy.deepcopy(ubi_image_template)) + + dependency_ubi_image_data = copy.deepcopy(ubi_image_template) + dependency_ubi_image_nvr = directly_affected_ubi_image.nvr + ".12345678" + dependency_ubi_image_data["brew"]["build"] = dependency_ubi_image_nvr + # A dependecy image is not directly published + dependency_ubi_image_data["repositories"] = [] + dependency_ubi_image = ContainerImage.create(dependency_ubi_image_data) + + python_image = ContainerImage.create({ + "brew": {"package": "python-36-container", "build": "python-36-container-1-10"}, + "parent_brew_build": directly_affected_ubi_image.nvr, + "parent_image_builds": {}, + "repository": "containers/python-36", + "commit": "3a740231deab2abf335d5cad9a80d466c783be7d", + "repositories": [{"repository": "ubi8/python-36"}], + "rpm_manifest": [{ + "rpms": [ + {"srpm_name": vulnerable_srpm_name} + ] + }] + }) + + nodejs_image = ContainerImage.create({ + "brew": {"package": "nodejs-12-container", "build": "nodejs-12-container-1-20.45678"}, + "parent_brew_build": dependency_ubi_image.nvr, + "repository": "containers/nodejs-12", + "commit": "97d57a9db975b58b43113e15d29e35de6c1a3f0b", + "repositories": [{"repository": "ubi8/nodejs-12"}], + "rpm_manifest": [{ + "rpms": [ + {"srpm_name": vulnerable_srpm_name} + ] + }] + }) + + def fake_find_parent_images_with_package(image, *args, **kwargs): + parents = { + directly_affected_ubi_image.nvr: directly_affected_ubi_image, + dependency_ubi_image.nvr: dependency_ubi_image, + } + parent = parents.get(image.get("parent_brew_build")) + if parent: + return [parent] + return [] + + find_parent_images_with_package.side_effect = fake_find_parent_images_with_package + + find_images_with_packages_from_content_set.return_value = [ + directly_affected_ubi_image, python_image, nodejs_image, + ] + + def fake_get_images_by_nvrs(nvrs, **kwargs): + if nvrs == [dependency_ubi_image.nvr]: + return [dependency_ubi_image] + elif nvrs == [directly_affected_ubi_image.nvr]: + return [directly_affected_ubi_image] + raise ValueError("Unexpected test data, {}".format(nvrs)) + + get_images_by_nvrs.side_effect = fake_get_images_by_nvrs + + lb = LightBlue(server_url=self.fake_server_url, + cert=self.fake_cert_file, + private_key=self.fake_private_key) + batches = lb.find_images_to_rebuild([vulnerable_srpm_nvr], [vulnerable_srpm_name]) + expected_batches = [ + # The dependency ubi image has a higher NVR and it should be used as + # the parent for both images. + {dependency_ubi_image.nvr}, + {python_image.nvr, nodejs_image.nvr}, + ] + for batch, expected_batch_nvrs in zip(batches, expected_batches): + batch_nvrs = set(image.nvr for image in batch) + self.assertEqual(batch_nvrs, expected_batch_nvrs) + self.assertEqual(len(batches), len(expected_batches)) + @patch("freshmaker.lightblue.ContainerImage.resolve_published") @patch("freshmaker.lightblue.LightBlue.get_images_by_nvrs") @patch("os.path.exists") From fc43b0dc84722c7b3892aab53a0f01c352bc1431 Mon Sep 17 00:00:00 2001 From: Luiz Carvalho Date: May 20 2020 13:19:48 +0000 Subject: [PATCH 2/2] Fix flake8 errors Signed-off-by: Luiz Carvalho --- diff --git a/tests/handlers/koji/test_rebuild_images_on_parent_image_build.py b/tests/handlers/koji/test_rebuild_images_on_parent_image_build.py index 9c5e146..45c3996 100644 --- a/tests/handlers/koji/test_rebuild_images_on_parent_image_build.py +++ b/tests/handlers/koji/test_rebuild_images_on_parent_image_build.py @@ -20,13 +20,10 @@ # SOFTWARE. import json -import os -import sys import unittest from unittest import mock -sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..")) # noqa from tests import get_fedmsg, helpers from freshmaker import db, events, models diff --git a/tests/test_monitor.py b/tests/test_monitor.py index 01e7596..96e1094 100644 --- a/tests/test_monitor.py +++ b/tests/test_monitor.py @@ -57,8 +57,8 @@ class TestViews(helpers.ModelsTestCase): def test_monitor_api_structure(self): resp = self.client.get('/api/1/monitor/metrics') self.assertEqual( - len([l for l in resp.get_data(as_text=True).splitlines() - if l.startswith('# TYPE')]), num_of_metrics) + len([line for line in resp.get_data(as_text=True).splitlines() + if line.startswith('# TYPE')]), num_of_metrics) class ConsumerTest(helpers.ConsumerBaseTest): @@ -127,5 +127,5 @@ def test_standalone_metrics_server(): r = requests.get('http://127.0.0.1:10040/metrics') - assert len([l for l in r.text.splitlines() - if l.startswith('# TYPE')]) == num_of_metrics + assert len([line for line in r.text.splitlines() + if line.startswith('# TYPE')]) == num_of_metrics diff --git a/tests/test_views.py b/tests/test_views.py index 1cd1ad8..5bafeee 100644 --- a/tests/test_views.py +++ b/tests/test_views.py @@ -1185,7 +1185,7 @@ class TestPatchAPI(ViewBaseTest): def test_patch_event_not_allowed(self): with self.test_request_context(user='john_smith'): - resp = self.client.patch(f'/api/1/events/1', json={'action': 'cancel'}) + resp = self.client.patch('/api/1/events/1', json={'action': 'cancel'}) assert resp.status_code == 403 assert resp.json['message'] == ( 'User john_smith does not have any of the following roles: admin, manual_rebuilder'