diff --git a/CHANGELOG.md b/CHANGELOG.md index 598286af..9188fc85 100644 --- a/CHANGELOG.md +++ b/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). diff --git a/slurmweb/apps/gateway.py b/slurmweb/apps/gateway.py index 1c30aafe..01e231b8 100644 --- a/slurmweb/apps/gateway.py +++ b/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( diff --git a/slurmweb/errors.py b/slurmweb/errors.py index 4c995219..6f320d5f 100644 --- a/slurmweb/errors.py +++ b/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 diff --git a/slurmweb/tests/assets/agent/info.json b/slurmweb/tests/assets/agent/info.json index 09069861..1efbf45d 100644 --- a/slurmweb/tests/assets/agent/info.json +++ b/slurmweb/tests/assets/agent/info.json @@ -1,4 +1,5 @@ { "cluster": "tiny", - "infrastructure": "tiny" + "infrastructure": "tiny", + "metrics": true } \ No newline at end of file diff --git a/slurmweb/tests/metrics/test_db.py b/slurmweb/tests/metrics/test_db.py index 20c7c1e5..af690ca1 100644 --- a/slurmweb/tests/metrics/test_db.py +++ b/slurmweb/tests/metrics/test_db.py @@ -25,12 +25,12 @@ 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 .*$" ): @@ -38,7 +38,7 @@ def test_request_empty_result(self, mock_requests_get): @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 .*$" ): diff --git a/slurmweb/tests/test_agent.py b/slurmweb/tests/test_agent.py index ff25dfca..50edd931 100644 --- a/slurmweb/tests/test_agent.py +++ b/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 .*$") diff --git a/slurmweb/tests/test_gateway.py b/slurmweb/tests/test_gateway.py index 6f508c47..27c3d86d 100644 --- a/slurmweb/tests/test_gateway.py +++ b/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: diff --git a/slurmweb/tests/utils.py b/slurmweb/tests/utils.py index b948da2e..03d51a3c 100644 --- a/slurmweb/tests/utils.py +++ b/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: