launchpad-reviewers team mailing list archive

[Colin Watson <mp+429703@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:bpb-librarian-auth into launchpad:master.

Commit message:
Fetch files for private BPBs from librarian

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

This will allow us to lift the requirement for the source for private binary package builds to be published before we can dispatch them.  We were already using macaroon authentication for private source files due to the `SnapBase` work last year; this just switches from having the private PPA server do the authorization to having the librarian do it.

We now always fetch files for binary package builds using HTTPS, even for public builds, which seems like a better idea in 2022.

This is roughly half of https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/373741, but rebased on master and with some more precise tests for the behaviour of public builds.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:bpb-librarian-auth into launchpad:master.

diff --git a/lib/lp/soyuz/model/binarypackagebuildbehaviour.py b/lib/lp/soyuz/model/binarypackagebuildbehaviour.py
index 79bb712..9cef29a 100644
--- a/lib/lp/soyuz/model/binarypackagebuildbehaviour.py
+++ b/lib/lp/soyuz/model/binarypackagebuildbehaviour.py
@@ -22,13 +22,11 @@ from lp.buildmaster.model.buildfarmjobbehaviour import (
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.services.config import config
 from lp.services.twistedsupport import cancel_on_timeout
-from lp.services.webapp import urlappend
 from lp.soyuz.adapters.archivedependencies import (
     get_primary_current_component,
     get_sources_list_for_building,
 )
 from lp.soyuz.enums import ArchivePurpose
-from lp.soyuz.model.publishing import makePoolPath
 
 
 @implementer(IBuildFarmJobBehaviour)
@@ -70,35 +68,20 @@ class BinaryPackageBuildBehaviour(BuildFarmJobBehaviourBase):
         """See `IBuildFarmJobBehaviour`."""
         # Build filemap structure with the files required in this build
         # and send them to the worker.
-        if self.build.archive.private:
-            # Builds in private archive may have restricted files that
-            # we can't obtain from the public librarian. Prepare a pool
-            # URL from which to fetch them.
-            pool_url = urlappend(
-                self.build.archive.archive_url,
-                makePoolPath(
-                    self.build.source_package_release.sourcepackagename.name,
-                    self.build.current_component.name,
-                ),
-            )
         filemap = OrderedDict()
         macaroon_raw = None
         for source_file in self.build.source_package_release.files:
             lfa = source_file.libraryfile
-            if not self.build.archive.private:
-                filemap[lfa.filename] = {
-                    "sha1": lfa.content.sha1,
-                    "url": lfa.http_url,
-                }
-            else:
+            filemap[lfa.filename] = {
+                "sha1": lfa.content.sha1,
+                "url": lfa.https_url,
+            }
+            if self.build.archive.private:
                 if macaroon_raw is None:
                     macaroon_raw = yield self.issueMacaroon()
-                filemap[lfa.filename] = {
-                    "sha1": lfa.content.sha1,
-                    "url": urlappend(pool_url, lfa.filename),
-                    "username": "buildd",
-                    "password": macaroon_raw,
-                }
+                filemap[lfa.filename].update(
+                    username="", password=macaroon_raw
+                )
         return filemap
 
     def verifyBuildRequest(self, logger):
diff --git a/lib/lp/soyuz/tests/test_binarypackagebuildbehaviour.py b/lib/lp/soyuz/tests/test_binarypackagebuildbehaviour.py
index 2d1a99b..f3625ec 100644
--- a/lib/lp/soyuz/tests/test_binarypackagebuildbehaviour.py
+++ b/lib/lp/soyuz/tests/test_binarypackagebuildbehaviour.py
@@ -18,7 +18,6 @@ from twisted.internet import defer
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
-from lp.archivepublisher.diskpool import poolify
 from lp.archivepublisher.interfaces.archivegpgsigningkey import (
     IArchiveGPGSigningKey,
 )
@@ -92,8 +91,6 @@ class TestBinaryBuildPackageBehaviour(StatsMixin, TestCaseWithFactory):
         archive,
         archive_purpose,
         component=None,
-        extra_uploads=None,
-        filemap_names=None,
     ):
         matcher = yield self.makeExpectedInteraction(
             builder,
@@ -103,8 +100,6 @@ class TestBinaryBuildPackageBehaviour(StatsMixin, TestCaseWithFactory):
             archive,
             archive_purpose,
             component,
-            extra_uploads,
-            filemap_names,
         )
         self.assertThat(call_log, matcher)
 
@@ -118,8 +113,6 @@ class TestBinaryBuildPackageBehaviour(StatsMixin, TestCaseWithFactory):
         archive,
         archive_purpose,
         component=None,
-        extra_uploads=None,
-        filemap_names=None,
     ):
         """Build the log of calls that we expect to be made to the worker.
 
@@ -144,11 +137,17 @@ class TestBinaryBuildPackageBehaviour(StatsMixin, TestCaseWithFactory):
         arch_indep = das.isNominatedArchIndep
         if component is None:
             component = build.current_component.name
-        if filemap_names is None:
-            filemap_names = []
-        if extra_uploads is None:
-            extra_uploads = []
+        files = build.source_package_release.files
 
+        uploads = [(chroot.http_url, "", "")]
+        for sprf in files:
+            if build.archive.private:
+                password = MacaroonVerifies(
+                    "binary-package-build", build.archive
+                )
+            else:
+                password = ""
+            uploads.append((sprf.libraryfile.https_url, "", password))
         upload_logs = [
             MatchesListwise(
                 [Equals("ensurepresent")]
@@ -157,7 +156,7 @@ class TestBinaryBuildPackageBehaviour(StatsMixin, TestCaseWithFactory):
                     for item in upload
                 ]
             )
-            for upload in [(chroot.http_url, "", "")] + extra_uploads
+            for upload in uploads
         ]
 
         extra_args = {
@@ -182,7 +181,7 @@ class TestBinaryBuildPackageBehaviour(StatsMixin, TestCaseWithFactory):
                 build.build_cookie,
                 "binarypackage",
                 chroot.content.sha1,
-                filemap_names,
+                [sprf.libraryfile.filename for sprf in files],
                 extra_args,
             )
         ]
@@ -208,6 +207,10 @@ class TestBinaryBuildPackageBehaviour(StatsMixin, TestCaseWithFactory):
         build = self.factory.makeBinaryPackageBuild(
             builder=builder, archive=archive
         )
+        build.source_package_release.addFile(
+            self.factory.makeLibraryFileAlias(db_only=True),
+            filetype=SourcePackageFileType.ORIG_TARBALL,
+        )
         lf = self.factory.makeLibraryFileAlias(db_only=True)
         build.distro_arch_series.addOrUpdateChroot(lf)
         bq = build.queueBuild()
@@ -248,6 +251,10 @@ class TestBinaryBuildPackageBehaviour(StatsMixin, TestCaseWithFactory):
         build = self.factory.makeBinaryPackageBuild(
             builder=builder, archive=archive
         )
+        build.source_package_release.addFile(
+            self.factory.makeLibraryFileAlias(db_only=True),
+            filetype=SourcePackageFileType.ORIG_TARBALL,
+        )
         self.factory.makeSourcePackagePublishingHistory(
             distroseries=build.distro_series,
             archive=archive.distribution.main_archive,
@@ -284,6 +291,10 @@ class TestBinaryBuildPackageBehaviour(StatsMixin, TestCaseWithFactory):
         build = self.factory.makeBinaryPackageBuild(
             builder=builder, archive=archive
         )
+        build.source_package_release.addFile(
+            self.factory.makeLibraryFileAlias(db_only=True),
+            filetype=SourcePackageFileType.ORIG_TARBALL,
+        )
         lf = self.factory.makeLibraryFileAlias(db_only=True)
         build.distro_arch_series.addOrUpdateChroot(lf)
         bq = build.queueBuild()
@@ -325,21 +336,10 @@ class TestBinaryBuildPackageBehaviour(StatsMixin, TestCaseWithFactory):
         build = self.factory.makeBinaryPackageBuild(
             builder=builder, archive=archive
         )
-        sprf = build.source_package_release.addFile(
+        build.source_package_release.addFile(
             self.factory.makeLibraryFileAlias(db_only=True),
             filetype=SourcePackageFileType.ORIG_TARBALL,
         )
-        sprf_url = (
-            "http://private-ppa.launchpad.test/%s/%s/ubuntu/pool/%s/%s";
-            % (
-                archive.owner.name,
-                archive.name,
-                poolify(
-                    build.source_package_release.sourcepackagename.name, "main"
-                ).as_posix(),
-                sprf.libraryfile.filename,
-            )
-        )
         lf = self.factory.makeLibraryFileAlias(db_only=True)
         build.distro_arch_series.addOrUpdateChroot(lf)
         bq = build.queueBuild()
@@ -357,14 +357,6 @@ class TestBinaryBuildPackageBehaviour(StatsMixin, TestCaseWithFactory):
             lf,
             archive,
             ArchivePurpose.PPA,
-            extra_uploads=[
-                (
-                    Equals(sprf_url),
-                    Equals("buildd"),
-                    MacaroonVerifies("binary-package-build", archive),
-                )
-            ],
-            filemap_names=[sprf.libraryfile.filename],
         )
 
     @defer.inlineCallbacks
@@ -379,6 +371,10 @@ class TestBinaryBuildPackageBehaviour(StatsMixin, TestCaseWithFactory):
         build = self.factory.makeBinaryPackageBuild(
             builder=builder, archive=archive
         )
+        build.source_package_release.addFile(
+            self.factory.makeLibraryFileAlias(db_only=True),
+            filetype=SourcePackageFileType.ORIG_TARBALL,
+        )
         lf = self.factory.makeLibraryFileAlias(db_only=True)
         build.distro_arch_series.addOrUpdateChroot(lf)
         bq = build.queueBuild()