launchpad-reviewers team mailing list archive

[Tom Wardill <tom.wardill@xxxxxxxxxxxxx>]

Tom Wardill has proposed merging ~twom/launchpad:oci-registry-upload-with-credentials into launchpad:master with ~twom/launchpad:oci-registry-upload as a prerequisite.

Commit message:
Add auth handling using OCIRegistryCredentials

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

If registry credentials have been set in the OCIRegistryCredentials and OCIPushRule, we should use them.
Pass them through to urlfetch() in the correct short form for HTTPBasicAuth.
Also allow the credentials to be missing and operate without auth in this case.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~twom/launchpad:oci-registry-upload-with-credentials into launchpad:master.

diff --git a/database/schema/security.cfg b/database/schema/security.cfg
index 55226e1..8911de7 100644
--- a/database/schema/security.cfg
+++ b/database/schema/security.cfg
@@ -2707,7 +2707,7 @@ public.product                          = SELECT
 public.ocirecipe                        = SELECT, UPDATE
 public.ocirecipearch                    = SELECT
 public.ocirecipebuild                   = SELECT, INSERT, UPDATE
-public.ocirecipebuildjob                = SELECT, UPDATE
+public.ocirecipebuildjob                = SELECT, INSERT, UPDATE
 public.ocifile                          = SELECT
 public.ociproject                       = SELECT
 public.ociprojectname                   = SELECT
diff --git a/lib/lp/oci/model/ocirecipebuildjob.py b/lib/lp/oci/model/ocirecipebuildjob.py
index a7abed9..eaa808b 100644
--- a/lib/lp/oci/model/ocirecipebuildjob.py
+++ b/lib/lp/oci/model/ocirecipebuildjob.py
@@ -256,5 +256,6 @@ class OCIRegistryUploadJob(OCIRecipeBuildJobDerived):
                 self.error_detail = getattr(e, "detail", None)
                 raise
         except Exception:
+            import ipdb; ipdb.set_trace()
             transaction.commit()
             raise
diff --git a/lib/lp/oci/model/ociregistryclient.py b/lib/lp/oci/model/ociregistryclient.py
index fa38429..717ce43 100644
--- a/lib/lp/oci/model/ociregistryclient.py
+++ b/lib/lp/oci/model/ociregistryclient.py
@@ -44,7 +44,7 @@ class OCIRegistryClient:
             reference.close()
 
     @classmethod
-    def _upload(cls, digest, push_rule, name, fileobj):
+    def _upload(cls, digest, push_rule, name, fileobj, auth):
         """Upload a blob to the registry, using a given digest.
 
         :param digest: The digest to store the file under.
@@ -59,7 +59,8 @@ class OCIRegistryClient:
             head_response = urlfetch(
                 "{}/v2/{}/blobs/{}".format(
                     push_rule.registry_credentials.url, name, digest),
-                method="HEAD")
+                method="HEAD",
+                auth=auth)
             if head_response.status_code == 200:
                 log.info("{} already found".format(digest))
                 return
@@ -71,7 +72,7 @@ class OCIRegistryClient:
         post_response = urlfetch(
             "{}/v2/{}/blobs/uploads/".format(
                 push_rule.registry_credentials.url, name),
-            method="POST")
+            method="POST", auth=auth)
 
         post_location = post_response.headers["Location"]
         query_parsed = {"digest": digest}
@@ -80,14 +81,15 @@ class OCIRegistryClient:
             post_location,
             params=query_parsed,
             data=fileobj,
-            method="PUT")
+            method="PUT",
+            auth=auth)
 
         if put_response.status_code != 201:
             raise BlobUploadFailed(
                 "Upload of {} for {} failed".format(digest, name))
 
     @classmethod
-    def _upload_layer(cls, digest, push_rule, name, lfa):
+    def _upload_layer(cls, digest, push_rule, name, lfa, auth):
         """Upload a layer blob to the registry.
 
         Uses _upload, but opens the LFA and extracts the necessary files
@@ -105,7 +107,7 @@ class OCIRegistryClient:
                 if tarinfo.name != 'layer.tar':
                     continue
                 fileobj = un_zipped.extractfile(tarinfo)
-                cls._upload(digest, push_rule, name, fileobj)
+                cls._upload(digest, push_rule, name, fileobj, auth)
         finally:
             lfa.close()
 
@@ -205,6 +207,12 @@ class OCIRegistryClient:
         preloaded_data = cls._preloadFiles(build, manifest, digests)
 
         for push_rule in build.recipe.push_rules:
+            # Work out the auth details for the registry
+            auth = push_rule.registry_credentials.getCredentials()
+            if auth.get('username'):
+                auth_tuple = (auth['username'], auth.get('password'))
+            else:
+                auth_tuple = None
             for section in manifest:
                 # Work out names and tags
                 image_name = push_rule.image_name
@@ -217,7 +225,8 @@ class OCIRegistryClient:
                         diff_id,
                         push_rule,
                         image_name,
-                        file_data[diff_id])
+                        file_data[diff_id],
+                        auth_tuple)
                 # The config file is required in different forms, so we can
                 # calculate the sha, work these out and upload
                 config_json = json.dumps(config).encode()
@@ -226,7 +235,8 @@ class OCIRegistryClient:
                     "sha256:{}".format(config_sha),
                     push_rule,
                     image_name,
-                    BytesIO(config_json))
+                    BytesIO(config_json),
+                    auth_tuple)
 
                 # Build the registry manifest from the image manifest
                 # and associated configs
@@ -246,7 +256,8 @@ class OCIRegistryClient:
                             "application/"
                             "vnd.docker.distribution.manifest.v2+json"
                         },
-                    method="PUT")
+                    method="PUT",
+                    auth=auth_tuple)
                 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 5f83e38..9abf063 100644
--- a/lib/lp/oci/tests/test_ociregistryclient.py
+++ b/lib/lp/oci/tests/test_ociregistryclient.py
@@ -49,7 +49,7 @@ class TestOCIRegistryClient(OCIConfigHelperMixin, TestCaseWithFactory):
         }]
         self.config = {"rootfs": {"diff_ids": ["diff_id_1", "diff_id_2"]}}
         self.build = self.factory.makeOCIRecipeBuild()
-        self.factory.makeOCIPushRule(recipe=self.build.recipe)
+        self.push_rule = self.factory.makeOCIPushRule(recipe=self.build.recipe)
         self.client = OCIRegistryClient()
 
     def _makeFiles(self):
@@ -128,6 +128,30 @@ class TestOCIRegistryClient(OCIConfigHelperMixin, TestCaseWithFactory):
                 "application/vnd.docker.distribution.manifest.v2+json")
         }))
 
+    @responses.activate
+    def test_upload_formats_credentials(self):
+        self._makeFiles()
+        _upload_fixture = self.useFixture(MockPatch(
+            "lp.oci.model.ociregistryclient.OCIRegistryClient._upload"))
+        self.useFixture(MockPatch(
+            "lp.oci.model.ociregistryclient.OCIRegistryClient._upload_layer"))
+
+        self.push_rule.registry_credentials.setCredentials({
+            "username": "test-username",
+            "password": "test-password"
+        })
+
+        manifests_url = "{}/v2/{}/manifests/edge".format(
+            self.build.recipe.push_rules[0].registry_credentials.url,
+            self.build.recipe.push_rules[0].image_name
+        )
+        responses.add("PUT", manifests_url, status=201)
+        self.client.upload(self.build)
+
+        self.assertIn(
+            ('test-username', 'test-password'),
+            _upload_fixture.mock.call_args_list[0][0])
+
     def test_preloadFiles(self):
         self._makeFiles()
         files = self.client._preloadFiles(
@@ -186,7 +210,11 @@ class TestOCIRegistryClient(OCIConfigHelperMixin, TestCaseWithFactory):
             "test-digest")
         responses.add("HEAD", blobs_url, status=200)
         self.client._upload(
-            "test-digest", self.build.recipe.push_rules[0], "test-name", None)
+            "test-digest", self.build.recipe.push_rules[0],
+            "test-name", None, None)
+        # There should be no auth headers for these calls
+        for call in responses.calls:
+            self.assertNotIn('Authorization', call.request.headers.keys())
 
     @responses.activate
     def test_upload_raises_non_404(self):
@@ -201,4 +229,21 @@ class TestOCIRegistryClient(OCIConfigHelperMixin, TestCaseWithFactory):
             "test-digest",
             self.build.recipe.push_rules[0],
             "test-name",
+            None,
             None)
+
+    @responses.activate
+    def test_upload_passes_basic_auth(self):
+        blobs_url = "{}/v2/{}/blobs/{}".format(
+            self.build.recipe.push_rules[0].registry_credentials.url,
+            "test-name",
+            "test-digest")
+        responses.add("HEAD", blobs_url, status=200)
+        self.client._upload(
+            "test-digest", self.build.recipe.push_rules[0],
+            "test-name", None, ('user', 'password'))
+
+        for call in responses.calls:
+            self.assertEqual(
+                'Basic dXNlcjpwYXNzd29yZA==',
+                call.request.headers['Authorization'])