Merge ~cjwatson/launchpad:oci-registry-client-fix-exceptions into launchpad:master

[Colin Watson]

Fix HTTP error handling in OCIRegistryClient

urlfetch raises 4xx/5xx HTTP errors as exceptions, so we need to account for that when deciding whether to raise BlobUploadFailed or ManifestUploadFailed.

[Tom Wardill (twom)]

review: Needs Fixing

e325dbf... by Colin Watson on 2020-06-02

Fix handling of non-201 success responses

[Tom Wardill (twom)]

review: Approve

diff --git a/lib/lp/oci/model/ociregistryclient.py b/lib/lp/oci/model/ociregistryclient.py
index 771d0cf..f65a986 100644
--- a/lib/lp/oci/model/ociregistryclient.py
+++ b/lib/lp/oci/model/ociregistryclient.py
@@ -97,12 +97,14 @@ class OCIRegistryClient:
         post_location = post_response.headers["Location"]
         query_parsed = {"digest": digest}
 
-        put_response = http_client.request(
-            post_location,
-            params=query_parsed,
-            data=fileobj,
-            method="PUT")
-
+        try:
+            put_response = http_client.request(
+                post_location,
+                params=query_parsed,
+                data=fileobj,
+                method="PUT")
+        except HTTPError as http_error:
+            put_response = http_error.response
         if put_response.status_code != 201:
             msg = "Upload of {} for {} failed".format(
                 digest, push_rule.image_name)
@@ -256,15 +258,18 @@ class OCIRegistryClient:
                     preloaded_data[section["Config"]])
 
                 # Upload the registry manifest
-                manifest_response = http_client.requestPath(
-                    "/manifests/{}".format(tag),
-                    json=registry_manifest,
-                    headers={
-                        "Content-Type":
-                            "application/"
-                            "vnd.docker.distribution.manifest.v2+json"
-                        },
-                    method="PUT")
+                try:
+                    manifest_response = http_client.requestPath(
+                        "/manifests/{}".format(tag),
+                        json=registry_manifest,
+                        headers={
+                            "Content-Type":
+                                "application/"
+                                "vnd.docker.distribution.manifest.v2+json"
+                            },
+                        method="PUT")
+                except HTTPError as http_error:
+                    manifest_response = http_error.response
                 if manifest_response.status_code != 201:
                     raise ManifestUploadFailed(
                         "Failed to upload manifest for {} in {}".format(
diff --git a/lib/lp/oci/tests/test_ociregistryclient.py b/lib/lp/oci/tests/test_ociregistryclient.py
index e188790..879a562 100644
--- a/lib/lp/oci/tests/test_ociregistryclient.py
+++ b/lib/lp/oci/tests/test_ociregistryclient.py
@@ -7,9 +7,11 @@ from __future__ import absolute_import, print_function, unicode_literals
 
 __metaclass__ = type
 
+from functools import partial
 import json
 import os
 import tarfile
+import uuid
 
 from fixtures import MockPatch
 from requests.exceptions import (
@@ -20,13 +22,20 @@ import responses
 from six import StringIO
 from tenacity import RetryError
 from testtools.matchers import (
+    AfterPreprocessing,
     Equals,
     MatchesDict,
+    MatchesException,
     MatchesListwise,
+    Raises,
     )
 import transaction
 from zope.security.proxy import removeSecurityProxy
 
+from lp.oci.interfaces.ociregistryclient import (
+    BlobUploadFailed,
+    ManifestUploadFailed,
+    )
 from lp.oci.model.ociregistryclient import (
     BearerTokenRegistryClient,
     OCIRegistryAuthenticationError,
@@ -274,7 +283,7 @@ class TestOCIRegistryClient(OCIConfigHelperMixin, SpyProxyCallsMixin,
             self.assertNotIn('Authorization', call.request.headers.keys())
 
     @responses.activate
-    def test_upload_raises_non_404(self):
+    def test_upload_check_existing_raises_non_404(self):
         push_rule = self.build.recipe.push_rules[0]
         http_client = RegistryHTTPClient(push_rule)
         blobs_url = "{}/blobs/{}".format(
@@ -336,6 +345,124 @@ class TestOCIRegistryClient(OCIConfigHelperMixin, SpyProxyCallsMixin,
             5, self.client._upload.retry.statistics["attempt_number"])
         self.assertEqual(5, self.retry_count)
 
+    @responses.activate
+    def test_upload_put_blob_raises_error(self):
+        push_rule = self.build.recipe.push_rules[0]
+        http_client = RegistryHTTPClient(push_rule)
+        blobs_url = "{}/blobs/{}".format(
+            http_client.api_url, "test-digest")
+        uploads_url = "{}/blobs/uploads/".format(http_client.api_url)
+        upload_url = "{}/blobs/uploads/{}".format(
+            http_client.api_url, uuid.uuid4())
+        put_errors = [
+            {
+                "code": "BLOB_UPLOAD_INVALID",
+                "message": "blob upload invalid",
+                "detail": [],
+                },
+            ]
+        responses.add("HEAD", blobs_url, status=404)
+        responses.add("POST", uploads_url, headers={"Location": upload_url})
+        responses.add(
+            "PUT", upload_url, status=400, json={"errors": put_errors})
+        self.assertThat(
+            partial(
+                self.client._upload,
+                "test-digest", push_rule, None, http_client),
+            Raises(MatchesException(
+                BlobUploadFailed,
+                AfterPreprocessing(
+                    str,
+                    Equals(
+                        "Upload of {} for {} failed".format(
+                            "test-digest", push_rule.image_name))))))
+
+    @responses.activate
+    def test_upload_put_blob_raises_non_201_success(self):
+        push_rule = self.build.recipe.push_rules[0]
+        http_client = RegistryHTTPClient(push_rule)
+        blobs_url = "{}/blobs/{}".format(
+            http_client.api_url, "test-digest")
+        uploads_url = "{}/blobs/uploads/".format(http_client.api_url)
+        upload_url = "{}/blobs/uploads/{}".format(
+            http_client.api_url, uuid.uuid4())
+        responses.add("HEAD", blobs_url, status=404)
+        responses.add("POST", uploads_url, headers={"Location": upload_url})
+        responses.add("PUT", upload_url, status=200)
+        self.assertThat(
+            partial(
+                self.client._upload,
+                "test-digest", push_rule, None, http_client),
+            Raises(MatchesException(
+                BlobUploadFailed,
+                AfterPreprocessing(
+                    str,
+                    Equals(
+                        "Upload of {} for {} failed".format(
+                            "test-digest", push_rule.image_name))))))
+
+    @responses.activate
+    def test_upload_put_manifest_raises_error(self):
+        self._makeFiles()
+        self.useFixture(MockPatch(
+            "lp.oci.model.ociregistryclient.OCIRegistryClient._upload"))
+        self.useFixture(MockPatch(
+            "lp.oci.model.ociregistryclient.OCIRegistryClient._upload_layer"))
+
+        push_rule = self.build.recipe.push_rules[0]
+        responses.add(
+            "GET", "{}/v2/".format(push_rule.registry_url), status=200)
+
+        manifests_url = "{}/v2/{}/manifests/edge".format(
+            push_rule.registry_credentials.url,
+            push_rule.image_name)
+        put_errors = [
+            {
+                "code": "MANIFEST_INVALID",
+                "message": "manifest invalid",
+                "detail": [],
+                },
+            ]
+        responses.add(
+            "PUT", manifests_url, status=400, json={"errors": put_errors})
+
+        self.assertThat(
+            partial(self.client.upload, self.build),
+            Raises(MatchesException(
+                ManifestUploadFailed,
+                AfterPreprocessing(
+                    str,
+                    Equals(
+                        "Failed to upload manifest for {} in {}".format(
+                            self.build.recipe.name, self.build.id))))))
+
+    @responses.activate
+    def test_upload_put_manifest_raises_non_201_success(self):
+        self._makeFiles()
+        self.useFixture(MockPatch(
+            "lp.oci.model.ociregistryclient.OCIRegistryClient._upload"))
+        self.useFixture(MockPatch(
+            "lp.oci.model.ociregistryclient.OCIRegistryClient._upload_layer"))
+
+        push_rule = self.build.recipe.push_rules[0]
+        responses.add(
+            "GET", "{}/v2/".format(push_rule.registry_url), status=200)
+
+        manifests_url = "{}/v2/{}/manifests/edge".format(
+            push_rule.registry_credentials.url,
+            push_rule.image_name)
+        responses.add("PUT", manifests_url, status=200)
+
+        self.assertThat(
+            partial(self.client.upload, self.build),
+            Raises(MatchesException(
+                ManifestUploadFailed,
+                AfterPreprocessing(
+                    str,
+                    Equals(
+                        "Failed to upload manifest for {} in {}".format(
+                            self.build.recipe.name, self.build.id))))))
+
 
 class TestRegistryHTTPClient(OCIConfigHelperMixin, SpyProxyCallsMixin,
                              TestCaseWithFactory):