launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:filemap-issue-macaroons into launchpad:master.

Commit message:
Issue macaroons for BinaryPackageBuild filemaps

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

This removes the last use of the old buildd secret from build dispatch code, paving the way for it to be removed entirely.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:filemap-issue-macaroons into launchpad:master.

diff --git a/lib/lp/buildmaster/interfaces/buildfarmjobbehaviour.py b/lib/lp/buildmaster/interfaces/buildfarmjobbehaviour.py
index d210d64..651bd05 100644
--- a/lib/lp/buildmaster/interfaces/buildfarmjobbehaviour.py
+++ b/lib/lp/buildmaster/interfaces/buildfarmjobbehaviour.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2019 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Interface for build farm job behaviours."""
@@ -39,7 +39,8 @@ class IBuildFarmJobBehaviour(Interface):
     def determineFilesToSend():
         """Work out which files to send to the builder.
 
-        :return: A dict mapping filenames to dicts as follows::
+        :return: A dict mapping filenames to dicts as follows, or a Deferred
+                resulting in the same::
             'sha1': SHA-1 of file content
             'url': URL from which the builder can fetch content
             'username' (optional): username to authenticate as
diff --git a/lib/lp/buildmaster/model/buildfarmjobbehaviour.py b/lib/lp/buildmaster/model/buildfarmjobbehaviour.py
index e0d5380..0572cda 100644
--- a/lib/lp/buildmaster/model/buildfarmjobbehaviour.py
+++ b/lib/lp/buildmaster/model/buildfarmjobbehaviour.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2020 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Base and idle BuildFarmJobBehaviour classes."""
@@ -106,9 +106,10 @@ class BuildFarmJobBehaviourBase:
     @defer.inlineCallbacks
     def composeBuildRequest(self, logger):
         args = yield self.extraBuildArgs(logger=logger)
+        filemap = yield self.determineFilesToSend()
         defer.returnValue(
             (self.builder_type, self.distro_arch_series, self.pocket,
-             self.determineFilesToSend(), args))
+             filemap, args))
 
     def verifyBuildRequest(self, logger):
         """The default behaviour is a no-op."""
diff --git a/lib/lp/soyuz/model/binarypackagebuildbehaviour.py b/lib/lp/soyuz/model/binarypackagebuildbehaviour.py
index 2876ac0..b34311c 100644
--- a/lib/lp/soyuz/model/binarypackagebuildbehaviour.py
+++ b/lib/lp/soyuz/model/binarypackagebuildbehaviour.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2019 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2021 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."""
@@ -62,6 +62,7 @@ class BinaryPackageBuildBehaviour(BuildFarmJobBehaviourBase):
             distroname, distroseriesname, archname, sourcename, version,
             state))
 
+    @defer.inlineCallbacks
     def determineFilesToSend(self):
         """See `IBuildFarmJobBehaviour`."""
         # Build filemap structure with the files required in this build
@@ -76,18 +77,21 @@ class BinaryPackageBuildBehaviour(BuildFarmJobBehaviourBase):
                     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:
+                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': self.build.archive.buildd_secret}
-        return filemap
+                    'password': macaroon_raw}
+        defer.returnValue(filemap)
 
     def verifyBuildRequest(self, logger):
         """Assert some pre-build checks.
diff --git a/lib/lp/soyuz/tests/test_binarypackagebuildbehaviour.py b/lib/lp/soyuz/tests/test_binarypackagebuildbehaviour.py
index cc27742..0131ffb 100644
--- a/lib/lp/soyuz/tests/test_binarypackagebuildbehaviour.py
+++ b/lib/lp/soyuz/tests/test_binarypackagebuildbehaviour.py
@@ -1,4 +1,4 @@
-# Copyright 2010-2020 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for BinaryPackageBuildBehaviour."""
@@ -13,8 +13,14 @@ import shutil
 import tempfile
 
 from storm.store import Store
-from testtools.matchers import MatchesListwise
-from testtools.twistedsupport import AsynchronousDeferredRunTest
+from testtools.matchers import (
+    Equals,
+    MatchesListwise,
+    )
+from testtools.twistedsupport import (
+    AsynchronousDeferredRunTest,
+    AsynchronousDeferredRunTestForBrokenTwisted,
+    )
 import transaction
 from twisted.internet import defer
 from zope.component import getUtility
@@ -55,9 +61,11 @@ from lp.registry.interfaces.pocket import (
     )
 from lp.registry.interfaces.series import SeriesStatus
 from lp.registry.interfaces.sourcepackage import SourcePackageFileType
+from lp.services.authserver.testing import InProcessAuthServerFixture
 from lp.services.config import config
 from lp.services.librarian.interfaces import ILibraryFileAliasSet
 from lp.services.log.logger import BufferLogger
+from lp.services.macaroons.testing import MacaroonVerifies
 from lp.services.statsd.tests import StatsMixin
 from lp.services.webapp import canonical_url
 from lp.soyuz.adapters.archivedependencies import (
@@ -86,7 +94,8 @@ class TestBinaryBuildPackageBehaviour(StatsMixin, TestCaseWithFactory):
     """
 
     layer = LaunchpadZopelessLayer
-    run_tests_with = AsynchronousDeferredRunTest.make_factory(timeout=30)
+    run_tests_with = AsynchronousDeferredRunTestForBrokenTwisted.make_factory(
+        timeout=30)
 
     def setUp(self):
         super(TestBinaryBuildPackageBehaviour, self).setUp()
@@ -98,10 +107,10 @@ class TestBinaryBuildPackageBehaviour(StatsMixin, TestCaseWithFactory):
                                   chroot, archive, archive_purpose,
                                   component=None, extra_uploads=None,
                                   filemap_names=None):
-        expected = yield self.makeExpectedInteraction(
+        matcher = yield self.makeExpectedInteraction(
             builder, build, behaviour, chroot, archive, archive_purpose,
             component, extra_uploads, filemap_names)
-        self.assertEqual(expected, call_log)
+        self.assertThat(call_log, matcher)
 
     @defer.inlineCallbacks
     def makeExpectedInteraction(self, builder, build, behaviour, chroot,
@@ -135,7 +144,10 @@ class TestBinaryBuildPackageBehaviour(StatsMixin, TestCaseWithFactory):
             extra_uploads = []
 
         upload_logs = [
-            ('ensurepresent',) + upload
+            MatchesListwise(
+                [Equals('ensurepresent')] +
+                [item if hasattr(item, 'match') else Equals(item)
+                 for item in upload])
             for upload in [(chroot.http_url, '', '')] + extra_uploads]
 
         extra_args = {
@@ -157,7 +169,9 @@ class TestBinaryBuildPackageBehaviour(StatsMixin, TestCaseWithFactory):
         build_log = [
             ('build', build.build_cookie, 'binarypackage',
              chroot.content.sha1, filemap_names, extra_args)]
-        result = upload_logs + build_log
+        result = MatchesListwise([
+            item if hasattr(item, 'match') else Equals(item)
+            for item in upload_logs + build_log])
         defer.returnValue(result)
 
     @defer.inlineCallbacks
@@ -253,6 +267,9 @@ class TestBinaryBuildPackageBehaviour(StatsMixin, TestCaseWithFactory):
 
     @defer.inlineCallbacks
     def test_private_source_dispatch(self):
+        self.useFixture(InProcessAuthServerFixture())
+        self.pushConfig(
+            "launchpad", internal_macaroon_secret_key="some-secret")
         archive = self.factory.makeArchive(private=True)
         slave = OkSlave()
         builder = self.factory.makeBuilder()
@@ -282,7 +299,10 @@ class TestBinaryBuildPackageBehaviour(StatsMixin, TestCaseWithFactory):
         yield self.assertExpectedInteraction(
             slave.call_log, builder, build, behaviour, lf, archive,
             ArchivePurpose.PPA,
-            extra_uploads=[(sprf_url, 'buildd', 'sekrit')],
+            extra_uploads=[
+                (Equals(sprf_url),
+                 Equals('buildd'),
+                 MacaroonVerifies('binary-package-build', archive))],
             filemap_names=[sprf.libraryfile.filename])
 
     @defer.inlineCallbacks