Repository navigation
Conversation
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
|
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>
|
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. |
|
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. |
|
Not stale |
This branch has not been deployed
2 participants
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