Repository navigation
fix(postgres): include restore workloads in the ImageVolume exception - #825
danielgaskins wants to merge 2 commits into
Conversation
fiunchinho
left a comment
There was a problem hiding this comment.
Thank you for this contribution, @danielgaskins! The restore gap in the ImageVolume exception is a real problem, and the regression check plus the detailed validation notes made this easy to review.
The selector fix looks correct to me. It allows exactly {clusterName, clusterName-restore}, only for Pod and Job in the source namespace, and it agrees with the -restore selectors that already exist in kagent/netpol.yaml and the backstage network policies. CI has not run on this PR yet because it comes from a fork. On the PR head (3e93984), I ran make verify-postgres and make verify-kyverno locally, and both pass. verify-kyverno.py reads matchExpressions, so its admission simulation still covers the exception.
No blocking findings. The comments below are about the runbook and some small items.
Non-blocking
1. The RBAC of cnpg-drill 0.1.6 can reach the source Cluster (docs/postgres-backup-verification.md, docs/examples/postgres-restore-check-values.yaml)
When I render the chart with the example values, the Role gives delete on every clusters.postgresql.cnpg.io and create on every pods/exec in the namespace. It has no resourceNames. If an operator follows the runbook in kagent, the drill image can delete the production kagent-pg Cluster, or exec into its primary. The runbook should not install that much access next to the live database.
Suggested fix: in the chart, scope the clusters get/delete rule with resourceNames: [<drillClusterName>], and scope pods/exec to the restore Pod names. Also pin the chart in the runbook by digest, not only by version. The image is already pinned by digest. Separately, we must decide as a team whether to reference a chart from a personal registry. You could split this PR so the selector fix can merge on its own.
2. set -euo pipefail closes the operator's shell (docs/postgres-backup-verification.md:42 and every code block)
The runbook says to run the steps "in the same shell". In an interactive shell, set -e closes the terminal when any check fails, for example the [ "$RECOVERY_STORE" != "$WRITER_STORE" ] guard or a jq -e assertion. The exported variables are then lost, and this can happen in the middle of a drill.
Suggested fix: remove set -e from the blocks, or tell the operator to run each block as a script or in a subshell.
3. Duplicate ### Fixed heading (CHANGELOG.md:10)
## [Unreleased] already has a ### Fixed block, at line 30 after the merge from main. The new heading makes two "Fixed" sections in the release notes.
Suggested fix: move the entry into the existing block. Also use the bold-title-plus-PR-link format of the entries around it.
Suggestions
docs/postgres-backup-verification.md:43:export KUBECONFIG="$HOME/.kube/config"overwrites the KUBECONFIG the operator already uses. Use"${KUBECONFIG:-$HOME/.kube/config}".- Steps 1–3 write
recovery-objectstore.json,recovery-check.yamlandrecovery-result.jsoninto the current directory. BecauseDRILL_VALUESis relative to the repo, that directory is the repo checkout. Use amktemp -ddirectory. tests/verify-postgres-restore.py:26-28: the script splits the render on\n---and finds the document with string search.tests/pick-doc.py($(PICK)) already does this. As another option, add akagent-pg-restoreworkload case toverify-kyverno.py, which already simulates selector matches. A missing exception currently fails with a bareStopIteration. Also, thename-restore-other/unrelatedloop (lines 36-39) does not add coverage, because theset(allowed) == {...}assertion on line 40 already covers it.policy-exception.yaml:14-17: the edited comment wraps badly ("...as the source. The rule / is restrict-volume-types'..."). Please re-flow the paragraph.
fiunchinho
left a comment
There was a problem hiding this comment.
Thank you for this PR, and for the detailed testing notes.
We ask you to split it in two:
- The
PolicyExceptionchange for<clusterName>-restore(withtests/verify-postgres-restore.pyand its Makefile target) is useful on its own. Our README already tells operators to use that restore name. Please send it as a separate PR so that we can review it on its own. - The cnpg-drill runbook (
docs/postgres-backup-verification.md,docs/examples/postgres-restore-check-values.yaml, the README link and its changelog entry): we cannot accept this now. Please remove it.
Our reasons for the runbook:
1. Supply chain we cannot verify (main reason)
The runbook installs oci://ghcr.io/danielgaskins/charts/cnpg-drill by version tag, not by digest. OCI tags can change, so the owner of that registry decides which chart, and with it which image, runs in the cluster. The chart pins its image by digest, but neither the chart nor the image has a signature or build provenance (cosign, SLSA). We have no way to verify that they were built from the source we reviewed.
The chart's Role can create and delete CloudNativePG Cluster resources, and it can exec into every pod in the namespace. In this runbook that namespace is kagent, where the production database and the kagent controller run. The runbook also asks operators to put archive credentials in the same namespace. A compromised or malicious image with these permissions could read or change production data. We do not run third-party code with this access unless we can verify where it comes from.
2. Maturity
cnpg-drill is about one week old, has one maintainer, and its README calls it "early alpha". As your validation notes say, nobody has tested it against our IAM, Cilium policies, Kyverno admission or private registries. Our docs must point only to tools that we can support for a long time.
3. Conflict with our restore procedure
The drill uses <clusterName>-restore, the name we reserve for a real recovery. A failed or retained drill leaves a Cluster with that name, holding a full copy of production data, and it blocks the restore procedure until someone removes it. We do not want a verification tool to share a name with the incident path.
We are not saying that the tool is bad, and we see that you designed it with care (read-only recovery store, cleanup guarded by UID, hashed query output). We can look at it again when it has signed releases with provenance and has more users and maintainers. Until then, the restore procedure in our README stays the supported path.
Thank you again.
|
Thanks for the review. I've split the restore-policy fix and regression checks into a separate PR. I'm closing this mixed PR and leaving the cnpg-drill runbook out. |
Summary
Extend the PostgreSQL ImageVolume exception to cover
<clusterName>-restore. The current selector covers only the source Cluster, while the recovery procedure uses a second Cluster that needs the same extension image. Scope the selector to those two exact names, the source namespace, and Pod/Job workloads.Add an opt-in backup verification runbook and cnpg-drill chart 0.1.6 values. The check restores through a separate read-only ObjectStore, verifies API v2 core/vector migration records, a saved conversation context and pgvector, then checks cleanup. The runbook uses the required recovery name, keeps scheduling suspended, and covers failed reports, IAM setup and placement limits.
I maintain cnpg-drill.
Validation
make verify-postgrespasses, including the new selector regression check.The local matrix used Kubernetes 1.35.8, CNPG 1.30.1, Barman Cloud plugin 0.15.0, PostgreSQL 18.6, pgvector 0.8.2 and RustFS 1.0.0. It did not run a kagent controller. Live Kyverno admission, Giant Swarm IAM/Cilium policies, private-registry access and cloud storage remain untested.
Checklist