Repository navigation
fix(backupstrategy-controller): gate the default MongoDB Strategy on its CRD - #3980
Conversation
…its CRD Upgrading a cluster to a build that carries the new MongoDB backup driver wedges the backupstrategy-controller release permanently: Helm upgrade failed: resource mapping not found for name: "cozy-default-mongodb" ... no matches for kind "MongoDB" in version "strategy.backups.cozystack.io/v1alpha1"; ensure CRDs are installed first This chart ships its strategy.backups.cozystack.io CRDs as ordinary templates, and Helm resolves every document in a release manifest through the cluster's RESTMapper in Build() BEFORE applying any of them. A Strategy CR whose CRD is not served yet therefore fails Build(), so the release applies ZERO objects — including the CRD that would have made the mapping resolve — and every retry renders the same manifest. Nothing recovers on its own. The chart already knows about this trap: it is why every default Strategy gates on the bucket name. That gate covers the first install of a fresh cluster, where nothing is resolvable and no Strategy renders at all. It cannot cover shipping a NEW kind to a chart that is already installed, which is what happened here: the bucket name resolved releases ago, so the first upgraded revision renders the new CRD and the new Strategy CR together and wedges. Gate the CR on its CRD being served, via a crdEstablished helper that asks about the CRD OBJECT rather than listing the custom kind — `lookup` on a kind the apiserver does not serve is a render error, not an empty result, which would trade this wedge for a different permanent failure. apiextensions.k8s.io/v1 is always served, so that lookup can only answer yes or no. Established=True rather than mere existence, because that is when the kind enters the RESTMapper. Convergence afterwards is the mechanism the bucket gate already relies on and is NOT the HelmRelease interval: once the CRD is served, the controller's DefaultObjectsGate sees the kind mapped and the CR absent and forces a real Helm upgrade whose render includes it. Kinds that do not map at all are skipped there (meta.IsNoMatchError), which is what stops the gate forcing upgrades against a render that cannot yet produce the object. Found by the release upgrade E2E lane on the v1.6.2 → main path, where `mongodbs.strategy.backups.cozystack.io` is the only definition added since the tag. Verified against a live cluster that is in exactly the pre-upgrade state (seven older strategy CRDs, no mongodbs): `helm template --dry-run=server` of main's chart fails with the error above and produces zero documents, while the same render with this change succeeds with 21 documents and the Strategy correctly withheld. The helper's two branches were probed the same way — a served CRD answers true, an absent one answers empty. The new unit case pins the gate offline through the external-S3 path, where the bucket name resolves without the offline override so only the CRD gate can hold the CR back; removing the gate turns exactly that case red. Assisted-By: Claude <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
github-actions
Bot
added
size/L
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughChangesBackup strategy rendering
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change is localized to gating the MongoDB Strategy until its CRD is served, with no actionable merge-blocking risk remaining after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant Helm as Helm rendering
participant Helper as crdEstablished helper
participant APIServer as Kubernetes apiserver
participant Strategy as MongoDB Strategy template
Helm->>Helper: evaluate bucket override and CRD
Helper->>APIServer: lookup CustomResourceDefinition
APIServer-->>Helper: return Established condition
Helper-->>Strategy: return gate result
Strategy-->>Helm: render or omit Strategy
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (3 skipped: 3 unsupported.) ✨ 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 |
myasnikovdaniil
added
area/storage
Andrey Kolkov (androndo)
left a comment
Andrey Kolkov (androndo)
left a comment
There was a problem hiding this comment.
Verdict: LGTM
Reviewed the change end-to-end (helm-unittest, both gate branches under helm template, a mutation test, and reading the DefaultObjectsGate convergence path in Go). The fix is correct, minimal, and safe on both fresh install and upgrade.
Why it holds. The chart ships its strategy.backups.cozystack.io CRDs as ordinary templates, and Helm resolves every rendered document through the RESTMapper in Build() before applying any of them — so a Strategy CR whose CRD is not yet served fails Build() for the whole release and applies zero objects, including the CRD that would have fixed the mapping. The existing $bucketName gate only covers a fresh install; it cannot cover shipping a new kind to an already-installed chart, where the bucket resolved releases ago. The crdEstablished gate closes exactly that second window. Asking about the apiextensions.k8s.io/v1 CRD object (always served) rather than lookup-ing the custom kind is the right call — a lookup on an unserved kind is a render error, not an empty result, which would trade one permanent failure for another.
Convergence is closed. Once the CRD is Established, DefaultObjectsGate maps the kind, sees cozy-default-mongodb absent, and forces a real Helm upgrade whose render includes it; kinds that don't map yet are skipped via meta.IsNoMatchError, so the gate never forces an upgrade against a render that can't produce the object. The bucketNameOverride short-circuit correctly marks offline renders (helm template, CI, unit tests) where there is no apiserver to ask.
Tests. The new helm-unittest case is load-bearing: it uses provisionBucket=false so $bucketName resolves without the offline override, leaving the CRD gate as the only thing that can withhold the CR — the exact shape of an already-running cluster on the upgrade that first ships the CRD. Reverting the gate turns precisely that case red (count 0 → 1) and nothing else; the suite is 19/19 green with it.
Non-blocking notes
_helpers.tpl— "every default Strategy added from here on should gate on this helper" is a convention, not enforced; a future co-shipped CRD+Strategy added without the gate would re-introduce the same wedge. The right safety net is what already caught this — the upgrade E2E lane onv1.6.2 → main— not a static template-grepping drift test, which wouldn't exercise the realBuild()/RESTMapper contract anyway. No action needed.backupclass-default.yaml— the auto-heal path relies on thecozy-defaultBackupClass routing tokind: MongoDB(present), which the gate reads as the existence manifest. A gated-but-unrouted Strategy would render never. Not applicable here; noting for future kinds.
2 participants
What breaks today
Upgrading any existing cluster to a build that carries the new MongoDB backup driver wedges the
backupstrategy-controllerHelmRelease permanently, withresource mapping not found for name: "cozy-default-mongodb" ... no matches for kind "MongoDB" in version "strategy.backups.cozystack.io/v1alpha1"; ensure CRDs are installed first.This chart ships its
strategy.backups.cozystack.ioCRDs as ordinary templates, and Helm resolves every document in a release manifest through the cluster's RESTMapper inBuild()before it applies any of them. A Strategy CR whose CRD is not served yet therefore failsBuild(), so the release applies zero objects — including the CRD that would have made the mapping resolve — and every retry renders the same manifest. Nothing recovers on its own.Why the gate that exists does not cover it
The chart already knows this trap: it is why every default Strategy gates on the bucket name. That gate covers the first install of a fresh cluster, where nothing is resolvable yet and no Strategy renders at all. It cannot cover shipping a new kind to a chart that is already installed — there the bucket name resolved releases ago, so the first upgraded revision renders the new CRD and the new Strategy CR together and wedges.
The fix
Gate the CR on its CRD actually being served, through a
crdEstablishedhelper. The helper asks about the CRD object rather than listing the custom kind, becauselookupon a kind the apiserver does not serve is a render error, not an empty result, which would trade this wedge for a different permanent failure;apiextensions.k8s.io/v1is always served, so that lookup can only answer yes or no. It requiresEstablished=Truerather than mere existence, because that is when the kind enters the RESTMapper.Convergence afterwards is the mechanism the bucket gate already relies on, and is not the HelmRelease interval: once the CRD is served, the controller's
DefaultObjectsGatesees the kind mapped and the CR absent and forces a real Helm upgrade whose render includes it. Kinds that do not map at all are skipped there (meta.IsNoMatchError), which is what stops the gate forcing upgrades against a render that cannot yet produce the object.bucketNameOverrideshort-circuits the CRD lookup for the same reason it short-circuits the bucket lookup: it marks an offline render —helm template, a CI diff, a unit test — where there is no apiserver to ask. Live deploys go through Flux and never set it.How it was found
The release upgrade E2E lane in #3276, on the
v1.6.2 → mainpath.mongodbs.strategy.backups.cozystack.iois the only definition added to this chart since the tag, so this is reachable only by upgrading an existing install — no install-only suite can see it.Verification
Reproduced and fixed against a live cluster that happens to be in exactly the pre-upgrade state (the seven older strategy CRDs present,
mongodbsabsent).helm template --dry-run=serverof main's chart fails with the error above and produces zero documents; the same render with this change succeeds with 21 documents and the Strategy correctly withheld. The helper's two branches were probed the same way against that cluster: a served CRD answerstrue, an absent one answers empty.The new unit case pins the gate offline through the external-S3 path, where the bucket name resolves without the offline override so that only the CRD gate can hold the CR back. Removing the gate turns exactly that case red; the suite is 19/19 green with it.
Summary by CodeRabbit
Bug Fixes
Tests