fix(helm): default sandbox Services to ClusterIP (#3929) by zhfeng · Pull Request #4190 · bytedance/deer-flow

zhfeng · 2026-07-15T02:52:41Z

Summary

Closes #3929.

The K8s sandbox provisioner exposes each sandbox Pod behind a per-sandbox Service. #4016 added SANDBOX_SERVICE_TYPE so the provisioner supports both NodePort and ClusterIP - but the Helm chart never set it, so real-cluster installs inherited the NodePort default. That:

Binds the code-execution sandbox on every node's interfaces, including externally-reachable ones (GKE/EKS/AKS open NodePorts on the node's external IP by default). Anyone who can reach any node IP on the auto-allocated 30xxx port talks to the sandbox HTTP API directly, bypassing the gateway.
Pins every sandbox URL to one node IP (status.hostIP / provisioner.nodeHost) - a SPOF when that node reboots, drains, or gets a new ephemeral IP.
Couples to environment-dependent pod->node-IP reachability (CNI / NetworkPolicy / hairpin quirks).

This flips the chart default to ClusterIP. The provisioner returns a cluster-DNS URL (http://sandbox-<id>-svc.<ns>.svc.cluster.local:8080) so the gateway->sandbox hop stays inside the cluster network - no node IP, no 30xxx port, no external exposure. The chart always runs the gateway in-cluster, so ClusterIP is always correct there.

Why only the chart default (not the provisioner code)

The provisioner code default stays NodePort for the Docker-Compose/hybrid dev path (docker-compose-dev.yaml, docker-compose.yaml), where the gateway is a compose container - not in K8s - and reaches sandbox Pods on the host's Docker Desktop K8s via host.docker.internal:NodePort. That gateway uses Docker DNS, not CoreDNS, so .svc.cluster.local does not resolve -> NodePort + NODE_HOST is genuinely required there. The chart path (gateway always in-cluster) is the one that should default to ClusterIP.

This is the one deliberate divergence from the issue's literal "drop NODE_HOST" suggestion; nodeHost is kept as the NodePort-mode opt-in knob.

Changes

File	Change
`values.yaml`	New `provisioner.sandboxServiceType: "ClusterIP"` (default); `nodeHost` comment scoped to NodePort mode
`provisioner-deployment.yaml`	Emits `SANDBOX_SERVICE_TYPE` from the value; `NODE_HOST` block gated on `== "NodePort"` (`default "ClusterIP"` for upgrade safety so a stale values file can't send an empty string that the provisioner rejects)
`NOTES.txt`	Mode-conditional reachability text - ClusterIP shows the cluster-DNS URL; NodePort shows the `{NODE_HOST}:{NodePort}` flow + a nudge back to ClusterIP
`README.md`	"Sandbox NodePort reachability" -> "Sandbox Service type", documenting ClusterIP default + NodePort opt-in

No change to docker/provisioner/app.py (already mode-aware since #4016) or RBAC (the Role already grants services get/list/watch/create/delete, which covers ClusterIP - NodePort needs no extra RBAC either, it's a Service type).

Verification

helm lint deploy/helm/deer-flow -> 0 failures
helm template default render -> SANDBOX_SERVICE_TYPE=ClusterIP emitted, no NODE_HOST env; NOTES shows the cluster-DNS URL with no exposure warning
helm template --set provisioner.sandboxServiceType=NodePort -> SANDBOX_SERVICE_TYPE=NodePort + NODE_HOST emitted (downward-API status.hostIP when nodeHost empty; literal IP when set)
--set provisioner.nodeHost=192.168.1.10 -> rendered as the literal value
Live cluster: create a sandbox -> provisioner returns http://sandbox-<id>-svc.deer-flow.svc.cluster.local:8080; curl from the gateway pod -> 200 from /v1/sandbox; 30xxx port no longer reachable on any node. (No cluster in this environment - left for the reviewer/maintainer.)

Out of scope

The RWO->RWX (persistence.home.accessMode) multi-node sandbox scheduling blocker - orthogonal to the Service type, called out in NOTES/README.
Broader production-hardening batch (security contexts, RBAC narrowing to sandbox Pods, startupProbe).

🤖 Generated with Claude Code

The K8s sandbox provisioner supports both NodePort and ClusterIP via SANDBOX_SERVICE_TYPE (added in bytedance#4016), but the Helm chart never set it, so real-cluster installs inherited the NodePort default. That bound the code-execution sandbox on every node's interfaces - including externally reachable ones on GKE/EKS/AKS - and pinned every sandbox URL to one node IP (SPOF on node reboot/drain/ephemeral-IP). Default the chart to ClusterIP: the provisioner returns a cluster-DNS URL (http://sandbox-<id>-svc.<ns>.svc.cluster.local:8080) so the gateway-> sandbox hop stays inside the cluster network - no node IP, no 30xxx port, no external exposure. The chart always runs the gateway in-cluster, so ClusterIP is always correct there. NodePort remains an opt-in (provisioner.sandboxServiceType: NodePort + nodeHost) for the Docker-Compose/hybrid path where the gateway is not in K8s and cannot resolve .svc.cluster.local; the provisioner code default stays NodePort for that path. - values.yaml: add provisioner.sandboxServiceType ("ClusterIP") - provisioner-deployment.yaml: emit SANDBOX_SERVICE_TYPE; gate the NODE_HOST block on NodePort mode (default "ClusterIP" for upgrade safety) - NOTES.txt + README.md: document ClusterIP default + NodePort opt-in No change to docker/provisioner/app.py (already mode-aware since bytedance#4016) or RBAC (services verbs already cover ClusterIP).

willem-bd

Review summary

Approve, pending the live-cluster smoke (inline #1). A focused, well-reasoned security hardening: the right default flip on the right surface (the sandbox code-execution API was bound on every node's interfaces via NodePort), with a documented NodePort escape hatch for the compose/hybrid path. The provisioner (docker/provisioner/app.py) is unchanged - mode-aware since #4016.

Verified by code reading (commit 008bc91):

Provisioner consumes SANDBOX_SERVICE_TYPE (default NodePort, validated at app.py:76-77); ClusterIP mode returns http://sandbox-<id>-svc.<K8S_NAMESPACE>.svc.cluster.local:<SANDBOX_CONTAINER_PORT> (app.py:248-249) and creates the Service in K8S_NAMESPACE with port = target_port = SANDBOX_CONTAINER_PORT (app.py:496-525) - so the DNS URL's namespace/port match.
Chart wiring consistent: K8S_NAMESPACE = deer-flow.namespace, SANDBOX_CONTAINER_PORT = sandboxPort (8080, already wired), SANDBOX_SERVICE_TYPE = sandboxServiceType. Helpers/values all exist.
default "ClusterIP" applied consistently in both the value: emit and the if eq guard, so a stale/empty values.yaml can't produce an empty string the provisioner rejects. Upgrade-safety intent confirmed.
No stray NODE_HOST emitters; sandbox Services are provisioner-created (no static chart template). Provisioner test sets the type explicitly per case, so it's unaffected.

Findings: #1 (medium) live-cluster behavior unverified by author and review; #2 (low) no chart-render test for the gating; #4 (nit) no CHANGELOG / upgrade callout for the default flip.

Label note (no code anchor): the PR is labeled area:docs, but it mutates values.yaml and provisioner-deployment.yaml - a runtime default flip, not docs-only. risk:medium is right; area:docs understates it for triage.

🤖 Generated with Claude Code

willem-bd · 2026-07-15T03:46:03Z

+  #    in-cluster gateway and avoids binding the code-execution surface on
+  #    every node's interfaces. `NodePort` is the Docker-Compose/hybrid escape
+  #    hatch (used when the gateway is not in K8s); pair with `nodeHost`.
+  sandboxServiceType: "ClusterIP"


🟡 Live-cluster behavior is unverified - by both the author and this review

This default flip (NodePort -> ClusterIP) is the core of the change, and neither side has confirmed it works in a real cluster: the PR body's own checklist item 4 (create a sandbox, curl from the gateway pod, confirm the 30xxx port is no longer reachable) is explicitly unchecked ("No cluster in this environment"). I couldn't render either (helm not available here).

The wiring is correct by inspection - the provisioner (app.py:248-249) returns http://sandbox-<id>-svc.<K8S_NAMESPACE>.svc.cluster.local:<SANDBOX_CONTAINER_PORT> and creates the Service in K8S_NAMESPACE with port = target_port = SANDBOX_CONTAINER_PORT (app.py:496-525), so the DNS URL's namespace and port match - and the provisioner has been mode-aware since #4016. So confidence is reasonable. But for a default-flip on a code-execution exposure surface, recommend a maintainer run the unchecked live checklist (or at minimum helm template + a kubectl run ... curl smoke) before merge.

willem-bd · 2026-07-15T03:46:03Z

+            # escape hatch (gateway not in K8s), which needs NODE_HOST below.
+            - name: SANDBOX_SERVICE_TYPE
+              value: {{ .Values.provisioner.sandboxServiceType | default "ClusterIP" | quote }}
+            {{- if eq (default "ClusterIP" .Values.provisioner.sandboxServiceType) "NodePort" }}


🟢 No automated chart-render test for this gating

There's no helm template-based test asserting that the chart emits SANDBOX_SERVICE_TYPE=ClusterIP by default, omits NODE_HOST by default, and emits both under --set provisioner.sandboxServiceType=NodePort. The repo has provisioner tests (test_provisioner_request_threading.py) but no chart-render tests, so this relies on the author's manual helm lint / helm template. A small render test would lock in this gating and catch a future regression (e.g., someone re-adding unconditional NODE_HOST). Coverage gap, not a defect.

willem-bd · 2026-07-15T03:46:03Z

-
-Override `provisioner.nodeHost` only if your CNI or network policy blocks
-pod->node-IP traffic:
+## Sandbox Service type


🟢 Consider an upgrade callout / CHANGELOG entry for the default flip

This is a user-facing default change: existing chart installs flip NodePort -> ClusterIP on upgrade. The README documents the new default but doesn't call out the upgrade behavior for operators who may have relied on NodePort reachability (e.g., an external probe hitting the 30xxx port). AGENTS.md's doc-sync policy points at CHANGELOG.md for changes - a one-line entry noting the flip + the sandboxServiceType: NodePort opt-back-in would help. Defensible to ship silently (it's a security improvement), but an explicit note is friendlier.

…t flip (bytedance#3929) Address review on bytedance#4190: - Add scripts/check_chart_sandbox_service.sh: renders the chart for the default (ClusterIP, no NODE_HOST), the NodePort opt-in (both emitted), and NodePort+nodeHost (literal value, not downward API). Locks in the bytedance#3929 gating so a regression (e.g. re-adding an unconditional NODE_HOST, or dropping the `default "ClusterIP"` upgrade-safety fallback) fails CI. Wired into .github/workflows/chart.yaml validate-chart job. (bytedance#2) - CHANGELOG [Unreleased] -> Changed: note the NodePort->ClusterIP default flip on upgrade + the `sandboxServiceType: NodePort` opt-back-in. (bytedance#4) No chart template changes (the gating itself landed in the first commit).

zhfeng · 2026-07-16T04:50:19Z

Thanks @willem-bd for the thorough review — #2, #4, and the label note are addressed in 93850e67. Detail below.

#2 — chart-render test for the gating ✅
Added scripts/check_chart_sandbox_service.sh (modeled on check_config_version.sh) and wired it into the validate-chart job (.github/workflows/chart.yaml). It renders the chart three ways and asserts:

default → SANDBOX_SERVICE_TYPE=ClusterIP and no NODE_HOST
--set provisioner.sandboxServiceType=NodePort → SANDBOX_SERVICE_TYPE=NodePort + NODE_HOST
+ --set provisioner.nodeHost=192.168.1.10 → NODE_HOST="192.168.1.10" (literal, not the downward API)

It greps the env-var list item (- name: NODE_HOST), not the comments that merely mention the name, so the "NODE_HOST absent" assertion is meaningful. Passes locally 7/7; CI runs it on this PR. A future regression (e.g. re-adding an unconditional NODE_HOST, or dropping the default "ClusterIP" upgrade-safety fallback) will fail here.

#4 — upgrade callout / CHANGELOG ✅
Added a [Unreleased] → ### Changed entry noting the NodePort→ClusterIP default flip on upgrade and the provisioner.sandboxServiceType: NodePort opt-back-in (for an external probe that relied on the 30xxx port, or the compose/hybrid path where the gateway isn't in K8s).

Label note ✅
Removed area:docs.

#1 — live-cluster smoke
The render test above also covers the helm template minimum bar you cited for the author. For the live A/B: I have a docker-desktop deployment of the chart running locally (provisioner pod up, currently on the NodePort code default — no SANDBOX_SERVICE_TYPE env, i.e. the pre-PR state; no existing sandbox Services). The steps are:

kubectl set env deploy/...-provisioner SANDBOX_SERVICE_TYPE=ClusterIP → rollout
POST /api/sandboxes → assert the returned URL is http://sandbox-<id>-svc.deer-flow.svc.cluster.local:8080 and kubectl get svc sandbox-<id>-svc shows TYPE=ClusterIP with no node port
curl /v1/sandbox from a gateway pod → 200
DELETE /api/sandboxes/<id>, then unset the env to restore the NodePort default

I'm running this now and will post the results here; if a maintainer prefers to run it themselves (as you suggested), the steps above are self-contained.

🤖 Generated with Claude Code

zhfeng · 2026-07-16T07:27:56Z

#1 - live-cluster smoke: done. Full chain verified end-to-end on a docker-desktop cluster.

Setup: the docker-desktop deployment's deer-flow-provisioner:latest image predated #4016, so the old binary ignored SANDBOX_SERVICE_TYPE. I rebuilt the image from current main and loaded it into the node's containerd (docker save … | docker exec -i desktop-control-plane ctr -n k8s.io images import -), then rollout restart — imagePullPolicy is IfNotPresent, so the running pod won't re-resolve :latest on its own.

A/B on the same cluster — same provisioner, only SANDBOX_SERVICE_TYPE differs:

	Service	TYPE	NODE-PORT
`SANDBOX_SERVICE_TYPE=ClusterIP` (chart default)	`sandbox-cip-smoke-svc`	ClusterIP	<none>
unset (NodePort code default)	`sandbox-pr4190-smoke-svc`	NodePort	31143

The new binary creates a ClusterIP Service with no nodePort field at all — the code-execution surface is no longer bound on every node's interfaces. The URL it returns is the cluster-DNS form: http://sandbox-cip-smoke-svc.deer-flow.svc.cluster.local:8080.

Reachability (curl from an in-cluster pod):

HTTP 200 via http://sandbox-cip-smoke-svc.deer-flow.svc.cluster.local:8080/v1/sandbox

So the gateway→sandbox hop stays inside the cluster network — no node IP, no 30xxx port, no external exposure. This matches the ClusterIP case in backend/tests/test_provisioner_request_threading.py (asserts service.spec.type == "ClusterIP" + the DNS URL form), now confirmed live.

Dev-env note (not PR-related): the sandbox Pod initially stuck in ContainerCreating on hostPath /skills/public is not a directory — the new 3-way skills layout (app.py _build_volumes) mounts /skills/public as a strict Directory, which the docker-desktop node didn't have. Creating the dir unblocked it; this is a chart/provisioner hostPath setup gap independent of #3929.

Test artifacts cleaned up; provisioner env restored to the NodePort code default.

🤖 Generated with Claude Code

github-actions Bot added area:docs Documentation and Markdown only risk:medium Medium risk: regular code changes size/M PR changes 100-300 lines labels Jul 15, 2026

willem-bd reviewed Jul 15, 2026

View reviewed changes

github-actions Bot added area:ci GitHub Actions, CI config, repo tooling risk:high High risk: backend API, agents, sandbox, auth, deps, CI and removed risk:medium Medium risk: regular code changes labels Jul 16, 2026

zhfeng removed the area:docs Documentation and Markdown only label Jul 16, 2026

Merge branch 'main' into fix/sandbox-clusterip-service-3929

92a296d

github-actions Bot added the area:docs Documentation and Markdown only label Jul 16, 2026

WillemJiang approved these changes Jul 16, 2026

View reviewed changes

WillemJiang merged commit d57f695 into bytedance:main Jul 16, 2026
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(helm): default sandbox Services to ClusterIP (#3929)#4190

fix(helm): default sandbox Services to ClusterIP (#3929)#4190
WillemJiang merged 3 commits into
bytedance:mainfrom
zhfeng:fix/sandbox-clusterip-service-3929

zhfeng commented Jul 15, 2026

Uh oh!

willem-bd left a comment

Uh oh!

willem-bd Jul 15, 2026

Uh oh!

willem-bd Jul 15, 2026

Uh oh!

willem-bd Jul 15, 2026

Uh oh!

zhfeng commented Jul 16, 2026

Uh oh!

Uh oh!

zhfeng commented Jul 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

zhfeng commented Jul 15, 2026

Summary

Why only the chart default (not the provisioner code)

Changes

Verification

Out of scope

Uh oh!

willem-bd left a comment

Choose a reason for hiding this comment

Review summary

Uh oh!

willem-bd Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

willem-bd Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

willem-bd Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

zhfeng commented Jul 16, 2026

Uh oh!

Uh oh!

zhfeng commented Jul 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants