fix(api): editable-spec detail + PATCH-merge saves + shared gateway parser
Deployment/program edits could silently drop spec fields:
- GET /deployments/{name} served the runtime view (no reach/program/root/expose)
for deployed deployments, so the dashboard round-tripped a lossy manifest and
stripped fields on save (broke astro). Now serves the editable castle.yaml spec
when the deployment is in config.
- _save_deployment and save_program replaced the whole object with client input.
Now shallow-merge over the existing spec (omit = preserve, null = clear), so a
partial/lossy save can't drop untouched fields.
- save_yaml rebuilt GatewayConfig from `port` only, wiping tls/domain/tunnel/
cert_hook on a whole-file save. Now uses a shared parse_gateway() (also used by
load_config) so gateway fields can't drift between the two.
Dashboard forms (ServiceFields/StaticFields) send explicit null to clear, per the
merge contract; adds exposure host-label helpers. Coverage: detail-serves-spec,
save round-trip, partial-patch-preserves, null-clears, program partial-patch,
parse_gateway, and config save/load round-trip.
This commit is contained in:
@@ -11,12 +11,12 @@ from pydantic import BaseModel
|
||||
|
||||
from castle_core.config import (
|
||||
CastleConfig,
|
||||
GatewayConfig,
|
||||
_DEPLOYMENT_ADAPTER,
|
||||
_normalize_deployment_dict,
|
||||
_program_to_yaml_dict,
|
||||
_spec_to_yaml_dict,
|
||||
load_config,
|
||||
parse_gateway,
|
||||
save_config,
|
||||
)
|
||||
from castle_core.manifest import ProgramSpec, kind_for
|
||||
@@ -173,11 +173,12 @@ def save_yaml(request: ConfigSaveRequest) -> ConfigSaveResponse:
|
||||
svc_count = sum(1 for d in deployments.values() if kind_for(d) == "service")
|
||||
job_count = sum(1 for d in deployments.values() if kind_for(d) == "job")
|
||||
|
||||
gateway_data = data.get("gateway", {})
|
||||
config = CastleConfig(
|
||||
root=root,
|
||||
repo=repo_path,
|
||||
gateway=GatewayConfig(port=gateway_data.get("port", 9000)),
|
||||
# Parse the FULL gateway block (tls/domain/tunnel/cert_hook/…), not just
|
||||
# port — otherwise a whole-file save silently wipes the gateway config.
|
||||
gateway=parse_gateway(data.get("gateway", {})),
|
||||
programs=programs,
|
||||
deployments=deployments,
|
||||
)
|
||||
@@ -194,21 +195,32 @@ def save_yaml(request: ConfigSaveRequest) -> ConfigSaveResponse:
|
||||
|
||||
@router.put("/programs/{name}")
|
||||
def save_program(name: str, request: ProgramConfigRequest) -> dict:
|
||||
"""Update a single program's config in castle.yaml."""
|
||||
"""Update a single program's config in castle.yaml (PATCH semantics).
|
||||
|
||||
Like deployments: the incoming config is shallow-merged over the existing
|
||||
program spec, so a partial save can't drop source/stack/commands/build/… that
|
||||
the client didn't send. Omitted keys are preserved; an explicit ``null`` clears.
|
||||
"""
|
||||
_require_repo()
|
||||
config = get_config()
|
||||
incoming = dict(request.config)
|
||||
|
||||
if name in config.programs:
|
||||
base = config.programs[name].model_dump(mode="json", exclude_none=True)
|
||||
merged = {**base, **incoming, "id": name}
|
||||
merged = {k: v for k, v in merged.items() if v is not None}
|
||||
else:
|
||||
merged = {**incoming, "id": name}
|
||||
|
||||
try:
|
||||
prog_data = dict(request.config)
|
||||
prog_data["id"] = name
|
||||
ProgramSpec.model_validate(prog_data)
|
||||
spec = ProgramSpec.model_validate(merged)
|
||||
except Exception as e:
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_422_UNPROCESSABLE_ENTITY,
|
||||
detail=f"Invalid program config: {e}",
|
||||
)
|
||||
|
||||
config = get_config()
|
||||
config.programs[name] = ProgramSpec.model_validate({**request.config, "id": name})
|
||||
config.programs[name] = spec
|
||||
save_config(config)
|
||||
return {"ok": True, "program": name}
|
||||
|
||||
@@ -271,23 +283,33 @@ async def delete_program(name: str, cascade: bool = False) -> dict:
|
||||
|
||||
|
||||
def _save_deployment(name: str, config_dict: dict) -> dict:
|
||||
"""Validate a deployment (any manager) and persist it to config.deployments."""
|
||||
"""Create/update a deployment (any manager) with PATCH semantics.
|
||||
|
||||
The incoming config is shallow-merged over the existing spec, so a save can
|
||||
never silently drop a field the client didn't send (the astro/postgres bug):
|
||||
a present key replaces wholesale, an **omitted** key is preserved, and an
|
||||
explicit ``null`` clears the key (back to its default). On CREATE there's no
|
||||
base, so the incoming config stands alone.
|
||||
"""
|
||||
_require_repo()
|
||||
config = get_config()
|
||||
config_dict = dict(config_dict)
|
||||
incoming = dict(config_dict)
|
||||
|
||||
# On CREATE (a new deployment) with no description of its own, inherit the
|
||||
# referenced program's description — a deployment reads as its program by
|
||||
# default. Edits keep whatever the user set (including a cleared field).
|
||||
if name not in config.deployments and not config_dict.get("description"):
|
||||
prog = config_dict.get("program")
|
||||
if prog and prog in config.programs and config.programs[prog].description:
|
||||
config_dict["description"] = config.programs[prog].description
|
||||
if name in config.deployments:
|
||||
base = config.deployments[name].model_dump(mode="json", exclude_none=True)
|
||||
merged = {**base, **incoming, "id": name}
|
||||
# An explicit null means "clear" — drop the key so its default applies.
|
||||
merged = {k: v for k, v in merged.items() if v is not None}
|
||||
else:
|
||||
merged = {**incoming, "id": name}
|
||||
# On CREATE with no description, inherit the referenced program's.
|
||||
if not merged.get("description"):
|
||||
prog = merged.get("program")
|
||||
if prog and prog in config.programs and config.programs[prog].description:
|
||||
merged["description"] = config.programs[prog].description
|
||||
|
||||
try:
|
||||
dep = _DEPLOYMENT_ADAPTER.validate_python(
|
||||
_normalize_deployment_dict({**config_dict, "id": name})
|
||||
)
|
||||
dep = _DEPLOYMENT_ADAPTER.validate_python(_normalize_deployment_dict(merged))
|
||||
except Exception as e:
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_422_UNPROCESSABLE_ENTITY,
|
||||
|
||||
@@ -783,39 +783,49 @@ def get_component(name: str) -> DeploymentDetail:
|
||||
deployed = registry.deployed[name]
|
||||
summary = _summary_from_deployed(name, deployed)
|
||||
|
||||
# Backfill source from castle.yaml program ref
|
||||
root = get_castle_root()
|
||||
if root and summary.source is None:
|
||||
config = None
|
||||
if root:
|
||||
try:
|
||||
from castle_core.config import load_config
|
||||
|
||||
config = load_config(root)
|
||||
if name in config.programs:
|
||||
summary.source = config.programs[name].source
|
||||
else:
|
||||
ref = None
|
||||
if name in config.services:
|
||||
ref = config.services[name].program
|
||||
elif name in config.jobs:
|
||||
ref = config.jobs[name].program
|
||||
if ref and ref in config.programs:
|
||||
summary.source = config.programs[ref].source
|
||||
except FileNotFoundError:
|
||||
pass
|
||||
config = None
|
||||
|
||||
raw = {
|
||||
"manager": deployed.manager,
|
||||
"launcher": deployed.launcher,
|
||||
"run_cmd": deployed.run_cmd,
|
||||
"env": deployed.env,
|
||||
"secret_env_keys": deployed.secret_env_keys,
|
||||
"port": deployed.port,
|
||||
"health_path": deployed.health_path,
|
||||
"subdomain": deployed.subdomain,
|
||||
"managed": deployed.managed,
|
||||
"kind": deployed.kind,
|
||||
"stack": deployed.stack,
|
||||
}
|
||||
# Backfill source from castle.yaml program ref
|
||||
if config and summary.source is None:
|
||||
if name in config.programs:
|
||||
summary.source = config.programs[name].source
|
||||
else:
|
||||
ref = None
|
||||
if name in config.services:
|
||||
ref = config.services[name].program
|
||||
elif name in config.jobs:
|
||||
ref = config.jobs[name].program
|
||||
if ref and ref in config.programs:
|
||||
summary.source = config.programs[ref].source
|
||||
|
||||
# The edit form needs the *editable source spec* (reach/program/root/expose/
|
||||
# defaults), not the runtime view — serve the castle.yaml spec whenever the
|
||||
# deployment is defined there. Fall back to the runtime dict only for a
|
||||
# deployed-but-not-in-config item (e.g. a discovered remote reference).
|
||||
if config and name in config.deployments:
|
||||
raw = config.deployments[name].model_dump(mode="json", exclude_none=True)
|
||||
else:
|
||||
raw = {
|
||||
"manager": deployed.manager,
|
||||
"launcher": deployed.launcher,
|
||||
"run_cmd": deployed.run_cmd,
|
||||
"env": deployed.env,
|
||||
"secret_env_keys": deployed.secret_env_keys,
|
||||
"port": deployed.port,
|
||||
"health_path": deployed.health_path,
|
||||
"subdomain": deployed.subdomain,
|
||||
"managed": deployed.managed,
|
||||
"kind": deployed.kind,
|
||||
"stack": deployed.stack,
|
||||
}
|
||||
return DeploymentDetail(**summary.model_dump(), manifest=raw)
|
||||
|
||||
# Fall back to castle.yaml
|
||||
|
||||
78
castle-api/tests/test_deployment_roundtrip.py
Normal file
78
castle-api/tests/test_deployment_roundtrip.py
Normal file
@@ -0,0 +1,78 @@
|
||||
"""Edit-safety tests for the deployment detail + save endpoints.
|
||||
|
||||
These guard the regression that broke astro (and postgres): the detail endpoint
|
||||
served the *runtime view* (no reach/program/root/expose) for a deployed deployment,
|
||||
so the dashboard edit form round-tripped a lossy manifest and stripped spec-only
|
||||
fields on save. The fixtures already have `test-svc` deployed AND defined in
|
||||
castle.yaml (with legacy `proxy: true` → `reach: internal`) — exactly that case.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from fastapi.testclient import TestClient
|
||||
|
||||
|
||||
class TestDeploymentEditSafety:
|
||||
def test_detail_serves_editable_spec_not_runtime(self, client: TestClient) -> None:
|
||||
"""A deployed deployment that's in castle.yaml serves its EDITABLE SPEC —
|
||||
the shape the edit form consumes (launcher nested under `run`, plus
|
||||
reach/expose) — not the flat runtime view (`run_cmd`, top-level launcher)."""
|
||||
m = client.get("/deployments/test-svc").json()["manifest"]
|
||||
assert m["run"]["launcher"] == "python" # spec shape (nested)
|
||||
assert "run_cmd" not in m # runtime-only key absent
|
||||
assert m.get("reach") == "internal" # normalized from proxy:true
|
||||
assert m["expose"]["http"]["internal"]["port"] == 19000
|
||||
|
||||
def test_save_roundtrip_preserves_spec_fields(self, client: TestClient) -> None:
|
||||
"""GET a deployment's manifest → PUT it back unchanged → GET again: no
|
||||
spec field may be lost. This is the exact round-trip the dashboard does on
|
||||
an edit, and the exact thing that dropped astro's program/root."""
|
||||
before = client.get("/deployments/test-svc").json()["manifest"]
|
||||
resp = client.put("/config/deployments/test-svc", json={"config": before})
|
||||
assert resp.status_code == 200, resp.text
|
||||
after = client.get("/deployments/test-svc").json()["manifest"]
|
||||
for key in ("reach", "expose", "run", "program"):
|
||||
assert after.get(key) == before.get(key), f"{key} lost on save round-trip"
|
||||
|
||||
def test_partial_patch_preserves_untouched_fields(self, client: TestClient) -> None:
|
||||
"""PATCH semantics: sending ONLY the changed field must not drop the rest.
|
||||
This is what makes the astro-class regression structurally impossible —
|
||||
even a client that sends a lossy payload can't nuke fields it omitted."""
|
||||
before = client.get("/deployments/test-svc").json()["manifest"]
|
||||
resp = client.put("/config/deployments/test-svc", json={"config": {"reach": "off"}})
|
||||
assert resp.status_code == 200, resp.text
|
||||
after = client.get("/deployments/test-svc").json()["manifest"]
|
||||
assert after["reach"] == "off" # the change applied
|
||||
assert after["program"] == before["program"] # untouched → preserved
|
||||
assert after["run"] == before["run"]
|
||||
assert after["expose"] == before["expose"]
|
||||
|
||||
def test_explicit_null_clears_a_field(self, client: TestClient) -> None:
|
||||
"""An explicit null clears a field — so a form can still *remove* exposure
|
||||
under merge semantics (omit = preserve, null = clear). Removing the port
|
||||
goes with reach: off (a port-only process), which is what ServiceFields
|
||||
sends when the port is cleared."""
|
||||
resp = client.put(
|
||||
"/config/deployments/test-svc",
|
||||
json={"config": {"expose": None, "reach": "off"}},
|
||||
)
|
||||
assert resp.status_code == 200, resp.text
|
||||
after = client.get("/deployments/test-svc").json()["manifest"]
|
||||
assert after.get("expose") is None # cleared
|
||||
assert after["reach"] == "off"
|
||||
assert after["program"] == "test-svc-comp" # rest preserved
|
||||
|
||||
|
||||
class TestProgramEditSafety:
|
||||
def test_program_partial_patch_preserves(self, client: TestClient) -> None:
|
||||
"""save_program is the same footgun: a partial edit must not drop
|
||||
source/commands. Send only a description; the rest survives."""
|
||||
resp = client.put(
|
||||
"/config/programs/wired-in",
|
||||
json={"config": {"description": "renamed"}},
|
||||
)
|
||||
assert resp.status_code == 200, resp.text
|
||||
m = client.get("/programs/wired-in").json()["manifest"]
|
||||
assert m["description"] == "renamed" # change applied
|
||||
assert m["source"].endswith("wired-in") # source preserved
|
||||
assert m["commands"]["lint"] == [["make", "lint"]] # commands preserved
|
||||
@@ -63,7 +63,10 @@ class TestDeploymentDetail:
|
||||
data = response.json()
|
||||
assert data["id"] == "test-svc"
|
||||
assert "manifest" in data
|
||||
assert data["manifest"]["launcher"] == "python"
|
||||
# A deployed deployment that's defined in castle.yaml serves its editable
|
||||
# source spec (launcher nested under `run`), not the runtime view — so the
|
||||
# dashboard edit form gets reach/expose/defaults and can't strip them.
|
||||
assert data["manifest"]["run"]["launcher"] == "python"
|
||||
|
||||
def test_not_found(self, client: TestClient) -> None:
|
||||
"""Returns 404 for unknown component."""
|
||||
|
||||
Reference in New Issue
Block a user