Skip to content

Fence the culler, and refuse a release that leaves a pod unfenced - #277

Merged
davidmckayv merged 4 commits into
CopilotKit:mainfrom
zopeVaibhav:fix/culler-network-policy
Aug 28, 2026
Merged

Fence the culler, and refuse a release that leaves a pod unfenced#277
davidmckayv merged 4 commits into
CopilotKit:mainfrom
zopeVaibhav:fix/culler-network-policy

Conversation

@zopeVaibhav

Copy link
Copy Markdown
Contributor

Closes #276

What this changes

networkPolicy.enabled rendered two policies, selecting component: server and component: computer. The culler CronJob's pod carries component: culler and was selected by neither. A NetworkPolicy applies to the pods its selector matches, and a pod nothing selects keeps the cluster default rather than being denied — so turning the switch on fenced the API and the computers and left open the one pod that mounts a service account token bound to create, patch, delete on Sandboxes and carries the API's whole environment, KEY_ENCRYPTION_KEY and BETTER_AUTH_SECRET included. It wakes every five minutes.

This adds the third policy. It is narrower than the API's because the culler does less: DNS, the database, and the Kubernetes API server. No rule to the internet — it never calls Intelligence, an identity provider or a Bot, and the exception-list egress the API needs is exactly what this pod should not have. No ingress at all, since nothing connects to a CronJob.

The database rule needed care. The bundled-database egress is behind postgresql.enabled, and both shipped targets that turn policy on use an external one — so a policy handling only the bundled case would have left the culler unable to reach its own database and turned a silent gap into a broken sweep. The chart already refuses networkPolicy.enabled with an external database unless networkPolicy.extraEgress names it, so that list is where the address already is. cullerExtraEgress falls back to it, which keeps an operator from writing the same CIDR twice and keeps an upgrade from breaking the sweep. Set it to give the culler a narrower list than the API's.

The check that would have caught it

scripts/check-rendered-chart.ts gains one: on a release that has NetworkPolicies at all, every standing workload must be selected by one. The migrations Job is skipped, because it runs once at install and is torn down.

This is asked of the rendered objects rather than the templates, because the question is which pods came out, not which conditionals were written. It is the same reason the existing 443 check is written that way, and it generalises: the next component added to this chart cannot quietly arrive without a policy.

Where it runs

  • New state that outlives a request? None. Chart templates and a render-time check; no server code is touched.
  • What happens on the second replica? Nothing differs. The culler is a CronJob, and the policy selects it by label however many of anything else are running.
  • Anything serialised? Nothing new.
  • Anything fanned out to a browser? No.
  • New listener, port, or schedule? No new workload. This narrows what an existing one may reach.

Boundary and audit

  • Every acting call still goes through the gateway: no acting call is touched.
  • New refusals and new failures each write a row: the new refusal is at render time in CI, before anything runs, so there is no trail to write to.
  • Nothing new is trusted from the client: no request handling is touched.

Changelog

No CHANGELOG.md entry proposed, but this is the one of my recent changes where an entry has the best case, so say the word. A deployment with networkPolicy.enabled does behave differently afterwards: the culler's egress is restricted where it was unrestricted. On a cluster whose CNI enforces policy and whose database sits somewhere extraEgress does not name, that is a sweep that starts failing — which is the correct outcome, but not a silent one.

Proof

The gates

Rendered against charts/openbot/ci/eks-sandbox-values.yaml, which sets networkPolicy.enabled: true:

policy off                    -> 0 policies
policy on                     -> 3 policies   (server, computer, culler)
culler.enabled=false          -> 2 policies
computers.mode=shared         -> 2 policies   (no culler CronJob exists there)

The culler policy is selected by the culler pod and by nothing else. Parsing the rendered output and comparing every policy's podSelector.matchLabels against the CronJob's pod labels:

ci-openbot-server | selects culler pod: false
ci-openbot-culler | selects culler pod: true

Both database shapes

External database, inheriting extraEgress, which is what the two policy-enabled targets use:

egress:
  - ports: [53/UDP, 53/TCP]
  - ports: [443/TCP, 6443/TCP]
  - to: [ipBlock: 10.0.0.0/16]
    ports: [5432/TCP]

Bundled database:

egress:
  - ports: [53/UDP, 53/TCP]
  - to: [podSelector: app.kubernetes.io/name: postgresql]
    ports: [5432/TCP]
  - ports: [443/TCP, 6443/TCP]

The check fails on main

Rendering main in a detached worktree and running this branch's checker against that output:

$ bun scripts/check-rendered-chart.ts main-render.yaml
::error::This release has NetworkPolicies but none selects ci-openbot-culler (component: culler),
so it is the one pod left unfenced while everything around it is restricted.
exit 1

Against this branch's render:

exit 0
16 objects, 17 secret keys demanded, and every required one is written. The server's egress reaches 443.

Everything else

All five CI values files render and pass the checker:

aks: render=0 check=0 policies=0
eks: render=0 check=0 policies=0
eks-sandbox: render=0 check=0 policies=3
gke: render=0 check=0 policies=0
self-hosted: render=0 check=0 policies=3

helm lint passes on all five. bun run typecheck passes across app, server and worker. biome lint reports nothing on 446 files and biome format leaves the tree clean. The server suite is 1293 pass, 0 fail.

Not done: no live cluster. Everything here is helm template output and a check over it, so it proves what the chart renders and not that a CNI enforces it. The claim that an unselected pod is unrestricted rather than denied is the Kubernetes rule this rests on, and it is the same rule the computers' policy in this file was written for.

@zopeVaibhav

zopeVaibhav commented Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto main and fixed the routines gap here, since you left the file to me.

Confirmed, and it reaches a shipped target: self-hosted-values.yaml ships routines.enabled: true alongside networkPolicy.enabled: true, so the unfenced routines pod is there by default rather than only in a configuration somebody could reach. Rendering that target against main and running this branch's gate over the result:

::error::This release has NetworkPolicies but none selects ci-openbot-routines (component: routines),
so it is the one pod left unfenced while everything around it is restricted.
exit 1

Three NetworkPolicies render there — postgresql, server, computer — and the CronJob carries component: routines.

The second commit adds that policy. It is narrower than the culler's, because the pod holds less: DNS, the database, and the API server on its service port, which is where SERVER_INTERNAL_URL points and the only thing fire-routines.ts calls. No Kubernetes API rule, since automountServiceAccountToken is already false on it and there is no token there to protect. No egress to the internet, and no ingress, since nothing connects to a CronJob. An external database falls back to networkPolicy.extraEgress the same way the culler's does, so an operator does not write the CIDR twice.

After it, every values file renders and the gate passes:

aks: render=0 gate=0 policies=0
eks: render=0 gate=0 policies=0
eks-sandbox: render=0 gate=0 policies=3
gke: render=0 gate=0 policies=0
self-hosted: render=0 gate=0 policies=4

helm lint passes on all five, typecheck and biome are clean, and the server suite is 1599 pass, 0 fail.

The gate was written for the culler and caught this on its own, which is the case it was written for. If you would rather the routines policy landed with #283 instead, say so and I will drop the second commit — this PR just happened to own the file.

@zopeVaibhav
zopeVaibhav force-pushed the fix/culler-network-policy branch from 66c404f to 768fac8 Compare August 28, 2026 04:26
@zopeVaibhav

Copy link
Copy Markdown
Contributor Author

Rebased onto v0.0.5. Three commits now.

The third one touches your check-new-values-keys.ts, which I would rather flag than have you find.

Its carrier lookup matches any resource with the component label, and the culler now has a NetworkPolicy carrying that label too — so it counted two and failed on a CronJob field that had not changed:

::error::Rendering with computers.sandbox.culler.activeDeadlineSeconds absent produced 2 workloads labelled culler, not one.

The message already says "workloads", so I narrowed the filter to the kinds that carry a pod rather than relabelling the policy — every other policy in that file carries its component label and mine would have been the odd one out. Twelve lines, no behaviour change otherwise.

Checked it still catches what it is for: drifting the culler's activeDeadlineSeconds default from 600 to 999 fails it, correctly.

aks:          render=0 gate=0 keys=0 policies=0
eks:          render=0 gate=0 keys=0 policies=0
eks-sandbox:  render=0 gate=0 keys=0 policies=3
gke:          render=0 gate=0 keys=0 policies=0
self-hosted:  render=0 gate=0 keys=0 policies=4

helm lint passes on all five, typecheck and biome are clean, server suite is 1604 pass 0 fail.

If you would rather that file stayed yours, say so and I will drop the label from my policy instead.

@zopeVaibhav

Copy link
Copy Markdown
Contributor Author

Correcting something I claimed earlier in this PR.

I wrote that the culler's policy gives it no rule to the internet. That is not what renders. kubernetesApiCidr is empty by default, so the API-server rule comes out as ports with no to, which permits 443 and 6443 to any address:

53/UDP,53/TCP    -> ANY DESTINATION
443/TCP,6443/TCP -> ANY DESTINATION
5432/TCP         -> ipBlock 10.0.0.0/16

It is the same rule the server's policy already carries, for the reason that one gives — the API server answers on a ClusterIP from a range no chart knows at template time — and the culler ends up with strictly fewer rules than the server, which also has 80 and 443 to everything outside the private ranges. So the shape is the chart's existing trade rather than something new here. But "no rule to the internet" was wrong, and it should not need a render to discover, so the template now says what the rule permits and kubernetesApiCidr says two rules depend on it.

The routines policy has no such rule: DNS aside, its database and API-server egress are both bound by podSelector.

I also cut my comments back to single lines while I was in there. The three block comments I had added were out of proportion to what they explained.

Nothing changed in what renders — comments and one values line. All five targets still render and pass both checks, helm lint passes on all five, and the server suite is 1604 pass 0 fail.

@davidmckayv
davidmckayv merged commit df0c736 into CopilotKit:main Aug 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Turning on networkPolicy fences the API and the computers and leaves the culler open

2 participants