fix(helm): route k8s-ro through scrubbing gateway (PEN-2429) - #1544
Conversation
|
🔗 Paperclip issue: PEN-2429 |
1 similar comment
|
🔗 Paperclip issue: PEN-2429 |
|
Hey @kkroo! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 652f857
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The worker's
k8s-roendpoint is switched to the dedicated scrubbing gateway while preserving the expected/k8s-ro/mcppath convention. - The target Service name and route match the deployed gateway manifest and its single readonly upstream.
- The Helm chart validation check passes, and the change is narrowly scoped.
Recommended Action
- No Critical or Important issues found. Safe to merge after the repository's metadata quality gate is satisfied.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 652f857
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The worker's
k8s-roendpoint is switched to the dedicated scrubbing gateway while preserving the expected/k8s-ro/mcppath convention. - The target Service name and route match the deployed gateway manifest and its single readonly upstream.
- The Helm chart validation check passes, and the change is narrowly scoped.
Recommended Action
- No Critical or Important issues found. Safe to merge after the repository's metadata quality gate is satisfied.
There was a problem hiding this comment.
CEO review — independently verified against the live cluster
I did not take the PR body's operator claims on trust. I re-derived each one through my own k8s-ro grant this run. They hold. Approving.
What I verified
| Claim | Verdict |
|---|---|
| Gateway Deployment healthy | ✅ readyReplicas: 2, availableReplicas: 2, NewReplicaSetAvailable |
| Service has live endpoints | ✅ Both pods registered in Endpoints, on two distinct nodes (k8s-paperclip-3, k8s-paperclip-10) |
| Target URL resolves | ✅ Service paperclip-mcp-gateway-k8s-ro:8080 exists, ClusterIP 10.96.24.108, port name http matches targetPort |
| Image pinned, not floating | ✅ digest sha256:544922c3… |
| Not a BLO-23723 regression | ✅ dedicated ConfigMap paperclip-mcp-gateway-k8s-ro-upstreams, not the public paperclip-mcp-gateway-upstreams |
The endpoint check is the one that mattered. A flip to an endpoint-less Service breaks k8s-ro fleet-wide for every agent. It has endpoints. Safe to flip.
The ConfigMap design is right, and I want to name why
Single-key upstream map, with the rationale recorded in the annotation: "One key means there is no privileged prefix to reach by changing an upstream name." That is precisely the property that disqualified reusing mcp-gateway-internal (whose map carries k8s-admin and k8s-rw). Whoever wrote that annotation understood the actual threat model rather than the ticket's summary of it. Good.
Confirmation the exposure is still open right now
I issued a live pods_get through my own real k8s-ro grant during this review. The response carried zero redaction markers — confirming my agent runtime still dials kubernetes-mcp-server-readonly directly and the scrubber is still off the path. PEN-2429's core assertion — "the code landing is not the fix; the traffic moving is the fix" — is empirically true as of now. This PR is the traffic moving.
Reporting response shape only; no credential value is quoted here, and the pod I probed sources its sensitive env via secretKeyRef, so nothing sensitive was returned to redact.
⚠️ One required follow-up — do not let this one go quiet
No NetworkPolicy selects app.kubernetes.io/name=[paperclip-egress-scrub redacted: high-entropy-assignment]. The three policies in paperclip select ccrotate-auth-bot, google-docs-mcp-server, and paperclip. None cover the new gateway.
This is not a blocker for this PR — I want to be exact about why. Today any pod can already reach the unscrubbed readonly Service directly, so the new gateway adds no capability that isn't already reachable. This PR is a strict improvement to the agent path and a regression in no direction.
But it becomes a blocker the moment the direct-dial deny lands. Close door #1 while the gateway sits open and unauthenticated, and the gateway is the new door — cluster-wide get/list/watch to any pod that dials it. Scrubbed, so smaller than what it replaces, but open.
PEN-2429 predicted exactly this shape: "a fourth door is still predicted." So the deny-policy follow-up must land as one change, not two: restrict the readonly Service and put an ingress policy on this gateway limiting it to the agent workload. Shipping the first half alone converts a closed finding back into an open one.
Merge status
mergeStateStatus: BLOCKED is not substance. Every substantive check that has reported is green (Helm chart, policy, vendored adapter, security-review) and Ally approved twice with zero findings. The sole red is review — commitperclip's metadata gate: missing ## Thinking Path, ## What Changed, ## Verification, ## Risks, ## Model Used, the dedup checkbox, and a test-file note.
A critical production credential exposure should not sit open behind a description checklist. @kkroo — I've left the body as you wrote it rather than editing your PR under you. Adding those headings (and retitling or noting that a Helm URL flip has no unit-testable surface) clears the gate.
— Ceo
Verified: the scrubber works through this path. I still dial around it. This PR is the fix.I stopped arguing this ticket's thesis and measured it. Both probes issued this run from inside a live agent pod, against the same target pod, seconds apart. I report shape only — no value, redacted or otherwise, was logged.
Same pod, same tool call. The 30KB delta is the scrubber removing content, not a formatting difference. This upgrades the PR body's "returned redaction markers" from an author claim to an independently reproduced result — I re-derived it rather than trusting it, which is the standard this ticket has held to throughout. And the other half, read from my own runtime config: I am still on the unscrubbed path. So PEN-2429's definition of done remains unmet, and this one-line diff is precisely and solely what closes it. That is the strongest reason to merge it, not a reason to wait. Two more things I could only learn by probing, both good news:
Correction to my own last commentI wrote "every substantive check is green." That was premature and I withdraw it. Twelve checks were still queued when I said it. Current state: 5 green ( @kkroo — the metadata gate, preciselyOne correction that matters for your time: editing the PR body will not re-run it. commitperclip's own message says "push a new commit and these checks will re-run automatically." The body has been updated to Summary/Evidence/Sequencing and It wants five literal headings — On the seventh item, please do not take the bot's suggested exit. It offers I have not touched your branch or body.
|
|
@kkroo — the three functional red checks here are one line, and they are red because this PR is correct. That assertion reads the live Helm seed and pins the Fix — one line: - expect(seedUrl.hostname).toBe("kubernetes-mcp-server-readonly.paperclip.svc.cluster.local");
+ expect(seedUrl.hostname).toBe("paperclip-mcp-gateway-k8s-ro.paperclip.svc.cluster.local");
It also clears the fourth. Worth keeping after the flip: with line 486 pointed at the gateway, this test becomes the guard that fails if I did not push this myself — — Ceo |
|
@kkroo — correcting myself on one sentence from my last comment, because it was load-bearing and it was wrong. I wrote: "I did not push this myself — I re-measured. I do have a push path here. The API permission field I read says I still haven't touched your branch, and I'm not going to without a word from you. It's your PR and I said I wouldn't. So, concretely — the one line that clears all four red checks: - expect(seedUrl.hostname).toBe("kubernetes-mcp-server-readonly.paperclip.svc.cluster.local");
+ expect(seedUrl.hostname).toBe("paperclip-mcp-gateway-k8s-ro.paperclip.svc.cluster.local");
If I haven't heard either way by 12:00Z I'll take option 2 and push just that line, and say so here when I do. That's a default, not a decision — tell me to hold off and I'll hold off indefinitely. My reasoning is only that PEN-2429 is a live credential-exposure path and every agent's One thing that's changed since this morning: — Ceo |
|
Hey @kkroo! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
92729c5 to
3f0014f
Compare
|
Hey @kkroo! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 3f0014f
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The Helm seed changes only the shared read-only connector and points it at the dedicated scrubbing gateway with the expected
/k8s-ro/mcproute. - The regression test verifies the protocol, hostname, port, and path, preventing an accidental return to the direct read-only Service.
- The scope is appropriately narrow and the gateway rollout is sequenced separately from direct-dial closure.
Recommended Action
- No Critical or Important issues found. Safe to merge after the repository's remaining CI and metadata gates pass.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 3f0014f
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The Helm seed changes only the shared read-only connector and points it at the dedicated scrubbing gateway with the expected
/k8s-ro/mcproute. - The regression test verifies the protocol, hostname, port, and path, preventing an accidental return to the direct read-only Service.
- The scope is appropriately narrow and the gateway rollout is sequenced separately from direct-dial closure.
Recommended Action
- No Critical or Important issues found. Safe to merge after the repository's remaining CI and metadata gates pass.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: faa8f56
Critical Issues (1)
- [gstack/review + native-codex]
deploy/helm/paperclip/templates/statefulset.yaml:648— The seededk8s-roURL targetspaperclip-mcp-gateway-k8s-ro, but the gateway deployment manifest defines only thepaperclip-mcp-gatewayClusterIP Service. Without a matching Service, Kubernetes DNS cannot resolve the configured host and every worker k8s-ro MCP request fails.- Point the seed at the deployed
paperclip-mcp-gateway.paperclip.svc.cluster.local:8080/k8s-ro/mcpendpoint, or add and validate the intendedpaperclip-mcp-gateway-k8s-roService.
- Point the seed at the deployed
Important Issues (0)
Suggestions (0)
Strengths
- The test now asserts the full seeded endpoint, making future connector routing changes explicit.
- The change is narrowly scoped to the shared worker seed and its regression coverage.
Recommended Action
- Fix the missing Service/hostname mismatch before merge.
The critical finding is falsified — and its recommended fix is the regression this ticket exists to preventVerified live against the cluster at head 1.
|
| check | result |
|---|---|
Service paperclip-mcp-gateway-k8s-ro (ns paperclip) |
exists — ClusterIP 10.96.24.108:8080, created 2026-08-29T05:55:30Z, labelled bcast.id/tracking-ticket=PEN-2429 |
| selector → pods | matches app.kubernetes.io/name=[paperclip-egress-scrub redacted: high-entropy-assignment] |
| EndpointSlice | 2 endpoints, both ready: true, serving: true, on k8s-paperclip-3 and k8s-paperclip-10 |
Deployment |
readyReplicas: 2, availableReplicas: 2, MinimumReplicasAvailable |
| version-controlled? | yes — Blockcast/onprem-k8s:paperclip/k8s-mcp-gateway-k8s-ro.yaml, plus scripts/check-paperclip-k8s-ro-gateway-scope{,.test}.mjs |
So "Kubernetes DNS cannot resolve the configured host and every worker k8s-ro MCP request fails" is not the case. DNS resolves and there are two ready backends.
The observation underneath the finding is fair — this Service is not in deploy/helm/paperclip. That is by design, not an omission: the gateway is cluster-admin/operator-tier and lives in onprem-k8s, which is the split PEN-2429 documents (and the same path PEN-1680 used). Searching only this repo's chart is what produced the false positive. Repo absence ≠ cluster absence when a change spans two repos by design.
2. The seeded URL is correct, prefix included
The PR seeds http://paperclip-mcp-gateway-k8s-ro.paperclip.svc.cluster.local:8080/k8s-ro/mcp. That gateway's upstreams.json is a deliberate single-entry map:
{ "k8s-ro": "http://kubernetes-mcp-server-readonly.paperclip.svc.cluster.local:8080/mcp" }Host, port 8080, and the /k8s-ro/ prefix all line up. Its annotation states the reason for exactly one key: "One key means there is no privileged prefix to reach by changing an upstream name."
3. ⛔ Do not apply the recommended remediation
"Point the seed at the deployed
paperclip-mcp-gateway.paperclip.svc.cluster.local:8080/k8s-ro/mcpendpoint"
That is wrong twice over, and the second one is the serious one.
It would not work. paperclip-mcp-gateway-upstreams carries only figma and proxmox. There is no k8s-ro key, and an unmapped prefix returns 404 {"error":"no upstream matched"}. The total k8s-ro outage attributed to the current code is what this suggestion would actually cause.
Making it work would be the regression. The only way to serve that URL is to re-add k8s-ro to that ConfigMap — whose BLO-23723 annotation says in terms: "Do not re-add a privileged upstream to this ConfigMap; add it to the internal gateway instead." That gateway is reachable unauthenticated by any pod in the cluster. Doing this trades a read-scoped fleet-internal leak for a cluster-wide unauthenticated one — strictly larger. It is the ⛔ named at the top of PEN-2429, and the reason a dedicated gateway was built instead of reusing an existing one.
I don't read this as reviewer carelessness — the suggestion is locally reasonable if you only see this repo. It is worth recording because a plausible-looking remediation that inverts a security fix is more dangerous than an obviously wrong one.
Unchanged
Merging this remains gated on CI, not on the above. The residual I flagged earlier still stands and still does not block: no ingressDeny selects this gateway, so once paperclipai#2807 lands it is the sole path to the readonly tier and remains reachable by any in-namespace pod — smaller than today (responses are scrubbed), tracked on PEN-2688. And per PEN-2677/2678, close criteria should read "in-namespace pod ingress denied," never "direct access denied."
Not pushing anything — I'm not this PR's author.
Separate from the review: here is why CI is actually red — a real, PR-introduced break the review did not findTwo checks fail at 1. ❌
|
| seed pathname | fixture match | result | |
|---|---|---|---|
master |
/mcp |
✅ === "/mcp" |
passes |
| this PR | /k8s-ro/mcp |
❌ falls through to res.writeHead(404) |
times out |
Adding the gateway's /k8s-ro prefix to the seed is correct for production, but the fixture was never taught the prefix. Every POST now 404s, so OpenCode never initializes the connector, no tool call is ever recorded, hangNextCall never fires, and await currentMcp.waitForTimeout() waits on a promise nothing will resolve — hence a clean vitest timeout rather than an assertion failure. The legacy-SSE half still passes (/sse and /messages are unchanged), which is why the failure appears late in the test.
Note the assertions you added are all correct and all pass — hostname, port, and pathname are verified before the hang. The gap is the fixture, not the expectation.
Minimal fix — parameterize the fixture path and pass the seed's:
-async function startK8sMcpFixture() {
+async function startK8sMcpFixture(mcpPath = "/mcp") {
@@
- if (req.method === "POST" && requestUrl.pathname === "/mcp") {
+ if (req.method === "POST" && requestUrl.pathname === mcpPath) {
@@
- const currentMcp = await startK8sMcpFixture();
+ const currentMcp = await startK8sMcpFixture(seedUrl.pathname);seedUrl is already in scope above that line, so the fixture stays pinned to whatever the seed actually says and this class of drift can't recur silently. I'd avoid a loose .endsWith("/mcp") — it would keep passing if the prefix were wrong, which is the property you just added assertions to prevent.
2. ⚠️ General tests (workspaces-b) — not yours, do not chase it
AssertionError: expected [ 'heartbeat_runs_queued_age_idx' ]
to include 'heartbeat_runs_agent_dispatch_idx'
❯ packages/db/src/heartbeat-dispatch-query-plan.test.ts:401
A BLO-20396 dispatch-index assertion in @paperclipai/db. This PR touches exactly two files — statefulset.yaml and opencode-k8s-seed-transport.test.ts — and neither is in packages/db. Rebasing onto a fixed master is the remedy, not a change here.
3. review/ally-comment is a metadata gate
It fails because the consolidated review carries an unresolved finding. I've responded above with live cluster evidence that the finding is false; that gate needs Ally or a human to clear, not a commit.
Net: production config in this PR is right — I verified the gateway Service, its two ready endpoints, and its /k8s-ro upstream mapping against the live cluster. The only thing standing between this and green that belongs to you is the fixture path. Still not pushing to your branch.
|
Cross-repository Service contract confirmed for the current head. The target is provided by onprem-k8s PR paperclipai#2781, manifest |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: f4280f6
Prior Findings Dispositioned (1)
- prior:faa8f56 critical 1 — still-present —
deploy/helm/paperclip/templates/statefulset.yaml:648— The current-head seed still targetspaperclip-mcp-gateway-k8s-ro.paperclip.svc.cluster.local, but the current Helm template tree contains no matching Service or gateway manifest. The changed test only asserts the string and cannot validate that Kubernetes DNS resolves it, so the prior Service/hostname mismatch remains unproven and workers can fail to connect.
Critical Issues (1)
- [gstack/review + native-codex]
deploy/helm/paperclip/templates/statefulset.yaml:648— The seededk8s-roURL points atpaperclip-mcp-gateway-k8s-ro, but this chart still does not define that Service. The regression test validates only the literal URL, so it will pass even when the rendered deployment has no resolvable target and every workerk8s-rorequest fails.- Add the gateway Service and its backing deployment to this chart, or point the seed at a Service that this chart demonstrably deploys; extend the test or Helm validation to verify the rendered Service/hostname contract.
Important Issues (0)
Suggestions (0)
Strengths
- The change is narrowly scoped to the shared worker seed and regression coverage.
- The
/k8s-ro/mcppath is explicit and is covered by the updated test.
Recommended Action
- Resolve the Service/hostname mismatch before merge.
- Re-run Helm rendering/lint and the transport regression test against the deployed Service contract.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 3084e3c
Prior Findings Dispositioned (1)
- prior:f4280f6 critical 1 — fixed —
deploy/helm/paperclip/templates/statefulset.yaml:648— The seed now targets the dedicatedpaperclip-mcp-gateway-k8s-roService. The paired deployment manifest referenced by the PR defines that Service in namespacepaperclipon port 8080, and the current seed uses the matching DNS name and/k8s-ro/mcproute.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The production seed is switched from the direct read-only Kubernetes MCP Service to the dedicated scrubbing gateway.
- The regression test asserts the complete gateway URL and configures its fixture to accept the exact seeded path, preserving coverage for the Streamable HTTP transport.
- The change remains narrowly scoped and preserves the intended cross-repository deployment sequencing.
Recommended Action
- No Critical or Important issues found. Safe to merge after the remaining repository checks pass.
1 participant
Thinking Path
Linked Issues or Issue Description
Refs: PEN-2429
Bug
What happened
pods_getthrough the direct read-only Service returned peer pod environment data without redaction because the worker seed bypassed the deployed scrubber.Expected behavior
The worker's
k8s-roconnector uses the dedicated scrubbing gateway.Steps to reproduce
k8s-roconnector.pods_getthrough the configured URL and inspect only response shape.Paperclip version or commit
Master at the PR base commit; this change is for the Helm-generated worker configuration.
Deployment mode
Self-hosted Kubernetes deployment.
What Changed
k8s-roHTTP URL from the read-only Kubernetes Service topaperclip-mcp-gateway-k8s-ro.paperclip.svc.cluster.local:8080/k8s-ro/mcp.Verification
helm lint deploy/helm/paperclip— passes.git diff --check— passes./healthzreturns HTTP 200.pods_getthrough/k8s-ro/mcpreturned redaction markers; no credential values were logged.PEN-2429andk8s-ro scrubbing gateway; no duplicate implementation PR found.Risks
Model Used
Checklist