Repository navigation
Conversation
jtatum
reviewed
Oct 4, 2026
Contributor
There was a problem hiding this comment.
The separate sandbox origin and proxy support are useful. I have one correctness concern with the proxy-disabled configuration, noted inline.
Nonblocking design suggestion: would you consider making the added ports and proxy listener conditional on MCP Apps being enabled? That would avoid changing Pod templates and ingress allowances for instances that do not use this feature. I see the unconditional behavior is documented, so this is a design suggestion rather than a correctness blocker.
Validated locally with OpenClaw 2026.9.4 on Kubernetes 1.36.4.
With spec.storage.fixOwnership unset, the controller reads the instance's Namespace to decide whether init-data-owner is admissible. When that read failed (for example RBAC without the namespaces "get" rule), it kept the init container enabled, which could flip a working StatefulSet in a restricted namespace to a Pod template admission rejects. The read failure now aborts reconciliation before any managed resource is touched, surfaces through the ReconcileFailed event and Ready condition, and is retried. An explicit fixOwnership still bypasses the lookup. Adds envtest coverage: a restricted-compatible instance whose namespace read then fails keeps its StatefulSet unchanged, and explicit values skip the lookup. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
In `watchNamespaces` mode, #618 places `get namespaces` in tenant Roles. Namespace objects are cluster-scoped, so those grants cannot authorize the Pod Security label lookup. The controller then retains the ownership helper default, potentially producing a pod rejected by restricted admission. Move Namespace reads into the chart's existing cluster-scoped rule set and ClusterRoleBinding. In namespaced mode, constrain `resourceNames` to the configured watched namespaces. Cluster-wide mode permits reading any Namespace. Neither mode gains Namespace list/watch/write permissions. This PR targets `fix/data-owner-pss-aware` as a focused follow-up to #618. It can be retargeted/rebased onto main after that PR merges. The separate namespace-lookup failure handling review on #618 remains relevant; this change fixes the chart permission grant. Validation: - A regression test renders the chart, applies its actual RBAC to a disposable envtest API server configured with RBAC, and checks authorization as the rendered operator ServiceAccount identity. - The original #618 template fails the watched-Namespace read assertion. The fixed template permits the two watched namespaces, denies unwatched/operator namespaces, and denies Namespace list/watch/patch. Cluster-wide mode is checked too. - Full `make test`, `make lint`, and both Helm RBAC guard scripts pass. CI installs Helm for the authorization test. --------- Co-authored-by: Jannes Stubbemann <jannes.stubbemann@gmail.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
…en enabled Addresses review on #617: - Pin mcp.apps.sandboxPort to 18795 with the gateway proxy disabled too, and make the Service, NetworkPolicy, container port and nginx upstream follow the effective listener port (a user-supplied port wins). With the proxy disabled the Service used to target 18795 while OpenClaw listened on its default 18790. - Only render the sandbox container ports, nginx listener, Service port and NetworkPolicy ingress when spec.config.raw enables MCP Apps, so instances that do not use it see no change. With configMapRef or mergeMode=merge the operator cannot see the effective config and keeps exposing the sandbox. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts: # api/v1alpha1/openclawinstance_types.go # bundle/manifests/openclaw.rocks_openclawinstances.yaml # charts/openclaw-operator/templates/crds/openclaw.rocks_openclawinstances.yaml # config/crd/bases/openclaw.rocks_openclawinstances.yaml # docs/api-reference.md
#619 made every operator init container that writes pod storage use the gateway's effective UID. Do the same for init-tmp-dir so the sticky /tmp directory belongs to the user that writes to it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…2e-pod-diagnostics
Release Please has failed on every push since the repository moved from paperclipinc to stubbi: vars.RELEASE_BOT_APP_ID and secrets.RELEASE_BOT_PRIVATE_KEY were settings of the old organization, so actions/create-github-app-token aborts with "Input required and not supplied: app-id" before release-please runs. - release-please.yaml and release.yaml mint the App token only when the App is configured, then fall back to RELEASE_PLEASE_TOKEN and finally GITHUB_TOKEN. With GITHUB_TOKEN, tag pushes and release publishing do not trigger other workflows, so release.yaml and operatorhub.yaml are dispatched explicitly (release.yaml gains a workflow_dispatch trigger and only runs on tag refs). - The GHCR namespace for the image and the chart comes from one place, vars.IMAGE_NAMESPACE (default paperclipinc), used by release.yaml and .goreleaser.yaml. release.yaml previously used the repository owner (now stubbi) for signing and the chart while GoReleaser pushed to paperclipinc. A preflight fails the release if the namespace does not match the chart's default image.repository (#536). - GHCR login accepts GHCR_PUSH_TOKEN / GHCR_PUSH_USER, needed to push into a namespace the repository owner does not own. - GoReleaser creates the release in stubbi/openclaw-operator. - make verify-signing accepts both the old and the new workflow identity. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts: # .github/workflows/ci.yaml
This was referenced Oct 11, 2026
Owner
Author
|
Merge order note (maintenance pass, 2026-10-11). The open stubbi PRs are stacked so they apply cleanly in this order: #623 → #627 → #620 → #618 → #617 → #628 → #629. This is position 5 of 7. This branch has #618 (and everything below it) merged in, so until those land the diff here also shows their changes; once they are squash-merged, "Update branch" reduces it to this PR's own changes without conflicts. #623 is at the bottom because the required E2E check cannot pass without its sticky /tmp fix (OpenClaw >= 2026.9.6 crash-loops otherwise). Branch protection requires an up-to-date branch, so after each merge click "Update branch" on the next PR and let CI re-run before merging it. |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
Thanks—the proxy-disabled inline-config case from my earlier review is fixed. I found two remaining P2 cases where the sandbox listener and proxy disagree:
Please resolve the listener and proxy ports consistently for these supported config modes, with regression coverage for PVC-only MCP Apps settings and JSON5 custom ports. I reproduced these through the resource builders and generated merge script; this was not a live gateway connectivity test. |
Contributor
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. If this PR is still relevant, please push updates or leave a comment. |
Contributor
|
Not stale |
stubbi
added a commit
that referenced
this pull request
Oct 25, 2026
## Problem The required E2E check is red on `main` and on every same-repo PR: the `Chromium Full Integration Tests` `BeforeAll` times out because the `openclaw` container never becomes ready. The diagnostics from #620 show why. The container exits with code 1 and goes into CrashLoopBackOff, and its log says: ``` Doctor could not complete maintenance. Check the reported service state and resolve the failure. temp workspace ancestor is group/world writable without sticky protection ``` The main container has a read-only root filesystem, so its `/tmp` is an emptyDir. With `fsGroup` set, the kubelet gives the root of an emptyDir mode 2777: world-writable, setgid, no sticky bit. OpenClaw 2026.9.6 and later refuse to create a temporary workspace below such a directory. The startup doctor needs that workspace whenever it has to install a configured plugin. This is the same check #616 fixed for `init-plugins`. That container could fall back to the image's own `/tmp` because its root filesystem is writable; the main container cannot. Only the Full Integration suite hits it because it is the only one that passes `OPENROUTER_API_KEY`, which makes OpenClaw install its Perplexity plugin at startup. Fork PRs skip that suite (no secret), and the smoke test has no key, so both stay green. Real instances are affected whenever the doctor has to install a plugin at startup. ## Change - New init container `init-tmp-dir` creates a subdirectory in the `tmp` emptyDir and sets it to mode 1777. - The main container mounts that subdirectory at `/tmp` via `subPath`, which hides the 2777 volume root from its path ancestry. - The init container runs as the pod UID with all capabilities dropped and a read-only root filesystem, so it is admitted under the restricted Pod Security Standard. It is always rendered, including when `storage.fixOwnership` is false. - `init-tmp-dir` is added to the reserved init container names in the webhook. Nothing in the pod spec can set the mode of a volume root, and the pod UID does not own it, so it cannot chmod the root itself. A directory it creates inside the volume is its own. ## Behavior change to note Every instance gets one more init container, so pods roll on operator upgrade. ## Testing - `make test` passes locally (unit + envtest). New unit tests in `internal/resources/tmp_dir_test.go`, new e2e spec `test/e2e/e2e_tmp_dir_test.go`. - Reproduced outside the cluster with `ghcr.io/openclaw/openclaw:latest` (2026.9.8) under Docker, using the operator's rendered config, volume layout, read-only root and dropped capabilities: - `/tmp` as a 2777 root-owned directory with `OPENROUTER_API_KEY` set: exits 1 with the message above. - Same, without the key: boots normally, which matches the smoke test passing. - The generated init script run as 1000:1000 against a 2777 root:1000 volume produces a 1777 directory owned by 1000, and is idempotent. With that directory mounted at `/tmp` and the key set, the gateway reaches `readyz 200`. - Not run on a real cluster before this PR; this PR's own E2E run is the first. I could not run golangci-lint locally (it runs out of memory in my environment), so Lint is covered by CI only. ## Follow-ups - `init-pip` still mounts the root of the same emptyDir at `/tmp`. It does not run the doctor, so it is left unchanged here. - #617, #618 and #620 need "Update branch" after this lands before their E2E can pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_011HrjPjDpPgTdCFicDWQpvH --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. If this PR is still relevant, please push updates or leave a comment. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #615.
Problem
OpenClaw serves MCP Apps and script-enabled dashboard widgets from a separate sandbox listener. Two things made it unusable on operator-managed instances:
GatewayProxyPort, which the nginx gateway proxy sidecar already holds on0.0.0.0, so the listener fails withAddress already in use.Change
The sandbox is now handled like the gateway and canvas ports, but only for instances that use it:
McpAppsSandboxPort(18795, loopback listener) andMcpAppsSandboxProxyPort(18796, nginx).mcp.apps.sandboxPortto 18795 whenmcp.apps.enabledis true and no port is set, with or without the gateway proxy. A user-supplied port wins. Configs withoutmcp.apps.enabled: trueare not touched, so OpenClaw versions that predate MCP Apps never see the key.ResolveMcpAppsSandboxdecides whether the sandbox is exposed and on which listener port. It is exposed whenspec.config.rawenables MCP Apps, and always when the operator cannot see the effective config (spec.config.configMapRef, ormergeMode: mergewhere runtime changes on the PVC can enable it).mcp-apps(18795) targeting the proxy port, or the listener port directly whenspec.gateway.enabled: false; the NetworkPolicy allows ingress to the same target under the same rules as gateway and canvas.spec.networking.service.ports.No CRD changes.
Behavior change to note
Instances whose inline config enables MCP Apps gain one Service port, one NetworkPolicy ingress port, container ports and an nginx listener. Instances using
configMapReformergeMode: mergegain them unconditionally, because the operator cannot tell whether MCP Apps is on. Other instances see no change.Review follow-up (a903cf7): with the gateway proxy disabled the Service previously targeted 18795 while OpenClaw listened on its default 18790; the port is now pinned in that mode too and the Service follows a user-supplied port.
Testing
make testpasses locally (unit + envtest).internal/resources/mcp_apps_test.gocover port collisions, config enrichment (enabled, disabled, user override, proxy disabled), exposure resolution (raw, configMapRef, merge), the nginx upstream (default, user port, configMapRef port), and container ports, Service and NetworkPolicy with MCP Apps enabled, disabled, proxy disabled and a user port.test/e2e/e2e_mcp_apps_test.gochecks the rendered ConfigMap and Service on a real cluster for the default case, the proxy-disabled case with a user port, and MCP Apps disabled.🤖 Generated with Claude Code
https://claude.ai/code/session_011HrjPjDpPgTdCFicDWQpvH