Make the repo sanity checks much more strict #2373

puiterwijk · 2018-05-11T13:20:26Z

This makes us use librepo/hawkey to verify that a repo can be correctly read and used
by DNF, which should prevent us from accepting a repo if DNF will then crash on using
it.

Note: per the hawkey docs, it's obsoleted, and one is supposed to use libhif,
but the libhif (redirected to libdnf) repo says it is being "reworked and is unstable".
From my tests, it seems that hawkey calls libdnf underwater, so I think that this
is reasonable to do for now.

Signed-off-by: Patrick Uiterwijk [email protected]

bowlofeggs

We should document the new dependencies on hawkey and libcomps in the release notes and requirements.txt.

bowlofeggs · 2018-06-01T19:35:27Z

bodhi/server/util.py

 import libravatar
 import librepo
 import markdown
 import requests
 import rpm
+import shutil


For PEP-8, this import should be included with the first group of imports, since it's part of the Python stdlib.

bowlofeggs · 2018-06-01T19:35:57Z

bodhi/server/util.py

+    """
+    Inject a file into the repodata with the help of createrepo_c.
+
+    Args:


Let's also document the comp_type argument here.

bowlofeggs · 2018-06-01T19:38:58Z

bodhi/server/util.py

    """
    Generate package metadata for a given directory.

    If the metadata doesn't exist, then create it.

    Args:
        path (basestring): The directory to generate metadata for.
-    """
+        updateinfo (basestring or None or False): The updateinfo to insert instead of example.
+            No updateinfo is inserted if False is passed.


It would be good to document the subtle difference between None and False here. Also, technically False isn't a type - the ()'s should document types that are accepted, so we should probably put bool there instead of False. It would also be good to document that passing True causes undefined behavior.

bowlofeggs · 2018-06-01T19:39:47Z

bodhi/server/util.py

+        comps (basestring or None): The comps to insert instead of example.
+    """
+    compsfile = '''<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE comps PUBLIC "-//Red Hat, Inc.//DTD Comps info//EN" "comps.dtd">


I'm betting the answer is no... but is there a URL to this dtd? Do we have a copy of it anywhere to reference?

Yes, but not an official one that the libraries will probably accept: https://github.com/rpm-software-management/libcomps/blob/master/libcomps/tests/comps.dtd

bowlofeggs · 2018-06-01T19:40:44Z

bodhi/server/util.py

+    if not comps:
+        comps = os.path.join(path, 'comps.xml')
+        with open(comps, 'w') as f:
+            f.write(compsfile)


Is this for unit tests? If so, I would prefer we use mock to insert it rather than include this in the production code.

The entire function mkmetadatadir is only ever used in the tests.....

Ah, in that case can we move it into the tests folder somewhere? Perhaps bodhi/tests/server/test_util.py?

We talked about this on IRC - for now let's just leave it here and I'll move it in another PR separately.

bowlofeggs · 2018-06-01T19:43:38Z

bodhi/server/util.py

@@ -264,7 +335,7 @@ def sanity_check_repodata(myurl):
    Args:
        myurl (basestring): A path to a repodata directory.
    Raises:
-        bodhi.server.exceptions.RepodataException: If the repodata is not valid or does not exist.
+        Exception: If the repodata is not valid or does not exist.


It looks like the code still raises RepodataException - should we change this doc back?

Well, I've seen all kinds of methods of hawkey and librepo fail due to... weird reasons unrelated to the actual repodata in the other functions.

bowlofeggs · 2018-06-04T15:08:53Z

devel/ci/rawhide-packages

-    python3-pyramid-fas-openid
+    python3-pyramid-fas-openid \
+    python2-hawkey \
+    python2-libcomps


I have a feeling the Python 3 versions of these packages are getting pulled in via requirements.txt instead of via RPM packages. We should probably add those to devel/bodhi/devel/ci/rpm-packages.

They are in the base system as they're used by DNF...

bowlofeggs · 2018-06-05T17:00:06Z

requirements.txt

@@ -24,6 +24,8 @@ pyramid_mako
 pyramid_tm
 python-bugzilla
 python-fedora
+python-hawkey


It looks like hawkey is not available under PyPI (or at least, not under this name). This is OK, as there are other Bodhi dependencies that are also not on PyPI (like Koji), but it does cause the pip container to fail to build:

pip Collecting python-hawkey (from -r /bodhi/requirements.txt (line 27)) pip �[91m Could not find a version that satisfies the requirement python-hawkey (from -r /bodhi/requirements.txt (line 27)) (from versions: ) pip �[0m�[91mNo matching distribution found for python-hawkey (from -r /bodhi/requirements.txt (line 27))

To fix this, you can add it to devel/ci/pip-packages so that it gets installed via RPM for that container.

bowlofeggs · 2018-06-11T13:19:30Z

requirements.txt

@@ -24,6 +24,8 @@ pyramid_mako
 pyramid_tm
 python-bugzilla
 python-fedora
+python-hawkey


There is something called hawkey, but it has no description so I have no idea if it's the same one:

https://pypi.org/project/hawkey/

Anyways, CI is still failing due to hawkey:

pip Collecting python-hawkey (from -r /bodhi/requirements.txt (line 27)) pip �[91m Could not find a version that satisfies the requirement python-hawkey (from -r /bodhi/requirements.txt (line 27)) (from versions: ) pip �[0m�[91mNo matching distribution found for python-hawkey (from -r /bodhi/requirements.txt (line 27))

I'm going to just drop hawkey from requirements.txt and document it in the release notes. Then we can get this PR moving along.

bowlofeggs · 2018-07-23T20:24:25Z

I might be able to fix this next week if you don't have time to get to it.

bowlofeggs · 2018-08-20T19:30:14Z

Hmm, the rawhide container is busted due to GPG sigs :/

bowlofeggs · 2018-08-20T20:32:32Z

bodhi/server/util.py

+        hk_repo.repomd_fn = repo_info['repomd']
+        hk_repo.updateinfo_fn = repo_info['updateinfo']
+    except KeyError:
+        raise RepodataException('Required part not in repomd.xml')


I manually ran the CI tests on my laptop, and they failed due to missing coverage on this exception handler.

I have a test to address this now. I'll push it up shortly:

diff --git a/bodhi/tests/server/test_util.py b/bodhi/tests/server/test_util.py index 206ec88f..d916816b 100644 --- a/bodhi/tests/server/test_util.py +++ b/bodhi/tests/server/test_util.py @@ -16,6 +16,7 @@ # You should have received a copy of the GNU General Public License # along with this program; if not, write to the Free Software # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +from xml.etree import ElementTree import json import os import shutil @@ -577,6 +578,24 @@ class TestSanityCheckRepodata(unittest.TestCase): self.assertEqual(str(exc.exception), 'Comps file empty') + def test_repomd_missing_updateinfo(self): + """If the updateinfo data tag is missing in repomd.xml, an Exception should be raised.""" + util.mkmetadatadir(self.tempdir) + repomd_path = os.path.join(self.tempdir, 'repodata', 'repomd.xml') + repomd = ElementTree.parse(repomd_path) + ElementTree.register_namespace('', 'http://linux.duke.edu/metadata/repo') + root = repomd.getroot() + # Find the <data type="updateinfo"> tag and delete it + for data in root.findall('{http://linux.duke.edu/metadata/repo}data'): + if data.attrib['type'] == 'updateinfo': + root.remove(data) + repomd.write(repomd_path, encoding='UTF-8', xml_declaration=True) + + with self.assertRaises(util.RepodataException) as exc: + util.sanity_check_repodata(self.tempdir) + + self.assertEqual(str(exc.exception), 'Required part not in repomd.xml') + class TestType2Icon(unittest.TestCase): """Test the type2icon() function."""

bowlofeggs · 2018-08-20T20:35:11Z

Also, in order to get the tests to run, I applied this patch:

diff --git a/devel/ci/Dockerfile-header b/devel/ci/Dockerfile-header
index 25417d93..9e2f2151 100644
--- a/devel/ci/Dockerfile-header
+++ b/devel/ci/Dockerfile-header
@@ -14,9 +14,11 @@ RUN dnf install --disablerepo rawhide-modular -y \
     liberation-mono-fonts \
     packagedb-cli \
     python2-createrepo_c \
+    python2-hawkey \
     python2-jinja2 \
     python2-koji \
     python2-librepo \
     python2-yaml \
     python3-createrepo_c \
+    python3-hawkey \
     python3-yaml \
diff --git a/devel/ci/f27-packages b/devel/ci/f27-packages
index d95e404f..8f35cf1d 100644
--- a/devel/ci/f27-packages
+++ b/devel/ci/f27-packages
@@ -7,7 +7,6 @@
     python-pytest \
     python-simplemediawiki \
     python-webtest \
-    python2-hawkey \
     python2-libcomps
 
 RUN pip-2 install cornice
diff --git a/devel/ci/pip-packages b/devel/ci/pip-packages
index ef3cf715..0086abb0 100644
--- a/devel/ci/pip-packages
+++ b/devel/ci/pip-packages
@@ -6,9 +6,7 @@
     python3-devel \
     python3-simplemediawiki \
     redhat-rpm-config \
-    python2-hawkey \
     python2-libcomps \
-    python3-hawkey \
     python3-libcomps
 
 COPY requirements.txt /bodhi/requirements.txt
diff --git a/devel/ci/rawhide-packages b/devel/ci/rawhide-packages
index 7f90a0e4..5a1a13d9 100644
--- a/devel/ci/rawhide-packages
+++ b/devel/ci/rawhide-packages
@@ -11,5 +11,4 @@
     python3-cornice-sphinx \
     python2-webtest \
     python3-pyramid-fas-openid \
-    python2-hawkey \
     python2-libcomps
diff --git a/devel/ci/rpm-packages b/devel/ci/rpm-packages
index a1e39fab..dc1abaa0 100644
--- a/devel/ci/rpm-packages
+++ b/devel/ci/rpm-packages
@@ -26,7 +26,6 @@
     python3-fedora \
     python3-feedgen \
     python3-flake8 \
-    python3-hawkey \
     python3-kitchen \
     python3-libcomps \
     python3-markdown \
diff --git a/requirements.txt b/requirements.txt
index 8619c603..3f78b9c3 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -8,7 +8,6 @@ cryptography
 dogpile.cache
 fedmsg[consumers]
 feedgen
-hawkey
 iniparse
 jinja2
 kitchen
@@ -25,7 +24,6 @@ pyramid_mako
 pyramid_tm
 python-bugzilla
 python-fedora
-python-libcomps
 simplemediawiki
 six
 sqlalchemy

I will push that up to your branch now.

This makes us use librepo/hawkey to verify that a repo can be correctly read and used by DNF, which should prevent us from accepting a repo if DNF will then crash on using it. Note: per the hawkey docs, it's obsoleted, and one is supposed to use libhif, but the libhif (redirected to libdnf) repo says it is being "reworked and is unstable". From my tests, it seems that hawkey calls libdnf underwater, so I think that this is reasonable to do for now. Signed-off-by: Patrick Uiterwijk <[email protected]>

bowlofeggs · 2018-08-28T17:34:14Z

This patch is planned to be included in the upcoming 3.10.0 release: #2556.

bowlofeggs · 2018-08-31T16:38:58Z

This patch has been deployed to Fedora's staging Bodhi instance:

https://bodhi.stg.fedoraproject.org/

puiterwijk added Composer Issues related to the composer reliability Issues pertaining to Bodhi's reliability labels May 11, 2018

puiterwijk requested a review from bowlofeggs May 11, 2018 13:20

puiterwijk force-pushed the paranoia-over-9000 branch from 32df816 to a580b7b Compare May 11, 2018 13:56

fedora-infra deleted a comment from centos-ci May 11, 2018

puiterwijk force-pushed the paranoia-over-9000 branch from a580b7b to 6415dc2 Compare May 11, 2018 17:09

fedora-infra deleted a comment from centos-ci May 11, 2018

puiterwijk force-pushed the paranoia-over-9000 branch from 6415dc2 to cc0e0d1 Compare May 11, 2018 17:37

fedora-infra deleted a comment from centos-ci May 11, 2018

puiterwijk force-pushed the paranoia-over-9000 branch from cc0e0d1 to 552c938 Compare May 17, 2018 13:14

fedora-infra deleted a comment from centos-ci May 17, 2018

bowlofeggs suggested changes Jun 1, 2018

View reviewed changes

puiterwijk force-pushed the paranoia-over-9000 branch 2 times, most recently from 686e1ca to ef83dc2 Compare June 1, 2018 20:01

fedora-infra deleted a comment from centos-ci Jun 1, 2018

bowlofeggs suggested changes Jun 4, 2018

View reviewed changes

fedora-infra deleted a comment from centos-ci Jun 4, 2018

puiterwijk force-pushed the paranoia-over-9000 branch from ef83dc2 to 377d1dc Compare June 4, 2018 20:39

fedora-infra deleted a comment from centos-ci Jun 5, 2018

bowlofeggs suggested changes Jun 5, 2018

View reviewed changes

puiterwijk force-pushed the paranoia-over-9000 branch from 377d1dc to f02d13f Compare June 7, 2018 13:59

fedora-infra deleted a comment from centos-ci Jun 11, 2018

bowlofeggs suggested changes Jun 11, 2018

View reviewed changes

bowlofeggs force-pushed the paranoia-over-9000 branch from f02d13f to 585f130 Compare August 20, 2018 19:19

puiterwijk requested a review from a team as a code owner August 20, 2018 19:19

bowlofeggs approved these changes Aug 20, 2018

View reviewed changes

fedora-infra deleted a comment from centos-ci Aug 20, 2018

bowlofeggs suggested changes Aug 20, 2018

View reviewed changes

bowlofeggs force-pushed the paranoia-over-9000 branch from 585f130 to 41aa2ed Compare August 20, 2018 20:35

fedora-infra deleted a comment from centos-ci Aug 23, 2018

bowlofeggs force-pushed the paranoia-over-9000 branch from 41aa2ed to c3f09e5 Compare August 23, 2018 22:59

bowlofeggs approved these changes Aug 23, 2018

View reviewed changes

mergify bot merged commit 7dda554 into fedora-infra:develop Aug 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the repo sanity checks much more strict #2373

Make the repo sanity checks much more strict #2373

puiterwijk commented May 11, 2018

bowlofeggs left a comment

bowlofeggs Jun 1, 2018

bowlofeggs Jun 1, 2018

bowlofeggs Jun 1, 2018

bowlofeggs Jun 1, 2018

puiterwijk Jun 1, 2018

bowlofeggs Jun 1, 2018

puiterwijk Jun 1, 2018

bowlofeggs Jun 1, 2018

bowlofeggs Jun 1, 2018

bowlofeggs Jun 1, 2018

puiterwijk Jun 1, 2018

bowlofeggs Jun 4, 2018

puiterwijk Jun 4, 2018

bowlofeggs Jun 4, 2018

bowlofeggs Jun 5, 2018

bowlofeggs Jun 11, 2018

bowlofeggs Aug 20, 2018

bowlofeggs commented Jul 23, 2018

bowlofeggs commented Aug 20, 2018 •

edited

Loading

bowlofeggs Aug 20, 2018

bowlofeggs Aug 23, 2018

bowlofeggs commented Aug 20, 2018

bowlofeggs commented Aug 28, 2018

bowlofeggs commented Aug 31, 2018

Make the repo sanity checks much more strict #2373

Make the repo sanity checks much more strict #2373

Conversation

puiterwijk commented May 11, 2018

bowlofeggs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bowlofeggs commented Jul 23, 2018

bowlofeggs commented Aug 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bowlofeggs commented Aug 20, 2018

bowlofeggs commented Aug 28, 2018

bowlofeggs commented Aug 31, 2018

bowlofeggs commented Aug 20, 2018 •

edited

Loading