launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/uefi-signing-failures into lp:launchpad.

Commit message:
Make some UEFI-related errors into logged warnings instead, since they aren't important enough to justify the resulting publisher chaos.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1071562 in Launchpad itself: "UEFI signing failures cause binaries to be republished continuously"
  https://bugs.launchpad.net/launchpad/+bug/1071562

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/uefi-signing-failures/+merge/133666

== Summary ==

Bug 1071562: Failures when publishing UEFI custom uploads cause continuous republication attempts, resulting in vast numbers of excess BPPH rows.

== Proposed fix ==

The fundamental problem is that errors when publishing custom uploads involve non-transactional filesystem changes which are difficult to roll back; until that's fixed (perhaps by moving custom upload publication from process-accepted to the publisher proper), this is never going to be very graceful.  However, we can at least avoid causing gratuitous trouble for ourselves by making some less important errors into logged warnings.

If nothing else, it is relatively common for people to copy kernel source from quantal to their PPA without removing the UEFI tarball emission or arranging for their PPA to have UEFI signing keys.  This should be handled more gracefully by just not signing the resulting files.

== Tests ==

bin/test -vvct lp.archivepublisher.tests.test_uefi -t distroseriesqueue

== Demo and Q/A ==

Copy a version of efilinux that emits a UEFI tarball to a PPA without signing configuration, and make sure it gets published correctly.
-- 
https://code.launchpad.net/~cjwatson/launchpad/uefi-signing-failures/+merge/133666
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/uefi-signing-failures into lp:launchpad.

=== modified file 'lib/lp/archivepublisher/customupload.py'
--- lib/lp/archivepublisher/customupload.py	2012-07-03 17:09:00 +0000
+++ lib/lp/archivepublisher/customupload.py	2012-11-09 12:55:31 +0000
@@ -94,12 +94,13 @@
     # This should be set as a class property on each subclass.
     custom_type = None
 
-    def __init__(self):
+    def __init__(self, logger=None):
         self.targetdir = None
         self.version = None
         self.arch = None
 
         self.tmpdir = None
+        self.logger = logger
 
     def process(self, pubconf, tarfile_path, distroseries):
         """Process the upload and install it into the archive."""

=== modified file 'lib/lp/archivepublisher/ddtp_tarball.py'
--- lib/lp/archivepublisher/ddtp_tarball.py	2012-07-10 11:21:11 +0000
+++ lib/lp/archivepublisher/ddtp_tarball.py	2012-11-09 12:55:31 +0000
@@ -81,12 +81,12 @@
         pass
 
 
-def process_ddtp_tarball(pubconf, tarfile_path, distroseries):
+def process_ddtp_tarball(pubconf, tarfile_path, distroseries, logger=None):
     """Process a raw-ddtp-tarball tarfile.
 
     Unpacking it into the given archive for the given distroseries.
     Raises CustomUploadError (or some subclass thereof) if
     anything goes wrong.
     """
-    upload = DdtpTarballUpload()
+    upload = DdtpTarballUpload(logger=logger)
     upload.process(pubconf, tarfile_path, distroseries)

=== modified file 'lib/lp/archivepublisher/debian_installer.py'
--- lib/lp/archivepublisher/debian_installer.py	2012-07-10 11:21:11 +0000
+++ lib/lp/archivepublisher/debian_installer.py	2012-11-09 12:55:31 +0000
@@ -74,12 +74,12 @@
         return filename.startswith('%s/' % self.version)
 
 
-def process_debian_installer(pubconf, tarfile_path, distroseries):
+def process_debian_installer(pubconf, tarfile_path, distroseries, logger=None):
     """Process a raw-installer tarfile.
 
     Unpacking it into the given archive for the given distroseries.
     Raises CustomUploadError (or some subclass thereof) if anything goes
     wrong.
     """
-    upload = DebianInstallerUpload()
+    upload = DebianInstallerUpload(logger=logger)
     upload.process(pubconf, tarfile_path, distroseries)

=== modified file 'lib/lp/archivepublisher/dist_upgrader.py'
--- lib/lp/archivepublisher/dist_upgrader.py	2012-07-10 11:21:11 +0000
+++ lib/lp/archivepublisher/dist_upgrader.py	2012-11-09 12:55:31 +0000
@@ -100,12 +100,12 @@
         return version and not filename.startswith('current')
 
 
-def process_dist_upgrader(pubconf, tarfile_path, distroseries):
+def process_dist_upgrader(pubconf, tarfile_path, distroseries, logger=None):
     """Process a raw-dist-upgrader tarfile.
 
     Unpacking it into the given archive for the given distroseries.
     Raises CustomUploadError (or some subclass thereof) if anything goes
     wrong.
     """
-    upload = DistUpgraderUpload()
+    upload = DistUpgraderUpload(logger=logger)
     upload.process(pubconf, tarfile_path, distroseries)

=== modified file 'lib/lp/archivepublisher/tests/test_uefi.py'
--- lib/lp/archivepublisher/tests/test_uefi.py	2012-06-28 16:32:27 +0000
+++ lib/lp/archivepublisher/tests/test_uefi.py	2012-11-09 12:55:31 +0000
@@ -11,11 +11,7 @@
     CustomUploadAlreadyExists,
     CustomUploadBadUmask,
     )
-from lp.archivepublisher.uefi import (
-    UefiConfigurationError,
-    UefiNothingToSign,
-    UefiUpload,
-    )
+from lp.archivepublisher.uefi import UefiUpload
 from lp.services.osutils import write_file
 from lp.services.tarfile_helpers import LaunchpadWriteTarFile
 from lp.testing import TestCase
@@ -67,23 +63,30 @@
             "%s-%s" % (loader_type, arch))
 
     def test_unconfigured(self):
-        # If there is no key/cert configuration, processing fails.
+        # If there is no key/cert configuration, processing succeeds but
+        # nothing is signed.
         self.pubconf = FakeConfig(self.temp_dir, None)
         self.openArchive("test", "1.0", "amd64")
-        self.assertRaises(UefiConfigurationError, self.process)
+        self.archive.add_file("1.0/empty.efi", "")
+        upload = self.process()
+        self.assertEqual(0, upload.sign.call_count)
 
     def test_missing_key_and_cert(self):
-        # If the configured key/cert are missing, processing fails.
+        # If the configured key/cert are missing, processing succeeds but
+        # nothing is signed.
         self.openArchive("test", "1.0", "amd64")
         self.archive.add_file("1.0/empty.efi", "")
-        self.assertRaises(UefiConfigurationError, self.process)
+        upload = self.process()
+        self.assertEqual(0, upload.sign.call_count)
 
     def test_no_efi_files(self):
-        # Tarballs containing no *.efi files are rejected.
+        # Tarballs containing no *.efi files are extracted without complaint.
         self.setUpKeyAndCert()
         self.openArchive("empty", "1.0", "amd64")
-        self.archive.add_file("hello", "world")
-        self.assertRaises(UefiNothingToSign, self.process)
+        self.archive.add_file("1.0/hello", "world")
+        self.process()
+        self.assertTrue(os.path.exists(os.path.join(
+            self.getUefiPath("empty", "amd64"), "1.0", "hello")))
 
     def test_already_exists(self):
         # If the target directory already exists, processing fails.

=== modified file 'lib/lp/archivepublisher/uefi.py'
--- lib/lp/archivepublisher/uefi.py	2012-07-10 11:21:11 +0000
+++ lib/lp/archivepublisher/uefi.py	2012-11-09 12:55:31 +0000
@@ -19,27 +19,10 @@
 import os
 import subprocess
 
-from lp.archivepublisher.customupload import (
-    CustomUpload,
-    CustomUploadError,
-    )
+from lp.archivepublisher.customupload import CustomUpload
 from lp.services.osutils import remove_if_exists
 
 
-class UefiConfigurationError(CustomUploadError):
-    """No signing key location is configured."""
-    def __init__(self, message):
-        CustomUploadError.__init__(
-            self, "UEFI signing configuration error: %s" % message)
-
-
-class UefiNothingToSign(CustomUploadError):
-    """The tarball contained no *.efi files."""
-    def __init__(self, tarfile_path):
-        CustomUploadError.__init__(
-            self, "UEFI upload '%s' contained no *.efi files" % tarfile_path)
-
-
 class UefiUpload(CustomUpload):
     """UEFI boot loader custom upload.
 
@@ -77,16 +60,23 @@
 
     def setTargetDirectory(self, pubconf, tarfile_path, distroseries):
         if pubconf.uefiroot is None:
-            raise UefiConfigurationError(
-                "no UEFI root configured for this archive")
-        self.key = os.path.join(pubconf.uefiroot, "uefi.key")
-        self.cert = os.path.join(pubconf.uefiroot, "uefi.crt")
-        if not os.access(self.key, os.R_OK):
-            raise UefiConfigurationError(
-                "UEFI private key %s not readable" % self.key)
-        if not os.access(self.cert, os.R_OK):
-            raise UefiConfigurationError(
-                "UEFI certificate %s not readable" % self.cert)
+            if self.logger is not None:
+                self.logger.warning("No UEFI root configured for this archive")
+            self.key = None
+            self.cert = None
+        else:
+            self.key = os.path.join(pubconf.uefiroot, "uefi.key")
+            self.cert = os.path.join(pubconf.uefiroot, "uefi.crt")
+            if not os.access(self.key, os.R_OK):
+                if self.logger is not None:
+                    self.logger.warning(
+                        "UEFI private key %s not readable" % self.key)
+                self.key = None
+            if not os.access(self.cert, os.R_OK):
+                if self.logger is not None:
+                    self.logger.warning(
+                        "UEFI certificate %s not readable" % self.cert)
+                self.cert = None
 
         loader_type, self.version, self.arch = self.parsePath(tarfile_path)
         self.targetdir = os.path.join(
@@ -114,7 +104,11 @@
 
     def sign(self, image):
         """Sign an image."""
-        subprocess.check_call(self.getSigningCommand(image))
+        if subprocess.call(self.getSigningCommand(image)) != 0:
+            # Just log this rather than failing, since custom upload errors
+            # tend to make the publisher rather upset.
+            if self.logger is not None:
+                self.logger.warning("Failed to sign %s" % image)
 
     def extract(self):
         """Copy the custom upload to a temporary directory, and sign it.
@@ -122,23 +116,22 @@
         No actual extraction is required.
         """
         super(UefiUpload, self).extract()
-        efi_filenames = list(self.findEfiFilenames())
-        if not efi_filenames:
-            raise UefiNothingToSign(self.tarfile_path)
-        for efi_filename in efi_filenames:
-            remove_if_exists("%s.signed" % efi_filename)
-            self.sign(efi_filename)
+        if self.key is not None and self.cert is not None:
+            efi_filenames = list(self.findEfiFilenames())
+            for efi_filename in efi_filenames:
+                remove_if_exists("%s.signed" % efi_filename)
+                self.sign(efi_filename)
 
     def shouldInstall(self, filename):
         return filename.startswith("%s/" % self.version)
 
 
-def process_uefi(pubconf, tarfile_path, distroseries):
+def process_uefi(pubconf, tarfile_path, distroseries, logger=None):
     """Process a raw-uefi tarfile.
 
     Unpacking it into the given archive for the given distroseries.
     Raises CustomUploadError (or some subclass thereof) if anything goes
     wrong.
     """
-    upload = UefiUpload()
+    upload = UefiUpload(logger=logger)
     upload.process(pubconf, tarfile_path, distroseries)

=== modified file 'lib/lp/soyuz/model/queue.py'
--- lib/lp/soyuz/model/queue.py	2012-08-13 01:53:37 +0000
+++ lib/lp/soyuz/model/queue.py	2012-11-09 12:55:31 +0000
@@ -1484,7 +1484,7 @@
         copy_and_close(self.libraryfilealias, temp_file)
         return temp_file_name
 
-    def _publishCustom(self, action_method):
+    def _publishCustom(self, action_method, logger=None):
         """Publish custom formats.
 
         Publish Either an installer, an upgrader or a ddtp upload using the
@@ -1496,7 +1496,7 @@
         try:
             # See the XXX near the import for getPubConfig.
             archive_config = getPubConfig(self.packageupload.archive)
-            action_method(archive_config, temp_filename, suite)
+            action_method(archive_config, temp_filename, suite, logger=logger)
         finally:
             shutil.rmtree(os.path.dirname(temp_filename))
 
@@ -1507,7 +1507,7 @@
         from lp.archivepublisher.debian_installer import (
             process_debian_installer)
 
-        self._publishCustom(process_debian_installer)
+        self._publishCustom(process_debian_installer, logger=logger)
 
     def publishDistUpgrader(self, logger=None):
         """See `IPackageUploadCustom`."""
@@ -1516,7 +1516,7 @@
         from lp.archivepublisher.dist_upgrader import (
             process_dist_upgrader)
 
-        self._publishCustom(process_dist_upgrader)
+        self._publishCustom(process_dist_upgrader, logger=logger)
 
     def publishDdtpTarball(self, logger=None):
         """See `IPackageUploadCustom`."""
@@ -1525,7 +1525,7 @@
         from lp.archivepublisher.ddtp_tarball import (
             process_ddtp_tarball)
 
-        self._publishCustom(process_ddtp_tarball)
+        self._publishCustom(process_ddtp_tarball, logger=logger)
 
     def publishRosettaTranslations(self, logger=None):
         """See `IPackageUploadCustom`."""
@@ -1607,7 +1607,7 @@
         # to instantiate the object in question and avoid circular imports
         from lp.archivepublisher.uefi import process_uefi
 
-        self._publishCustom(process_uefi)
+        self._publishCustom(process_uefi, logger=logger)
 
     publisher_dispatch = {
         PackageUploadCustomFormat.DEBIAN_INSTALLER: publishDebianInstaller,