From 855febddd1b38e0bcce634fac7d70eead64c5398 Mon Sep 17 00:00:00 2001 From: James Wiesebron Date: Tue, 12 May 2026 11:03:11 -0700 Subject: [PATCH 1/9] test(integration): cover deployment-validation routes past auth gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #2641. Adds `integration_tests/test_deployment_validation_logic.py` exercising post-auth behaviour of `POST /api/v1/deployment/validate-manifests` and `POST /api/v1/deployment/validate-network-isolation`. The existing sibling `test_k8s_deployment_tools.py` covers `@require_lifecycle_secret` parity (401/503 on missing/wrong bearer) but stops at the auth gate because the lifecycle bearer wasn't surfaced through the shared `EggStack` fixture. `integration_tests/conftest.py` now reads `lifecycle-secret` from `gateway-secrets` alongside `launcher-secret` and exposes a session-scoped `lifecycle_secret` fixture that skips when the cluster Secret has no such key. Surfaced bugs (filed as follow-ups; xfail or current-state assertions referenced from the test module's docstring): - #2646 — orchestrator SA can't list `kube-system` DaemonSets, so `validate-network-isolation` always short-circuits with `network_policy_enforcement_not_detected` in production. - #2647 — orchestrator container has no `kustomize`/`kubectl` on PATH, so `validate-manifests` returns 500 `kustomize_unavailable` for any default-overlay call. - #2648 — orchestrator SA can `get` but not `list` Deployments in `egg-system`, so `get_deployment_context` always reports `images_unavailable: true`. - #2652 — probe field `orchestrator_direct_blocked` is misleading (`allow-agent-to-orchestrator` policy intentionally permits the same path the field "checks"). `validate_config` is deliberately out of scope for the k3s tier: it's an MCP-side Pydantic handler with no HTTP route. Its coverage stays in `orchestrator/tests/test_mcp_tools.py::TestValidateConfig`. --- integration_tests/conftest.py | 50 ++ .../test_deployment_validation_logic.py | 717 ++++++++++++++++++ 2 files changed, 767 insertions(+) create mode 100644 integration_tests/test_deployment_validation_logic.py diff --git a/integration_tests/conftest.py b/integration_tests/conftest.py index d81b0c418a..7a7b99b165 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 def gateway_session(egg_stack: EggStack) -> Generator[dict[str, Any]]: """Function-scoped fixture: create a gateway session for isolation. diff --git a/integration_tests/test_deployment_validation_logic.py b/integration_tests/test_deployment_validation_logic.py new file mode 100644 index 0000000000..a1dbd1c827 --- /dev/null +++ b/integration_tests/test_deployment_validation_logic.py @@ -0,0 +1,717 @@ +"""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.) + +## Bugs surfaced while building the suite (filed as follow-ups) + +The default-overlay / probe happy paths in the deployed orchestrator +are currently broken in three independent ways. The tests below lock +in the *observable* behaviour today (so any silent fix would flip the +assertion and force a deliberate test update); the happy-path variants +are marked ``xfail(strict=True)`` and point at the relevant bug. + +* **#2647 — orchestrator container has no ``kustomize``/``kubectl`` on + PATH.** ``orchestrator/Dockerfile`` installs ``git curl gosu`` only, + so ``_run_kustomize`` falls through both subprocess invocations and + raises ``kustomize_unavailable``. Any default-overlay validation + returns HTTP 500. +* **#2646 — orchestrator ServiceAccount cannot list DaemonSets in + ``kube-system`` or nodes cluster-wide.** ``_detect_cni`` and + ``_detect_k3s`` both rely on these reads, so ``validate-network- + isolation`` always short-circuits with + ``network_policy_enforcement_not_detected`` and ``validate- + manifests`` always reports ``is_k3s=False`` even on a real k3s + cluster (the k3s-gated image-tag rule never fires). +* **#2648 — orchestrator ServiceAccount can only ``get`` (not + ``list``) Deployments in ``egg-system``.** Tangential to #2641 but + observed in the same audit: ``_collect_egg_image_tags`` always + returns ``{}`` and ``get_deployment_context`` always sets + ``images_unavailable: true`` in production. + +The tests below do not depend on any of these bugs being fixed. +""" + +from __future__ import annotations + +import concurrent.futures +import time + +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. + + The orchestrator container ships without ``kustomize``/``kubectl`` and + without the egg repo bind-mounted, so the happy-path (rendered overlay + + warnings list) cannot run in CI today (see B1 in the module + docstring). The tests here cover what is reachable: the 404 / 400 + error paths and the deterministic 500 the missing tooling produces. + """ + + 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_in_deployed_orchestrator_returns_500_today( + self, + orchestrator_url: str, + lifecycle_secret: str, + ) -> None: + """Default overlay against a real orchestrator pod returns 500 today (B1). + + With the egg repo bind-mounted at ``/home/egg/repos`` (local + overlay), the route finds ``k8s/overlays/local`` but the + orchestrator container has neither ``kustomize`` nor ``kubectl`` + installed, so ``_run_kustomize`` raises ``kustomize_unavailable`` + and the route returns 500. Locking in the current observable + behaviour so the contract is explicit; when B1 is fixed this + assertion will need to flip to 200. + + The fixture path that mounts the repo only exists under the + local overlay; in CI the repo isn't mounted at all and the + route returns 404 instead. Accept either to keep the test + portable across both deployment shapes. + """ + resp = _post( + orchestrator_url, + "/api/v1/deployment/validate-manifests", + secret=lifecycle_secret, + body={}, + ) + # Two valid shapes today: + # - 500 kustomize_unavailable: repo IS mounted (local overlay + # pattern), overlay found, kustomize missing. + # - 404 overlay not found: repo is NOT mounted (CI default + # overlay isn't reachable from the orchestrator pod). + # Both expose a real gap; 200 would be the post-fix state. + assert resp.status_code in (404, 500), ( + f"expected 404 or 500 in current deployment, got {resp.status_code}: {resp.text[:500]}" + ) + body = resp.json() + assert body["success"] is False + msg = (body.get("message") or "").lower() + if resp.status_code == 500: + assert "kustomize" in msg, ( + f"500 should be the kustomize_unavailable bug (B1); got: {msg!r}" + ) + else: + assert "not found" in msg, f"404 should be the overlay-not-found path; got: {msg!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. + + The probe-pod happy path is currently unreachable because the + orchestrator ServiceAccount can't list ``kube-system`` DaemonSets + (B2), so ``_detect_cni`` returns ``(None, False)`` and the route + short-circuits with ``network_policy_enforcement_not_detected``. + The tests below cover the K8s-label-validation logic (which runs + before the CNI gate) and the current short-circuit; a + ``xfail(strict=True)`` test guards the future happy-path shape. + """ + + 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_route_short_circuits_when_cni_not_detected( + self, + orchestrator_url: str, + lifecycle_secret: str, + ) -> None: + """Current (B2) behaviour: route reports ``network_policy_enforcement_not_detected``. + + With the production RBAC the orchestrator can't list ``kube- + system`` DaemonSets, so the CNI gate fires unconditionally. + This test locks in the current observable shape so a future + change to the gate (or a fix to B2) surfaces as a deliberate + test update. + + When B2 is fixed the route will run the probe instead and this + test should be replaced with the happy-path probe-output + assertions in + ``test_probe_runs_and_returns_expected_shape``. + """ + resp = _post( + orchestrator_url, + "/api/v1/deployment/validate-network-isolation", + secret=lifecycle_secret, + body={"pipeline_id": "test-2641", "role": "coder"}, + ) + assert resp.status_code == 200 + data = resp.json()["data"] + # B2 is the dominant failure mode in production today. If the + # probe ever runs, the response shape changes to {probe_id, + # namespace, result} — let that flip be a hard signal by + # asserting the error key explicitly. + assert data.get("error") == "network_policy_enforcement_not_detected", ( + "route stopped short-circuiting; B2 may be fixed — flip this " + "test to the happy-path assertions" + ) + + @pytest.mark.xfail( + strict=True, + reason=( + "Blocked on B2: orchestrator SA can't list kube-system " + "DaemonSets so _detect_cni returns (None, False) and the probe " + "never launches. Fix the RBAC and this should pass." + ), + ) + 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. + + Marked ``xfail(strict=True)`` until B2 lands — when the + orchestrator gains RBAC to list kube-system DaemonSets the + probe will actually launch and this assertion holds. + + Expected probe-output shape (per ``PROBE_COMMAND_TEMPLATE``): + + * ``gateway_reachable: True`` — ``allow-agent-to-gateway`` + permits agent→gateway:9848. + * ``internet_blocked: True`` — ``default-deny-egress`` blocks + arbitrary egress (curl example.com). + * ``agent_pods_unreachable: True`` — no policy allows agent→ + random-peer:80. + + ``orchestrator_direct_blocked`` is deliberately NOT asserted: + ``allow-agent-to-orchestrator`` permits agent→orchestrator:9849 + for heartbeats, so the field returns ``False`` even when + isolation is correctly enforced. The field's name is misleading + — see the bug discussion in the PR. + """ + 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 + 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 + + +# --------------------------------------------------------------------------- +# 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 response without 409 contention. + + Under B2 the route short-circuits before submitting a probe Job, + so this test mostly proves the route is genuinely stateless + across calls. When B2 is fixed and the probe actually launches, + the test additionally 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 used here, but a + regression that hard-codes 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 regardless of B1/B2.""" + + 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. + """ + secret_snippet = lifecycle_secret[:16] + # 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: + assert secret_snippet not in resp.text, ( + f"response body contained a prefix of the bearer secret — " + f"{resp.request.url}: {resp.text[:500]}" + ) + + def test_validation_routes_reject_invalid_json( + self, + orchestrator_url: str, + lifecycle_secret: str, + ) -> None: + """Both routes tolerate / reject malformed JSON without crashing. + + ``request.get_json(silent=True) or {}`` is the pattern in the + route handlers; an upstream regression that flipped to a + non-silent ``get_json()`` would 500 with a Flask traceback. + Catch that here. + """ + for path in ( + "/api/v1/deployment/validate-manifests", + "/api/v1/deployment/validate-network-isolation", + ): + resp = requests.post( + f"{orchestrator_url}{path}", + data="this is not json", + headers={ + **_auth_headers(lifecycle_secret), + "Content-Type": "application/json", + }, + timeout=30, + ) + # The handlers use ``request.get_json(silent=True) or {}`` so a + # malformed body becomes the same shape as an empty one — no + # 500 with a Flask traceback should ever reach the caller. + assert "traceback" not in resp.text.lower(), ( + f"{path}: malformed JSON produced a Flask traceback: {resp.text[:500]}" + ) + + +# --------------------------------------------------------------------------- +# 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. This test enforces + that no probe Job persists in ``egg-agents`` after a call — even + today's short-circuit (B2) path, which never creates one, must not + leave one behind from a previous run. When B2 is fixed and the + probe actually launches, the same assertion catches a cleanup + regression. + """ + + def test_no_orphan_probe_jobs_after_call( + self, + orchestrator_url: str, + lifecycle_secret: str, + ) -> None: + import subprocess + + _post( + orchestrator_url, + "/api/v1/deployment/validate-network-isolation", + secret=lifecycle_secret, + body={"pipeline_id": "cleanup-2641", "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", + "egg.probe=true", + "-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}" + ) From 55250d1ca154862a3966f9d4fb71797e8de1baef Mon Sep 17 00:00:00 2001 From: James Wiesebron Date: Tue, 12 May 2026 11:34:43 -0700 Subject: [PATCH 2/9] Fix #2641 follow-ups: RBAC + kustomize + probe-field rename Bundles fixes for the four follow-up issues filed against #2658 so the integration suite can assert happy paths instead of locked-in regressions. - #2646: add ClusterRole egg-cluster-topology-reader granting the orchestrator SA get,list on apps/daemonsets and cluster-scoped nodes. _detect_cni / _detect_k3s now resolve against a real cluster. - #2647: install pinned kustomize v5.6.0 in the orchestrator image so _run_kustomize no longer raises kustomize_unavailable. The secondary repo-not-mounted gap remains acknowledged out of scope. - #2648: add list verb to apps/deployments in the egg-service-log-reader Role so _collect_egg_image_tags returns populated tags instead of {}. - #2652: rename probe field orchestrator_direct_blocked -> orchestrator_api_reachable with flipped polarity. The agent->orchestrator heartbeat path is deliberately permitted so the field now reads positively. Updates callers in mcp_tools, unit tests, docs, and the agent-diagnose skill. Removes the xfail and short-circuit assertions in the integration suite and replaces them with positive happy-path assertions. --- docs/reference/mcp-deployment-tools.md | 12 +- .../test_deployment_validation_logic.py | 213 +++++++----------- k8s/base/rbac.yaml | 41 +++- orchestrator/Dockerfile | 13 ++ orchestrator/mcp_tools.py | 2 +- orchestrator/routes/deployment.py | 8 +- orchestrator/tests/test_deployment_routes.py | 4 +- orchestrator/tests/test_mcp_tools.py | 2 +- skills/agent-diagnose/SKILL.md | 4 +- 9 files changed, 152 insertions(+), 147 deletions(-) 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/test_deployment_validation_logic.py b/integration_tests/test_deployment_validation_logic.py index a1dbd1c827..9036ae8ee0 100644 --- a/integration_tests/test_deployment_validation_logic.py +++ b/integration_tests/test_deployment_validation_logic.py @@ -18,33 +18,32 @@ ``TestValidateConfig``; reproducing it in the k3s tier would add cost without adding signal.) -## Bugs surfaced while building the suite (filed as follow-ups) +## Follow-up fixes shipped in the same PR The default-overlay / probe happy paths in the deployed orchestrator -are currently broken in three independent ways. The tests below lock -in the *observable* behaviour today (so any silent fix would flip the -assertion and force a deliberate test update); the happy-path variants -are marked ``xfail(strict=True)`` and point at the relevant bug. - -* **#2647 — orchestrator container has no ``kustomize``/``kubectl`` on - PATH.** ``orchestrator/Dockerfile`` installs ``git curl gosu`` only, - so ``_run_kustomize`` falls through both subprocess invocations and - raises ``kustomize_unavailable``. Any default-overlay validation - returns HTTP 500. -* **#2646 — orchestrator ServiceAccount cannot list DaemonSets in - ``kube-system`` or nodes cluster-wide.** ``_detect_cni`` and - ``_detect_k3s`` both rely on these reads, so ``validate-network- - isolation`` always short-circuits with - ``network_policy_enforcement_not_detected`` and ``validate- - manifests`` always reports ``is_k3s=False`` even on a real k3s - cluster (the k3s-gated image-tag rule never fires). -* **#2648 — orchestrator ServiceAccount can only ``get`` (not - ``list``) Deployments in ``egg-system``.** Tangential to #2641 but - observed in the same audit: ``_collect_egg_image_tags`` always - returns ``{}`` and ``get_deployment_context`` always sets - ``images_unavailable: true`` in production. - -The tests below do not depend on any of these bugs being fixed. +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 @@ -91,11 +90,12 @@ def _post( class TestValidateDeploymentManifestsLogic: """``POST /api/v1/deployment/validate-manifests`` — post-auth behaviour. - The orchestrator container ships without ``kustomize``/``kubectl`` and - without the egg repo bind-mounted, so the happy-path (rendered overlay - + warnings list) cannot run in CI today (see B1 in the module - docstring). The tests here cover what is reachable: the 404 / 400 - error paths and the deterministic 500 the missing tooling produces. + 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( @@ -165,25 +165,25 @@ def test_relative_traversal_outside_repo_root_returns_400( f"expected 400 for relative traversal, got {resp.status_code}: {resp.text[:500]}" ) - def test_default_overlay_in_deployed_orchestrator_returns_500_today( + def test_default_overlay_returns_404_or_200_depending_on_repo_mount( self, orchestrator_url: str, lifecycle_secret: str, ) -> None: - """Default overlay against a real orchestrator pod returns 500 today (B1). - - With the egg repo bind-mounted at ``/home/egg/repos`` (local - overlay), the route finds ``k8s/overlays/local`` but the - orchestrator container has neither ``kustomize`` nor ``kubectl`` - installed, so ``_run_kustomize`` raises ``kustomize_unavailable`` - and the route returns 500. Locking in the current observable - behaviour so the contract is explicit; when B1 is fixed this - assertion will need to flip to 200. - - The fixture path that mounts the repo only exists under the - local overlay; in CI the repo isn't mounted at all and the - route returns 404 instead. Accept either to keep the test - portable across both deployment shapes. + """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. """ resp = _post( orchestrator_url, @@ -191,24 +191,20 @@ def test_default_overlay_in_deployed_orchestrator_returns_500_today( secret=lifecycle_secret, body={}, ) - # Two valid shapes today: - # - 500 kustomize_unavailable: repo IS mounted (local overlay - # pattern), overlay found, kustomize missing. - # - 404 overlay not found: repo is NOT mounted (CI default - # overlay isn't reachable from the orchestrator pod). - # Both expose a real gap; 200 would be the post-fix state. - assert resp.status_code in (404, 500), ( - f"expected 404 or 500 in current deployment, got {resp.status_code}: {resp.text[:500]}" + assert resp.status_code in (200, 404), ( + f"expected 200 or 404, got {resp.status_code}: {resp.text[:500]}" ) body = resp.json() - assert body["success"] is False - msg = (body.get("message") or "").lower() - if resp.status_code == 500: - assert "kustomize" in msg, ( - f"500 should be the kustomize_unavailable bug (B1); got: {msg!r}" - ) + 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 "not found" in msg, f"404 should be the overlay-not-found path; got: {msg!r}" + 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, @@ -251,13 +247,12 @@ def test_re_validation_is_idempotent_on_error_path( class TestValidateNetworkIsolationLogic: """``POST /api/v1/deployment/validate-network-isolation`` — post-auth. - The probe-pod happy path is currently unreachable because the - orchestrator ServiceAccount can't list ``kube-system`` DaemonSets - (B2), so ``_detect_cni`` returns ``(None, False)`` and the route - short-circuits with ``network_policy_enforcement_not_detected``. - The tests below cover the K8s-label-validation logic (which runs - before the CNI gate) and the current short-circuit; a - ``xfail(strict=True)`` test guards the future happy-path shape. + 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( @@ -397,49 +392,6 @@ def test_default_pipeline_id_and_role_pass_label_validation( f"expected 200 with default labels, got {resp.status_code}: {resp.text[:500]}" ) - def test_route_short_circuits_when_cni_not_detected( - self, - orchestrator_url: str, - lifecycle_secret: str, - ) -> None: - """Current (B2) behaviour: route reports ``network_policy_enforcement_not_detected``. - - With the production RBAC the orchestrator can't list ``kube- - system`` DaemonSets, so the CNI gate fires unconditionally. - This test locks in the current observable shape so a future - change to the gate (or a fix to B2) surfaces as a deliberate - test update. - - When B2 is fixed the route will run the probe instead and this - test should be replaced with the happy-path probe-output - assertions in - ``test_probe_runs_and_returns_expected_shape``. - """ - resp = _post( - orchestrator_url, - "/api/v1/deployment/validate-network-isolation", - secret=lifecycle_secret, - body={"pipeline_id": "test-2641", "role": "coder"}, - ) - assert resp.status_code == 200 - data = resp.json()["data"] - # B2 is the dominant failure mode in production today. If the - # probe ever runs, the response shape changes to {probe_id, - # namespace, result} — let that flip be a hard signal by - # asserting the error key explicitly. - assert data.get("error") == "network_policy_enforcement_not_detected", ( - "route stopped short-circuiting; B2 may be fixed — flip this " - "test to the happy-path assertions" - ) - - @pytest.mark.xfail( - strict=True, - reason=( - "Blocked on B2: orchestrator SA can't list kube-system " - "DaemonSets so _detect_cni returns (None, False) and the probe " - "never launches. Fix the RBAC and this should pass." - ), - ) def test_probe_runs_and_returns_expected_shape( self, orchestrator_url: str, @@ -447,10 +399,6 @@ def test_probe_runs_and_returns_expected_shape( ) -> None: """Happy path: with enforcement detected the probe runs and reports. - Marked ``xfail(strict=True)`` until B2 lands — when the - orchestrator gains RBAC to list kube-system DaemonSets the - probe will actually launch and this assertion holds. - Expected probe-output shape (per ``PROBE_COMMAND_TEMPLATE``): * ``gateway_reachable: True`` — ``allow-agent-to-gateway`` @@ -459,12 +407,10 @@ def test_probe_runs_and_returns_expected_shape( arbitrary egress (curl example.com). * ``agent_pods_unreachable: True`` — no policy allows agent→ random-peer:80. - - ``orchestrator_direct_blocked`` is deliberately NOT asserted: - ``allow-agent-to-orchestrator`` permits agent→orchestrator:9849 - for heartbeats, so the field returns ``False`` even when - isolation is correctly enforced. The field's name is misleading - — see the bug discussion in the PR. + * ``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, @@ -476,11 +422,12 @@ def test_probe_runs_and_returns_expected_shape( assert resp.status_code == 200 data = resp.json()["data"] # Probe-launched shape, not the short-circuit shape. - assert "probe_id" in data + 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 # --------------------------------------------------------------------------- @@ -531,15 +478,13 @@ def test_concurrent_validate_network_isolation_calls_get_distinct_probe_ids( orchestrator_url: str, lifecycle_secret: str, ) -> None: - """Concurrent calls each get a distinct response without 409 contention. + """Concurrent calls each get a distinct probe_id without 409 contention. - Under B2 the route short-circuits before submitting a probe Job, - so this test mostly proves the route is genuinely stateless - across calls. When B2 is fixed and the probe actually launches, - the test additionally guards against a probe-id collision + 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 used here, but a - regression that hard-codes the id would surface here. + 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: @@ -572,7 +517,7 @@ def _call(i: int) -> requests.Response: class TestValidationRouteSelfConsistency: - """Cross-cutting invariants the routes must hold regardless of B1/B2.""" + """Cross-cutting invariants the routes must hold.""" def test_validation_routes_never_leak_secrets_in_error_messages( self, @@ -658,12 +603,10 @@ 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. This test enforces - that no probe Job persists in ``egg-agents`` after a call — even - today's short-circuit (B2) path, which never creates one, must not - leave one behind from a previous run. When B2 is fixed and the - probe actually launches, the same assertion catches a cleanup - regression. + 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( diff --git a/k8s/base/rbac.yaml b/k8s/base/rbac.yaml index 0b460e67e4..cf7cea2b39 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,39 @@ 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`: +# `_detect_k3s` lists nodes cluster-wide, `_detect_cni` / `_detect_k3s` +# list kube-system DaemonSets. Without this binding 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"] + - apiGroups: ["apps"] + resources: ["daemonsets"] + 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 diff --git a/orchestrator/Dockerfile b/orchestrator/Dockerfile index c426ce4acd..8fa9f6c13b 100644 --- a/orchestrator/Dockerfile +++ b/orchestrator/Dockerfile @@ -5,6 +5,19 @@ 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. +ARG KUSTOMIZE_VERSION=5.6.0 +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 \ + && 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 1b63351193..419ed4f6be 100644 --- a/orchestrator/mcp_tools.py +++ b/orchestrator/mcp_tools.py @@ -1078,7 +1078,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..52ef6324ff 100644 --- a/orchestrator/routes/deployment.py +++ b/orchestrator/routes/deployment.py @@ -944,7 +944,13 @@ 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). + "orchestrator_api_reachable": "$orch".startswith("2") or "$orch".startswith("3"), "raw": { "gateway_status": "$gw", "internet_status": "$internet", diff --git a/orchestrator/tests/test_deployment_routes.py b/orchestrator/tests/test_deployment_routes.py index fb8ec799e9..72569452c8 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 diff --git a/orchestrator/tests/test_mcp_tools.py b/orchestrator/tests/test_mcp_tools.py index e89d8aeead..d3f59e6293 100644 --- a/orchestrator/tests/test_mcp_tools.py +++ b/orchestrator/tests/test_mcp_tools.py @@ -2758,7 +2758,7 @@ def test_returns_probe_result(self, handler): "gateway_reachable": True, "internet_blocked": True, "agent_pods_unreachable": True, - "orchestrator_direct_blocked": True, + "orchestrator_api_reachable": True, }, } with patch.object( diff --git a/skills/agent-diagnose/SKILL.md b/skills/agent-diagnose/SKILL.md index cc90eefcc9..78c2c4f813 100644 --- a/skills/agent-diagnose/SKILL.md +++ b/skills/agent-diagnose/SKILL.md @@ -163,7 +163,7 @@ Record the returned map: - `gateway_reachable` (expect `true`) - `internet_blocked` (expect `true`) - `agent_pods_unreachable` (expect `true`) -- `orchestrator_direct_blocked` (expect `true`) +- `orchestrator_api_reachable` (expect `true`) Any deviation is a NetworkPolicy drift — flag it high-severity in the Top finding. The probe runs in a throwaway Job with @@ -226,7 +226,7 @@ identifier-translation or role/auth boundary cluster.> - 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 From 0bb9c45ea9552a09f2b61d4f80b2206cb5bc6323 Mon Sep 17 00:00:00 2001 From: jwbron <8340608+jwbron@users.noreply.github.com> Date: Tue, 12 May 2026 18:44:02 +0000 Subject: [PATCH 3/9] Fix hardcoded-ports lint: suppress EGG002 in docstring comment --- integration_tests/test_deployment_validation_logic.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integration_tests/test_deployment_validation_logic.py b/integration_tests/test_deployment_validation_logic.py index 9036ae8ee0..ea85f003f0 100644 --- a/integration_tests/test_deployment_validation_logic.py +++ b/integration_tests/test_deployment_validation_logic.py @@ -402,7 +402,7 @@ def test_probe_runs_and_returns_expected_shape( Expected probe-output shape (per ``PROBE_COMMAND_TEMPLATE``): * ``gateway_reachable: True`` — ``allow-agent-to-gateway`` - permits agent→gateway:9848. + 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→ From a83cb1f13e4866972a0d33ab8590ebb93df5cb3b Mon Sep 17 00:00:00 2001 From: "james-in-a-box[bot]" <246424927+james-in-a-box[bot]@users.noreply.github.com> Date: Tue, 12 May 2026 19:04:45 +0000 Subject: [PATCH 4/9] Fix probe double-000: remove redundant || echo 000 in PROBE_COMMAND_TEMPLATE curl -w '%{http_code}' already outputs '000' when no HTTP response is received (connection refused / timeout / egress blocked). The previous || echo 000 fallback ran on curl's non-zero exit, concatenating a second '000' so internet_status became '000000'. The comparison "" == "000" then evaluated False, reporting internet_blocked: False even on a correctly-isolated cluster. Drop the fallback entirely and redirect stderr so the function is silent; || true keeps the function's exit code at 0 regardless. --- orchestrator/routes/deployment.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/orchestrator/routes/deployment.py b/orchestrator/routes/deployment.py index 52ef6324ff..24310ab39a 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") From 77c33171e1561c437dbc830776487df1cd3f74fb Mon Sep 17 00:00:00 2001 From: "egg-reviewer[bot]" <261018737+egg-reviewer[bot]@users.noreply.github.com> Date: Tue, 12 May 2026 20:21:08 +0000 Subject: [PATCH 5/9] Address review feedback on PR #2658 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes raised by egg-reviewer across the three review rounds on this PR: * PROBE_COMMAND_TEMPLATE backticks (blocking-grade): the unquoted ``< None: - """Both routes tolerate / reject malformed JSON without crashing. + """Malformed JSON yields the same response shape as an empty body. ``request.get_json(silent=True) or {}`` is the pattern in the - route handlers; an upstream regression that flipped to a - non-silent ``get_json()`` would 500 with a Flask traceback. - Catch that here. + 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", ): - resp = requests.post( + 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=30, + timeout=90, ) - # The handlers use ``request.get_json(silent=True) or {}`` so a - # malformed body becomes the same shape as an empty one — no - # 500 with a Flask traceback should ever reach the caller. - assert "traceback" not in resp.text.lower(), ( - f"{path}: malformed JSON produced a Flask traceback: {resp.text[:500]}" + # 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" ) @@ -616,11 +690,18 @@ def test_no_orphan_probe_jobs_after_call( ) -> 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": "cleanup-2641", "role": "coder"}, + body={"pipeline_id": pipeline_id, "role": "coder"}, timeout=90, ) @@ -638,7 +719,7 @@ def test_no_orphan_probe_jobs_after_call( "get", "jobs", "-l", - "egg.probe=true", + f"egg.probe=true,egg.pipeline.id={pipeline_id}", "-o", "jsonpath={.items[*].metadata.name}", ], diff --git a/k8s/base/rbac.yaml b/k8s/base/rbac.yaml index cf7cea2b39..ab69cd69e9 100644 --- a/k8s/base/rbac.yaml +++ b/k8s/base/rbac.yaml @@ -94,10 +94,13 @@ subjects: namespace: egg-system --- # Cluster-scope reads for cluster topology probes used by -# `get_deployment_context` and `validate_network_isolation`: -# `_detect_k3s` lists nodes cluster-wide, `_detect_cni` / `_detect_k3s` -# list kube-system DaemonSets. Without this binding the heuristics -# always returned null/false, masking real k3s + CNI state (#2646). +# `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: @@ -109,9 +112,6 @@ rules: - apiGroups: [""] resources: ["nodes"] verbs: ["get", "list"] - - apiGroups: ["apps"] - resources: ["daemonsets"] - verbs: ["get", "list"] --- apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRoleBinding @@ -128,3 +128,36 @@ 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/routes/deployment.py b/orchestrator/routes/deployment.py index 24310ab39a..3a34d2aa07 100644 --- a/orchestrator/routes/deployment.py +++ b/orchestrator/routes/deployment.py @@ -944,12 +944,16 @@ 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", - # `allow-agent-to-orchestrator` (k8s/base/network-policies.yaml) + # 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 + # field was previously named orchestrator_direct_blocked with # inverted polarity, which read backwards from intent (#2652). + # NOTE: this heredoc is unquoted (< Date: Tue, 12 May 2026 14:07:52 -0700 Subject: [PATCH 6/9] Fix #2681: verify sha256 on kustomize tarball install (#2686) Defense-in-depth follow-up to PR #2658. Pins the published linux_amd64 checksum (`KUSTOMIZE_SHA256`) and runs `sha256sum -c` before extracting so a release-mirror compromise or in-flight tampering on the build node fails the build instead of silently shipping a swapped binary. Bumping `KUSTOMIZE_VERSION` now requires updating `KUSTOMIZE_SHA256` in lockstep (called out in the comment above the ARGs). Stacked on egg/2641-deployment-validation-integration-tests (PR #2658) since the install line itself isn't on main yet. --- orchestrator/Dockerfile | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/orchestrator/Dockerfile b/orchestrator/Dockerfile index 2bfb3840ae..56aeb8e90c 100644 --- a/orchestrator/Dockerfile +++ b/orchestrator/Dockerfile @@ -10,10 +10,15 @@ RUN apt-get update && apt-get install -y \ # 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. +# 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 From 497866683c3f93b24511b6b790268e09b96bc705 Mon Sep 17 00:00:00 2001 From: jwbron <8340608+jwbron@users.noreply.github.com> Date: Tue, 12 May 2026 21:38:14 +0000 Subject: [PATCH 7/9] Fix #2658: bypass kubernetes client auto-deserialization for probe log MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The kubernetes-python client's ApiClient.deserialize() unconditionally runs json.loads() on every response body before coercing to the declared response_type. For pod logs declared as `str`, when the log content happens to be JSON-parseable, the deserializer turns it into a dict and then str()'s the dict — yielding Python repr (single quotes, ``True``) instead of the original JSON. The probe's ``print(json.dumps(...))`` output thus reaches ``_parse_probe_output`` as Python dict repr, which fails JSON parsing, and the route returns ``probe_output_unparseable``. Pass ``_preload_content=False`` to bypass the deserialize path and read the raw bytes off the urllib3 HTTPResponse directly. --- orchestrator/routes/deployment.py | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/orchestrator/routes/deployment.py b/orchestrator/routes/deployment.py index 3a34d2aa07..fb45182fa1 100644 --- a/orchestrator/routes/deployment.py +++ b/orchestrator/routes/deployment.py @@ -1120,12 +1120,26 @@ def _wait_for_probe_pod(k8s: Any, namespace: str, probe_id: str, *, timeout: flo def _read_probe_log(k8s: Any, namespace: str, pod_name: str) -> 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. 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 + ) 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 raw is None: + return "" + data = getattr(raw, "data", raw) + 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: From aabc90c1dd1e34a03c6281531717528c3378d0f5 Mon Sep 17 00:00:00 2001 From: "egg-reviewer[bot]" <261018737+egg-reviewer[bot]@users.noreply.github.com> Date: Tue, 12 May 2026 22:01:20 +0000 Subject: [PATCH 8/9] Fix #2658: widen _read_probe_log try/except and cover with unit tests Two non-blocking concerns from the latest review on commit 4978666: 1. _preload_content=False defers the actual network read to .data access. The previous try/except only covered the read_namespaced_pod_log call itself, so a mid-stream connection reset or malformed transfer-encoding at .data access would propagate up and 500 the route handler. Extend the try block to cover the .data access too. 2. Add a new TestReadProbeLog unit test class covering: - bytes path (the happy path on the new code) - invalid-utf-8 bytes (errors="replace" works as intended) - str fallback when the response has no .data attribute - None response returns "" rather than crashing - Exception from read_namespaced_pod_log returns "" - Exception from .data access returns "" (the regression-window this commit closes) The integration suite already exercises the end-to-end happy path against a real cluster; these unit tests pin the bytes/str branching and the body-read exception envelope so a future "simplification" of getattr(raw, "data", raw) -> raw.data or the removal of the .data exception coverage surfaces in fast CI rather than only on the integration tier. --- orchestrator/routes/deployment.py | 12 ++- orchestrator/tests/test_deployment_routes.py | 97 ++++++++++++++++++++ 2 files changed, 106 insertions(+), 3 deletions(-) diff --git a/orchestrator/routes/deployment.py b/orchestrator/routes/deployment.py index fb45182fa1..caca3c5400 100644 --- a/orchestrator/routes/deployment.py +++ b/orchestrator/routes/deployment.py @@ -1127,16 +1127,22 @@ def _read_probe_log(k8s: Any, namespace: str, pod_name: str) -> str: # 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, _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 "" - if raw is None: - return "" - data = getattr(raw, "data", raw) if isinstance(data, bytes): return data.decode("utf-8", errors="replace") return str(data) diff --git a/orchestrator/tests/test_deployment_routes.py b/orchestrator/tests/test_deployment_routes.py index 3d083a219b..36582c7290 100644 --- a/orchestrator/tests/test_deployment_routes.py +++ b/orchestrator/tests/test_deployment_routes.py @@ -1526,6 +1526,103 @@ def test_template_is_shell_syntax_valid(self): ) +class TestReadProbeLog: + """Unit coverage of ``_read_probe_log``. + + The function passes ``_preload_content=False`` to bypass the + kubernetes-python ``ApiClient.deserialize()`` JSON-coercion path, + so the returned object is a raw ``urllib3.HTTPResponse`` whose + ``.data`` attribute contains bytes. The integration suite covers + the end-to-end happy path against a real cluster, but the + bytes/str branching and the body-read exception envelope need + direct unit coverage. + """ + + def test_decodes_bytes_payload(self): + """The ``.data`` bytes path round-trips through utf-8 decode.""" + from routes.deployment import _read_probe_log + + raw = MagicMock() + raw.data = b'{"gateway_reachable": true}' + k8s = MagicMock() + k8s.core_api.read_namespaced_pod_log.return_value = raw + + result = _read_probe_log(k8s, "egg-agents", "egg-probe-abc") + + assert result == '{"gateway_reachable": true}' + k8s.core_api.read_namespaced_pod_log.assert_called_once_with( + name="egg-probe-abc", namespace="egg-agents", _preload_content=False + ) + + def test_replaces_undecodable_bytes(self): + """Invalid utf-8 sequences are replaced rather than raising.""" + from routes.deployment import _read_probe_log + + raw = MagicMock() + raw.data = b"hello \xff\xfe world" + k8s = MagicMock() + k8s.core_api.read_namespaced_pod_log.return_value = raw + + result = _read_probe_log(k8s, "egg-agents", "egg-probe-abc") + + assert "hello" in result + assert "world" in result + + def test_str_fallback_for_object_without_data(self): + """If the response has no ``.data`` attribute, ``str()`` it.""" + from routes.deployment import _read_probe_log + + k8s = MagicMock() + k8s.core_api.read_namespaced_pod_log.return_value = "plain string log" + + result = _read_probe_log(k8s, "egg-agents", "egg-probe-abc") + + assert result == "plain string log" + + def test_returns_empty_string_on_none(self): + """A ``None`` response yields an empty string, not a crash.""" + from routes.deployment import _read_probe_log + + k8s = MagicMock() + k8s.core_api.read_namespaced_pod_log.return_value = None + + result = _read_probe_log(k8s, "egg-agents", "egg-probe-abc") + + assert result == "" + + def test_swallows_exception_from_request(self): + """Errors during the kubernetes-python call return ``''``.""" + from routes.deployment import _read_probe_log + + k8s = MagicMock() + k8s.core_api.read_namespaced_pod_log.side_effect = RuntimeError("boom") + + result = _read_probe_log(k8s, "egg-agents", "egg-probe-abc") + + assert result == "" + + def test_swallows_exception_from_data_access(self): + """Body-read failures via ``.data`` are caught, not propagated. + + ``_preload_content=False`` defers the actual network read to + ``.data`` access. A mid-stream connection reset or malformed + transfer-encoding raises there; the route handler relies on + ``_read_probe_log`` returning ``''`` rather than 500'ing. + """ + from routes.deployment import _read_probe_log + + raw = MagicMock() + type(raw).data = property( + lambda self: (_ for _ in ()).throw(ConnectionResetError("stream reset")) + ) + k8s = MagicMock() + k8s.core_api.read_namespaced_pod_log.return_value = raw + + result = _read_probe_log(k8s, "egg-agents", "egg-probe-abc") + + assert result == "" + + # --------------------------------------------------------------------------- # module sanity # --------------------------------------------------------------------------- From 899323ff8c57a76e3de80bb97e7bc262760f9dbf Mon Sep 17 00:00:00 2001 From: "egg-reviewer[bot]" <261018737+egg-reviewer[bot]@users.noreply.github.com> Date: Tue, 12 May 2026 22:43:47 +0000 Subject: [PATCH 9/9] Address review nits on _read_probe_log: guard data is None + clarify defensive test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add explicit if data is None: return '' after the getattr() in _read_probe_log so a hypothetical response with .data = None yields '' rather than the literal string 'None' (which would flow into _parse_probe_output as probe log content). - Add TestReadProbeLog::test_returns_empty_string_on_none_data unit test pinning the new guard. - Clarify test_str_fallback_for_object_without_data's docstring to state it is a defensive-only branch — kubernetes-python with _preload_content=False always returns urllib3.HTTPResponse, which always has .data; the str() fallback exists only so a future client upgrade or mock returning a plain string degrades cleanly. Both items raised non-blocking on the 1e5daf1b re-review. --- orchestrator/routes/deployment.py | 2 ++ orchestrator/tests/test_deployment_routes.py | 30 +++++++++++++++++++- 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/orchestrator/routes/deployment.py b/orchestrator/routes/deployment.py index caca3c5400..187ec0a0af 100644 --- a/orchestrator/routes/deployment.py +++ b/orchestrator/routes/deployment.py @@ -1143,6 +1143,8 @@ def _read_probe_log(k8s: Any, namespace: str, pod_name: str) -> str: except Exception as exc: logger.warning("probe log read failed", pod=pod_name, error=str(exc)) return "" + if data is None: + return "" if isinstance(data, bytes): return data.decode("utf-8", errors="replace") return str(data) diff --git a/orchestrator/tests/test_deployment_routes.py b/orchestrator/tests/test_deployment_routes.py index 36582c7290..bad1a00eb1 100644 --- a/orchestrator/tests/test_deployment_routes.py +++ b/orchestrator/tests/test_deployment_routes.py @@ -1569,7 +1569,15 @@ def test_replaces_undecodable_bytes(self): assert "world" in result def test_str_fallback_for_object_without_data(self): - """If the response has no ``.data`` attribute, ``str()`` it.""" + """If the response has no ``.data`` attribute, ``str()`` it. + + Defensive-only branch — not reachable in production with the + current kubernetes-python client, which always returns a + ``urllib3.HTTPResponse`` (always has ``.data``) under + ``_preload_content=False``. The fallback exists so a future + client upgrade or mock that returns a plain string degrades + cleanly rather than crashing. + """ from routes.deployment import _read_probe_log k8s = MagicMock() @@ -1590,6 +1598,26 @@ def test_returns_empty_string_on_none(self): assert result == "" + def test_returns_empty_string_on_none_data(self): + """A response whose ``.data`` is ``None`` yields ``''``, not ``'None'``. + + Closes a gap where ``getattr(raw, "data", raw)`` returns ``None`` + for ``raw.data is None``, then ``isinstance(None, bytes)`` is + ``False`` and ``str(None)`` yields the literal ``'None'`` — + which would then flow into ``_parse_probe_output`` as probe log + content. + """ + from routes.deployment import _read_probe_log + + raw = MagicMock() + raw.data = None + k8s = MagicMock() + k8s.core_api.read_namespaced_pod_log.return_value = raw + + result = _read_probe_log(k8s, "egg-agents", "egg-probe-abc") + + assert result == "" + def test_swallows_exception_from_request(self): """Errors during the kubernetes-python call return ``''``.""" from routes.deployment import _read_probe_log