launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/webhook-proxy-errors into lp:launchpad.

Commit message:
Turn X-Squid-Error into a human-readable connection_error.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/webhook-proxy-errors/+merge/268865

Turn X-Squid-Error into a human-readable connection_error.

This works well for HTTP connections, but HTTPS will require some monkeypatching of httplib._tunnel later on.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/webhook-proxy-errors into lp:launchpad.

=== modified file 'lib/lp/services/webhooks/client.py'
--- lib/lp/services/webhooks/client.py	2015-08-10 08:09:09 +0000
+++ lib/lp/services/webhooks/client.py	2015-08-24 01:23:47 +0000
@@ -18,6 +18,24 @@
 from lp.services.webhooks.interfaces import IWebhookClient
 
 
+SQUID_ERROR_MESSAGES = {
+    "ERR_ACCESS_DENIED": "URL not allowed",
+    "ERR_READ_TIMEOUT": "Connection read timeout",
+    "ERR_LIFETIME_EXP": "Connection lifetime expired",
+    "ERR_READ_ERROR": "Connection read error",
+    "ERR_WRITE_ERROR": "Connection write error",
+    "ERR_CONNECT_FAIL": "Connection failed",
+    "ERR_SOCKET_FAILURE": "Connection failed",
+    "ERR_DNS_FAIL": "DNS lookup failed",
+    "ERR_TOO_BIG": "HTTP request or reply too large",
+    "ERR_INVALID_RESP": "HTTP response invalid",
+    "ERR_INVALID_REQ": "HTTP request invalid",
+    "ERR_UNSUP_REQ": "HTTP request unsupported",
+    "ERR_INVALID_URL": "HTTP URL invalid",
+    "ERR_ZERO_SIZE_OBJECT": "HTTP response empty",
+    }
+
+
 def create_request(user_agent, secret, delivery_id, event_type, payload):
     body = json.dumps(payload)
     headers = {
@@ -67,11 +85,23 @@
             }
         try:
             resp = session.send(preq, proxies=proxies, timeout=timeout)
+        except requests.ConnectionError as e:
+            result['connection_error'] = str(e)
+            return result
+        # If there was a request error, try to interpret any Squid
+        # error.
+        squid_error = resp.headers.get('X-Squid-Error')
+        if resp.status_code < 200 or resp.status_code > 299 and squid_error:
+            human_readable = SQUID_ERROR_MESSAGES.get(
+                squid_error.split(' ', 1)[0])
+            if human_readable:
+                result['connection_error'] = human_readable
+            else:
+                result['connection_error'] = 'Proxy error: %s' % squid_error
+        else:
             result['response'] = {
                 'status_code': resp.status_code,
                 'headers': dict(resp.headers),
                 'body': resp.content,
                 }
-        except requests.ConnectionError as e:
-            result['connection_error'] = str(e)
         return result

=== modified file 'lib/lp/services/webhooks/tests/test_job.py'
--- lib/lp/services/webhooks/tests/test_job.py	2015-08-14 06:11:00 +0000
+++ lib/lp/services/webhooks/tests/test_job.py	2015-08-24 01:23:47 +0000
@@ -137,7 +137,7 @@
 class TestWebhookClient(TestCase):
     """Tests for `WebhookClient`."""
 
-    def sendToWebhook(self, response_status=200, raises=None):
+    def sendToWebhook(self, response_status=200, raises=None, headers=None):
         reqs = []
 
         @urlmatch(netloc='example.com')
@@ -145,7 +145,9 @@
             if raises:
                 raise raises
             reqs.append(request)
-            return {'status_code': response_status, 'content': 'Content'}
+            return {
+                'status_code': response_status, 'content': 'Content',
+                'headers': headers}
 
         with HTTMock(endpoint_mock):
             result = WebhookClient().deliver(
@@ -211,6 +213,32 @@
                 }))
         self.assertEqual([], reqs)
 
+    def test_proxy_error_known(self):
+        # Squid error headers are interpreted to populate
+        # connection_error.
+        [request], result = self.sendToWebhook(
+            response_status=403,
+            headers={"X-Squid-Error": "ERR_ACCESS_DENIED 0"})
+        self.assertThat(
+            result,
+            MatchesDict({
+                'request': self.request_matcher,
+                'connection_error': Equals('URL not allowed'),
+                }))
+
+    def test_proxy_error_unknown(self):
+        # Squid errors that don't have a human-readable mapping are
+        # included verbatim.
+        [request], result = self.sendToWebhook(
+            response_status=403,
+            headers={"X-Squid-Error": "ERR_BORKED 1234"})
+        self.assertThat(
+            result,
+            MatchesDict({
+                'request': self.request_matcher,
+                'connection_error': Equals('Proxy error: ERR_BORKED 1234'),
+                }))
+
 
 class MockWebhookClient(WebhookClient):