Skip to content

fix(postgres): include restore workloads in the ImageVolume exception - #825

Closed
danielgaskins wants to merge 2 commits into
giantswarm:mainfrom
danielgaskins:docs/postgres-backup-verification
Closed

danielgaskins wants to merge 2 commits into
giantswarm:mainfrom
danielgaskins:docs/postgres-backup-verification

Conversation

@danielgaskins

Copy link
Copy Markdown

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-postgres passes, including the new selector regression check.
  • The regression check fails with the original template and passes with this change. It covers default/custom Cluster names, unrelated-name rejection and extension gating.
  • Bash syntax, local links, empty/failed report rejection and diff checks pass.
  • Ran the documented commands with the published chart against a local fixture using the API v2 baseline SQL, synthetic migration records and a conversation context. All four assertions passed after restoring the archived context, which had been removed from the live fixture. The source Cluster spec stayed unchanged; temporary Cluster/PVC/PV cleanup passed.
  • A backup without a saved context failed the data assertion and cleaned up.

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

  • Update changelog in CHANGELOG.md.
  • Make sure values.yaml and values.schema.json are valid (unchanged; chart render checks pass).
  • Serving preset benchmark: not applicable.

@danielgaskins
danielgaskins requested a review from a team as a code owner October 6, 2026 22:55

@fiunchinho fiunchinho left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.yaml and recovery-result.json into the current directory. Because DRILL_VALUES is relative to the repo, that directory is the repo checkout. Use a mktemp -d directory.
  • 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 a kagent-pg-restore workload case to verify-kyverno.py, which already simulates selector matches. A missing exception currently fails with a bare StopIteration. Also, the name-restore-other / unrelated loop (lines 36-39) does not add coverage, because the set(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 fiunchinho left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this PR, and for the detailed testing notes.

We ask you to split it in two:

  • The PolicyException change for <clusterName>-restore (with tests/verify-postgres-restore.py and 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.

@danielgaskins

Copy link
Copy Markdown
Author

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.

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.

2 participants