launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/no-key-oopses into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #862032 in Launchpad itself: "Changes file parse failures shouldn't OOPS"
  https://bugs.launchpad.net/launchpad/+bug/862032

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/no-key-oopses/+merge/77447

This branch fixes some user-error Soyuz OOPSes: FatalUploadErrors from process-upload.

FatalUploadErrors are caused by ChangesFile in case of signature issues (no signature, bad signature, unrecognized key, etc.) or corrupt changes files. Since we use the changes file to determine the notification recipient, such a failure means that we're unable to email anyone about it.

Long ago it was decided to turn such unreportable user errors into OOPSes. But that runs counter to ZeroOopsPolicy, so we should not do it. This branch logs them at DEBUG instead, with the rest of the normal processing stuff.
-- 
https://code.launchpad.net/~wgrant/launchpad/no-key-oopses/+merge/77447
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/no-key-oopses into lp:launchpad.

=== modified file 'lib/lp/archiveuploader/nascentupload.py'
--- lib/lp/archiveuploader/nascentupload.py	2011-08-23 14:35:43 +0000
+++ lib/lp/archiveuploader/nascentupload.py	2011-09-29 05:22:06 +0000
@@ -49,10 +49,6 @@
 PARTNER_COMPONENT_NAME = 'partner'
 
 
-class FatalUploadError(Exception):
-    """A fatal error occurred processing the upload; processing aborted."""
-
-
 class EarlyReturnUploadError(Exception):
     """An error occurred that prevented further error collection."""
 
@@ -114,23 +110,14 @@
     def from_changesfile_path(cls, changesfile_path, policy, logger):
         """Create a NascentUpload from the given changesfile path.
 
-        May raise FatalUploadError due to unrecoverable problems building
+        May raise UploadError due to unrecoverable problems building
         the ChangesFile object.
 
         :param changesfile_path: path to the changesfile to be uploaded.
         :param policy: the upload policy to be used.
         :param logger: the logger to be used.
         """
-        try:
-            changesfile = ChangesFile(changesfile_path, policy, logger)
-        except UploadError, e:
-            # We can't run reject() because unfortunately we don't have
-            # the address of the uploader to notify -- we broke in that
-            # exact step.
-            # XXX cprov 2007-03-26: we should really be emailing this
-            # rejection to the archive admins. For now, this will end
-            # up in the script log.
-            raise FatalUploadError(str(e))
+        changesfile = ChangesFile(changesfile_path, policy, logger)
         return cls(changesfile, policy, logger)
 
     def process(self, build=None):

=== modified file 'lib/lp/archiveuploader/tests/nascentupload.txt'
--- lib/lp/archiveuploader/tests/nascentupload.txt	2011-06-28 15:04:29 +0000
+++ lib/lp/archiveuploader/tests/nascentupload.txt	2011-09-29 05:22:06 +0000
@@ -45,14 +45,14 @@
 Constructing a NascentUpload instance verifies that the changes file
 specified exists and tries to build a ChangesFile (see
 doc/nascentuploadfile.txt) object based on that. If anything goes
-wrong during this process FatalUploadError is raised:
+wrong during this process UploadError is raised:
 
     >>> from lp.services.log.logger import DevNullLogger, FakeLogger
     >>> NascentUpload.from_changesfile_path(
     ...     datadir("DOES-NOT-EXIST"), buildd_policy, FakeLogger())
     Traceback (most recent call last):
     ...
-    FatalUploadError:...
+    UploadError:...
 
 Otherwise a ChangesFile object is ready to use.
 

=== modified file 'lib/lp/archiveuploader/tests/test_uploadprocessor.py'
--- lib/lp/archiveuploader/tests/test_uploadprocessor.py	2011-08-26 06:14:54 +0000
+++ lib/lp/archiveuploader/tests/test_uploadprocessor.py	2011-09-29 05:22:06 +0000
@@ -15,6 +15,7 @@
 from StringIO import StringIO
 import tempfile
 
+from fixtures import MonkeyPatch
 from storm.locals import Store
 import transaction
 from zope.component import (
@@ -42,6 +43,7 @@
     parse_build_upload_leaf_name,
     UploadHandler,
     UploadProcessor,
+    UploadStatusEnum,
     )
 from lp.buildmaster.enums import (
     BuildFarmJobType,
@@ -1361,28 +1363,27 @@
         self.options.builds = False
         processor = self.getUploadProcessor(self.layer.txn)
 
-        upload_dir = self.queueUpload("foocomm_1.0-1_proposed")
-        bogus_changesfile_data = '''
-        Ubuntu is a community developed, Linux-based operating system that is
-        perfect for laptops, desktops and servers. It contains all the
-        applications you need - a web browser, presentation, document and
-        spreadsheet software, instant messaging and much more.
-        '''
-        file_handle = open(
-            '%s/%s' % (upload_dir, 'bogus.changes'), 'w')
-        file_handle.write(bogus_changesfile_data)
-        file_handle.close()
+        self.queueUpload("foocomm_1.0-1_proposed")
+
+        # Any code that causes an OOPS is a bug that must be fixed, so
+        # let's monkeypatch something in that we know will OOPS.
+        class SomeException(Exception):
+            pass
+
+        def from_changesfile_path(cls, changesfile_path, policy, logger):
+            raise SomeException("I am an explanation.")
+        self.useFixture(
+            MonkeyPatch(
+                'lp.archiveuploader.nascentupload.NascentUpload.'
+                'from_changesfile_path',
+                classmethod(from_changesfile_path)))
 
         processor.processUploadQueue()
 
         error_utility = ErrorReportingUtility()
         error_report = error_utility.getLastOopsReport()
-        self.assertEqual('FatalUploadError', error_report.type)
-        # The upload policy requires a signature but none is present, so
-        # we get gpg verification errors.
-        expected_explanation = (
-            "Verification failed 3 times: ['No data', 'No data', 'No data']")
-        self.assertIn(expected_explanation, error_report.tb_text)
+        self.assertEqual('SomeException', error_report.type)
+        self.assertIn("I am an explanation", error_report.tb_text)
 
     def testLZMADebUpload(self):
         """Make sure that data files compressed with lzma in Debs work.
@@ -1919,6 +1920,21 @@
         self.PGPSignatureNotPreserved(archive=self.breezy.main_archive)
         self.switchToUploader()
 
+    def test_unsigned_upload_is_silently_rejected(self):
+        """Unsigned uploads are silently rejected without an OOPS."""
+        uploadprocessor = self.setupBreezyAndGetUploadProcessor()
+        upload_dir = self.queueUpload("netapplet_1.0-1")
+
+        last_oops = ErrorReportingUtility().getLastOopsReport()
+
+        [result] = self.processUpload(uploadprocessor, upload_dir)
+
+        self.assertEqual(UploadStatusEnum.REJECTED, result)
+        self.assertLogContains(
+            "DEBUG Failed to parse changes file: GPG verification")
+        self.assertEqual(len(stub.test_emails), 0)
+        self.assertNoNewOops(last_oops)
+
 
 class TestBuildUploadProcessor(TestUploadProcessorBase):
     """Test that processing build uploads works."""

=== modified file 'lib/lp/archiveuploader/uploadprocessor.py'
--- lib/lp/archiveuploader/uploadprocessor.py	2011-05-04 06:37:50 +0000
+++ lib/lp/archiveuploader/uploadprocessor.py	2011-09-29 05:22:06 +0000
@@ -63,8 +63,8 @@
 from lp.app.errors import NotFoundError
 from lp.archiveuploader.nascentupload import (
     EarlyReturnUploadError,
-    FatalUploadError,
     NascentUpload,
+    UploadError,
     )
 from lp.archiveuploader.uploadpolicy import (
     BuildDaemonUploadPolicy,
@@ -370,8 +370,20 @@
         # The path we want for NascentUpload is the path to the folder
         # containing the changes file (and the other files referenced by it).
         changesfile_path = os.path.join(self.upload_path, changes_file)
-        upload = NascentUpload.from_changesfile_path(
-            changesfile_path, policy, self.processor.log)
+        try:
+            upload = NascentUpload.from_changesfile_path(
+                changesfile_path, policy, self.processor.log)
+        except UploadError, e:
+            # We failed to parse the changes file, so we have no key or
+            # Changed-By to notify of the rejection. Just log it and
+            # move on.
+            # XXX wgrant 2011-09-29 bug=499438: With some refactoring we
+            # could do better here: if we have a signature then we have
+            # somebody to email, even if the rest of the file is
+            # corrupt.
+            logger.debug("Failed to parse changes file: %s" % str(e))
+            logger.debug("Nobody to notify.")
+            return UploadStatusEnum.REJECTED
 
         # Reject source upload to buildd upload paths.
         first_path = relative_path.split(os.path.sep)[0]
@@ -401,10 +413,6 @@
                               "%s " % e)
                 logger.debug(
                     "UploadPolicyError escaped upload.process", exc_info=True)
-            except FatalUploadError, e:
-                upload.reject("UploadError escaped upload.process: %s" % e)
-                logger.debug(
-                    "UploadError escaped upload.process", exc_info=True)
             except (KeyboardInterrupt, SystemExit):
                 raise
             except EarlyReturnUploadError: