Merge lp:~cjwatson/launchpad/faster-archive-signing-key-tests into lp:launchpad

[Colin Watson]

Speed up tests that use archive signing keys using an in-process keyserver fixture.

This only works for tests that only talk to the keyserver asynchronously, but for those that do it's very much faster to start one up in-process rather than using a .tac file: it saves on the order of 10 seconds per affected test on my laptop.

The tests for the fixture itself seem to hit some slightly buggy bit of Twisted and require a couple of extra spins through the reactor to clear up cancelled DelayedCalls, but fortunately testtools.deferredruntest already has machinery to tolerate that.

[William Grant (wgrant)]

review: Approve (code)

=== modified file 'lib/lp/archivepublisher/archivesigningkey.py'
--- lib/lp/archivepublisher/archivesigningkey.py	2016-06-17 21:38:32 +0000
+++ lib/lp/archivepublisher/archivesigningkey.py	2017-04-29 15:42:19 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """ArchiveSigningKey implementation."""
@@ -13,8 +13,13 @@
 import os
 
 import gpgme
+from twisted.internet.threads import deferToThread
 from zope.component import getUtility
 from zope.interface import implementer
+from zope.security.proxy import (
+    ProxyFactory,
+    removeSecurityProxy,
+    )
 
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.archivepublisher.config import getPubConfig
@@ -78,7 +83,7 @@
         secret_key = getUtility(IGPGHandler).generateKey(key_displayname)
         self._setupSigningKey(secret_key)
 
-    def setSigningKey(self, key_path):
+    def setSigningKey(self, key_path, async_keyserver=False):
         """See `IArchiveSigningKey`."""
         assert self.archive.signing_key is None, (
             "Cannot override signing_keys.")
@@ -88,22 +93,21 @@
         with open(key_path) as key_file:
             secret_key_export = key_file.read()
         secret_key = getUtility(IGPGHandler).importSecretKey(secret_key_export)
-        self._setupSigningKey(secret_key)
-
-    def _setupSigningKey(self, secret_key):
-        """Mandatory setup for signing keys.
-
-        * Export the secret key into the protected disk location.
-        * Upload public key to the keyserver.
-        * Store the public GPGKey reference in the database and update
-          the context archive.signing_key.
-        """
-        self.exportSecretKey(secret_key)
-
-        gpghandler = getUtility(IGPGHandler)
+        return self._setupSigningKey(
+            secret_key, async_keyserver=async_keyserver)
+
+    def _uploadPublicSigningKey(self, secret_key):
+        """Upload the public half of a signing key to the keyserver."""
+        # The handler's security proxying doesn't protect anything useful
+        # here, and when we're running in a thread we don't have an
+        # interaction.
+        gpghandler = removeSecurityProxy(getUtility(IGPGHandler))
         pub_key = gpghandler.retrieveKey(secret_key.fingerprint)
         gpghandler.uploadPublicKey(pub_key.fingerprint)
+        return pub_key
 
+    def _storeSigningKey(self, pub_key):
+        """Store signing key reference in the database."""
         key_owner = getUtility(ILaunchpadCelebrities).ppa_key_guard
         key, _ = getUtility(IGPGKeySet).activate(
             key_owner, pub_key, pub_key.can_encrypt)
@@ -111,6 +115,31 @@
         self.archive.signing_key_fingerprint = key.fingerprint
         del get_property_cache(self.archive).signing_key
 
+    def _setupSigningKey(self, secret_key, async_keyserver=False):
+        """Mandatory setup for signing keys.
+
+        * Export the secret key into the protected disk location.
+        * Upload public key to the keyserver.
+        * Store the public GPGKey reference in the database and update
+          the context archive.signing_key.
+        """
+        self.exportSecretKey(secret_key)
+        if async_keyserver:
+            # If we have an asynchronous keyserver running in the current
+            # thread using Twisted, then we need some contortions to ensure
+            # that the GPG handler doesn't deadlock.  This is most easily
+            # done by deferring the GPG handler work to another thread.
+            # Since that thread won't have a Zope interaction, we need to
+            # unwrap the security proxy for it.
+            d = deferToThread(
+                self._uploadPublicSigningKey, removeSecurityProxy(secret_key))
+            d.addCallback(ProxyFactory)
+            d.addCallback(self._storeSigningKey)
+            return d
+        else:
+            pub_key = self._uploadPublicSigningKey(secret_key)
+            self._storeSigningKey(pub_key)
+
     def signRepository(self, suite):
         """See `IArchiveSigningKey`."""
         assert self.archive.signing_key is not None, (

=== modified file 'lib/lp/archivepublisher/interfaces/archivesigningkey.py'
--- lib/lp/archivepublisher/interfaces/archivesigningkey.py	2016-06-17 21:07:22 +0000
+++ lib/lp/archivepublisher/interfaces/archivesigningkey.py	2017-04-29 15:42:19 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """ArchiveSigningKey interface."""
@@ -70,9 +70,12 @@
             upload to the keyserver.
         """
 
-    def setSigningKey(key_path):
+    def setSigningKey(key_path, async_keyserver=False):
         """Set a given secret key export as the context archive signing key.
 
+        :param key_path: full path to the secret key.
+        :param async_keyserver: true if the keyserver is running
+            asynchronously in the current thread.
         :raises AssertionError: if the context archive already has a
             `signing_key`.
         :raises AssertionError: if the given 'key_path' does not exist.

=== modified file 'lib/lp/archivepublisher/tests/test_archivesigningkey.py'
--- lib/lp/archivepublisher/tests/test_archivesigningkey.py	2016-06-17 21:07:22 +0000
+++ lib/lp/archivepublisher/tests/test_archivesigningkey.py	2017-04-29 15:42:19 +0000
@@ -1,4 +1,4 @@
-# Copyright 2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2016-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test ArchiveSigningKey."""
@@ -7,6 +7,8 @@
 
 import os
 
+from testtools.deferredruntest import AsynchronousDeferredRunTest
+from twisted.internet import defer
 from zope.component import getUtility
 
 from lp.archivepublisher.config import getPubConfig
@@ -18,14 +20,16 @@
 from lp.soyuz.enums import ArchivePurpose
 from lp.testing import TestCaseWithFactory
 from lp.testing.gpgkeys import gpgkeysdir
-from lp.testing.keyserver import KeyServerTac
+from lp.testing.keyserver import InProcessKeyServerFixture
 from lp.testing.layers import ZopelessDatabaseLayer
 
 
 class TestArchiveSigningKey(TestCaseWithFactory):
 
     layer = ZopelessDatabaseLayer
+    run_tests_with = AsynchronousDeferredRunTest.make_factory(timeout=10)
 
+    @defer.inlineCallbacks
     def setUp(self):
         super(TestArchiveSigningKey, self).setUp()
         self.temp_dir = self.makeTemporaryDirectory()
@@ -38,9 +42,11 @@
         self.archive_root = getPubConfig(self.archive).archiveroot
         self.suite = "distroseries"
 
-        with KeyServerTac():
+        with InProcessKeyServerFixture() as keyserver:
+            yield keyserver.start()
             key_path = os.path.join(gpgkeysdir, '[e-mail]')
-            IArchiveSigningKey(self.archive).setSigningKey(key_path)
+            yield IArchiveSigningKey(self.archive).setSigningKey(
+                key_path, async_keyserver=True)
 
     def test_signfile_absolute_within_archive(self):
         filename = os.path.join(self.archive_root, "signme")

=== modified file 'lib/lp/archivepublisher/tests/test_publishdistro.py'
--- lib/lp/archivepublisher/tests/test_publishdistro.py	2016-11-07 16:42:23 +0000
+++ lib/lp/archivepublisher/tests/test_publishdistro.py	2017-04-29 15:42:19 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Functional tests for publish-distro.py script."""
@@ -11,10 +11,12 @@
 import subprocess
 import sys
 
+from testtools.deferredruntest import AsynchronousDeferredRunTest
 from testtools.matchers import (
     Not,
     PathExists,
     )
+from twisted.internet import defer
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
@@ -47,13 +49,15 @@
 from lp.testing.fakemethod import FakeMethod
 from lp.testing.faketransaction import FakeTransaction
 from lp.testing.gpgkeys import gpgkeysdir
-from lp.testing.keyserver import KeyServerTac
+from lp.testing.keyserver import InProcessKeyServerFixture
 from lp.testing.layers import ZopelessDatabaseLayer
 
 
 class TestPublishDistro(TestNativePublishingBase):
     """Test the publish-distro.py script works properly."""
 
+    run_tests_with = AsynchronousDeferredRunTest.make_factory(timeout=10)
+
     def runPublishDistro(self, extra_args=None, distribution="ubuntutest"):
         """Run publish-distro without invoking the script.
 
@@ -222,6 +226,7 @@
         pub_source.sync()
         self.assertEqual(PackagePublishingStatus.PENDING, pub_source.status)
 
+    @defer.inlineCallbacks
     def testForPPA(self):
         """Try to run publish-distro in PPA mode.
 
@@ -247,11 +252,10 @@
         naked_archive.distribution = self.ubuntutest
 
         self.setUpRequireSigningKeys()
-        tac = KeyServerTac()
-        tac.setUp()
-        self.addCleanup(tac.tearDown)
+        yield self.useFixture(InProcessKeyServerFixture()).start()
         key_path = os.path.join(gpgkeysdir, '[e-mail]')
-        IArchiveSigningKey(cprov.archive).setSigningKey(key_path)
+        yield IArchiveSigningKey(cprov.archive).setSigningKey(
+            key_path, async_keyserver=True)
         name16.archive.signing_key_owner = cprov.archive.signing_key_owner
         name16.archive.signing_key_fingerprint = (
             cprov.archive.signing_key_fingerprint)
@@ -282,6 +286,7 @@
             'ppa/ubuntutest/pool/main/b/bar/bar_666.dsc')
         self.assertEqual('bar', open(bar_path).read().strip())
 
+    @defer.inlineCallbacks
     def testForPrivatePPA(self):
         """Run publish-distro in private PPA mode.
 
@@ -299,11 +304,10 @@
         self.layer.txn.commit()
 
         self.setUpRequireSigningKeys()
-        tac = KeyServerTac()
-        tac.setUp()
-        self.addCleanup(tac.tearDown)
+        yield self.useFixture(InProcessKeyServerFixture()).start()
         key_path = os.path.join(gpgkeysdir, '[e-mail]')
-        IArchiveSigningKey(private_ppa).setSigningKey(key_path)
+        yield IArchiveSigningKey(private_ppa).setSigningKey(
+            key_path, async_keyserver=True)
 
         # Try a plain PPA run, to ensure the private one is NOT published.
         self.runPublishDistro(['--ppa'])
@@ -398,17 +402,17 @@
             self.config.distsroot)
         self.assertNotExists(index_path)
 
+    @defer.inlineCallbacks
     def testCarefulRelease(self):
         """publish-distro can be asked to just rewrite Release files."""
         archive = self.factory.makeArchive(distribution=self.ubuntutest)
         pub_source = self.getPubSource(filecontent='foo', archive=archive)
 
         self.setUpRequireSigningKeys()
-        tac = KeyServerTac()
-        tac.setUp()
-        self.addCleanup(tac.tearDown)
+        yield self.useFixture(InProcessKeyServerFixture()).start()
         key_path = os.path.join(gpgkeysdir, '[e-mail]')
-        IArchiveSigningKey(archive).setSigningKey(key_path)
+        yield IArchiveSigningKey(archive).setSigningKey(
+            key_path, async_keyserver=True)
 
         self.layer.txn.commit()
 

=== modified file 'lib/lp/archivepublisher/tests/test_publisher.py'
--- lib/lp/archivepublisher/tests/test_publisher.py	2016-11-07 16:42:23 +0000
+++ lib/lp/archivepublisher/tests/test_publisher.py	2017-04-29 15:42:19 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for publisher class."""
@@ -32,6 +32,7 @@
 except ImportError:
     from backports import lzma
 import pytz
+from testtools.deferredruntest import AsynchronousDeferredRunTest
 from testtools.matchers import (
     ContainsAll,
     DirContains,
@@ -49,6 +50,7 @@
     SamePath,
     )
 import transaction
+from twisted.internet import defer
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
@@ -101,7 +103,7 @@
 from lp.testing import TestCaseWithFactory
 from lp.testing.fakemethod import FakeMethod
 from lp.testing.gpgkeys import gpgkeysdir
-from lp.testing.keyserver import KeyServerTac
+from lp.testing.keyserver import InProcessKeyServerFixture
 from lp.testing.layers import (
     LaunchpadZopelessLayer,
     ZopelessDatabaseLayer,
@@ -2927,6 +2929,8 @@
 class TestPublisherRepositorySignatures(TestPublisherBase):
     """Testing `Publisher` signature behaviour."""
 
+    run_tests_with = AsynchronousDeferredRunTest.make_factory(timeout=10)
+
     archive_publisher = None
 
     def tearDown(self):
@@ -3005,6 +3009,7 @@
         self.assertNotIn('Release.gpg', sync_args[1])
         self.assertNotIn('InRelease', sync_args[1])
 
+    @defer.inlineCallbacks
     def testRepositorySignatureWithSigningKey(self):
         """Check publisher behaviour when signing repositories.
 
@@ -3016,12 +3021,12 @@
         self.assertTrue(cprov.archive.signing_key is None)
 
         # Start the test keyserver, so the signing_key can be uploaded.
-        tac = KeyServerTac()
-        tac.setUp()
+        yield self.useFixture(InProcessKeyServerFixture()).start()
 
         # Set a signing key for Celso's PPA.
         key_path = os.path.join(gpgkeysdir, '[e-mail]')
-        IArchiveSigningKey(cprov.archive).setSigningKey(key_path)
+        yield IArchiveSigningKey(cprov.archive).setSigningKey(
+            key_path, async_keyserver=True)
         self.assertTrue(cprov.archive.signing_key is not None)
 
         self.setupPublisher(cprov.archive)
@@ -3061,9 +3066,6 @@
         self.assertThat(
             sync_args[1], ContainsAll(['Release', 'Release.gpg', 'InRelease']))
 
-        # All done, turn test-keyserver off.
-        tac.tearDown()
-
 
 class TestPublisherLite(TestCaseWithFactory):
     """Lightweight unit tests for the publisher."""
@@ -3299,7 +3301,9 @@
     """Unit tests for DirectoryHash object, signing functionality."""
 
     layer = ZopelessDatabaseLayer
+    run_tests_with = AsynchronousDeferredRunTest.make_factory(timeout=10)
 
+    @defer.inlineCallbacks
     def setUp(self):
         super(TestDirectoryHashSigning, self).setUp()
         self.temp_dir = self.makeTemporaryDirectory()
@@ -3313,13 +3317,11 @@
         self.suite = "distroseries"
 
         # Setup a keyserver so we can install the archive key.
-        tac = KeyServerTac()
-        tac.setUp()
-
-        key_path = os.path.join(gpgkeysdir, '[e-mail]')
-        IArchiveSigningKey(self.archive).setSigningKey(key_path)
-
-        tac.tearDown()
+        with InProcessKeyServerFixture() as keyserver:
+            yield keyserver.start()
+            key_path = os.path.join(gpgkeysdir, '[e-mail]')
+            yield IArchiveSigningKey(self.archive).setSigningKey(
+                key_path, async_keyserver=True)
 
     def test_basic_directory_add_signed(self):
         tmpdir = unicode(self.makeTemporaryDirectory())

=== modified file 'lib/lp/archivepublisher/tests/test_signing.py'
--- lib/lp/archivepublisher/tests/test_signing.py	2016-06-22 08:54:11 +0000
+++ lib/lp/archivepublisher/tests/test_signing.py	2017-04-29 15:42:19 +0000
@@ -1,4 +1,4 @@
-# Copyright 2012-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test UEFI custom uploads."""
@@ -10,6 +10,8 @@
 import tarfile
 
 from fixtures import MonkeyPatch
+from testtools.deferredruntest import AsynchronousDeferredRunTest
+from twisted.internet import defer
 from zope.component import getUtility
 
 from lp.archivepublisher.config import getPubConfig
@@ -31,7 +33,7 @@
 from lp.testing import TestCaseWithFactory
 from lp.testing.fakemethod import FakeMethod
 from lp.testing.gpgkeys import gpgkeysdir
-from lp.testing.keyserver import KeyServerTac
+from lp.testing.keyserver import InProcessKeyServerFixture
 from lp.testing.layers import ZopelessDatabaseLayer
 
 
@@ -87,6 +89,7 @@
 class TestSigningHelpers(TestCaseWithFactory):
 
     layer = ZopelessDatabaseLayer
+    run_tests_with = AsynchronousDeferredRunTest.make_factory(timeout=10)
 
     def setUp(self):
         super(TestSigningHelpers, self).setUp()
@@ -122,10 +125,13 @@
         if not os.path.exists(pubconf.temproot):
             os.makedirs(pubconf.temproot)
 
+    @defer.inlineCallbacks
     def setUpArchiveKey(self):
-        with KeyServerTac():
+        with InProcessKeyServerFixture() as keyserver:
+            yield keyserver.start()
             key_path = os.path.join(gpgkeysdir, '[e-mail]')
-            IArchiveSigningKey(self.archive).setSigningKey(key_path)
+            yield IArchiveSigningKey(self.archive).setSigningKey(
+                key_path, async_keyserver=True)
 
     def setUpUefiKeys(self, create=True):
         self.key = os.path.join(self.signing_dir, "uefi.key")
@@ -648,11 +654,12 @@
              "1.0", "SHA256SUMS")
         self.assertTrue(os.path.exists(sha256file))
 
+    @defer.inlineCallbacks
     def test_checksumming_tree_signed(self):
         # Specifying no options should leave us with an open tree,
         # confirm it is checksummed.  Supply an archive signing key
         # which should trigger signing of the checksum file.
-        self.setUpArchiveKey()
+        yield self.setUpArchiveKey()
         self.setUpUefiKeys()
         self.setUpKmodKeys()
         self.openArchive("test", "1.0", "amd64")

=== modified file 'lib/lp/testing/keyserver/__init__.py'
--- lib/lp/testing/keyserver/__init__.py	2013-01-07 02:40:55 +0000
+++ lib/lp/testing/keyserver/__init__.py	2017-04-29 15:42:19 +0000
@@ -1,8 +1,10 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the GNU
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the GNU
 # Affero General Public License version 3 (see the file LICENSE).
 
 __all__ = [
+    'InProcessKeyServerFixture',
     'KeyServerTac',
     ]
 
 from lp.testing.keyserver.harness import KeyServerTac
+from lp.testing.keyserver.inprocess import InProcessKeyServerFixture

=== added file 'lib/lp/testing/keyserver/inprocess.py'
--- lib/lp/testing/keyserver/inprocess.py	1970-01-01 00:00:00 +0000
+++ lib/lp/testing/keyserver/inprocess.py	2017-04-29 15:42:19 +0000
@@ -0,0 +1,69 @@
+# Copyright 2017 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""In-process keyserver fixture."""
+
+from __future__ import absolute_import, print_function, unicode_literals
+
+__metaclass__ = type
+__all__ = [
+    'InProcessKeyServerFixture',
+    ]
+
+from textwrap import dedent
+
+from fixtures import (
+    Fixture,
+    TempDir,
+    )
+from twisted.internet import (
+    defer,
+    endpoints,
+    reactor,
+    )
+from twisted.python.compat import nativeString
+from twisted.web import server
+
+from lp.services.config import config
+from lp.testing.keyserver.web import KeyServerResource
+
+
+class InProcessKeyServerFixture(Fixture):
+    """A fixture that runs an in-process key server.
+
+    This is much faster than the out-of-process `KeyServerTac`, but it can
+    only be used if all the tests relying on it are asynchronous.
+
+    Users of this fixture must call the `start` method, which returns a
+    `Deferred`, and arrange for that to get back to the reactor.  This is
+    necessary because the basic fixture API does not allow `setUp` to return
+    anything.  For example:
+
+        class TestSomething(TestCase):
+
+            run_tests_with = AsynchronousDeferredRunTest.make_factory(
+                timeout=10)
+
+            @defer.inlineCallbacks
+            def setUp(self):
+                super(TestSomething, self).setUp()
+                yield self.useFixture(InProcessKeyServerFixture()).start()
+    """
+
+    @defer.inlineCallbacks
+    def start(self):
+        resource = KeyServerResource(self.useFixture(TempDir()).path)
+        endpoint = endpoints.serverFromString(reactor, nativeString("tcp:0"))
+        port = yield endpoint.listen(server.Site(resource))
+        self.addCleanup(port.stopListening)
+        config.push("in-process-key-server-fixture", dedent("""
+            [gpghandler]
+            port: %s
+            """) % port.getHost().port)
+        self.addCleanup(config.pop, "in-process-key-server-fixture")
+
+    @property
+    def url(self):
+        """The URL that the web server will be running on."""
+        return ("http://%s:%d" % (
+            config.gpghandler.host, config.gpghandler.port)).encode("UTF-8")

=== added file 'lib/lp/testing/keyserver/tests/test_inprocess.py'
--- lib/lp/testing/keyserver/tests/test_inprocess.py	1970-01-01 00:00:00 +0000
+++ lib/lp/testing/keyserver/tests/test_inprocess.py	2017-04-29 15:42:19 +0000
@@ -0,0 +1,44 @@
+# Copyright 2017 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""In-process keyserver fixture tests."""
+
+from __future__ import absolute_import, print_function, unicode_literals
+
+__metaclass__ = type
+
+from testtools.deferredruntest import (
+    AsynchronousDeferredRunTestForBrokenTwisted,
+    )
+from twisted.internet import defer
+from twisted.web.client import getPage
+
+from lp.services.config import config
+from lp.testing import TestCase
+from lp.testing.keyserver import InProcessKeyServerFixture
+from lp.testing.keyserver.web import GREETING
+
+
+class TestInProcessKeyServerFixture(TestCase):
+
+    run_tests_with = AsynchronousDeferredRunTestForBrokenTwisted.make_factory(
+        timeout=10)
+
+    @defer.inlineCallbacks
+    def test_url(self):
+        # The url is the one that gpghandler is configured to hit.
+        fixture = self.useFixture(InProcessKeyServerFixture())
+        yield fixture.start()
+        self.assertEqual(
+            ("http://%s:%d" % (
+                config.gpghandler.host,
+                config.gpghandler.port)).encode("UTF-8"),
+            fixture.url)
+
+    @defer.inlineCallbacks
+    def test_starts_properly(self):
+        # The fixture starts properly and we can load the page.
+        fixture = self.useFixture(InProcessKeyServerFixture())
+        yield fixture.start()
+        content = yield getPage(fixture.url)
+        self.assertEqual(GREETING, content)