fix(platform): forward backupStorage overrides to the backupstrategy-controller Package

[Andrey Kolkov (androndo)]

What this PR does

Fixes #3245.

The documented admin override for the cozy-default backup S3 coordinates — spec.components.backupstrategy-controller.values on the Package cozystack.cozystack-platform (docs/operations/backup-classes.md, "Admin overrides for cozy-default") — had no effect: the cozystack-platform PackageSource exposes exactly one component (platform), so the operator silently ignored the block, and the platform chart rendered the child Package cozystack.backupstrategy-controller via the .default helper with no components argument, so nothing forwarded backupStorage either. Admins running against an external or TLS S3 had no working supported path to set backupStorage.endpoint / region / systemSecretName.

This PR wires the override through the platform chart, the same way bundles/iaas.yaml forwards kubevirt and gpu-operator values:

• packages/core/platform/values.yaml gains a top-level backupStorage: {} value.
• templates/bundles/system.yaml renders it into the emitted Package cozystack.backupstrategy-controller as components.backupstrategy-controller.values.backupStorage, from where the cozystack operator merges it into the backupstrategy-controller HelmRelease over the chart defaults. When backupStorage is unset the components block is omitted and the emitted Package is byte-identical to the historical render.
• docs/operations/backup-classes.md now documents the working path (spec.components.platform.values.backupStorage) and explicitly calls out the two plausible-looking paths that do not work (the old documented one, and patching the child Package directly, which the platform helm-reconcile reverts).
• A new helm-unittest suite (packages/core/platform/tests/bundles_backupstorage_wiring_test.yaml) is the behavioural regression test: it proves the unset case emits no components block and the set case lands the values on the child Package, on both isp-full and isp-full-generic.

Verified with make test in packages/core/platform (22 suites, 91 tests, all green). make show requires a live cluster (cozyhr + lookup in templates/repository.yaml), so the unittest render of templates/bundles/system.yaml is the offline verification path.

Screenshots

Not a UI change.

Downstream repositories

• No downstream repository is affected by this change
• cozystack/website - follow-up: fix(backup): simplify UX with default backupclass and correct the admin override path (next + v1.5) website#622
• cozystack/terraform-provider-cozystack - follow-up:
• cozystack/ansible-cozystack - follow-up:
• cozystack/ccp - follow-up:
• cozystack/talm - follow-up:
• cozystack/cozyhr - follow-up:
• cozystack/cozy-proxy - follow-up:
• cozystack/cozystack-telemetry-server - follow-up:
• cozystack/external-apps-example - follow-up:
• cozystack/examples - follow-up:

Walking the trigger map against the diff: the change to packages/core/platform/values.yaml reaches cozystack/website twice — content/en/docs/next/operations/configuration/platform-package.md (hand-written table of spec.components.platform.values.* keys gains backupStorage) and content/en/docs/next/operations/services/backup-classes.md (mirrors the broken override example this PR corrects). The follow-up is cozystack/website#622, which also carries the superseded website#547 content and covers the next/ and v1.5/ docs (release-1.4 does not ship the cozy-default feature, so v1.4 docs stay on the manual flow), marked not to merge before this PR lands; its v1.5 half assumes this fix is backported to release-1.5, hence the backport label. No other downstream repository restates the touched files: the templates change is chart-internal, no app values.schema.json, CRD, variant list, hack/ file, or namespace changed.

Release note

fix(platform): the documented admin override for the cozy-default backup S3 coordinates now works — set spec.components.platform.values.backupStorage on the cozystack.cozystack-platform Package and the platform chart forwards it to the backupstrategy-controller component (previously the documented spec.components.backupstrategy-controller path was silently ignored)

Summary by CodeRabbit

• New Features

  • Added a platform-level backupStorage configuration block that is conditionally forwarded to the backup controller.
  • The backup controller spec.components is only set when backupStorage is provided; defaults remain unchanged when unset.

• Documentation

  • Updated backup-class instructions for system-managed bucket naming and refined Velero/BSL, Postgres placeholder-credentials, FoundationDB cleanup notes, observability alerts, and admin override examples.

• Tests

  • Added regression tests to verify backupStorage wiring for both the standard and isp-full-generic variants.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

The platform chart adds a top-level backupStorage configuration, forwards configured values to the child cozystack.backupstrategy-controller Package, documents the supported override path, and adds regression tests for default and variant-specific rendering.

Changes

Backup storage forwarding

Layer / File(s)	Summary
Override contract and documentation packages/core/platform/values.yaml, docs/operations/backup-classes.md	Defines the top-level backupStorage values block, documents overrides through the platform component of the cozystack.cozystack-platform Package, and clarifies bootstrap, observability, and upgrade behavior.
Child package value forwarding packages/core/platform/templates/bundles/system.yaml	Conditionally renders spec.components.backupstrategy-controller.values.backupStorage when platform backup storage overrides are configured.
Forwarding regression coverage packages/core/platform/tests/bundles_backupstorage_wiring_test.yaml	Verifies omission without overrides and forwarding for isp-full and isp-full-generic variants.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• cozystack/cozystack#3270: Updates backup strategy controller handling of provisioned S3 endpoints used by the forwarded storage configuration.

Suggested labels: area/testing

Suggested reviewers: lllamnyp, sircthulhu, kvaps, myasnikovdaniil, ivanhunters

Sequence Diagram(s)

sequenceDiagram
  participant PlatformPackage
  participant PlatformChart
  participant BackupStrategyPackage
  PlatformPackage->>PlatformChart: provide platform backupStorage values
  PlatformChart->>BackupStrategyPackage: render forwarded component values
  BackupStrategyPackage->>BackupStrategyPackage: apply backupStorage values

Loading

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	The docs also change FoundationDB, observability, and upgrade-note content unrelated to the backupStorage wiring objective.	Split unrelated documentation tweaks into a separate PR so this change stays focused on the backupStorage override fix.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: forwarding backupStorage overrides to the backupstrategy-controller Package.
Linked Issues check	✅ Passed	The platform chart now forwards backupStorage into the child Package, and tests/docs cover the supported override path from #3245.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/3245-backupstorage-override

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[gemini-code-assist]

Summary of Changes

Hello, I'm Gemini Code Assist¹! 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 resolves an issue where administrative overrides for cozy-default backup S3 coordinates were being ignored. By updating the platform chart to explicitly forward backupStorage values to the backupstrategy-controller component, admins can now successfully configure external S3 endpoints, regions, and secrets. The changes ensure that these overrides are correctly merged into the HelmRelease while maintaining backward compatibility for default configurations.

Highlights

• Configuration Forwarding: Implemented a mechanism to forward backupStorage overrides from the cozystack-platform Package to the backupstrategy-controller component.
• Documentation Update: Updated backup-classes.md to reflect the correct override path and clarify why previous attempts were unsuccessful.
• Regression Testing: Added a new helm-unittest suite to verify that backupStorage values are correctly propagated to the child Package.

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 Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	Gemini (@gemini-code-assist)	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request implements forwarding of backupStorage overrides from the cozystack-platform Package CR down to the backupstrategy-controller Package CR. It updates the platform chart templates to dynamically inject these values when configured, adds a comprehensive suite of Helm unit tests to verify the wiring behavior, and updates the documentation and default values.yaml to reflect the new configuration path. Since there are no review comments, no additional feedback is provided.

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.

[IvanHunters]

Verdict

LGTM

Every claim in the PR body was independently verified against the operator source, the chart helpers, and an offline byte-level render comparison; no blocking issues found on either the upgrade or the fresh-install scenario.

Caveats

• Existing-customer upgrade (Phase 5b-A), verified no breakage. With backupStorage unset (the shipped default {}), the emitted Package cozystack.backupstrategy-controller is byte-identical to the historical render. Verified two ways: (1) the new unittest asserts notExists: spec.components (packages/core/platform/tests/bundles_backupstorage_wiring_test.yaml:94-95), and (2) an isolated helm template harness rendering the old cozystack.platform.package.default call vs the new 4-arg cozystack.platform.package call with empty dict produced a zero-byte diff (empty dict is falsy at templates/_helpers.tpl:20, so the components: block is skipped). No migration script is needed and migrations.targetVersion is correctly untouched: no cluster-side state changes, no renames, no adoption. Customers who previously worked around #3245 by patching the child Package directly see the same revert-on-reconcile behaviour as before when the new value is unset. Rollback is clean and two-way: downgrading drops the components block and the child HR reverts to chart defaults.
• Fresh install (Phase 5b-B), verified no breakage. No new PackageSource, bundle entry, dependsOn, _cluster/_namespace key, image, or CRD. helm.sh/resource-policy: keep is preserved because the direct helper call goes through the same cozystack.platform.package define that stamps it (templates/_helpers.tpl:17, confirmed in the set-case render). The forwarding block sits in the variant-agnostic tail of templates/bundles/system.yaml:219-233 (after all bundles.system.variant conditionals), so it applies uniformly to isp-full, isp-full-generic, and isp-hosted.
• Full value-flow chain verified end-to-end: the operator-generated cozystack-platform PackageSource exposes exactly one component named platform for every variant (cmd/cozystack-operator/main.go:692-702), which confirms both the bug diagnosis (the old documented spec.components.backupstrategy-controller path matched no component and was silently dropped by the reconciler loop at internal/operator/package_reconciler.go:197-204) and the fix path (spec.components.platform.values.backupStorage lands on the platform HR via package_reconciler.go:292-293, the chart forwards it into the child Package, and the child PackageSource's backupstrategy-controller component does carry an install: block at packages/core/platform/sources/backupstrategy-controller.yaml, so the reconciler consumes the forwarded values rather than dropping them).
• Every knob named in the values.yaml comment (provisionBucket, bucketName, endpoint, region, forcePathStyle, systemSecretName, systemNamespaces) exists in packages/system/backupstrategy-controller/values.yaml.
• Reproduced the test claim: helm unittest . in packages/core/platform passes 22 suites / 91 tests, matching the PR body exactly.
• The bootstrap chart-lint render error (repository.yaml OCIRepository lookup requires a live cluster) and the .Values.registries.* missing-refs are pre-existing chart-wide conditions (registries: {} at values.yaml:483), not introduced by this PR.
• The new tests cover isp-full and isp-full-generic but not isp-hosted. Not blocking: the forwarding code is outside the variant conditionals, so the code path is identical; noted only for completeness.

[coderabbitai]

Actionable comments posted: 2

🤖 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 `@docs/operations/backup-classes.md`:
- Line 107: Update the backup credential alert recommendation to detect stale
successes using counter activity rather than absent_over_time. Scope an
increase() check to each expected namespace, or use an exposed last-success
gauge/heartbeat, while retaining the existing failure-rate alert.
- Line 164: Update the external S3 instructions to use the supported platform
Package configuration path, `spec.components.platform.values.backupStorage` on
`cozystack.cozystack-platform`, instead of direct `backupstrategy-controller`
values. Preserve the existing guidance for `provisionBucket`, source
credentials, `endpoint`, and `region`, and clarify that only the `backupStorage`
block is forwarded.

🪄 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: d8140cf1-5d6b-4dbc-b8bd-feae2ae5bee9

📥 Commits

Reviewing files that changed from the base of the PR and between 94757b3 and aede89a.

📒 Files selected for processing (1)

• docs/operations/backup-classes.md

[coderabbitai]

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Use counter activity, not absent_over_time, for stale-success detection.

absent_over_time() only fires when Prometheus has no samples for the metric; an unchanged success counter remains present and will not trigger it. It also does not detect a stale individual namespace when another namespace continues succeeding. Use increase() scoped to the expected namespace(s), or expose a last-success gauge/heartbeat instead.

🤖 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 `@docs/operations/backup-classes.md` at line 107, Update the backup credential
alert recommendation to detect stale successes using counter activity rather
than absent_over_time. Scope an increase() check to each expected namespace, or
use an exposed last-success gauge/heartbeat, while retaining the existing
failure-rate alert.

[IvanHunters]

Verdict

LGTM with non-blocking notes

The fix is minimal, matches the established bundles/iaas.yaml forwarding idiom, and ships a non-vacuous regression test; the only open points are downstream merge semantics that can only be proven by precedent from this chart's vantage.

Findings

No blocking findings. Verification performed:

• Helper signature (packages/core/platform/templates/_helpers.tpl:1-24) — cozystack.platform.package takes (list name variant $root $components) and emits spec.components only when $components is non-empty. The new call (list "cozystack.backupstrategy-controller" "default" $ $backupStrategyComponents) at templates/bundles/system.yaml:233 is correct against that signature.
• Unset-path identity (upgrade safety) — templates/bundles/system.yaml:230-232 gates population of $backupStrategyComponents behind {{- if .Values.backupStorage -}}; the default backupStorage: {} (values.yaml:426) is falsy in Helm, so the dict stays empty and the helper omits spec.components. Confirmed by executed unittest (tests/bundles_backupstorage_wiring_test.yaml, test 1 asserts notExists: spec.components). An existing cluster that never set backupStorage gets a byte-identical Package cozystack.backupstrategy-controller CR on upgrade — no state migration needed.
• Test non-vacuity (mutation) — reverting system.yaml:230-233 to the old cozystack.platform.package.default call turns the 3 "forwards" assertions RED (unknown path spec.components.backupstrategy-controller.values.backupStorage.*) while the unset/notExists test correctly stays green. The suite genuinely guards the fix.

Caveats

• Deep-merge of a partial override is verified by precedent, not executed. The real use case (admin sets only backupStorage.endpoint) is correct only if the operator + Helm value-coalescing deep-merges the forwarded partial map over the child chart's backupStorage defaults (packages/system/backupstrategy-controller/values.yaml:29-42: provisionBucket: true, namespace: tenant-root, bucketName: cozy-backups). A shallow replace would drop those keys and break bucket provisioning. This chart's helm-unittest can only assert the block lands in the emitted Package CR — the actual merge happens downstream (operator → child HR → Helm coalesce) and was not exercised here. It is the identical mechanism already used by the kubevirt / gpu-operator partial-value forwarding in bundles/iaas.yaml:116-149, which works in production, so I treat deep-merge as proven-by-precedent. Uncovered corner: "partial backupStorage override preserves child defaults" is not (and cannot be) asserted in this suite.
• Override surface is intentionally partial. Only the backupStorage block is forwarded; velero.bslEnabled and any other child-chart value are not reachable via this path. This is explicitly documented (docs/operations/backup-classes.md, "Disabling the platform-managed bucket" note), so the tradeoff is surfaced, not hidden — noted, not blocking.
• Phase 5c corners rendered — the template branches only on .Values.backupStorage. Corners rendered via helm-unittest: unset ({} default → no components), set on isp-full, isp-full-generic, and isp-hosted (all forward the block). No dependsOn, no mode/enum, no mutually-exclusive fields, no immutable-object transition introduced. All corners admittable.
• No make generate artifacts due — this chart ships no values.schema.json or README.md, so the additive backupStorage: {} value needs no regenerated sibling. Release-note block present; title follows Conventional Commits.
• Bundled unrelated doc edits in docs/operations/backup-classes.md (FDB CR apiGroup foundationdb.org → apps.foundationdb.org, S3 bucket-naming wording, observability metric names, "pre-PR" → "pre-v1.5", and the reconcile-interval claim "Flux default 10m" → "cozystack operator helmrelease-interval, 5m default"). These are same-file corrections adjacent to the feature; the 5-minute-interval attribution is a factual claim I did not verify against operator source — low risk, doc-only.

Recommended follow-ups

• Consider a short comment on the backupStorage value block (or a one-line note in the PR) that partial-override correctness rests on Helm map coalescing on the child HR, so a future maintainer knows the forwarding is deliberately a whole-block pass-through and the defaults live in the child chart.