Handle empty response from APM Server's config endpoint more gracefully

also, add a bunch of tests that went forgotten in the original PR

fixes #562
closes #563

Author: beniwohli

CHANGELOG.md

@@ -1,5 +1,11 @@
 # Changelog
 
+## Unreleased
+[Check the diff](https://github.com/elastic/apm-agent-python/compare/v5.1.0...master)
+
+### Bugfixes
+ * fixed an issue with empty responses from APM Server's config endpoint (#562, #563)
+
 ## v5.1.0 
 [Check the diff](https://github.com/elastic/apm-agent-python/compare/v5.0.0...v5.1.0)
 

elasticapm/transport/http.py

@@ -106,6 +106,20 @@ def send(self, data):
                 response.close()
 
     def get_config(self, current_version=None, keys=None):
+        """
+        Gets configuration from a remote APM Server
+
+        :param current_version: version of the current configuration
+        :param keys: a JSON-serializable dict to identify this instance, e.g.
+                {
+                    "service": {
+                        "name": "foo",
+                        "environment": "bar"
+                    }
+                }
+        :return: a three-tuple of new version, config dictionary and validity in seconds.
+                 Any element of the tuple can be None.
+        """
         url = self._config_url
         data = json_encoder.dumps(keys).encode("utf-8")
         headers = self._headers.copy()
@@ -124,14 +138,18 @@ def get_config(self, current_version=None, keys=None):
             try:
                 max_age = int(next(re.finditer(r"max-age=(\d+)", response.headers["Cache-Control"])).groups()[0])
             except StopIteration:
-                logger.debug("Could not parse Cache-Control header: %s", response["Cache-Control"])
+                logger.debug("Could not parse Cache-Control header: %s", response.headers["Cache-Control"])
         if response.status == 304:
             # config is unchanged, return
             logger.debug("Configuration unchanged")
             return current_version, None, max_age
         elif response.status >= 400:
             return None, None, max_age
 
+        if not body:
+            logger.debug("APM Server answered with empty body and status code %s", response.status)
+            return current_version, None, max_age
+
         return response.headers.get("Etag"), json_encoder.loads(body.decode("utf-8")), max_age
 
     @property

elasticapm/transport/http_base.py

@@ -75,15 +75,16 @@ def get_config(self, current_version=None, keys=None):
         """
         Gets configuration from a remote APM Server
 
+        :param current_version: version of the current configuration
         :param keys: a JSON-serializable dict to identify this instance, e.g.
                 {
                     "service": {
                         "name": "foo",
                         "environment": "bar"
                     }
                 }
-        :param current_version: version of the current configuration
-        :return: dictionary or None
+        :return: a three-tuple of new version, config dictionary and validity in seconds.
+                 Any element of the tuple can be None.
         """
         raise NotImplementedError()
 

tests/transports/test_urllib3.py

@@ -37,6 +37,7 @@
 from urllib3.exceptions import MaxRetryError, TimeoutError
 from urllib3_mock import Responses
 
+from elasticapm.conf import constants
 from elasticapm.transport.base import TransportException
 from elasticapm.transport.http import Transport
 from elasticapm.utils import compat
@@ -215,8 +216,99 @@ def test_ssl_cert_pinning_fails(waiting_httpsserver):
     transport = Transport(
         url, server_cert=os.path.join(os.path.dirname(__file__), "wrong_cert.pem"), verify_server_cert=True
     )
-    with pytest.raises(TransportException) as exc_info:
-        transport.send(compat.b("x"))
-    transport.close()
+    try:
+        with pytest.raises(TransportException) as exc_info:
+            transport.send(compat.b("x"))
+    finally:
+        transport.close()
 
     assert "Fingerprints did not match" in exc_info.value.args[0]
+
+
+def test_config_url():
+    transport = Transport("http://example.com/" + constants.EVENTS_API_PATH)
+    try:
+        assert transport._config_url == "http://example.com/" + constants.AGENT_CONFIG_PATH
+    finally:
+        transport.close()
+
+
+def test_get_config(waiting_httpserver):
+    waiting_httpserver.serve_content(
+        code=200, content=b'{"x": "y"}', headers={"Cache-Control": "max-age=5", "Etag": "2"}
+    )
+    url = waiting_httpserver.url
+    transport = Transport(url + "/" + constants.EVENTS_API_PATH)
+    try:
+        version, data, max_age = transport.get_config("1", {})
+        assert version == "2"
+        assert data == {"x": "y"}
+        assert max_age == 5
+    finally:
+        transport.close()
+
+
+@mock.patch("urllib3.poolmanager.PoolManager.urlopen")
+def test_get_config_handle_exception(mock_urlopen, caplog):
+    transport = Transport("http://example.com/" + constants.EVENTS_API_PATH)
+    mock_urlopen.side_effect = urllib3.exceptions.RequestError(transport.http, "http://example.com/", "boom")
+    try:
+        with caplog.at_level("DEBUG", "elasticapm.transport.http"):
+            version, data, max_age = transport.get_config("1", {})
+        assert version == "1"
+        assert max_age == 300
+        record = caplog.records[-1]
+        assert "HTTP error" in record.msg
+    finally:
+        transport.close()
+
+
+def test_get_config_cache_headers_304(waiting_httpserver, caplog):
+    waiting_httpserver.serve_content(code=304, content=b"", headers={"Cache-Control": "max-age=5"})
+    url = waiting_httpserver.url
+    transport = Transport(url + "/" + constants.EVENTS_API_PATH)
+    try:
+        with caplog.at_level("DEBUG", "elasticapm.transport.http"):
+            version, data, max_age = transport.get_config("1", {})
+        assert waiting_httpserver.requests[0].headers["If-None-Match"] == "1"
+        assert version == "1"
+        assert data is None
+        assert max_age == 5
+        record = caplog.records[-1]
+        assert "Configuration unchanged" in record.msg
+    finally:
+        transport.close()
+
+
+def test_get_config_bad_cache_control_header(waiting_httpserver, caplog):
+    waiting_httpserver.serve_content(
+        code=200, content=b'{"x": "y"}', headers={"Cache-Control": "max-age=fifty", "Etag": "2"}
+    )
+    url = waiting_httpserver.url
+    transport = Transport(url + "/" + constants.EVENTS_API_PATH)
+    try:
+        with caplog.at_level("DEBUG", "elasticapm.transport.http"):
+            version, data, max_age = transport.get_config("1", {})
+        assert version == "2"
+        assert data == {"x": "y"}
+        assert max_age == 300
+        record = caplog.records[-1]
+        assert record.message == "Could not parse Cache-Control header: max-age=fifty"
+    finally:
+        transport.close()
+
+
+def test_get_config_empty_response(waiting_httpserver, caplog):
+    waiting_httpserver.serve_content(code=200, content=b"", headers={"Cache-Control": "max-age=5"})
+    url = waiting_httpserver.url
+    transport = Transport(url + "/" + constants.EVENTS_API_PATH)
+    try:
+        with caplog.at_level("DEBUG", "elasticapm.transport.http"):
+            version, data, max_age = transport.get_config("1", {})
+        assert version == "1"
+        assert data is None
+        assert max_age == 5
+        record = caplog.records[-1]
+        assert record.message == "APM Server answered with empty body and status code 200"
+    finally:
+        transport.close()