launchpad-reviewers team mailing list archive

[Gary Poster <gary.poster@xxxxxxxxxxxxx>]

Gary Poster has proposed merging lp:~gary/launchpad/bug492314 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #492314 in Launchpad itself: "+opstats OOPSes for logged in users"
  https://bugs.launchpad.net/launchpad/+bug/492314

For more details, see:
https://code.launchpad.net/~gary/launchpad/bug492314/+merge/63946

This change makes it possible to see +opstats and +haproxy even if you are authenticated.  The only reason we care about this is because we do not want an oops: normally our robots poll these pages as unauthenticated requests.

This is the simplest possible change.  I toyed with making a method on the request to consolidate the identification of special "no-db" requests, but decided simplicity was a good approach as long as there are no more than two instances of the same small check (see also LaunchpadDatabasePolicyFactory in lib/canonical/launchpad/webapp/dbpolicy.py).  We can abstract more when we move to three such instances.

The critical bug to which this is linked only refers to +opstats; however, the newer +haproxy page has the same problem, so I fixed it as well.

lin is happy.

Thank you!
-- 
https://code.launchpad.net/~gary/launchpad/bug492314/+merge/63946
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gary/launchpad/bug492314 into lp:launchpad.

=== modified file 'lib/canonical/launchpad/pagetests/standalone/xx-opstats.txt'
--- lib/canonical/launchpad/pagetests/standalone/xx-opstats.txt	2011-02-25 04:19:04 +0000
+++ lib/canonical/launchpad/pagetests/standalone/xx-opstats.txt	2011-06-09 01:25:14 +0000
@@ -1,4 +1,5 @@
-= Operational Statistics and Metrics =
+Operational Statistics and Metrics
+==================================
 
 We make Zope 3 give us real time statistics about Launchpad's operation.
 We can access them via XML-RPC:
@@ -25,7 +26,8 @@
     ...     reset()
     ...
 
-== Number of requests and XML-RPC requests ==
+Number of requests and XML-RPC requests
+---------------------------------------
 
 Even though XML-RPC requests are technically HTTP requests, we do not
 count them as such. Note that the call to obtain statistics will increment
@@ -59,7 +61,8 @@
     requests: 1
     xml-rpc requests: 1
 
-== Number of HTTP requests and success codes ==
+Number of HTTP requests and success codes
+-----------------------------------------
 
     >>> output = http("GET / HTTP/1.1\nHost: bugs.launchpad.dev\n")
     >>> output.getStatus()
@@ -69,7 +72,8 @@
     http requests: 1
     requests: 1
 
-== Number of 404s ==
+Number of 404s
+--------------
 
 Note that retries is incremented too. As per the standard Launchpad
 database policy, this request first uses the slave DB. The requested
@@ -86,7 +90,8 @@
     requests: 1
     retries: 1
 
-== Number of 500 Internal Server Errors (unhandled exceptions) ==
+Number of 500 Internal Server Errors (unhandled exceptions)
+-----------------------------------------------------------
 
 This is normally the number of OOPS pages displayed to the user, but
 may also include the odd case where the OOPS system has failed and a
@@ -129,7 +134,8 @@
     http requests: 1
     requests: 1
 
-== Number of XML-RPC Faults ==
+Number of XML-RPC Faults
+------------------------
 
     >>> try:
     ...     opstats = lp_xmlrpc.invalid() # XXX: Need a HTTP test too
@@ -142,7 +148,8 @@
     xml-rpc requests: 1
 
 
-== Number of soft timeouts ==
+Number of soft timeouts
+-----------------------
 
     >>> from canonical.config import config
     >>> test_data = dedent("""
@@ -162,7 +169,8 @@
     requests: 1
     soft timeouts: 1
 
-== Number of Timeouts ==
+Number of Timeouts
+------------------
 
 We can't reliably track this using the 503 response code as other
 Launchpad code may well return this status and an XML-RPC request may
@@ -190,7 +198,8 @@
     timeouts: 1
 
 
-== HTTP access for Cricket ==
+HTTP access for Cricket
+-----------------------
 
 Stats can also be retrieved via HTTP in cricket-graph format:
 
@@ -219,13 +228,13 @@
     xmlrpc_requests:0@...
     <BLANKLINE>
 
-== No DB access required ==
+No DB access required
+---------------------
 
-Accessing the opstats page as an anonymous user will make no database
-queries. This is important to make it as reliable as possible since we
-use this page for monitoring. Because of this property, the load
-balancers also use this page to determine if a Launchpad instance is
-responsive.
+Accessing the opstats page will make no database queries. This is important to
+make it as reliable as possible since we use this page for monitoring. Because
+of this property, the load balancers also use this page to determine if a
+Launchpad instance is responsive.
 
 To confirm this, we first point all our database connection information
 to somewhere that doesn't exist.
@@ -257,6 +266,20 @@
     <BLANKLINE>
     1XXs:0@...
 
+This is also true if we are provide authentication.
+
+    >>> print http(r"""
+    ... GET /+opstats HTTP/1.1
+    ... Host: launchpad.dev
+    ... Authorization: Basic Zm9vLmJhckBjYW5vbmljYWwuY29tOnRlc3Q=
+    ... """)
+    HTTP/1.1 200 Ok
+    ...
+    Content-Type: text/plain; charset=US-ASCII
+    ...
+    <BLANKLINE>
+    1XXs:0@...
+
 But our database connections are broken.
 
     >>> from canonical.launchpad.interfaces.lpstorm import IStore
@@ -271,4 +294,3 @@
 
     >>> IStore(Person).find(Person, name='janitor').one().name
     u'janitor'
-

=== modified file 'lib/canonical/launchpad/webapp/publication.py'
--- lib/canonical/launchpad/webapp/publication.py	2011-05-27 21:12:25 +0000
+++ lib/canonical/launchpad/webapp/publication.py	2011-06-09 01:25:14 +0000
@@ -330,7 +330,17 @@
         personless account, return the unauthenticated principal.
         """
         auth_utility = getUtility(IPlacelessAuthUtility)
-        principal = auth_utility.authenticate(request)
+        principal = None
+        # +opstats and +haproxy are status URLs that must not query the DB at
+        # all.  This is enforced (see
+        # lib/canonical/launchpad/webapp/dbpolicy.py). If the request is for
+        # one of those two pages, don't even try to authenticate, because we
+        # may fail.  We haven't traversed yet, so we have to sniff the request
+        # this way.  Even though PATH_INFO is always present in real requests,
+        # we need to tread carefully (``get``) because of test requests in our
+        # automated tests.
+        if request.get('PATH_INFO') not in [u'/+opstats', u'/+haproxy']:
+            principal = auth_utility.authenticate(request)
         if principal is None or principal.person is None:
             # This is either an unauthenticated user or a user who
             # authenticated on our OpenID server using a personless account.

=== modified file 'lib/canonical/launchpad/webapp/tests/test_haproxy.py'
--- lib/canonical/launchpad/webapp/tests/test_haproxy.py	2011-02-18 18:12:43 +0000
+++ lib/canonical/launchpad/webapp/tests/test_haproxy.py	2011-06-09 01:25:14 +0000
@@ -35,6 +35,15 @@
         result = self.http(u'GET /+haproxy HTTP/1.0', handle_errors=False)
         self.assertEquals(200, result.getStatus())
 
+    def test_authenticated_HAProxyStatusView_works(self):
+        # We don't use authenticated requests, but this keeps us from
+        # generating oopses.
+        result = self.http(
+            u'GET /+haproxy HTTP/1.0\n'
+            u'Authorization: Basic Zm9vLmJhckBjYW5vbmljYWwuY29tOnRlc3Q=\n',
+            handle_errors=False)
+        self.assertEquals(200, result.getStatus())
+
     def test_HAProxyStatusView_going_down_returns_500(self):
         haproxy.set_going_down_flag(True)
         result = self.http(u'GET /+haproxy HTTP/1.0', handle_errors=False)
@@ -61,4 +70,3 @@
         haproxy.set_going_down_flag(True)
         result = self.http(u'GET /+haproxy HTTP/1.0', handle_errors=False)
         self.assertEquals(499, result.getStatus())
-