[core][sandbox] Make Ray Sandbox run Docker-built images out of the box - #65570
Conversation
Adds first-class SandboxConfig support for what previously required the private _oci_spec_transform_fn hook and client-side workarounds (found while integrating Ray Sandbox with the Harbor eval framework): - capabilities: additional Linux capabilities unioned into the bounding/effective/inheritable/permitted sets (ambient untouched), plumbed through create(), the Sandbox actor, SandboxRuntime, and the gVisor backend. New DOCKER_DEFAULT_CAPABILITIES constant for Docker parity: the default spec grants only three capabilities, and standard images break without Docker's set — apt-get needs CAP_SETUID and CAP_SETGID for its _apt sub-processes, and tar extracting as root fatally fails to restore archived uid/gids (e.g. 1000) without CAP_CHOWN. - network="host" now works out of the box: the generated OCI spec drops its empty "network" namespace (which previously left the sandbox loopback-only despite --network=host) and bind-mounts the host's /etc/resolv.conf read-only, mirroring Docker, so DNS works. - SandboxConfig validates the network mode. Defaults are unchanged (network="none", capabilities=None). Signed-off-by: xyuzh <xinyzng@gmail.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for configuring Linux capabilities and network modes in the Ray sandbox environment. It introduces a capabilities parameter to grant additional Linux capabilities (with DOCKER_DEFAULT_CAPABILITIES provided as a helper) and supports "host" networking by dropping the empty network namespace and bind-mounting the host's /etc/resolv.conf. The review feedback identifies potential AttributeError risks when using setdefault on OCI spec dictionaries (specifically for the "capabilities" and "linux" keys) if they are already present with null values, and suggests safer dictionary retrieval and validation patterns.
| if capabilities: | ||
| caps = spec["process"].setdefault("capabilities", {}) | ||
| for cap_set in _OCI_CAPABILITY_SETS: | ||
| # Union rather than replace, so anything the runtime default | ||
| # grants survives. | ||
| merged = dict.fromkeys([*caps.get(cap_set, []), *capabilities]) | ||
| caps[cap_set] = list(merged) |
There was a problem hiding this comment.
Using setdefault on spec["process"] can return None if the "capabilities" key already exists in the dictionary with a null (or None) value. This will cause an AttributeError when calling caps.get(...) on the next lines. To prevent this, we should explicitly check if caps is a dictionary and initialize it if it is not.
if capabilities:
process_spec = spec["process"]
caps = process_spec.get("capabilities")
if not isinstance(caps, dict):
caps = {}
process_spec["capabilities"] = caps
for cap_set in _OCI_CAPABILITY_SETS:
# Union rather than replace, so anything the runtime default
# grants survives.
current_caps = caps.get(cap_set)
if not isinstance(current_caps, list):
current_caps = []
merged = dict.fromkeys([*current_caps, *capabilities])
caps[cap_set] = list(merged)There was a problem hiding this comment.
Done in e762a8e — guarded with an isinstance check (and the same for the linux section below), plus a regression test covering a caller-supplied base_spec with null capabilities/linux sections.
| namespaces = spec.setdefault("linux", {}).get("namespaces", []) | ||
| spec["linux"]["namespaces"] = [ | ||
| ns | ||
| for ns in namespaces | ||
| if not (ns.get("type") == "network" and not ns.get("path")) | ||
| ] |
There was a problem hiding this comment.
Using spec.setdefault("linux", {}) can return None if the "linux" key already exists in the dictionary with a null (or None) value. This will cause an AttributeError when calling .get("namespaces", []) on it. Additionally, if "namespaces" is not a list, iterating over it or assigning to it can raise errors. We should safely retrieve and validate the "linux" section and "namespaces" list.
| namespaces = spec.setdefault("linux", {}).get("namespaces", []) | |
| spec["linux"]["namespaces"] = [ | |
| ns | |
| for ns in namespaces | |
| if not (ns.get("type") == "network" and not ns.get("path")) | |
| ] | |
| linux_spec = spec.get("linux") | |
| if not isinstance(linux_spec, dict): | |
| linux_spec = {} | |
| spec["linux"] = linux_spec | |
| namespaces = linux_spec.get("namespaces") | |
| if isinstance(namespaces, list): | |
| linux_spec["namespaces"] = [ | |
| ns | |
| for ns in namespaces | |
| if isinstance(ns, dict) and not (ns.get("type") == "network" and not ns.get("path")) | |
| ] |
There was a problem hiding this comment.
Done in e762a8e — the linux section is validated the same way, and namespaces is only rewritten when it is actually a list.
| timeout_seconds: Timeout in seconds for sandbox creation. | ||
| rootless: If True, run gVisor in rootless mode. | ||
| network: Network mode for runsc. | ||
| network: Network mode for runsc ("none", "host", "sandbox"). With |
There was a problem hiding this comment.
It is worth checking out the runsc do defaults https://github.com/google/gvisor/blob/6d32647a93203f3c1cc3b1bd3b50791718b13874/runsc/cmd/do.go#L356
"none" seems like the right default according to the principle to have safe defaults, but then on top of that we should have a setting that would be recommended to most people who want their sandboxes to have internet access and use that in our examples and make it prominent in the docs (maybe call it "public" and for that my hunch would be not to mount in the host /etc/resolv.conf but instead pass a very simple one that has a working public DNS server like 8.8.8.8, because we don't want the sandbox config to depend on the host config so it is portable, and also the host config might have stuff in there that people don't want to expose to their sandbox). We could have a separate "host" config, or just make it easy for people to bind mount their host configs directly and have examples for it.
There was a problem hiding this comment.
Agreed on keeping "none" as the safe default, and +1 to a prominent, recommended internet-access preset.
On the DNS shape: the reason this PR mirrors Docker's share-the-host-resolver behavior for host mode is a tradeoff we hit in practice — hardcoding a public resolver like 8.8.8.8 breaks clusters where outbound DNS to the internet is blocked and only the VPC/cluster resolver works (exactly the managed-cloud setup we tested this on: the host resolv.conf pointed at the VPC resolver and public DNS was not reachable). Since host mode already shares the host's network stack, inheriting its resolver seemed like the least surprising default there. Your portability point stands though, especially for --network=sandbox (netstack), where there is no host stack to inherit and a generated public-DNS default makes sense.
Concretely, happy to do either (here or as the follow-up that adds the "public" preset): (a) a dns_servers: Optional[List[str]] field on SandboxConfig — when set, we generate a minimal bundle-local resolv.conf with exactly those nameservers and mount that instead of the host file (so dns_servers=["8.8.8.8"] gives you the portable behavior), or (b) bake that generated file into the "public" preset itself. Which shape do you prefer?
There was a problem hiding this comment.
Surveyed what comparable products do before answering:
| Provider | Default egress | Network model | Internet / restriction config | DNS |
|---|---|---|---|---|
| Modal (gVisor) | Open ("Sandboxes can make outbound connections to any public IP"); inbound closed | Isolated per-sandbox network + provider NAT — no host/localhost access | block_network, outbound_cidr_allowlist, outbound_domain_allowlist (TLS/SNI) |
provider-generated |
| E2B (Firecracker) | Open; private ranges and the cloud metadata endpoint (RFC1918, 169.254.0.0/16) are always blocked at the edge | Isolated microVM network | allowInternetAccess: false, denyOut/allowOut (IP/CIDR/domain) |
provider-generated |
| Docker | Open (bridge + MASQUERADE NAT) | Isolated netns + veth/bridge | --network none, custom networks |
default bridge: copy of the host's resolv.conf; user-defined networks: embedded DNS at 127.0.0.11 forwarding to the host's upstreams |
runsc do --net |
Open (veth + MASQUERADE it sets up itself) | Isolated, pathed netns | n/a | generates nameserver 8.8.8.8 plus /etc/hostname and /etc/hosts |
| Harbor (consumer) | Tasks default to network_mode: public; tasks may declare no-network or allowlist (hostnames + CIDRs) which environments must enforce or reject |
provider-specific | per-task policy | provider-specific |
Two takeaways:
- Every comparable product's internet path is an isolated network with NAT'd egress plus generated network configs — none of them point users at host networking. And notably, Modal and E2B keep the host and private ranges unreachable even with egress open (E2B hard-blocks RFC1918 and 169.254.0.0/16). That's also the strongest argument against recommending
hostfor untrusted code: a host-netns sandbox on a Ray node can reach node-local services (GCS, dashboard, raylet ports on localhost) and the instance metadata service. noneas our default is stricter than Modal/E2B/Docker, but for a library primitive I agree it's right — the gap to fill is a good recommended "internet on" story.
So +1 to your "recommend sandbox with configs we generate", concretely:
- Productize what
runsc do --netalready does: veth pair + MASQUERADE into a pathed netns, plus generated/etc/resolv.conf,/etc/hostname,/etc/hosts. For DNS I'd generate Docker-style rather than hardcode 8.8.8.8: copy the host's nameservers (filtered), falling back to public DNS only when none remain — hardcoded 8.8.8.8 breaks clusters where outbound 53 is blocked and only the VPC resolver works (the managed-cloud setup I tested this PR on), while copy-with-fallback works in both worlds and the file is still ours, not a host bind-mount. - Follow Modal/E2B and default-deny private ranges + the metadata IP on that path even when egress is open.
- That veth is also exactly where allowlists land (iptables/nftables or netstack filtering) — matching both Modal's outbound options and the
allowlistpolicy Harbor already models (hostnames + CIDRs), so the Harbor integration can eventually mappublic/no-network/allowlistall onto this one mechanism. - One constraint to design around: veth + iptables needs CAP_NET_ADMIN/root on the node, which conflicts with the rootless posture we currently recommend — rootless needs slirp4netns-style userspace NAT (how rootless Docker/Podman solve it) or a small privileged node helper. Until that exists, docs-wise I'd say:
nonedefault; the generated-configsandboxpath as the recommendation once available;hostdocumented as the trusted-single-tenant escape hatch with an explicit warning about node-local services and metadata exposure.
There was a problem hiding this comment.
Thanks a lot for looking into this, let's also add what https://github.com/google/gvisor/blob/master/runsc/cmd/do.go does, and then why don't you make a recommendation on what you think is the best /recommended/ setting, while keeping it minimal for now but extensible for the future? :)
There was a problem hiding this comment.
Done — the recommendation is implemented in this PR as network="public" (fb7c66e + docs): host egress with a synthetic /etc/resolv.conf generated from dns (default 8.8.8.8, 1.1.1.1), inheriting nothing from the host resolver. That's deliberately the minimal version of what runsc do does — it writes exactly nameserver 8.8.8.8 for its containers (do.go's makeFile), and its fuller recipe (veth pair + MASQUERADE NAT into a pathed netns) is the natural later implementation of the same mode once we want isolation from the host network too, plus the place allowlists land. The user-facing surface shouldn't need to change for that: public keeps meaning 'internet, portable config', and only the plumbing under it gets stronger. dns= covers locked-down VPCs today, and the docs got a mode table with the security property of each mode plus the recommended public + DOCKER_DEFAULT_CAPABILITIES example.
| # extracting as root restores the archived owner uid/gid (needs CAP_CHOWN), | ||
| # both fatally. Pass ``capabilities=DOCKER_DEFAULT_CAPABILITIES`` to run an | ||
| # image the way Docker would. | ||
| DOCKER_DEFAULT_CAPABILITIES = [ |
There was a problem hiding this comment.
Is this an officially documented list by docker somewhere? If yes, can you add a URL to the comment?
I like this btw -- very minimal capabilities by default, but have a very convenient list that users can use (and that we can e.g. use for the harbor integration).
There was a problem hiding this comment.
Added in e762a8e — cited both https://docs.docker.com/engine/containers/run/#runtime-privilege-and-linux-capabilities and moby's canonical list (https://github.com/moby/moby/blob/master/oci/caps/defaults.go), and noted that the runtime default is whatever runsc spec emits.
There was a problem hiding this comment.
agree this will be pretty handy
| spec["process"]["env"] = envs | ||
|
|
||
| if capabilities: | ||
| caps = spec["process"].setdefault("capabilities", {}) |
There was a problem hiding this comment.
If I understand this correctly, these defaults come from runsc spec, which is pretty bare bones but also somewhat arbitrary (https://github.com/google/gvisor/blob/master/runsc/cmd/spec.go#L101). It might be good to document that and tell people if they want no capabilities, they can use the _oci_spec_transform_fn to erase them.
Another choice that would be worth considering is to start with no capabilities by default. Curious about your thoughts @andrewsykim
There was a problem hiding this comment.
Documented in e762a8e: the default set is whatever runsc spec emits (minimal but somewhat arbitrary, with a link to spec.go), capabilities only ever adds on top of it, and running with no capabilities at all is still possible by erasing the sets via _oci_spec_transform_fn.
On defaulting to no capabilities at all: this PR deliberately keeps the default unchanged to stay behavior-neutral, but I don't have a strong attachment — with the capabilities field in place, either default is a one-liner for users, and flipping to an empty default while the API is alpha would be a small follow-up if you and Andrew land there. The main consumer-facing need was a sanctioned way to get Docker parity without the private transform hook.
… citations - Guard against caller-supplied base_specs carrying null capabilities/ linux sections (setdefault would hand the null straight back), with a regression test. - Cite Docker's documented default capability list (docs.docker.com and moby's oci/caps/defaults.go) and note that the runtime's own default set is whatever runsc spec emits. - Document that the capabilities option only ever adds capabilities, and that running with none at all is still possible by erasing the sets via _oci_spec_transform_fn. Signed-off-by: xyuzh <xinyzng@gmail.com>
…tays visible The workdir bind-mount (a host-backed scratch directory) shadows any content the image ships at its WORKDIR — e.g. an image built with WORKDIR /app and task files baked into /app comes up with an empty /app. Consumers had to work around this by creating sandboxes with workdir="/" (skipping the mount) and passing a cwd on every exec, which defeats the intuitive default of running commands in the image's own working directory. mount_workdir=True keeps the historical behavior (workdir is the host-backed writable path on readonly rootfses). mount_workdir=False leaves the image filesystem untouched, so workdir only selects the process working directory — combine with readonly=False for writability via the per-sandbox overlay. Signed-off-by: xyuzh <xinyzng@gmail.com>
| # extracting as root restores the archived owner uid/gid (needs CAP_CHOWN), | ||
| # both fatally. Pass ``capabilities=DOCKER_DEFAULT_CAPABILITIES`` to run an | ||
| # image the way Docker would. | ||
| DOCKER_DEFAULT_CAPABILITIES = [ |
There was a problem hiding this comment.
agree this will be pretty handy
| the runtime defaults; the ambient set is deliberately left alone. | ||
| Use :data:`DOCKER_DEFAULT_CAPABILITIES` to match how Docker runs | ||
| images. None (default) keeps the runtime's defaults — the set that | ||
| ``runsc spec`` emits, which is minimal but somewhat arbitrary. This |
There was a problem hiding this comment.
nit: i think we can remove the "which is minimal but somewhat arbitrary" part here
| Use :data:`DOCKER_DEFAULT_CAPABILITIES` to match how Docker runs | ||
| images. None (default) keeps the runtime's defaults — the set that | ||
| ``runsc spec`` emits, which is minimal but somewhat arbitrary. This | ||
| option only ever adds capabilities; to remove even the runtime |
There was a problem hiding this comment.
why not an empty array to remove all capabilities?
There was a problem hiding this comment.
Good call — changed in bb96843: capabilities now sets the bounding/effective/permitted sets to exactly the given list instead of unioning on top of the runtime defaults, so [] runs the sandbox with no capabilities at all and no transform hook is needed to drop privileges. None keeps the runtime default, and DOCKER_DEFAULT_CAPABILITIES is a superset of the runtime defaults, so the Docker-parity path is byte-identical to before.
|
|
||
| # The OCI capability sets a container process starts with. "ambient" is | ||
| # deliberately excluded: these are the sets Docker populates, and ambient | ||
| # capabilities would additionally survive into non-root execve'd children. |
There was a problem hiding this comment.
is there official docker documentation for these similar to https://docs.docker.com/engine/containers/run/#runtime-privilege-and-linux-capabilities?
There was a problem hiding this comment.
Added citations in bb96843: the set semantics are the OCI runtime-spec process.capabilities schema (https://github.com/opencontainers/runtime-spec/blob/main/config.md#linux-process), and the sets Docker populates are in moby's oci/defaults.go (https://github.com/moby/moby/blob/master/oci/defaults.go). While adding them I also aligned the constant with modern Docker: it no longer writes the inheritable set, which Docker stopped setting for CVE-2022-24769 (GHSA-2mm7-x5h6-5pvq); ambient stays untouched.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…eout detection - Sandboxes are now created without a workdir override and with mount_workdir=False (ray-project/ray#65570): the sandbox's default cwd is the image's own WORKDIR — the same default as the Docker environment — instead of requiring workdir='/' to avoid Ray's host scratch mount shadowing image content. Exec cwd precedence is unchanged: explicit cwd > [environment].workdir > image WORKDIR. - Timeout detection now uses isinstance against the real SandboxTimeoutError (Ray re-raises remote errors as a RayTaskError subclass of the original type), replacing exception-name matching. - preflight() now checks for both capabilities and mount_workdir support in the installed Ray.
… impossible network/rootless combo - mount_workdir was declared on SandboxRuntime.create() but never forwarded into the SandboxConfig it builds, so the feature was inert for every caller (including the Sandbox actor). Add a runtime-layer test suite that pins config fields actually reaching the backend — the gap that let this ship green. - Guard the per-set capability union and the namespaces comprehension against null capability sets and non-dict namespace entries in caller-supplied base_specs (follow-ups to the earlier dict-level guards). Malformed namespace entries are kept, not dropped, so a bad spec fails in runsc with runsc's own error. - Reject network='sandbox' with rootless=True at config time: runsc refuses the combination at container start, after the image pull, with an error naming a flag the user never set. - Keep existing_dests in sync when the workdir bind is appended. Signed-off-by: xyuzh <xinyzng@gmail.com>
Three findings behind the 1h default: - It is wall-clock from creation, not an idle timeout: an actively running sandbox was deleted mid-work at the 1h mark — a silent, hard-to-attribute failure partway through long agent runs. - Below the actor layer it was a documented no-op: SandboxRuntime accepted and stored ttl_seconds but nothing enforced it, so direct runtime users (which the docs recommend for custom actors) got no cleanup while believing they configured it. - Opt-in destruction is the safer default: no TTL leaks a sandbox the user can see and kill; a default TTL loses work the user did not know was on a clock. ttl_seconds now defaults to None everywhere, SandboxRuntime enforces a set TTL with a daemon timer (canceled on delete), and the Sandbox actor drops its duplicate timer in favor of the runtime's. Signed-off-by: xyuzh <xinyzng@gmail.com>
The workdir bind exists for exactly one reason: giving a readonly rootfs one writable path. On a writable rootfs (readonly=False) the per-sandbox overlay already covers writes, so the mount buys nothing and costs the image's WORKDIR content. mount_workdir now defaults to None = auto: mounted when readonly=True (today's behavior for the readonly default), unmounted when readonly=False, giving Docker-like WORKDIR behavior with no extra flag. Explicit True/False still forces either combination. Signed-off-by: xyuzh <xinyzng@gmail.com>
'public' is the recommended internet-access mode: host egress (--network=host under the hood) with a synthetic /etc/resolv.conf generated from dns (default: 8.8.8.8, 1.1.1.1) instead of the host's file. Nothing about the host resolver is inherited, so the sandbox config stays portable and does not leak host search domains or internal resolver addresses (a cluster node's resolv.conf typically carries internal Kubernetes search domains, the cluster DNS VIP, and ndots:5). 'host' remains the explicit power mode — full host network identity, including internal networks reachable from the node, with the host's own resolv.conf mounted read-only (dns= overrides it with a generated file). Keeping the two modes separate lets the docs state that difference plainly. 'none' stays the default. dns= mirrors docker --dns and is the escape hatch for locked-down VPCs that block public DNS: pass the internal resolver IPs. It is rejected for modes that mount no resolv.conf. The resolv.conf policy lives in prepare_oci_bundle (which owns the bundle directory the generated file is written into); create_oci_spec just mounts an explicit resolv_conf_source. Signed-off-by: xyuzh <xinyzng@gmail.com>
String commands ran under /bin/sh, which is dash on Debian-family images; agent-generated commands overwhelmingly assume bash, and the failures are the confusing kind ([[ ]], pipefail, arrays, source) because dash's diagnostics never say 'you are not in bash'. Consumers were re-wrapping every exec in bash -c with a quoting round-trip. SandboxConfig gains shell (default None = auto-detect: probe /bin/bash once at creation with a single argv exec and cache it in the sandbox metadata, falling back to /bin/sh), and exec() gains a per-command shell override. List commands still bypass the shell entirely. Signed-off-by: xyuzh <xinyzng@gmail.com>
…ties Adds a 'Networking and DNS' section with the four modes and the security property of each — stating outright that 'host' exposes the node's internal network while 'public' inherits nothing from the host resolver — plus the recommended internet-access example (network='public' + DOCKER_DEFAULT_CAPABILITIES) and the dns= escape hatch for locked-down VPCs. Also updates the OCI-transform section: networking, DNS, and capabilities no longer need the hook (it remains the answer for host mounts, capability *removal*, and the long tail), and fixes its example, which used network='sandbox' — now rejected under the default rootless=True — and hand-mounted resolv.conf. Signed-off-by: xyuzh <xinyzng@gmail.com>
Signed-off-by: xyuzh <xinyzng@gmail.com>
…n; document requirements - Timeout detection: Ray re-raises remote exceptions as a RayTaskError subclass of the original type, and when the type can't be subclassed it falls back to a plain RayTaskError carrying the original in .cause. Exception __cause__/__context__ chains do not survive Ray serialization, so the cause-chain walk could never find anything isinstance would miss — replaced with isinstance + .cause. The test that justified the walk asserted an in-process shape production cannot produce; it now pins the .cause fallback instead. - Directory uploads take tar's default ownership restore like every other environment: the original --no-same-owner reason (root without CAP_CHOWN) is gone now that the default capability set is Docker's. This also reverts the same_owner parameter added to tar_transfer.py, shrinking the diff outside the new environment. The capabilities docstring notes uploads rely on CAP_CHOWN when narrowing the set. - Document that task images must provide bash and tar (execs are wrapped in bash -c; transfers stage through tar), note that mount_workdir=False additionally depends on ray-project/ray#65570's runtime forwarding fix (the preflight gate can only check field presence), and add a drift TODO for the inlined Docker capability list.
…n; document requirements - Timeout detection: Ray re-raises remote exceptions as a RayTaskError subclass of the original type, and when the type can't be subclassed it falls back to a plain RayTaskError carrying the original in .cause. Exception __cause__/__context__ chains do not survive Ray serialization, so the cause-chain walk could never find anything isinstance would miss — replaced with isinstance + .cause. The test that justified the walk asserted an in-process shape production cannot produce; it now pins the .cause fallback instead. - Directory uploads take tar's default ownership restore like every other environment: the original --no-same-owner reason (root without CAP_CHOWN) is gone now that the default capability set is Docker's. This also reverts the same_owner parameter added to tar_transfer.py, shrinking the diff outside the new environment. The capabilities docstring notes uploads rely on CAP_CHOWN when narrowing the set. - Document that task images must provide bash and tar (execs are wrapped in bash -c; transfers stage through tar), note that mount_workdir=False additionally depends on ray-project/ray#65570's runtime forwarding fix (the preflight gate can only check field presence), and add a drift TODO for the inlined Docker capability list.
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 9065a34. Configure here.
The REST service layer is the second step of this work and comes as a follow-up PR; it was swept in by a directory-level git add. Signed-off-by: xyuzh <xinyzng@gmail.com>
…ern Docker Address review: - capabilities now sets the bounding/effective/permitted sets to exactly the given list instead of unioning on top of the runtime defaults, so [] runs the sandbox with no capabilities at all (no transform hook needed to drop privileges). None keeps the runtime default, and DOCKER_DEFAULT_CAPABILITIES — a superset of the runtime defaults — reproduces Docker's behavior unchanged. - The inheritable set is no longer written: modern Docker stopped setting inheritable capabilities (CVE-2022-24769, GHSA-2mm7-x5h6-5pvq); citations for the set behavior added (OCI runtime-spec process.capabilities, moby oci/defaults.go). - Drop an editorializing clause from the runtime-defaults comment. Signed-off-by: xyuzh <xinyzng@gmail.com>
Comment-only: trims the review-cycle rationale essays down to the constraint each comment exists to state, keeping the citations reviewers asked for (Docker capability list, OCI process.capabilities schema, CVE-2022-24769). No code changes. Signed-off-by: xyuzh <xinyzng@gmail.com>
d0e1e9c to
07b0f60
Compare
pcmoritz
left a comment
pcmoritz
left a comment
There was a problem hiding this comment.
This is great, thanks a lot for doing this!
DOCKER_DEFAULT_CAPABILITIES and DEFAULT_PUBLIC_DNS are not documented py:data targets (the API page is autosummary-only), so the :data: roles in the create()/SandboxRuntime.create/Sandbox docstrings emitted 'reference target not found' warnings on the docs build. Plain literals instead. Signed-off-by: xyuzh <xinyzng@gmail.com>
|
@dstrodtman Can you have a look for the docs? |
There was a problem hiding this comment.
thanks @xyuzh LGTM -- left some non-blocking comments
| timeout: Optional[float] = None, | ||
| cwd: Optional[str] = None, | ||
| env: Optional[Dict[str, str]] = None, | ||
| shell: Optional[str] = None, |
There was a problem hiding this comment.
I'm still in favor of naming this entrypoint, but we can revist this in a follow-up
There was a problem hiding this comment.
The trade-off as I see it: shell narrowly describes what it does today (the <shell> -c <command> wrapper for string commands), while entrypoint reads as a general argv prefix — which would also cover non-shell wrappers and be the more future-proof name if we generalize it that way.
There was a problem hiding this comment.
We had a small discussion about this offline, one argument to keep it shell is that sandbox.exec behaves more like subprocess.run where the command can either be a list (in that case it is just executed) or a string (in that case it will be run via a shell), and in the string case, the prefix needs to be a shell, so probably naming it shell is most natural.
| # "public" is host egress plus a generated, host-independent resolv.conf. | ||
| VALID_NETWORK_MODES = ("none", "public", "host", "sandbox") | ||
|
|
||
| # Default resolvers for network="public" (Google and Cloudflare public DNS). |
There was a problem hiding this comment.
is it common to use both Google and Cloudflare, is it copying what Docker does by default?
There was a problem hiding this comment.
Not copying Docker, deliberately: Docker's fallback (used when the host resolv.conf yields no usable nameservers) is Google-only — 8.8.8.8 + 8.8.4.4 (moby daemon/libnetwork/internal/resolvconf/resolvconf.go). Mixing providers follows systemd-resolved instead, whose default fallback list interleaves Cloudflare, Google, and Quad9 (meson_options.txt dns-servers): two resolvers from one provider share a failure domain, so the cross-provider pair keeps DNS working if a single provider has an outage or is blocked at the network edge.
lightning.ai serves its docs as an SPA and returns an HTML page for objects.inv, and recent readthedocs versions redirect there and do the same — so every 'make html' (-W) build fails with 'unknown or unsupported inventory version: invalid inventory header: <!doctype html>'. Pre-existing on master, hit while building this PR's docs. Pin the newest readthedocs version that still serves a real inventory (2.0.9), following the maintenance pattern this mapping block already documents for unreliable hosted inventories. Signed-off-by: xyuzh <xinyzng@gmail.com>
…t images out of the box (#65570) (#65622) ## Description Cherry-pick of #65570 (`7408258f64`) into `releases/2.58.0`. Brings the Ray Sandbox Docker-image support onto the release branch, complementing the sandbox documentation already cherry-picked in #65573 (#65503). All changes are confined to `python/ray/experimental/sandbox/` plus its doc page and `doc/source/conf.py`. ## Verification - Clean cherry-pick (`git cherry-pick -x -s 7408258`), no conflicts; 14 files changed matching the original commit. - The original change passed full CI on master (merged 2026-08-20); release-branch CI on this PR will validate on 2.58.0. Not a duplicate: no existing cherry-pick PR for #65570 targets `releases/2.58.0`. AI assistance (Claude Code) was used to prepare this cherry-pick. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: xyuzh <xinyzng@gmail.com> Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com> Co-authored-by: Xinyu Zhang <60529799+xyuzh@users.noreply.github.com>
Follow-up polish on the "Networking and DNS" section added in #65570. Content is unchanged; every edit is a style-guide fix: - Soft-wrap the new prose, which was hard-wrapped at ~72 columns while the rest of the page uses one source line per paragraph. - Split clauses joined by em dashes, semicolons, and a mid-sentence colon into sentences. - Drop bold used for emphasis in a table cell, and promote the "DNS in locked-down networks" bold lead-in to an H3 so it gets a TOC entry and a linkable anchor. - Restore parallel structure in the _oci_spec_transform_fn list, and move the multi-sentence migration guidance out of the bullet into prose after the list. - Recast the noun-phrase lead-in above the network="public" example as a complete sentence. - Replace hedged "Prefer public" with direct advice, and the passive "Anything beyond that can be configured" with an imperative. Signed-off-by: Douglas Strodtman <douglas@anyscale.com>
## Description Style-only follow-up on the "Networking and DNS" section added in #65570. No technical claim changes, and the section's structure and content are the author's; this is polish against the [Ray documentation style guide](https://docs.ray.io/en/master/ray-contribute/writing-style.html). I was asked for a docs review on #65570 but it merged before I finished, so the feedback is landing here instead of as suggestions on a merged PR. ### What changed The one pattern worth naming, because it repeats: the new prose was hard-wrapped at roughly 72 columns while the rest of the page uses one source line per paragraph. The guide asks for [soft-wrapped prose](https://docs.ray.io/en/master/ray-contribute/writing-style.html#soft-wrap-prose) so diffs stay legible. Beyond that: * **Sentence splits** where an em dash, a semicolon, or a mid-sentence colon was joining clauses. The guide asks us to [restructure rather than punctuate](https://docs.ray.io/en/master/ray-contribute/writing-style.html#sentence-structure). After this the whole page is free of Unicode dashes. * **`### DNS in locked-down networks`** instead of a bold paragraph lead-in. The guide reserves [bold](https://docs.ray.io/en/master/ray-contribute/writing-style.html#bold-and-italics) for UI elements, admonition lead-ins, and definition lists. As a real heading it also picks up a TOC entry and a linkable anchor. A `note` admonition would work too if you'd rather it read as an aside. * **Dropped `— **recommended for internet access**`** from the `public` row's first cell. That's bold for emphasis, and the prose above the table already makes the recommendation. * **Parallel structure in the `_oci_spec_transform_fn` list.** The second bullet had lost the `**Term**: explanation` shape of the first and grown to four sentences, so the migration guidance moves into prose after the list. This also drops "no longer need this hook," which orients a reader who knew the previous behavior rather than one arriving fresh. * **A complete lead-in** above the `network="public"` example, which was a noun phrase with a colon on the end. * **Direct advice and active voice**: "Prefer `public` for untrusted code" to "Use `public`", and "Anything beyond that can be configured" to "Configure anything beyond that." ### One question I couldn't answer myself The `sandbox` row's "Network access" cell reads "gVisor netstack," which says how the mode is implemented but not what the reader gets. Does `network="sandbox"` provide egress, or is it isolated like `none`? That's what someone scanning the table to pick a mode is asking, and it's the one cell that doesn't answer it. I left the cell alone rather than guess. Happy to add a few words here if you tell me which it is. ## Related issues Follows #65570. Related to #64964. ## Additional information `DOCKER_DEFAULT_CAPABILITIES` is in the sandbox package's `__all__`, so naming it in the prose above the example is safe. `myst_heading_anchors = 4`, so the new H3 and the existing `#pass-custom-oci-configurations-to-gvisor` link both resolve. Note that no pre-commit hook covers Markdown under `doc/source/`, and Vale is scoped to the Ray Data docs and the example gallery, so nothing lints this file automatically. Verified by hand: heading depth tops out at H3, no stacked headings, and both in-page anchors match real headings. Signed-off-by: Douglas Strodtman <douglas@anyscale.com>
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>
…#65737) ## Why are these changes needed? `extract_tar_layer` writes regular files with `copyfileobj` and never calls `os.utime`, so every file in an extracted sandbox rootfs carries the extraction time as its mtime instead of the archived one. That breaks in-sandbox tools that trust timestamps. The most visible victim is **apt**: it revalidates `/var/lib/apt/lists` with `If-Modified-Since` derived from the file mtime, so with a reset-to-now mtime the mirror answers `304 Not Modified` for image-baked lists whose Release files expired long ago — and every `apt-get install` then 404s on package versions that no longer exist. Docker-extracted images don't have this problem because layer extraction preserves mtimes. Found while running Terminal-Bench 2.1 oracle evaluations under Harbor against a Ray Sandbox deployment: every TB verifier bootstraps with `apt-get install curl`, and ~30% of the suite failed with this signature (verified in a live sandbox: `rm -rf /var/lib/apt/lists/* && apt-get update` immediately fixes it). Fix: apply `os.utime` from the tar member after writing each regular file; directory mtimes are applied after the extraction loop (extracting children would bump them), best-effort. ## Related issue number Follow-up to #65570. ## Checks - [x] Signed off (DCO); pre-commit hooks pass on the changed files. - [ ] Unit test included: `test_extract_tar_layer_preserves_mtimes` (pure, no runsc needed). --------- Signed-off-by: xyuzh <xinyzng@gmail.com> Signed-off-by: Philipp Moritz <pcmoritz@gmail.com> Co-authored-by: Philipp Moritz <pcmoritz@gmail.com>
## Why are these changes needed? Two filesystem-behavior divergences from Docker, both found running Terminal-Bench 2.1 oracle evaluations in Ray sandboxes (via the Harbor integration): 1. **`localhost` does not resolve.** Container engines inject `/etc/hosts` at run time, so images do not ship a usable one — and the sandbox never provided it either. glibc consults hosts files before DNS, and public resolvers will not answer for `localhost`, so every task that starts a local server and connects to it fails with `Name or service not known` (6 of 89 TB tasks). Fix: generate a per-sandbox hosts file next to the generated `resolv.conf` (seeded from the node's file under host networking) and bind-mount it **read-write**, matching engine behavior — the source is always a per-sandbox copy, never the node's file. 2. **`/tmp` sits on a different device than the rootfs.** runsc mounts a private tmpfs over an *empty* `/tmp`, so the common tempfile-then-`rename(2)` pattern fails with `EXDEV` where it works under Docker (plain rootfs `/tmp`). Fix: seed the extracted rootfs `/tmp` with a placeholder so runsc keeps it on the rootfs; **readonly** sandboxes get an explicit tmpfs mount instead, preserving the always-writable `/tmp` they have today. ## Related issue number Follow-up to #65570, sibling of #65737. ## Checks - [x] Signed off (DCO); pre-commit hooks pass on the changed files. - [ ] Unit tests included (`test_oci_spec_docker_parity_hosts_and_tmp`, `test_prepare_oci_bundle_writes_hosts_file`); both run without gVisor. --------- Signed-off-by: xyuzh <xinyzng@gmail.com>
## Why are these changes needed? Sandbox image pulls are anonymous Docker Hub pulls. A cluster of nodes pulling distinct multi-GB benchmark images concurrently runs straight into Docker Hub's anonymous rate limits and pays WAN latency on every node — during a Terminal-Bench 2.1 evaluation under Harbor, big-image tasks failed in the concurrent sweep but passed in isolation. `RAY_SANDBOX_REGISTRY_MIRROR` names a registry that mirrors Docker Hub, as `host[:port][/repo-prefix]`: - an **ECR pull-through cache** (`<acct>.dkr.ecr.<region>.amazonaws.com/dockerhub`), - an **Artifact Registry remote repository**, or - an in-cluster `registry:2` proxy. Docker Hub pulls are rewritten to the mirror (the prefix prepended to the repository, as ECR requires); other registries pass through untouched. This mirrors Docker's own `registry-mirrors` semantics, minus the fallback: when set, the mirror is authoritative, and it uses the same anonymous token flow as any registry. ## Related issue number Follow-up to #65570; sibling of #65737 and #65744. ## Checks - [x] Signed off (DCO); pre-commit hooks pass on the changed files. - [ ] Unit test included (`test_registry_mirror_rewrites_docker_hub_only`); runs without gVisor or network. --------- Signed-off-by: xyuzh <xinyzng@gmail.com> Signed-off-by: Philipp Moritz <pcmoritz@gmail.com> Co-authored-by: Philipp Moritz <pcmoritz@gmail.com>
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>
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>
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>
5 participants
Description
ray.experimental.sandboxcannot run standard Docker-built images without reaching for the private_oci_spec_transform_fnhook and client-side workarounds. We hit all of these while integrating Ray Sandbox with the Harbor eval framework (see the workarounds carried in harbor-framework/harbor#2725), and the follow-up items while testing this PR end-to-end on a live Ray cluster with real gVisor.Capabilities
SandboxConfig.capabilities(plumbed throughcreate(), theSandboxactor,SandboxRuntime.create, and the gVisor backend): None keeps the runtime default; otherwise the bounding/effective/permitted sets are written exactly, so[]runs with no capabilities at all (inheritable and ambient untouched, matching modern Docker, which stopped setting inheritable for CVE-2022-24769). A newDOCKER_DEFAULT_CAPABILITIESconstant (Docker's documented default set) gives Docker parity in one line: the runtime's default (whateverrunsc specemits) grants only three capabilities, and standard images break confusingly without more —apt-getneedsCAP_SETUID/CAP_SETGIDfor its_aptsub-processes,tarextracting as root fatally fails restoring archived uid/gids (e.g.1000) withoutCAP_CHOWN.Networking and DNS
network="public"— the recommended internet-access mode: host egress (--network=hostunder the hood) with a synthetic/etc/resolv.confgenerated fromdns(default8.8.8.8,1.1.1.1). Nothing about the host resolver is inherited, so the config stays portable and doesn't leak host search domains or internal resolver addresses (a cluster node's resolv.conf typically carries internal Kubernetes search domains, the cluster DNS VIP, andndots:5).network="host"stays the explicit power mode — full host network identity (including internal networks reachable from the node) with the host's own resolv.conf mounted read-only;dns=overrides it with a generated file. Keeping the modes separate lets the docs state that difference plainly."none"remains the default.dns=mirrorsdocker --dnsand is the escape hatch for locked-down VPCs that block public DNS; it's rejected for modes that mount no resolv.conf. Host-side networking also drops the spec's empty network namespace, which previously left the sandbox loopback-only despite--network=host.network="sandbox"withrootless=Trueis rejected at config time: runsc refuses the combination at container start, after the image pull, with an error naming a flag the user never set.Working directory and writability
Writability is now fully explicit — determined by the two parameters the user actually set, with the image's
WORKDIRinherited for the process cwd only:readonlyworkdirTrue(default)None(default)/tmpremain writable)WORKDIR, else/True/data/data(host-backed scratch)/dataFalseWORKDIRPreviously the backend defaulted
workdirto the image'sWORKDIR(or/) and scratch-mounted there, so what was writable depended on image metadata invisible in the config. An inheritedWORKDIRis never silently made writable now. This made the interimmount_workdirknob redundant, so it's removed; the backend also no longer mutatesconfig.workdir(the resolved cwd lives in sandbox metadata), and inherited imageWORKDIRvalues are no longer used in host path joins, removing that traversal surface.TTL
ttl_secondsnow defaults toNone(no TTL) everywhere: the old 1h default was wall-clock from creation, not idle time, so actively-running sandboxes were deleted mid-work — a silent, hard-to-attribute failure in long agent runs. Opt-in destruction is the safer default.SandboxRuntimenow actually enforces a set TTL (daemon timer, canceled on delete); previously the parameter was a documented no-op below the actor layer, and theSandboxactor's duplicate timer is dropped in favor of the runtime's.Shell for string commands
/bin/sh—dashon Debian-family images — while agent-generated commands overwhelmingly assume bash, failing confusingly on[[ ]]/pipefail/arrays.SandboxConfig.shellnow defaults to"/bin/bash"outright (no auto-detection — deterministic per review), with a per-commandshell=override onexec(). Images without bash setshell="/bin/sh"explicitly, as the busybox-based tests now do; list commands still bypass the shell.Robustness
SandboxRuntime.create()forwardsmount_workdirinto the config (it was silently dropped — caught by live testing; a new runtime-layer test suite pins config fields actually reaching the backend, the gap that let it ship green).base_specs with null capability sets or non-dict namespace entries no longer raise; malformed namespace entries are kept for runsc to reject with its own error.SandboxConfigvalidates the network mode against("none", "public", "host", "sandbox").Defaults are behavior-preserving except where called out (TTL, and mount_workdir under
readonly=False); the API is alpha.Related issues
Related to #64964 (Ray Sandboxing with gVisor). Enables removing the OCI-spec transform, DNS seeding, and
tar --no-same-ownerworkarounds in harbor-framework/harbor#2725 / harbor-framework/harbor#2785.Additional information
Usage:
Testing:
base_specwith a stub image manager, config validation, resolv.conf generation policy at theprepare_oci_bundlelayer, and a newSandboxRuntime-layer suite (fake backend) covering field forwarding and TTL enforcement.TEST_SANDBOX=1suite with real gVisor/runsc on Linux: 64 tests pass, including the shell-detection and rootless-rejection paths.dns=escape hatch, and an updated OCI-transform section (its example used the now-rejectednetwork="sandbox"default combination and hand-mounted resolv.conf).This is the first of two changes for running Harbor evals on Ray Sandbox; a follow-up adds an HTTP API service so sandboxes can be consumed as a standalone service.