From afeea5b5b891027d14d76944d6b2029f90d90360 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20=C4=8Cerm=C3=A1k?= Date: Tue, 26 Mar 2024 17:46:56 +0100 Subject: [PATCH] Properly handle errors and use consistent content type in logs endpoints --- supervisor/api/__init__.py | 7 ++++--- supervisor/api/host.py | 4 ++-- supervisor/api/supervisor.py | 6 +++--- supervisor/api/utils.py | 32 ++++++++++++++++++++++++++++---- tests/api/test_addons.py | 27 +++++++++++++++++++++++++++ tests/api/test_host.py | 14 ++++++++------ tests/api/test_supervisor.py | 2 +- 7 files changed, 73 insertions(+), 19 deletions(-) diff --git a/supervisor/api/__init__.py b/supervisor/api/__init__.py index a6607900d..58e54f680 100644 --- a/supervisor/api/__init__.py +++ b/supervisor/api/__init__.py @@ -7,7 +7,6 @@ from typing import Any from aiohttp import web from aiohttp_fast_url_dispatcher import FastUrlDispatcher, attach_fast_url_dispatcher -from ..addons.addon import Addon from ..const import AddonState from ..coresys import CoreSys, CoreSysAttributes from ..exceptions import APIAddonNotInstalled @@ -17,6 +16,7 @@ from .audio import APIAudio from .auth import APIAuth from .backups import APIBackups from .cli import APICli +from .const import CONTENT_TYPE_TEXT from .discovery import APIDiscovery from .dns import APICoreDNS from .docker import APIDocker @@ -38,7 +38,7 @@ from .security import APISecurity from .services import APIServices from .store import APIStore from .supervisor import APISupervisor -from .utils import api_process +from .utils import api_process, api_process_custom _LOGGER: logging.Logger = logging.getLogger(__name__) @@ -518,8 +518,9 @@ class RestAPI(CoreSysAttributes): ] ) + @api_process_custom(CONTENT_TYPE_TEXT) async def get_addon_logs(request, *args, **kwargs): - addon: Addon = api_addons.get_addon_for_request(request) + addon = api_addons.get_addon_for_request(request) kwargs["identifier"] = f"addon_{addon.slug}" return await self._api_host.advanced_logs(request, *args, **kwargs) diff --git a/supervisor/api/host.py b/supervisor/api/host.py index 0e8ca9204..6305b6ac9 100644 --- a/supervisor/api/host.py +++ b/supervisor/api/host.py @@ -53,7 +53,7 @@ from .const import ( CONTENT_TYPE_TEXT, CONTENT_TYPE_X_LOG, ) -from .utils import api_process, api_validate +from .utils import api_process, api_process_custom, api_validate _LOGGER: logging.Logger = logging.getLogger(__name__) @@ -163,7 +163,7 @@ class APIHost(CoreSysAttributes): raise APIError() from err return possible_offset - @api_process + @api_process_custom(CONTENT_TYPE_TEXT) async def advanced_logs( self, request: web.Request, identifier: str | None = None, follow: bool = False ) -> web.StreamResponse: diff --git a/supervisor/api/supervisor.py b/supervisor/api/supervisor.py index dd8de4389..ba6bd0e29 100644 --- a/supervisor/api/supervisor.py +++ b/supervisor/api/supervisor.py @@ -49,8 +49,8 @@ from ..store.validate import repositories from ..utils.sentry import close_sentry, init_sentry from ..utils.validate import validate_timezone from ..validate import version_tag, wait_boot -from .const import CONTENT_TYPE_BINARY -from .utils import api_process, api_process_raw, api_validate +from .const import CONTENT_TYPE_TEXT +from .utils import api_process, api_process_custom, api_validate _LOGGER: logging.Logger = logging.getLogger(__name__) @@ -229,7 +229,7 @@ class APISupervisor(CoreSysAttributes): """Soft restart Supervisor.""" return asyncio.shield(self.sys_supervisor.restart()) - @api_process_raw(CONTENT_TYPE_BINARY) + @api_process_custom(CONTENT_TYPE_TEXT) def logs(self, request: web.Request) -> Awaitable[bytes]: """Return supervisor Docker logs.""" return self.sys_supervisor.logs() diff --git a/supervisor/api/utils.py b/supervisor/api/utils.py index deae62ba1..fff3e34ff 100644 --- a/supervisor/api/utils.py +++ b/supervisor/api/utils.py @@ -91,7 +91,7 @@ def require_home_assistant(method): return wrap_api -def api_process_raw(content): +def api_process_raw(content, *, error_type=None): """Wrap content_type into function.""" def wrap_method(method): @@ -101,13 +101,15 @@ def api_process_raw(content): """Return api information.""" try: msg_data = await method(api, *args, **kwargs) + if isinstance(msg_data, (web.Response, web.StreamResponse)): + return msg_data msg_type = content - except (APIError, APIForbidden) as err: + except (APIError, APIForbidden, HassioError) as err: msg_data = str(err).encode() - msg_type = CONTENT_TYPE_BINARY + msg_type = error_type or CONTENT_TYPE_BINARY except HassioError: msg_data = b"" - msg_type = CONTENT_TYPE_BINARY + msg_type = error_type or CONTENT_TYPE_BINARY return web.Response(body=msg_data, content_type=msg_type) @@ -116,6 +118,28 @@ def api_process_raw(content): return wrap_method +def api_process_custom(content_type): + """Ensure errors are handled and returned with specified content_type.""" + + def decorator(method): + async def wrapper(api, *args, **kwargs): + status = 200 + try: + response = await method(api, *args, **kwargs) + except HassioError as err: + response = str(err) + status = 400 + + if isinstance(response, (web.Response, web.StreamResponse)): + return response + + return web.Response(body=response, status=status, content_type=content_type) + + return wrapper + + return decorator + + def api_return_error( error: Exception | None = None, message: str | None = None ) -> web.Response: diff --git a/tests/api/test_addons.py b/tests/api/test_addons.py index 99913bd67..c07033cfb 100644 --- a/tests/api/test_addons.py +++ b/tests/api/test_addons.py @@ -13,6 +13,7 @@ from supervisor.coresys import CoreSys from supervisor.docker.addon import DockerAddon from supervisor.docker.const import ContainerState from supervisor.docker.monitor import DockerContainerStateEvent +from supervisor.exceptions import HassioError from supervisor.store.repository import Repository from ..const import TEST_ADDON_SLUG @@ -76,6 +77,32 @@ async def test_api_addon_logs( ) +async def test_api_addon_logs_not_installed(api_client: TestClient): + """Test error is returned for non-existing add-on.""" + resp = await api_client.get("/addons/hic_sunt_leones/logs") + + assert resp.status == 400 + assert resp.content_type == "text/plain" + content = await resp.text() + assert content == "Addon hic_sunt_leones does not exist" + + +async def test_api_addon_logs_error( + api_client: TestClient, + journald_logs: MagicMock, + docker_logs: MagicMock, + install_addon_ssh: Addon, +): + """Test errors are properly handled for add-on logs.""" + journald_logs.side_effect = HassioError("Something bad happened!") + resp = await api_client.get("/addons/local_ssh/logs") + + assert resp.status == 400 + assert resp.content_type == "text/plain" + content = await resp.text() + assert content == "Something bad happened!" + + async def test_api_addon_start_healthcheck( api_client: TestClient, coresys: CoreSys, diff --git a/tests/api/test_host.py b/tests/api/test_host.py index eca644cd6..902701a64 100644 --- a/tests/api/test_host.py +++ b/tests/api/test_host.py @@ -310,15 +310,17 @@ async def test_advanced_logs_errors(api_client: TestClient): """Test advanced logging API errors.""" # coresys = coresys_logs_control resp = await api_client.get("/host/logs") - result = await resp.json() - assert result["result"] == "error" - assert result["message"] == "No systemd-journal-gatewayd Unix socket available" + assert resp.content_type == "text/plain" + assert resp.status == 400 + content = await resp.text() + assert content == "No systemd-journal-gatewayd Unix socket available" headers = {"Accept": "application/json"} resp = await api_client.get("/host/logs", headers=headers) - result = await resp.json() - assert result["result"] == "error" + assert resp.content_type == "text/plain" + assert resp.status == 400 + content = await resp.text() assert ( - result["message"] + content == "Invalid content type requested. Only text/plain and text/x-log supported for now." ) diff --git a/tests/api/test_supervisor.py b/tests/api/test_supervisor.py index 7ecb602fd..3573d4cff 100644 --- a/tests/api/test_supervisor.py +++ b/tests/api/test_supervisor.py @@ -169,7 +169,7 @@ async def test_api_supervisor_fallback( ) assert resp.status == 200 - assert resp.content_type == "application/octet-stream" + assert resp.content_type == "text/plain" content = await resp.read() assert content.split(b"\n")[0:2] == [ b"\x1b[36m22-10-11 14:04:23 DEBUG (MainThread) [supervisor.utils.dbus] D-Bus call - org.freedesktop.DBus.Properties.call_get_all on /io/hass/os\x1b[0m",