launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:oci-registry-client-content-length into launchpad:master.

Commit message:
Send Content-Length when uploading OCI blobs to a registry

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/399526

In Python 3.5, `tarfile.ExFileObject` has a `fileno` method that raises `AttributeError`, which is enough to confuse `requests` into thinking that it needs to do a chunked upload, which in turn causes `urllib3` not to use the configured proxy.  Explicitly passing a Content-Length header is polite anyway, and works around this misbehaviour.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:oci-registry-client-content-length into launchpad:master.

diff --git a/lib/lp/oci/model/ociregistryclient.py b/lib/lp/oci/model/ociregistryclient.py
index 3ba3348..2ffe052 100644
--- a/lib/lp/oci/model/ociregistryclient.py
+++ b/lib/lp/oci/model/ociregistryclient.py
@@ -104,12 +104,13 @@ class OCIRegistryClient:
         before=before_log(log, logging.INFO),
         retry=retry_if_exception_type(ConnectionError),
         stop=stop_after_attempt(5))
-    def _upload(cls, digest, push_rule, fileobj, http_client):
+    def _upload(cls, digest, push_rule, fileobj, length, http_client):
         """Upload a blob to the registry, using a given digest.
 
         :param digest: The digest to store the file under.
         :param push_rule: `OCIPushRule` to use for the URL and credentials.
         :param fileobj: An object that looks like a buffer.
+        :param length: The length of the blob in bytes.
 
         :raises BlobUploadFailed: if the registry does not accept the blob.
         """
@@ -137,6 +138,7 @@ class OCIRegistryClient:
                 post_location,
                 params=query_parsed,
                 data=fileobj,
+                headers={"Content-Length": str(length)},
                 method="PUT")
         except HTTPError as http_error:
             put_response = http_error.response
@@ -160,13 +162,18 @@ class OCIRegistryClient:
         """
         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)
-                cls._upload(digest, push_rule, fileobj, http_client)
-                return tarinfo.size
+            with tarfile.open(fileobj=lfa, mode='r|gz') as un_zipped:
+                for tarinfo in un_zipped:
+                    if tarinfo.name != 'layer.tar':
+                        continue
+                    fileobj = un_zipped.extractfile(tarinfo)
+                    try:
+                        cls._upload(
+                            digest, push_rule, fileobj, tarinfo.size,
+                            http_client)
+                    finally:
+                        fileobj.close()
+                    return tarinfo.size
         finally:
             lfa.close()
 
@@ -328,6 +335,7 @@ class OCIRegistryClient:
                 "sha256:{}".format(config_sha),
                 push_rule,
                 BytesIO(config_json),
+                len(config_json),
                 http_client)
 
             # Build the registry manifest from the image manifest
diff --git a/lib/lp/oci/tests/test_ociregistryclient.py b/lib/lp/oci/tests/test_ociregistryclient.py
index dcf71a8..3e0d90f 100644
--- a/lib/lp/oci/tests/test_ociregistryclient.py
+++ b/lib/lp/oci/tests/test_ociregistryclient.py
@@ -26,6 +26,7 @@ import responses
 from tenacity import RetryError
 from testtools.matchers import (
     AfterPreprocessing,
+    ContainsDict,
     Equals,
     Is,
     MatchesAll,
@@ -67,6 +68,7 @@ from lp.oci.tests.helpers import OCIConfigHelperMixin
 from lp.registry.interfaces.series import SeriesStatus
 from lp.services.compat import mock
 from lp.services.features.testing import FeatureFixture
+from lp.services.tarfile_helpers import LaunchpadWriteTarFile
 from lp.testing import (
     admin_logged_in,
     person_logged_in,
@@ -87,9 +89,11 @@ class SpyProxyCallsMixin:
             self.proxy_call_count += 1
             return proxy_urlfetch(*args, **kwargs)
 
-        self.useFixture(MockPatch(
+        proxy_mock = self.useFixture(MockPatch(
             'lp.oci.model.ociregistryclient.proxy_urlfetch',
-            side_effect=count_proxy_call_count))
+            side_effect=count_proxy_call_count)).mock
+        # Avoid reference cycles with file objects passed to urlfetch.
+        self.addCleanup(proxy_mock.reset_mock)
 
 
 class TestOCIRegistryClient(OCIConfigHelperMixin, SpyProxyCallsMixin,
@@ -459,7 +463,7 @@ class TestOCIRegistryClient(OCIConfigHelperMixin, SpyProxyCallsMixin,
         push_rule = self.build.recipe.push_rules[0]
         push_rule.registry_credentials.setCredentials({})
         self.client._upload(
-            "test-digest", push_rule, None, http_client)
+            "test-digest", push_rule, None, 0, http_client)
 
         self.assertEqual(len(responses.calls), self.proxy_call_count)
         # There should be no auth headers for these calls
@@ -481,6 +485,7 @@ class TestOCIRegistryClient(OCIConfigHelperMixin, SpyProxyCallsMixin,
             "test-digest",
             push_rule,
             None,
+            0,
             http_client)
 
     @responses.activate
@@ -493,7 +498,7 @@ class TestOCIRegistryClient(OCIConfigHelperMixin, SpyProxyCallsMixin,
         push_rule.registry_credentials.setCredentials({
             "username": "user", "password": "password"})
         self.client._upload(
-            "test-digest", push_rule, None,
+            "test-digest", push_rule, None, 0,
             http_client)
 
         self.assertEqual(len(responses.calls), self.proxy_call_count)
@@ -521,7 +526,7 @@ class TestOCIRegistryClient(OCIConfigHelperMixin, SpyProxyCallsMixin,
             push_rule = self.build.recipe.push_rules[0]
             self.client._upload(
                 "test-digest", push_rule,
-                None, RegistryHTTPClient(push_rule))
+                None, 0, RegistryHTTPClient(push_rule))
         except RetryError:
             pass
         # Check that tenacity and our counting agree
@@ -552,7 +557,7 @@ class TestOCIRegistryClient(OCIConfigHelperMixin, SpyProxyCallsMixin,
         self.assertThat(
             partial(
                 self.client._upload,
-                "test-digest", push_rule, None, http_client),
+                "test-digest", push_rule, None, 0, http_client),
             Raises(MatchesException(
                 BlobUploadFailed,
                 MatchesAll(
@@ -578,7 +583,7 @@ class TestOCIRegistryClient(OCIConfigHelperMixin, SpyProxyCallsMixin,
         self.assertThat(
             partial(
                 self.client._upload,
-                "test-digest", push_rule, None, http_client),
+                "test-digest", push_rule, None, 0, http_client),
             Raises(MatchesException(
                 BlobUploadFailed,
                 MatchesAll(
@@ -652,6 +657,30 @@ class TestOCIRegistryClient(OCIConfigHelperMixin, SpyProxyCallsMixin,
                     MatchesStructure(errors=Is(None))))))
 
     @responses.activate
+    def test_upload_layer_put_blob_sends_content_length(self):
+        lfa = self.factory.makeLibraryFileAlias(
+            content=LaunchpadWriteTarFile.files_to_bytes(
+                {"layer.tar": b"test layer"}))
+        transaction.commit()
+        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=201)
+        self.client._upload_layer("test-digest", push_rule, lfa, http_client)
+        self.assertThat(responses.calls[2].request, MatchesStructure(
+            method=Equals("PUT"),
+            headers=ContainsDict({
+                "Content-Length": Equals(str(len(b"test layer"))),
+                }),
+            ))
+
+    @responses.activate
     def test_multi_arch_manifest_upload_skips_superseded_builds(self):
         recipe = self.build.recipe
         processor = self.build.processor