fix(backupstrategy-controller): repair lookup-gated backup objects - #3524

sequenceDiagram
  participant DefaultObjectsGate
  participant KubernetesAPI
  participant FluxHelmRelease
  DefaultObjectsGate->>KubernetesAPI: Read credentials and backup configuration
  DefaultObjectsGate->>KubernetesAPI: Check referenced default objects
  DefaultObjectsGate->>FluxHelmRelease: Patch requestedAt and forceAt
  FluxHelmRelease->>KubernetesAPI: Reconcile missing resources

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 @coderabbitai help to get the list of available commands.}

Both findings are correct, and both are fixed in e85501e. I verified each against the code rather than taking them at face value, and they hold.

Unbounded context. Period defaults to one minute and the loop is sequential, so a single stalled API call did stop every later check for the life of the pod. Each check now runs under a timeout of half the Period, capped at 30s, so it can never overlap the next tick. I derived it from Period rather than hardcoding 30s so the invariant "shorter than the interval" also holds for a caller that configures a short period.

NotFound on the source Secret. Right, and it contradicted the doc comment two lines below, which frames an unresolvable bucket as a graceful no-op. An absent Secret and an empty bucket name are the same bootstrap state seen a moment apart, so the absent Secret now takes the same silent path. Other Get errors still propagate.

Two tests added, both failing against the previous behaviour: one reverts to an error on the absent Secret, the other to a timeout equal to Period. go test ./internal/backupcontroller/ passes.

Verdict

NOT LGTM

The static review surfaced one MAJOR code defect on a supported configuration toggle, plus a set of minor issues and unverified live behavior. Details below; every finding cites file:line and, where a corner was rendered, the render result.

Findings

[MAJOR] templates/backupclass-default.yaml:60-72 + templates/strategy-velero-vminstance-default.yaml:2 / templates/strategy-velero-vmdisk-default.yaml:2 + internal/backupcontroller/default_objects_gate.go:318-355 — with velero.bslEnabled=false the gate enters a permanent forced-upgrade loop (one real Helm upgrade every MinForceInterval, forever).

backupclass-default.yaml emits the Velero/cozy-default-velero-vminstance and .../-vmdisk strategyRefs unconditionally, while the Velero Strategy CRs themselves are gated on and .Values.velero.bslEnabled $bucketName. The veleroes.strategy.backups.cozystack.io CRD is always installed (crds.yaml globs definitions/*.yaml), so RESTMapping succeeds (this is not a meta.IsNoMatchError skip), Get returns NotFound, and both objects are counted as missing forever. Check therefore stamps forceAt+requestedAt every interval indefinitely: cozystack_backup_default_objects_missing is pinned at 2 (the documented alert fires forever as a false positive), cozystack_backup_default_objects_force_reconciles_total climbs without bound, and the whole chart is SSA-reapplied with Flux revision churn every minute. bslEnabled=false is a documented, supported opt-out (values.yaml:148-166), not an exotic state.

Rendered corner: helm template . --set velero.bslEnabled=false --set backupStorage.bucketNameOverride=x produces a BackupClass carrying both Velero strategyRefs and zero kind: Velero objects.

Fix: make the set the gate checks equal to the set that actually renders under the current values. Either gate the Velero strategyRefs in backupclass-default.yaml on velero.bslEnabled as well, or (matching the intent stated in values.yaml) drop velero.bslEnabled from the Strategy CR templates so the Velero CRs still ship and only the BSL is skipped.

[MINOR] internal/backupcontroller/default_objects_gate.go:363 — the meta.IsNoMatchError branch in the BSL check is dead. The BSL Get uses the hardcoded backupStorageLocationGVR directly through the dynamic client, bypassing the RESTMapper; against an absent Velero CRD the apiserver returns a 404 (apierrors.IsNotFound, caught on line 361), and NoMatchError is only produced by the RESTMapper, so line 363 is unreachable and its comment ("Velero CRDs absent: nothing to materialise") describes behavior that never happens. Consequence: if Velero is removed after bootstrap while bslEnabled=true, the BSL is counted missing and the gate force-loops (same class as the MAJOR above). Route the BSL lookup through RESTMapper like the strategy path so a genuinely-absent Velero API is skipped rather than force-looped.

[MINOR] packages/system/bucket/templates/user-credentials.yaml:30-36 — the new fail aborts the entire render on the first user whose COSI Secret is absent. For a bucket with users {a, b} where a's Secret exists but b's BucketAccess is stuck, the whole release now fails (retries: -1 → visible Failed) and the healthy a no longer gets its <bucket>-a-credentials Secret until b converges — previously each present user's Secret rendered independently. It converges eventually, but this is a behavior regression for multi-user buckets. Consider failing per-user (or collecting the missing set) rather than aborting on the first.

[MINOR] packages/system/backupstrategy-controller/values.yaml:157-159 — stale comment: it states that with bslEnabled=false the cozy-default-velero-vminstance/-vmdisk strategy CRs "still ship". The render above shows zero such CRs. This is not only doc drift — it is the false premise behind the MAJOR finding: the gate expects those CRs to exist precisely because the comment (and the author's model) assumed they always render.

Claim mismatches

[PARTIAL] PR body / values.yaml:143 — "cozystack_backup_default_objects_missing … is non-zero regardless of the HelmRelease's Ready condition." The gauge is Set in exactly one place, internal/backupcontroller/default_objects_gate.go:287, on the success path. Every earlier return bypasses it: source Secret NotFound (:261), parse error (:270), bucket unresolved (:275), BackupClass get error (:280), missingObjects error (:285). During the pre-bucket bootstrap window and on any persistent Check error the gauge holds its prior value (0 on a fresh pod). The alert covers the post-resolution state (bucket resolved, objects missing → gauge > 0) but not a cluster stuck before the bucket resolves; the "regardless of Ready" wording overstates the coverage.

Operational risks

• Live convergence is unverified (render-blind). The core repair chain — forceAt+requestedAt on an operator-generated HelmRelease actually causing helm-controller to run a Helm upgrade, the cozystack-operator's own reconcile not stripping those annotations before helm-controller consumes them, and the gate detecting readiness within its 1-minute tick — cannot be settled by helm template/go test and was not exercised on a cluster. The PR body already flags this. It should be confirmed on a bootstrap dev cluster before merge.
• Stuck-gate observability is log-only. On a persistent Check error (Forbidden/timeout/malformed source Secret) the error is logged at Info (default_objects_gate.go:233) with no Event and no Ready=False (the Runnable owns no status object), and the missing-gauge freezes at its last value. A gate that silently stops working is detectable only from the log line.

Caveats

• Throttle is not durable across restarts / leader changes. lastForce is in-memory (default_objects_gate.go:138); a new leader or restarted pod has lastForce == 0 and forces immediately on the first missing-check, so the "one force / 5 min" bound does not hold across pod lifecycle churn. Each force is idempotent, so impact is bounded to extra Helm upgrades, visible via force_reconciles_total.
• External-S3 / admin-managed Secret without bucketName → permanent no-op. parseSourceSecret returns bucket == "" and Check no-ops (default_objects_gate.go:272-276); the gate never repairs objects on that path (acknowledged in the PR body).
• Pre-existing, unrelated: packages/system/bucket/templates/ingress.yaml:1 nil-pointers on .Values._namespace.host / ._cluster in an offline helm template (a runtime-injected value). Untouched by this PR; noted only because it blocks whole-chart offline rendering, so the bucket fail corners were validated via the isolated helm-unittest suite instead.

Recommended follow-ups

• Run an end-to-end test on a bootstrap dev cluster to confirm: (1) the gate forces and the Strategy CRs + cozy-default BSL appear within a minute of the bucket becoming ready; (2) the <bucket>-system release with the new fail converges (retries then renders) rather than parking as Failed; (3) the cozystack-operator reconcile does not strip reconcile.fluxcd.io/forceAt/requestedAt before helm-controller consumes them.

Verdict

NOT LGTM. The DefaultObjectsGate core is sound and well-tested, but the bucket chart's new render-fail changes behaviour on a universal path (every bucket, every cluster) in a way that was not verified on a live cluster, and it regresses blast radius for multi-user buckets with no per-bucket escape.

Findings

[MAJOR] packages/system/bucket/templates/user-credentials.yaml:35 — the fail aborts the entire <bucket>-system release, and one bad user wedges a multi-user bucket permanently.

The fail sits inside range $name, $user := .Values.users (line 30). A single declared user whose COSI Secret is absent aborts the whole release render, which also carries deployment.yaml, service.yaml, ingress.yaml, httproute.yaml. Two concrete regressions vs the prior per-user silent skip:

• Fresh install of a multi-user bucket where one user's BucketAccess never provisions (misconfigured bucketAccessClassName, COSI failure): the whole bucket — the other users' Secrets and the serving Deployment/Ingress — never comes up, instead of just that one user's Secret being missing.
• Existing multi-user bucket, add a second user that never provisions: the release parks in Failed and every future legitimate upgrade of that release is blocked.

There is no per-bucket escape (see the next finding), so the operator cannot unwedge one bucket without touching cluster-wide state. The PR body justifies why fail is safe (retries converge, no self-deadlock) but does not surface this all-or-nothing / multi-user tradeoff. For the common single-user case (including the platform cozy-backups bucket) the change is an improvement; the regression is the multi-user-partial-failure corner.

Fix options: scope the failure per-user (render the resolvable users, signal the unresolved one — Helm fail cannot render a partial set, so this likely means a per-user status signal instead of a fatal fail), or expose requireUserCredentials on the parent apps/bucket chart so a single bucket can opt out.

[MAJOR] packages/apps/bucket/templates/helmrelease.yaml:23-25 + packages/system/bucket/values.yaml:9 — the requireUserCredentials escape hatch is unreachable per-bucket.

The parent HR passes only bucketName and users; the apps/bucket chart exposes no knob for requireUserCredentials, and editing the child HR by hand is reverted by the parent's next render. So the only levers for the wedged tenant above are fixing COSI, or setting requireUserCredentials: false in the cluster-wide cozystack-values Secret (valuesFrom), which silently disables the loud-failure guarantee for every bucket on the cluster. The values.yaml comment also states the flag is "only for offline renders", which contradicts using it as a recovery lever.

Additionally: this is a behaviour change on the fresh-install path of every bucket (silent-skip → transient Failed until COSI writes the Secret). The PR's own "Not verified" section asks a reviewer to confirm the fail converges on a bootstrap cluster rather than parking the release. That is render-blind (live helm-controller timing + install-gate behaviour) and cannot be cleared by a static render; it needs a live-cluster run before merge.

[MINOR] internal/backupcontroller/default_objects_gate.go:287 — the cozystack_backup_default_objects_missing gauge is only set on the happy path, so the documented alert is overstated.

Check returns before the Set at line 287 on an absent source Secret (line 264), an empty bucket name (line 275), or a BackupClass/per-object Get error. docs/operations/backup-classes.md:148 claims the gauge is "non-zero whatever the HelmRelease's Ready condition says" and recommends alerting on it. But if the source Secret is deleted later (credential rotation) while objects are broken, the gauge freezes at its last value (0 → alert never fires; or N → stuck non-zero that never clears even after recovery). Either set the gauge in a defer covering every return, or soften the doc claim to state the alert only tracks the resolved-bucket steady state.

[MINOR] internal/backupcontroller/default_objects_gate.go:280 (forceHelmRelease) — a suspended HelmRelease produces a perpetual force loop and a misleading counter.

The gate patches forceAt/requestedAt without checking spec.suspend. cozyhr suspend sets spec.suspend: true (standard dev workflow); helm-controller then ignores the annotations, so while objects are missing the gate re-patches every MinForceInterval forever and cozystack_backup_default_objects_force_reconciles_total climbs. docs/operations/backup-classes.md:149 attributes a climbing counter to "the forced render is not producing the objects (a missing CRD)", pointing the operator at the wrong diagnosis. A point Get of the HR (RBAC already present) before patching, skipping/logging when suspended, closes both.

Caveats

• No live-cluster verification (static review). The gate's recovery mechanism was verified by reading: the HR is named backupstrategy-controller/cozy-backup-controller by the operator (internal/operator/package_reconciler.go:243-263; buildHelmReleaseSpec sets no spec.releaseName), so .Release.Name matches the patched HR; leader election is on (--leader-elect, replicas: 2) and the gate is a LeaderElectionRunnable; the Secret cache is disabled (cmd/backupstrategy-controller/main.go:170-171); remediation.retries: -1 is set on the -system HR (packages/apps/bucket/templates/helmrelease.yaml:16,19). SSA / admission / live install timing remain render-blind.

Checked and correct

• Velero Strategy CRs gate on velero.bslEnabled (strategy-velero-vm{instance,disk}-default.yaml:2), so the gate's kind == "Velero" skip and the values.yaml comment are consistent — no force-loop, no doc drift.
• Gate missingObjects aligns with the chart render gates: all routed strategies gate on $bucketName, which the gate's readiness signal tracks.
• No migration needed (controller-driven fix).
• go build / go vet clean; Go tests and both helm-unittest suites green; test fixtures faithful to the production cozy-default BackupClass.

All four confirmed against the code. Pushed as f859b71.

The two MAJORs: the fail is gone, not mitigated

You are right that fail inside range is all-or-nothing, and I had not weighed what else rides in that release — deployment.yaml, service.yaml, ingress.yaml, httproute.yaml go down with it. Neither fix option you offered actually closes it, though:

• Scope the failure per-user — Helm cannot render a partial set, as you note. A non-fatal per-user status signal also gives up the only thing the fail was buying, which is a retry that re-runs the lookup.
• Expose requireUserCredentials on apps/bucket — that is an unwedge lever, but it puts a Helm implementation detail into the public app API (schema, Go types, the Terraform provider), and it only helps an operator who already knows to reach for it.

So I dropped the fail and moved the repair into the gate, which is the mechanism you already found sound.

The gate reads bucket-cozy-backups-system-credentials to resolve the bucket name, and previously did nothing when it was absent — that is the case the chart fail existed to cover. It now forces the bucket-<name>-system release instead. Same annotation pair, same throttle, and per-object rather than per-release, so nothing else in that release is affected and no escape hatch is needed.

Two preconditions I checked rather than assumed:

• No self-deadlock. The BucketClaim and the BucketAccess whose COSI Secret the lookup reads are rendered unconditionally by the parent release (packages/apps/bucket/templates/bucketclaim.yaml — no lookup, no gate), so forcing the child cannot block its own precondition.
• No RBAC delta. patch on helmreleases is already granted through a ClusterRole, so reaching tenant-root costs nothing new.

The two releases get independent throttles: on a bootstrap they are forced in sequence (credentials, then the objects the resolved bucket unblocks), so one shared timestamp would have delayed the second by a full MinForceInterval for no reason. Coordinates are plumbed from the chart under provisionBucket, and omitted on external S3 where the Secret is admin-managed and no release renders it.

The chart is back to a partial render, with the comment now stating why the skip is not self-healing and why failing is the wrong lever. Added a case that renders the bucket UI with every user unresolvable, so a reintroduced fail fails the suite rather than being caught in review again.

Scope this narrows, deliberately: tenant buckets keep the pre-existing silent skip. The gate only owns the platform bucket, and that is the cluster-breaking path (projector → every strategy → Velero → migration 50). Generalising it needs a controller that owns tenant buckets, which is a separate change — happy to file it if you want it tracked.

MINOR: stale gauge

Both branches you name were real. Fixed by making the unresolved path a first-class result instead of an early return: the credentials Secret is now counted in missing, which it always was in substance, so credential rotation moves the gauge to 1 rather than freezing at 0.

On API errors I kept the gauge deliberately unwritten — flapping the alert on a transient apiserver error is worse than a stale value — so I added cozystack_backup_default_objects_check_errors_total to mark it stale, and the doc now pairs the two instead of claiming the gauge alone is sufficient. force_reconciles_total also carries namespace/name now, since two releases can be forced.

MINOR: suspended HelmRelease

Fixed as you described: point Get before the patch, skip and log on spec.suspend: true, and no counter increment — so a climbing counter keeps meaning "the forced render is not producing the objects". Documented that a release left suspended is never repaired until resumed.

Verification

go build, go vet, go test ./internal/..., make helm-unit-tests, make generate (no diff), make rd-presets-check test-check-readiness migrations-target-check — all green. 6 new Go cases (force on absent Secret, force on empty bucket name, external-S3 reports-but-does-not-force, independent throttles, suspend skip on each release) and 3 new helm cases (bucket release coordinates, that they track a renamed bucket together with systemSecretName, and unwired on provisionBucket=false).

Still no live-cluster run — I don't have a bootstrap cluster to hand. What that gap covers is now much smaller, which was the point of taking this route: the universal behaviour change on every bucket's install path is gone, so there is no longer a render-blind fail whose convergence has to be observed. What remains unobserved is the same gate mechanism you already reviewed, applied to one more release. The claim that a forced upgrade of bucket-<name>-system re-runs the COSI lookup and materialises the Secret is the documented forceAt+requestedAt behaviour and matches the manual recovery in the PR body, which is a procedure that has been run by hand on the reporter's cluster — but it is read code, not an observed install, and I'd rather say so than imply otherwise.

I'll update the PR body next; section 2 still describes the fail.

CI note — the two red checks are not from this diff.

• Unit & controller tests fails on hack/cozyreport.bats: FAIL: the set of unconverted EXIT-trap files changed, because hack/run-kubernetes-talos-diagnostics_test.bats (added by cfbfd77, "fix(e2e): bound the tenant Talos capture kubectl calls") carries 8 EXIT traps and is not in the frozen list at hack/cozyreport.bats:2554. That list is the one fix(tests): freeze the EXIT-trap count for multus-install-cni-plugins.bats #3584 last updated for multus. It reproduces on every open PR right now — fix(apps/kubernetes): report the pre-delete cleanup outcome honestly #3592, test(tests): pin the keep annotation on cert-manager and etcd-operator CRDs #3590 and fix(kubernetes): keep tenant addon CRDs when a release is uninstalled #3586 all fail the same check — and does not show on main because the guard only runs in the pull_request workflow. Nothing in this PR touches hack/. A one-line freeze-list update, following the fix(tests): freeze the EXIT-trap count for multus-install-cni-plugins.bats #3584 precedent, unblocks all of them.
• Build Talos is the usual fork-PR registry push: denied: Anonymous users are only allowed read access on public repos. It failed identically on 8b00639, the previous head.

Everything else is green: pre-commit, DCO, CodeQL, Analyze (go), API owner review, Plan build, size, label.

Successfully created backport PR for release-1.6:

• [Backport release-1.6] fix(backupstrategy-controller): repair lookup-gated backup objects #3731

Created backport PR for release-1.5:

• [Backport release-1.5] fix(backupstrategy-controller): repair lookup-gated backup objects #3880 with remaining conflicts!

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin backport-3524-to-release-1.5
git worktree add --checkout .worktree/backport-3524-to-release-1.5 backport-3524-to-release-1.5
cd .worktree/backport-3524-to-release-1.5
git reset --hard HEAD^
git cherry-pick -x 51ad22670684b7bc3b7b13bb54403e1aecedc997 e85501e9f5d3dc6effe5e8721b5db073af59bdb4 aa3accd12800fd977678d771c719e89fd064ff5c bf152ba8640da08fd77f486eaa7c4714c6ba6a9d 8b006391c2ba89280096dcee8fd7f85dd3a41c7d f859b710a45d7df4bd255991ab1d980b048d352a
git push --force-with-lease