Skip to content

fix(chart): a failed pg_dump fails the kates backup job, and backup.enabled without the bundled database fails rendering - #372

Merged
bmscomp merged 1 commit into
mainfrom
fix/audit-03-backup-job
Oct 17, 2026
Merged

bmscomp merged 1 commit into
mainfrom
fix/audit-03-backup-job

Conversation

@bmscomp

@bmscomp bmscomp commented Oct 17, 2026

Copy link
Copy Markdown
Owner

Why

The kates chart's backup CronJob has two defects. One means production never gets a backup; the other means a failing backup can delete good ones. Checked in the code:

  • Production renders no backup. templates/backup-cronjob.yaml rendered only under {{- if and .Values.backup.enabled .Values.postgresql.enabled }}, because it dumps the bundled PostgreSQL only. values-prod.yaml:76-82 set backup.enabled: true, retention 30 and a 50Gi PVC, but prod uses externalDatabase, since productionMode refuses the bundled database. So production rendered no CronJob and no PVC. And NOTES.txt:70-75 checks only backup.enabled, so it still printed Backups: Schedule 0 1 * * * | Retention: 30 days | Storage: PVC (50Gi). The production example in charts/kates/README.md (--set backup.enabled=true with an external database) has the same bug. values-corporate.yaml has no backup block.
  • A failed dump passes and then deletes the real backups. The script ran pg_dump ... | gzip > "${FILENAME}" without pipefail. If pg_dump fails (wrong password, database down), gzip still writes a valid, empty archive. The script prints "Backup complete" and exits 0, and find /backups -name "kates-*.sql.gz" -mtime +N -delete then prunes the real backups older than the retention. pipefail alone isn't enough: gzip creates the file before pg_dump fails, so the empty archive stays as the newest file, the one a restore would pick. With restartPolicy: OnFailure, each retry in the same pod adds another. On staging (values-staging.yaml: bundled database, 10Gi PVC, retention 14), a broken dump leaves no usable backup after 14 days, and every job shows green.

The emptyDir case (backup on, backup.persistence.enabled off) isn't silent: NOTES says emptyDir (non-persistent), and no shipped overlay enables backup without persistence. It is left alone (see Decisions).

Changes

charts/kates/templates/backup-cronjob.yaml

  • The guard is split. With backup.enabled set and postgresql.enabled off, rendering fails: "backup.enabled: the backup CronJob dumps the bundled PostgreSQL (postgresql.enabled) only, so with externalDatabase no backup would run. Back the external database up with its own tooling and set backup.enabled=false." This follows the fail style in secret.yaml and deployment.yaml. With the fail in place, NOTES prints Backups: only when the CronJob renders, so NOTES.txt needs no edit.
  • The script starts with set -eo pipefail. It clears /backups/kates-*.sql.gz.partial left by a failed try, dumps to ${FILENAME}.partial, and runs mv to the final name only after the dump succeeds. A failed dump now fails the job before the prune and leaves no archive. The prune is unchanged, and its kates-*.sql.gz pattern doesn't match .partial files.
  • A five-line comment above the script says why, like the commented script in cleanup-cronjob.yaml.
  • The pod keeps kates.selectorLabels. Postgres' NetworkPolicy admits only pods that carry them, and staging sets networkPolicy.enabled.

charts/kates/values-prod.yaml

  • The backup: block is gone. In its place is a four-line comment: the chart's backup dumps only the bundled PostgreSQL, which productionMode refuses, so back the external database up with its own tooling (CloudNativePG backups, a managed service's point-in-time recovery). It points to the externalDatabase comment in values.yaml by content, not by line number. Prod never rendered a backup, so no object goes away.

charts/kates/values.yaml

  • The backup.enabled comment says it dumps the bundled PostgreSQL only, and that rendering fails with postgresql.enabled=false. The edit is at line 677. The anchored caveat refs into values.yaml:395-405 (mcp_caveats.go) and :605-673 (mcp_tools_scenario.go) end above it, so they don't move.

charts/kates/README.md

  • The backup.enabled row says "bundled PostgreSQL only; rendering fails with postgresql.enabled=false".
  • "Example: Production with External DB" drops --set backup.enabled=true, which now fails rendering.

charts/kates/Chart.yaml

  • version goes from 0.10.11 to 0.11.0, and the change comment above it describes this PR. appVersion stays 1.25.1.
  • A new artifacthub.io/changes entry (kind: fixed) starts with ACTION REQUIRED, because values that rendered before now fail.

Version tables

  • README.md chart table and docs/book/appendix-d-versions.md: the kates chart is 0.11.0. Neither generator writes in place, so I edited both cells by hand and confirmed them with scripts/gen-chart-table.sh --check and scripts/gen-version-matrix.sh --check.

.github/workflows/ci.yml

  • A new helm-lint step, "Assert backup needs the bundled database", after "Template validation", modelled on the Strimzi assert. It renders the prod overlay with --set externalDatabase.existingSecret=ci. Then it checks that backup.enabled=true with postgresql.enabled=false and an external database fails to render. It also checks that the failure output contains the new message, so a render that fails for some other reason (the credentials check, say) can't pass by accident.

Tests

  • charts/kates has no helm-unittest suite: templates/tests holds only the helm-test hooks. The new ci.yml step is the lasting test of the new branch. It runs in helm-lint on every PR that triggers that job.
  • The backend is unchanged. The full backend suite ran on main and passed.
  • Run locally, all passing:
    • git diff --name-only origin/main...HEAD: one commit ahead of origin/main. Changed paths: .github/workflows/ci.yml, README.md, charts/kates/{Chart.yaml,README.md,templates/backup-cronjob.yaml,values-prod.yaml,values.yaml}, docs/book/appendix-d-versions.md. Jobs triggered:

      • ci.yml: the code filter fires because ci.yml itself changed, so build-and-test, dependency-scan, yaml-validation and go-tests run, plus helm-lint and kyverno-policy-test;
      • lint.yml: actionlint, yamllint;
      • docs.yml: chart table, mermaid, link-check;
      • book.yml: render;
      • ci-kafka-charts.yml: the dashboards, library and matrix jobs, because charts/kates/** is in its filter.

      The worktree was still clean after every check.

    • The new ci.yml step, run locally: the prod overlay renders, and the refused combination fails with the expected message ("OK: backup without the bundled database is refused").

    • helm lint charts/kates/: 1 chart linted, 0 failed. Local helm is v4.3.0; CI pins v3.17.0.

    • helm template kates charts/kates/ with the defaults and each values-*.yaml overlay:

      • default, dev, generic, kind, local, native-local, native and staging all render;
      • prod and corporate (both productionMode) render with --set externalDatabase.existingSecret=x. Without it they fail on the existing credentials check, as on main;
      • staging renders the new script: set -eo pipefail, then the .partial file and its mv. The image is postgres:16-alpine, whose shell is busybox ash.
    • scripts/check-chart-test-paths.sh: all 2 /api paths resolve.

    • helm dependency build, then helm lint and helm template for every chart, in a scratch copy with an isolated helm config: rc 0. kates-platform packages the kates 0.11.0 subchart from disk and renders; it sets no backup values.

    • kyverno apply of the kates policies against kates/k8s/deployment.yaml (kyverno 1.19.1): fail 0, error 0.

    • yamllint -c .yamllint.yaml .github config charts (1.38.0): 0 errors. The one warning, a blank line at values-corporate.yaml:142, is in a file this PR doesn't touch.

    • actionlint 1.7.12: rc 0. It also shellchecks the new run block.

    • cd cli && go test -count=1 ./...: all packages ok. Run because CLI tests read charts/kates/values.yaml through anchored caveat refs (see Changes). CI runs it as -p 1 -race -v; here it ran without -race.

    • go vet ./...; golangci-lint run --new-from-rev=origin/main ./... (2.14.0): 0 issues; govulncheck ./...: no vulnerabilities.

    • scripts/check-cli-style.sh, scripts/check-cli-compat.sh.

    • scripts/check-versions.sh and --workflows: every pin agrees, and no workflow hardcodes a CI toolchain pin.

    • scripts/gen-chart-table.sh --check (kates 0.11.0) and scripts/gen-version-matrix.sh --check.

    • scripts/check-book-style.sh (within the ratchet baseline), check-book-contrast.py (all 170 colour pairs meet WCAG AA), check-mermaid.sh --quiet (156 diagrams, 0 invalid).

    • A scratch-copy HTML render of the book (prepare-book-render.sh, then quarto 1.10.19): 39 chapters, rc 0, no WARN or ERROR lines, and appendix-d shows 0.11.0 (app 1.25.1). check-book-fragments.py finds all 7,458 fragment links in 38 pages landing on an id.

    • The ci-kafka-charts dashboards job: scripts/check-dashboards.py passes all 12 dashboards, and python3 -m unittest discover -s dashboards is OK (5 skipped).

  • Skipped locally:
    • cd kates && ./mvnw verify: kates/ is unchanged, so it gives the same result as on main;
    • gen-dashboards.py --check and ci.yml's yaml-validation, because PyYAML isn't installed locally. This PR touches no dashboard and nothing under config/, and yamllint parsed config/ cleanly;
    • lychee's link check, because lychee isn't installed. The only README.md and appendix-d changes are a version cell each, with no links;
    • kubeconform, because it isn't installed. CI renders the default values, where backup is off, so the changed CronJob isn't in that output anyway;
    • make check-help and shellcheck on scripts, because no Makefile or shell script changed;
    • the book PDF render, left to CI.
  • Runs only in CI:
    • Java build-and-test (./mvnw verify, surefire plus the *IT failsafe tests). It runs because ci.yml changed; kates/ is unchanged;
    • dependency-scan (Trivy fs on kates, govulncheck);
    • go-tests with -race -p 1 -v;
    • helm-lint's kubeconform validation of every rendered chart;
    • ci-kafka-charts: the promtool PromQL check, the Grafana compat check (Docker), gen-dashboards --check, the kafka-common helm-unittest library job, and the chart matrix;
    • docs.yml's lychee link check;
    • the book PDF render (book.yml's pdf job);
    • the Kind smoke tests and the Docker image builds don't run, because these paths don't trigger them (no kates/, cli/, config/ or Dockerfile change);
    • the github-advanced-security AI scan, which fails on every PR for a GitHub-side reason.

Decisions

  • Chart version 0.11.0, decided by the user. Values that rendered before (backup.enabled with externalDatabase, including the README example) now fail on upgrade, and 0.9.0 also shipped its removed-key fails as a minor. kates-platform's range >=0.5.0 <1.0.0 accepts it.
  • External database: fail, and remove prod's backup block. I didn't make the CronJob dump externalDatabase. That needs the host from externalDatabase.host/port and a different secret key (password, not QUARKUS_DATASOURCE_PASSWORD). It would also put a production database's backups on an in-cluster PVC. The fail and the removal ship together, or values-prod.yaml would stop rendering.
  • emptyDir: left alone. NOTES already says emptyDir (non-persistent), and a fail would break a bare --set backup.enabled=true on kind.
  • The ci.yml assert step is added. It's the only lasting test of the new branch, at the cost of the full backend CI on this PR (about 30 minutes).
  • The script runs with set -eo pipefail, dumps to a .partial file, renames it with mv after success, and clears stale partials at the start.

Deliberately not done

  • Upgrades break on purpose for values that set backup.enabled with externalDatabase, such as copied prod values or the README example. Their backups never ran. The ACTION REQUIRED changelog entry says so.
  • Staging's job turns red if its dumps have been failing quietly. That's the point, but it will be the first time it shows.
  • The script wasn't run in postgres:16-alpine, because Docker is off locally. So busybox ash's pipefail support wasn't checked in the real image. If it were missing, every run would fail loudly rather than pass silently.
  • postgresql.auth.existingSecret not checked. The job reads QUARKUS_DATASOURCE_* keys from it, and I didn't check that such a secret carries them. That is outside this finding.
  • kubeconform and lychee weren't run locally (see Tests).

Landing

This PR shares no file with the earlier plan PRs (#370, #371), so it can land at any time. It also merges cleanly with the open Actions bump #367, which edits ci.yml too. A later PR that edits charts/kates will conflict on the chart version in Chart.yaml, README.md and appendix-d-versions.md. Land chart PRs one at a time and redo the bump on rebase.

…nabled without the bundled database fails rendering

The kates chart's backup CronJob dumps the bundled PostgreSQL only, and
rendered only when backup.enabled and postgresql.enabled were both set.
values-prod.yaml set backup.enabled (retention 30, a 50Gi PVC) with
externalDatabase, since productionMode refuses the bundled database, so
production rendered no CronJob and no PVC while NOTES printed a backup
schedule. The README's production example did the same.

Where the job did run, as on values-staging.yaml, its script piped pg_dump
into gzip without pipefail. A dump that failed (wrong password, database
down) still left a valid, empty archive as the newest file, the job printed
"Backup complete" and passed, and the prune then deleted the real backups
older than the retention. Each OnFailure retry added another empty archive.

backup.enabled without postgresql.enabled now fails rendering and says to
back the external database up with its own tooling. values-prod.yaml and the
README example no longer set it. The script runs with set -eo pipefail and
dumps to a .partial file, renamed only once the dump succeeds; each run
clears partials a failed try left. A failed dump now fails the job before
the prune and leaves no archive a restore would pick.

A helm-lint step in ci.yml renders the prod overlay and asserts the
combination is refused with this message. Chart 0.11.0: values that rendered
before now fail on upgrade.
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.

1 participant