Merge ~pappacena/launchpad:oci-upload-manifest-upsert into launchpad:master

[Thiago F. Pappacena]

Updating existing manifest (if any) when uploading OCI manifest, instead of totally replacing it.

[Colin Watson (cjwatson)]

LGTM in principle, but I'd definitely like to see a bit more thought on error handling before landing this, since at present it could be difficult to debug what's going on if there's some kind of outage and I'm not sure the implemented behaviour in that case is necessarily what we want.

review: Approve

6db1b18... by Thiago F. Pappacena on 2020-11-23

Do not override current manifest on HTTP errors other than 404

[Thiago F. Pappacena (pappacena)]

Pushed the requested changes.

[Colin Watson (cjwatson)]

review: Approve

62fff65... by Thiago F. Pappacena on 2020-11-23

More log details when 404 GETting existing OCI manifest

[Colin Watson (cjwatson)]

review: Approve

diff --git a/lib/lp/oci/model/ociregistryclient.py b/lib/lp/oci/model/ociregistryclient.py
index be370b4..56225da 100644
--- a/lib/lp/oci/model/ociregistryclient.py
+++ b/lib/lp/oci/model/ociregistryclient.py
@@ -235,6 +235,18 @@ class OCIRegistryClient:
         return "{}".format("edge")
 
     @classmethod
+    def _getCurrentRegistryManifest(cls, http_client, push_rule):
+        """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(
+            url, method="GET", headers={"Accept": accept})
+        return response.json()
+
+    @classmethod
     def _uploadRegistryManifest(cls, http_client, registry_manifest,
                                 push_rule, build=None):
         """Uploads the build manifest, returning its content information.
@@ -353,11 +365,46 @@ class OCIRegistryClient:
             raise MultipleOCIRegistryError(exceptions)
 
     @classmethod
-    def makeMultiArchManifest(cls, build_request, uploaded_builds):
+    def makeMultiArchManifest(cls, http_client, push_rule, build_request,
+                              uploaded_builds):
         """Returns the multi-arch manifest content including all uploaded
         builds of the given build_request.
         """
-        manifests = []
+        def get_manifest_for_architecture(manifests, arch):
+            """Find, in the manifests list, the manifest for the given arch."""
+            expected_platform = {"architecture": arch, "os": "linux"}
+            for m in manifests:
+                if m["platform"] == expected_platform:
+                    return m
+            return None
+
+        try:
+            current_manifest = cls._getCurrentRegistryManifest(
+                http_client, push_rule)
+            # 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:
+                current_manifest = None
+        except HTTPError as e:
+            if e.response.status_code == 404:
+                # If there is no manifest file (or it doesn't follow the
+                # multi-arch spec), we should proceed adding our own
+                # manifest file.
+                current_manifest = None
+                msg_tpl = (
+                    "No multi-arch manifest on registry %s (image name: %s). "
+                    "Uploading a new one.")
+                log.info(msg_tpl % (
+                    push_rule.registry_url, push_rule.image_name))
+            else:
+                raise
+        if current_manifest is None:
+            current_manifest = {
+                "schemaVersion": 2,
+                "mediaType": ("application/"
+                              "vnd.docker.distribution.manifest.list.v2+json"),
+                "manifests": []}
+        manifests = current_manifest["manifests"]
         for build in uploaded_builds:
             build_manifest = build_request.uploaded_manifests.get(build.id)
             if not build_manifest:
@@ -365,28 +412,33 @@ class OCIRegistryClient:
             digest = build_manifest["digest"]
             size = build_manifest["size"]
             arch = build.processor.name
-            manifests.append({
-                "mediaType": ("application/"
-                              "vnd.docker.distribution.manifest.v2+json"),
-                "size": size,
-                "digest": digest,
-                "platform": {"architecture": arch, "os": "linux"}
-            })
-        return {
-          "schemaVersion": 2,
-          "mediaType": ("application/"
-                        "vnd.docker.distribution.manifest.list.v2+json"),
-          "manifests": manifests}
+
+            manifest = get_manifest_for_architecture(manifests, arch)
+            if manifest is None:
+                manifest = {
+                    "mediaType": ("application/"
+                                  "vnd.docker.distribution.manifest.v2+json"),
+                    "size": size,
+                    "digest": digest,
+                    "platform": {"architecture": arch, "os": "linux"}
+                }
+                manifests.append(manifest)
+            else:
+                manifest["digest"] = digest
+                manifest["size"] = size
+                manifest["platform"]["architecture"] = arch
+
+        return current_manifest
 
     @classmethod
     def uploadManifestList(cls, build_request, uploaded_builds):
         """Uploads to all build_request.recipe.push_rules the manifest list
         for the builds in the given build_request.
         """
-        multi_manifest_content = cls.makeMultiArchManifest(
-            build_request, uploaded_builds)
         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)
 
diff --git a/lib/lp/oci/tests/test_ociregistryclient.py b/lib/lp/oci/tests/test_ociregistryclient.py
index 162de72..65e416b 100644
--- a/lib/lp/oci/tests/test_ociregistryclient.py
+++ b/lib/lp/oci/tests/test_ociregistryclient.py
@@ -542,7 +542,7 @@ class TestOCIRegistryClient(OCIConfigHelperMixin, SpyProxyCallsMixin,
                     MatchesStructure(errors=Is(None))))))
 
     @responses.activate
-    def test_multi_arch_manifest_upload(self):
+    def test_multi_arch_manifest_upload_new_manifest(self):
         """Ensure that multi-arch manifest upload works and tags correctly
         the uploaded image."""
         # Creates a build request with 2 builds.
@@ -576,15 +576,21 @@ class TestOCIRegistryClient(OCIConfigHelperMixin, SpyProxyCallsMixin,
 
         push_rule = self.build.recipe.push_rules[0]
         responses.add(
+            "GET", "{}/v2/{}/manifests/edge".format(
+                push_rule.registry_url, push_rule.image_name),
+            status=404)
+        self.addManifestResponses(push_rule, status_code=201)
+
+        responses.add(
             "GET", "{}/v2/".format(push_rule.registry_url), status=200)
         self.addManifestResponses(push_rule, status_code=201)
 
         # Let's try to generate the manifest for just 2 of the 3 builds:
         self.client.uploadManifestList(build_request, [build1, build2])
-        self.assertEqual(2, len(responses.calls))
-        auth_call, manifest_call = responses.calls
+        self.assertEqual(3, len(responses.calls))
+        auth_call, get_manifest_call, send_manifest_call = responses.calls
         self.assertEndsWith(
-            manifest_call.request.url,
+            send_manifest_call.request.url,
             "/v2/%s/manifests/edge" % push_rule.image_name)
         self.assertEqual({
             "schemaVersion": 2,
@@ -603,7 +609,185 @@ class TestOCIRegistryClient(OCIConfigHelperMixin, SpyProxyCallsMixin,
                 "digest": "build2digest",
                 "size": 321
             }]
-        }, json.loads(manifest_call.request.body))
+        }, json.loads(send_manifest_call.request.body))
+
+    @responses.activate
+    def test_multi_arch_manifest_upload_update_manifest(self):
+        """Makes sure we update only new architectures if there is already
+        a manifest file in registry.
+        """
+        current_manifest = {
+            "schemaVersion": 2,
+            "mediaType": "application/"
+                         "vnd.docker.distribution.manifest.list.v2+json",
+            "manifests": [{
+                "platform": {"os": "linux", "architecture": "386"},
+                "mediaType": "application/"
+                             "vnd.docker.distribution.manifest.v2+json",
+                "digest": "initial-386-digest",
+                "size": 110
+            }, {
+                "platform": {"os": "linux", "architecture": "amd64"},
+                "mediaType": "application/"
+                             "vnd.docker.distribution.manifest.v2+json",
+                "digest": "initial-amd64-digest",
+                "size": 220
+            }]
+        }
+
+        # Creates a build request with 2 builds: amd64 (which is already in
+        # the manifest) and hppa (that should be added)
+        recipe = self.build.recipe
+        build1 = self.build
+        build2 = self.factory.makeOCIRecipeBuild(
+            recipe=recipe)
+        naked_build1 = removeSecurityProxy(build1)
+        naked_build2 = removeSecurityProxy(build2)
+        naked_build1.processor = getUtility(IProcessorSet).getByName('amd64')
+        naked_build2.processor = getUtility(IProcessorSet).getByName('hppa')
+
+        # Creates a mock IOCIRecipeRequestBuildsJobSource, as it was created
+        # by the celery job and triggered the 3 registry uploads already.
+        job = mock.Mock()
+        job.builds = [build1, build2]
+        job.uploaded_manifests = {
+            build1.id: {"digest": "new-build1-digest", "size": 1111},
+            build2.id: {"digest": "new-build2-digest", "size": 2222},
+        }
+        job_source = mock.Mock()
+        job_source.getByOCIRecipeAndID.return_value = job
+        self.useFixture(
+            ZopeUtilityFixture(job_source, IOCIRecipeRequestBuildsJobSource))
+        build_request = OCIRecipeBuildRequest(recipe, -1)
+
+        push_rule = self.build.recipe.push_rules[0]
+        responses.add(
+            "GET", "{}/v2/{}/manifests/edge".format(
+                push_rule.registry_url, push_rule.image_name),
+            json=current_manifest,
+            status=200)
+        self.addManifestResponses(push_rule, status_code=201)
+
+        responses.add(
+            "GET", "{}/v2/".format(push_rule.registry_url), status=200)
+        self.addManifestResponses(push_rule, status_code=201)
+
+        self.client.uploadManifestList(build_request, [build1, build2])
+        self.assertEqual(3, len(responses.calls))
+        auth_call, get_manifest_call, send_manifest_call = responses.calls
+        self.assertEndsWith(
+            send_manifest_call.request.url,
+            "/v2/%s/manifests/edge" % push_rule.image_name)
+        self.assertEqual({
+            "schemaVersion": 2,
+            "mediaType": "application/"
+                         "vnd.docker.distribution.manifest.list.v2+json",
+            "manifests": [{
+                "platform": {"os": "linux", "architecture": "386"},
+                "mediaType": "application/"
+                             "vnd.docker.distribution.manifest.v2+json",
+                "digest": "initial-386-digest",
+                "size": 110
+            }, {
+                "platform": {"os": "linux", "architecture": "amd64"},
+                "mediaType": "application/"
+                             "vnd.docker.distribution.manifest.v2+json",
+                "digest": "new-build1-digest",
+                "size": 1111
+            }, {
+                "platform": {"os": "linux", "architecture": "hppa"},
+                "mediaType": "application/"
+                             "vnd.docker.distribution.manifest.v2+json",
+                "digest": "new-build2-digest",
+                "size": 2222
+            }]
+        }, json.loads(send_manifest_call.request.body))
+
+    @responses.activate
+    def test_multi_arch_manifest_upload_invalid_current_manifest(self):
+        """Makes sure we create a new multi-arch manifest if existing manifest
+        file is using another unknown format.
+        """
+        current_manifest = {'schemaVersion': 1, 'layers': []}
+
+        # Creates a build request with 1 build for amd64.
+        recipe = self.build.recipe
+        build1 = self.build
+        naked_build1 = removeSecurityProxy(build1)
+        naked_build1.processor = getUtility(IProcessorSet).getByName('amd64')
+
+        job = mock.Mock()
+        job.builds = [build1]
+        job.uploaded_manifests = {
+            build1.id: {"digest": "new-build1-digest", "size": 1111},
+        }
+        job_source = mock.Mock()
+        job_source.getByOCIRecipeAndID.return_value = job
+        self.useFixture(
+            ZopeUtilityFixture(job_source, IOCIRecipeRequestBuildsJobSource))
+        build_request = OCIRecipeBuildRequest(recipe, -1)
+
+        push_rule = self.build.recipe.push_rules[0]
+        responses.add(
+            "GET", "{}/v2/{}/manifests/edge".format(
+                push_rule.registry_url, push_rule.image_name),
+            json=current_manifest,
+            status=200)
+        self.addManifestResponses(push_rule, status_code=201)
+
+        responses.add(
+            "GET", "{}/v2/".format(push_rule.registry_url), status=200)
+        self.addManifestResponses(push_rule, status_code=201)
+
+        self.client.uploadManifestList(build_request, [build1])
+        self.assertEqual(3, len(responses.calls))
+        auth_call, get_manifest_call, send_manifest_call = responses.calls
+        self.assertEndsWith(
+            send_manifest_call.request.url,
+            "/v2/%s/manifests/edge" % push_rule.image_name)
+        self.assertEqual({
+            "schemaVersion": 2,
+            "mediaType": "application/"
+                         "vnd.docker.distribution.manifest.list.v2+json",
+            "manifests": [{
+                "platform": {"os": "linux", "architecture": "amd64"},
+                "mediaType": "application/"
+                             "vnd.docker.distribution.manifest.v2+json",
+                "digest": "new-build1-digest",
+                "size": 1111
+            }]
+        }, json.loads(send_manifest_call.request.body))
+
+    @responses.activate
+    def test_multi_arch_manifest_upload_registry_error_fetching_current(self):
+        """Makes sure we abort the image upload if we get an error that is
+        not 404 when fetching the current manifest file.
+        """
+        job = mock.Mock()
+        job.builds = [self.build]
+        job.uploaded_manifests = {
+            self.build.id: {"digest": "new-build1-digest", "size": 1111},
+        }
+        job_source = mock.Mock()
+        job_source.getByOCIRecipeAndID.return_value = job
+        self.useFixture(
+            ZopeUtilityFixture(job_source, IOCIRecipeRequestBuildsJobSource))
+        build_request = OCIRecipeBuildRequest(self.build.recipe, -1)
+
+        push_rule = self.build.recipe.push_rules[0]
+        responses.add(
+            "GET", "{}/v2/{}/manifests/edge".format(
+                push_rule.registry_url, push_rule.image_name),
+            json={"error": "Unknown"},
+            status=503)
+
+        responses.add(
+            "GET", "{}/v2/".format(push_rule.registry_url), status=200)
+        self.addManifestResponses(push_rule, status_code=201)
+
+        self.assertRaises(
+            HTTPError, self.client.uploadManifestList,
+            build_request, [self.build])
 
 
 class TestRegistryHTTPClient(OCIConfigHelperMixin, SpyProxyCallsMixin,