Launchpad itself

Merge lp:~barry/launchpad/403606-expat into lp:launchpad

  1. 403606-expat
  2. Merge into devel
Proposed by Barry Warsaw
Status: Merged
Merged at revision: not available
Proposed branch: lp:~barry/launchpad/403606-expat
Merge into: lp:launchpad
Diff against target: 278 lines
9 files modified
lib/canonical/launchpad/mailman/monkeypatches/lpmoderate.py (+2/-1)
lib/canonical/launchpad/webapp/publication.py (+1/-1)
lib/canonical/launchpad/webapp/servers.py (+1/-0)
lib/canonical/launchpad/xmlrpc/mailinglist.py (+11/-2)
lib/lp/registry/doc/mailinglist-xmlrpc.txt (+7/-6)
lib/lp/registry/doc/message-holds-xmlrpc.txt (+50/-1)
lib/lp/registry/interfaces/mailinglist.py (+2/-2)
lib/lp/services/mailman/doc/create-lists.txt (+2/-2)
lib/lp/services/mailman/doc/postings.txt (+32/-0)
To merge this branch: bzr merge lp:~barry/launchpad/403606-expat
Reviewer Review Type Date Requested Status
Paul Hummer (community) code Approve
Canonical Launchpad Engineering Pending
Review via email: mp+12734@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Barry Warsaw (barry) wrote :
Download full text (6.5 KiB)

= Summary =

Bug 403606 is a fun one. For a long while now, we've been getting mysterious
OOPSes in the private XMLRPC server that Mailman talks too. Unfortunately,
Zope swallows all the information that would be useful to diagnose the
problem. OTOH, it never seemed to degrade the service, but it annoyed the
LOSAs and helping them out is always a good thing!

Now that 3.0 is out, it was time to look into this problem, so with the LOSAs
help, I instrumented the private server to gather the additional information.
Specifically, I added some code so that when the XMLRPC request failed, I
logged the entire request body. We ran that code overnight and it was obvious
what the problem was.

It turns out that IMailingListAPIView.holdMessage() is the culprit, and
specifically it's when Mailman tries to hold messages with non-ascii
characters in its headers or body. This makes sense as non-ascii must be
encoded over the wire for XMLRPC.

The downside is that all the messages in the overnight log that I could see
that triggered this were spam. That jives with my experience in Mailman (why
spammers would send at worst illegal or at best often incompatible messages is
beyond me, but there ya go). So the good news is that this should eliminate
or at least largely reduce the OOPSes we're seeing, but the bad news is that
it will probably increase the amount of spam list owners have to moderate.
That is a separate problem though, out of scope for this branch (and we have
ways of mitigating that).

== Proposed fix ==

On the XMLRPC client side in Mailman, wrap the message body texts in an
xmlrpc.Binary for transport over the wire. On the private XMLRPC server side,
unwrap the Binary to get the text bytes of the original message.

== Pre-implementation notes ==

I didn't have any discussions specifically about this fix, but had several
discussions with the LOSAs, Brad, and Francis about how to diagnose the
problem.

== Implementation details ==

A few things to note. On the server side, I do a type-test of the 'bytes'
argument to see whether to unwrap the Binary or not. The reason I don't do
this unconditionally is that it would require a rewrite of all the existing
tests, where most of the message bodies are pure-ascii. So while the client
always sends a Binary, most of the tests will still send strings.

I paid some tech debt along the way. Apologies for cluttering up the meat of
this change.

== Tests ==

% bin/test -vv -t mailinglist-xmlrpc.txt
% bin/test -vv --layer=MailmanLayer -t create-list -t postings

Note that the MailmanLayer tests are sometimes flaky and are not run by 'make
check' by default. Still, you should be able to get through them, though ping
me if you can't.

== Demo and Q/A ==

There are no user visible changes on this branch, but we'll QA it by watching
the daily OOPS reports.

= Launchpad lint =

These lint warnings are bogus.

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/services/mailman/doc/postings.txt
  lib/canonical/launchpad/webapp/publication.py
  lib/canonical/launchpad/mailman/monkeypatches/lp...

Read more...

Revision history for this message
Paul Hummer (rockstar) :
review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/canonical/launchpad/mailman/monkeypatches/lpmoderate.py'
--- lib/canonical/launchpad/mailman/monkeypatches/lpmoderate.py 2009-06-25 05:30:52 +0000
+++ lib/canonical/launchpad/mailman/monkeypatches/lpmoderate.py 2009-10-01 16:59:15 +0000
@@ -82,7 +82,8 @@
82 # This will fail if we can't talk to Launchpad. That's okay though82 # This will fail if we can't talk to Launchpad. That's okay though
83 # because Mailman's IncomingRunner will re-queue the message and re-start83 # because Mailman's IncomingRunner will re-queue the message and re-start
84 # processing at this handler.84 # processing at this handler.
85 proxy.holdMessage(mlist.internal_name(), msg.as_string())85 proxy.holdMessage(mlist.internal_name(),
86 xmlrpclib.Binary(msg.as_string()))
86 syslog('vette', 'Holding message for LP approval: %s', message_id)87 syslog('vette', 'Holding message for LP approval: %s', message_id)
87 # Raise this exception, signaling to the incoming queue runner that it is88 # Raise this exception, signaling to the incoming queue runner that it is
88 # done processing this message, and should not send it through any further89 # done processing this message, and should not send it through any further
8990
=== modified file 'lib/canonical/launchpad/webapp/publication.py'
--- lib/canonical/launchpad/webapp/publication.py 2009-09-17 14:29:22 +0000
+++ lib/canonical/launchpad/webapp/publication.py 2009-10-01 16:59:15 +0000
@@ -482,7 +482,7 @@
482 pass482 pass
483483
484 # Log a soft OOPS for DisconnectionErrors as per Bug #373837.484 # Log a soft OOPS for DisconnectionErrors as per Bug #373837.
485 # We need to do this before we re-raise the excaptionsas a Retry.485 # We need to do this before we re-raise the exception as a Retry.
486 if isinstance(exc_info[1], DisconnectionError):486 if isinstance(exc_info[1], DisconnectionError):
487 getUtility(IErrorReportingUtility).raising(exc_info, request)487 getUtility(IErrorReportingUtility).raising(exc_info, request)
488488
489489
=== modified file 'lib/canonical/launchpad/webapp/servers.py'
--- lib/canonical/launchpad/webapp/servers.py 2009-07-28 22:35:01 +0000
+++ lib/canonical/launchpad/webapp/servers.py 2009-10-01 16:59:15 +0000
@@ -1301,6 +1301,7 @@
1301 """Request type for doing private XML-RPC in Launchpad."""1301 """Request type for doing private XML-RPC in Launchpad."""
1302 # For now, the same as public requests.1302 # For now, the same as public requests.
13031303
1304
1304# ---- Protocol errors1305# ---- Protocol errors
13051306
1306class ProtocolErrorRequest(LaunchpadBrowserRequest):1307class ProtocolErrorRequest(LaunchpadBrowserRequest):
13071308
=== modified file 'lib/canonical/launchpad/xmlrpc/mailinglist.py'
--- lib/canonical/launchpad/xmlrpc/mailinglist.py 2009-06-25 05:30:52 +0000
+++ lib/canonical/launchpad/xmlrpc/mailinglist.py 2009-10-01 16:59:15 +0000
@@ -9,6 +9,8 @@
9 ]9 ]
1010
1111
12import xmlrpclib
13
12from zope.component import getUtility14from zope.component import getUtility
13from zope.interface import implements15from zope.interface import implements
14from zope.security.proxy import removeSecurityProxy16from zope.security.proxy import removeSecurityProxy
@@ -222,10 +224,17 @@
222 return person.personal_standing in (PersonalStanding.GOOD,224 return person.personal_standing in (PersonalStanding.GOOD,
223 PersonalStanding.EXCELLENT)225 PersonalStanding.EXCELLENT)
224226
225 def holdMessage(self, team_name, text):227 def holdMessage(self, team_name, bytes):
226 """See `IMailingListAPIView`."""228 """See `IMailingListAPIView`."""
229 # For testing purposes, accept both strings and Binary instances. In
230 # production, and as tested by 'bin/test--layer=MailmanLayer' bytes
231 # will always be a Binary so that unencoded non-ascii characters in
232 # the message can be safely passed across XMLRPC. For most tests
233 # though it's much more convenient to just pass 8-bit strings.
234 if isinstance(bytes, xmlrpclib.Binary):
235 bytes = bytes.data
227 mailing_list = getUtility(IMailingListSet).get(team_name)236 mailing_list = getUtility(IMailingListSet).get(team_name)
228 message = getUtility(IMessageSet).fromEmail(text)237 message = getUtility(IMessageSet).fromEmail(bytes)
229 mailing_list.holdMessage(message)238 mailing_list.holdMessage(message)
230 return True239 return True
231240
232241
=== modified file 'lib/lp/registry/doc/mailinglist-xmlrpc.txt'
--- lib/lp/registry/doc/mailinglist-xmlrpc.txt 2009-05-06 15:13:39 +0000
+++ lib/lp/registry/doc/mailinglist-xmlrpc.txt 2009-10-01 16:59:15 +0000
@@ -1,3 +1,4 @@
1=====================================
1XML-RPC integration for mailing lists2XML-RPC integration for mailing lists
2=====================================3=====================================
34
@@ -11,7 +12,7 @@
1112
1213
13Registered lists14Registered lists
14----------------15================
1516
16When there's nothing for Mailman to do, we'll just get an empty dictionary17When there's nothing for Mailman to do, we'll just get an empty dictionary
17back from the XMLRPC call.18back from the XMLRPC call.
@@ -93,7 +94,7 @@
9394
9495
95Creating the mailing lists96Creating the mailing lists
96--------------------------97==========================
9798
98Mailman will attempt to satisfy the requested creation. This will either99Mailman will attempt to satisfy the requested creation. This will either
99succeed or fail, and this status is communicated back to Launchpad. Let's say100succeed or fail, and this status is communicated back to Launchpad. Let's say
@@ -158,7 +159,7 @@
158159
159160
160Deactivating mailing lists161Deactivating mailing lists
161--------------------------162==========================
162163
163The other action that Mailman needs to take is to deactivate a list when the164The other action that Mailman needs to take is to deactivate a list when the
164team owner requests it. These also show up as pending actions when Mailman165team owner requests it. These also show up as pending actions when Mailman
@@ -220,7 +221,7 @@
220221
221222
222Modifying mailing lists223Modifying mailing lists
223-----------------------224=======================
224225
225Activate mailing lists can also be modified. Currently the only modification226Activate mailing lists can also be modified. Currently the only modification
226supported is changing the list's welcome message. Because deactivated and227supported is changing the list's welcome message. Because deactivated and
@@ -289,7 +290,7 @@
289290
290291
291Error conditions292Error conditions
292----------------293================
293294
294Although we control both ends of this XMLRPC interface, it might still be295Although we control both ends of this XMLRPC interface, it might still be
295possible to pass bad input, due to bugs and such. The XMLRPC interface is296possible to pass bad input, due to bugs and such. The XMLRPC interface is
@@ -391,7 +392,7 @@
391392
392393
393Public teams394Public teams
394------------395============
395396
396We provide a method to query whether or not a given team is public. That's397We provide a method to query whether or not a given team is public. That's
397useful when deciding whether or not anonymous access to the team's mailing398useful when deciding whether or not anonymous access to the team's mailing
398399
=== modified file 'lib/lp/registry/doc/message-holds-xmlrpc.txt'
--- lib/lp/registry/doc/message-holds-xmlrpc.txt 2009-05-12 21:39:35 +0000
+++ lib/lp/registry/doc/message-holds-xmlrpc.txt 2009-10-01 16:59:15 +0000
@@ -1,3 +1,4 @@
1=====================================
1XMLRPC interface for holding messages2XMLRPC interface for holding messages
2=====================================3=====================================
34
@@ -176,7 +177,7 @@
176177
177178
178Private membership teams179Private membership teams
179------------------------180========================
180181
181When a private membership team has a mailing list, its messages can also be182When a private membership team has a mailing list, its messages can also be
182held for moderator approval.183held for moderator approval.
@@ -220,3 +221,51 @@
220 >>> dispositions = mailinglist_api.getMessageDispositions()221 >>> dispositions = mailinglist_api.getMessageDispositions()
221 >>> print_dispositions(dispositions)222 >>> print_dispositions(dispositions)
222 <fourth-post> team-private accept223 <fourth-post> team-private accept
224
225
226Non-ascii messages
227==================
228
229Messages with non-ascii in their headers or bodies are not exactly legal (they
230should be encoded) but do occur especially in spam. These messages can be
231held for moderator approval too.
232
233 >>> spam_message = message_from_string("""\
234 ... From: Anne \xa9 Person <anne.person@example.com>
235 ... To: team-one@lists.launchpad.dev
236 ... Subject: \xa9 Badgers!
237 ... Message-ID: <fifth-post>
238 ... Date: Fri, 01 Aug 2000 01:08:59 -0000
239 ...
240 ... Watch out for badgers! \xa9
241 ... """)
242
243 >>> import xmlrpclib
244 >>> mailinglist_api.holdMessage(
245 ... 'team-one',
246 ... xmlrpclib.Binary(spam_message.as_string()))
247 True
248 >>> commit()
249
250 >>> held_message_spam = message_set.getMessageByMessageID('<fifth-post>')
251 >>> print held_message_spam.message_id
252 <fifth-post>
253 >>> print held_message_spam.posted_by.displayname
254 Anne Person
255
256 >>> held_message_spam.posted_message.open()
257 >>> try:
258 ... print held_message_spam.posted_message.read()
259 ... finally:
260 ... held_message_spam.posted_message.close()
261 From: Anne © Person <anne.person@example.com>
262 To: team-one@lists.launchpad.dev
263 Subject: © Badgers!
264 Message-ID: <fifth-post>
265 Date: Fri, 01 Aug 2000 01:08:59 -0000
266 <BLANKLINE>
267 Watch out for badgers! ©
268 <BLANKLINE>
269
270 >>> held_message_spam.status
271 <DBItem PostedMessageStatus.NEW, (0) New status>
223272
=== modified file 'lib/lp/registry/interfaces/mailinglist.py'
--- lib/lp/registry/interfaces/mailinglist.py 2009-06-25 04:06:00 +0000
+++ lib/lp/registry/interfaces/mailinglist.py 2009-10-01 16:59:15 +0000
@@ -651,12 +651,12 @@
651 team.651 team.
652 """652 """
653653
654 def holdMessage(team_name, text):654 def holdMessage(team_name, bytes):
655 """Hold the message for approval though the Launchpad u/i.655 """Hold the message for approval though the Launchpad u/i.
656656
657 :param team_name: The name of the team/mailing list that this message657 :param team_name: The name of the team/mailing list that this message
658 was posted to.658 was posted to.
659 :param text: The original text of the message.659 :param bytes: The original text of the message as bytes.
660 :return: True660 :return: True
661 """661 """
662662
663663
=== modified file 'lib/lp/services/mailman/doc/create-lists.txt'
--- lib/lp/services/mailman/doc/create-lists.txt 2009-09-24 18:51:29 +0000
+++ lib/lp/services/mailman/doc/create-lists.txt 2009-10-01 16:59:15 +0000
@@ -15,8 +15,8 @@
15 >>> browser.getControl(name='field.subscriptionpolicy').displayValue = [15 >>> browser.getControl(name='field.subscriptionpolicy').displayValue = [
16 ... 'Open Team']16 ... 'Open Team']
17 >>> browser.getControl('Create').click()17 >>> browser.getControl('Create').click()
18 >>> browser.title18 >>> print browser.title
19 'ITest One in Launchpad'19 ITest One in Launchpad
2020
21To request a mailing list, No Privileges Person navigates to the 'Configure21To request a mailing list, No Privileges Person navigates to the 'Configure
22mailing list' page and registers their application for a mailing list.22mailing list' page and registers their application for a mailing list.
2323
=== modified file 'lib/lp/services/mailman/doc/postings.txt'
--- lib/lp/services/mailman/doc/postings.txt 2009-09-24 20:10:01 +0000
+++ lib/lp/services/mailman/doc/postings.txt 2009-10-01 16:59:15 +0000
@@ -899,3 +899,35 @@
899 >>> vette_watcher.wait_for_discard('quahog')899 >>> vette_watcher.wait_for_discard('quahog')
900 >>> print_message_summaries()900 >>> print_message_summaries()
901 Number of messages: 0901 Number of messages: 0
902
903
904Non-ascii messages
905==================
906
907Cate joins Launchpad and posts a message containing non-ascii characters to
908the mailing list.
909
910 >>> login('admin@canonical.com')
911 >>> cate = factory.makePersonByName('Cate')
912 >>> logout()
913 >>> transaction.commit()
914
915 >>> inject('itest-one', """\
916 ... From: Cate \xa9 Person <cperson@example.org>
917 ... To: team-one@lists.launchpad.dev
918 ... Subject: \xa9 Badgers!
919 ... Message-ID: <racoon>
920 ...
921 ... Watch out for badgers! \xa9
922 ... """)
923
924However, because Cate is not a member of the mailing list, her message gets
925held for moderator approval.
926
927 >>> vette_watcher.wait_for_hold('itest-one', 'racoon')
928 >>> print_message_summaries()
929 Number of messages: 1
930 bounces@canonical.com
931 ...
932 Itest One <noreply@launchpad.net>
933 New mailing list message requiring approval for Itest One