launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #24578
Re: [Merge] ~twom/launchpad:oci-registry-upload into launchpad:master
Diff comments:
> diff --git a/configs/development/launchpad-lazr.conf b/configs/development/launchpad-lazr.conf
> index a0b8f0e..2911838 100644
> --- a/configs/development/launchpad-lazr.conf
> +++ b/configs/development/launchpad-lazr.conf
> @@ -171,13 +171,18 @@ password: guest
> virtual_host: /
>
> [snappy]
> -builder_proxy_auth_api_admin_username: admin-launchpad.test
> +builder_proxy_auth_api_admin_username:
> builder_proxy_auth_api_endpoint: http://snap-proxy.launchpad.test:8080/tokens
> -builder_proxy_host: snap-proxy.launchpad.test
> -builder_proxy_port: 3128
> +builder_proxy_host:
> +builder_proxy_port:
OK, I guess I can see why this is helpful in order to be able to dispatch local builds with no proxy at all; but you could just omit builder_proxy_host and builder_proxy_port entirely rather than setting them to the empty string. (I'd be inclined to leave builder_proxy_auth_api_admin_username in place; like builder_proxy_auth_api_endpoint, it's harmless if builder_proxy_host is unset, and it's one less thing to configure if you actually are trying to set up a local proxy.)
> store_search_url: https://api.snapcraft.io/
> tools_source: deb http://ppa.launchpad.net/snappy-dev/snapcraft-daily/ubuntu %(series)s main
> +store_secrets_private_key: JCdUpBw60qKGURMWiAOi9M0c0J/ObnhV0zdnxCdd3nw=
> +store_secrets_public_key: xV+6FiHRgD5ch/ewN6qaV5NlSk0kfYg52UchRT5gXXs=
>
> +[oci]
> +registry_secrets_private_key: JCdUpBw60qKGURMWiAOi9M0c0J/ObnhV0zdnxCdd3nw=
> +registry_secrets_public_key: xV+6FiHRgD5ch/ewN6qaV5NlSk0kfYg52UchRT5gXXs=
Can we have a blank line to separate this from the next section?
Also generating NaCl private keys is easy; I'd suggest having the development keys for snappy.store_secrets_* and oci.registry_secrets_* be different, just in case some bit of code ever confuses the two.
> [rosetta]
> global_suggestions_enabled: True
> generate_templates: True
> diff --git a/database/schema/security.cfg b/database/schema/security.cfg
> index d3ee546..a7fb546 100644
> --- a/database/schema/security.cfg
> +++ b/database/schema/security.cfg
> @@ -2674,3 +2674,42 @@ public.teammembership = SELECT
> public.teamparticipation = SELECT
> public.webhook = SELECT
> public.webhookjob = SELECT, INSERT
> +
> +[oci-build-job]
As mentioned on IRC, we'll need to get a corresponding DB user created before this can be landed.
> +type=user
> +groups=script
> +public.account = SELECT
> +public.archive = SELECT
> +public.branch = SELECT
> +public.builder = SELECT
> +public.buildfarmjob = SELECT, INSERT
> +public.buildqueue = SELECT, INSERT, UPDATE
> +public.distribution = SELECT
> +public.distroarchseries = SELECT
> +public.distroseries = SELECT
> +public.emailaddress = SELECT
> +public.gitref = SELECT
> +public.gitrepository = SELECT
> +public.job = SELECT, INSERT, UPDATE
> +public.libraryfilealias = SELECT
> +public.libraryfilecontent = SELECT
> +public.person = SELECT
> +public.personsettings = SELECT
> +public.pocketchroot = SELECT
> +public.processor = SELECT
> +public.product = SELECT
> +public.ocirecipe = SELECT, UPDATE
> +public.ocirecipearch = SELECT
> +public.ocirecipebuild = SELECT, INSERT, UPDATE
> +public.ocirecipebuildjob = SELECT, UPDATE
> +public.ocifile = SELECT
> +public.ociproject = SELECT
> +public.ociprojectname = SELECT
> +public.ociprojectseries = SELECT
> +public.ocipushrule = SELECT
> +public.ociregistrycredentials = SELECT
> +public.sourcepackagename = SELECT
> +public.teammembership = SELECT
> +public.teamparticipation = SELECT
> +public.webhook = SELECT
> +public.webhookjob = SELECT, INSERT
> diff --git a/lib/lp/oci/browser/ocirecipebuild.py b/lib/lp/oci/browser/ocirecipebuild.py
> index 8b3a727..81453c6 100644
> --- a/lib/lp/oci/browser/ocirecipebuild.py
> +++ b/lib/lp/oci/browser/ocirecipebuild.py
> @@ -21,6 +22,10 @@ from lp.services.webapp import (
> )
>
>
> +class IOCIBuildStatusChangedEvent(IObjectEvent):
> + """The status of an OCI recipe build changed."""
This doesn't seem to have a corresponding definition yet.
Also, we should consider whether we can use the approach used in LiveFSBuild.updateStatus rather than having a custom event in this case.
> +
> +
> class OCIRecipeBuildNavigation(Navigation):
>
> usedfor = IOCIRecipeBuild
> diff --git a/lib/lp/oci/model/ocirecipebuild.py b/lib/lp/oci/model/ocirecipebuild.py
> index 12046d6..08e7fec 100644
> --- a/lib/lp/oci/model/ocirecipebuild.py
> +++ b/lib/lp/oci/model/ocirecipebuild.py
> @@ -65,6 +69,7 @@ from lp.services.propertycache import (
> get_property_cache,
> )
> from lp.services.webapp.snapshot import notify_modified
> +from lp.services.job.model.job import Job
Sort imports.
>
>
> @implementer(IOCIFile)
> diff --git a/lib/lp/oci/model/ocirecipebuildjob.py b/lib/lp/oci/model/ocirecipebuildjob.py
> index 7d8699c..7b1152a 100644
> --- a/lib/lp/oci/model/ocirecipebuildjob.py
> +++ b/lib/lp/oci/model/ocirecipebuildjob.py
> @@ -138,3 +144,40 @@ class OCIRecipeBuildJobDerived(BaseRunnableJob):
> ('oci_project_name', self.context.build.recipe.oci_project.name)
> ])
> return oops_vars
> +
> +
> +@implementer(IOCIRegistryUploadJob)
> +@provider(IOCIRegistryUploadJobSource)
> +class OCIRegistryUploadJob(OCIRecipeBuildJobDerived):
> +
> + class_job_type = OCIRecipeBuildJobType.REGISTRY_UPLOAD
> +
> + @classmethod
> + def create(cls, build):
> + """See `IOCIRegistryUploadJobSource`"""
> + oci_build_job = OCIRecipeBuildJob(
> + build, cls.class_job_type, {})
> + job = cls(oci_build_job)
> + job.celeryRunOnCommit()
> + del get_property_cache(build).last_registry_upload_job
> + # notify(SnapBuildStoreUploadStatusChangedEvent(ocibuild))
This seems like it might be quick to fill out properly, or else replace with a proper XXX comment. (In the snap build case it's used for webhooks, which will probably eventually make sense here too.)
> + return job
> +
> + @property
> + def error_message(self):
> + """See `IOCIRegistryUploadJob`."""
> + return self.json_data.get("error_message")
> +
> + @error_message.setter
> + def error_message(self, message):
> + """See `IOCIRegistryUploadJob`."""
> + self.json_data["error_message"] = message
> +
> + def run(self):
> + """See `IRunnableJob`."""
> + client = OCIRegistryClient()
> + try:
> + client = client.upload(self.build)
> + except Exception as e:
> + import pdb; pdb.set_trace()
> + print(e)
This seems like a debugging relic that shouldn't be left in production code.
> diff --git a/lib/lp/oci/model/ociregistryclient.py b/lib/lp/oci/model/ociregistryclient.py
> new file mode 100644
> index 0000000..8a6c4b8
> --- /dev/null
> +++ b/lib/lp/oci/model/ociregistryclient.py
> @@ -0,0 +1,167 @@
standard_template.py prologue?
Also, I think this file could generally use some more docstring attention. It's all going to be fairly obscure to somebody who needs to maintain this in a few years' time and might not have the context of having personally done battle with container registries.
> +import hashlib
> +import json
> +from StringIO import StringIO
> +import tarfile
> +
> +import requests
Could you use the urlfetch wrapper from lp.services.timeout so that the requests made here get proper timeouts applied to them?
> +
> +
> +class LayerNotFound(Exception):
> + pass
> +
> +
> +class LayerMountFailed(Exception):
> + pass
> +
> +
> +class LayerUploadFailed(Exception):
> + pass
> +
> +
> +class ManifestUploadFailed(Exception):
> + pass
> +
> +
> +class OCIRegistryClient():
You probably don't need the () here. (And standard_template.py would include the `__metaclass__ = type` business that arranges for this to be a new-style class even on Python 2 despite not explicitly inheriting from object.)
> +
> + def _getJSONfile(self, reference):
> + _, lfa, lfc = reference
> + try:
> + lfa.open()
> + return json.loads(lfa.read())
> + finally:
> + lfa.close()
> +
> + def _upload(self, digest, push_rule, name, fileobj):
> +
> + # Check if it already exists
> + head_response = requests.head(
> + "{}/v2/{}/blobs/{}".format(
> + push_rule.registry_credentials.url, name, digest))
> + if head_response.status_code == 200:
> + print("{} already found".format(digest))
This should probably use a logger instead.
> + return
> +
> + post_request = requests.post(
> + "{}/v2/{}/blobs/uploads/".format(
> + push_rule.registry_credentials.url, name))
> +
> + post_location = post_request.headers["Location"]
> + query_parsed = {"digest": digest}
> +
> + put_response = requests.put(
> + post_location, params=query_parsed,
> + data=fileobj)
> +
> + if put_response.status_code != 201:
> + raise LayerUploadFailed(
> + "Upload of {} for {} failed".format(digest, name))
> +
> + def _upload_layer(self, digest, push_rule, name, lfa):
> +
> + lfa.open()
> + try:
> + un_zipped = tarfile.open(fileobj=lfa, mode='r|gz')
> + for tarinfo in un_zipped:
> + if tarinfo.name != 'layer.tar':
> + continue
> + fileobj = un_zipped.extractfile(tarinfo)
> + self._upload(digest, push_rule, name, fileobj)
> + finally:
> + lfa.close()
> +
> + def build_image_manifest(self, name, tag, digests,
> + config, config_json, config_sha):
> + manifest = {
> + "schemaVersion": 2,
> + "mediaType":
> + "application/vnd.docker.distribution.manifest.v2+json",
> + "config": {
> + "mediaType": "application/vnd.docker.container.image.v1+json",
> + "size": len(config_json),
> + "digest": "sha256:{}".format(config_sha),
> + },
> + "layers": [],
> + }
> +
> + for layer in config["rootfs"]["diff_ids"]:
> + manifest["layers"].append(
> + {
> + "mediaType":
> + "application/vnd.docker.image.rootfs.diff.tar.gzip",
> + "size": 0, # This doesn't appear to matter?
> + "digest": layer,
> + }
> + )
> + return manifest
> +
> + def _preloadFiles(self, build, manifest, digests):
> + # preload the data from the librarian to avoid potential multiple
> + # pulls if there is more than one push rule
> + data = {}
> + for section in manifest:
> + config = self._getJSONfile(
> + build.getByFileName(section['Config']))
> + files = {"config_file": config}
> + for diff_id in config["rootfs"]["diff_ids"]:
> + files[diff_id] = {}
> + source = digests[diff_id]["source"]
> + source_digest = digests[diff_id]["digest"]
> + files[diff_id]["source"] = source_digest
> + _, lfa, _ = build.getLayerFileByDigest(source_digest)
> + files[diff_id]["lfa"] = lfa
> + files['attempt_mount'] = bool(source)
> + data[section["Config"]] = files
> + return data
> +
> + def upload(self, build):
> + # get manifest
> + manifest = self._getJSONfile(build.manifest)
> + digests_list = self._getJSONfile(build.digests)
> + digests = {}
> + for digest_dict in digests_list:
> + for k, v in digest_dict.items():
> + digests[k] = v
> +
> + # XXX twom 2020-04-06 This should be calculated
> + build_tag = "temp-build-tag"
This is only passed to build_image_manifest, which doesn't seem to actually use its value at all. Maybe it should just be removed?
> +
> + preloaded_data = self._preloadFiles(build, manifest, digests)
> +
> + for push_rule in build.recipe.push_rules:
> + for section in manifest:
> + file_data = preloaded_data[section["Config"]]
> + config = file_data["config_file"]
> + for diff_id in config["rootfs"]["diff_ids"]:
> + self._upload_layer(
> + diff_id,
> + push_rule,
> + push_rule.image_name,
> + file_data[diff_id].get('lfa'))
> + config_json = json.dumps(config).encode()
> + config_sha = hashlib.sha256(config_json).hexdigest()
> + self._upload(
> + "sha256:{}".format(config_sha),
> + push_rule,
> + push_rule.image_name,
> + StringIO(config_json)
StringIO.StringIO doesn't work on Python 3. The usual porting rule for this is that you figure out the actual type of the contents: bytes, Unicode, or whatever a native string is on the Python version in question. Then you pick a replacement according to the following table:
bytes: io.BytesIO
Unicode: io.StringIO
native string: six.StringIO
> + )
> + image_manifest = self.build_image_manifest(
> + push_rule.image_name, build_tag, digests,
> + config, config_json, config_sha)
> +
> + manifest_response = requests.put(
> + "{}/v2/{}/manifests/latest".format(
> + push_rule.registry_credentials.url,
> + push_rule.image_name),
> + json=image_manifest,
> + headers={
> + "Content-Type":
> + "application/"
> + "vnd.docker.distribution.manifest.v2+json"
> + },
> + )
> + if manifest_response.status_code != 201:
> + raise ManifestUploadFailed(
> + "Failed to upload manifest for {} in {}".format(
> + build.recipe.name, build.id))
> diff --git a/lib/lp/oci/tests/test_ociregistryclient.py b/lib/lp/oci/tests/test_ociregistryclient.py
> new file mode 100644
> index 0000000..2ab1734
> --- /dev/null
> +++ b/lib/lp/oci/tests/test_ociregistryclient.py
> @@ -0,0 +1,89 @@
> +# Copyright 2020 Canonical Ltd. This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""Tests for the OCI Registry client."""
> +
> +from __future__ import absolute_import, print_function, unicode_literals
> +
> +__metaclass__ = type
> +
> +import json
> +
> +import responses
> +import transaction
> +
> +from lp.oci.interfaces.ocirecipe import OCI_RECIPE_ALLOW_CREATE
> +from lp.oci.model.ociregistryclient import OCIRegistryClient
> +from lp.oci.tests.helpers import OCIConfigHelperMixin
> +from lp.services.features.testing import FeatureFixture
> +
> +from lp.testing import TestCaseWithFactory
> +from lp.testing.layers import LaunchpadZopelessLayer
> +from fixtures import MockPatch
Sort imports.
> +
> +
> +class TestOCIRegistryClient(OCIConfigHelperMixin, TestCaseWithFactory):
> +
> + layer = LaunchpadZopelessLayer
> +
> + def setUp(self):
> + super(TestOCIRegistryClient, self).setUp()
> + self.setConfig()
> + self.manifest = [{
> + "Config": "config_file_1.json",
> + "Layers": ["layer_1/layer.tar", "layer_2/layer.tar"]}]
> + self.digests = [{
> + "diff_id_1": {
> + "digest": "digest_1",
> + "source": "test/base_1",
> + "layer_id": "layer_1"
> + },
> + "diff_id_2": {
> + "digest": "digest_2",
> + "source": "",
> + "layer_id": "layer_2"
> + }
> + }]
> + self.config = {"rootfs": {"diff_ids": ["diff_id_1", "diff_id_2"]}}
> + self.build = self.factory.makeOCIRecipeBuild()
> + self.factory.makeOCIPushRule(recipe=self.build.recipe)
> +
> + @responses.activate
> + def test_upload(self):
> + self.factory.makeOCIFile(
> + build=self.build,
> + content=json.dumps(self.manifest),
> + filename='manifest.json',
> + )
> + self.factory.makeOCIFile(
> + build=self.build,
> + content=json.dumps(self.digests),
> + filename='digests.json',
> + )
> + self.factory.makeOCIFile(
> + build=self.build,
> + content=json.dumps(self.config),
> + filename='config_file_1.json'
> + )
> +
> + # make layer files
> + self.factory.makeOCIFile(
> + build=self.build,
> + content="digest_2",
> + filename="digest_2_filename",
> + layer_file_digest="digest_2"
> + )
> +
> + transaction.commit()
> + client = OCIRegistryClient()
> + self.useFixture(MockPatch(
> + "lp.oci.model.ociregistryclient.OCIRegistryClient._do_mount"))
> + self.useFixture(MockPatch(
> + "lp.oci.model.ociregistryclient.OCIRegistryClient._do_upload"))
> +
> + manifests_url = "{}/v2/{}/manifests/latest".format(
> + self.build.recipe.push_rules[0].registry_credentials.url,
> + self.build.recipe.push_rules[0].image_name
> + )
> + responses.add("PUT", manifests_url, status=201)
> + client.upload(self.build)
I feel like this should test the content of the upload somehow.
> diff --git a/lib/lp/oci/tests/test_ociregistrycredentials.py b/lib/lp/oci/tests/test_ociregistrycredentials.py
> index 50953da..62186bd 100644
> --- a/lib/lp/oci/tests/test_ociregistrycredentials.py
> +++ b/lib/lp/oci/tests/test_ociregistrycredentials.py
> @@ -102,6 +102,8 @@ class TestOCIRegistryCredentialsSet(OCIConfigHelperMixin, TestCaseWithFactory):
> self.factory.makeOCIRegistryCredentials()
> transaction.commit()
>
> + import transaction
> + transaction.commit()
Seems to be a duplicate of the commit immediately beforehand - possibly a rebase accident?
> found = getUtility(IOCIRegistryCredentialsSet).findByOwner(owner)
> self.assertEqual(found.count(), 3)
> for oci_credentials in found:
> diff --git a/lib/lp/snappy/model/snapstoreclient.py b/lib/lp/snappy/model/snapstoreclient.py
> index 9d007de..3e05464 100644
> --- a/lib/lp/snappy/model/snapstoreclient.py
> +++ b/lib/lp/snappy/model/snapstoreclient.py
> @@ -7,6 +7,7 @@ from __future__ import absolute_import, print_function, unicode_literals
>
> __metaclass__ = type
> __all__ = [
> + 'LibraryFileAliasWrapper',
I don't think this is meant to be exported at the moment (if it is, then we should move it somewhere else), and you haven't made use of it anywhere else in this branch.
> 'SnapStoreClient',
> ]
>
--
https://code.launchpad.net/~twom/launchpad/+git/launchpad/+merge/381981
Your team Launchpad code reviewers is requested to review the proposed merge of ~twom/launchpad:oci-registry-upload into launchpad:master.
