launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/build-private-bpb-immediately into lp:launchpad with lp:~cjwatson/launchpad/librarian-accept-macaroon as a prerequisite.

Commit message:
Dispatch private BPBs immediately, using macaroon auth for source files.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/build-private-bpb-immediately/+merge/345104

The prerequisite branch must be deployed to the librarian before landing this.

I haven't yet tested this beyond what's in the test suite.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/build-private-bpb-immediately into lp:launchpad.

=== modified file 'lib/lp/buildmaster/tests/test_builder.py'
--- lib/lp/buildmaster/tests/test_builder.py	2018-01-10 10:45:24 +0000
+++ lib/lp/buildmaster/tests/test_builder.py	2018-05-04 17:00:50 +0000
@@ -401,13 +401,13 @@
         build = getUtility(IBinaryPackageBuildSet).getByQueueEntry(next_job)
         self.assertEqual('joesppa', build.archive.name)
 
-        # If the source for the build is still pending, it won't be
-        # dispatched because the builder has to fetch the source files
-        # from the (password protected) repo area, not the librarian.
+        # If the source for the build is still pending, it will still be
+        # dispatched: the builder will use macaroon authentication to fetch
+        # the source files from the librarian.
         pub = build.current_source_publication
         pub.status = PackagePublishingStatus.PENDING
         candidate = removeSecurityProxy(self.builder4)._findBuildCandidate()
-        self.assertNotEqual(next_job.id, candidate.id)
+        self.assertEqual(next_job.id, candidate.id)
 
 
 class TestFindBuildCandidateDistroArchive(TestFindBuildCandidateBase):

=== modified file 'lib/lp/soyuz/model/binarypackagebuild.py'
--- lib/lp/soyuz/model/binarypackagebuild.py	2018-05-04 17:00:50 +0000
+++ lib/lp/soyuz/model/binarypackagebuild.py	2018-05-04 17:00:50 +0000
@@ -87,10 +87,7 @@
     )
 from lp.services.macaroons.interfaces import IMacaroonIssuer
 from lp.soyuz.adapters.buildarch import determine_architectures_to_build
-from lp.soyuz.enums import (
-    ArchivePurpose,
-    PackagePublishingStatus,
-    )
+from lp.soyuz.enums import ArchivePurpose
 from lp.soyuz.interfaces.archive import (
     InvalidExternalDependencies,
     validate_external_dependencies,
@@ -1210,33 +1207,12 @@
     @staticmethod
     def addCandidateSelectionCriteria(processor, virtualized):
         """See `ISpecificBuildFarmJobSource`."""
-        private_statuses = (
-            PackagePublishingStatus.PUBLISHED,
-            PackagePublishingStatus.SUPERSEDED,
-            PackagePublishingStatus.DELETED,
-            )
         return """
-            SELECT TRUE FROM Archive, BinaryPackageBuild, DistroArchSeries
+            SELECT TRUE FROM BinaryPackageBuild
             WHERE
             BinaryPackageBuild.build_farm_job = BuildQueue.build_farm_job AND
-            BinaryPackageBuild.distro_arch_series =
-                DistroArchSeries.id AND
-            BinaryPackageBuild.archive = Archive.id AND
-            ((Archive.private IS TRUE AND
-              EXISTS (
-                  SELECT SourcePackagePublishingHistory.id
-                  FROM SourcePackagePublishingHistory
-                  WHERE
-                      SourcePackagePublishingHistory.distroseries =
-                         DistroArchSeries.distroseries AND
-                      SourcePackagePublishingHistory.sourcepackagerelease =
-                         BinaryPackageBuild.source_package_release AND
-                      SourcePackagePublishingHistory.archive = Archive.id AND
-                      SourcePackagePublishingHistory.status IN %s))
-              OR
-              archive.private IS FALSE) AND
             BinaryPackageBuild.status = %s
-        """ % sqlvalues(private_statuses, BuildStatus.NEEDSBUILD)
+        """ % sqlvalues(BuildStatus.NEEDSBUILD)
 
     @staticmethod
     def postprocessCandidate(job, logger):

=== modified file 'lib/lp/soyuz/model/binarypackagebuildbehaviour.py'
--- lib/lp/soyuz/model/binarypackagebuildbehaviour.py	2018-03-01 17:36:31 +0000
+++ lib/lp/soyuz/model/binarypackagebuildbehaviour.py	2018-05-04 17:00:50 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Builder behaviour for binary package builds."""
@@ -10,7 +10,9 @@
     ]
 
 from twisted.internet import defer
+from zope.component import getUtility
 from zope.interface import implementer
+from zope.security.proxy import removeSecurityProxy
 
 from lp.buildmaster.interfaces.builder import CannotBuild
 from lp.buildmaster.interfaces.buildfarmjobbehaviour import (
@@ -20,16 +22,13 @@
     BuildFarmJobBehaviourBase,
     )
 from lp.registry.interfaces.pocket import PackagePublishingPocket
-from lp.services.webapp import (
-    canonical_url,
-    urlappend,
-    )
+from lp.services.macaroons.interfaces import IMacaroonIssuer
+from lp.services.webapp import canonical_url
 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)
@@ -63,14 +62,9 @@
         # Build filemap structure with the files required in this build
         # and send them to the slave.
         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))
+            issuer = getUtility(IMacaroonIssuer, 'binary-package-build')
+            password = removeSecurityProxy(issuer).issueMacaroon(
+                self.build).serialize()
         filemap = {}
         for source_file in self.build.source_package_release.files:
             lfa = source_file.libraryfile
@@ -80,9 +74,10 @@
             else:
                 filemap[lfa.filename] = {
                     'sha1': lfa.content.sha1,
-                    'url': urlappend(pool_url, lfa.filename),
-                    'username': 'buildd',
-                    'password': self.build.archive.buildd_secret}
+                    'url': lfa.https_url,
+                    'username': '',
+                    'password': password,
+                    }
         return filemap
 
     @defer.inlineCallbacks

=== modified file 'lib/lp/soyuz/tests/test_binarypackagebuildbehaviour.py'
--- lib/lp/soyuz/tests/test_binarypackagebuildbehaviour.py	2018-03-01 17:36:31 +0000
+++ lib/lp/soyuz/tests/test_binarypackagebuildbehaviour.py	2018-05-04 17:00:50 +0000
@@ -12,8 +12,15 @@
 import shutil
 import tempfile
 
+from pymacaroons import Macaroon
 from storm.store import Store
-from testtools.matchers import MatchesListwise
+from testtools.matchers import (
+    Contains,
+    Equals,
+    Matcher,
+    MatchesListwise,
+    Mismatch,
+    )
 from testtools.twistedsupport import AsynchronousDeferredRunTest
 import transaction
 from twisted.internet import defer
@@ -21,7 +28,6 @@
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
-from lp.archivepublisher.diskpool import poolify
 from lp.archivepublisher.interfaces.archivesigningkey import (
     IArchiveSigningKey,
     )
@@ -58,6 +64,7 @@
 from lp.services.config import config
 from lp.services.librarian.interfaces import ILibraryFileAliasSet
 from lp.services.log.logger import BufferLogger
+from lp.services.macaroons.interfaces import IMacaroonIssuer
 from lp.services.webapp import canonical_url
 from lp.soyuz.adapters.archivedependencies import (
     get_sources_list_for_building,
@@ -74,6 +81,26 @@
 from lp.testing.layers import LaunchpadZopelessLayer
 
 
+class BinaryPackageBuildMacaroonVerifies(Matcher):
+    """Matches if a binary-package-build macaroon passes verification."""
+
+    def __init__(self, build, lfa_id):
+        self.build = build
+        self.lfa_id = lfa_id
+
+    def match(self, macaroon_raw):
+        macaroon = Macaroon.deserialize(macaroon_raw)
+        expected_caveat = (
+            "lp.binary-package-build %s" % removeSecurityProxy(self.build).id)
+        mismatch = Contains(expected_caveat).match(
+            [caveat.caveat_id for caveat in macaroon.caveats])
+        if mismatch is not None:
+            return mismatch
+        issuer = getUtility(IMacaroonIssuer, "binary-package-build")
+        if not issuer.verifyMacaroon(macaroon, self.lfa_id):
+            return Mismatch("Macaroon does not verify")
+
+
 class TestBinaryBuildPackageBehaviour(TestCaseWithFactory):
     """Tests for the BinaryPackageBuildBehaviour.
 
@@ -98,7 +125,7 @@
         expected = yield self.makeExpectedInteraction(
             builder, build, chroot, archive, archive_purpose, component,
             extra_uploads, filemap_names)
-        self.assertEqual(expected, call_log)
+        self.assertThat(call_log, MatchesListwise(expected))
 
     @defer.inlineCallbacks
     def makeExpectedInteraction(self, builder, build, chroot, archive,
@@ -115,7 +142,7 @@
             builder. We specify this separately from the archive because
             sometimes the behaviour object has to give a different purpose
             in order to trick the slave into building correctly.
-        :return: A list of the calls we expect to be made.
+        :return: A list of matchers for the calls we expect to be made.
         """
         das = build.distro_arch_series
         ds_name = das.distroseries.name
@@ -131,8 +158,9 @@
             extra_uploads = []
 
         upload_logs = [
-            ('ensurepresent',) + upload
-            for upload in [(chroot.http_url, '', '')] + extra_uploads]
+            MatchesListwise((Equals('ensurepresent'),) + upload)
+            for upload in [(Equals(chroot.http_url), Equals(''), Equals(''))] +
+                          extra_uploads]
 
         extra_args = {
             'arch_indep': arch_indep,
@@ -149,8 +177,9 @@
             'trusted_keys': trusted_keys,
             }
         build_log = [
-            ('build', build.build_cookie, 'binarypackage',
-             chroot.content.sha1, filemap_names, extra_args)]
+            MatchesListwise([Equals(arg) for arg in (
+                'build', build.build_cookie, 'binarypackage',
+                chroot.content.sha1, filemap_names, extra_args)])]
         result = upload_logs + build_log
         defer.returnValue(result)
 
@@ -244,13 +273,6 @@
         sprf = build.source_package_release.addFile(
             self.factory.makeLibraryFileAlias(db_only=True),
             filetype=SourcePackageFileType.ORIG_TARBALL)
-        sprf_url = (
-            'http://private-ppa.launchpad.dev/%s/%s/ubuntu/pool/%s/%s'
-            % (archive.owner.name, archive.name,
-               poolify(
-                   build.source_package_release.sourcepackagename.name,
-                   'main'),
-               sprf.libraryfile.filename))
         lf = self.factory.makeLibraryFileAlias()
         transaction.commit()
         build.distro_arch_series.addOrUpdateChroot(lf)
@@ -262,7 +284,10 @@
             interactor.getBuildBehaviour(bq, builder, slave), BufferLogger())
         yield self.assertExpectedInteraction(
             slave.call_log, builder, build, lf, archive, ArchivePurpose.PPA,
-            extra_uploads=[(sprf_url, 'buildd', 'sekrit')],
+            extra_uploads=[(
+                Equals(sprf.libraryfile.https_url), Equals(''),
+                BinaryPackageBuildMacaroonVerifies(
+                    build, sprf.libraryfile.id))],
             filemap_names=[sprf.libraryfile.filename])
 
     @defer.inlineCallbacks