From ffc52fc26dab33dea67b44a8d0a6cf7856f7a2c6 Mon Sep 17 00:00:00 2001 From: Johnny Bouder Date: Mon, 19 Oct 2026 11:39:40 -0400 Subject: [PATCH] fix(api): keep CRD-only fields through edits, cap inline JSON sources, restart 409, safe owner label - Models carry what the CRD allows: env[].valueFrom, runtime.keepAlive and any resource name (ephemeral-storage, nvidia.com/gpu, claims). - PATCH merges `runtime` and `access` field by field: fields the request leaves out keep their stored value (null/empty removes one), so a UI or MCP edit no longer drops what kubectl or GitOps set. The UI form carries resource names it does not edit, sends null to clear, and shows a valueFrom variable read-only. - POST and PATCH (and so launch_app) return 413 when inline files exceed MAX_UPLOAD_BYTES, the cap uploads already had. - restart returns 409 for a stopped app (replicas 0), as documented. - The owner label is written only when the username is a valid label value; spec.owner stays the owner, so `a@b.c` can create and edit apps. Event matching by exact child names already landed in #82. Closes #57 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016pvMUQJoLkBDk6B94TTm45 --- api/src/nebari_apps_api/cr.py | 38 ++++++- api/src/nebari_apps_api/main.py | 14 ++- api/src/nebari_apps_api/models.py | 24 ++-- api/tests/test_model_fidelity.py | 123 +++++++++++++++++++++ docs/src/content/docs/api-reference.md | 4 +- docs/src/content/docs/app-crd-reference.md | 2 +- mcp/tests/test_tools.py | 10 ++ ui/src/components/app-form.tsx | 33 ++++-- ui/src/lib/types.ts | 15 ++- 9 files changed, 236 insertions(+), 27 deletions(-) create mode 100644 api/tests/test_model_fidelity.py diff --git a/api/src/nebari_apps_api/cr.py b/api/src/nebari_apps_api/cr.py index aa8ebab..d6940ef 100644 --- a/api/src/nebari_apps_api/cr.py +++ b/api/src/nebari_apps_api/cr.py @@ -2,13 +2,27 @@ from __future__ import annotations +import re from typing import Any +from pydantic import BaseModel + from .k8s import GROUP, VERSION from .models import AppCreate, AppOut, AppPatch, AppStatus API_VERSION = f"{GROUP}/{VERSION}" +# A Kubernetes label value. Usernames that are not one (an email-style +# preferred_username) get no owner label: spec.owner is the owner either way, +# and rewriting the name to fit could collide with another user's. +_LABEL_VALUE = re.compile(r"^(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?$") + + +def owner_labels(owner: str) -> dict[str, str]: + if owner and len(owner) <= 63 and _LABEL_VALUE.match(owner): + return {"apps.nebari.dev/owner": owner} + return {} + def _clean(obj: Any) -> Any: """Drop None values and empty containers so CRs stay tidy.""" @@ -44,13 +58,29 @@ def to_cr(req: AppCreate, owner: str) -> dict[str, Any]: "metadata": { "name": req.name, "namespace": req.namespace, - "labels": ({"apps.nebari.dev/owner": owner} if owner else {}) - | {"apps.nebari.dev/subdomain": req.access.subdomain}, + "labels": owner_labels(owner) | {"apps.nebari.dev/subdomain": req.access.subdomain}, }, "spec": _clean(spec) | {"access": {**_clean(spec["access"]), "public": req.access.public}}, } +def _merge(current: Any, change: BaseModel) -> dict[str, Any]: + """Merge the fields a PATCH sent into the stored object. Fields it did not + send keep their stored value, so a partial runtime or access edit does not + drop what another producer (kubectl, GitOps) set; a field sent empty or + null is removed. + """ + merged = dict(current) if isinstance(current, dict) else {} + sent = change.model_dump(include=change.model_fields_set) + for key, value in sent.items(): + value = _clean(value) + if value in (None, {}, [], ""): + merged.pop(key, None) + else: + merged[key] = value + return merged + + def apply_patch(cr: dict[str, Any], patch: AppPatch) -> dict[str, Any]: spec = cr.setdefault("spec", {}) if patch.displayName is not None: @@ -62,9 +92,9 @@ def apply_patch(cr: dict[str, Any], patch: AppPatch) -> dict[str, Any]: if patch.source is not None: spec["source"] = _clean(patch.source.model_dump()) if patch.runtime is not None: - spec["runtime"] = _clean(patch.runtime.model_dump()) + spec["runtime"] = _merge(spec.get("runtime"), patch.runtime) if patch.access is not None: - spec["access"] = _clean(patch.access.model_dump()) | {"public": patch.access.public} + spec["access"] = _merge(spec.get("access"), patch.access) | {"public": patch.access.public} if patch.landingPage is not None: if patch.landingPage.enabled is None: spec.pop("landingPage", None) # back to the cluster default diff --git a/api/src/nebari_apps_api/main.py b/api/src/nebari_apps_api/main.py index 7f01221..9ca888d 100644 --- a/api/src/nebari_apps_api/main.py +++ b/api/src/nebari_apps_api/main.py @@ -301,7 +301,9 @@ def start_app(namespace: str, name: str, store: Store, user: Me) -> AppOut: @app.post(PREFIX + "/apps/{namespace}/{name}/restart", response_model=AppOut) def restart_app(namespace: str, name: str, store: Store, user: Me) -> AppOut: - authorized_app(store, user, namespace, name) + cr = authorized_app(store, user, namespace, name) + if ((cr.get("spec") or {}).get("runtime") or {}).get("replicas", 1) == 0: + raise HTTPException(409, f"app {namespace}/{name} is stopped; start it instead") try: store.restart_app(namespace, name) except NotFoundError as exc: @@ -425,6 +427,16 @@ def _reject_users(access: Any) -> None: def _check_source(source: Any) -> None: + if source is not None and source.inline is not None: + # The same cap as uploads: inline files become one ConfigMap, which + # the apiserver refuses past ~1 MiB. + size = sum(len(c.encode()) for c in source.inline.files.values()) + if size > settings.max_upload_bytes: + raise HTTPException( + 413, + f"inline files total {size} bytes, over the {settings.max_upload_bytes}-byte cap; " + "use a git or pvc source for larger apps", + ) if source is not None and source.type == "git" and source.git is not None: try: source.git.check() diff --git a/api/src/nebari_apps_api/models.py b/api/src/nebari_apps_api/models.py index 68fd24c..8f5abb4 100644 --- a/api/src/nebari_apps_api/models.py +++ b/api/src/nebari_apps_api/models.py @@ -3,7 +3,7 @@ from __future__ import annotations import ipaddress -from typing import Literal +from typing import Any, Literal from urllib.parse import urlsplit from pydantic import BaseModel, ConfigDict, Field @@ -71,24 +71,34 @@ class AppSource(BaseModel): class EnvVar(BaseModel): + """A Kubernetes EnvVar: a literal `value`, or `valueFrom` (a Secret, + ConfigMap or field reference, passed through as written). + """ + name: str value: str = "" + valueFrom: dict[str, Any] | None = None -class ResourceAmounts(BaseModel): - cpu: str | None = None - memory: str | None = None +class Resources(BaseModel): + """Kubernetes ResourceRequirements. Any resource name is kept (cpu, + memory, ephemeral-storage, extended resources such as nvidia.com/gpu), + so an App authored with kubectl keeps them through an edit. + """ + model_config = ConfigDict(extra="allow") -class Resources(BaseModel): - requests: ResourceAmounts | None = None - limits: ResourceAmounts | None = None + requests: dict[str, str] | None = None + limits: dict[str, str] | None = None class AppRuntime(BaseModel): env: list[EnvVar] = Field(default_factory=list) resources: Resources | None = None replicas: int = Field(default=1, ge=0, le=10) + # Reserved for scale-to-zero idle reaping (not implemented); kept so an + # App that sets it does not lose it on an edit. + keepAlive: bool | None = None # When set, the app runs as a Python/pixi service: the operator installs # the pixi environment and executes `pixi run `. The task must # start a server listening on 0.0.0.0:8080. diff --git a/api/tests/test_model_fidelity.py b/api/tests/test_model_fidelity.py new file mode 100644 index 0000000..07969b8 --- /dev/null +++ b/api/tests/test_model_fidelity.py @@ -0,0 +1,123 @@ +"""Edits through the API keep what other producers (kubectl, GitOps) wrote.""" + +from __future__ import annotations + +import pytest +from fastapi.testclient import TestClient + +from nebari_apps_api.auth import User, current_user +from nebari_apps_api.config import settings +from nebari_apps_api.main import create_app + +from .test_apps import make_app_body + +URL = "/api/v1/apps/apps/docs-site" + +KUBECTL_RUNTIME = { + "replicas": 2, + "keepAlive": True, + "pixiTask": "", + "env": [ + {"name": "LOG_LEVEL", "value": "info"}, + {"name": "TOKEN", "valueFrom": {"secretKeyRef": {"name": "site", "key": "token"}}}, + ], + "resources": { + "requests": {"cpu": "100m", "memory": "128Mi", "ephemeral-storage": "1Gi"}, + "limits": {"memory": "512Mi", "nvidia.com/gpu": "1"}, + }, +} + + +@pytest.fixture +def kubectl_app(client, store): + """An App created through the API, then edited with kubectl. Returns a + reader for the stored CR (writes replace it). + """ + assert client.post("/api/v1/apps", json=make_app_body()).status_code == 201 + runtime = {k: v for k, v in KUBECTL_RUNTIME.items() if v != ""} + store.apps[("apps", "docs-site")]["spec"]["runtime"] = runtime + return lambda: store.apps[("apps", "docs-site")] + + +def test_get_returns_crd_only_fields(client, kubectl_app): + runtime = client.get(URL).json()["runtime"] + assert runtime["keepAlive"] is True + assert runtime["env"][1]["valueFrom"] == {"secretKeyRef": {"name": "site", "key": "token"}} + assert runtime["resources"]["limits"]["nvidia.com/gpu"] == "1" + + +def test_patch_display_name_keeps_runtime(client, kubectl_app): + before = dict(kubectl_app()["spec"]["runtime"]) + assert client.patch(URL, json={"displayName": "Renamed"}).status_code == 200 + assert kubectl_app()["spec"]["runtime"] == before + + +def test_patch_runtime_merges_fields(client, kubectl_app): + resp = client.patch(URL, json={"runtime": {"replicas": 3}}) + assert resp.status_code == 200, resp.text + runtime = kubectl_app()["spec"]["runtime"] + assert runtime["replicas"] == 3 + assert runtime["keepAlive"] is True + assert runtime["env"][1]["valueFrom"]["secretKeyRef"]["key"] == "token" + assert runtime["resources"]["requests"]["ephemeral-storage"] == "1Gi" + + +def test_ui_round_trip_keeps_everything(client, kubectl_app): + """The UI edits by sending back the runtime it fetched.""" + runtime = client.get(URL).json()["runtime"] + runtime["env"][0]["value"] = "debug" + assert client.patch(URL, json={"runtime": runtime}).status_code == 200 + stored = kubectl_app()["spec"]["runtime"] + assert stored["env"][0] == {"name": "LOG_LEVEL", "value": "debug"} + assert stored["env"][1] == {"name": "TOKEN", "valueFrom": {"secretKeyRef": {"name": "site", "key": "token"}}} + assert stored["keepAlive"] is True + assert stored["resources"]["limits"]["nvidia.com/gpu"] == "1" + + +def test_patch_can_clear_a_runtime_field(client, kubectl_app): + assert client.patch(URL, json={"runtime": {"env": []}}).status_code == 200 + assert "env" not in kubectl_app()["spec"]["runtime"] + assert kubectl_app()["spec"]["runtime"]["replicas"] == 2 + + +def test_inline_files_over_the_cap_are_413(client, monkeypatch): + monkeypatch.setattr(settings, "max_upload_bytes", 1000) + body = make_app_body() + body["source"]["inline"]["files"] = {"index.html": "x" * 600, "app.js": "y" * 600} + resp = client.post("/api/v1/apps", json=body) + assert resp.status_code == 413, resp.text + assert "1000-byte cap" in resp.json()["detail"] + + assert client.post("/api/v1/apps", json=make_app_body()).status_code == 201 + big = {"type": "inline", "inline": {"files": {"index.html": "z" * 1001}}} + assert client.patch(URL, json={"source": big}).status_code == 413 + + +def test_restart_a_stopped_app_is_409(client, store): + client.post("/api/v1/apps", json=make_app_body()) + assert client.post(f"{URL}/stop").status_code == 200 + resp = client.post(f"{URL}/restart") + assert resp.status_code == 409 + assert store.restarted == [] + client.post(f"{URL}/start") + assert client.post(f"{URL}/restart").status_code == 200 + + +def test_email_style_owner(store, monkeypatch): + monkeypatch.setattr(settings, "auth_enabled", True) + app = create_app(store) + app.dependency_overrides[current_user] = lambda: User(username="a@b.c", groups=[]) + client = TestClient(app) + assert client.post("/api/v1/apps", json=make_app_body()).status_code == 201 + cr = store.apps[("apps", "docs-site")] + assert cr["spec"]["owner"] == "a@b.c" + assert "apps.nebari.dev/owner" not in cr["metadata"]["labels"] + # spec.owner alone makes them the owner. + assert client.get(URL).status_code == 200 + assert client.patch(URL, json={"displayName": "Mine"}).status_code == 200 + + +def test_patch_null_resources_clears_them(client, kubectl_app): + assert client.patch(URL, json={"runtime": {"resources": None}}).status_code == 200 + assert "resources" not in kubectl_app()["spec"]["runtime"] + assert kubectl_app()["spec"]["runtime"]["keepAlive"] is True diff --git a/docs/src/content/docs/api-reference.md b/docs/src/content/docs/api-reference.md index b4e8d11..114777a 100644 --- a/docs/src/content/docs/api-reference.md +++ b/docs/src/content/docs/api-reference.md @@ -57,7 +57,7 @@ create. | `POST /apps` | Create + launch. Body mirrors `App.spec` plus `name`/`namespace` (see below). `409` if the name exists in the namespace or the subdomain is used by any other app. | | `POST /apps/upload` | Create + launch an app from an upload. Multipart: `manifest` (JSON, source omitted) + `file` (`.zip` or `.html`). A manifest with `runtime.pixiTask` launches the zip as a Python/pixi app. | | `GET /apps/{ns}/{name}` | Full spec + status. | -| `PATCH /apps/{ns}/{name}` | Update any of `displayName`, `description`, `thumbnail`, `source`, `runtime`, `access`. `access.subdomain` cannot change (`422`). | +| `PATCH /apps/{ns}/{name}` | Update any of `displayName`, `description`, `thumbnail`, `source`, `runtime`, `access`. `runtime` and `access` merge field by field: a field you send replaces the stored one (`null` or empty removes it), and fields you leave out keep their value, including ones set with `kubectl` such as `env[].valueFrom`, `keepAlive` or extra resource names. `source` is replaced whole. `access.subdomain` cannot change (`422`). | | `DELETE /apps/{ns}/{name}` | Delete (cascades to all children). | | `POST /apps/{ns}/{name}/stop` | Scale to zero. | | `POST /apps/{ns}/{name}/start` | Scale back to one. | @@ -135,7 +135,7 @@ curl -X POST https://apps.example.ai/api/v1/apps/upload \ | `403` | Namespace not available (not a launchable namespace), or the caller is neither the app's owner nor an admin. | | `404` | App (or its pods, for logs) not found. | | `409` | An app with that name already exists in the namespace, or the subdomain is already used by another app. | -| `413` | Request body over 2 MiB, or an upload (or its extracted contents) over the ~900 KB inline cap. Zip members are checked against the cap before they are decompressed. | +| `413` | Request body over 2 MiB, or inline files over the ~900 KB inline cap (`MAX_UPLOAD_BYTES`), whether they come from an upload (or its extracted contents) or from `source.inline.files` in a JSON `POST`/`PATCH`. Zip members are checked against the cap before they are decompressed. | | `422` | Invalid request: unsupported source type, missing required fields, or a non-empty `access.users` (reserved, not enforced). | Error bodies are `{"detail": "..."}` with an actionable message. diff --git a/docs/src/content/docs/app-crd-reference.md b/docs/src/content/docs/app-crd-reference.md index 49efa1d..92f47f3 100644 --- a/docs/src/content/docs/app-crd-reference.md +++ b/docs/src/content/docs/app-crd-reference.md @@ -58,7 +58,7 @@ status: | `displayName` | string | Yes | Human-readable name (max 64 chars); shown in the UI and on the landing page. | | `description` | string | No | Short description (max 256 chars). | | `thumbnail` | string | No | Data-URI image for catalogs / the landing-page tile. | -| `owner` | string | No | Keycloak `preferred_username` that manages the app. The API sets this from the caller's token. | +| `owner` | string | No | Keycloak `preferred_username` that manages the app. The API sets this from the caller's token. It is the owner of record; the API also writes it to the `apps.nebari.dev/owner` label when the name is a valid label value (an email-style username such as `a@b.c` gets no label). | | `source` | [AppSource](#specsource) | Yes | Where the app's content comes from. | | `runtime` | [AppRuntime](#specruntime) | No | Process configuration: env, resources, replicas. | | `access` | [AppAccess](#specaccess) | Yes | Who can reach the app and at which subdomain. | diff --git a/mcp/tests/test_tools.py b/mcp/tests/test_tools.py index 8a6689c..b4fde5a 100644 --- a/mcp/tests/test_tools.py +++ b/mcp/tests/test_tools.py @@ -216,3 +216,13 @@ async def test_launch_passes_resources(store): async def test_launch_without_resources_leaves_defaults_to_operator(store): await call("launch_app", LAUNCH_ARGS) assert "resources" not in store.apps[("apps", "docs-site")]["spec"]["runtime"] + + +async def test_launch_over_the_inline_cap_is_413(store, monkeypatch): + from nebari_apps_api.config import settings as api_settings + + monkeypatch.setattr(api_settings, "max_upload_bytes", 1000) + result = await call("launch_app", {**LAUNCH_ARGS, "inline_files": {"index.html": "x" * 1001}}) + assert result["status"] == 413 + assert "1000-byte cap" in result["error"] + assert ("apps", "docs-site") not in store.apps diff --git a/ui/src/components/app-form.tsx b/ui/src/components/app-form.tsx index f66a2d3..88da66a 100644 --- a/ui/src/components/app-form.tsx +++ b/ui/src/components/app-form.tsx @@ -13,7 +13,7 @@ import { } from "@/components/ui/select"; import { Switch } from "@/components/ui/switch"; import { Tabs, TabsList, TabsPanel, TabsTab } from "@/components/ui/tabs"; -import type { App, AppCreate, AppPatch, AppSource, EnvVar } from "@/lib/types"; +import type { App, AppCreate, AppPatch, AppSource, EnvVar, ResourceAmounts } from "@/lib/types"; const SUBDOMAIN_RE = /^[a-z0-9]([-a-z0-9]*[a-z0-9])?$/; @@ -148,12 +148,21 @@ export function AppForm({ setMemLim(p.memLim); }; + // The form edits cpu and memory; any other resource the App already has + // (ephemeral-storage, a GPU limit set with kubectl) is carried over. null + // clears resources on an edit, since an omitted field keeps its value. const buildResources = () => { - const requests = - cpuReq || memReq ? { cpu: cpuReq || undefined, memory: memReq || undefined } : undefined; - const limits = - cpuLim || memLim ? { cpu: cpuLim || undefined, memory: memLim || undefined } : undefined; - return requests || limits ? { requests, limits } : undefined; + const merge = (base: ResourceAmounts | undefined, cpu: string, memory: string) => { + const { cpu: _cpu, memory: _memory, ...rest } = base ?? {}; + const out: ResourceAmounts = { ...rest }; + if (cpu) out.cpu = cpu; + if (memory) out.memory = memory; + return Object.keys(out).length ? out : undefined; + }; + const current = initial?.runtime?.resources ?? undefined; + const requests = merge(current?.requests, cpuReq, memReq); + const limits = merge(current?.limits, cpuLim, memLim); + return requests || limits ? { ...current, requests, limits } : null; }; const buildSource = (): AppSource | undefined => { @@ -524,7 +533,9 @@ export function AppForm({ setEnvVars( envVars.map((v, j) => (j === i ? { ...v, value: e.target.value } : v)), @@ -635,6 +646,14 @@ export function AppForm({ ); } +/** "secret site/token", "configMap cfg/key" or the reference's kind. */ +function describeValueFrom(ref: Record): string { + const [kind, target] = Object.entries(ref)[0] ?? ["reference", {}]; + const { name, key } = (target ?? {}) as { name?: string; key?: string }; + const label = kind.replace(/KeyRef$|Ref$/, ""); + return name ? `from ${label} ${name}${key ? `/${key}` : ""}` : `from ${label}`; +} + function splitGroups(raw: string): string[] { return raw .split(",") diff --git a/ui/src/lib/types.ts b/ui/src/lib/types.ts index a8b2baa..c695d5b 100644 --- a/ui/src/lib/types.ts +++ b/ui/src/lib/types.ts @@ -15,16 +15,21 @@ export interface AppSource { export interface EnvVar { name: string; - value: string; + value?: string; + /** A Secret, ConfigMap or field reference set with kubectl; kept as is. */ + valueFrom?: Record; } +/** Resource name (cpu, memory, ephemeral-storage, nvidia.com/gpu, ...) -> quantity. */ +export type ResourceAmounts = Record; + export interface AppRuntime { env?: EnvVar[]; replicas?: number; - resources?: { - requests?: { cpu?: string; memory?: string }; - limits?: { cpu?: string; memory?: string }; - }; + /** null in a PATCH clears it; an omitted field keeps the stored value. */ + resources?: { requests?: ResourceAmounts; limits?: ResourceAmounts } | null; + /** Reserved (scale-to-zero); only round-tripped. */ + keepAlive?: boolean; /** When set, the app runs as a Python/pixi service via `pixi run `. */ pixiTask?: string; }