feat(EN-1353): default volume deletion protection to on (opt-out)#1480
Conversation
Flip both opt-in layers of ledger PVC/PV deletion protection to default-ON, so a freshly deployed ledger is protected without extra configuration and must be explicitly opted out instead of opted in. - helm pvcProtection.enabled: false -> true (installs the cluster-scoped ValidatingAdmissionPolicy by default). - spec.persistence.deletionProtection: bool(default false) -> *bool(default true). Pointer so an omitted field defaults to true while an explicit false still opts out. Add PersistenceSpec.DeletionProtectionEnabled() (nil -> true) and route all consumers through it; hash.go zeroes it to nil. - Regenerate CRDs (config + chart) and deepcopy; update README and values docs from opt-in to opt-out, including the multi-release singleton caveat (all but one release must set pvcProtection.enabled=false). Reverse of formancehq/ledger-v3-poc#567.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
💬 Comments — automated reviewThe PR correctly implements opt-out semantics for volume deletion protection across API types, controller helpers, CRD regeneration, hash exclusion, and Helm gating. All modified lines are covered by tests. The only finding is a documentation gap in the Helm NOTES.txt: the opt-out guidance tells operators to set the Helm-level flag to suppress the DeletionProtectionInactive warning, but omits the necessary per-CR opt-out ( |
…atingAdmissionPolicy Defaulting pvcProtection.enabled to true broke helm install/upgrade on Kubernetes < 1.30, where the ValidatingAdmissionPolicy kind does not exist (the API server rejects the rendered objects with 'no matches for kind'). - Gate the VAP/VAPBinding template on .Capabilities.APIVersions.Has "admissionregistration.k8s.io/v1/ValidatingAdmissionPolicy" so an old cluster silently skips the objects and the default install succeeds. - Add NOTES.txt warning when protection is enabled but the API is absent, so the degrade-to-unprotected is visible (upgrade to >=1.30 or set enabled=false). - Clarify deletionProtectionPolicyInstalled: the probe's IsNotFound branch also covers old clusters (unregistered resource answers GET with 404), so reconcile reports not-installed instead of erroring on every pass. - Update README/values docs: old clusters auto-skip (install succeeds) rather than fail; note --api-versions for offline helm template.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release/v3.0 #1480 +/- ##
================================================
- Coverage 74.01% 73.95% -0.06%
================================================
Files 388 388
Lines 39676 39734 +58
================================================
+ Hits 29366 29387 +21
- Misses 7797 7831 +34
- Partials 2513 2516 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
- deletion_protection_test.go: probe reports not-installed on a bare 404 from an unregistered ValidatingAdmissionPolicy API (K8s < 1.30) via IsNotFound's code-404 fallback, and propagates any other API error instead of swallowing it. - api/v1alpha1 persistence_test.go: DeletionProtectionEnabled() resolves nil to true (protected), pinning the opt-out default; explicit true/false honored. - justfile test-helm-render: cluster-free check that the VAP is skipped when the ValidatingAdmissionPolicy capability is absent and rendered when present, guarding the default pvcProtection.enabled=true against breaking old-cluster installs.
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 1 new inline finding.
Summary: #1480 (comment)
3 participants
Summary
Flips the ledger PVC/PV deletion protection (originally shipped opt-in in
formancehq/ledger-v3-poc#567) to default-ON, explicit opt-out — the reverse of today. A freshly deployed ledger is now protected without any extra configuration; both layers must be explicitly turned off to opt out.Jira: https://formance-team.atlassian.net/browse/EN-1353
Changes
helm/operator/values.yaml:pvcProtection.enabledfalse → true. The cluster-scopedValidatingAdmissionPolicyinstalls by default.spec.persistence.deletionProtection:bool(+kubebuilder:default=false) →*bool(+kubebuilder:default=true). The pointer is what makes "default true, but an explicitfalsestill opts out" correct — a plainbool+omitemptycan't distinguish unset from false.PersistenceSpec.DeletionProtectionEnabled()(nil → true) so in-process construction and the runtime resolve the default consistently; all consumers route through it.hash.gozeroes the field tonil.config/crd/bases+helm/crds/templates, both nowdefault: true) and deepcopy viacontroller-gen.values.yamlcomments from opt-in to opt-out.Old-cluster safety (K8s < 1.30)
ValidatingAdmissionPolicyis GA only in Kubernetes >= 1.30. DefaultingpvcProtection.enabledto true would otherwise breakhelm install/upgradeon older clusters ("no matches for kind ValidatingAdmissionPolicy"). Fixed by:.Capabilities.APIVersions.Has "admissionregistration.k8s.io/v1/ValidatingAdmissionPolicy"— an old cluster silently skips the objects and the default install succeeds. (helm templateoffline: pass--api-versions admissionregistration.k8s.io/v1/ValidatingAdmissionPolicyto force-render.)NOTES.txtwarning when protection is enabled but the API is absent, so the degrade-to-unprotected is visible (upgrade to >=1.30, or setenabled=false).IsNotFoundmatches), so reconcile does not error on every pass — clarified in a comment.Verified with
helm template: 0 VAP objects rendered without the capability, 4 with it.Multi-release caveat worth reviewing
Making the Helm layer default-ON interacts with the singleton constraint from #567: the cluster VAP objects have fixed, release-independent names, so a second operator release in the same cluster now fails its
helm install/upgradeby default (ownership conflict). In multi-release clusters, setpvcProtection.enabled=falseon all but one release. Documented in bothvalues.yamland the README.The other consequence — legitimate
kubectl delete pvc/ GitOps teardown now needs theformance.com/allow-deletionannotation (or an exempt SA) — is the intended safety trade-off.Testing
go build ./...,go test ./...,golangci-lint run ./...all green (operator module);helm lintpasses.controller-genregeneration is stable (no drift).e2e/tests/pvc-deletion-protection) still valid: it setsdeletionProtection: trueexplicitly and exercises the toggle-to-falseopt-out path.