Better handling of fatal upload errors

Bug #499438 reported by Michael Nelson
24
This bug affects 4 people
Affects Status Importance Assigned to Milestone
Launchpad itself
Fix Released
High
Colin Watson

Bug Description

Currently when a person uploads a bad source, such as a changes file without a mandatory binary field, a FatalUploadError is raised, and an oops created but the user never receives an email response.

Examples:
http://pastebin.ubuntu.com/344658/

After uploading the package to dogfood, we were able to see the (obvious) issue:
http://pastebin.ubuntu.com/344696/

  FatalUploadError: Unable to find mandatory field 'Binary' in the changes file.
   OOPS-1921BUILDMASTER1, OOPS-1921BUILDMASTER2, OOPS-1921PPA6

  FatalUploadError: Unable to find mandatory field 'Files' in the changes file.
   OOPS-1921PPA4, OOPS-1921PPA5

  FatalUploadError: Unable to find mandatory field 'Date' in the changes file.
   OOPS-2021FTPMASTER5

  FatalUploadError: File /srv/launchpad.net/ppa-queue/incoming/upload-ftp-20110728-172729-005330/~ferramroberto/varie/ubuntu/grsync_1.2.0-1~lffl~maverick~ppa_source.changes is signed with a deactivated key 85C20848
   OOPS-2035PPA5

  FatalUploadError: Signing key 0D9EDC5F633B05D4763342C376D4635FB2A34D75 not registered in launchpad.
   OOPS-2047PPA10

We should handle this exception and notify the user via email.

Related branches

tags: added: soyuz-upload
Changed in soyuz:
milestone: none → 10.01
status: New → Triaged
importance: Undecided → High
Changed in soyuz:
assignee: nobody → Michael Nelson (michael.nelson)
Changed in soyuz:
status: Triaged → In Progress
Revision history for this message
Michael Nelson (michael.nelson) wrote :

Pre-implementation notes requiring feedback:

After a brief survey of the code in lp.archiveuploader.changesfile.ChangesFile.__init__(), it seems that we generate an oops (rather than rejecting the upload with a nice email) if:

 1. The changes file can't be parsed correctly,
 2. *any* of the mandatory fields are not present (currently source, binary, architecture, version, distribution, maintainer, files, changes),
 3. the format of the changes file is not supported, or
 4. the signature could not be verified (for a number of reasons).

I assume (2) is happening because if we can't verify the maintainer (one of the mandatory fields), we know we can't email them, but I'm not sure why we've included the other fields in this check.

So, the plan is to update (2) above simply to:

2. The maintainer field is not present (hrm, should really be changed-by? What's the reason that changed-by is not already a mandatory field in this check?)

and remove (3) altogether from __init__, and then add two new methods:

ChangesFile.checkMandatoryFields()
ChangesFile.checkChangesFormat()

which can be used by NascentUpload.process (which calls run_and_reject_on_errors).

The result will be that we only oops in situations where we don't have a signer or maintainer email.

Note: at first I was confused why we don't include changed-by as a mandatory field, but realised it might be because it's not required by policy (http://www.debian.org/doc/debian-policy/ch-controlfields.html#s-debianchangesfiles). But we do raise an UploadError during checkAddresses later if it's not present (which doesn't generate an oops. but rejects the upload, which will try to email the signer as per normal).

Note: currently PackageUpload._getRecipients() always includes either the signer or the changed-by address. But even though these may be present, an email will still not be sent if they have no preferredEmail etc.

Changed in soyuz:
status: In Progress → Triaged
assignee: Michael Nelson (michael.nelson) → nobody
Changed in soyuz:
milestone: 10.01 → 10.02
importance: High → Low
Ursula Junque (ursinha)
description: updated
Changed in soyuz:
milestone: 10.02 → none
Ursula Junque (ursinha)
description: updated
description: updated
tags: added: oops
Changed in launchpad:
importance: Low → Critical
description: updated
description: updated
Changed in launchpad:
assignee: nobody → Julian Edwards (julian-edwards)
description: updated
Revision history for this message
William Grant (wgrant) wrote :

The OOPS is being handled separately, in bug #862032. This bug is now about trying harder to notify; in some cases we have a valid signature, so we can notify the signer even if the rest of the file is corrupt.

Changed in launchpad:
importance: Critical → High
tags: removed: oops
description: updated
Revision history for this message
Robert Collins (lifeless) wrote :

Note that this would be much easier if we knew the uploader and thus could just notify their account. We do for SFTP uploads. I'm inclined to suggest that we deprecate FTP uploads and spend no effort to make them better - diminishing returns, and a known complex stack which is hard for new users (exactly the users that will make bad packages) to work with,

Revision history for this message
Robert Collins (lifeless) wrote :

Also, @bigjools, are you hacking on this still?

Revision history for this message
Julian Edwards (julian-edwards) wrote :

Nope, removed myself.

Changed in launchpad:
assignee: Julian Edwards (julian-edwards) → nobody
Revision history for this message
Julian Edwards (julian-edwards) wrote :

"I'm inclined to suggest that we deprecate FTP uploads"

That will not happen - you will encounter *serious* resistance from Ubuntu people.

Revision history for this message
Francis J. Lacoste (flacoste) wrote :

@julian resistance on what ground?

Revision history for this message
Julian Edwards (julian-edwards) wrote :

* dput with SFTP provides no progress information
* client support in Ubuntu is recent and the dput.cf defaults to FTP so it's not reasonable to disable it in LP until that's changed, including stable releases
* the dput.cf for SFTP has to have the user's LP name in it, so is hard to have a default at all
* Debian's dput doesn't have SFTP support yet

It's not insurmountable but has both political and technical issues.

Revision history for this message
Robert Collins (lifeless) wrote : Re: [Bug 499438] Re: Better handling of fatal upload errors

On Thu, Jan 5, 2012 at 5:45 AM, Julian Edwards
<email address hidden> wrote:
> * dput with SFTP provides no progress information
> * client support in Ubuntu is recent and the dput.cf defaults to FTP so it's not reasonable to disable it in LP until that's changed, including stable releases
> * the dput.cf for SFTP has to have the user's LP name in it, so is hard to have a default at all
> * Debian's dput doesn't have SFTP support yet
>
> It's not insurmountable but has both political and technical issues.

FWIW I talked with cjwatson about this before adding that comment.

Dput could default to sftp, prompt for the username, and fallback to
FTP if none was supplied or there was no terminal on which to prompt.
If it succeeded with a username, it would save it in a personal
dotfile and use it the next time.

-Rob

Colin Watson (cjwatson)
Changed in launchpad:
assignee: nobody → Colin Watson (cjwatson)
status: Triaged → In Progress
ejjou (ejjou)
information type: Public → Private
Colin Watson (cjwatson)
information type: Private → Public
Revision history for this message
Launchpad QA Bot (lpqabot) wrote :
tags: added: qa-needstesting
Changed in launchpad:
status: In Progress → Fix Committed
Colin Watson (cjwatson)
tags: added: qa-ok
removed: qa-needstesting
Colin Watson (cjwatson)
Changed in launchpad:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Duplicates of this bug

Other bug subscribers

Related questions

Remote bug watches

Bug watches keep track of this bug in other bug trackers.