[core][sandbox] Isolate network="public" sandboxes in per-sandbox netns via pasta - #65820
[core][sandbox] Isolate network="public" sandboxes in per-sandbox netns via pasta#65820xyuzh wants to merge 18 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces an experimental REST API service for Ray Sandbox, implemented as a FastAPI application on Ray Serve, allowing sandboxes to be managed externally. It also adds support for private network namespaces via pasta for the network="public" mode, enabling port isolation. Feedback focuses on improving robustness: failing fast during namespace holder startup, correctly parsing username:gid during user resolution, preventing a single broken sandbox from failing the list API, and adding timeouts to network downloads of runsc and pasta binaries.
| script = ( | ||
| # The holder pins the namespaces for the sandbox's lifetime; | ||
| # --kill-child ties it to this script's process group. | ||
| "unshare --user --map-root-user --net --fork --kill-child " | ||
| f"bash -c 'echo $$ > {pidfile}; exec sleep infinity' & " | ||
| f"for i in $(seq 1 100); do [ -s {pidfile} ] && break; sleep 0.1; done; " | ||
| f"NSPID=$(cat {pidfile}); " | ||
| # pasta runs from the pod side (its uplink is the pod's real | ||
| # interface), attaches to the holder's namespaces, and | ||
| # daemonizes; it exits when the namespaces empty. | ||
| f"{pasta} --netns /proc/$NSPID/ns/net --userns /proc/$NSPID/ns/user && " | ||
| f"exec nsenter --preserve-credentials -U -n -t $NSPID -- {runsc}" | ||
| ) |
There was a problem hiding this comment.
The loop waiting for the pidfile to be written runs up to 100 times (10 seconds) even if the background unshare process fails immediately (e.g., due to permission or seccomp errors). Additionally, if NSPID is empty, the script proceeds with empty paths like /proc//ns/net, leading to cryptic failures. We should capture the background PID ($!), check if it is still alive in the loop to fail fast, and validate that NSPID is not empty before proceeding.
| script = ( | |
| # The holder pins the namespaces for the sandbox's lifetime; | |
| # --kill-child ties it to this script's process group. | |
| "unshare --user --map-root-user --net --fork --kill-child " | |
| f"bash -c 'echo $$ > {pidfile}; exec sleep infinity' & " | |
| f"for i in $(seq 1 100); do [ -s {pidfile} ] && break; sleep 0.1; done; " | |
| f"NSPID=$(cat {pidfile}); " | |
| # pasta runs from the pod side (its uplink is the pod's real | |
| # interface), attaches to the holder's namespaces, and | |
| # daemonizes; it exits when the namespaces empty. | |
| f"{pasta} --netns /proc/$NSPID/ns/net --userns /proc/$NSPID/ns/user && " | |
| f"exec nsenter --preserve-credentials -U -n -t $NSPID -- {runsc}" | |
| ) | |
| script = ( | |
| # The holder pins the namespaces for the sandbox's lifetime; | |
| # --kill-child ties it to this script's process group. | |
| "unshare --user --map-root-user --net --fork --kill-child " | |
| f"bash -c 'echo $$ > {pidfile}; exec sleep infinity' & " | |
| "HOLDER_PID=$!; " | |
| f"for i in $(seq 1 100); do [ -s {pidfile} ] && break; kill -0 $HOLDER_PID 2>/dev/null || break; sleep 0.1; done; " | |
| f"NSPID=$(cat {pidfile} 2>/dev/null); " | |
| "[ -z '$NSPID' ] && { echo 'Failed to start namespace holder process' >&2; exit 1; }; " | |
| # pasta runs from the pod side (its uplink is the pod's real | |
| # interface), attaches to the holder's namespaces, and | |
| # daemonizes; it exits when the namespaces empty. | |
| f"{pasta} --netns /proc/$NSPID/ns/net --userns /proc/$NSPID/ns/user && " | |
| f"exec nsenter --preserve-credentials -U -n -t $NSPID -- {runsc}" | |
| ) |
There was a problem hiding this comment.
Fixed in a6911aa.
The wait loop now captures the holder pid (HOLDER=$!) and breaks the instant it dies (kill -0 $HOLDER 2>/dev/null || break) instead of spinning the full 10s, and an empty NSPID is rejected ([ -n "$NSPID" ] || { echo …; exit 1; }) so a lost holder can no longer yield /proc//ns/net. Both guards are pinned in test_build_run_command_public_wraps_with_pasta.
| head = user.split(":", 1)[0] | ||
| if head.isdigit(): | ||
| return user | ||
| rootfs = os.path.join(self._image_manager.get_image_dir(image), "rootfs") | ||
| try: | ||
| passwd = Path(os.path.join(rootfs, "etc", "passwd")).read_text( | ||
| encoding="utf-8", errors="replace" | ||
| ) | ||
| except OSError: | ||
| passwd = "" | ||
| for line in passwd.splitlines(): | ||
| parts = line.split(":") | ||
| if len(parts) >= 4 and parts[0] == user: | ||
| return f"{parts[2]}:{parts[3]}" |
There was a problem hiding this comment.
If user is passed as username:gid (e.g., postfix:104), parts[0] == user will fail because parts[0] is just the username. This raises SandboxExecError and prevents running as a specific user name with a specific gid. We should split user into head (username) and specified_gid, match parts[0] == head, and use specified_gid if provided.
parts_user = user.split(':', 1)
head = parts_user[0]
if head.isdigit():
return user
specified_gid = parts_user[1] if len(parts_user) > 1 else None
rootfs = os.path.join(self._image_manager.get_image_dir(image), 'rootfs')
try:
passwd = Path(os.path.join(rootfs, 'etc', 'passwd')).read_text(
encoding='utf-8', errors='replace'
)
except OSError:
passwd = ''
for line in passwd.splitlines():
parts = line.split(':')
if len(parts) >= 4 and parts[0] == head:
gid = specified_gid if specified_gid is not None else parts[3]
return f'{parts[2]}:{gid}'There was a problem hiding this comment.
Fixed in a6911aa.
_resolve_exec_user now splits with partition(":"), matches the passwd entry on the name alone, and honors an explicit gid when the caller wrote name:gid (falling back to the passwd login group otherwise). Added a postfix:999 -> 102:999 case to test_resolve_exec_user.
| for info in results: | ||
| if isinstance(info, _ApiError) and info.code == "sandbox_not_found": | ||
| continue | ||
| if isinstance(info, BaseException): | ||
| raise info |
There was a problem hiding this comment.
If any sandbox fails to describe (e.g., due to being permanently unschedulable or having some other actor/system error), list_sandboxes raises the exception, causing the entire GET /sandboxes request to fail. We should log the error and continue/skip the failed sandbox so that a single broken sandbox doesn't break the list API for all other sandboxes.
| for info in results: | |
| if isinstance(info, _ApiError) and info.code == "sandbox_not_found": | |
| continue | |
| if isinstance(info, BaseException): | |
| raise info | |
| for info in results: | |
| if isinstance(info, _ApiError) and info.code == 'sandbox_not_found': | |
| continue | |
| if isinstance(info, BaseException): | |
| logger.error('Failed to describe sandbox: %s', info) | |
| continue |
There was a problem hiding this comment.
This lives in the HTTP service layer (http/app.py), which is reviewed on the HTTP API PR #65633 — http/ only rides along in this diff because each PR is cut against master, not chained. To keep the two branches from diverging on the same file I'm addressing list_sandboxes robustness on #65633 rather than duplicating a fix here. Tracking it there.
| tmp_path = f"{runsc_path}.tmp.{os.getpid()}" | ||
| urllib.request.urlretrieve(url, tmp_path) |
There was a problem hiding this comment.
Using urllib.request.urlretrieve without a timeout can cause the thread to block indefinitely if the network connection hangs. We should use urllib.request.urlopen with an explicit timeout parameter and write the file using shutil.copyfileobj.
| tmp_path = f"{runsc_path}.tmp.{os.getpid()}" | |
| urllib.request.urlretrieve(url, tmp_path) | |
| tmp_path = f'{runsc_path}.tmp.{os.getpid()}' | |
| with urllib.request.urlopen(url, timeout=60) as response: | |
| with open(tmp_path, 'wb') as f: | |
| shutil.copyfileobj(response, f) |
There was a problem hiding this comment.
http/host.py is HTTP-service-layer code reviewed on the HTTP API PR #65633 (it only appears in this backend/netns diff because each PR is cut against master). The auto-install download paths are being reworked there — auto_install_runsc was already removed in review and auto_install_pasta is an open decision on that PR — so I'll fold the urlopen(timeout=…) hardening into whichever download path survives, on #65633, to keep this PR from diverging on that file.
| tmp_path = f"{pasta_path}.tmp.{os.getpid()}" | ||
| urllib.request.urlretrieve(url, tmp_path) |
There was a problem hiding this comment.
Using urllib.request.urlretrieve without a timeout can cause the thread to block indefinitely if the network connection hangs. We should use urllib.request.urlopen with an explicit timeout parameter and write the file using shutil.copyfileobj.
| tmp_path = f"{pasta_path}.tmp.{os.getpid()}" | |
| urllib.request.urlretrieve(url, tmp_path) | |
| tmp_path = f'{pasta_path}.tmp.{os.getpid()}' | |
| with urllib.request.urlopen(url, timeout=60) as response: | |
| with open(tmp_path, 'wb') as f: | |
| shutil.copyfileobj(response, f) |
There was a problem hiding this comment.
http/host.py is HTTP-service-layer code reviewed on the HTTP API PR #65633 (it only appears in this backend/netns diff because each PR is cut against master). The auto-install download paths are being reworked there — auto_install_runsc was already removed in review and auto_install_pasta is an open decision on that PR — so I'll fold the urlopen(timeout=…) hardening into whichever download path survives, on #65633, to keep this PR from diverging on that file.
Exposes ray.experimental.sandbox over a versioned REST API (/api/v1) served by Ray Serve, so sandboxes can be managed from outside the Ray cluster with nothing but an HTTP client and a bearer token — e.g. as an Anyscale service, or by agent-evaluation frameworks like Harbor. Design: - Each sandbox is a named, detached SandboxHost actor; the actors are the registry, so the Serve app is stateless and replicas can scale or restart without losing sandboxes. - Creation and execution are async submit + poll (with optional long-poll wait_seconds <= 30s) because image pulls and agent commands outlive HTTP requests and load-balancer limits. - The TTL reclaims both the sandbox and its hosting actor (the core runtime's TTL is deliberately disabled here so there is one owner). - Capabilities, network modes, DNS, shell, and workdir semantics are the core SandboxConfig's (ray-project#65570); the API validates network against VALID_NETWORK_MODES and defaults capabilities to DOCKER_DEFAULT_CAPABILITIES, patching nothing. - fastapi is only needed by this subpackage (ray[serve]); the base sandbox package never imports it. Testing: 54 unit tests run with no cluster and no runsc (fake runtime + fake actor resolver + FastAPI TestClient), including an OpenAPI contract snapshot; a runsc-gated integration test covers the real path. Validated end to end as a local 'serve run' and as an Anyscale service, driving real gVisor sandboxes. Signed-off-by: xyuzh <xinyzng@gmail.com>
POST /sandboxes now synthesizes its 202 response from the request instead of awaiting describe() on the new actor: on a saturated cluster the actor may be queued behind capacity for longer than a client (or load balancer) read timeout, and the endpoint has everything it needs to answer without touching the actor. Surfaced by a Harbor concurrency test that oversubscribed a single node. Signed-off-by: xyuzh <xinyzng@gmail.com>
A 16-way Terminal-Bench run against a cold cluster surfaced two capacity bugs: - The Serve deployment used the default max_ongoing_requests, but this API is long-poll based (requests deliberately hold a slot for up to ~30s), so a handful of concurrent clients saturated the replica and the platform load balancer answered 503 for everyone else. Raise it to 1000; the app is entirely async I/O. - Calls to a SandboxHost whose detached actor exists but has not been *scheduled* yet (cluster autoscaling) block indefinitely. Bound every actor call by the request's own long-poll budget plus a configurable scheduling grace: describe paths report a synthesized 'pending' instead of hanging, and exec/file paths return 409 with a retry hint. Signed-off-by: xyuzh <xinyzng@gmail.com>
An actor whose cpu/memory shape can never fit the cluster raises ActorUnschedulableError from any call; the app let it escape as an opaque 500. Terminal-Bench tasks declaring cpus=4/memory_mb=8192 on a cluster of 4CPU-16GB workers hit this for every large task. Map it to 409 'unschedulable' carrying Ray's own message, so clients see exactly which resource shape cannot be satisfied. Signed-off-by: xyuzh <xinyzng@gmail.com>
Proxies in front of a deployed service cap request bodies (an Anyscale ingress rejected a 4.8MB upload with 413 and killed an 11MB one mid-body), so single-request file uploads have a hidden size ceiling. PUT /files gains an append flag, plumbed through host, runtime, and backend (cat >> instead of cat >), so clients can chunk arbitrarily large uploads into proxy-sized pieces. Signed-off-by: xyuzh <xinyzng@gmail.com>
Match the style established in ray-project#65627 for the networking section: soft-wrapped prose, em-dash and semicolon clauses split into separate sentences, and bold list leads. Also document the new append flag on PUT /files and the 409 unschedulable error code. Signed-off-by: xyuzh <xinyzng@gmail.com>
… shared runsc cache, concurrent listing Three review findings: - A dead detached actor made its client_token permanently return 404; the idempotent-create path now clears the dead actor and creates a fresh sandbox under the same name. - The opt-in runsc download leaked a ~40MB temp dir per boot and raced concurrent boots; it now uses one shared cached path per node with an atomic rename. - GET /sandboxes described actors sequentially; it now gathers concurrently. Signed-off-by: xyuzh <xinyzng@gmail.com>
runsc exec takes numeric -user uid[:gid]; names are resolved against the image's own /etc/passwd host-side. Plumbed through runtime, the HTTP API (StartExecRequest.user), and the backend, with tests. Brings exec to parity with container engines' exec --user and the Harbor environment contract's user= parameter. Signed-off-by: xyuzh <xinyzng@gmail.com>
… via pasta Each public-mode sandbox now runs inside its own network namespace bridged by pasta (passt) user-mode networking: internet egress works, but ports and loopback are private — a bind on 0.0.0.0 cannot collide with the worker pod or other sandboxes, and nothing in the sandbox is reachable from outside it. This fulfills public's documented contract (egress without the host's network identity) and unblocks workloads that bind fixed ports, e.g. QEMU hostfwd tasks in Terminal-Bench. runsc still runs with --network=host; host is simply the private namespace. The mount namespace stays shared so runsc control sockets keep working. RAY_SANDBOX_PUBLIC_HOST_NETNS=1 on workers restores the previous shared-namespace behavior without a deploy; the auto_install_pasta server setting (default off) downloads a static pasta build on nodes that lack the passt package. Signed-off-by: xyuzh <xinyzng@gmail.com>
…ns via pasta Each public-mode sandbox gets a private user+network namespace pair, bridged by pasta (passt) user-mode networking in the rootless-Podman topology: a holder process pins the namespaces, pasta attaches from the pod side (so its uplink is the pod's real interface) and daemonizes, and runsc enters via nsenter as mapped root. Internet egress works, but ports and loopback are private — a bind on 0.0.0.0 cannot collide with the worker pod or other sandboxes, and nothing in the sandbox is reachable from outside it. This fulfills public's documented contract (egress without the host's network identity) and unblocks workloads that bind fixed ports, e.g. QEMU hostfwd tasks in Terminal-Bench. runsc still runs with --network=host — "host" is simply the private namespace — and without --rootless: it is already root in the holder's user namespace, and a nested user namespace would put the gofer's /proc/<pid>/root magic links out of reach. Mount and pid namespaces stay shared, so the bundle and the runsc control sockets keep working for pod-side state/exec/kill/delete. The host boot path creates /dev/net/tun (absent in fresh Kubernetes pods) via passwordless sudo. RAY_SANDBOX_PUBLIC_HOST_NETNS=1 on workers restores the previous shared-namespace behavior without a deploy; the auto_install_pasta server setting (default off) downloads a static pasta build on nodes without the passt package. Signed-off-by: xyuzh <xinyzng@gmail.com>
Cut the duplication and the over-long prose the netns tests picked up: - conftest: fold the runsc and pasta downloads into one _install_on_path helper; the two fixtures were the same twenty lines twice. - Share _public_config() and a _run_argv() argv builder instead of rebuilding a backend and config in every test. - Add a no_host_netns fixture for the repeated kill-switch delenv, and parametrize the non-public network modes so a failure names the mode. - Drop the real_which passthrough in the missing-pasta test: patch gvisor.shutil.which by path and return a stub for everything else. - Lift _pasta_pids and _host_ip to module scope, and trim the docstrings and comments to the claim each test actually makes. No coverage changes: same eight tests, same assertions. Signed-off-by: xyuzh <xinyzng@gmail.com>
8e48d3c to
1238fd1
Compare
…t the sandbox The pasta path parks each sandbox in an `unshare --user --net` holder and has pasta and a non-rootless runsc re-enter it via `nsenter -U -n`. Some sandboxed CI environments allow a single unprivileged user namespace (enough for the rootless sandbox tests) and even entering another process's, yet still deny the nested user namespace runsc opens when it drops the sandbox process to `nobody` -- surfacing only once the sandbox boots as `Started as root, will change to nobody. Couldn't open user namespace ...: Permission denied`. A namespace-entry probe passes in those environments and the tests then fail, so `ensure_pasta` instead brings a throwaway busybox `network="public"` sandbox all the way up and tears it down: only the real path exercises that nested open. runsc and pasta are installed before the probe runs. Signed-off-by: xyuzh <xinyzng@gmail.com>
1238fd1 to
04e7ef8
Compare
The sandbox HTTP API library (//python/ray/experimental/sandbox/http:sandbox_http_lib) depends on //python/ray/serve:serve_lib for the FastAPI/@serve.ingress app, but serve_lib's visibility did not include the sandbox http package. Bazel failed analysis with 'target //python/ray/serve:serve_lib is not visible', so the sandbox test job aborted the build even though the analyzed sandbox tests passed. Add the http package to serve_lib's visibility list. Signed-off-by: xyuzh <xinyzng@gmail.com>
The core sandbox test job builds with --install-mask all-ray-libraries, which removes python/ray/serve from the tree, so the //python/ray/serve:serve_lib bazel dep resolves to "no such package" and the runsc-gated integration test has no serve to import. app.py already imports serve lazily inside build_app, so the py_library needs no serve dep: drop it (and the now-moot visibility grant on serve/BUILD.bazel) and importorskip serve in the end-to-end test, which only runs in a full dev container where serve is installed. Signed-off-by: xyuzh <xinyzng@gmail.com>
Two failures surfaced once the core sandbox job started running these
tests (it installs with --install-mask all-ray-libraries):
- test_http_app: three tests drove TestClient with a bare client instead
of `with _client(...) as client:`, so each request ran on a fresh event
loop while the SandboxHost's asyncio.Events stayed bound to the first
one ("bound to a different event loop"). Wrap them like the others.
- test_http_integration: the mask leaves `ray.serve` importable but
without `serve.run`, so importorskip did not skip. Add a hasattr guard.
Signed-off-by: xyuzh <xinyzng@gmail.com>
|
@xyuzh Are you working on making this PR not depend on the http stuff and merging the backend first? |
Address review feedback on the pasta/netns backend: - Record the pasta daemon pid (pasta --pid) and SIGKILL it in both teardown paths. pasta daemonizes via setsid() and escapes the process group _terminate_tree reaps, so on passt builds that don't self-exit when the namespaces empty it would leak; _kill_pasta reaps it by pid. - Fast-fail the holder wait loop the moment the holder dies (kill -0) instead of spinning the full 10s, and refuse an empty NSPID so a lost holder can't resolve to /proc//ns/net. - Bound runsc delete with a timeout so a wedged gVisor can't block the process-group kill and pasta reap that actually free the sandbox. - Poll runsc state before the boot deadline check, so a sandbox that reaches 'running' just as the deadline passes is kept, not torn down. - Resolve exec user 'name:gid' by matching the name and honoring the explicit gid (previously compared the whole 'name:gid' to the passwd name and always missed). Signed-off-by: xyuzh <xinyzng@gmail.com>
Done |
Resolve doc conflict in doc/source/ray-core/sandboxes.md: the network-mode table's public/host rows are rewritten by this PR to describe the per-sandbox pasta network namespace (and move the untrusted-code guidance from 'none' to 'host'), so keep this branch's rows over master's pre-pasta 'Host egress' description. Signed-off-by: xyuzh <xinyzng@gmail.com>
|
ci failure not related |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
Reviewed by Cursor Bugbot for commit 82cebd7. Configure here.
| try: | ||
| os.kill(pid, signal.SIGKILL) | ||
| except (ProcessLookupError, PermissionError): | ||
| pass |
There was a problem hiding this comment.
Distro pasta pidfile misses daemon
Medium Severity
_kill_pasta SIGKILLs the PID from pasta --pid, but Debian 12 / Ubuntu 23.04 passt writes that file with the pre-fork parent. The parent has already exited by the time nsenter runs, so teardown hits a dead PID and the setsid daemon is left behind. Those are the distro packages the docs tell operators to install.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 82cebd7. Configure here.
| # runsc runs as mapped root inside the holder's user namespace; | ||
| # --rootless would nest a second user namespace whose | ||
| # /proc/<pid>/root magic links the gofer cannot dereference. | ||
| args = [a for a in args if a != "--rootless"] |
There was a problem hiding this comment.
Public mode drops rootless cgroups
Medium Severity
network="public" strips --rootless from runsc run so the gofer can enter the holder userns, but it does not pass --ignore-cgroups. runsc then sets up cgroups as mapped root against the worker's cgroupfs, which fails in the same unprivileged environments where none and host still start.
Reviewed by Cursor Bugbot for commit 82cebd7. Configure here.
… layer Signed-off-by: xyuzh <xinyzng@gmail.com>


Description
network="public"sandboxes currently run with runsc--network=hostin the Ray worker's own network namespace: every sandbox on a node shares one port space, so concurrent workloads that bind a fixed port collide and can reach each other's listeners. The concrete failure is terminal-bench's QEMU tasks (qemu-startup,qemu-alpine-ssh), which start QEMU withhostfwd=tcp::2222-:22and then SSH tolocalhost:2222from inside the same sandbox — under co-tenancy the second bind getsEADDRINUSE, and a verifier can connect to a different sandbox's guest.This PR gives each
publicsandbox a private user+network namespace pair bridged by pasta (passt) user-mode networking, the rootless-Podman topology:unshare --user --map-root-user --net) pins the namespaces for the sandbox's lifetime;pastaattaches from the pod side (--netns/--userns /proc/$PID/ns/*), configures a tap with the pod's addressing, and daemonizes;-t/-u/-T/-U none --no-map-gwmake it egress-only — in-sandbox binds are never republished on the pod, pod-local services are unreachable from the sandbox loopback, and there is no inbound path;runsc runexecutes inside viansenteras mapped root (no--rootless; nesting a second userns breaks the gofer's/procmagic-link derefs). runsc still gets--network=host, but "host" is now private to the sandbox. Mount and pid namespaces stay shared, so the bundle and control sockets under--rootkeep working for pod-sidestate/exec/kill/delete.Semantics:
publicfinally matches its documented contract (egress without the host's network identity);hostremains the explicit shared-namespace mode.RAY_SANDBOX_PUBLIC_HOST_NETNS=1on workers restores the previous behavior without a code deploy. The HTTP API gains an opt-inauto_install_pastasetting mirroringauto_install_runsc, and boot best-effort-creates/dev/net/tun(K8s containers ship a minimal/devwithout it). Requirespastaandnsenteron nodes forpublicsandboxes; docs updated (mode table, requirements, install snippets, troubleshooting).Stacked on #65633 — the first 8 commits are that PR; please review the top 2 commits only until it merges.
Related issues
Related to #65633.
Additional information
Tested with
TEST_SANDBOX=1gated tests in a privileged dev container (non-root user, matching production posture): two concurrentpublicsandboxes both bind0.0.0.0:2222and each reaches its own listener on127.0.0.1:2222; the worker namespace shows nothing on 2222; no address names one sandbox from another (pasta gives every sandbox the pod's own IP, so cross-sandbox fetches resolve to self or fail); egress + generated-resolv.conf DNS work; teardown reaps the holder/pasta/runsc group with no leaked processes, including the create-failure path. The exact pasta flag list is pinned by a unit test since the flags are the isolation property.