diff --git a/docs/reference/mcp-deployment-tools.md b/docs/reference/mcp-deployment-tools.md index cc07999bc3..18df03a751 100644 --- a/docs/reference/mcp-deployment-tools.md +++ b/docs/reference/mcp-deployment-tools.md @@ -320,15 +320,19 @@ that performs four probes and returns a structured allow/deny matrix. "gateway_reachable": true, "internet_blocked": true, "agent_pods_unreachable": true, - "orchestrator_direct_blocked": true, + "orchestrator_api_reachable": true, "probe_job": "egg-probe-", "probe_pod_phase": "Succeeded" } ``` All four boolean fields should be `true` for a correctly isolated agent -(the agent can reach the gateway for proxied API calls, nothing else). -Any `false` indicates a NetworkPolicy regression. +(the agent can reach the gateway for proxied API calls and heartbeat the +orchestrator on `:9849`; nothing else). Any `false` indicates a +NetworkPolicy regression. `orchestrator_api_reachable` was previously +named `orchestrator_direct_blocked` with inverted polarity, which read +backwards from intent — `allow-agent-to-orchestrator` deliberately +permits the heartbeat path (#2652). **Probe Job design** (RISK-1 mitigation): @@ -370,7 +374,7 @@ result = await mcp.call_tool("validate_network_isolation", { assert result["gateway_reachable"] is True assert result["internet_blocked"] is True assert result["agent_pods_unreachable"] is True -assert result["orchestrator_direct_blocked"] is True +assert result["orchestrator_api_reachable"] is True ``` The expected allow/deny matrix is documented in diff --git a/integration_tests/conftest.py b/integration_tests/conftest.py index 855532b042..c8d8c2af4a 100644 --- a/integration_tests/conftest.py +++ b/integration_tests/conftest.py @@ -82,6 +82,12 @@ class EggStack(GatewayClientMixin): gateway_port: int proxy_port: int launcher_secret: str + # Lifecycle bearer for the orchestrator's /api/v1/deployment/* and + # other ``@require_lifecycle_secret`` routes. Sourced from the same + # gateway-secrets Secret the orchestrator pod mounts; empty when the + # cluster has no lifecycle-secret key, so deployment-route tests can + # skip rather than fail closed. + lifecycle_secret: str # Under k3s this carries the ``k8s-`` sentinel — legacy # docker-only fixtures key off the prefix to skip cleanly. Some tests # (e.g. test_stack_lifecycle, test_worktree_integration) still consume @@ -271,6 +277,31 @@ def _k8s_egg_stack() -> Generator[EggStack]: else: launcher_secret = os.environ.get("EGG_LAUNCHER_SECRET", secrets.token_urlsafe(32)) + # Pull the lifecycle bearer from the same Secret so tests targeting + # ``@require_lifecycle_secret`` routes (e.g. /api/v1/deployment/*) + # can authenticate. Optional: if the cluster doesn't expose this + # key the bearer is left empty and callers should skip cleanly. + lifecycle_result = subprocess.run( + [ + "kubectl", + "-n", + "egg-system", + "get", + "secret", + "gateway-secrets", + "-o", + "jsonpath={.data.lifecycle-secret}", + ], + capture_output=True, + text=True, + timeout=10, + check=False, + ) + if lifecycle_result.returncode == 0 and lifecycle_result.stdout: + lifecycle_secret = base64.b64decode(lifecycle_result.stdout).decode().strip() + else: + lifecycle_secret = "" + config_dir = tempfile.mkdtemp(prefix="egg-test-config-") _write_test_config(config_dir, launcher_secret) @@ -285,6 +316,7 @@ def _k8s_egg_stack() -> Generator[EggStack]: gateway_port=int(gateway_port_str), proxy_port=PROXY_PORT, launcher_secret=launcher_secret, + lifecycle_secret=lifecycle_secret, compose_project=f"k8s-{test_namespace}", config_dir=config_dir, isolated_network=test_namespace, @@ -327,6 +359,24 @@ def orchestrator_url(egg_stack: EggStack) -> str: return egg_stack.orchestrator_url +@pytest.fixture(scope="session") +def lifecycle_secret(egg_stack: EggStack) -> str: + """Lifecycle bearer for orchestrator `@require_lifecycle_secret` routes. + + Skips the test when the cluster's ``gateway-secrets`` Secret has no + ``lifecycle-secret`` key — auth-required routes can't be exercised + without it, and the auth-reject suite in + ``test_k8s_deployment_tools.py`` already covers the missing-secret + failure mode. + """ + if not egg_stack.lifecycle_secret: + pytest.skip( + "no lifecycle-secret key in gateway-secrets — auth-required " + "deployment-route tests need it" + ) + return egg_stack.lifecycle_secret + + @pytest.fixture(scope="session") def orchestrator_mcp_url(egg_stack: EggStack) -> str: """Streamable-HTTP URL for the orchestrator's MCP server. diff --git a/integration_tests/test_deployment_validation_logic.py b/integration_tests/test_deployment_validation_logic.py new file mode 100644 index 0000000000..e250cf7170 --- /dev/null +++ b/integration_tests/test_deployment_validation_logic.py @@ -0,0 +1,741 @@ +"""Integration coverage for deployment-validation route LOGIC (issue #2641). + +The sibling file ``test_k8s_deployment_tools.py`` covers the +``@require_lifecycle_secret`` parity for every #1759 deployment route +(401/503 on missing or wrong bearer). What it deliberately did *not* +cover is the post-auth behaviour of the validation routes, since the +lifecycle bearer wasn't surfaced through the shared ``EggStack`` +fixture. This file fills that gap for the two routes called out in +#2641: + +* ``POST /api/v1/deployment/validate-manifests`` +* ``POST /api/v1/deployment/validate-network-isolation`` + +(``validate_config`` — the third route named in #2641 — is not an HTTP +route. It is a pure MCP-side handler that runs Pydantic validation in +the orchestrator process and never touches the k3s cluster. Its +coverage lives in ``orchestrator/tests/test_mcp_tools.py`` under +``TestValidateConfig``; reproducing it in the k3s tier would add cost +without adding signal.) + +## Follow-up fixes shipped in the same PR + +The default-overlay / probe happy paths in the deployed orchestrator +were each broken in independent ways when this suite was first written; +the fixes ride this PR: + +* **#2647 — kustomize is now installed in the orchestrator image.** + ``_run_kustomize`` no longer raises ``kustomize_unavailable``. The + default-overlay path still 404s in CI because the egg repo isn't + bind-mounted into the orchestrator pod (a separate gap acknowledged + in #2647); locally with the local-overlay host mounts it returns + 200. +* **#2646 — orchestrator SA gained ``get,list`` on + ``apps/daemonsets`` and ``nodes`` (ClusterRole + ``egg-cluster-topology-reader``).** ``_detect_cni`` / ``_detect_k3s`` + now run, so ``validate-network-isolation`` launches the probe + against the Calico-equipped integration cluster instead of + short-circuiting. +* **#2648 — orchestrator SA gained ``list`` on ``apps/deployments`` + in ``egg-system``.** Tangential to #2641 but observed in the same + audit: ``_collect_egg_image_tags`` now returns populated image tags + instead of ``{}``. +* **#2652 — probe field renamed from ``orchestrator_direct_blocked`` + to ``orchestrator_api_reachable`` with flipped polarity.** The + agent→orchestrator heartbeat path is deliberately permitted; the + field now reads positively as a healthy-heartbeat signal. +""" + +from __future__ import annotations + +import concurrent.futures +import time +from pathlib import Path + +import pytest +import requests + +pytestmark = pytest.mark.integration + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +def _auth_headers(secret: str) -> dict[str, str]: + return {"Authorization": f"Bearer {secret}"} + + +def _post( + orchestrator_url: str, + path: str, + *, + secret: str, + body: dict, + timeout: int = 60, +) -> requests.Response: + return requests.post( + f"{orchestrator_url}{path}", + json=body, + headers={**_auth_headers(secret), "Content-Type": "application/json"}, + timeout=timeout, + ) + + +# --------------------------------------------------------------------------- +# validate_deployment_manifests +# --------------------------------------------------------------------------- + + +class TestValidateDeploymentManifestsLogic: + """``POST /api/v1/deployment/validate-manifests`` — post-auth behaviour. + + With #2647 fixed the orchestrator image now ships ``kustomize``. + The remaining gap (egg repo not bind-mounted in CI) is acknowledged + in #2647 and is out of scope for this PR; the default-overlay path + therefore returns 404 in CI and 200 locally with the local-overlay + host mounts. The tests below cover both shapes plus the 400 / 404 + error paths. + """ + + def test_missing_overlay_returns_404( + self, + orchestrator_url: str, + lifecycle_secret: str, + ) -> None: + """A relative ``overlay_path`` that doesn't exist returns 404.""" + resp = _post( + orchestrator_url, + "/api/v1/deployment/validate-manifests", + secret=lifecycle_secret, + body={"overlay_path": "k8s/does-not-exist-2641"}, + ) + assert resp.status_code == 404, ( + f"expected 404 for missing overlay, got {resp.status_code}: {resp.text[:500]}" + ) + body = resp.json() + assert body["success"] is False + assert "not found" in (body.get("message") or "").lower() + + def test_absolute_path_outside_repo_root_returns_400( + self, + orchestrator_url: str, + lifecycle_secret: str, + ) -> None: + """An absolute path that escapes the repo-root scope guard returns 400. + + Regression guard for the auth-gated probe-via-200/404 + differentiation worry called out in + ``orchestrator/routes/deployment.py``: even an authenticated + caller must not be able to use the route as an arbitrary + filesystem-existence probe. + """ + resp = _post( + orchestrator_url, + "/api/v1/deployment/validate-manifests", + secret=lifecycle_secret, + body={"overlay_path": "/etc/passwd"}, + ) + assert resp.status_code == 400, ( + f"expected 400 for traversal attempt, got {resp.status_code}: {resp.text[:500]}" + ) + body = resp.json() + assert body["success"] is False + assert "repo root" in (body.get("message") or "").lower() + + def test_relative_traversal_outside_repo_root_returns_400( + self, + orchestrator_url: str, + lifecycle_secret: str, + ) -> None: + """``../`` segments that resolve outside the repo root return 400. + + The route resolves the overlay path before the in-scope check, + so a relative traversal must be caught the same way as an + absolute one. This is a regression guard for the path-traversal + comment in the route's docstring. + """ + resp = _post( + orchestrator_url, + "/api/v1/deployment/validate-manifests", + secret=lifecycle_secret, + body={"overlay_path": "../../../../../etc"}, + ) + assert resp.status_code == 400, ( + f"expected 400 for relative traversal, got {resp.status_code}: {resp.text[:500]}" + ) + + def test_default_overlay_returns_404_or_200_depending_on_repo_mount( + self, + orchestrator_url: str, + lifecycle_secret: str, + ) -> None: + """Default overlay returns 200 (repo mounted) or 404 (repo absent). + + With #2647 fixed the orchestrator image ships ``kustomize``, so + 500 ``kustomize_unavailable`` is no longer a valid shape. The + remaining gap is whether the egg repo is bind-mounted at + ``/home/egg/repos``: + + * Local-dev (local overlay + ``$HOME/repos/egg`` populated): 200 + with rendered overlay + warnings list. + * CI integration tier (local overlay, ``$HOME/repos`` empty per + the workflow's seed step): 404 ``overlay not found``. + + Both are deliberate; 500 anywhere is a regression in either + the Dockerfile change or the route's error path. We pin to + exactly one expected outcome per environment by probing the + same host bind-mount path the orchestrator pod sees — a + regression that flipped 200↔404 in either environment would + otherwise still pass here. + """ + # The orchestrator pod's overlay search resolves relative paths + # under ``/home/egg/repos/egg`` (via the host bind-mount of + # ``$HOME/repos`` → ``/home/egg/repos``). The test runner can + # see the same overlay path through its own ``$HOME/repos`` — + # populated in local-dev, empty in CI per the workflow's seed + # step. Using ``Path.home()`` keeps this portable across + # ``/home/egg`` (sandbox) and ``/home/runner`` (CI). + overlay_on_host = Path.home() / "repos/egg/k8s/overlays/local" + expected = 200 if overlay_on_host.exists() else 404 + + resp = _post( + orchestrator_url, + "/api/v1/deployment/validate-manifests", + secret=lifecycle_secret, + body={}, + ) + assert resp.status_code == expected, ( + f"expected {expected} based on overlay presence at " + f"{overlay_on_host} (exists={overlay_on_host.exists()}); " + f"got {resp.status_code}: {resp.text[:500]}" + ) + body = resp.json() + if resp.status_code == 200: + assert body["success"] is True + assert "data" in body + assert "overlay_path" in body["data"] + assert "warnings" in body["data"] + else: + assert body["success"] is False + assert "not found" in (body.get("message") or "").lower(), ( + f"404 should be overlay-not-found; got: {body.get('message')!r}" + ) + + def test_re_validation_is_idempotent_on_error_path( + self, + orchestrator_url: str, + lifecycle_secret: str, + ) -> None: + """Two identical calls return identical status + message. + + Issue #2641's gap audit calls out "idempotent re-validation" as + an invariant we should lock in. Exercising it on the 404 error + path (the only deterministic shape in CI today — see B1/B2 in + the module docstring) is still a real regression guard: a + future refactor that adds caching, request-IDs, or transient + state in the route would surface as a diff between the two + responses. + """ + body = {"overlay_path": "k8s/does-not-exist-2641-idempotent"} + first = _post( + orchestrator_url, + "/api/v1/deployment/validate-manifests", + secret=lifecycle_secret, + body=body, + ) + second = _post( + orchestrator_url, + "/api/v1/deployment/validate-manifests", + secret=lifecycle_secret, + body=body, + ) + assert first.status_code == second.status_code + assert first.json()["success"] is second.json()["success"] + assert first.json().get("message") == second.json().get("message") + + +# --------------------------------------------------------------------------- +# validate_network_isolation +# --------------------------------------------------------------------------- + + +class TestValidateNetworkIsolationLogic: + """``POST /api/v1/deployment/validate-network-isolation`` — post-auth. + + With #2646 fixed the orchestrator SA can now list ``kube-system`` + DaemonSets and ``nodes`` cluster-wide, so ``_detect_cni`` resolves + to ``("calico", True)`` against the integration cluster (which + installs Calico via ``scripts/install-calico.sh``). The probe Job + actually launches; the happy-path test below exercises its result + shape. The earlier short-circuit assertion has been removed. + """ + + def test_invalid_pipeline_id_returns_400( + self, + orchestrator_url: str, + lifecycle_secret: str, + ) -> None: + """``pipeline_id`` with K8s-invalid characters (space) returns 400. + + ``_K8S_LABEL_VALUE_RE`` enforces RFC1123-ish label values to + avoid an opaque 422 from the apiserver when the Job is created. + """ + resp = _post( + orchestrator_url, + "/api/v1/deployment/validate-network-isolation", + secret=lifecycle_secret, + body={"pipeline_id": "bad id with spaces", "role": "coder"}, + ) + assert resp.status_code == 400, ( + f"expected 400 for invalid pipeline_id, got {resp.status_code}: {resp.text[:500]}" + ) + body = resp.json() + assert body["success"] is False + assert "pipeline_id" in (body.get("message") or "") + + def test_invalid_role_returns_400( + self, + orchestrator_url: str, + lifecycle_secret: str, + ) -> None: + """``role`` with K8s-invalid characters returns 400.""" + resp = _post( + orchestrator_url, + "/api/v1/deployment/validate-network-isolation", + secret=lifecycle_secret, + body={"pipeline_id": "p1", "role": "coder!"}, + ) + assert resp.status_code == 400, ( + f"expected 400 for invalid role, got {resp.status_code}: {resp.text[:500]}" + ) + body = resp.json() + assert body["success"] is False + assert "role" in (body.get("message") or "").lower() + + @pytest.mark.parametrize( + "pipeline_id", + [ + pytest.param("-leading-hyphen", id="leading-hyphen"), + pytest.param("trailing-hyphen-", id="trailing-hyphen"), + pytest.param(".leading-dot", id="leading-dot"), + pytest.param("x" * 64, id="too-long-64"), + ], + ) + def test_pipeline_id_regex_boundary_violations_return_400( + self, + orchestrator_url: str, + lifecycle_secret: str, + pipeline_id: str, + ) -> None: + """Boundary cases that violate ``_K8S_LABEL_VALUE_RE`` all reject. + + The regex is ``^[a-z0-9A-Z]([-._a-z0-9A-Z]{0,61}[a-z0-9A-Z])?$``: + + * Bookend chars must be alphanumeric (no leading/trailing ``-``, + ``.``, ``_``). + * Middle run is capped at 61 chars → total max 63. + + Each parametrize case is one corner of that envelope. A regex + regression that loosened any of these would let an apiserver + 422 leak through the route's 400 guard. + """ + resp = _post( + orchestrator_url, + "/api/v1/deployment/validate-network-isolation", + secret=lifecycle_secret, + body={"pipeline_id": pipeline_id, "role": "coder"}, + ) + assert resp.status_code == 400, ( + f"expected 400 for {pipeline_id!r}, got {resp.status_code}: {resp.text[:500]}" + ) + + @pytest.mark.parametrize( + "pipeline_id", + [ + pytest.param("a", id="single-char"), + pytest.param("x" * 63, id="max-length-63"), + pytest.param("p1.b2_c3-d4", id="middle-dot-underscore-hyphen"), + pytest.param("Pipe1", id="uppercase-allowed"), + ], + ) + def test_pipeline_id_regex_valid_at_boundaries_pass( + self, + orchestrator_url: str, + lifecycle_secret: str, + pipeline_id: str, + ) -> None: + """Valid-at-boundary labels pass the regex and reach the CNI gate. + + Companion to ``test_pipeline_id_regex_boundary_violations_return_400``: + single-char, max-length-63, and the full set of middle-position + special chars all must NOT reject at the label-validator stage. + They may then short-circuit at the CNI gate (B2 today) or run + the probe — but they must not 400. + """ + resp = _post( + orchestrator_url, + "/api/v1/deployment/validate-network-isolation", + secret=lifecycle_secret, + body={"pipeline_id": pipeline_id, "role": "coder"}, + ) + assert resp.status_code == 200, ( + f"expected 200 for valid label {pipeline_id!r}, got {resp.status_code}: " + f"{resp.text[:500]}" + ) + + def test_default_pipeline_id_and_role_pass_label_validation( + self, + orchestrator_url: str, + lifecycle_secret: str, + ) -> None: + """Omitted body → defaults ``pipeline_id=manual``/``role=coder`` pass. + + The label-validator runs before the CNI gate, so the test + passes regardless of whether the probe actually launches. + """ + resp = _post( + orchestrator_url, + "/api/v1/deployment/validate-network-isolation", + secret=lifecycle_secret, + body={}, + ) + # 200 either way: either the probe launches (post-B2 fix), or + # the route short-circuits with ``network_policy_enforcement_ + # not_detected``. A 400 here would mean the default values are + # rejecting against the label regex — that's the regression. + assert resp.status_code == 200, ( + f"expected 200 with default labels, got {resp.status_code}: {resp.text[:500]}" + ) + + def test_probe_runs_and_returns_expected_shape( + self, + orchestrator_url: str, + lifecycle_secret: str, + ) -> None: + """Happy path: with enforcement detected the probe runs and reports. + + Expected probe-output shape (per ``PROBE_COMMAND_TEMPLATE``): + + * ``gateway_reachable: True`` — ``allow-agent-to-gateway`` + permits agent→gateway:9848. # noqa: EGG002 + * ``internet_blocked: True`` — ``default-deny-egress`` blocks + arbitrary egress (curl example.com). + * ``agent_pods_unreachable: True`` — no policy allows agent→ + random-peer:80. + * ``orchestrator_api_reachable: True`` — ``allow-agent-to- + orchestrator`` permits the agent→orchestrator:9849 heartbeat + path. Renamed from ``orchestrator_direct_blocked`` (#2652); + the old name read backwards from intent. + """ + resp = _post( + orchestrator_url, + "/api/v1/deployment/validate-network-isolation", + secret=lifecycle_secret, + body={"pipeline_id": "happy-2641", "role": "coder"}, + timeout=90, + ) + assert resp.status_code == 200 + data = resp.json()["data"] + # Probe-launched shape, not the short-circuit shape. + assert "probe_id" in data, f"expected probe-launched shape with probe_id; got {data!r}" + result = data["result"] + assert result.get("gateway_reachable") is True + assert result.get("internet_blocked") is True + assert result.get("agent_pods_unreachable") is True + assert result.get("orchestrator_api_reachable") is True + + +# --------------------------------------------------------------------------- +# Cross-route invariants +# --------------------------------------------------------------------------- + + +class TestValidationRouteConcurrency: + """Both validation routes are read-only / per-call; concurrent calls must not interfere. + + Unlike ``rebuild_and_rollout`` (which has an in-process + ``_REBUILD_LOCK`` and rejects concurrent invocations with 409), + ``validate-manifests`` and ``validate-network-isolation`` have no + shared mutable state per call — concurrent requests must each + receive a self-consistent response. A regression that wired the + rebuild-lock into either of these would surface as a 409 here. + """ + + def test_concurrent_validate_manifests_calls_are_independent( + self, + orchestrator_url: str, + lifecycle_secret: str, + ) -> None: + """5 parallel calls return identical (404, success=false) responses.""" + body = {"overlay_path": "k8s/does-not-exist-2641-concurrency"} + + def _call() -> requests.Response: + return _post( + orchestrator_url, + "/api/v1/deployment/validate-manifests", + secret=lifecycle_secret, + body=body, + ) + + with concurrent.futures.ThreadPoolExecutor(max_workers=5) as ex: + results = list(ex.map(lambda _: _call(), range(5))) + + # All should be the same shape; no race-induced 5xx or 409. + statuses = {r.status_code for r in results} + assert statuses == {404}, f"got mixed/unexpected statuses: {statuses}" + for r in results: + body_json = r.json() + assert body_json["success"] is False + assert "not found" in (body_json.get("message") or "").lower() + + def test_concurrent_validate_network_isolation_calls_get_distinct_probe_ids( + self, + orchestrator_url: str, + lifecycle_secret: str, + ) -> None: + """Concurrent calls each get a distinct probe_id without 409 contention. + + With #2646 fixed the probe now launches, so each call produces + a ``probe_id`` and the test guards against a probe-id collision + regression — ``uuid.uuid4().hex[:12]`` is 48 bits of entropy, + more than enough for the 5-way fan-out, but a regression that + hard-coded the id would surface here. + """ + + def _call(i: int) -> requests.Response: + return _post( + orchestrator_url, + "/api/v1/deployment/validate-network-isolation", + secret=lifecycle_secret, + body={"pipeline_id": f"concur-{i}", "role": "coder"}, + timeout=90, + ) + + with concurrent.futures.ThreadPoolExecutor(max_workers=5) as ex: + results = list(ex.map(_call, range(5))) + + for r in results: + assert r.status_code == 200, ( + f"concurrent call got non-200: {r.status_code}: {r.text[:300]}" + ) + assert r.json()["success"] is True + + # If the probe ever runs (post-B2), distinct probe_ids confirm + # no collision. Under the current short-circuit there's no + # probe_id at all — that's also fine. + probe_ids = [r.json()["data"].get("probe_id") for r in results] + non_null = [p for p in probe_ids if p] + if non_null: + assert len(set(non_null)) == len(non_null), ( + f"concurrent calls produced duplicate probe_ids: {probe_ids}" + ) + + +class TestValidationRouteSelfConsistency: + """Cross-cutting invariants the routes must hold.""" + + def test_validation_routes_never_leak_secrets_in_error_messages( + self, + orchestrator_url: str, + lifecycle_secret: str, + ) -> None: + """The orchestrator must not echo the bearer back in any error response. + + The lifecycle secret is the production credential for every + ``@require_lifecycle_secret`` route; a bug that included the + Authorization header value in an error body (e.g. via a + broad ``request.get_data()`` dump) would leak it to anyone who + already had it — surfacing the regression for the next person + who runs these tests is the cheap guard. + + Check the full 64-hex-char secret, not just a prefix: a bug + that echoed the suffix or middle of the bearer (e.g. via a + log line that printed ``...{secret[-16:]}``) would slip past + a prefix-only check. + """ + # Drive a few error paths and inspect their bodies. + responses = [ + _post( + orchestrator_url, + "/api/v1/deployment/validate-manifests", + secret=lifecycle_secret, + body={"overlay_path": "/etc/passwd"}, + ), + _post( + orchestrator_url, + "/api/v1/deployment/validate-manifests", + secret=lifecycle_secret, + body={"overlay_path": "k8s/does-not-exist-2641-leak"}, + ), + _post( + orchestrator_url, + "/api/v1/deployment/validate-network-isolation", + secret=lifecycle_secret, + body={"pipeline_id": "leak test"}, + ), + ] + for resp in responses: + # Full-secret check: 64 hex chars is more than enough + # entropy that a false positive is impossible. Catches + # echoes of any contiguous substring of the secret (prefix, + # suffix, or middle). + assert lifecycle_secret not in resp.text, ( + f"response body contained the bearer secret in full — " + f"{resp.request.url}: {resp.text[:500]}" + ) + # Also guard against substring leaks (e.g. a bug that + # printed the last 16 hex chars). Use a couple of + # non-overlapping windows. + for start in (0, 16, 32, 48): + window = lifecycle_secret[start : start + 16] + assert window not in resp.text, ( + f"response body contained a 16-char window of the " + f"bearer secret starting at offset {start} — " + f"{resp.request.url}: {resp.text[:500]}" + ) + + def test_validation_routes_reject_invalid_json( + self, + orchestrator_url: str, + lifecycle_secret: str, + ) -> None: + """Malformed JSON yields the same response shape as an empty body. + + ``request.get_json(silent=True) or {}`` is the pattern in the + route handlers; on malformed input the handlers see ``{}`` and + run the default-body path. The original docstring framed this + as a "500 with Flask traceback" regression guard, but a switch + to non-silent ``get_json()`` actually surfaces as a Flask-default + 400 BadRequest page (no traceback) — so a bare ``"traceback" + not in text`` assertion would silently pass through the + regression. + + The real invariant the silent-mode path guarantees is that the + malformed-JSON response is identical to the empty-body response + (same status, same parsed body). Asserting that pins the + contract: any switch to non-silent ``get_json()`` would diverge + the two (400 BadRequest vs. the route's own default-path + response) and fail the assertion. + """ + for path in ( + "/api/v1/deployment/validate-manifests", + "/api/v1/deployment/validate-network-isolation", + ): + empty = requests.post( + f"{orchestrator_url}{path}", + json={}, + headers={ + **_auth_headers(lifecycle_secret), + "Content-Type": "application/json", + }, + timeout=90, + ) + malformed = requests.post( + f"{orchestrator_url}{path}", + data="this is not json", + headers={ + **_auth_headers(lifecycle_secret), + "Content-Type": "application/json", + }, + timeout=90, + ) + # Belt-and-braces: a 500 with a Flask traceback would also + # be a regression. Keep the cheap negative guard. + assert "traceback" not in malformed.text.lower(), ( + f"{path}: malformed JSON produced a Flask traceback: {malformed.text[:500]}" + ) + # The core invariant: same status, same parsed body shape. + assert malformed.status_code == empty.status_code, ( + f"{path}: malformed-JSON status {malformed.status_code} " + f"diverged from empty-body status {empty.status_code} — " + f"upstream may have flipped from get_json(silent=True) to " + f"non-silent. Body: {malformed.text[:500]}" + ) + try: + empty_body = empty.json() + malformed_body = malformed.json() + except ValueError: + pytest.fail( + f"{path}: response was not JSON — " + f"empty={empty.text[:200]!r} malformed={malformed.text[:200]!r}" + ) + assert malformed_body.get("success") is empty_body.get("success"), ( + f"{path}: success-flag diverged between malformed " + f"({malformed_body.get('success')!r}) and empty " + f"({empty_body.get('success')!r}) bodies" + ) + + +# --------------------------------------------------------------------------- +# Probe-job cleanup +# --------------------------------------------------------------------------- + + +class TestProbeJobCleanup: + """Belt-and-braces: no orphan probe Jobs after the route returns. + + The route uses a ``try/finally`` to call ``_delete_probe_job`` and + sets ``ttlSecondsAfterFinished: 0`` on the Job. With #2646 fixed + the probe actually launches, so this assertion catches a cleanup + regression where the finally path failed to delete the Job (or the + ttl-after-finished GC failed to fire). + """ + + def test_no_orphan_probe_jobs_after_call( + self, + orchestrator_url: str, + lifecycle_secret: str, + ) -> None: + import subprocess + + # Scope the selector to this test's pipeline_id so a concurrent + # probe from another test in the same session (e.g. + # TestValidationRouteConcurrency::test_concurrent_validate_*) + # can't be misattributed as this test's leak. The label is set + # in ``_build_probe_job_manifest`` (orchestrator/routes/ + # deployment.py). + pipeline_id = "cleanup-2641" + _post( + orchestrator_url, + "/api/v1/deployment/validate-network-isolation", + secret=lifecycle_secret, + body={"pipeline_id": pipeline_id, "role": "coder"}, + timeout=90, + ) + + # Allow up to a few seconds for ttlSecondsAfterFinished=0 to + # reap any Job that did launch — the API call returned, but + # the controller may not have run the GC pass yet. + deadline = time.time() + 15 + leftover: list[str] = [] + while time.time() < deadline: + result = subprocess.run( + [ + "kubectl", + "-n", + "egg-agents", + "get", + "jobs", + "-l", + f"egg.probe=true,egg.pipeline.id={pipeline_id}", + "-o", + "jsonpath={.items[*].metadata.name}", + ], + capture_output=True, + text=True, + timeout=10, + check=False, + ) + if result.returncode != 0: + pytest.skip(f"kubectl listing failed: {result.stderr!r}") + names = [n for n in result.stdout.strip().split() if n] + if not names: + return # success + leftover = names + time.sleep(1) + pytest.fail( + f"probe Jobs still present after route returned (route should " + f"clean up via finally + ttlSecondsAfterFinished=0): {leftover}" + ) diff --git a/k8s/base/rbac.yaml b/k8s/base/rbac.yaml index 0b460e67e4..ab69cd69e9 100644 --- a/k8s/base/rbac.yaml +++ b/k8s/base/rbac.yaml @@ -63,9 +63,12 @@ metadata: app.kubernetes.io/name: orchestrator app.kubernetes.io/part-of: egg rules: + # `list` is required by `_collect_egg_image_tags` in + # orchestrator/routes/deployment.py (#2648); without it the wrapper + # 403s and `get_deployment_context` reports `images_unavailable`. - apiGroups: ["apps"] resources: ["deployments"] - verbs: ["get"] + verbs: ["get", "list"] - apiGroups: [""] resources: ["pods"] verbs: ["get", "list"] @@ -89,3 +92,72 @@ subjects: - kind: ServiceAccount name: egg-orchestrator namespace: egg-system +--- +# Cluster-scope reads for cluster topology probes used by +# `get_deployment_context` and `validate_network_isolation`. Only +# `nodes` legitimately needs cluster scope (cluster-scoped resource); +# `_detect_cni` / `_detect_k3s` list DaemonSets only in `kube-system`, +# so that grant lives in a namespaced Role + RoleBinding below to keep +# the cluster-wide read minimal (least-privilege per review feedback). +# Without these bindings the heuristics always returned null/false, +# masking real k3s + CNI state (#2646). +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + name: egg-cluster-topology-reader + labels: + app.kubernetes.io/name: orchestrator + app.kubernetes.io/part-of: egg +rules: + - apiGroups: [""] + resources: ["nodes"] + verbs: ["get", "list"] +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRoleBinding +metadata: + name: egg-cluster-topology-reader + labels: + app.kubernetes.io/name: orchestrator + app.kubernetes.io/part-of: egg +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: ClusterRole + name: egg-cluster-topology-reader +subjects: + - kind: ServiceAccount + name: egg-orchestrator + namespace: egg-system +--- +# `_detect_cni` / `_detect_k3s` only call +# `list_namespaced_daemon_set("kube-system")`; the DaemonSet read is +# namespace-scoped, not cluster-wide. +apiVersion: rbac.authorization.k8s.io/v1 +kind: Role +metadata: + name: egg-kube-system-topology-reader + namespace: kube-system + labels: + app.kubernetes.io/name: orchestrator + app.kubernetes.io/part-of: egg +rules: + - apiGroups: ["apps"] + resources: ["daemonsets"] + verbs: ["get", "list"] +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: RoleBinding +metadata: + name: egg-kube-system-topology-reader + namespace: kube-system + labels: + app.kubernetes.io/name: orchestrator + app.kubernetes.io/part-of: egg +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: Role + name: egg-kube-system-topology-reader +subjects: + - kind: ServiceAccount + name: egg-orchestrator + namespace: egg-system diff --git a/orchestrator/Dockerfile b/orchestrator/Dockerfile index 7880168737..56aeb8e90c 100644 --- a/orchestrator/Dockerfile +++ b/orchestrator/Dockerfile @@ -5,6 +5,24 @@ RUN apt-get update && apt-get install -y \ git curl gosu \ && rm -rf /var/lib/apt/lists/* +# Install kustomize so `validate_deployment_manifests` (POST +# /api/v1/deployment/validate-manifests) can render overlays. Without +# it `_run_kustomize` falls through both kustomize and `kubectl +# kustomize` invocations and the route returns 500 +# `kustomize_unavailable` for every call (#2647). Pinned to a known- +# good release; bump deliberately. SHA256 is the published checksum +# for the linux_amd64 tarball (see checksums.txt alongside the +# release); bumping KUSTOMIZE_VERSION requires updating +# KUSTOMIZE_SHA256 in lockstep (#2681). +ARG KUSTOMIZE_VERSION=5.6.0 +ARG KUSTOMIZE_SHA256=54e4031ddc4e7fc59e408da29e7c646e8e57b8088c51b84b3df0864f47b5148f +RUN curl -fsSL "https://github.com/kubernetes-sigs/kustomize/releases/download/kustomize%2Fv${KUSTOMIZE_VERSION}/kustomize_v${KUSTOMIZE_VERSION}_linux_amd64.tar.gz" \ + -o /tmp/kustomize.tar.gz \ + && echo "${KUSTOMIZE_SHA256} /tmp/kustomize.tar.gz" | sha256sum -c - \ + && tar -xzf /tmp/kustomize.tar.gz -C /usr/local/bin kustomize \ + && rm /tmp/kustomize.tar.gz \ + && chmod +x /usr/local/bin/kustomize + # Create egg user with UID/GID 1000 as default RUN groupadd -g 1000 egg && \ useradd -m -u 1000 -g 1000 -s /bin/bash egg diff --git a/orchestrator/mcp_tools.py b/orchestrator/mcp_tools.py index 003a90fc11..198274d193 100644 --- a/orchestrator/mcp_tools.py +++ b/orchestrator/mcp_tools.py @@ -1085,7 +1085,7 @@ def _is_timeout_error(exc: BaseException) -> bool: "Spawn a throwaway probe Job in the egg-agents namespace to verify " "Calico NetworkPolicy enforcement. Returns a structured " "{gateway_reachable, internet_blocked, agent_pods_unreachable, " - "orchestrator_direct_blocked} result. The Job self-deletes on exit " + "orchestrator_api_reachable} result. The Job self-deletes on exit " "(ttlSecondsAfterFinished=0). Only available on the Kubernetes " "runtime and on CNIs that enforce NetworkPolicies." ), diff --git a/orchestrator/routes/deployment.py b/orchestrator/routes/deployment.py index 9431e41068..187ec0a0af 100644 --- a/orchestrator/routes/deployment.py +++ b/orchestrator/routes/deployment.py @@ -930,7 +930,7 @@ def prune_worktrees_proxy() -> tuple[Response, int]: probe() { local url="$1" - curl --silent --show-error --max-time 3 -o /dev/null -w '%{http_code}' "$url" || echo 000 + curl --silent --max-time 3 -o /dev/null -w '%{http_code}' "$url" 2>/dev/null || true } gw=$(probe "$gateway_url/api/v1/health") @@ -944,7 +944,17 @@ def prune_worktrees_proxy() -> tuple[Response, int]: "gateway_reachable": "$gw".startswith("2") or "$gw".startswith("3"), "internet_blocked": "$internet" == "000", "agent_pods_unreachable": "$peer" == "000", - "orchestrator_direct_blocked": "$orch" == "000", + # allow-agent-to-orchestrator (k8s/base/network-policies.yaml) + # deliberately permits agent->orchestrator:9849 so agents can + # heartbeat. So on a correctly-configured cluster this is True; + # False is the regression signal (heartbeat path is broken). The + # field was previously named orchestrator_direct_blocked with + # inverted polarity, which read backwards from intent (#2652). + # NOTE: this heredoc is unquoted (< str: + # The probe writes JSON to stdout. The kubernetes-python client's + # ApiClient.deserialize() runs json.loads() on every response body + # before coercing to the declared response_type, so a JSON-shaped + # pod log gets parsed to a dict and then str()'d back, yielding the + # Python dict repr (single quotes, ``True``) instead of the + # original JSON. _preload_content=False bypasses that path and + # returns the urllib3 HTTPResponse so we can decode the raw bytes. + # + # With ``_preload_content=False`` the actual network read happens at + # ``.data`` access (urllib3 reads-to-EOF lazily and caches), so the + # ``try/except`` must wrap the ``.data`` access too — otherwise a + # mid-stream connection reset or malformed transfer-encoding would + # propagate up and 500 the route handler. try: - raw = k8s.core_api.read_namespaced_pod_log(name=pod_name, namespace=namespace) + raw = k8s.core_api.read_namespaced_pod_log( + name=pod_name, namespace=namespace, _preload_content=False + ) + if raw is None: + return "" + data = getattr(raw, "data", raw) except Exception as exc: logger.warning("probe log read failed", pod=pod_name, error=str(exc)) return "" - return str(raw) if raw is not None else "" + if data is None: + return "" + if isinstance(data, bytes): + return data.decode("utf-8", errors="replace") + return str(data) def _delete_probe_job(k8s: Any, namespace: str, probe_id: str) -> None: diff --git a/orchestrator/tests/test_deployment_routes.py b/orchestrator/tests/test_deployment_routes.py index fb8ec799e9..bad1a00eb1 100644 --- a/orchestrator/tests/test_deployment_routes.py +++ b/orchestrator/tests/test_deployment_routes.py @@ -839,7 +839,7 @@ def test_enforcing_cni_submits_probe_and_returns_result(self, client, monkeypatc fake_pod.metadata.name = "egg-probe-abc123" fake_log = ( '{"gateway_reachable": true, "internet_blocked": true, ' - '"agent_pods_unreachable": true, "orchestrator_direct_blocked": true}' + '"agent_pods_unreachable": true, "orchestrator_api_reachable": true}' ) with ( @@ -1474,7 +1474,7 @@ def test_template_references_expected_env_vars(self): "gateway_reachable", "internet_blocked", "agent_pods_unreachable", - "orchestrator_direct_blocked", + "orchestrator_api_reachable", ): assert key in PROBE_COMMAND_TEMPLATE @@ -1484,6 +1484,172 @@ def test_template_does_not_reference_secrets(self): assert "EGG_LIFECYCLE_SECRET" not in PROBE_COMMAND_TEMPLATE assert "EGG_SESSION_TOKEN" not in PROBE_COMMAND_TEMPLATE + def test_template_contains_no_backticks(self): + """Backticks in the unquoted ``< - Recent Warning events (): - Log tail matches: `` → `` (line ) - Env keys present (): , , , ... (all values redacted) -- Egress probe: gateway_reachable=, internet_blocked=, agent_pods_unreachable=, orchestrator_direct_blocked= +- Egress probe: gateway_reachable=, internet_blocked=, agent_pods_unreachable=, orchestrator_api_reachable= - Pattern classifier: / `` → ### Per-primitive data