Merge ~pappacena/launchpad:refactor-oci-push-auth into launchpad:master

[Thiago F. Pappacena]

Refactoring OCIRegistryClient to prepare for DockerHub.

Dockerhub authentication seems to need token authorization, and the way OCIRegistryClient is structured today (with HTTP Basic auth) makes it a bit difficult to keep the token. This refactoring creates a couple of differente "HTTP clients": one for the registry protocol we have today, and another for DocherHub (to be implemented in another MP).

Summary of reorganization:

- Creation of RegistryHTTPClient helper class, that holds authentication information and does the HTTP requests to registry. That's where DockerHub auth will be implemented in the future.

- OCIRegistryClient.upload delegates to RegistryHTTPClient the authentication info, and stops dealing with tuple of (username, password)

- Changed the tests to use instances of RegistryHTTPClient instead of tuple of auth information

- Changed the tests to use RegistryHTTPClient's URL builder, instead of hardcoding it (DockerHub will need to use a different URL schema, so this was also delegated to RegistryHTTPClient class)

- Not really necessary for the tests we have, but I've changed the fake layers files to be a bit more realistic (it's helping me to debug things on dockerhub, and I guess it doesn't hurt to make it a bit closer to reality here).

In the end, a lot of lines got changes, but nothing should have changed in the behavior.

a0addb5... by Thiago F. Pappacena on 2020-05-19

Fixing naming conventions

[Colin Watson (cjwatson)]

review: Approve

[Tom Wardill (twom)]

I like the approach of wrapping urlfetch to handle the different authentication methods, this seems pretty extensible for the future.

However, I think there's a better way to determine which client and auth method to use rather than just hardcoding the registry url.

Doing `docker login` with the client makes a request to <registry url>/v2/ (See: https://docs.docker.com/registry/spec/api/#api-version-check). This is a call that requires authentication, so making it without the auth will fail. However, it does include the required authentication method in the response headers: `Www-Authenticate: Bearer realm="https://auth.docker.io/token",service="registry.docker.io"`.
A registry that uses Basic (like ROCKS) looks like: `Www-Authenticate: Basic realm="docker-registry"`

Given this, I think we can rename the `DockerHubHTTPClient` to something like `BearerTokenRegistryClient` or similar.

The flow will then look similar to how the cli client does:
1. Make request to /v2/ (`docker login`)
2. Work out which client to use from the response header
(3. Optionally get the bearer token if we're doing that method)
4. Make further requests

The spec does state that auth requirements can differ per resource requested, but I think we can miss that for now (It doesn't seem to apply to either ROCKS or DockerHub as yet)

review: Needs Fixing

[Tom Wardill (twom)]

Having looked at the DockerHubClient MP further, I note you've already got the header parsing, so I think it's probably as simple as swapping the `getInstance` method to something that uses those methods.

[Thiago F. Pappacena (pappacena)]

Thanks for the review, twom and cjwatson.

I'll make the requested changes (dockerhub url format, new class renaming and check the login flow at getInstance) in the next MP, keeping only the refactoring in this one.

[Tom Wardill (twom)]

LGTM, and sorting the auth things in the next MP makes sense.

review: Approve

diff --git a/lib/lp/oci/interfaces/ocipushrule.py b/lib/lp/oci/interfaces/ocipushrule.py
index 26f636d..5b43f25 100644
--- a/lib/lp/oci/interfaces/ocipushrule.py
+++ b/lib/lp/oci/interfaces/ocipushrule.py
@@ -19,8 +19,8 @@ from lazr.restful.declarations import (
     export_write_operation,
     exported,
     mutator_for,
-    operation_parameters,
     operation_for_version,
+    operation_parameters,
     )
 from lazr.restful.fields import Reference
 from six.moves import http_client
diff --git a/lib/lp/oci/model/ociregistryclient.py b/lib/lp/oci/model/ociregistryclient.py
index 2bb82ab..c16e1a0 100644
--- a/lib/lp/oci/model/ociregistryclient.py
+++ b/lib/lp/oci/model/ociregistryclient.py
@@ -18,9 +18,10 @@ import logging
 import tarfile
 
 from requests.exceptions import (
-    HTTPError,
     ConnectionError,
+    HTTPError,
     )
+from six.moves.urllib.parse import urlsplit
 from tenacity import (
     before_log,
     retry,
@@ -61,23 +62,20 @@ class OCIRegistryClient:
         before=before_log(log, logging.INFO),
         retry=retry_if_exception_type(ConnectionError),
         stop=stop_after_attempt(5))
-    def _upload(cls, digest, push_rule, name, fileobj, auth):
+    def _upload(cls, digest, push_rule, fileobj, http_client):
         """Upload a blob to the registry, using a given digest.
 
         :param digest: The digest to store the file under.
         :param push_rule: `OCIPushRule` to use for the URL and credentials.
-        :param name: Name of the image the blob is part of.
         :param fileobj: An object that looks like a buffer.
 
         :raises BlobUploadFailed: if the registry does not accept the blob.
         """
         # Check if it already exists
         try:
-            head_response = urlfetch(
-                "{}/v2/{}/blobs/{}".format(
-                    push_rule.registry_credentials.url, name, digest),
-                method="HEAD",
-                auth=auth)
+            head_response = http_client.requestPath(
+                "/blobs/{}".format(digest),
+                method="HEAD")
             if head_response.status_code == 200:
                 log.info("{} already found".format(digest))
                 return
@@ -86,27 +84,25 @@ class OCIRegistryClient:
             if http_error.response.status_code != 404:
                 raise http_error
 
-        post_response = urlfetch(
-            "{}/v2/{}/blobs/uploads/".format(
-                push_rule.registry_credentials.url, name),
-            method="POST", auth=auth)
+        post_response = http_client.requestPath(
+            "/blobs/uploads/", method="POST")
 
         post_location = post_response.headers["Location"]
         query_parsed = {"digest": digest}
 
-        put_response = urlfetch(
+        put_response = http_client.request(
             post_location,
             params=query_parsed,
             data=fileobj,
-            method="PUT",
-            auth=auth)
+            method="PUT")
 
         if put_response.status_code != 201:
-            raise BlobUploadFailed(
-                "Upload of {} for {} failed".format(digest, name))
+            msg = "Upload of {} for {} failed".format(
+                digest, push_rule.image_name)
+            raise BlobUploadFailed(msg)
 
     @classmethod
-    def _upload_layer(cls, digest, push_rule, name, lfa, auth):
+    def _upload_layer(cls, digest, push_rule, lfa, http_client):
         """Upload a layer blob to the registry.
 
         Uses _upload, but opens the LFA and extracts the necessary files
@@ -114,7 +110,6 @@ class OCIRegistryClient:
 
         :param digest: The digest to store the file under.
         :param push_rule: `OCIPushRule` to use for the URL and credentials.
-        :param name: Name of the image the blob is part of.
         :param lfa: The `LibraryFileAlias` for the layer.
         """
         lfa.open()
@@ -124,7 +119,7 @@ class OCIRegistryClient:
                 if tarinfo.name != 'layer.tar':
                     continue
                 fileobj = un_zipped.extractfile(tarinfo)
-                cls._upload(digest, push_rule, name, fileobj, auth)
+                cls._upload(digest, push_rule, fileobj, http_client)
         finally:
             lfa.close()
 
@@ -223,15 +218,10 @@ class OCIRegistryClient:
         preloaded_data = cls._preloadFiles(build, manifest, digests)
 
         for push_rule in build.recipe.push_rules:
-            # Work out the auth details for the registry
-            auth = push_rule.registry_credentials.getCredentials()
-            if auth.get('username'):
-                auth_tuple = (auth['username'], auth.get('password'))
-            else:
-                auth_tuple = None
+            http_client = RegistryHTTPClient.getInstance(push_rule)
+
             for section in manifest:
                 # Work out names and tags
-                image_name = push_rule.image_name
                 tag = cls._calculateTag(build, push_rule)
                 file_data = preloaded_data[section["Config"]]
                 config = file_data["config_file"]
@@ -240,9 +230,8 @@ class OCIRegistryClient:
                     cls._upload_layer(
                         diff_id,
                         push_rule,
-                        image_name,
                         file_data[diff_id],
-                        auth_tuple)
+                        http_client)
                 # The config file is required in different forms, so we can
                 # calculate the sha, work these out and upload
                 config_json = json.dumps(config).encode("UTF-8")
@@ -250,9 +239,8 @@ class OCIRegistryClient:
                 cls._upload(
                     "sha256:{}".format(config_sha),
                     push_rule,
-                    image_name,
                     BytesIO(config_json),
-                    auth_tuple)
+                    http_client)
 
                 # Build the registry manifest from the image manifest
                 # and associated configs
@@ -261,20 +249,82 @@ class OCIRegistryClient:
                     preloaded_data[section["Config"]])
 
                 # Upload the registry manifest
-                manifest_response = urlfetch(
-                    "{}/v2/{}/manifests/{}".format(
-                        push_rule.registry_credentials.url,
-                        image_name,
-                        tag),
+                manifest_response = http_client.requestPath(
+                    "/manifests/{}".format(tag),
                     json=registry_manifest,
                     headers={
                         "Content-Type":
                             "application/"
                             "vnd.docker.distribution.manifest.v2+json"
                         },
-                    method="PUT",
-                    auth=auth_tuple)
+                    method="PUT")
                 if manifest_response.status_code != 201:
                     raise ManifestUploadFailed(
                         "Failed to upload manifest for {} in {}".format(
                             build.recipe.name, build.id))
+
+
+class RegistryHTTPClient:
+    def __init__(self, push_rule):
+        self.push_rule = push_rule
+
+    @property
+    def credentials(self):
+        """Returns a tuple of (username, password)."""
+        auth = self.push_rule.registry_credentials.getCredentials()
+        if auth.get('username'):
+            return auth['username'], auth.get('password')
+        return None, None
+
+    @property
+    def api_url(self):
+        """Returns the base API URL for this registry."""
+        push_rule = self.push_rule
+        return "{}/v2/{}".format(push_rule.registry_url, push_rule.image_name)
+
+    def request(self, url, *args, **request_kwargs):
+        username, password = self.credentials
+        if username is not None:
+            request_kwargs.setdefault("auth", (username, password))
+        return urlfetch(url, **request_kwargs)
+
+    def requestPath(self, path, *args, **request_kwargs):
+        """Shortcut to do a request to {self.api_url}/{path}."""
+        url = "{}{}".format(self.api_url, path)
+        return self.request(url, *args, **request_kwargs)
+
+    @classmethod
+    def getInstance(cls, push_rule):
+        """Returns an instance of RegistryHTTPClient adapted to the
+        given push rule."""
+        split = urlsplit(push_rule.registry_url)
+        if split.netloc.endswith('registry.hub.docker.com'):
+            return DockerHubHTTPClient(push_rule)
+        return RegistryHTTPClient(push_rule)
+
+
+class DockerHubHTTPClient(RegistryHTTPClient):
+    def __init__(self, push_rule):
+        super(DockerHubHTTPClient, self).__init__(push_rule)
+        self.auth_token = None
+
+    @property
+    def api_url(self):
+        """Get the base API URL for Dockerhub projects."""
+        push_rule = self.push_rule
+        return "{}/v2/{}/{}".format(
+            push_rule.registry_url, push_rule.username, push_rule.image_name)
+
+    def authenticate(self, last_failure):
+        """Tries to authenticate, considering the last HTTP 401 error."""
+        raise NotImplementedError("DockerHub auth is not implemented yet.")
+
+    def request(self, url, auth_retry=True, *args, **request_kwargs):
+        try:
+            return super(DockerHubHTTPClient, self).request(
+                url, *args, **request_kwargs)
+        except HTTPError as e:
+            if e.response.status_code == 401:
+                self.authenticate(e.response)
+                return self.request(url, auth_retry=False, **request_kwargs)
+            raise
diff --git a/lib/lp/oci/tests/test_ociregistryclient.py b/lib/lp/oci/tests/test_ociregistryclient.py
index 2a01b33..6ba25c0 100644
--- a/lib/lp/oci/tests/test_ociregistryclient.py
+++ b/lib/lp/oci/tests/test_ociregistryclient.py
@@ -8,6 +8,8 @@ from __future__ import absolute_import, print_function, unicode_literals
 __metaclass__ = type
 
 import json
+import os
+import tarfile
 
 from fixtures import MockPatch
 from requests.exceptions import (
@@ -15,6 +17,7 @@ from requests.exceptions import (
     HTTPError,
     )
 import responses
+from six import StringIO
 from tenacity import RetryError
 from testtools.matchers import (
     Equals,
@@ -23,7 +26,10 @@ from testtools.matchers import (
     )
 import transaction
 
-from lp.oci.model.ociregistryclient import OCIRegistryClient
+from lp.oci.model.ociregistryclient import (
+    OCIRegistryClient,
+    RegistryHTTPClient,
+    )
 from lp.oci.tests.helpers import OCIConfigHelperMixin
 from lp.testing import TestCaseWithFactory
 from lp.testing.layers import LaunchpadZopelessLayer
@@ -74,19 +80,29 @@ class TestOCIRegistryClient(OCIConfigHelperMixin, TestCaseWithFactory):
             filename='config_file_1.json'
         )
 
-        # make layer files
-        self.layer_1_file = self.factory.makeOCIFile(
-            build=self.build,
-            content="digest_1",
-            filename="digest_1_filename",
-            layer_file_digest="digest_1"
-        )
-        self.layer_2_file = self.factory.makeOCIFile(
-            build=self.build,
-            content="digest_2",
-            filename="digest_2_filename",
-            layer_file_digest="digest_2"
-        )
+        tmpdir = self.makeTemporaryDirectory()
+        self.layer_files = []
+        for i in range(1, 3):
+            digest = 'digest_%i' % i
+            file_name = 'digest_%s_filename' % i
+            file_path = os.path.join(tmpdir, file_name)
+
+            with open(file_path, 'w') as fd:
+                fd.write(digest)
+
+            fileout = StringIO()
+            tar = tarfile.open(mode="w:gz", fileobj=fileout)
+            tar.add(file_path, 'layer.tar')
+            tar.close()
+
+            fileout.seek(0)
+            # make layer files
+            self.layer_files.append(self.factory.makeOCIFile(
+                build=self.build,
+                content=fileout.read(),
+                filename=file_name,
+                layer_file_digest=digest
+            ))
 
         transaction.commit()
 
@@ -113,12 +129,14 @@ class TestOCIRegistryClient(OCIConfigHelperMixin, TestCaseWithFactory):
                     "mediaType": Equals(
                         "application/vnd.docker.image.rootfs.diff.tar.gzip"),
                     "digest": Equals("diff_id_1"),
-                    "size": Equals(8)}),
+                    "size": Equals(
+                        self.layer_files[0].library_file.content.filesize)}),
                 MatchesDict({
                     "mediaType": Equals(
                         "application/vnd.docker.image.rootfs.diff.tar.gzip"),
                     "digest": Equals("diff_id_2"),
-                    "size": Equals(8)})
+                    "size": Equals(
+                        self.layer_files[1].library_file.content.filesize)})
             ]),
             "schemaVersion": Equals(2),
             "config": MatchesDict({
@@ -153,9 +171,9 @@ class TestOCIRegistryClient(OCIConfigHelperMixin, TestCaseWithFactory):
         responses.add("PUT", manifests_url, status=201)
         self.client.upload(self.build)
 
-        self.assertIn(
-            ('test-username', 'test-password'),
-            _upload_fixture.mock.call_args_list[0][0])
+        http_client = _upload_fixture.mock.call_args_list[0][0][-1]
+        self.assertEquals(
+            http_client.credentials, ('test-username', 'test-password'))
 
     def test_preloadFiles(self):
         self._makeFiles()
@@ -165,8 +183,8 @@ class TestOCIRegistryClient(OCIConfigHelperMixin, TestCaseWithFactory):
         self.assertThat(files, MatchesDict({
             'config_file_1.json': MatchesDict({
                 'config_file': Equals(self.config),
-                'diff_id_1': Equals(self.layer_1_file.library_file),
-                'diff_id_2': Equals(self.layer_2_file.library_file)})}))
+                'diff_id_1': Equals(self.layer_files[0].library_file),
+                'diff_id_2': Equals(self.layer_files[1].library_file)})}))
 
     def test_calculateTag(self):
         result = self.client._calculateTag(
@@ -189,12 +207,14 @@ class TestOCIRegistryClient(OCIConfigHelperMixin, TestCaseWithFactory):
                     "mediaType": Equals(
                         "application/vnd.docker.image.rootfs.diff.tar.gzip"),
                     "digest": Equals("diff_id_1"),
-                    "size": Equals(8)}),
+                    "size": Equals(
+                        self.layer_files[0].library_file.content.filesize)}),
                 MatchesDict({
                     "mediaType": Equals(
                         "application/vnd.docker.image.rootfs.diff.tar.gzip"),
                     "digest": Equals("diff_id_2"),
-                    "size": Equals(8)})
+                    "size": Equals(
+                        self.layer_files[1].library_file.content.filesize)})
             ]),
             "schemaVersion": Equals(2),
             "config": MatchesDict({
@@ -209,44 +229,47 @@ class TestOCIRegistryClient(OCIConfigHelperMixin, TestCaseWithFactory):
 
     @responses.activate
     def test_upload_handles_existing(self):
-        blobs_url = "{}/v2/{}/blobs/{}".format(
-            self.build.recipe.push_rules[0].registry_credentials.url,
-            "test-name",
-            "test-digest")
+        push_rule = self.build.recipe.push_rules[0]
+        http_client = RegistryHTTPClient(push_rule)
+        blobs_url = "{}/blobs/{}".format(
+            http_client.api_url, "test-digest")
         responses.add("HEAD", blobs_url, status=200)
+        push_rule = self.build.recipe.push_rules[0]
+        push_rule.registry_credentials.setCredentials({})
         self.client._upload(
-            "test-digest", self.build.recipe.push_rules[0],
-            "test-name", None, None)
+            "test-digest", push_rule, None, http_client)
         # There should be no auth headers for these calls
         for call in responses.calls:
             self.assertNotIn('Authorization', call.request.headers.keys())
 
     @responses.activate
     def test_upload_raises_non_404(self):
-        blobs_url = "{}/v2/{}/blobs/{}".format(
-            self.build.recipe.push_rules[0].registry_credentials.url,
-            "test-name",
-            "test-digest")
+        push_rule = self.build.recipe.push_rules[0]
+        http_client = RegistryHTTPClient(push_rule)
+        blobs_url = "{}/blobs/{}".format(
+            http_client.api_url, "test-digest")
         responses.add("HEAD", blobs_url, status=500)
+        push_rule = self.build.recipe.push_rules[0]
         self.assertRaises(
             HTTPError,
             self.client._upload,
             "test-digest",
-            self.build.recipe.push_rules[0],
-            "test-name",
+            push_rule,
             None,
-            None)
+            http_client)
 
     @responses.activate
     def test_upload_passes_basic_auth(self):
-        blobs_url = "{}/v2/{}/blobs/{}".format(
-            self.build.recipe.push_rules[0].registry_credentials.url,
-            "test-name",
-            "test-digest")
+        push_rule = self.build.recipe.push_rules[0]
+        http_client = RegistryHTTPClient(push_rule)
+        blobs_url = "{}/blobs/{}".format(
+            http_client.api_url, "test-digest")
         responses.add("HEAD", blobs_url, status=200)
+        push_rule.registry_credentials.setCredentials({
+            "username": "user", "password": "password"})
         self.client._upload(
-            "test-digest", self.build.recipe.push_rules[0],
-            "test-name", None, ('user', 'password'))
+            "test-digest", push_rule, None,
+            http_client)
 
         for call in responses.calls:
             self.assertEqual(
@@ -269,9 +292,10 @@ class TestOCIRegistryClient(OCIConfigHelperMixin, TestCaseWithFactory):
         self.client._upload.retry.sleep = lambda x: None
 
         try:
+            push_rule = self.build.recipe.push_rules[0]
             self.client._upload(
-                "test-digest", self.build.recipe.push_rules[0],
-                "test-name", None, ('user', 'password'))
+                "test-digest", push_rule,
+                None, RegistryHTTPClient(push_rule))
         except RetryError:
             pass
         # Check that tenacity and our counting agree