Use cURL-based HTTP client in diff server (#579)

This is effectively a revival of #293 (from @vbanos). The cURL HTTP client in Tornado is both faster and supports more funny business that various HTTP servers do. This might also improve some of the weird memory & SSL errors we run into in production every so often.

Last time around, we had some concerns about whether `CURLOPT_MAXFILESIZE` would be good enough to restrict the size of responses and mitigate memory issues. Happily, we now have tests around this and it does.

To continue using the simple HTTP client, set the `USE_SIMPLE_HTTP_CLIENT` environment variable. As long as the cURL client works well, though, we’ll probably eventually drop support for the simple one.

Author: Mr0grog

.env.example

@@ -29,6 +29,10 @@ export ACCESS_CONTROL_ALLOW_ORIGIN_HEADER="*"
 # Maximum diffable body size, in bytes.
 export DIFFER_MAX_BODY_SIZE='10485760' # 10 MB
 
+# Use Tornado's "simple" HTTP client to get diffable content. By default, the
+# diff server uses a cURL-based client, which is faster and more robust.
+# export USE_SIMPLE_HTTP_CLIENT='true'
+
 # The diff server does not normally validate SSL certificates when requesting
 # pages to diff. If this is set to "true", diff requests will fail if upstream
 # https:// requests have invalid certificates.

Dockerfile

@@ -3,7 +3,8 @@ FROM python:3.7-slim
 LABEL maintainer="enviroDGI@gmail.com"
 
 RUN apt-get update && apt-get install -y --no-install-recommends \
-    git gcc g++ pkg-config libxml2-dev libxslt-dev libz-dev
+    git gcc g++ pkg-config libxml2-dev libxslt-dev libz-dev \
+    libssl-dev openssl libcurl4-openssl-dev
 
 # Set the working directory to /app
 WORKDIR /app

README.md

@@ -41,19 +41,20 @@ Legacy projects that may be revisited:
    privileges to install or use it, and it won't interfere with any other
    installations of Python already on your system.)
 
-2. Install libxml2 and libxslt. (This package uses lxml, which requires your system to have the libxml2 and libxslt libraries.)
+2. Install libxml2, libxslt, and openssl. (This package uses lxml, which requires your system to have the libxml2 and libxslt libraries, and pycurl, which requires libcurl [built-in on MacOS] and openssl.)
 
     On MacOS, use Homebrew:
 
     ```sh
     brew install libxml2
     brew install libxslt
+    brew install openssl
     ```
 
     On Debian Linux:
 
     ```sh
-    apt-get install libxml2-dev libxslt-dev
+    apt-get install libxml2-dev libxslt-dev libssl-dev openssl libcurl4-openssl-dev
     ```
 
     On other systems, the packages might have slightly different names.
@@ -65,6 +66,14 @@ Legacy projects that may be revisited:
     python setup.py develop
     ```
 
+    **On MacOS,** you may need additional configuration to use the Homebrew
+    openssl. Try the following:
+
+    ```sh
+    PYCURL_SSL_LIBRARY=openssl LDFLAGS="-L/usr/local/opt/openssl/lib" CPPFLAGS="-I/usr/local/opt/openssl/include" pip install -r requirements.txt
+    python setup.py develop
+    ```
+
 4. Copy the script `.env.example` to `.env` and supply any local configuration
    info you need. (Only some of the package's functionality requires this.)
    Apply the configuration:

requirements.txt

@@ -7,6 +7,7 @@ git+https://github.com/anastasia/htmldiffer@develop
 git+https://github.com/danielballan/htmltreediff@customize
 html5-parser ~=0.4.9 --no-binary lxml
 lxml ~=4.5.2
+pycurl ~=7.43
 PyPDF2 ~=1.26.0
 sentry-sdk ~=0.17.2
 requests ~=2.24.0

web_monitoring/diff_server/server.py

@@ -8,11 +8,14 @@
 import logging
 import mimetypes
 import os
+import pycurl
 import re
 import cchardet
 import sentry_sdk
 import signal
 import sys
+from tornado.curl_httpclient import CurlAsyncHTTPClient, CurlError
+import tornado.simple_httpclient
 import tornado.httpclient
 import tornado.ioloop
 import tornado.web
@@ -82,11 +85,37 @@
     print('DIFFER_MAX_BODY_SIZE must be an integer', file=sys.stderr)
     sys.exit(1)
 
-# TODO: we'd like to support CurlAsyncHTTPClient, but we need to figure out how
-# to enforce something like max_body_size with it.
-tornado.httpclient.AsyncHTTPClient.configure(None,
+
+class LimitedCurlAsyncHTTPClient(CurlAsyncHTTPClient):
+    """
+    A customized version of Tornado's CurlAsyncHTTPClient that adds support for
+    maximum response body sizes. The API is the same as that for Tornado's
+    SimpleAsyncHTTPClient: set ``max_body_size`` to an integer representing the
+    maximum number of bytes in a response body.
+    """
+    def initialize(self, max_clients=10, defaults=None, max_body_size=None):
+        self.max_body_size = max_body_size
+        defaults = defaults or {}
+        defaults['prepare_curl_callback'] = self.prepare_curl
+        super().initialize(max_clients=max_clients, defaults=defaults)
+
+    def prepare_curl(self, curl):
+        if self.max_body_size:
+            # NOTE: cURL's docs suggest this doesn't work if the server doesn't
+            # send a Content-Length header, but it seems to do just fine in
+            # tests. ¯\_(ツ)_/¯
+            curl.setopt(pycurl.MAXFILESIZE, self.max_body_size)
+
+
+HTTP_CLIENT = LimitedCurlAsyncHTTPClient
+if os.getenv('USE_SIMPLE_HTTP_CLIENT'):
+    HTTP_CLIENT = None
+tornado.httpclient.AsyncHTTPClient.configure(HTTP_CLIENT,
                                              max_body_size=MAX_BODY_SIZE)
-client = tornado.httpclient.AsyncHTTPClient()
+
+
+def get_http_client():
+    return tornado.httpclient.AsyncHTTPClient()
 
 
 class PublicError(tornado.web.HTTPError):
@@ -329,6 +358,12 @@ async def fetch_diffable_content(self, url, expected_hash, query_params):
             with open(url[7:], 'rb') as f:
                 body = f.read()
                 response = MockResponse(url, body)
+        # Only support HTTP(S) URLs.
+        elif not url.startswith('http://') and not url.startswith('https://'):
+            raise PublicError(400,
+                              f'URL must use HTTP or HTTPS protocol: "{url}"',
+                              'Invalid URL for upstream content',
+                              extra={'url': url})
         else:
             # Include request headers defined by the query param
             # `pass_headers=HEADER_NAMES` in the upstream request. This is
@@ -344,15 +379,19 @@ async def fetch_diffable_content(self, url, expected_hash, query_params):
                         headers[header_key] = header_value
 
             try:
+                client = get_http_client()
                 response = await client.fetch(url, headers=headers,
                                               validate_cert=VALIDATE_TARGET_CERTIFICATES)
             except ValueError as error:
                 raise PublicError(400, str(error))
+            # Only raised by the simple client and not by the cURL client.
             except OSError as error:
                 raise PublicError(502,
                                   f'Could not fetch "{url}": {error}',
                                   'Could not fetch upstream content',
                                   extra={'url': url, 'cause': str(error)})
+
+            # --- SIMPLE CLIENT ERRORS ----------------------------------------
             except tornado.simple_httpclient.HTTPTimeoutError:
                 raise PublicError(504,
                                   f'Timed out while fetching "{url}"',
@@ -371,6 +410,46 @@ async def fetch_diffable_content(self, url, expected_hash, query_params):
                                   'Connection closed while fetching upstream',
                                   extra={'url': url,
                                          'max_size': client.max_body_size})
+
+            # --- CURL CLIENT ERRORS ------------------------------------------
+            except CurlError as error:
+                # Documentation for cURL error codes:
+                #   https://curl.haxx.se/libcurl/c/libcurl-errors.html
+                # PyCurl has constants named `E_*` vs. libcurl's `CURLE_*`
+                if error.errno == pycurl.E_URL_MALFORMAT:
+                    raise PublicError(400,
+                                      str(error),
+                                      'Invalid URL for cURL',
+                                      extra={'url': url})
+                # TODO: raise a nicer error from LimitedCurlAsyncHTTPClient
+                elif error.errno == pycurl.E_FILESIZE_EXCEEDED:
+                    raise PublicError(502,
+                                      f'Upstream response too big for "{url}"'
+                                      f'(max: {client.max_body_size} bytes)',
+                                      'Upstream content too big',
+                                      extra={'url': url,
+                                             'max_size': client.max_body_size})
+                elif (error.errno == pycurl.E_COULDNT_RESOLVE_PROXY
+                      or error.errno == pycurl.E_COULDNT_CONNECT
+                      or error.errno == 8  # E_WEIRD_SERVER_REPLY
+                      or error.errno == pycurl.E_REMOTE_ACCESS_DENIED
+                      or error.errno == pycurl.E_HTTP2):
+                    raise PublicError(502,
+                                      f'Could not fetch "{url}": {error}',
+                                      'Could not fetch upstream content',
+                                      extra={'url': url, 'cause': str(error)})
+                elif error.errno == pycurl.E_OPERATION_TIMEDOUT:
+                    raise PublicError(504,
+                                      f'Timed out while fetching "{url}"',
+                                      'Could not fetch upstream content',
+                                      extra={'url': url})
+                else:
+                    raise PublicError(502,
+                                      f'Unknown error fetching "{url}"',
+                                      f'Unknown error fetching upstream content: {error}',
+                                      extra={'url': url})
+
+            # --- COMMON ERRORS SUPPORTED BY ALL CLIENTS ----------------------
             except tornado.httpclient.HTTPError as error:
                 # If the response is actually coming from a web archive,
                 # allow error codes. The Memento-Datetime header indicates

web_monitoring/tests/test_diffing_server_exc_handling.py

@@ -1,3 +1,4 @@
+import asyncio
 import json
 import os
 import unittest
@@ -17,6 +18,17 @@
 from io import BytesIO
 
 
+def patch_http_client(**kwargs):
+    """
+    Create HTTP clients in the diffing server with the specified parameters
+    during this patch. Can be a function decorator or context manager.
+    """
+    def get_client():
+        return tornado.httpclient.AsyncHTTPClient(force_instance=True,
+                                                  **kwargs)
+    return patch.object(df, 'get_http_client', get_client)
+
+
 class DiffingServerTestCase(AsyncHTTPTestCase):
 
     def get_app(self):
@@ -88,7 +100,7 @@ class DiffingServerFetchTest(DiffingServerTestCase):
 
     def test_pass_headers(self):
         mock = MockAsyncHttpClient()
-        with patch.object(df, 'client', wraps=mock):
+        with patch.object(df, 'get_http_client', return_value=mock):
             mock.respond_to(r'/a$')
             mock.respond_to(r'/b$')
 
@@ -175,6 +187,18 @@ def test_not_reachable_url_b(self):
         self.assertEqual(response.code, 502)
         self.assertFalse(response.headers.get('Etag'))
 
+    @patch_http_client(defaults=dict(request_timeout=0.5))
+    def test_timeout_upstream(self):
+        async def responder(handler):
+            await asyncio.sleep(1)
+            handler.write('HELLO!'.encode('utf-8'))
+
+        with SimpleHttpServer(responder) as server:
+            response = self.fetch('/html_source_dmp?'
+                                  f'a={server.url("/whatever1")}&'
+                                  f'b={server.url("/whatever2")}')
+            assert response.code == 504
+
     def test_missing_params_caller_func(self):
         response = self.fetch('http://example.org/')
         with self.assertRaises(KeyError):
@@ -196,7 +220,7 @@ def test_accepts_errors_from_web_archives(self):
         we proceed with diffing.
         """
         mock = MockAsyncHttpClient()
-        with patch.object(df, 'client', wraps=mock):
+        with patch.object(df, 'get_http_client', return_value=mock):
             mock.respond_to(r'/error$', code=404, headers={'Memento-Datetime': 'Tue Sep 25 2018 03:38:50'})
             mock.respond_to(r'/success$')
 
@@ -307,34 +331,31 @@ def test_validates_bad_hashes(self):
 
 
 class DiffingServerResponseSizeTest(DiffingServerTestCase):
+    @patch_http_client(max_body_size=100 * 1024)
     def test_succeeds_if_response_is_small_enough(self):
         async def responder(handler):
             text = (80 * 1024) * 'x'
             handler.write(text.encode('utf-8'))
 
-        client = tornado.httpclient.AsyncHTTPClient(force_instance=True,
-                                                    max_body_size=100 * 1024)
-        with patch.object(df, 'client', client):
-            with SimpleHttpServer(responder) as server:
-                response = self.fetch('/html_source_dmp?'
-                                      f'a={server.url("/whatever1")}&'
-                                      f'b={server.url("/whatever2")}')
-                assert response.code == 200
+        with SimpleHttpServer(responder) as server:
+            response = self.fetch('/html_source_dmp?'
+                                  f'a={server.url("/whatever1")}&'
+                                  f'b={server.url("/whatever2")}')
+            assert response.code == 200
 
+    @patch_http_client(max_body_size=100 * 1024)
     def test_stops_if_response_is_too_big(self):
         async def responder(handler):
             text = (110 * 1024) * 'x'
             handler.write(text.encode('utf-8'))
 
-        client = tornado.httpclient.AsyncHTTPClient(force_instance=True,
-                                                    max_body_size=100 * 1024)
-        with patch.object(df, 'client', client):
-            with SimpleHttpServer(responder) as server:
-                response = self.fetch('/html_source_dmp?'
-                                      f'a={server.url("/whatever1")}&'
-                                      f'b={server.url("/whatever2")}')
-                assert response.code == 502
+        with SimpleHttpServer(responder) as server:
+            response = self.fetch('/html_source_dmp?'
+                                  f'a={server.url("/whatever1")}&'
+                                  f'b={server.url("/whatever2")}')
+            assert response.code == 502
 
+    @patch_http_client(max_body_size=100 * 1024)
     def test_stops_reading_early_when_content_length_is_a_lie(self):
         async def responder(handler):
             # Tornado tries to be careful and prevent us from sending more
@@ -347,26 +368,23 @@ async def responder(handler):
             text = (110 * 1024) * 'x'
             handler.write(text.encode('utf-8'))
 
-        client = tornado.httpclient.AsyncHTTPClient(force_instance=True,
-                                                    max_body_size=100 * 1024)
-        with patch.object(df, 'client', client):
-            with SimpleHttpServer(responder) as server:
-                response = self.fetch('/html_source_dmp?'
-                                      f'a={server.url("/whatever1")}&'
-                                      f'b={server.url("/whatever2")}')
-                # Even though the response was longer than the max_body_size,
-                # the client should have stopped reading when it hit the number
-                # of bytes set in the Content-Length, header, which is less
-                # than the client's limit. So what should actually happen is a
-                # successful diff of the first <Content-Length> bytes of the
-                # responses.
-                assert response.code == 200
-                # The responses should be the same, and should only be
-                # <Content-Length> bytes long (all our chars are basic ASCII
-                # in this case, so len(bytes) == len(characters)).
-                result = json.loads(response.body)
-                assert result['change_count'] == 0
-                assert len(result['diff'][0][1]) == 1024
+        with SimpleHttpServer(responder) as server:
+            response = self.fetch('/html_source_dmp?'
+                                  f'a={server.url("/whatever1")}&'
+                                  f'b={server.url("/whatever2")}')
+            # Even though the response was longer than the max_body_size,
+            # the client should have stopped reading when it hit the number
+            # of bytes set in the Content-Length, header, which is less
+            # than the client's limit. So what should actually happen is a
+            # successful diff of the first <Content-Length> bytes of the
+            # responses.
+            assert response.code == 200
+            # The responses should be the same, and should only be
+            # <Content-Length> bytes long (all our chars are basic ASCII
+            # in this case, so len(bytes) == len(characters)).
+            result = json.loads(response.body)
+            assert result['change_count'] == 0
+            assert len(result['diff'][0][1]) == 1024
 
 
 def mock_diffing_method(c_body):