launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #24744
[Merge] ~pappacena/launchpad:refactor-oci-push-auth into launchpad:master
Thiago F. Pappacena has proposed merging ~pappacena/launchpad:refactor-oci-push-auth into launchpad:master.
Commit message:
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).
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/384052
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.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~pappacena/launchpad:refactor-oci-push-auth into launchpad:master.
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..c51d752 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.request_path(
+ "/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.request_path(
+ "/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.get_instance(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.request_path(
+ "/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 request_path(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 get_instance(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
