Skip to content

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

Open
zopeVaibhav wants to merge 1 commit into
CopilotKit:mainfrom
zopeVaibhav:fix/culler-network-policy
Open

Fence the culler, and refuse a release that leaves a pod unfenced#277
zopeVaibhav wants to merge 1 commit 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.

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

1 participant