fix(keycloak): use barman-capable system image when backups enabled - #3306
fix(keycloak): use barman-capable system image when backups enabled#3306Andrey Kolkov (androndo) wants to merge 2 commits into
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a failure in Keycloak database backups where the 'standard' container image lacked the necessary Barman binaries. By dynamically switching to the 'system' image variant when backups are enabled, the fix ensures that base backups and WAL archiving function correctly. This is a temporary measure pending a migration to the Barman Cloud Plugin. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
📝 WalkthroughWalkthroughThe Keycloak Postgres CNPG template selects the ChangesKeycloak CNPG backup image selection
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Code Review
This pull request updates the Keycloak database template to conditionally select the 'system' PostgreSQL image variant instead of 'standard' when backups are enabled, ensuring the availability of barman-cloud binaries. The review feedback highlights two important issues: a regex matching bug that bypasses the variant switch for non-17 PostgreSQL versions if the image already has a suffix, and potential template rendering errors from using Sprig's 'ternary' function with non-boolean values. The feedback suggests stripping the suffix first and using a standard 'if' block for safety.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| {{- $backupOn := and .Values.backup.enabled (not (empty .Values.backup.destinationPath)) }} | ||
| {{- $variant := ternary "system" "standard" $backupOn }} | ||
| imageName: {{ if regexMatch ":17\\." $image }}ghcr.io/cloudnative-pg/postgresql:17.7-{{ $variant }}-trixie{{ else if regexMatch ":[0-9]+\\.[0-9]+$" $image }}{{ printf "%s-%s-trixie" $image $variant }}{{ else }}{{ $image }}{{ end }} |
There was a problem hiding this comment.
There are two issues with the current implementation:
- Variant Switch Bypass: If
$imagealready contains a suffix like-standard-trixieor-system-trixie(which is common for existing clusters or the default pinned image), the regex:[0-9]+\.[0-9]+$will not match because of the trailing suffix. As a result, for any non-17 PostgreSQL version (e.g.,16.3-standard-trixie), the template will fall through to theelseblock and output{{ $image }}directly, meaning the variant will not be switched tosystemeven when backups are enabled. - Ternary Type Safety: Sprig's
ternaryfunction expects a strictboolas its third argument. If.Values.backup.enabledor.Values.backup.destinationPathevaluates to a non-boolean type (such asnilor a string), it can cause a template rendering error. Using a standardifblock is safer and more idiomatic.
We can resolve both issues by stripping the existing suffix first to get a clean $baseImage, and using a standard if block to set the $variant.
{{- $baseImage := regexReplaceAll "-(standard|system)-trixie$" $image "" }}
{{- $variant := "standard" }}
{{- if and .Values.backup.enabled .Values.backup.destinationPath }}
{{- $variant = "system" }}
{{- end }}
imageName: {{ if regexMatch ":17\\." $baseImage }}ghcr.io/cloudnative-pg/postgresql:17.7-{{ $variant }}-trixie{{ else if regexMatch ":[0-9]+\\.[0-9]+$" $baseImage }}{{ printf "%s-%s-trixie" $baseImage $variant }}{{ else }}{{ $image }}{{ end }}There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/system/keycloak/templates/db.yaml`:
- Line 21: Update the image selection expression around imageName to recognize
existing -standard-trixie and -system-trixie suffixes, replace that variant with
$variant for non-17 images, and preserve any digest suffix. Keep the existing
special handling for PostgreSQL 17 and unchanged behavior for images without a
recognized PostgreSQL tag.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c8937649-9b3f-405a-af2a-c6513e3e0016
📒 Files selected for processing (1)
packages/system/keycloak/templates/db.yaml
| */}} | ||
| {{- $backupOn := and .Values.backup.enabled (not (empty .Values.backup.destinationPath)) }} | ||
| {{- $variant := ternary "system" "standard" $backupOn }} | ||
| imageName: {{ if regexMatch ":17\\." $image }}ghcr.io/cloudnative-pg/postgresql:17.7-{{ $variant }}-trixie{{ else if regexMatch ":[0-9]+\\.[0-9]+$" $image }}{{ printf "%s-%s-trixie" $image $variant }}{{ else }}{{ $image }}{{ end }} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Replace the variant on already-suffixed image tags.
The else if branch only matches bare tags such as :18.0. On a later reconciliation, lookup may return ...:18.0-standard-trixie; for non-17 versions this falls through to $image, so enabling backups still deploys the standard variant and Barman backups fail. Normalize existing -standard-trixie/-system-trixie suffixes and replace only the variant, preserving any digest if present.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/system/keycloak/templates/db.yaml` at line 21, Update the image
selection expression around imageName to recognize existing -standard-trixie and
-system-trixie suffixes, replace that variant with $variant for non-17 images,
and preserve any digest suffix. Keep the existing special handling for
PostgreSQL 17 and unchanged behavior for images without a recognized PostgreSQL
tag.
The Keycloak DB backup (#3174) uses the in-tree native `barmanObjectStore`, whose `barman-cloud-*` binaries ship ONLY in the CNPG `system` image variant. Since #2342 the DB is pinned to the `standard` variant, which omits them — so with backups enabled a base backup fails `barman-cloud-backup: executable file not found` and WAL archiving cannot run (also risks WAL accumulation). Pin the barman-capable `system` variant only when backups are enabled (backup.enabled + destinationPath); otherwise keep `standard`. Verified on a freedom-portal cluster: with `system-trixie` the CNPG Backup completes and WAL archiving works to S3; with `standard-trixie` it fails as above. TEMPORARY bridge: `system` is deprecated in CNPG 1.27 (removed in 1.29). Remove this once Postgres backups migrate to the Barman Cloud Plugin — see #3300. Refs #3300, #2342, #3174. Signed-off-by: Andrey Kolkov <androndo@gmail.com>
892bad8 to
0b10d4b
Compare
The image-variant switch in templates/db.yaml pins the barman-capable CNPG `system` image exactly when native barmanObjectStore backups render; a regression there silently drops backups to a barman-less `standard` image (#3300). The package is otherwise covered by helm unittest, but db.yaml and this new branching logic had none. Add tests/db_backup_image_test.yaml locking input -> rendered spec.imageName: backups off -> standard, enabled with a destination -> system, and enabled with an empty destinationPath -> standard (the two-part gate). Each case also asserts the barmanObjectStore block co-renders with the system image, so the variant switch and the object-store gate cannot drift apart. Assisted-By: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com>
VerdictLGTM with non-blocking notes The image-variant switch is correctly root-caused, minimally scoped, gated byte-identically to the barmanObjectStore render, and test-locked; the only gap is a missing required Findings[MINOR] PR body — missing required The PR body replaces the template with a custom Problem/Fix/Verification structure and drops the Caveats
Recommended follow-ups
|
|
Thanks for the review. Addressed the The |
VerdictLGTM with non-blocking notes No blocking defects found after active falsification attempts (gate-disagreement renders, template mutation testing, full input-class enumeration of the imageName expression, upgrade/rollback tracing); two MINOR gaps remain, both non-blocking. Findings[MINOR] Enumerated all input classes of [MINOR] Flipping Claim mismatches[PARTIAL] "pin the barman-capable system variant only when backups are enabled" - holds for all chart-managed image states, but not universally: the else-branch classes above keep their variant while backups still render (see Finding 1, harness outputs). [UNVERIFIABLE] "system is deprecated in CNPG 1.27 (removed in 1.29)" - upstream sources do not confirm the 1.29 date. cloudnative-pg/postgres-containers README: system images "are deprecated and will be removed once in-core support for Barman Cloud in CloudNativePG is phased out" (no version named). CNPG 1.27 backup docs: Caveats
Recommended follow-ups
|
Cluster-side validation (dev3, CNPG operator 1.27.3)Tested the fix live in an isolated throwaway namespace (did not touch the live Negative control (the cleanest proof). Same which is verbatim the failure this PR fixes. On Render / gate / upgrade. Backups off renders CNPG deprecation, confirmed live. The admission webhook on apply warned: One additional MINOR (operational, not a defect in this PR)Since this PR activates the |
The keycloak-db backup added in #3174 uses the in-tree native spec.backup.barmanObjectStore, whose barman-cloud-* binaries ship only in the CNPG system image variant; keycloak-db is pinned to the standard variant, so base backups fail with 'barman-cloud-backup: executable file not found'. The proposed workaround (#3306) switches keycloak-db to the deprecated system image variant when backups are enabled. Migrate keycloak-db to the barman-cloud plugin instead, matching the rest of this PR: render spec.plugins referencing a barmancloud.cnpg.io ObjectStore (the S3/barman config moves there) and a method=plugin ScheduledBackup, keeping the Cluster on the standard image (the plugin runs the barman tooling in a sidecar). This removes the need for the system-variant workaround entirely, so #3306 can be closed. #3300. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com>
The keycloak-db backup added in #3174 uses the in-tree native spec.backup.barmanObjectStore, whose barman-cloud-* binaries ship only in the CNPG system image variant; keycloak-db is pinned to the standard variant, so base backups fail with 'barman-cloud-backup: executable file not found'. The proposed workaround (#3306) switches keycloak-db to the deprecated system image variant when backups are enabled. Migrate keycloak-db to the barman-cloud plugin instead, matching the rest of this PR: render spec.plugins referencing a barmancloud.cnpg.io ObjectStore (the S3/barman config moves there) and a method=plugin ScheduledBackup, keeping the Cluster on the standard image (the plugin runs the barman tooling in a sidecar). This removes the need for the system-variant workaround entirely, so #3306 can be closed. #3300. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com>
<!-- Thank you for making a contribution! Here are some tips for you: - Use Conventional Commits for the PR title: `type(scope): description` - Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore - Scopes are not an exhaustive list — pick the most specific scope for the change and extend the list when a genuinely new area appears. Examples: - System components: dashboard, platform, operator, cilium, kube-ovn, linstor, fluxcd, cluster-api - Managed apps: postgres, mariadb, redis, kafka, clickhouse, virtual-machine, kubernetes - Development and maintenance: api, hack, tests, ci, docs, maintenance - Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or add a `BREAKING CHANGE:` footer - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does Fixes #3300. Migrates CNPG Postgres backups off the deprecated native `spec.backup.barmanObjectStore` (removed in CNPG 1.29; the `barman-cloud-*` binaries it needs are absent from the `standard` image variant keycloak-db pins, and the bare system-flavor tags apps/postgres pins are deprecated upstream) onto the [CloudNativePG Barman Cloud plugin](https://github.com/cloudnative-pg/plugin-barman-cloud), across every Postgres backup surface: `apps/postgres`, `system/keycloak`, and the `cozy-default-cnpg` BackupClass strategy driver. The chart renders `spec.plugins` + a `barmancloud.cnpg.io/ObjectStore` CR; the backup-controller driver SSA-applies an `ObjectStore` and patches `spec.plugins`; `Backup`/`ScheduledBackup` use `method: plugin`. The `postgres-operator` chart vendors and co-locates the `plugin-barman-cloud` operator (with its cert-manager-issued mTLS) so CNPG can discover it. One platform-level change is required for the plugin to run and ships with this PR (see the upgrade notes in `docs/operations/backup-classes.md` and the release note below): the CloudNativePG operator is bumped to 1.28.1 (also fixing an operator/CRD version skew that broke all backups). The other prerequisite — the LINSTOR scheduler admission webhook stripping `initContainers[].restartPolicy` cluster-wide, which broke the plugin's restartable sidecar — is already fixed on `main` (f4aef73 re-vendors `linstor-scheduler` to upstream chart 0.3.1 / extender v0.3.6, which keeps the webhook and stops stripping unknown pod fields), so after the rebase this PR no longer touches `linstor-scheduler`; just do not hold that package back on an older version when rolling out the plugin. Supersedes #3306 (the keycloak `system`-image workaround): keycloak-db now backs up via the plugin on the `standard` image, so that stopgap is no longer needed. ### Screenshots <!-- REQUIRED for UI changes: attach screenshots or screen recordings demonstrating the visual impact of your changes. PRs with UI changes without screenshots will not be merged. --> ### Downstream repositories <!-- Cozystack is upstream for repositories that are NOT kept in sync with it automatically, and nothing in CI compares the two sides, so a change here can break them silently. Which change forces what, and which file to touch, is the trigger map in docs/agents/contributing.md under "Downstream Repositories". Walk it before you tick anything. IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM TO FILL IN: 1. Walk the trigger map against the actual diff of this PR, file by file. Do not decide from the PR title or from your memory of what you changed. 2. For every repository you tick, open the follow-up PR there and link it on the same line. A ticked box with no link claims work that does not exist, which is worse than an empty box. Search that repository for an open PR or issue covering it first, and link that instead of filing a duplicate. 3. When the follow-up is out of scope, or needs a decision that is not yours to make, open an issue in that repository instead, link that, and say so in the PR body. Do not push a speculative PR into another repository just to fill a line here. 4. Do not tick "No downstream repository is affected" to make the checklist go away. If you are unsure, leave every box empty and say so in the PR body, so a human decides. --> - [ ] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: docs/operations/backup-classes.md changed; a website mirror follow-up is warranted (not yet opened). - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same `type(scope):` prefix as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note fix(backups): migrate CNPG Postgres backups to the CloudNativePG Barman Cloud plugin Postgres backups (apps/postgres, system/keycloak, and the cozy-default-cnpg BackupClass strategy) now use the CloudNativePG Barman Cloud plugin (spec.plugins + a barmancloud.cnpg.io/ObjectStore, method: plugin) instead of the deprecated native spec.backup.barmanObjectStore (removed in CNPG 1.29; the barman-cloud-* binaries it needs are absent from the standard image variant keycloak-db pins, and the bare system-flavor tags apps/postgres pins are deprecated upstream). The barman-cloud plugin operator is installed alongside the CNPG operator. One platform-level change rides along and requires operator attention on upgrade: - The CloudNativePG operator is bumped to 1.28.1 (fleet-wide minor upgrade); this also fixes an operator-newer-than-its-CRDs skew that made the operator report "instance manager was restarted during backup" and fail every backup. The vendored chart ships the CNPG CRDs as ordinary release manifests (crds.create), so helm upgrade updates them with the operator; the skew came from the image.tag pin outrunning the chart's CRDs, which this PR removes. - The plugin's restartable sidecar requires the linstor-scheduler admission webhook that no longer strips initContainers[].restartPolicy (upstream chart 0.3.1 / extender v0.3.6, shipped separately on main); do not hold the linstor-scheduler package back on an older version when rolling out the plugin. ``` Close #3300, #3246 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Switched PostgreSQL CNPG backups/restores to the plugin-based Barman Cloud workflow, using dedicated ObjectStore resources for backup and recovery. * Added the Barman Cloud CNPG-I plugin chart (deployment, TLS, RBAC, leader election). * **Bug Fixes** * Improved backup/restore attachment messaging when the CNPG Cluster isn’t ready yet. * Preserves server-name behavior to avoid WAL-archive prefix issues; clarified endpoint CA handling. * **Changes** * Helm integration no longer uses the legacy `spec.backup.barmanObjectStore` path; updated gating, credential projection, and ObjectStore-related RBAC/cleanup. * Removed the linstor-scheduler admission webhook; updated CloudNativePG/operator versions and CRDs. * **Tests** * Updated controller and Helm-unittest checks for the plugin flow; added/extended end-to-end backup/restore coverage. * **Documentation** * Expanded upgrade/migration and backup documentation; updated backup example materials and comments. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Problem
The Keycloak DB backup added in #3174 uses the in-tree native
spec.backup.barmanObjectStore, whosebarman-cloud-*binaries ship only in the CloudNativePGsystemimage variant. Since #2342keycloak-dbis pinned to thestandardvariant (17.7-standard-trixie), which omits those binaries — so with backups enabled:Base backups fail and WAL archiving can't run (the latter also risks WAL accumulation blocking recycling). This is the keycloak facet of #3300.
Fix
In
templates/db.yaml, pin the barman-capablesystemvariant only when backups are enabled (backup.enabled+backup.destinationPath); otherwise keepstandard. One-line variant switch, gated, fully documented inline.Verification
On a freedom-portal cluster (CNPG 1.27, PostgreSQL 17.7):
standard-trixie+ native barman →barman-cloud-backup: executable file not found(base backup fails).system-trixie+ native barman → CNPGBackupcompleted,LastBackupSucceeded=True,ContinuousArchiving=True, WAL archived to S3 (barman-cloud-wal-archiveruns).helm unittest48/48 pass (incl. newtests/db_backup_image_test.yamllocking the variant gate: backups off →standard, on+destination →system, on+empty-destination →standard);helm templateshowsstandard-trixiewith backups off andsystem-trixiewith backups on.Note for air-gapped installs
Enabling backups now pulls a new tag,
ghcr.io/cloudnative-pg/postgresql:17.7-system-trixie, reachable via a runtimebackup.enabledflip. Mirrors that only carry17.7-standard-trixiemust also mirror17.7-system-trixie, otherwise the first base backup fails on image pull.Temporary bridge
systemis deprecated in CNPG 1.27 (removed in 1.29). This is a stopgap so keycloak backups work today; it should be removed once Postgres backups migrate to the Barman Cloud Plugin — see #3300 (which covers the same issue for thecozy-default-cnpgstrategy andapps/postgres).Refs #3300, #2342, #3174.
Release note
🤖 Generated with Claude Code
Summary by CodeRabbit