launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:build-private-bpb-immediately into launchpad:master.

Commit message:
Dispatch private BPBs immediately

We now use macaroon authentication for source files.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

I haven't yet tested this beyond what's in the test suite.

This is essentially the same as https://code.launchpad.net/~cjwatson/launchpad/build-private-bpb-immediately/+merge/345104, converted to git and rebased on master.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:build-private-bpb-immediately into launchpad:master.

diff --git a/lib/lp/buildmaster/tests/test_builder.py b/lib/lp/buildmaster/tests/test_builder.py
index a97d3c9..3635643 100644
--- a/lib/lp/buildmaster/tests/test_builder.py
+++ b/lib/lp/buildmaster/tests/test_builder.py
@@ -403,13 +403,13 @@ class TestFindBuildCandidatePrivatePPA(TestFindBuildCandidatePPABase):
         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):
diff --git a/lib/lp/soyuz/model/binarypackagebuild.py b/lib/lp/soyuz/model/binarypackagebuild.py
index 748e102..1f87038 100644
--- a/lib/lp/soyuz/model/binarypackagebuild.py
+++ b/lib/lp/soyuz/model/binarypackagebuild.py
@@ -89,10 +89,7 @@ from lp.services.macaroons.interfaces import (
     )
 from lp.services.macaroons.model import MacaroonIssuerBase
 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,
@@ -1212,33 +1209,12 @@ class BinaryPackageBuildSet(SpecificBuildFarmJobSourceMixin):
     @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):
diff --git a/lib/lp/soyuz/model/binarypackagebuildbehaviour.py b/lib/lp/soyuz/model/binarypackagebuildbehaviour.py
index afb1df9..9d0591b 100644
--- a/lib/lp/soyuz/model/binarypackagebuildbehaviour.py
+++ b/lib/lp/soyuz/model/binarypackagebuildbehaviour.py
@@ -10,7 +10,9 @@ __all__ = [
     ]
 
 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,13 +22,12 @@ from lp.buildmaster.model.buildfarmjobbehaviour import (
     BuildFarmJobBehaviourBase,
     )
 from lp.registry.interfaces.pocket import PackagePublishingPocket
-from lp.services.webapp import urlappend
+from lp.services.macaroons.interfaces import IMacaroonIssuer
 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 +64,9 @@ class BinaryPackageBuildBehaviour(BuildFarmJobBehaviourBase):
         # 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 +76,10 @@ class BinaryPackageBuildBehaviour(BuildFarmJobBehaviourBase):
             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
 
     def verifyBuildRequest(self, logger):
diff --git a/lib/lp/soyuz/tests/test_binarypackagebuildbehaviour.py b/lib/lp/soyuz/tests/test_binarypackagebuildbehaviour.py
index 61e4847..be27e00 100644
--- a/lib/lp/soyuz/tests/test_binarypackagebuildbehaviour.py
+++ b/lib/lp/soyuz/tests/test_binarypackagebuildbehaviour.py
@@ -12,8 +12,15 @@ import os
 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 twisted.trial.unittest import TestCase as TrialTestCase
 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.registry.interfaces.sourcepackage import SourcePackageFileType
 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,27 @@ from lp.testing.keyserver import InProcessKeyServerFixture
 from lp.testing.layers import LaunchpadZopelessLayer
 
 
+class BinaryPackageBuildMacaroonVerifies(Matcher):
+    """Matches if a binary-package-build macaroon passes verification."""
+
+    def __init__(self, build, lfa):
+        self.build = build
+        self.lfa = lfa
+
+    def match(self, macaroon_raw):
+        macaroon = Macaroon.deserialize(macaroon_raw)
+        expected_caveat = (
+            "lp.principal.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):
+            return Mismatch("Macaroon does not verify")
+
+
 class TestBinaryBuildPackageBehaviour(TestCaseWithFactory):
     """Tests for the BinaryPackageBuildBehaviour.
 
@@ -98,7 +126,7 @@ class TestBinaryBuildPackageBehaviour(TestCaseWithFactory):
         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 +143,7 @@ class TestBinaryBuildPackageBehaviour(TestCaseWithFactory):
             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 +159,9 @@ class TestBinaryBuildPackageBehaviour(TestCaseWithFactory):
             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,
@@ -151,8 +180,9 @@ class TestBinaryBuildPackageBehaviour(TestCaseWithFactory):
             '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)
 
@@ -236,6 +266,8 @@ class TestBinaryBuildPackageBehaviour(TestCaseWithFactory):
 
     @defer.inlineCallbacks
     def test_private_source_dispatch(self):
+        self.pushConfig(
+            "launchpad", internal_macaroon_secret_key="some-secret")
         archive = self.factory.makeArchive(private=True)
         slave = OkSlave()
         builder = self.factory.makeBuilder()
@@ -246,13 +278,6 @@ class TestBinaryBuildPackageBehaviour(TestCaseWithFactory):
         sprf = 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'),
-               sprf.libraryfile.filename))
         lf = self.factory.makeLibraryFileAlias()
         transaction.commit()
         build.distro_arch_series.addOrUpdateChroot(lf)
@@ -264,7 +289,9 @@ class TestBinaryBuildPackageBehaviour(TestCaseWithFactory):
             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))],
             filemap_names=[sprf.libraryfile.filename])
 
     @defer.inlineCallbacks