fix(backups): request S3 checksum only when required for barman-cloud (non-AWS S3 / Ceph RGW) - #3417
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
📝 WalkthroughWalkthroughThe change configures barman-cloud ObjectStore sidecars to set ChangesS3 checksum configuration
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: 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 |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
NOT LGTM
The fix only reaches the legacy BYO-credentials backup flow and leaves the platform's own recommended useSystemBucket=true flow (backed by the same class of non-AWS S3 gateway) unpatched, and ships with no regression test — confirmed by mutation: the full helm-unittest suite stays green with the fix fully reverted.
Findings
[MAJOR] internal/backupcontroller/cnpgstrategy_controller.go:438-441 — the fix does not cover the platform-managed default backup flow (backup.useSystemBucket=true), which the chart's own docs call "recommended for new clusters."
The PR sets spec.instanceSidecarConfiguration.env[AWS_REQUEST_CHECKSUM_CALCULATION=when_required] only on the two Helm-template-rendered barmancloud.cnpg.io/v1 ObjectStore objects (packages/apps/postgres/templates/db.yaml, packages/system/keycloak/templates/db.yaml). A third ObjectStore for the same postgres app is constructed entirely in Go by applyClusterPluginBackup (internal/backupcontroller/cnpgstrategy_controller.go:418-441) and SSA-applied under field manager cozystack-cnpg-backup-driver whenever a tenant sets backup.useSystemBucket=true — the flow packages/apps/postgres/templates/backup.yaml's own header comment and packages/apps/postgres/README.md:27 label "Platform-managed flow (recommended for new clusters)". That Go path's typed spec mirror, cnpgtypes.ObjectStoreSpec (internal/backupcontroller/cnpgtypes/types.go:206-211), has no InstanceSidecarConfiguration field at all, and objStore.Spec = cnpgtypes.ObjectStoreSpec{Configuration: ..., RetentionPolicy: ...} never sets one (grepping the file for Sidecar/EnvVar/.Env returns nothing). The platform's own default system-bucket endpoint is http://seaweedfs-s3.tenant-root.svc.cozy.local:8333 (packages/system/backupstrategy-controller/values.yaml:70) — a non-AWS S3-compatible gateway of exactly the class this PR says rejects the flexible checksum. A tenant on the recommended useSystemBucket=true flow gets none of this fix. Fix: extend the Go-side ObjectStoreSpec/buildBarmanObjectStore construction (or set the field directly on objStore.Spec before the SSA patch) to carry the same env var, with coverage in cnpgstrategy_controller_test.go.
[MAJOR] packages/apps/postgres/tests/backup_storage_test.yaml, packages/system/keycloak/tests/ — no regression test protects the fix; confirmed by mutation.
helm unittest . passes 29/29 (postgres) and 45/45 (keycloak) with the fix present. Reverting both template changes to their pre-fix content (git show aef9e201:<path>) and re-running the exact same suites still passes 29/29 and 45/45 unchanged — no assertion in either suite reads spec.instanceSidecarConfiguration. This is a bug-fix PR; the render-layer regression test (helm-unittest) required by the test-adequacy standard is missing. Without it, a future chart-split, helper refactor, or merge conflict can silently drop any of the three env blocks with nothing in CI to catch it. Add an assertion on spec.instanceSidecarConfiguration.env[0] for all three rendered ObjectStore documents (backup + recovery in postgres, the single one in keycloak).
[MINOR] packages/apps/postgres/templates/db.yaml:265-269,300-301, packages/system/keycloak/templates/db.yaml:54-58 — the same 5-line comment plus 3-line YAML block is duplicated verbatim three times across two charts.
A shared named template (in cozy-lib or a per-chart _helpers.tpl) for instanceSidecarConfiguration would make a future change (a second env var, or reusing the literal value from the Go path once Finding 1 is addressed) a one-place edit instead of three.
Claim mismatches
[PARTIAL] "Applied to every barman-cloud ObjectStore this repo renders" / release-note "CNPG barman-cloud backups now work against non-AWS S3-compatible object stores ... No change for AWS S3" — see Finding 1: the useSystemBucket=true-flow ObjectStore, SSA-applied by internal/backupcontroller/cnpgstrategy_controller.go, is untouched, so backups on that (recommended) flow against a non-AWS endpoint are not fixed by this PR despite the unqualified release-note wording.
Caveats
- Whether SeaweedFS's S3 gateway (the platform's own default bucket backend) actually rejects the flexible-checksum header the same way Ceph RGW does was not independently reproduced here (no live cluster in scope for this review); the code-path gap in Finding 1 is verified directly from source, the real-world failure on SeaweedFS specifically is inferred by analogy to Ceph RGW, not tested end-to-end.
- Applying this chart change via
helm upgradeto an existing tenant already onbackup.enabled=true(legacy flow) patches the liveObjectStore; whether that causes CNPG to roll-restart already-running instance pods to pick up the new sidecar env was not verified against CNPG operator source (out of scope for a hermetic, cluster-free review). Expected to be a normal CNPG-managed reconcile, not a hard failure, but not independently confirmed. chart_lintrender_errors/missing_refs on_cluster.scheduling,_namespace.host,registrySecret(from localhelm template) are pre-existing harness artifacts from runtime-injected values not present outside a real cluster, located atpostgres/templates/db.yaml:194— outside the range this PR touches, not introduced by it. Verified by rendering the chart with_cluster/_namespacestubs supplied: all schema-valid corners exercised in this review (legacy backup with inline creds, bootstrap+recovery,useSystemBucket=true, keycloak backup on/off) render valid, admittable YAML including the new field.- Root-cause claim (botocore/boto3 defaulting to a flexible request checksum that non-AWS S3-compatible backends reject) was independently cross-checked against AWS's own S3 documentation ("for uploads, all AWS-owned clients calculate a checksum ... and send it with the upload request") and is consistent with the PR's description; not contradicted by anything found in this review.
Recommended follow-ups
- Extend the checksum fix to the Go-driven
useSystemBucket=trueObjectStore path (cnpgstrategy_controller.go/cnpgtypes.ObjectStoreSpec). - Add helm-unittest coverage for the new env var on all three rendered
ObjectStoredocuments. - Factor the duplicated comment/env block into a shared helper.
The barman-cloud plugin's sidecar uploads via boto3. Since botocore ~1.36 (early 2025) the default RequestChecksumCalculation is when_supported, which attaches a flexible checksum to every PutObject. Non-AWS S3-compatible backends (Ceph RADOS Gateway, some MinIO / Cloudflare R2 builds) reject the accompanying header with "InvalidArgument: x-amz-content-sha256 must be UNSIGNED-PAYLOAD, STREAMING-AWS4-HMAC-SHA256-PAYLOAD or a valid sha256 value", so every backup/WAL-archive upload fails against them. Pin the sidecar to when_required via the ObjectStore's spec.instanceSidecarConfiguration.env (botocore honors the AWS_REQUEST_CHECKSUM_CALCULATION env var) on both the keycloak system DB and the postgres app (backup + recovery ObjectStores). when_required is also accepted by AWS S3 on a plain PutObject, so it is a safe default everywhere. This mirrors the etcd-operator fix (cozystack/etcd-operator#342, released in v0.5.3), which applied the same WhenRequired policy to its aws-sdk-go-v2 S3 client for the same Ceph RGW backend. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <andrey.kolkov@aenix.io>
…hecksum helper Address review on the S3 request-checksum fix: - Extend the fix to the platform-managed useSystemBucket=true flow, whose ObjectStore is built in Go by applyClusterPluginBackup (SSA-applied), not chart-rendered. Add InstanceSidecarConfiguration to cnpgtypes.ObjectStoreSpec and set AWS_REQUEST_CHECKSUM_CALCULATION=when_required on it via barmanSidecarConfiguration(). The platform's own default system bucket is SeaweedFS, a non-AWS S3 gateway of the same class as Ceph RGW. Unit test in cnpgstrategy_controller_test.go asserts the applied ObjectStore carries it. - Factor the duplicated instanceSidecarConfiguration block (3x across two charts) into a shared cozy-lib helper (cozy-lib.barman.checksumSidecarConfiguration). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <andrey.kolkov@aenix.io>
2aff9f0 to
b574e21
Compare
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 `@internal/backupcontroller/cnpgtypes/zz_generated.deepcopy.go`:
- Around line 242-262: Do not edit the generated DeepCopy methods directly.
Update the corresponding InstanceSidecarConfiguration definition in types.go,
then rerun the repository’s deepcopy generator to regenerate
zz_generated.deepcopy.go reproducibly.
🪄 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: 5ecd7f9b-cdde-48ff-9687-faceb033ae5e
📒 Files selected for processing (7)
internal/backupcontroller/cnpgstrategy_controller.gointernal/backupcontroller/cnpgstrategy_controller_test.gointernal/backupcontroller/cnpgtypes/types.gointernal/backupcontroller/cnpgtypes/zz_generated.deepcopy.gopackages/apps/postgres/templates/db.yamlpackages/library/cozy-lib/templates/_barman.tplpackages/system/keycloak/templates/db.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/system/keycloak/templates/db.yaml
- packages/apps/postgres/templates/db.yaml
| if in.InstanceSidecarConfiguration != nil { | ||
| out.InstanceSidecarConfiguration = new(InstanceSidecarConfiguration) | ||
| in.InstanceSidecarConfiguration.DeepCopyInto(out.InstanceSidecarConfiguration) | ||
| } | ||
| } | ||
|
|
||
| func (in *InstanceSidecarConfiguration) DeepCopyInto(out *InstanceSidecarConfiguration) { | ||
| *out = *in | ||
| if in.Env != nil { | ||
| out.Env = make([]EnvVar, len(in.Env)) | ||
| copy(out.Env, in.Env) | ||
| } | ||
| } | ||
|
|
||
| func (in *InstanceSidecarConfiguration) DeepCopy() *InstanceSidecarConfiguration { | ||
| if in == nil { | ||
| return nil | ||
| } | ||
| out := new(InstanceSidecarConfiguration) | ||
| in.DeepCopyInto(out) | ||
| return out |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Regenerate this file instead of editing it directly.
internal/backupcontroller/cnpgtypes/zz_generated.deepcopy.go matches zz_generated.*.go; keep the source change in types.go and rerun the repository’s deepcopy generator so this output remains reproducible.
🤖 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 `@internal/backupcontroller/cnpgtypes/zz_generated.deepcopy.go` around lines
242 - 262, Do not edit the generated DeepCopy methods directly. Update the
corresponding InstanceSidecarConfiguration definition in types.go, then rerun
the repository’s deepcopy generator to regenerate zz_generated.deepcopy.go
reproducibly.
Source: Coding guidelines
…ed ObjectStores helm-unittest coverage for the S3 request-checksum fix (was mutation-negative before): assert spec.instanceSidecarConfiguration.env pins AWS_REQUEST_CHECKSUM_CALCULATION=when_required on all rendered barman-cloud ObjectStores — postgres backup + bootstrap-recovery (packages/apps/postgres/tests/backup_storage_test.yaml) and the Keycloak DB (new packages/system/keycloak/tests/db_backup_test.yaml). Verified by mutation: reverting the template change now fails these assertions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <andrey.kolkov@aenix.io>
|
Thanks IvanHunters — all three findings addressed. [MAJOR] Go-driven [MAJOR] Missing regression test — added in 80a03b1. helm-unittest now asserts [MINOR] Duplication — factored the 3× block into a shared [Claim mismatch] — PR description updated: the Go/ On the SeaweedFS caveat: I set |
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
LGTM
Additive fix reaching all three barman-cloud ObjectStore creation sites in the tree (the Go-driven useSystemBucket=true path plus both chart-rendered paths for postgres and keycloak), with mutation-verified regression tests at the appropriate layer and no upgrade/fresh-install/config-toggle regression identified.
Claim mismatches
[UNVERIFIABLE] "Verification: reproduced end-to-end against a real Ceph RGW endpoint... Deployed the plugin + a throwaway CNPG cluster whose ObjectStore carried this env" — this review is static and cluster-free (no live cluster available), so the live-endpoint repro could not be reproduced or checked. The underlying technical premise was instead independently verified against official documentation (see Caveats).
Caveats
- Existing-customer upgrade (Phase 5b.A):
spec.instanceSidecarConfigurationis an existing, already-vendored field on thebarmancloud.cnpg.io/v1ObjectStoreCRD (packages/system/postgres-operator/charts/plugin-barman-cloud/templates/crds/crds.yaml:416-455) — not itself changed by this PR — with no immutability marker. The addition is purely additive on both the Go-SSA-applied and chart-rendered paths; no migration script ortargetVersionbump is needed and none was added. One real side effect worth naming: every already-running CNPG cluster withbackup.enabled: truewill have this env var applied to itsObjectStoreon the next reconcile after this ships — an expected CNPG-managed rolling restart, not a data-loss or availability regression, but a real fleet-wide side effect of shipping the fix. - Fresh install (Phase 5b.B): no new
PackageSource/bundle wiring, no newvalues.yaml/values.schema.jsonfield (the pin is hardcoded in the shared helper, not user-configurable), no new image reference, no new CRD or cert-manager dependency. Verified:helm templaterenders cleanly on atenant-*namespace withbackup.enabled=trueand produces a schema-validObjectStore. - Config-combination matrix (Phase 5c): the new
{{- include "cozy-lib.barman.checksumSidecarConfiguration" }}calls are unconditional additions inside three pre-existing gates ($renderBarman,.Values.bootstrap.enabled,and .Values.backup.enabled .Values.backup.destinationPath); no new toggle or branch introduced. - Completeness:
grep -rl "barmancloud.cnpg.io/v1"/kind: ObjectStoreacrosspackages/(excluding tests and the vendored plugin chart) return exactly the two touched chart templates;ObjectStoreSpec{ininternal/backupcontroller/*.goreturns exactly the one touched Go site. All three covered; no fourth site found. - Test non-vacuity, verified by mutation: reverting the Go fix turns
TestApplyClusterPluginBackup_PatchesExistingClusterRED; reverting thecozy-libhelper body turns bothpackages/apps/postgresandpackages/system/keycloakhelm-unittest suites RED. Both green on the actual diff.go build/go vetclean;golangci-lintfindings all pre-date this PR. - Technical premise, independently checked: AWS SDK settings reference confirms
AWS_REQUEST_CHECKSUM_CALCULATIONis a real standardized env var, andwhen_requiredonly skips the checksum on operations whose model does not mandate one. Cited precedentcozystack/etcd-operator#342confirmed merged with a matching root cause. - Diff-artifact gap in tooling: bootstrap enumerated 8 of 9 files;
internal/backupcontroller/cnpgtypes/zz_generated.deepcopy.go(+21) was missing from the supplied patch. Reviewed directly: the addedDeepCopyIntoforObjectStoreSpec.InstanceSidecarConfiguration(pointer) and.Env(slice) correctly deep-copies, consistent with the file's hand-written pattern. - New
cnpgtypes.EnvVar(minimal Name/Value, nocorev1import) mirrors the package's existing minimal-type convention — not a deviation.
Recommended follow-ups
- The
useSystemBucket=trueGo path is covered by a Go unit test plus the author's manual Ceph RGW repro; there is no automated e2e/bats exercise of that path against a non-AWS backend. Not blocking, but worth considering for the shared e2e suite given this is the platform-default (SeaweedFS) backup path.
|
Successfully created backport PR for |
|
Created backport PR for
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-3417-to-release-1.5
git worktree add --checkout .worktree/backport-3417-to-release-1.5 backport-3417-to-release-1.5
cd .worktree/backport-3417-to-release-1.5
git reset --hard HEAD^
git cherry-pick -x 0c586b9d72d3c3d05c25d4ed74a5039482472589 b574e2176f099305c97e5a8e4a456f463cb7aef1 80a03b1e09bac7e34b49f5016c405df913302dbb
git push --force-with-lease |
Since botocore ~1.36 (early 2025) the default RequestChecksumCalculation is when_supported, so barman-cloud's boto3 attaches a flexible checksum to every PutObject. AWS S3 accepts it, but non-AWS S3-compatible backends - Ceph RADOS Gateway, the platform's own SeaweedFS system bucket, some MinIO / Cloudflare R2 builds - reject it with "InvalidArgument: x-amz-content-sha256 must be UNSIGNED-PAYLOAD, ...", which fails every backup and WAL-archive upload. Set AWS_REQUEST_CHECKSUM_CALCULATION=when_required through the CNPG Cluster's spec.env, which reaches the instance pods and therefore the barman-cloud subprocess the instance manager execs. This is a release-1.5-native equivalent of #3417 rather than a cherry-pick of it. That change sets spec.instanceSidecarConfiguration.env on barmancloud.cnpg.io ObjectStore objects, and release-1.5 has no barman-cloud plugin: there is no ObjectStore CRD on the branch and the rendered Cluster carries no spec.plugins, so those objects would template cleanly and then fail to apply, referenced by nothing. release-1.5 still uses CNPG's native in-tree spec.backup.barmanObjectStore, which shells out to the same barman-cloud client, so the bug is reachable here - the plugin is the packaging difference, not the cause. One spec.env covers all three paths on this branch at once: the chart-rendered legacy barmanObjectStore, the same field SSA-patched by the CNPG backup driver in the useSystemBucket flow, and externalClusters recovery. No Go change is needed - the driver's patch type carries only spec.backup and spec.bootstrap, so it cannot express or remove the Helm-owned spec.env. keycloak is out of scope on this branch. Its chart renders a CNPG Cluster but configures no backup at all - no backup key in values, no backup template - so it has no barman-cloud path to fix, and the upstream hunk for it references values that do not exist here. Verified against CNPG 1.27.3: spec.env is accepted by the webhook and stored unmodified, appears in the instance manager's own environment, and botocore inside the instance image then resolves request_checksum_calculation as when_required instead of its when_supported default. Adding the field to an already-running Cluster converges back to healthy after CNPG recreates the instance pod, which is the in-place upgrade path for existing releases with backups enabled. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
What this PR does
Fixes CNPG barman-cloud plugin backups to non-AWS, S3-compatible object
stores (Ceph RADOS Gateway, and some MinIO / Cloudflare R2 builds).
The plugin's sidecar uploads via boto3. Since botocore ~1.36 (early 2025)
the default
RequestChecksumCalculationiswhen_supported, so a flexiblechecksum (CRC32) plus the corresponding
x-amz-content-sha256handling isattached to every
PutObject. AWS S3 accepts it, but several S3-compatiblebackends reject the request:
so every backup / WAL-archive upload fails against them and the
ScheduledBackupnever stores anything.This sets
spec.instanceSidecarConfiguration.envon the renderedObjectStores to carryAWS_REQUEST_CHECKSUM_CALCULATION=when_required(botocore reads this env var from the sidecar's environment).
when_requiredcomputes a checksum only for operations that mandate one; a plain
PutObjectthen carries none, which both AWS S3 and the affected backends accept — a safe
default everywhere. No API/CRD change, no new value, no new dependency.
Applied to every barman-cloud
ObjectStoreCozystack creates:packages/system/keycloak/templates/db.yaml(Keycloaksystem DB) and
packages/apps/postgres/templates/db.yaml(postgres app —backup and bootstrap-recovery
ObjectStores), via a sharedcozy-libhelper (
cozy-lib.barman.checksumSidecarConfiguration);useSystemBucket=trueObjectStoreSSA-applied by
internal/backupcontroller/cnpgstrategy_controller.go(
barmanSidecarConfiguration), whose default system bucket is SeaweedFS.Regression coverage: a Go unit test on the SSA-applied
ObjectStore, plushelm-unittest assertions on
spec.instanceSidecarConfiguration.envfor allthree rendered
ObjectStores (mutation-checked — reverting the change failsthem).
Mirrors the same fix already applied to the etcd path in
cozystack/etcd-operator#342(released inv0.5.3) for the same Ceph RGWbackend.
Verification: reproduced end-to-end against a real Ceph RGW endpoint. With
default botocore,
PutObjectfails with theInvalidArgumentabove; withAWS_REQUEST_CHECKSUM_CALCULATION=when_requiredin the environment it succeeds.Deployed the plugin + a throwaway CNPG cluster whose
ObjectStorecarried thisenv: continuous WAL archiving reported
ContinuousArchiving=Trueand anon-demand
Backupcompleted, with the base backup and WAL segments present inthe bucket.
helm templaterenders the env on all threeObjectStores; thepackages/apps/postgresunit-test suite still passes.Screenshots
N/A — no UI changes.
Downstream repositories
Release note
Summary by CodeRabbit
AWS_REQUEST_CHECKSUM_CALCULATION=when_required) to prevent failures with strict S3 gateways.