feat(keycloak): add S3 backup for the CNPG database - #3174
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds Helm chart support for Keycloak CNPG database backups, including new backup values, conditional S3 credentials Secret rendering, cluster backup wiring, and a ScheduledBackup resource. ChangesKeycloak database backup configuration
Estimated code review effort: 2 (Simple) | ~15 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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.
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/backup.yaml`:
- Around line 10-20: The ScheduledBackup template guard is too loose and can
render a backup resource even when the cluster has no object-store backup
configuration. Update the condition in the ScheduledBackup template to match the
stricter guard used by db.yaml and backup-secret.yaml, so it only renders when
backup.enabled and backup.destinationPath are both set. Keep the schedule field
behavior intact, but ensure the template does not create a dangling
ScheduledBackup for the keycloak-db cluster when destinationPath is empty.
🪄 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: 6b4eab8c-5597-4b85-ac88-a33ff7f5b319
📒 Files selected for processing (4)
packages/system/keycloak/templates/backup-secret.yamlpackages/system/keycloak/templates/backup.yamlpackages/system/keycloak/templates/db.yamlpackages/system/keycloak/values.yaml
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 introduces optional, scheduled S3 backup capabilities for the Keycloak database cluster managed by CloudNativePG. By leveraging Barman, the changes enable robust data protection through WAL archiving and automated base backups. The implementation is designed to be non-breaking, remaining disabled by default, and provides flexible configuration options for storage paths, credentials, and retention policies. 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
|
There was a problem hiding this comment.
Code Review
This pull request adds scheduled S3 backup support for the Keycloak CNPG database by introducing templates for S3 credentials, scheduled backups, and configuring the database cluster's backup settings. The review feedback points out three important issues: the default cron schedule in values.yaml needs to be updated to a 6-field expression as required by CloudNativePG; the ScheduledBackup resource should only be rendered when backup.destinationPath is provided to prevent reconciliation errors; and values.schema.json must be regenerated to reflect the new configuration options.
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.
| # Enable S3 backups for keycloak-db. | ||
| enabled: false | ||
| # Cron schedule for recurring base backups (CNPG rejects an empty schedule). | ||
| schedule: "0 2 * * *" |
There was a problem hiding this comment.
CloudNativePG's ScheduledBackup resource uses a 6-field cron expression (which includes seconds as the first field), following the Go cron package specification.
Using a 5-field cron expression like "0 2 * * *" will either fail validation by the CNPG validating webhook or be parsed incorrectly (e.g., running every hour at minute 2 instead of daily at 2 AM). Please update the default schedule to a valid 6-field cron expression.
schedule: "0 0 2 * * *"| the recurring base backups. CNPG rejects an empty schedule, so the gate | ||
| keeps invalid manifests off the cluster. | ||
| */}} | ||
| {{- if and .Values.backup.enabled .Values.backup.schedule }} |
There was a problem hiding this comment.
If backup.enabled is set to true but backup.destinationPath is left empty, the Cluster resource (in db.yaml) will not have its backup configuration rendered. However, the ScheduledBackup resource will still be rendered because it only checks backup.enabled and backup.schedule.
Creating a ScheduledBackup for a cluster without any backup configuration will cause reconciliation errors in the CNPG operator. We should ensure that the ScheduledBackup is only rendered when backup.destinationPath is also provided.
{{- if and .Values.backup.enabled .Values.backup.schedule .Values.backup.destinationPath }}| # Scheduled S3 backup of the Keycloak CNPG database (keycloak-db). | ||
| # Disabled by default; enabling it configures WAL archiving + base backups | ||
| # to an S3-compatible object store via CNPG Barman. | ||
| backup: |
There was a problem hiding this comment.
When adding new configuration options to values.yaml, please ensure that values.schema.json is updated as well. You can run make generate from the repository root to automatically regenerate/update the schema file so that these new backup options are validated and exposed in the dashboard UI.
References
- Focus on type correctness, required fields, enum values, and default values in values.schema.json. (link)
Maxim Kitsunoff (kitsunoff)
left a comment
There was a problem hiding this comment.
Verified the rendered manifests against the CNPG v1 schema (chart 0.26.1): spec.backup.barmanObjectStore (destinationPath / endpointURL / s3Credentials / gzip data+wal) and spec.backup.retentionPolicy are correct, and the ScheduledBackup is valid — 6-field cron, backupOwnerReference: self, and the default barmanObjectStore method. The backup.enabled + destinationPath gates are consistent across db.yaml, backup.yaml and backup-secret.yaml, and disabled-by-default keeps existing installs untouched. LGTM.
Non-blocking notes:
- In-tree
barmanObjectStoreis deprecated in CNPG since 1.26 (removal now targeted at 1.31), and the platform already ships a BackupClass-based CNPG backup path. Worth confirming the per-chart approach is intentional for this system component rather than routing it through the BackupClass mechanism. - The branch currently conflicts with
main; needs a rebase before merge.
Wire optional scheduled S3 backups for the keycloak-db CNPG cluster via Barman. When backup.enabled=true with a destinationPath, the Cluster gets spec.backup.barmanObjectStore (gzip-compressed data + WAL, configurable retention) and a ScheduledBackup drives recurring base backups. Credentials are inlined into a Secret or referenced via existingSecretName (AWS_ACCESS_KEY_ID / AWS_SECRET_ACCESS_KEY). Disabled by default. Assisted-By: Claude Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>
…inationPath Address review: CNPG ScheduledBackup.spec.schedule is a 6-field cron (leading seconds field), so the 5-field default was invalid — switch it to "0 0 2 * * *". Also gate the ScheduledBackup on backup.destinationPath to match db.yaml/backup-secret.yaml: schedule has a non-empty default, so the old guard rendered a ScheduledBackup against a Cluster with no object store whenever backups were enabled without a destinationPath. Assisted-By: Claude Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>
dcd3e74 to
18e141f
Compare
IvanHunters
left a comment
There was a problem hiding this comment.
LGTM overall — disabled-by-default (defaults render byte-for-byte identical to the current keycloak-db Cluster, so no upgrade drift), the enabled path renders CRD-valid CNPG resources against the shipped operator (1.27.1), credentials live only in an Opaque Secret referenced by name, and there's no reconcile amplifier or immutability trap. The earlier bot findings (6-field cron, gating the ScheduledBackup on destinationPath) are addressed.
Two non-blocking notes:
retentionPolicyrenders unconditionally when backup is enabled; an explicitretentionPolicy: ""produces a CR the CNPG CRD rejects (pattern^[1-9][0-9]*[dwm]$). It fails loudly and the default30dis fine, but wrapping it in{{- with .Values.backup.retentionPolicy }}would harden it.- This uses CNPG's in-tree
barmanObjectStore, deprecated upstream since 1.26 (still the default in 1.27.1, so fine today). Worth a follow-up to move to the Barman Cloud Plugin.
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>
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>
What this PR does
Adds optional scheduled S3 backups for the
keycloak-dbCloudNativePG cluster.When
backup.enabled=truewith abackup.destinationPath, the Cluster getsspec.backup.barmanObjectStore(gzip-compressed data + WAL streams, aconfigurable
retentionPolicy) and aScheduledBackupdrives recurring basebackups. S3 credentials are either inlined into a rendered
keycloak-db-backup-s3-credsSecret, or referenced viabackup.existingSecretName(a Secret carryingAWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY).Disabled by default — no change to existing deployments; the object store
coordinates and credentials are supplied per-environment by the operator.
This is the Keycloak/Postgres half of backing up a Cozystack portal control
plane. The portal-side etcd backups (system Talos etcd + api-server etcd) live
in the companion downstream PR aenix-org/cozyportal#1008.
Release note
Summary by CodeRabbit