From 19f78e6c14a5c252b76d45928eb8de8a5ef99098 Mon Sep 17 00:00:00 2001 From: Min RK Date: Thu, 26 Sep 2024 17:10:19 +0200 Subject: [PATCH 1/3] relay all k8s errors to users turns lots of user errors from vague 500 into actionable errors e.g. 404 not found, invalid names, configmaps too big, etc. --- kbatch-proxy/kbatch_proxy/main.py | 38 ++++++++++++++++++------------- kbatch-proxy/tests/test_app.py | 12 ++++++++-- 2 files changed, 32 insertions(+), 18 deletions(-) diff --git a/kbatch-proxy/kbatch_proxy/main.py b/kbatch-proxy/kbatch_proxy/main.py index 17d1e59..795ea14 100644 --- a/kbatch-proxy/kbatch_proxy/main.py +++ b/kbatch-proxy/kbatch_proxy/main.py @@ -1,3 +1,4 @@ +import json import os import logging from typing import List, Optional, Tuple, Dict, Union @@ -168,6 +169,22 @@ def get_k8s_api() -> Tuple[kubernetes.client.CoreV1Api, kubernetes.client.BatchV # app +@app.exception_handler(kubernetes.client.ApiException) +async def kubernetes_exception_handler( + request: Request, exc: kubernetes.client.ApiException +): + """Relay kubernetes errors to users""" + try: + detail = json.loads(exc.body)["message"] + except (ValueError, KeyError): + detail = exc.body + + raise HTTPException( + status_code=exc.status, + detail=detail, + ) + + # cronjobs # @router.get("/cronjobs/{job_name}") async def read_cronjob(job_name: str, user: User = Depends(get_current_user)): @@ -390,22 +407,11 @@ def _create_job( patch.add_submitted_configmap_name(job_to_patch, config_map) logger.info("Submitting job") - try: - if issubclass(model, V1Job): - resp = batch_api.create_namespaced_job(namespace=user.namespace, body=job) - elif issubclass(model, V1CronJob): - job.spec.job_template = job_to_patch - resp = batch_api.create_namespaced_cron_job( - namespace=user.namespace, body=job - ) - - except kubernetes.client.exceptions.ApiException as e: - content_type = e.headers.get("Content-Type") - if content_type: - headers = {"Content-Type": content_type} - else: - headers = {} - raise HTTPException(status_code=e.status, detail=e.body, headers=headers) + if issubclass(model, V1Job): + resp = batch_api.create_namespaced_job(namespace=user.namespace, body=job) + elif issubclass(model, V1CronJob): + job.spec.job_template = job_to_patch + resp = batch_api.create_namespaced_cron_job(namespace=user.namespace, body=job) if config_map: logger.info( diff --git a/kbatch-proxy/tests/test_app.py b/kbatch-proxy/tests/test_app.py index 284c847..0f1c8c8 100644 --- a/kbatch-proxy/tests/test_app.py +++ b/kbatch-proxy/tests/test_app.py @@ -1,3 +1,4 @@ +import json import os import sys import pytest @@ -38,8 +39,7 @@ def test_read_main(): assert response.json() == {"message": "kbatch"} -@pytest.mark.usefixtures("mock_hub_auth") -def test_authorized(): +def test_authorized(mock_hub_auth): response = client.get("/authorized") assert response.status_code == 401 @@ -61,3 +61,11 @@ def test_loads_profile(): subprocess.check_output( f"KBATCH_PROFILE_FILE={profile} {sys.executable} -c '{code}'", shell=True ) + + +def test_error_handling(mock_hub_auth): + response = client.get("/jobs/nosuchjob", headers={"Authorization": "token abc"}) + err = json.loads(response.read().decode("utf8")) + assert response.status_code == 404 + assert err["code"] == 404 + assert "nosuchjob" in err["message"] From f4ae536302a8818efc752235c266060c90c8ed86 Mon Sep 17 00:00:00 2001 From: Min RK Date: Fri, 27 Sep 2024 12:51:24 +0200 Subject: [PATCH 2/3] avoid tracebacks for HTTP errors show only the status and error message --- kbatch/kbatch/__main__.py | 4 ++++ kbatch/kbatch/_core.py | 6 +----- kbatch/kbatch/cli.py | 30 ++++++++++++++++++++++++++++-- kbatch/setup.cfg | 4 ++-- 4 files changed, 35 insertions(+), 9 deletions(-) create mode 100644 kbatch/kbatch/__main__.py diff --git a/kbatch/kbatch/__main__.py b/kbatch/kbatch/__main__.py new file mode 100644 index 0000000..f8439fe --- /dev/null +++ b/kbatch/kbatch/__main__.py @@ -0,0 +1,4 @@ +if __name__ == "__main__": + from kbatch.cli import main + + main() diff --git a/kbatch/kbatch/_core.py b/kbatch/kbatch/_core.py index f77e659..fc5ccce 100644 --- a/kbatch/kbatch/_core.py +++ b/kbatch/kbatch/_core.py @@ -131,11 +131,7 @@ def _request_action( headers=headers, json=json_data, ) - try: - r.raise_for_status() - except Exception: - logger.exception(r.text) - raise + r.raise_for_status() return r.json() diff --git a/kbatch/kbatch/cli.py b/kbatch/kbatch/cli.py index 04f0a97..7aba6e7 100644 --- a/kbatch/kbatch/cli.py +++ b/kbatch/kbatch/cli.py @@ -1,6 +1,9 @@ +import sys import logging +from contextlib import contextmanager import click +import httpx import rich import rich.logging from kubernetes.client.models import V1Job, V1CronJob @@ -16,8 +19,23 @@ datefmt="[%X]", handlers=[rich.logging.RichHandler()], ) -# don't log every http request -logging.getLogger("httpx").setLevel(logging.WARNING) +# don't log http requests or responses +logging.getLogger("httpx").propagate = False + + +@contextmanager +def _render_http_error(): + """Render an HTTP Error from kbatch nicely""" + try: + yield + except httpx.HTTPStatusError as e: + response = e.response + try: + response_json: dict = response.json() + msg = response_json["detail"] + except (ValueError, KeyError): + msg = response.text + sys.exit(f"kbatch-proxy error {response.status_code}: {msg}") @click.group() @@ -354,3 +372,11 @@ def logs(pod_name, kbatch_url, token, stream, pretty, read_timeout): print(line) else: print(result) + + +def main(): + """Launch kbatch CLI""" + # catch and format HTTP errors + # not sure if there's a better way to inject this into cli() itself? + with _render_http_error(): + cli() diff --git a/kbatch/setup.cfg b/kbatch/setup.cfg index 97541a9..a6e87e2 100644 --- a/kbatch/setup.cfg +++ b/kbatch/setup.cfg @@ -40,7 +40,7 @@ docs = [options.entry_points] console_scripts = - kbatch = kbatch.cli:cli + kbatch = kbatch.cli:main [flake8] exclude = @@ -66,4 +66,4 @@ ignore = # black is set to 88, but isn't a strict limit so we add some wiggle room for # flake8 testing. -max-line-length = 100 \ No newline at end of file +max-line-length = 100 From 322ad9a6d36d9a2db0449080837789d7258c8fd8 Mon Sep 17 00:00:00 2001 From: Min RK Date: Fri, 27 Sep 2024 15:23:00 +0200 Subject: [PATCH 3/3] fix test expectation --- kbatch-proxy/tests/test_app.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/kbatch-proxy/tests/test_app.py b/kbatch-proxy/tests/test_app.py index 0f1c8c8..fcb413d 100644 --- a/kbatch-proxy/tests/test_app.py +++ b/kbatch-proxy/tests/test_app.py @@ -67,5 +67,5 @@ def test_error_handling(mock_hub_auth): response = client.get("/jobs/nosuchjob", headers={"Authorization": "token abc"}) err = json.loads(response.read().decode("utf8")) assert response.status_code == 404 - assert err["code"] == 404 - assert "nosuchjob" in err["message"] + assert "detail" in err + assert "nosuchjob" in err["detail"]