Fix AttributeError on requests.exception.JSONDecodeError

CHANGELOG.md

@@ -76,6 +76,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   - Fix compatibility issue with Requests >= 2.32.2 (#350).
   - Return HTTP/404 not found with meaningful error message when requesting
     unexisting node.
+- gateway:
+  - Catch generic `requests.exceptions.RequestException` when retrieving
+    information from agents to avoid `AttributeError` with more specific
+    exceptions on old versions on _Requests_ library (#391).
+  - Catch `JSONDecodeError` from _simpleson_ external library and _json_
+    standard library module not managed by Requests < 2.27.
 - frontend: Update dependencies to fix CVE-2024-45812 and CVE-2024-45811 (vite),
   CVE-2024-47068 (rollup).
 

slurmweb/apps/gateway.py

@@ -14,7 +14,11 @@
 from . import SlurmwebWebApp
 from ..views import SlurmwebAppRoute
 from ..views import gateway as views
-from ..errors import SlurmwebConfigurationError, SlurmwebAgentError
+from ..errors import (
+    SlurmwebConfigurationError,
+    SlurmwebCompatJSONDecodeError,
+    SlurmwebAgentError,
+)
 
 logger = logging.getLogger(__name__)
 
@@ -85,8 +89,8 @@ def agents(self):
                 logger.info("Retrieving info from agent at url %s", url.geturl())
                 agent = self._agent_info(url.geturl())
             except (
-                requests.exceptions.ConnectionError,
-                requests.exceptions.JSONDecodeError,
+                requests.exceptions.RequestException,
+                SlurmwebCompatJSONDecodeError,
                 SlurmwebAgentError,
             ) as err:
                 logger.error(

slurmweb/errors.py

@@ -27,3 +27,19 @@ class SlurmwebCacheError(Exception):
 
 class SlurmwebMetricsDBError(Exception):
     pass
+
+
+# Alias JSONDecodeError from simplejson external library and json standard library
+# module to catch generically the error raised with Requests < 2.27 on old systems in
+# presence of unexepected non-JSON responses.
+#
+# This is not needed with Requests >= 2.27 where the same logic is implemented with
+# requests.exceptions.JSONDecodeError wildcard exception. For reference, see:
+# https://github.com/psf/requests/pull/5856
+
+try:
+    from simplejson import JSONDecodeError
+except ImportError:
+    from json import JSONDecodeError
+
+SlurmwebCompatJSONDecodeError = JSONDecodeError

slurmweb/tests/assets/agent/info.json

@@ -1,4 +1,5 @@
 {
   "cluster": "tiny",
-  "infrastructure": "tiny"
+  "infrastructure": "tiny",
+  "metrics": true
 }

slurmweb/tests/metrics/test_db.py

@@ -25,20 +25,20 @@ def setUp(self):
 
     @mock.patch("slurmweb.metrics.db.requests.get")
     def test_request(self, mock_requests_get):
-        mock_requests_get.return_value = mock_prometheus_response("nodes-hour")
+        _, mock_requests_get.return_value = mock_prometheus_response("nodes-hour")
         self.db.request("nodes", "hour")
 
     @mock.patch("slurmweb.metrics.db.requests.get")
     def test_request_empty_result(self, mock_requests_get):
-        mock_requests_get.return_value = mock_prometheus_response("unknown-metric")
+        _, mock_requests_get.return_value = mock_prometheus_response("unknown-metric")
         with self.assertRaisesRegex(
             SlurmwebMetricsDBError, "^Empty result for query .*$"
         ):
             self.db.request("nodes", "hour")
 
     @mock.patch("slurmweb.metrics.db.requests.get")
     def test_request_unknown_path(self, mock_requests_get):
-        mock_requests_get.return_value = mock_prometheus_response("unknown-path")
+        _, mock_requests_get.return_value = mock_prometheus_response("unknown-path")
         with self.assertRaisesRegex(
             SlurmwebMetricsDBError, "^Prometheus error for query .*$"
         ):

slurmweb/tests/test_agent.py

@@ -803,7 +803,7 @@ def test_request_metrics_error(self):
 
     @mock.patch("slurmweb.metrics.db.requests.get")
     def test_request_metrics_nodes(self, mock_requests_get):
-        mock_requests_get.return_value = mock_prometheus_response("nodes-hour")
+        _, mock_requests_get.return_value = mock_prometheus_response("nodes-hour")
         response = self.client.get(f"/v{get_version()}/metrics/nodes")
         self.assertEqual(response.status_code, 200)
         self.assertCountEqual(
@@ -813,7 +813,7 @@ def test_request_metrics_nodes(self, mock_requests_get):
 
     @mock.patch("slurmweb.metrics.db.requests.get")
     def test_request_metrics_cores(self, mock_requests_get):
-        mock_requests_get.return_value = mock_prometheus_response("cores-hour")
+        _, mock_requests_get.return_value = mock_prometheus_response("cores-hour")
         response = self.client.get(f"/v{get_version()}/metrics/cores")
         self.assertEqual(response.status_code, 200)
         self.assertCountEqual(
@@ -823,7 +823,7 @@ def test_request_metrics_cores(self, mock_requests_get):
 
     @mock.patch("slurmweb.metrics.db.requests.get")
     def test_request_metrics_jobs(self, mock_requests_get):
-        mock_requests_get.return_value = mock_prometheus_response("jobs-hour")
+        _, mock_requests_get.return_value = mock_prometheus_response("jobs-hour")
         response = self.client.get(f"/v{get_version()}/metrics/jobs")
         self.assertEqual(response.status_code, 200)
         self.assertCountEqual(
@@ -896,7 +896,7 @@ def test_request_metrics_jobs_denied(self):
 
     @mock.patch("slurmweb.metrics.db.requests.get")
     def test_request_metrics_unexpected(self, mock_requests_get):
-        mock_requests_get.return_value = mock_prometheus_response("unknown-metric")
+        _, mock_requests_get.return_value = mock_prometheus_response("unknown-metric")
         response = self.client.get(f"/v{get_version()}/metrics/jobs")
         self.assertEqual(response.status_code, 500)
         self.assertRegex(response.json["description"], "^Empty result for query .*$")

slurmweb/tests/test_gateway.py

@@ -5,16 +5,19 @@
 # SPDX-License-Identifier: GPL-3.0-or-later
 
 import unittest
+from unittest import mock
 import tempfile
 import os
 import sys
 import shutil
 
+import requests
+
 from slurmweb.version import get_version
 from slurmweb.apps import SlurmwebConfSeed
-from slurmweb.apps.gateway import SlurmwebAppGateway
+from slurmweb.apps.gateway import SlurmwebAppGateway, SlurmwebAgent
 
-from .utils import SlurmwebCustomTestResponse
+from .utils import SlurmwebCustomTestResponse, mock_agent_response, fake_text_response
 
 CONF = """
 [agents]
@@ -25,8 +28,9 @@
 """
 
 
-class TestGateway(unittest.TestCase):
-    def setUp(self):
+class TestGatewayBase(unittest.TestCase):
+
+    def setup_app(self):
         # Generate JWT signing key
         key = tempfile.NamedTemporaryFile(mode="w+")
         key.write("hey")
@@ -60,6 +64,93 @@ def setUp(self):
                 "TESTING": True,
             }
         )
+
+
+class TestGatewayApp(TestGatewayBase):
+    def setUp(self):
+        self.setup_app()
+
+    @mock.patch("slurmweb.apps.gateway.requests.get")
+    def test_agents(self, mock_requests_get):
+        agent_info, mock_requests_get.return_value = mock_agent_response("info")
+        agents = self.app.agents
+        # Check presence of cluster name returned by agent in agents dict property.
+        self.assertIn(agent_info["cluster"], agents)
+        # Check SlurmwebAgent object is instanciated with all its attributes.
+        agent = agents[agent_info["cluster"]]
+        self.assertIsInstance(agent, SlurmwebAgent)
+        self.assertEqual(len(vars(agent)), 4)
+        self.assertEqual(agent.cluster, agent_info["cluster"])
+        self.assertEqual(agent.infrastructure, agent_info["infrastructure"])
+        self.assertEqual(agent.metrics, agent_info["metrics"])
+        self.assertEqual(agent.url, self.app.settings.agents.url[0].geturl())
+
+    @mock.patch("slurmweb.apps.gateway.requests.get")
+    def test_agents_missing_key(self, mock_requests_get):
+        agent_info, mock_requests_get.return_value = mock_agent_response(
+            "info", remove_key="infrastructure"
+        )
+        with self.assertLogs("slurmweb", level="ERROR") as cm:
+            agents = self.app.agents
+        self.assertEqual(agents, {})
+        self.assertEqual(
+            cm.output,
+            [
+                "ERROR:slurmweb.apps.gateway:Unable to retrieve agent info from url "
+                "http://localhost: [SlurmwebAgentError] Unable to retrieve cluster "
+                "name from agent"
+            ],
+        )
+
+    @mock.patch("slurmweb.apps.gateway.requests.get")
+    def test_agents_json_error(self, mock_requests_get):
+        _, mock_requests_get.return_value = fake_text_response()
+        with self.assertLogs("slurmweb", level="ERROR") as cm:
+            agents = self.app.agents
+        self.assertEqual(agents, {})
+        self.assertEqual(
+            cm.output,
+            [
+                "ERROR:slurmweb.apps.gateway:Unable to retrieve agent info from url "
+                "http://localhost: [JSONDecodeError] Expecting value: line 1 column 1 "
+                "(char 0)"
+            ],
+        )
+
+    @mock.patch("slurmweb.apps.gateway.requests.get")
+    def test_agents_ssl_error(self, mock_requests_get):
+        mock_requests_get.side_effect = requests.exceptions.SSLError("fake SSL error")
+        with self.assertLogs("slurmweb", level="ERROR") as cm:
+            agents = self.app.agents
+        self.assertEqual(agents, {})
+        self.assertEqual(
+            cm.output,
+            [
+                "ERROR:slurmweb.apps.gateway:Unable to retrieve agent info from url "
+                "http://localhost: [SSLError] fake SSL error"
+            ],
+        )
+
+    @mock.patch("slurmweb.apps.gateway.requests.get")
+    def test_agents_connection_error(self, mock_requests_get):
+        mock_requests_get.side_effect = requests.exceptions.ConnectionError(
+            "fake connection error"
+        )
+        with self.assertLogs("slurmweb", level="ERROR") as cm:
+            agents = self.app.agents
+        self.assertEqual(agents, {})
+        self.assertEqual(
+            cm.output,
+            [
+                "ERROR:slurmweb.apps.gateway:Unable to retrieve agent info from url "
+                "http://localhost: [ConnectionError] fake connection error"
+            ],
+        )
+
+
+class TestGatewayViews(TestGatewayBase):
+    def setUp(self):
+        self.setup_app()
         # On Python 3.6, use custom test response class to backport text property of
         # werkzeug.test.TestResponse in werkzeug 2.1.
         if sys.version_info.major == 3 and sys.version_info.minor <= 7:

slurmweb/tests/utils.py

@@ -107,23 +107,41 @@ def text(self):
         return self.get_data(as_text=True)
 
 
-def mock_prometheus_response(asset_name):
-    """Return mocked requests Response corresponding to the given Prometheus asset."""
-    with open(ASSETS / "prometheus/status.json") as fh:
+def fake_text_response():
+    """Return a real requests.Response() initialized with fake text content, designed
+    to fail when parsed in JSON."""
+    response = requests.Response()
+    text = "fake content"
+    response.url = "/mocked/query"
+    response.status_code = 200
+    response.headers = {"content-type": "text/plain"}
+    response._content = text.encode()
+    return text, response
+
+
+def mock_component_response(component, asset_name, remove_key=None):
+    """Return mocked requests Response corresponding to the given component asset."""
+    with open(ASSETS / component / "status.json") as fh:
         requests_statuses = json.load(fh)
 
     if asset_name not in requests_statuses:
         warnings.warn(
-            f"Unable to find asset {asset_name} in Prometheus requests status file"
+            f"Unable to find asset {asset_name} in {component} requests status file"
         )
         raise SlurmwebAssetUnavailable()
 
     is_json = True
     if requests_statuses[asset_name]["content-type"] == "application/json":
-        asset = load_json_asset(f"prometheus/{asset_name}.json")
+        asset = load_json_asset(f"{component}/{asset_name}.json")
     else:
         is_json = False
-        asset = load_asset(f"prometheus/{asset_name}.txt")
+        asset = load_asset(f"{component}/{asset_name}.txt")
+
+    # Remove specific key from asset, if JSON asset and key to remove is specified. This
+    # is useful to test some error case.
+    if is_json and remove_key:
+        del asset[remove_key]
+
     response = mock.create_autospec(requests.Response)
     response.url = "/mocked/query"
     response.status_code = requests_statuses[asset_name]["status"]
@@ -133,7 +151,17 @@ def mock_prometheus_response(asset_name):
     else:
         response.text = mock.PropertyMock(return_value=asset)
 
-    return response
+    return asset, response
+
+
+def mock_agent_response(asset_name, remove_key=None):
+    """Return mocked requests Response corresponding to the given agent asset."""
+    return mock_component_response("agent", asset_name, remove_key)
+
+
+def mock_prometheus_response(asset_name):
+    """Return mocked requests Response corresponding to the given Prometheus asset."""
+    return mock_component_response("prometheus", asset_name)
 
 
 class RemoveActionInPolicy: