launchpad-reviewers team mailing list archive

[Tom Wardill <tom.wardill@xxxxxxxxxxxxx>]

Tom Wardill has proposed merging ~twom/launchpad:oci-policy-tags-attached-by-strings into launchpad:master with ~twom/launchpad:oci-policy-push-tags-to-the-limit as a prerequisite.

Commit message:
Add multiple tag on push support.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~twom/launchpad/+git/launchpad/+merge/395420
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~twom/launchpad:oci-policy-tags-attached-by-strings into launchpad:master.

diff --git a/lib/lp/oci/model/ocirecipebuildjob.py b/lib/lp/oci/model/ocirecipebuildjob.py
index 1e4a7e8..b07c6bf 100644
--- a/lib/lp/oci/model/ocirecipebuildjob.py
+++ b/lib/lp/oci/model/ocirecipebuildjob.py
@@ -4,6 +4,7 @@
 """OCIRecipe build jobs."""
 
 from __future__ import absolute_import, print_function, unicode_literals
+import pdb
 
 __metaclass__ = type
 __all__ = [
@@ -351,15 +352,6 @@ class OCIRegistryUploadJob(OCIRecipeBuildJobDerived):
                     self.build_uploaded = True
 
                 self.uploadManifestList(client)
-                # Force this job status to COMPLETED in the same transaction we
-                # called `getUploadedBuilds` (in the uploadManifestList call
-                # above) to release the lock already including our new status.
-                # This way, any other transaction that was blocked waiting to
-                # get the info about the upload jobs will immediately have the
-                # new status of this job, avoiding race conditions. Keep the
-                # `manage_transaction=False` to prevent the method from
-                # commiting at the wrong moment.
-                self.complete(manage_transaction=False)
 
             except OCIRegistryError as e:
                 self.error_summary = str(e)
diff --git a/lib/lp/oci/model/ociregistryclient.py b/lib/lp/oci/model/ociregistryclient.py
index b7d41c6..dd09be8 100644
--- a/lib/lp/oci/model/ociregistryclient.py
+++ b/lib/lp/oci/model/ociregistryclient.py
@@ -242,22 +242,18 @@ class OCIRegistryClient:
         return data
 
     @classmethod
-    def _calculateTag(cls, build, push_rule):
+    def _calculateTags(cls, push_rule):
         """Work out the base tag for the image should be.
 
-        :param build: `OCIRecipeBuild` representing this build.
         :param push_rule: `OCIPushRule` that we are using.
         """
-        # XXX twom 2020-04-17 This needs to include OCIProjectSeries and
-        # base image name
-        return "{}".format("edge")
+        return push_rule.tags or ["{}".format("edge")]
 
     @classmethod
-    def _getCurrentRegistryManifest(cls, http_client, push_rule):
+    def _getCurrentRegistryManifest(cls, http_client, tag):
         """Get the current manifest for the given push rule. If manifest
         doesn't exist, raises HTTPError.
         """
-        tag = cls._calculateTag(None, push_rule)
         url = "/manifests/{}".format(tag)
         accept = "application/vnd.docker.distribution.manifest.list.v2+json"
         response = http_client.requestPath(
@@ -266,7 +262,7 @@ class OCIRegistryClient:
 
     @classmethod
     def _uploadRegistryManifest(cls, http_client, registry_manifest,
-                                push_rule, build=None):
+                                push_rule, tag, build=None):
         """Uploads the build manifest, returning its content information.
 
         The returned information can be used to create a Manifest list
@@ -275,18 +271,15 @@ class OCIRegistryClient:
 
         :return: A dict with {"digest": "sha256:xxx", "size": total_bytes}
         """
+        # This is
+        # https://docs.docker.com/registry/spec/manifest-v2-2/#manifest-list
+        # Specifically the Schema 2 manifest.
         digest = None
         data = json.dumps(registry_manifest)
         size = len(data)
         content_type = registry_manifest.get(
             "mediaType",
             "application/vnd.docker.distribution.manifest.v2+json")
-        if build is None:
-            # When uploading a manifest list, use the tag.
-            tag = cls._calculateTag(build, push_rule)
-        else:
-            # When uploading individual build manifests, use their digest.
-            tag = "sha256:%s" % hashlib.sha256(data).hexdigest()
         try:
             manifest_response = http_client.requestPath(
                 "/manifests/{}".format(tag),
@@ -310,7 +303,7 @@ class OCIRegistryClient:
 
     @classmethod
     def _upload_to_push_rule(
-            cls, push_rule, build, manifest, digests, preloaded_data):
+            cls, push_rule, build, manifest, digests, preloaded_data, tag):
         http_client = RegistryHTTPClient.getInstance(push_rule)
 
         for section in manifest:
@@ -345,7 +338,7 @@ class OCIRegistryClient:
 
             # Upload the registry manifest
             manifest = cls._uploadRegistryManifest(
-                http_client, registry_manifest, push_rule, build)
+                http_client, registry_manifest, push_rule, tag, build)
 
             # Save the uploaded manifest location, so we can use it in case
             # this is a multi-arch image upload.
@@ -372,11 +365,13 @@ class OCIRegistryClient:
 
         exceptions = []
         for push_rule in build.recipe.push_rules:
-            try:
-                cls._upload_to_push_rule(
-                    push_rule, build, manifest, digests, preloaded_data)
-            except Exception as e:
-                exceptions.append(e)
+            for tag in cls._calculateTags(push_rule):
+                try:
+                    cls._upload_to_push_rule(
+                        push_rule, build, manifest, digests, preloaded_data,
+                        tag)
+                except Exception as e:
+                    exceptions.append(e)
         if len(exceptions) == 1:
             raise exceptions[0]
         elif len(exceptions) > 1:
@@ -384,7 +379,7 @@ class OCIRegistryClient:
 
     @classmethod
     def makeMultiArchManifest(cls, http_client, push_rule, build_request,
-                              uploaded_builds):
+                              uploaded_builds, tag):
         """Returns the multi-arch manifest content including all uploaded
         builds of the given build_request.
         """
@@ -398,7 +393,7 @@ class OCIRegistryClient:
 
         try:
             current_manifest = cls._getCurrentRegistryManifest(
-                http_client, push_rule)
+                http_client, tag)
             # Check if the current manifest is not an incompatible version.
             version = current_manifest.get("schemaVersion", 1)
             if version < 2 or "manifests" not in current_manifest:
@@ -454,11 +449,14 @@ class OCIRegistryClient:
         for the builds in the given build_request.
         """
         for push_rule in build_request.recipe.push_rules:
-            http_client = RegistryHTTPClient.getInstance(push_rule)
-            multi_manifest_content = cls.makeMultiArchManifest(
-                http_client, push_rule, build_request, uploaded_builds)
-            cls._uploadRegistryManifest(
-                http_client, multi_manifest_content, push_rule, build=None)
+            for tag in cls._calculateTags(push_rule):
+                http_client = RegistryHTTPClient.getInstance(push_rule)
+                multi_manifest_content = cls.makeMultiArchManifest(
+                    http_client, push_rule, build_request, uploaded_builds,
+                    tag)
+                cls._uploadRegistryManifest(
+                    http_client, multi_manifest_content, push_rule, tag,
+                    build=None)
 
 
 class OCIRegistryAuthenticationError(Exception):
diff --git a/lib/lp/oci/tests/test_ociregistryclient.py b/lib/lp/oci/tests/test_ociregistryclient.py
index 79d62b1..c432a2e 100644
--- a/lib/lp/oci/tests/test_ociregistryclient.py
+++ b/lib/lp/oci/tests/test_ociregistryclient.py
@@ -63,6 +63,7 @@ from lp.services.compat import mock
 from lp.services.features.testing import FeatureFixture
 from lp.testing import (
     admin_logged_in,
+    person_logged_in,
     TestCaseWithFactory,
     )
 from lp.testing.fixture import ZopeUtilityFixture
@@ -169,11 +170,12 @@ class TestOCIRegistryClient(OCIConfigHelperMixin, SpyProxyCallsMixin,
             "PUT", re.compile(manifests_url), status=status_code, json=json)
 
         # PUT to tagged multi-arch manifest.
-        manifests_url = "{}/v2/{}/manifests/edge".format(
-            push_rule.registry_credentials.url,
-            push_rule.image_name
-        )
-        responses.add("PUT", manifests_url, status=status_code, json=json)
+        for tag in self.client._calculateTags(push_rule):
+            manifests_url = "{}/v2/{}/manifests/{}".format(
+                push_rule.registry_credentials.url,
+                push_rule.image_name,
+                tag)
+            responses.add("PUT", manifests_url, status=status_code, json=json)
 
     @responses.activate
     def test_upload(self):
@@ -220,6 +222,55 @@ class TestOCIRegistryClient(OCIConfigHelperMixin, SpyProxyCallsMixin,
         }))
 
     @responses.activate
+    def test_upload_multiple_tags(self):
+        self._makeFiles()
+        self.useFixture(MockPatch(
+            "lp.oci.model.ociregistryclient.OCIRegistryClient._upload"))
+        self.useFixture(MockPatch(
+            "lp.oci.model.ociregistryclient.OCIRegistryClient._upload_layer",
+            return_value=999))
+
+        push_rule = self.build.recipe.push_rules[0]
+        push_rule.tags = ['one', 'two']
+
+        responses.add("GET", "%s/v2/" % push_rule.registry_url, status=200)
+
+        self.addManifestResponses(push_rule)
+
+        self.client.upload(self.build)
+
+        put_responses = [json.loads(x.request.body) for x in responses.calls
+                         if x.request.method == "PUT"]
+        for request in put_responses:
+            self.assertThat(request, MatchesDict({
+                "layers": MatchesListwise([
+                    MatchesDict({
+                        "mediaType": Equals(
+                            "application/"
+                            "vnd.docker.image.rootfs.diff.tar.gzip"),
+                        "digest": Equals("diff_id_1"),
+                        "size": Equals(999)}),
+                    MatchesDict({
+                        "mediaType": Equals(
+                            "application/"
+                            "vnd.docker.image.rootfs.diff.tar.gzip"),
+                        "digest": Equals("diff_id_2"),
+                        "size": Equals(999)})
+                ]),
+                "schemaVersion": Equals(2),
+                "config": MatchesDict({
+                    "mediaType": Equals(
+                        "application/vnd.docker.container.image.v1+json"),
+                    "digest": Equals(
+                        "sha256:33b69b4b6e106f9fc7a8b93409"
+                        "36c85cf7f84b2d017e7b55bee6ab214761f6ab"),
+                    "size": Equals(52)
+                }),
+                "mediaType": Equals(
+                    "application/vnd.docker.distribution.manifest.v2+json")
+            }))
+
+    @responses.activate
     def test_upload_formats_credentials(self):
         self._makeFiles()
         _upload_fixture = self.useFixture(MockPatch(
@@ -307,10 +358,15 @@ class TestOCIRegistryClient(OCIConfigHelperMixin, SpyProxyCallsMixin,
                 '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(
-            self.build, self.build.recipe.push_rules[0])
-        self.assertEqual("edge", result)
+    def test_calculateTags(self):
+        result = self.client._calculateTags(self.build.recipe.push_rules[0])
+        self.assertEqual(["edge"], result)
+
+    def test_calculateTags_multiple_tags_in_push_rule(self):
+        tags = ['one', 'two']
+        self.build.recipe.push_rules[0].tags = tags
+        result = self.client._calculateTags(self.build.recipe.push_rules[0])
+        self.assertEqual(tags, result)
 
     def test_build_registry_manifest(self):
         self._makeFiles()