Launchpad itself

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

  1. Git
  2. lp:~cjwatson/launchpad
  3. oci-registry-client-fix-exceptions
  4. Merge into master
Proposed by Colin Watson
Status: Merged
Approved by: Colin Watson
Approved revision: e325dbfb0b688c23923c77ece224073bd9077c5b
Merge reported by: Otto Co-Pilot
Merged at revision: not available
Proposed branch: ~cjwatson/launchpad:oci-registry-client-fix-exceptions
Merge into: launchpad:master
Diff against target: 224 lines (+148/-16)
2 files modified
lib/lp/oci/model/ociregistryclient.py (+20/-15)
lib/lp/oci/tests/test_ociregistryclient.py (+128/-1)
Reviewer Review Type Date Requested Status
Tom Wardill (community) Approve
Review via email: mp+384957@code.launchpad.net

Commit message

Fix HTTP error handling in OCIRegistryClient

Description of the change

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

To post a comment you must log in.
Revision history for this message
Tom Wardill (twom) :
review: Needs Fixing
~cjwatson/launchpad:oci-registry-client-fix-exceptions updated
e325dbf... by Colin Watson

Fix handling of non-201 success responses

Revision history for this message
Colin Watson (cjwatson) :
Revision history for this message
Tom Wardill (twom) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
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:
97 post_location = post_response.headers["Location"]97 post_location = post_response.headers["Location"]
98 query_parsed = {"digest": digest}98 query_parsed = {"digest": digest}
9999
100 put_response = http_client.request(100 try:
101 post_location,101 put_response = http_client.request(
102 params=query_parsed,102 post_location,
103 data=fileobj,103 params=query_parsed,
104 method="PUT")104 data=fileobj,
105105 method="PUT")
106 except HTTPError as http_error:
107 put_response = http_error.response
106 if put_response.status_code != 201:108 if put_response.status_code != 201:
107 msg = "Upload of {} for {} failed".format(109 msg = "Upload of {} for {} failed".format(
108 digest, push_rule.image_name)110 digest, push_rule.image_name)
@@ -256,15 +258,18 @@ class OCIRegistryClient:
256 preloaded_data[section["Config"]])258 preloaded_data[section["Config"]])
257259
258 # Upload the registry manifest260 # Upload the registry manifest
259 manifest_response = http_client.requestPath(261 try:
260 "/manifests/{}".format(tag),262 manifest_response = http_client.requestPath(
261 json=registry_manifest,263 "/manifests/{}".format(tag),
262 headers={264 json=registry_manifest,
263 "Content-Type":265 headers={
264 "application/"266 "Content-Type":
265 "vnd.docker.distribution.manifest.v2+json"267 "application/"
266 },268 "vnd.docker.distribution.manifest.v2+json"
267 method="PUT")269 },
270 method="PUT")
271 except HTTPError as http_error:
272 manifest_response = http_error.response
268 if manifest_response.status_code != 201:273 if manifest_response.status_code != 201:
269 raise ManifestUploadFailed(274 raise ManifestUploadFailed(
270 "Failed to upload manifest for {} in {}".format(275 "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
77
8__metaclass__ = type8__metaclass__ = type
99
10from functools import partial
10import json11import json
11import os12import os
12import tarfile13import tarfile
14import uuid
1315
14from fixtures import MockPatch16from fixtures import MockPatch
15from requests.exceptions import (17from requests.exceptions import (
@@ -20,13 +22,20 @@ import responses
20from six import StringIO22from six import StringIO
21from tenacity import RetryError23from tenacity import RetryError
22from testtools.matchers import (24from testtools.matchers import (
25 AfterPreprocessing,
23 Equals,26 Equals,
24 MatchesDict,27 MatchesDict,
28 MatchesException,
25 MatchesListwise,29 MatchesListwise,
30 Raises,
26 )31 )
27import transaction32import transaction
28from zope.security.proxy import removeSecurityProxy33from zope.security.proxy import removeSecurityProxy
2934
35from lp.oci.interfaces.ociregistryclient import (
36 BlobUploadFailed,
37 ManifestUploadFailed,
38 )
30from lp.oci.model.ociregistryclient import (39from lp.oci.model.ociregistryclient import (
31 BearerTokenRegistryClient,40 BearerTokenRegistryClient,
32 OCIRegistryAuthenticationError,41 OCIRegistryAuthenticationError,
@@ -274,7 +283,7 @@ class TestOCIRegistryClient(OCIConfigHelperMixin, SpyProxyCallsMixin,
274 self.assertNotIn('Authorization', call.request.headers.keys())283 self.assertNotIn('Authorization', call.request.headers.keys())
275284
276 @responses.activate285 @responses.activate
277 def test_upload_raises_non_404(self):286 def test_upload_check_existing_raises_non_404(self):
278 push_rule = self.build.recipe.push_rules[0]287 push_rule = self.build.recipe.push_rules[0]
279 http_client = RegistryHTTPClient(push_rule)288 http_client = RegistryHTTPClient(push_rule)
280 blobs_url = "{}/blobs/{}".format(289 blobs_url = "{}/blobs/{}".format(
@@ -336,6 +345,124 @@ class TestOCIRegistryClient(OCIConfigHelperMixin, SpyProxyCallsMixin,
336 5, self.client._upload.retry.statistics["attempt_number"])345 5, self.client._upload.retry.statistics["attempt_number"])
337 self.assertEqual(5, self.retry_count)346 self.assertEqual(5, self.retry_count)
338347
348 @responses.activate
349 def test_upload_put_blob_raises_error(self):
350 push_rule = self.build.recipe.push_rules[0]
351 http_client = RegistryHTTPClient(push_rule)
352 blobs_url = "{}/blobs/{}".format(
353 http_client.api_url, "test-digest")
354 uploads_url = "{}/blobs/uploads/".format(http_client.api_url)
355 upload_url = "{}/blobs/uploads/{}".format(
356 http_client.api_url, uuid.uuid4())
357 put_errors = [
358 {
359 "code": "BLOB_UPLOAD_INVALID",
360 "message": "blob upload invalid",
361 "detail": [],
362 },
363 ]
364 responses.add("HEAD", blobs_url, status=404)
365 responses.add("POST", uploads_url, headers={"Location": upload_url})
366 responses.add(
367 "PUT", upload_url, status=400, json={"errors": put_errors})
368 self.assertThat(
369 partial(
370 self.client._upload,
371 "test-digest", push_rule, None, http_client),
372 Raises(MatchesException(
373 BlobUploadFailed,
374 AfterPreprocessing(
375 str,
376 Equals(
377 "Upload of {} for {} failed".format(
378 "test-digest", push_rule.image_name))))))
379
380 @responses.activate
381 def test_upload_put_blob_raises_non_201_success(self):
382 push_rule = self.build.recipe.push_rules[0]
383 http_client = RegistryHTTPClient(push_rule)
384 blobs_url = "{}/blobs/{}".format(
385 http_client.api_url, "test-digest")
386 uploads_url = "{}/blobs/uploads/".format(http_client.api_url)
387 upload_url = "{}/blobs/uploads/{}".format(
388 http_client.api_url, uuid.uuid4())
389 responses.add("HEAD", blobs_url, status=404)
390 responses.add("POST", uploads_url, headers={"Location": upload_url})
391 responses.add("PUT", upload_url, status=200)
392 self.assertThat(
393 partial(
394 self.client._upload,
395 "test-digest", push_rule, None, http_client),
396 Raises(MatchesException(
397 BlobUploadFailed,
398 AfterPreprocessing(
399 str,
400 Equals(
401 "Upload of {} for {} failed".format(
402 "test-digest", push_rule.image_name))))))
403
404 @responses.activate
405 def test_upload_put_manifest_raises_error(self):
406 self._makeFiles()
407 self.useFixture(MockPatch(
408 "lp.oci.model.ociregistryclient.OCIRegistryClient._upload"))
409 self.useFixture(MockPatch(
410 "lp.oci.model.ociregistryclient.OCIRegistryClient._upload_layer"))
411
412 push_rule = self.build.recipe.push_rules[0]
413 responses.add(
414 "GET", "{}/v2/".format(push_rule.registry_url), status=200)
415
416 manifests_url = "{}/v2/{}/manifests/edge".format(
417 push_rule.registry_credentials.url,
418 push_rule.image_name)
419 put_errors = [
420 {
421 "code": "MANIFEST_INVALID",
422 "message": "manifest invalid",
423 "detail": [],
424 },
425 ]
426 responses.add(
427 "PUT", manifests_url, status=400, json={"errors": put_errors})
428
429 self.assertThat(
430 partial(self.client.upload, self.build),
431 Raises(MatchesException(
432 ManifestUploadFailed,
433 AfterPreprocessing(
434 str,
435 Equals(
436 "Failed to upload manifest for {} in {}".format(
437 self.build.recipe.name, self.build.id))))))
438
439 @responses.activate
440 def test_upload_put_manifest_raises_non_201_success(self):
441 self._makeFiles()
442 self.useFixture(MockPatch(
443 "lp.oci.model.ociregistryclient.OCIRegistryClient._upload"))
444 self.useFixture(MockPatch(
445 "lp.oci.model.ociregistryclient.OCIRegistryClient._upload_layer"))
446
447 push_rule = self.build.recipe.push_rules[0]
448 responses.add(
449 "GET", "{}/v2/".format(push_rule.registry_url), status=200)
450
451 manifests_url = "{}/v2/{}/manifests/edge".format(
452 push_rule.registry_credentials.url,
453 push_rule.image_name)
454 responses.add("PUT", manifests_url, status=200)
455
456 self.assertThat(
457 partial(self.client.upload, self.build),
458 Raises(MatchesException(
459 ManifestUploadFailed,
460 AfterPreprocessing(
461 str,
462 Equals(
463 "Failed to upload manifest for {} in {}".format(
464 self.build.recipe.name, self.build.id))))))
465
339466
340class TestRegistryHTTPClient(OCIConfigHelperMixin, SpyProxyCallsMixin,467class TestRegistryHTTPClient(OCIConfigHelperMixin, SpyProxyCallsMixin,
341 TestCaseWithFactory):468 TestCaseWithFactory):

Subscribers

People subscribed via source and target branches

to status/vote changes: