Add bodhi-skopeo-lite - a skopeo-workalike with manifest list support

[owtaylor]

In order to support copying multi-arch containers and Flatpaks, we need
to be able to copy manifest lists and OCI image indexes from registry to
registry. Work is underway to add such support to skopeo
(containers/image#400), but as a temporary workaround
add 'bodhi-skopeo-lite', which implements the subset of 'skopeo copy' we need,
but with manifest list/image index support. Use of this needs to be specifically
configured.

Signed-off-by: Owen W. Taylor [email protected]

[owtaylor]

jenkies test

[bowlofeggs]

I haven't had time to do a thorough review and I need to go, but here are some initial thoughts.

One thing that @puiterwijk pointed out is that we currently have planned to use skopeo delete in some upcoming work, though there is also some uncertainty as to whether we will actually use that command. I'll need to talk with him more about this before knowing whether we will need support for this.

Are you sure that skopeo isn't on the verge of getting support for these features? It seems like you've put a lot of work into this.

[bowlofeggs]

I don't think we want to exclude all uses of AssertionError from coverage in Bodhi. It is possible to exclude certain lines using # pragme: no cover, but I usually strongly prefer to test the lines rather than exclude them.

[owtaylor]

The history here is that I was raising RuntimeError() in some places that could not be hit if the code was operating as designed through the public interface. I could have added # pragma: no cover - or just removed the branches all together (but the latter would have made the code a bit confusing)

But my reasoning was that:
if !x:
raise AssertionError("X should not have been true - we have a bug")

is just a more verbose way to write "assert x". I don't think there's any other reason to raise AssertionError. So excluding it seems entirely reasonable to me. If not, I'd rather use nocover here - figuring out how to write tests is just adding maintenance burden for no reason.

[bowlofeggs]

For PEP-8, let's regroup these imports by stdlib vs. third-party.

[bowlofeggs]

Bodhi doesn't use import * and I am morally opposed to the use of import *, so we don't need this here. IMO, it's better to use leading underscores if the intention is to indicate private vs. public methods.

[owtaylor]

My goal here was to avoid writing doc strings for a lot of classes/methods internal to the file - to make pydocstyle ignore them - with the idea that this is a self-contained script - I can _ prefix them - but it sounds from what you say elsewhere you'd prefer even internal functions have dockblocks.

[bowlofeggs]

Most of the classes and methods in this file lack docblocks. Bodhi requires docblocks:

https://bodhi.fedoraproject.org/docs/developer/index.html#contribution-guidelines

[bowlofeggs]

We need to document each of the parameters and their types, and the return value and its type.

[bowlofeggs]

For docblocks, Bodhi uses this style:

"""
Do a thing.

Args:
    x (bool): blah blah.
    y (int): Neat other stuff.
Returns:
    str: A string of cool thoughts.
Raises:
    ValueError: If you forgot to tie your shoes.
"""

[bowlofeggs]

successful? Should it just be "Unexpected response"?

[owtaylor]

if it was a failed response 4xx or 5xx then the raise_for_status() would have raised - so it's a "successful" response - but by the docker v2 api, a 202 ACCEPTED should be found here, not a 200 or other successful response. Alternate wording suggestions?

[bowlofeggs]

I think the text is OK, but can you put your reply in here as a code comment for future readers?

Maybe "Unexpected successful response: %s (202 expected)".

[bowlofeggs]

successful?

[bowlofeggs]

Maybe a comment here about the 202 code too.

[bowlofeggs]

Let's mention the new dependency in docs/user/release_notes.rst.

[owtaylor]

One thing that @puiterwijk pointed out is that we currently have planned to use skopeo delete in some upcoming work, though there is also some uncertainty as to whether we will actually use that command. I'll need to talk with him more about this before knowing whether we will need support for this.

delete support would be nearly trivial to add if necessary.

Are you sure that skopeo isn't on the verge of getting support for these features? It seems like you've put a lot of work into this.

I can't say for sure - my guess is that it could easily be a few months out to get the containers/image changes done and then merged into skopeo and released. (As I was spending a day getting the test coverage from 80% to 100% I did have the thought that I could have been spending the time on trying to push the skopeo work along instead ...) At the core, my interest here is not to have bodhi changes for flatpak support blocked on future skopeo work that we don't control - I'm more interested in minimizing uncertainty, then minimizing the amount of work I need to do.

[owtaylor]

Pushed a new version that adds doc-blocks everywhere, addresses other minor comments, and replaces AssertionError usage with a comment, avoiding need to bypass coverage.

[owtaylor]

(Actually pushed the updated version this time)

[bowlofeggs]

On 06/18/2018 10:35 AM, Owen Taylor wrote: it sounds from what you say elsewhere you'd prefer even internal functions have dockblocks.

Yeah I spent quite a bit of time to achieve this in the rest of Bodhi's code. Documenting internal code is just as important as documenting public code because other people will need to understand what it does in the future.

[owtaylor]

On 06/18/2018 10:35 AM, Owen Taylor wrote:
it sounds from what you say elsewhere you'd prefer even internal
functions have dockblocks.
Yeah I spent quite a bit of time to achieve this in the rest of Bodhi's
code. Documenting internal code is just as important as documenting
public code because other people will need to understand what it does in
the future.

OK, they are all there now in any case.

One thing I realized about this PR is that while we are doing orchestrated builds in Fedora's OSBS, we have manifest list generation disabled - so we are still generating manifests not manifests lists - this applies equally to Flatpak OCI's and Docker containers. Until we add multiple architectures to the OSBS cluster or turn on manifest lists for the heck of it, this PR is not strictly necessary. So it would be possible to hold off on this for now and hope that the skopeo support lands.

[bowlofeggs]

Cool, this looks pretty good. Just a few docblock changes and we should be good to go.

Bodhi's CLIs all have man pages, so I would like this one to have one as well. I am willing to write it if you don't have time, so just let me know if you'd rather me do it (I'll just make a separate PR with it). If you do want to write it, they go in docs/user/man_pages and I think you need to reference it in docs/conf.py to get it to build.

[bowlofeggs]

"""
Return a path to a directory containing TLS client certificates to use for authentication.

Returns:
    str or None: If a path is found, it is returned. Otherwise None is returned.
"""

[bowlofeggs]

"""
Return a TLS client certificate to be used to authenticate to servers.

Args:
    cert_dir (str or None): A directory to look for certs in. None indicates to use _find_cert_dir() to find the path. Defaults to None.
Raises:
    RuntimeError: If a key or certificate cannot be found.
"""

[bowlofeggs]

This method needs a docblock, and its name isn't very descriptive so it's tough for me to write one.

It would be helpful to give it a more descriptive name.

[owtaylor]

Added a doc block and renamed it to _wrap_method.

[bowlofeggs]

s/digest/digest (str)/

[bowlofeggs]

s/media_type(str)/media_type (str)/

[bowlofeggs]

Are these both strs? Let's put types in parenthesis.

[bowlofeggs]

I am applying this patch as well:

diff --git a/bodhi/server/scripts/skopeo_lite.py b/bodhi/server/scripts/skopeo_lite.py
index 33cce898..df0db013 100644
--- a/bodhi/server/scripts/skopeo_lite.py
+++ b/bodhi/server/scripts/skopeo_lite.py
@@ -669,8 +669,8 @@ class Copier(object):
         """Initialize the Copier.

         Args:
-            src: The source endpoint.
-            dest: The destination endpoint.
+            src (str): The source endpoint.
+            dest (str): The destination endpoint.
         """
         self.src = src
         self.dest = dest

[bowlofeggs]

"""
Copy a blob with given digest and size from the source to the destination.

Args:
    digest (str): The digest of the blob to copy.
    size (int): The size of the blob to copy.
"""

[bowlofeggs]

"""
Copy the manifest referenced by info from the source to destination.

Args:
    info (ManifestInfo): References the manifest to be copied.
    toplevel (bool): If True, this should be the main manifest referenced in the repository. Defaults to False.
Raises:
    RuntimeError: If the referenced media type is not supported by this client.
"""

[bowlofeggs]

Since you and I both work at Red Hat, I think we can drop the "and others" here for now. I usually add that when there are commits in the file from people who don't work at Red Hat.

[bowlofeggs]

Neat, I hadn't seen this lib before. Looks nice! I'd been meaning to try out vcrpy, but I mostly have just used mock and that's not pretty.

[owtaylor]

jenkies test

[owtaylor]

I fixed up the doc blocks (thanks for the suggestions!) and added a man page. I don't understand why the checkout for the test is failing - it's seems to be failing to commit a rebase to origin/develop, but the latest pushed version is fully rebased... confusing.

[bowlofeggs]

Hi @owtaylor!

Yeah I am also confused at the sudden CI failures. I wrote #2535 to see if it will fix things. I suspect that maybe the machine had manually been set up with a git user/e-mail and perhaps got reprovisioned or something like that? Anyways, I think things will work once that is reviewed and merged.

[bowlofeggs]

Hi @owtaylor!

I tried to run a "faked" compose in my vagrant box today, but it looks like the tool doesn't form the correct URL to the candidate registry:

[vagrant@bodhi-dev bodhi]$ /usr/bin/bodhi-skopeo-lite copy --dest-tls-verify=false docker://candidate-registry.fedoraproject.org/f28/cockpit:0-7.f28container docker://localhost:5000/f28/cockpit:0-7.f28container                           
Traceback (most recent call last):
  File "/usr/bin/bodhi-skopeo-lite", line 11, in <module>
    load_entry_point('bodhi-server', 'console_scripts', 'bodhi-skopeo-lite')()
  File "/usr/lib/python2.7/site-packages/click/core.py", line 721, in __call__
    return self.main(*args, **kwargs)
  File "/usr/lib/python2.7/site-packages/click/core.py", line 696, in main
    rv = self.invoke(ctx)
  File "/usr/lib/python2.7/site-packages/click/core.py", line 1065, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/usr/lib/python2.7/site-packages/click/core.py", line 894, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/usr/lib/python2.7/site-packages/click/core.py", line 534, in invoke
    return callback(*args, **kwargs)
  File "/home/vagrant/bodhi/bodhi/server/scripts/skopeo_lite.py", line 771, in copy
    Copier(src.get_endpoint(), dest.get_endpoint()).copy()
  File "/home/vagrant/bodhi/bodhi/server/scripts/skopeo_lite.py", line 724, in copy
    info = self.src.get_manifest()
  File "/home/vagrant/bodhi/bodhi/server/scripts/skopeo_lite.py", line 631, in get_manifest
    return get_manifest(self.session, self.repo, self.tag)
  File "/home/vagrant/bodhi/bodhi/server/scripts/skopeo_lite.py", line 342, in get_manifest
    response = session.get(url, headers=headers)
  File "/home/vagrant/bodhi/bodhi/server/scripts/skopeo_lite.py", line 259, in get
    return self._wrap_method(self.session.get, relative_url, **kwargs)
  File "/home/vagrant/bodhi/bodhi/server/scripts/skopeo_lite.py", line 246, in _wrap_method
    res = f(self._base + relative_url, *args, **kwargs)
  File "/usr/lib/python2.7/site-packages/requests/sessions.py", line 521, in get
    return self.request('GET', url, **kwargs)
  File "/usr/lib/python2.7/site-packages/requests/sessions.py", line 494, in request
    prep = self.prepare_request(req)
  File "/usr/lib/python2.7/site-packages/requests/sessions.py", line 437, in prepare_request
    hooks=merge_hooks(request.hooks, self.hooks),
  File "/usr/lib/python2.7/site-packages/requests/models.py", line 305, in prepare
    self.prepare_url(url, params)
  File "/usr/lib/python2.7/site-packages/requests/models.py", line 382, in prepare_url
    raise InvalidURL("Invalid URL %r: No host supplied" % url)
requests.exceptions.InvalidURL: Invalid URL u'https:///v2//candidate-registry.fedoraproject.org/f28/cockpit/manifests/0-7.f28container': No host supplied

For reference, to do this I created a container release in my Vagrant box:

$ bodhi releases create --name F28C --long-name "Fedora 28 Containers" --id-prefix "FEDORA-C" --version 28 --branch f28 --dist-tag f28-container --stable-tag f28-container-updates --testing-tag f28-container-updates-testing --candidate-tag f28-container-updates-candidate --pending-stable-tag f28-container-updates-pending --pending-testing-tag f28-container-testing-pending --pending-signing-tag f28-container-updates-pending-signing --override-tag f28-container-override --state current

Then I made an update:

$ bodhi updates new --type bugfix --severity medium --notes "test update" --autokarma cockpit-0-7.f28container

Then I used bshell to set the update to be signed so it could be pushed:

$ bshell
>>> u = m.Update.query.filter_by(alias="FEDORA-C-2018-9ee0efc1df").one()
>>> u.builds[0].signed = True
>>> s().commit()

Then I applied this patch to get bodhi-push to run the compose synchronously, and to work around some other dev-environment issues:

diff --git a/bodhi/server/buildsys.py b/bodhi/server/buildsys.py
index 304c6158..b6b04f4e 100644
--- a/bodhi/server/buildsys.py
+++ b/bodhi/server/buildsys.py
@@ -368,11 +368,11 @@ class DevBuildsys(Buildsystem):
         elif 'container' in build:
             result = [
                 {'arches': 'x86_64', 'id': 15, 'locked': True,
-                 'name': 'f28C-updates-candidate'},
+                 'name': 'f28-container-updates-candidate'},
                 {'arches': 'x86_64', 'id': 16, 'locked': True,
-                 'name': 'f28C-updates-testing'},
+                 'name': 'f28-container-updates-testing'},
                 {'arches': 'x86_64', 'id': 17, 'locked': True,
-                 'name': 'f28C'},
+                 'name': 'f28-container'},
             ]
         else:
             release = build.split('.')[-1].replace('fc', 'f')
diff --git a/bodhi/server/config.py b/bodhi/server/config.py
index b74bf45d..c0c77242 100644
--- a/bodhi/server/config.py
+++ b/bodhi/server/config.py
@@ -216,7 +216,7 @@ def validate_path(value):
     Raises:
         ValueError: If os.path.exists returns False.
     """
-    if not os.path.exists(value):
+    if not value or not os.path.exists(value):
         raise ValueError('"{}" does not exist.'.format(value))
 
     return six.text_type(value)
diff --git a/bodhi/server/push.py b/bodhi/server/push.py
index 8c33d148..6e0427e4 100644
--- a/bodhi/server/push.py
+++ b/bodhi/server/push.py
@@ -158,19 +158,29 @@ def push(username, cert_prefix, **kwargs):
 
     if composes:
         click.echo('\nSending masher.start fedmsg')
+        from bodhi.server.consumers import masher
+        import mock
+        import logging
+        log = logging.getLogger(__name__)
+        log.setLevel(logging.DEBUG)
+        log.addHandler(logging.StreamHandler())
+        with mock.patch('fedmsg.consumers.FedmsgConsumer.__init__'):
+            m = masher.Masher(mock.MagicMock(), db_factory, '/home/vagrant/test')
+        m.log = log
+        m.work({'body': {'msg': dict(api_version=2, composes=composes, resume=resume, agent=username)}})
         # Because we're a script, we want to send to the fedmsg-relay,
         # that's why we say active=True
-        bodhi.server.notifications.init(active=True, cert_prefix=cert_prefix)
-        bodhi.server.notifications.publish(
-            topic='masher.start',
-            msg=dict(
-                api_version=2,
-                composes=composes,
-                resume=resume,
-                agent=username,
-            ),
-            force=True,
-        )
+        #bodhi.server.notifications.init(active=True, cert_prefix=cert_prefix)
+        #bodhi.server.notifications.publish(
+        #    topic='masher.start',
+        #    msg=dict(
+        #        api_version=2,
+        #        composes=composes,
+        #        resume=resume,
+        #        agent=username,
+        #    ),
+        #    force=True,
+        #)
 
 
 def _filter_releases(session, query, releases=None):

Then I ran the push like this:

[vagrant@bodhi-dev bodhi]$ bodhi-push --username bowlofeggs --builds cockpit-0-7.f28container

[owtaylor]

GIGO - my manual testing, the tests in the code, and the code itself all think that references should be docker:hostname/repo/image:tag - but yes, skopeo wants docker://hostname/repo/image:tag. I'll fix everything up.

[owtaylor]

OK, new version is corrected to expect docker://<image-reference>

[bowlofeggs]

Looks like there are a few flake8 errors:

rawhide	./bodhi/server/scripts/skopeo_lite.py:744:15: W504 line break after binary operator
rawhide	              help='require HTTPS and verify certificates when talking to the ' +
rawhide	              ^
rawhide	./bodhi/server/scripts/skopeo_lite.py:747:15: W504 line break after binary operator
rawhide	              help='use certificates at PATH (*.crt, *.cert, *.key) to connect to the ' +
rawhide	              ^
rawhide	./bodhi/server/scripts/skopeo_lite.py:752:15: W504 line break after binary operator
rawhide	              help='require HTTPS and verify certificates when talking to the ' +
rawhide	              ^
rawhide	./bodhi/server/scripts/skopeo_lite.py:755:15: W504 line break after binary operator
rawhide	              help='use certificates at PATH (*.crt, *.cert, *.key) to connect to the ' +
rawhide	              ^
rawhide	./bodhi/tests/server/scripts/test_skopeo_lite.py:195:10: W605 invalid escape sequence '\s'
rawhide	        accepts = re.split('\s*,\s*', req.headers['Accept'])
rawhide	         ^
rawhide	./bodhi/tests/server/scripts/test_skopeo_lite.py:195:14: W605 invalid escape sequence '\s'
rawhide	        accepts = re.split('\s*,\s*', req.headers['Accept'])
rawhide	             ^

I'll fix these up for you and re-test. Thanks!

[bowlofeggs]

GitHub is giving me permission denied when I try to push a rebased version onto your branch, so I won't be able to fix this myself.

[bowlofeggs]

…or it would help if I were looking at the correct branch of yours. d'oh. (I'd been trying to work on your now-merged flatpak branch like an idiot. I'd say I hadn't had my morning coffee, but I did have one cup, so my excuse is that I haven't have my second cup today.)

Anyways, I think I can fix those things up for you.

[bowlofeggs]

I'm going to apply this patch on top to appease the flake8 gods:

diff --git a/bodhi/server/scripts/skopeo_lite.py b/bodhi/server/scripts/skopeo_lite.py
index 6f33599a..33cce898 100644
--- a/bodhi/server/scripts/skopeo_lite.py
+++ b/bodhi/server/scripts/skopeo_lite.py
@@ -741,19 +741,19 @@ class Copier(object):
 @click.option('--src-creds', '--screds', metavar='USERNAME[:PASSWORD]',
               help='Use USERNAME[:PASSWORD] for accessing the source registry')
 @click.option('--src-tls-verify', type=bool, default=True,
-              help='require HTTPS and verify certificates when talking to the ' +
-              'container source registry (defaults to true)')
+              help=('require HTTPS and verify certificates when talking to the '
+                    'container source registry (defaults to true)'))
 @click.option('--src-cert-dir', metavar='PATH',
-              help='use certificates at PATH (*.crt, *.cert, *.key) to connect to the ' +
-              'source registry')
+              help=('use certificates at PATH (*.crt, *.cert, *.key) to connect to the '
+                    'source registry'))
 @click.option('--dest-creds', '--dcreds', metavar='USERNAME[:PASSWORD]',
               help='Use USERNAME[:PASSWORD] for accessing the destination registry')
 @click.option('--dest-tls-verify', type=bool, default=True,
-              help='require HTTPS and verify certificates when talking to the ' +
-              'container destination registry (defaults to true)')
+              help=('require HTTPS and verify certificates when talking to the '
+                    'container destination registry (defaults to true)'))
 @click.option('--dest-cert-dir', metavar='PATH',
-              help='use certificates at PATH (*.crt, *.cert, *.key) to connect to the ' +
-              'destination registry')
+              help=('use certificates at PATH (*.crt, *.cert, *.key) to connect to the '
+                    'destination registry'))
 @click.argument('src', metavar='SOURCE-IMAGE-NAME')
 @click.argument('dest', metavar='DEST-IMAGE-NAME')
 def copy(src, dest, screds, src_tls_verify, src_cert_dir, dcreds, dest_tls_verify, dest_cert_dir):
diff --git a/bodhi/tests/server/scripts/test_skopeo_lite.py b/bodhi/tests/server/scripts/test_skopeo_lite.py
index ef422b4d..d382cbe5 100644
--- a/bodhi/tests/server/scripts/test_skopeo_lite.py
+++ b/bodhi/tests/server/scripts/test_skopeo_lite.py
@@ -192,7 +192,7 @@ class MockRegistry(object):
             else:
                 content_type = MEDIA_TYPE_OCI

-        accepts = re.split('\s*,\s*', req.headers['Accept'])
+        accepts = re.split(r'\s*,\s*', req.headers['Accept'])
         assert content_type in accepts

         if 'bad_index_content_type' in self.flags:

[bowlofeggs]

Why the pass?

[bowlofeggs]

I'm also applying this patch:

diff --git a/bodhi/tests/server/scripts/test_skopeo_lite.py b/bodhi/tests/server/scripts/test_skopeo_lite.py
index d382cbe5..f3744890 100644
--- a/bodhi/tests/server/scripts/test_skopeo_lite.py
+++ b/bodhi/tests/server/scripts/test_skopeo_lite.py
@@ -667,7 +667,6 @@ def test_skopeo_copy_system_cert():
      'Registry specification should be docker://REGISTRY/PATH'),
 ])
 def test_skopeo_copy_cli_errors(src, dest, error):
-    pass
     """
     Test errors triggered from bad command line arguments
     """
@@ -705,7 +704,6 @@ def test_skopeo_copy_cli_errors(src, dest, error):
      None),
 ])
 def test_skopeo_copy_protocol(src, dest, flags1, flags2, error):
-    pass
     """
     Tests various error and other code paths related to variations in server responses; the
     flags argument to the MockRegistry constructor is used to modify server behavior.

[bowlofeggs]

Here's a patch I'm adding to get the Python 2 tests to pass in Vagrant:

diff --git a/devel/ansible/roles/dev/tasks/main.yml b/devel/ansible/roles/dev/tasks/main.yml
index 08171fc0..7da123e9 100644
--- a/devel/ansible/roles/dev/tasks/main.yml
+++ b/devel/ansible/roles/dev/tasks/main.yml
@@ -57,6 +57,7 @@
       - python2-pyramid-mako
       - python2-pyramid-tm
       - python2-pytest-cov
+      - python2-responses
       - python2-rpdb
       - python2-simplemediawiki
       - python2-sphinx

[bowlofeggs]

Alright, the patch looks good, tests pass in Vagrant and CI, and I hand tested that it can compose container updates from Fedora's candidate registry into a locally running registry. Looks good, and thanks for the patch!

[bowlofeggs]

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

[bowlofeggs]

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

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