launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/snap-check-proxy-config into lp:launchpad.

Commit message:
Make SnapBuildBehaviour._requestProxyToken explicitly check whether its configuration is present.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/snap-check-proxy-config/+merge/327975

This is much less annoying than having to track down a failed build by stracing the proxy and base64-decoding the Authorization header sent by buildd-manager.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/snap-check-proxy-config into lp:launchpad.

=== modified file 'lib/lp/snappy/model/snapbuildbehaviour.py'
--- lib/lp/snappy/model/snapbuildbehaviour.py	2017-06-14 13:29:12 +0000
+++ lib/lp/snappy/model/snapbuildbehaviour.py	2017-07-24 15:58:37 +0000
@@ -128,8 +128,17 @@
     @defer.inlineCallbacks
     def _requestProxyToken(self):
         admin_username = config.snappy.builder_proxy_auth_api_admin_username
+        if not admin_username:
+            raise CannotBuild(
+                "builder_proxy_auth_api_admin_username is not configured.")
         secret = config.snappy.builder_proxy_auth_api_admin_secret
+        if not secret:
+            raise CannotBuild(
+                "builder_proxy_auth_api_admin_secret is not configured.")
         url = config.snappy.builder_proxy_auth_api_endpoint
+        if not secret:
+            raise CannotBuild(
+                "builder_proxy_auth_api_endpoint is not configured.")
         timestamp = int(time.time())
         proxy_username = '{build_id}-{timestamp}'.format(
             build_id=self.build.build_cookie,

=== modified file 'lib/lp/snappy/tests/test_snapbuildbehaviour.py'
--- lib/lp/snappy/tests/test_snapbuildbehaviour.py	2017-06-14 02:44:33 +0000
+++ lib/lp/snappy/tests/test_snapbuildbehaviour.py	2017-07-24 15:58:37 +0000
@@ -5,22 +5,25 @@
 
 __metaclass__ = type
 
+import base64
 from datetime import datetime
+import json
 import os.path
 from textwrap import dedent
 import uuid
 
 import fixtures
-from mock import (
-    Mock,
-    patch,
-    )
+from mock import patch
 from pymacaroons import Macaroon
 from testtools import ExpectedException
 from testtools.deferredruntest import AsynchronousDeferredRunTest
 from testtools.matchers import (
+    AfterPreprocessing,
+    Equals,
     IsInstance,
+    MatchesDict,
     MatchesListwise,
+    StartsWith,
     )
 import transaction
 from twisted.internet import defer
@@ -59,6 +62,7 @@
 from lp.soyuz.interfaces.archive import ArchiveDisabled
 from lp.soyuz.tests.soyuz import Base64KeyMatches
 from lp.testing import TestCaseWithFactory
+from lp.testing.fakemethod import FakeMethod
 from lp.testing.gpgkeys import gpgkeysdir
 from lp.testing.keyserver import InProcessKeyServerFixture
 from lp.testing.layers import LaunchpadZopelessLayer
@@ -201,16 +205,16 @@
         self.revocation_endpoint = "{endpoint}/{username}".format(
             endpoint=config.snappy.builder_proxy_auth_api_endpoint,
             username=build_username)
-        self.patcher = patch.object(
-            SnapBuildBehaviour, '_requestProxyToken',
-            Mock(return_value=self.mockRequestProxyToken())).start()
-
-    def tearDown(self):
-        super(TestAsyncSnapBuildBehaviour, self).tearDown()
-        self.patcher.stop()
-
-    def mockRequestProxyToken(self):
-        return defer.succeed(self.token)
+        self.api_admin_secret = "admin-secret"
+        self.pushConfig(
+            "snappy",
+            builder_proxy_auth_api_admin_secret=self.api_admin_secret)
+        self.mock_proxy_api = FakeMethod(
+            result=defer.succeed(json.dumps(self.token)))
+        patcher = patch(
+            "lp.snappy.model.snapbuildbehaviour.getPage", self.mock_proxy_api)
+        patcher.start()
+        self.addCleanup(patcher.stop)
 
     @defer.inlineCallbacks
     def test_composeBuildRequest(self):
@@ -222,6 +226,40 @@
         self.assertThat(build_request[3], IsInstance(dict))
 
     @defer.inlineCallbacks
+    def test_requestProxyToken_unconfigured(self):
+        self.pushConfig("snappy", builder_proxy_auth_api_admin_secret=None)
+        branch = self.factory.makeBranch()
+        job = self.makeJob(branch=branch)
+        expected_exception_msg = (
+            "builder_proxy_auth_api_admin_secret is not configured.")
+        with ExpectedException(CannotBuild, expected_exception_msg):
+            yield job._extraBuildArgs()
+
+    @defer.inlineCallbacks
+    def test_requestProxyToken(self):
+        branch = self.factory.makeBranch()
+        job = self.makeJob(branch=branch)
+        yield job._extraBuildArgs()
+        self.assertThat(self.mock_proxy_api.calls, MatchesListwise([
+            MatchesListwise([
+                MatchesListwise([
+                    Equals(config.snappy.builder_proxy_auth_api_endpoint),
+                    ]),
+                MatchesDict({
+                    "method": Equals("POST"),
+                    "postdata": AfterPreprocessing(json.loads, MatchesDict({
+                        "username": StartsWith(job.build.build_cookie + "-"),
+                        })),
+                    "headers": MatchesDict({
+                        "Authorization": Equals("Basic " + base64.b64encode(
+                            "admin-launchpad.dev:admin-secret")),
+                        "Content-Type": Equals("application/json"),
+                        }),
+                    }),
+                ]),
+            ]))
+
+    @defer.inlineCallbacks
     def test_extraBuildArgs_bzr(self):
         # _extraBuildArgs returns appropriate arguments if asked to build a
         # job for a Bazaar branch.