From 29eefd6ae580d514fcb62630556021052d203c7f Mon Sep 17 00:00:00 2001 From: "SREDevOps.org Agent" Date: Wed, 21 Oct 2026 13:27:19 -0300 Subject: [PATCH] Fix idle sweeper suspending claims the gateway does not own (#4) The sweeper listed every SandboxClaim in NAMESPACE and treated each dsh- name as one of its own users. A claim applied by hand from the resource-layer manifests (e.g. 30-claim-alice.yaml) therefore looked exactly like a gateway user: the idle clock started on first sighting and the pod was suspended after IDLE_TIMEOUT. That deletes the pod the README's step 3 logs/port-forward targets, so the walkthrough broke as soon as the gateway was deployed next to it. Mark claims created by create_claim() with aaas.example.com/managed-by=dsh-gateway and skip everything else in sweep_once() before any idle bookkeeping happens. Document the ownership boundary in the example docs, the manifest and ADR-5, add unit coverage for the marker and the skipped sweep, and assert in run-test-kind.sh that alice's hand-applied claim stays Running while bob is correctly idle-suspended. Co-authored-by: SREDevOpsAgents --- docs/architecture.md | 17 +++++-- .../dsh-agents-as-a-service/50-gateway.yaml | 5 +- examples/dsh-agents-as-a-service/README.md | 7 +++ .../dsh-agents-as-a-service/gateway/README.md | 2 +- .../gateway/gateway.py | 28 ++++++++++- .../gateway/test_gateway.py | 46 ++++++++++++++++++- .../dsh-agents-as-a-service/run-test-kind.sh | 9 ++++ 7 files changed, 107 insertions(+), 7 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index 8049b3c..b949c62 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -133,6 +133,13 @@ sequenceDiagram Suspend deletes only the pod. The PVC keeps conversations, sessions, and config, so resume reattaches exactly the same state. +**Ownership boundary.** The gateway shares its namespace with claims created outside it — the +example's resource-layer walkthrough applies `dsh-alice` by hand, and every claim is named +`dsh-`. Claims the gateway creates carry `aaas.example.com/managed-by=dsh-gateway`; the +sweeper skips everything else, so it can never suspend a sandbox an operator is using. Without +that marker the sweeper would suspend a hand-applied claim 60s after first sighting and break +`kubectl logs`/`port-forward` against its pod. + ### WebSocket ```mermaid @@ -210,11 +217,15 @@ sequenceDiagram ### ADR-5: Claims as the user record - **Decision:** a user is a `SandboxClaim` named `dsh-`; the bearer-token SHA-256 lives in - a claim annotation. + a claim annotation, and the gateway marks its own claims with + `aaas.example.com/managed-by=dsh-gateway`. - **Why:** no database, no separate account service; deleting the claim is account deletion and - cascades everything. + cascades everything. The ownership annotation keeps a namespaced claim list safe to share with + objects created by hand or by other controllers. - **Cost:** no password reset, no admin API, no audit log of its own; Kubernetes API load grows - with users. Fine for a reference pattern, not for a product. + with users. Claims created before the ownership marker existed must be annotated + (`kubectl annotate sandboxclaim ...`) to keep being swept. Fine for a reference pattern, not + for a product. ### ADR-6: Gateway cookie session instead of bearer tokens in the browser diff --git a/examples/dsh-agents-as-a-service/50-gateway.yaml b/examples/dsh-agents-as-a-service/50-gateway.yaml index ffcf10f..7895918 100644 --- a/examples/dsh-agents-as-a-service/50-gateway.yaml +++ b/examples/dsh-agents-as-a-service/50-gateway.yaml @@ -101,7 +101,10 @@ spec: value: dsh-pool # Seconds a sandbox may sit idle before the sweeper suspends it # (pod deleted, PVC kept), and the max time a proxy request waits - # for a suspended sandbox to become Ready again. + # for a suspended sandbox to become Ready again. Only claims this + # gateway created are swept: a claim applied by hand from the + # resource-layer manifests carries no aaas.example.com/managed-by + # annotation and is never suspended. - name: IDLE_TIMEOUT value: "60" - name: WAKE_TIMEOUT diff --git a/examples/dsh-agents-as-a-service/README.md b/examples/dsh-agents-as-a-service/README.md index a7f47c0..d823d61 100644 --- a/examples/dsh-agents-as-a-service/README.md +++ b/examples/dsh-agents-as-a-service/README.md @@ -130,6 +130,12 @@ kubectl -n dsh-demo port-forward "pod/$SB" 18081:3080 > never `3080`. `localhost` is always in DSH's trusted-host list, so the forwarded request is > accepted. +> **Why the pod stays up:** this claim was applied by hand, so it carries no gateway ownership +> annotation (`aaas.example.com/managed-by`). The gateway's idle sweeper only suspends claims it +> created through `POST /users`; the resource-layer claims from these manifests are never +> touched. You can deploy the gateway (step 8) and still keep a sandbox you are port-forwarding +> to. + ## 4. Write state on the PVC ```bash @@ -307,6 +313,7 @@ redirects `/u/` to the trailing-slash form with `308` so relative URLs res | WebSocket closes immediately (`4401`) | Missing/expired gateway cookie, or the upgrade's `Origin` does not match `Host` (CSWSH check); DSH rejecting `Host` instead surfaces as close `1011` — add the host to `DSH_TRUSTED_HOSTS` | | Signup is slow / `202` then `Waking` | Warm pool exhausted (`kubectl get sandboxes -l agents.x-k8s.io/warm-pool-sandbox`); scale the pool | | `403` from the Kubernetes API in gateway logs | The gateway ServiceAccount is missing claim/sandbox/pod-log verbs (see `50-gateway.yaml`) | +| Step 3: `pod "$SB" not found` after deploying the gateway | The claim was suspended by the idle sweeper. Claims created by the gateway carry `aaas.example.com/managed-by=dsh-gateway`; hand-applied claims (steps 1–7) do not and are never swept. Upgrade the gateway to a build that honors the marker, or resume the sandbox: `kubectl -n dsh-demo patch sandbox "$SB" --type merge -p '{"spec":{"operatingMode":"Running"}}'` | ## Limitations diff --git a/examples/dsh-agents-as-a-service/gateway/README.md b/examples/dsh-agents-as-a-service/gateway/README.md index a1fe4ab..295fc01 100644 --- a/examples/dsh-agents-as-a-service/gateway/README.md +++ b/examples/dsh-agents-as-a-service/gateway/README.md @@ -112,7 +112,7 @@ can also be injected into `create_app` for tests. | --- | --- | --- | | `NAMESPACE` | `dsh-demo` | Namespace holding the claims, sandboxes and pods. | | `POOL` | `dsh-pool` | `warmPoolRef.name` for new claims. | -| `IDLE_TIMEOUT` | `60` | Seconds of inactivity before the sweeper suspends a sandbox. | +| `IDLE_TIMEOUT` | `60` | Seconds of inactivity before the sweeper suspends a sandbox. Only claims the gateway created (annotation `aaas.example.com/managed-by=dsh-gateway`) are ever suspended. | | `WAKE_TIMEOUT` | `120` | Seconds to wait for a sandbox to become Ready (signup and wake-on-connect). | | `DSH_PORT` | `3080` | Upstream DSH port. | | `DSH_CONTAINER` | `dsh-web` | Container whose logs carry the launch token. | diff --git a/examples/dsh-agents-as-a-service/gateway/gateway.py b/examples/dsh-agents-as-a-service/gateway/gateway.py index ced9f86..81ffd45 100644 --- a/examples/dsh-agents-as-a-service/gateway/gateway.py +++ b/examples/dsh-agents-as-a-service/gateway/gateway.py @@ -69,6 +69,12 @@ from starlette.websockets import WebSocket GROUP, VERSION = "extensions.agents.x-k8s.io", "v1beta1" CORE_GROUP = "agents.x-k8s.io" TOKEN_ANNOTATION = "aaas.example.com/token-sha256" +# Ownership marker: the gateway shares its namespace with claims applied by +# hand (the resource-layer walkthrough in the example README). Every claim is +# named "dsh-", so without an explicit marker the sweeper cannot tell its +# own users from a claim someone created manually. +MANAGED_ANNOTATION = "aaas.example.com/managed-by" +MANAGED_BY = "dsh-gateway" POD_NAME_ANNOTATION = "agents.x-k8s.io/pod-name" USER_LABEL = "sandbox.users.io/dsh-user" @@ -149,6 +155,18 @@ def claim_user(claim: dict) -> str: return claim["metadata"]["name"].removeprefix("dsh-") +def gateway_managed(claim: dict) -> bool: + """True only for claims this gateway created. + + The sweeper must not suspend claims it does not own: a claim applied from + a resource-layer manifest (e.g. ``30-claim-alice.yaml``) has the same + ``dsh-`` name but no ownership marker, and suspending it deletes a + pod the operator is using. + """ + annotations = (claim.get("metadata") or {}).get("annotations") or {} + return annotations.get(MANAGED_ANNOTATION) == MANAGED_BY + + def condition(obj: dict, ctype: str) -> bool: for c in (obj.get("status") or {}).get("conditions", []): if c.get("type") == ctype: @@ -372,7 +390,10 @@ class Kubernetes: "kind": "SandboxClaim", "metadata": { "name": claim_name(user), - "annotations": {TOKEN_ANNOTATION: token_hash}, + "annotations": { + TOKEN_ANNOTATION: token_hash, + MANAGED_ANNOTATION: MANAGED_BY, + }, }, "spec": { "warmPoolRef": {"name": pool}, @@ -1169,6 +1190,11 @@ against the JSON API. The browser session is an HttpOnly cookie.

async def sweep_once(self) -> None: claims = await self.k8s.list_claims() for claim in claims: + # Only suspend claims this gateway created. Everything else in + # the namespace (resource-layer claims from the README walkthrough, + # other controllers' objects) is not ours to manage. + if not gateway_managed(claim): + continue user = claim_user(claim) try: sandbox = await self._sandbox_of(claim) diff --git a/examples/dsh-agents-as-a-service/gateway/test_gateway.py b/examples/dsh-agents-as-a-service/gateway/test_gateway.py index 52330a8..276c588 100644 --- a/examples/dsh-agents-as-a-service/gateway/test_gateway.py +++ b/examples/dsh-agents-as-a-service/gateway/test_gateway.py @@ -29,6 +29,7 @@ from gateway import ( derive_state, extract_dsh_cookies, filtered_query, + gateway_managed, parse_dsh_launch_token, rewrite_location, same_origin, @@ -318,6 +319,15 @@ def test_claim_name_and_user_roundtrip(): assert claim_user({"metadata": {"name": "dsh-alice"}}) == "alice" +def test_gateway_managed_requires_ownership_annotation(): + managed = {"metadata": {"annotations": {"aaas.example.com/managed-by": "dsh-gateway"}}} + manual = {"metadata": {"name": "dsh-alice"}} + other = {"metadata": {"annotations": {"aaas.example.com/managed-by": "someone"}}} + assert gateway_managed(managed) is True + assert gateway_managed(manual) is False + assert gateway_managed(other) is False + + def test_parse_dsh_launch_token_from_real_log_line(): log = ( "2026-10-21T10:00:00Z starting\n" @@ -443,6 +453,10 @@ async def test_kubernetes_create_claim_body(monkeypatch): body = args[4] assert body["metadata"]["name"] == "dsh-alice" assert body["metadata"]["annotations"]["aaas.example.com/token-sha256"] == "hash" + assert ( + body["metadata"]["annotations"]["aaas.example.com/managed-by"] + == "dsh-gateway" + ) assert body["spec"]["warmPoolRef"]["name"] == "test-pool" assert body["spec"]["additionalPodMetadata"]["labels"][ "sandbox.users.io/dsh-user" @@ -1159,7 +1173,10 @@ def test_ws_upstream_connect_failure_closes_1011(env): def ready_claim(user="alice"): return { - "metadata": {"name": claim_name(user)}, + "metadata": { + "name": claim_name(user), + "annotations": {"aaas.example.com/managed-by": "dsh-gateway"}, + }, "status": {"sandbox": {"name": f"dsh-{user}-sbx"}}, } @@ -1172,6 +1189,33 @@ def ready_sandbox(user="alice"): } +@pytest.mark.asyncio +async def test_sweeper_skips_claim_it_does_not_own(env): + """A hand-applied claim (e.g. README 30-claim-alice.yaml) must not be swept. + + The gateway shares the namespace with the resource-layer walkthrough; those + claims are named dsh- too, but carry no ownership annotation. The + sweeper used to suspend them 60s after first sighting, deleting the pod that + README step 3 port-forwards to. + """ + env.k8s.claims["alice"] = { + "metadata": {"name": claim_name("alice")}, + "status": {"sandbox": {"name": "dsh-alice-sbx"}}, + } + env.k8s.sandboxes["dsh-alice-sbx"] = ready_sandbox() + + # The sweeper never starts an idle clock for a claim it does not own... + await env.gateway.sweep_once() + assert "alice" not in env.gateway.state.last_activity + + # ...and does not suspend it even when it looks long-idle. + env.gateway.state.last_activity["alice"] = ( + time.time() - env.settings.idle_timeout - 10 + ) + await env.gateway.sweep_once() + assert env.k8s.patches == [] + + @pytest.mark.asyncio async def test_sweeper_suspends_idle_ready_sandbox(env): env.k8s.claims["alice"] = ready_claim() diff --git a/examples/dsh-agents-as-a-service/run-test-kind.sh b/examples/dsh-agents-as-a-service/run-test-kind.sh index 457e839..c81979f 100755 --- a/examples/dsh-agents-as-a-service/run-test-kind.sh +++ b/examples/dsh-agents-as-a-service/run-test-kind.sh @@ -228,6 +228,15 @@ PY [ "$STATE" = "Suspended" ] || fail "bob was not idle-suspended (state=$STATE)" pass "idle sweeper suspended bob" + # Regression: alice's claim was applied by hand in step 2, so it has no + # aaas.example.com/managed-by annotation. The sweeper must leave it alone - + # even though the same idle window just suspended bob. If it does not, README + # step 3 (logs/port-forward against alice's pod) breaks. + ALICE_MODE=$(kubectl -n "$NS" get sandbox "$SB" -o jsonpath='{.spec.operatingMode}') + [ "$ALICE_MODE" = "Running" ] \ + || fail "sweeper suspended a claim it does not own (alice: $ALICE_MODE)" + pass "idle sweeper left the hand-applied alice claim Running" + T0=$(date +%s) curl -sf -m 150 -b "$JAR" "http://$GW/u/bob/" >/dev/null \ || fail "wake-on-connect request failed"