From b1054877908de952700944ef7629408426fceec2 Mon Sep 17 00:00:00 2001 From: Paul Payne Date: Mon, 29 Jun 2026 07:04:06 -0700 Subject: [PATCH] Fix gateway-route drift when a service is edited MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Editing a service's routing (port, path_prefix, host) and then hitting "apply" or "gateway reload" — rather than a full deploy — left the running Caddyfile reflecting the old routing. The Caddyfile was generated from the derived registry.yaml snapshot, which only `deploy()` rebuilds from castle.yaml; apply/reload regenerated from that stale snapshot. Make castle.yaml the single source of truth for local routes: - caddyfile.py: new shared `service_proxy_targets()` derives a service's (proxy_path, proxy_host, port, base_url) from its spec. `compute_routes` now builds local routes from `config.services` when castle.yaml is loadable, falling back to `registry.deployed` only when it isn't. Since deploy/apply/reload all funnel through `compute_routes`, every regeneration path now tracks the spec. Mesh/remote and static-frontend routes unchanged. - deploy.py: `_build_deployed_service` uses the same shared deriver, so the written registry and the computed routes can't diverge. - config_editor.py: `/config/apply` now runs a full `deploy()` (rebuilding units + Caddyfile and reloading the gateway) before restarting services, so changed unit ExecStarts actually take effect. Scheduled jobs are no longer restarted on apply. Drops the now-unused direct caddy-reload path. - Regression tests: a stale registry's old port/path loses to castle.yaml; registry fallback still works when config is unavailable. --- castle-api/src/castle_api/config_editor.py | 55 ++++++-------- core/src/castle_core/deploy.py | 11 ++- core/src/castle_core/generators/caddyfile.py | 78 ++++++++++++++++--- core/tests/test_caddyfile.py | 80 +++++++++++++++++++- 4 files changed, 176 insertions(+), 48 deletions(-) diff --git a/castle-api/src/castle_api/config_editor.py b/castle-api/src/castle_api/config_editor.py index a61816d..438e122 100644 --- a/castle-api/src/castle_api/config_editor.py +++ b/castle-api/src/castle_api/config_editor.py @@ -3,7 +3,6 @@ from __future__ import annotations import asyncio -import shutil import yaml from fastapi import APIRouter, HTTPException, status @@ -323,14 +322,32 @@ def delete_job(name: str) -> dict: @router.post("/apply", response_model=ApplyResponse) async def apply_config() -> ApplyResponse: - """Apply config: restart managed services + regenerate and reload gateway.""" - registry = get_registry() + """Apply config: rebuild runtime from castle.yaml, then restart services. + + Runs a full ``deploy`` so the registry, systemd units, and Caddyfile are all + regenerated from the current castle.yaml (and the gateway reloaded) — this is + what keeps the running config from drifting behind an edit. Then restarts the + managed services so the freshly written units take effect (``deploy`` only + daemon-reloads; a running unit keeps its old ExecStart until restarted). + Scheduled jobs are left alone — applying config shouldn't fire every job. + """ + from castle_core.deploy import deploy + actions: list[str] = [] errors: list[str] = [] - # Restart managed services + # Rebuild registry + units + Caddyfile from castle.yaml off the event loop + # (deploy is blocking: it shells out to systemctl and the gateway). + try: + result = await asyncio.to_thread(deploy) + except Exception as e: + return ApplyResponse(ok=False, actions=actions, errors=[f"Deploy failed: {e}"]) + actions.extend(result.messages) + + # Restart managed services so the new units take effect (skip scheduled jobs). + registry = result.registry or get_registry() for name, deployed in registry.deployed.items(): - if not deployed.managed: + if not deployed.managed or deployed.schedule: continue unit = f"castle-{name}.service" ok, output = await _systemctl("restart", unit) @@ -339,24 +356,6 @@ async def apply_config() -> ApplyResponse: else: errors.append(f"Failed to restart {name}: {output}") - # Reload gateway - from castle_core.config import SPECS_DIR, ensure_dirs - from castle_core.generators.caddyfile import generate_caddyfile_from_registry - - ensure_dirs() - caddyfile_path = SPECS_DIR / "Caddyfile" - caddyfile_path.write_text(generate_caddyfile_from_registry(registry)) - actions.append("Generated Caddyfile") - - if shutil.which("caddy"): - ok, output = await _run( - "caddy", "reload", "--config", str(caddyfile_path), "--adapter", "caddyfile" - ) - if ok: - actions.append("Reloaded gateway") - else: - errors.append(f"Gateway reload failed: {output}") - await broadcast("config-changed", {"actions": actions}) return ApplyResponse(ok=len(errors) == 0, actions=actions, errors=errors) @@ -372,13 +371,3 @@ async def _systemctl(action: str, unit: str) -> tuple[bool, str]: ) stdout, stderr = await proc.communicate() return proc.returncode == 0, (stdout or stderr or b"").decode().strip() - - -async def _run(*args: str) -> tuple[bool, str]: - proc = await asyncio.create_subprocess_exec( - *args, - stdout=asyncio.subprocess.PIPE, - stderr=asyncio.subprocess.PIPE, - ) - stdout, stderr = await proc.communicate() - return proc.returncode == 0, (stdout or stderr or b"").decode().strip() diff --git a/core/src/castle_core/deploy.py b/core/src/castle_core/deploy.py index f8a8ae9..02bd8dc 100644 --- a/core/src/castle_core/deploy.py +++ b/core/src/castle_core/deploy.py @@ -22,7 +22,10 @@ from castle_core.config import ( load_config, resolve_env_split, ) -from castle_core.generators.caddyfile import generate_caddyfile_from_registry +from castle_core.generators.caddyfile import ( + generate_caddyfile_from_registry, + service_proxy_targets, +) from castle_core.generators.systemd import ( SECRET_ENV_DIR, generate_timer, @@ -221,10 +224,12 @@ def _build_deployed_service( if svc.manage and svc.manage.systemd and not svc.manage.systemd.enable: managed = False - port = None + # Routing fields (port/proxy_path/proxy_host/base_url) come from the shared + # deriver, so the registry written here and the Caddyfile computed from + # castle.yaml stay in lockstep. + proxy_path, proxy_host, port, base_url = service_proxy_targets(name, svc) health_path = None if svc.expose and svc.expose.http: - port = svc.expose.http.internal.port health_path = svc.expose.http.health_path # Env is exactly what's declared in defaults.env — no hidden convention diff --git a/core/src/castle_core/generators/caddyfile.py b/core/src/castle_core/generators/caddyfile.py index b48f59e..c3f9de3 100644 --- a/core/src/castle_core/generators/caddyfile.py +++ b/core/src/castle_core/generators/caddyfile.py @@ -17,7 +17,8 @@ from __future__ import annotations from dataclasses import dataclass from pathlib import Path -from castle_core.config import SPECS_DIR +from castle_core.config import SPECS_DIR, CastleConfig +from castle_core.manifest import ServiceSpec from castle_core.registry import NodeRegistry @@ -36,9 +37,60 @@ class GatewayRoute: return not self.address.startswith("/") +# (proxy_path, proxy_host, port, base_url) for a service's gateway route(s). +ProxyTargets = tuple[str | None, str | None, int | None, str | None] + + +def service_proxy_targets(name: str, svc: ServiceSpec) -> ProxyTargets: + """Derive a service's gateway routing fields from its spec. + + The single source of truth shared by the registry build (``deploy``) and + route computation (``compute_routes``), so the deployed registry and a + freshly regenerated Caddyfile can never disagree about ports/paths/hosts. + """ + port = None + if svc.expose and svc.expose.http: + port = svc.expose.http.internal.port + + proxy_path = None + proxy_host = None + if svc.proxy and svc.proxy.caddy and svc.proxy.caddy.enable: + caddy = svc.proxy.caddy + proxy_host = caddy.host + if caddy.path_prefix: + proxy_path = caddy.path_prefix + elif not caddy.host: + # No explicit path and no host → default to /. + proxy_path = f"/{name}" + + base_url = getattr(svc.run, "base_url", None) + return proxy_path, proxy_host, port, base_url + + +def _local_proxy_targets( + config: CastleConfig | None, registry: NodeRegistry +) -> list[tuple[str, ProxyTargets]]: + """Local services' routing fields, name-sorted for deterministic output. + + Prefers ``castle.yaml`` (``config.services``) as the source of truth so a + regenerated Caddyfile always reflects the current spec — this is what closes + the registry-staleness drift. Falls back to the deployed registry snapshot + only when config isn't available (load failed, or a pure-registry context). + """ + services = getattr(config, "services", None) + if services is not None: + return sorted( + (name, service_proxy_targets(name, svc)) for name, svc in services.items() + ) + return sorted( + (name, (d.proxy_path, d.proxy_host, d.port, d.base_url)) + for name, d in registry.deployed.items() + ) + + def compute_routes( registry: NodeRegistry, - config: object | None = None, + config: CastleConfig | None = None, remote_registries: dict[str, NodeRegistry] | None = None, ) -> list[GatewayRoute]: """Build the full ordered list of gateway routes (host, proxy, remote, static). @@ -57,18 +109,22 @@ def compute_routes( routes: list[GatewayRoute] = [] local_paths: set[str] = set() + # Local proxy routes, derived from castle.yaml when available (else the + # deployed registry) so a regenerated Caddyfile tracks the current spec. + local = _local_proxy_targets(config, registry) + # Host-based proxy routes (whole host → backend root). - for name, d in registry.deployed.items(): - if d.proxy_host and (d.port or d.base_url): - target = d.base_url or f"localhost:{d.port}" - routes.append(GatewayRoute(d.proxy_host, "proxy", target, name, node)) + for name, (proxy_path, proxy_host, port, base_url) in local: + if proxy_host and (port or base_url): + target = base_url or f"localhost:{port}" + routes.append(GatewayRoute(proxy_host, "proxy", target, name, node)) # Path-prefix proxy routes. - for name, d in registry.deployed.items(): - if d.proxy_path and (d.port or d.base_url): - local_paths.add(d.proxy_path) - target = d.base_url or f"localhost:{d.port}" - routes.append(GatewayRoute(d.proxy_path, "proxy", target, name, node)) + for name, (proxy_path, proxy_host, port, base_url) in local: + if proxy_path and (port or base_url): + local_paths.add(proxy_path) + target = base_url or f"localhost:{port}" + routes.append(GatewayRoute(proxy_path, "proxy", target, name, node)) # Cross-node routes — local paths take precedence. if remote_registries: diff --git a/core/tests/test_caddyfile.py b/core/tests/test_caddyfile.py index 7ab6d08..67fc900 100644 --- a/core/tests/test_caddyfile.py +++ b/core/tests/test_caddyfile.py @@ -5,7 +5,20 @@ from __future__ import annotations import pytest -from castle_core.generators.caddyfile import generate_caddyfile_from_registry +from castle_core.config import CastleConfig, GatewayConfig +from castle_core.generators.caddyfile import ( + compute_routes, + generate_caddyfile_from_registry, +) +from castle_core.manifest import ( + CaddySpec, + ExposeSpec, + HttpExposeSpec, + HttpInternal, + ProxySpec, + RunPython, + ServiceSpec, +) from castle_core.registry import Deployment, NodeConfig, NodeRegistry @@ -145,6 +158,71 @@ class TestCaddyfileFromRegistry: assert "reverse_proxy localhost:9002" in caddyfile +def _service(port: int, path: str | None = None, host: str | None = None) -> ServiceSpec: + """A minimal python ServiceSpec with an HTTP port and a caddy proxy route.""" + caddy = CaddySpec(path_prefix=path, host=host) + return ServiceSpec( + run=RunPython(runner="python", program="svc"), + expose=ExposeSpec(http=HttpExposeSpec(internal=HttpInternal(port=port))), + proxy=ProxySpec(caddy=caddy), + ) + + +def _config(services: dict[str, ServiceSpec]) -> CastleConfig: + return CastleConfig( + root=None, # type: ignore[arg-type] + gateway=GatewayConfig(port=9000), + repo=None, + programs={}, + services=services, + jobs={}, + ) + + +class TestLocalRoutesFromConfig: + """castle.yaml is the source of truth for local routes — a regenerated + Caddyfile must track the spec even when the deployed registry is stale. + This is the regression guard for the service-edit drift.""" + + def test_config_port_overrides_stale_registry(self) -> None: + # Registry was deployed with the OLD port; castle.yaml now says 8002. + registry = _make_registry( + deployed={ + "app": Deployment( + runner="python", run_cmd=["app"], port=8001, proxy_path="/app" + ), + } + ) + config = _config({"app": _service(port=8002, path="/app")}) + routes = compute_routes(registry, config) + targets = {r.target for r in routes if r.address == "/app"} + assert targets == {"localhost:8002"} # config wins, not the stale 8001 + + def test_config_path_overrides_stale_registry(self) -> None: + registry = _make_registry( + deployed={ + "app": Deployment( + runner="python", run_cmd=["app"], port=8001, proxy_path="/old" + ), + } + ) + config = _config({"app": _service(port=8001, path="/new")}) + addrs = {r.address for r in compute_routes(registry, config) if r.kind == "proxy"} + assert addrs == {"/new"} + + def test_falls_back_to_registry_without_config(self) -> None: + # No config available (load_config is isolated to raise) → use registry. + registry = _make_registry( + deployed={ + "app": Deployment( + runner="python", run_cmd=["app"], port=8001, proxy_path="/app" + ), + } + ) + caddyfile = generate_caddyfile_from_registry(registry) + assert "reverse_proxy localhost:8001" in caddyfile + + class TestCaddyfileRemoteRegistries: """Tests for cross-node routing in Caddyfile."""