Add deletion protection webhook for critical platform resources - #2402
Add deletion protection webhook for critical platform resources#2402myasnikovdaniil wants to merge 11 commits into
Conversation
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Label-based objectSelector (cozystack.io/deletion-protected=true) ensures zero overhead for non-protected resources. Reuses the existing lineage-controller-webhook Service and TLS certificates. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
… HelmRelease Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
…ap and cozystack-packages OCIRepository Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
…hitespace - LinstorCluster is cluster-scoped: fix webhook rule scope to Cluster, remove namespace from kindToResourceArg and test - OCIRepository webhook rule: scope "*" -> Namespaced - Unexport DeletionProtectedLabel constant (only used within package) - Normalize trailing whitespace in cert-manager-issuers template Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
📝 WalkthroughWalkthroughA deletion protection mechanism is implemented by adding an admission webhook handler that denies DELETE requests for resources labeled Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Summary of ChangesHello, I'm Gemini Code Assist1! 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 introduces a crucial safety mechanism for the Cozystack platform by implementing a deletion protection webhook. This webhook prevents the accidental removal of vital system components, which could lead to significant operational disruptions. The protection is applied to resources explicitly marked with a specific label, ensuring that critical infrastructure elements are safeguarded while allowing flexibility for other resources. This enhancement significantly improves the platform's resilience against unintended destructive operations. Highlights
🧠 New Feature in Public Preview: You can now enable Memory 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 AssistThe 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
Customization To customize 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 Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. 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
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/lineagecontrollerwebhook/validate_deletion_test.go`:
- Around line 132-134: The test currently lets denied cases pass when
resp.Result is nil; change the assertion in the test handling of !tt.wantAllowed
so it fails if resp.Result is nil and otherwise verifies resp.Result.Code equals
tt.wantCode. Concretely, in the block referencing resp.Result, tt.wantAllowed
and tt.wantCode (around the existing if !tt.wantAllowed && resp.Result != nil &&
resp.Result.Code != tt.wantCode), replace that compound condition with a
two-step check: if !tt.wantAllowed then first assert resp.Result != nil (error
if nil, mentioning tt.wantCode), and only then assert resp.Result.Code ==
tt.wantCode.
In `@packages/core/platform/templates/cozystack-version.yaml`:
- Around line 9-10: Existing installations won't get the deletion-protection
label because the template only renders when the ConfigMap is missing; update
the cozystack-version ConfigMap template to always render metadata.labels
(including cozystack.io/deletion-protected: "true") while preserving any
existing data/version value on the ConfigMap named "cozystack-version" so
upgrades backfill the label without clobbering the current version; specifically
change the template rendering logic that writes metadata (cozystack-version
ConfigMap) to merge or patch labels rather than only creating the object when
absent and ensure the label key cozystack.io/deletion-protected is always
present.
In
`@packages/system/lineage-controller-webhook/templates/validatingwebhookconfiguration.yaml`:
- Line 6: The YAML annotation cert-manager.io/inject-ca-from currently emits an
unquoted templated value causing lint/parse failures; modify the
ValidatingWebhookConfiguration template to wrap the entire annotation value in
double quotes so the rendered string "{{ .Release.Namespace
}}/lineage-controller-webhook" is quoted and valid YAML (update the
cert-manager.io/inject-ca-from entry in the template to use quoted templating).
🪄 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: c35978e4-9df9-410e-aa5d-1cd47eb9ddd3
📒 Files selected for processing (11)
cmd/lineage-controller-webhook/main.gointernal/crdinstall/install.gointernal/lineagecontrollerwebhook/validate_deletion.gointernal/lineagecontrollerwebhook/validate_deletion_test.gopackages/core/installer/templates/cozystack-operator.yamlpackages/core/platform/templates/cozystack-version.yamlpackages/core/platform/templates/repository.yamlpackages/system/cert-manager-issuers/templates/cluster-issuers.yamlpackages/system/cozystack-basics/templates/tenant-root.yamlpackages/system/lineage-controller-webhook/templates/validatingwebhookconfiguration.yamlpackages/system/linstor/templates/cluster.yaml
| if !tt.wantAllowed && resp.Result != nil && resp.Result.Code != tt.wantCode { | ||
| t.Errorf("Code = %d, want %d", resp.Result.Code, tt.wantCode) | ||
| } |
There was a problem hiding this comment.
Make forbidden-response assertions fail when Result is missing.
At Line 132, denied responses with resp.Result == nil currently pass. That can hide regressions in webhook error construction.
Proposed fix
- if !tt.wantAllowed && resp.Result != nil && resp.Result.Code != tt.wantCode {
- t.Errorf("Code = %d, want %d", resp.Result.Code, tt.wantCode)
- }
+ if !tt.wantAllowed {
+ if resp.Result == nil {
+ t.Fatalf("Result is nil for denied response")
+ }
+ if resp.Result.Code != tt.wantCode {
+ t.Errorf("Code = %d, want %d", resp.Result.Code, tt.wantCode)
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if !tt.wantAllowed && resp.Result != nil && resp.Result.Code != tt.wantCode { | |
| t.Errorf("Code = %d, want %d", resp.Result.Code, tt.wantCode) | |
| } | |
| if !tt.wantAllowed { | |
| if resp.Result == nil { | |
| t.Fatalf("Result is nil for denied response") | |
| } | |
| if resp.Result.Code != tt.wantCode { | |
| t.Errorf("Code = %d, want %d", resp.Result.Code, tt.wantCode) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/lineagecontrollerwebhook/validate_deletion_test.go` around lines 132
- 134, The test currently lets denied cases pass when resp.Result is nil; change
the assertion in the test handling of !tt.wantAllowed so it fails if resp.Result
is nil and otherwise verifies resp.Result.Code equals tt.wantCode. Concretely,
in the block referencing resp.Result, tt.wantAllowed and tt.wantCode (around the
existing if !tt.wantAllowed && resp.Result != nil && resp.Result.Code !=
tt.wantCode), replace that compound condition with a two-step check: if
!tt.wantAllowed then first assert resp.Result != nil (error if nil, mentioning
tt.wantCode), and only then assert resp.Result.Code == tt.wantCode.
| labels: | ||
| cozystack.io/deletion-protected: "true" |
There was a problem hiding this comment.
Existing installations may never receive deletion protection for this ConfigMap
Because this manifest is rendered only when the ConfigMap does not already exist, upgrades won’t backfill the new label on existing cozystack-version objects. Those existing objects stay deletable because the webhook uses label-based selection.
Suggested fix (preserve current version while always rendering metadata)
{{- $configMap := lookup "v1" "ConfigMap" .Release.Namespace "cozystack-version" }}
-{{- if not $configMap }}
+{{- $version := .Values.migrations.targetVersion | toString }}
+{{- if $configMap }}
+{{- $version = dig "data" "version" $version $configMap | toString }}
+{{- end }}
---
apiVersion: v1
kind: ConfigMap
metadata:
name: cozystack-version
namespace: {{ .Release.Namespace }}
labels:
cozystack.io/deletion-protected: "true"
annotations:
helm.sh/resource-policy: keep
data:
- version: {{ .Values.migrations.targetVersion | quote }}
-{{- end }}
+ version: {{ $version | quote }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/platform/templates/cozystack-version.yaml` around lines 9 - 10,
Existing installations won't get the deletion-protection label because the
template only renders when the ConfigMap is missing; update the
cozystack-version ConfigMap template to always render metadata.labels (including
cozystack.io/deletion-protected: "true") while preserving any existing
data/version value on the ConfigMap named "cozystack-version" so upgrades
backfill the label without clobbering the current version; specifically change
the template rendering logic that writes metadata (cozystack-version ConfigMap)
to merge or patch labels rather than only creating the object when absent and
ensure the label key cozystack.io/deletion-protected is always present.
| metadata: | ||
| name: deletion-protection | ||
| annotations: | ||
| cert-manager.io/inject-ca-from: {{ .Release.Namespace }}/lineage-controller-webhook |
There was a problem hiding this comment.
Quote the templated annotation value to avoid YAML parse failures.
Line 6 is currently parsed as invalid YAML by linting. Wrap the full value in quotes so template rendering and YAML parsing both succeed.
Proposed fix
- cert-manager.io/inject-ca-from: {{ .Release.Namespace }}/lineage-controller-webhook
+ cert-manager.io/inject-ca-from: "{{ .Release.Namespace }}/lineage-controller-webhook"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cert-manager.io/inject-ca-from: {{ .Release.Namespace }}/lineage-controller-webhook | |
| cert-manager.io/inject-ca-from: "{{ .Release.Namespace }}/lineage-controller-webhook" |
🧰 Tools
🪛 YAMLlint (1.38.0)
[error] 6-6: syntax error: expected , but found ''
(syntax)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/system/lineage-controller-webhook/templates/validatingwebhookconfiguration.yaml`
at line 6, The YAML annotation cert-manager.io/inject-ca-from currently emits an
unquoted templated value causing lint/parse failures; modify the
ValidatingWebhookConfiguration template to wrap the entire annotation value in
double quotes so the rendered string "{{ .Release.Namespace
}}/lineage-controller-webhook" is quoted and valid YAML (update the
cert-manager.io/inject-ca-from entry in the template to use quoted templating).
There was a problem hiding this comment.
Code Review
This pull request introduces a deletion protection mechanism for critical Cozystack resources by implementing a validating webhook that denies DELETE operations on resources labeled with 'cozystack.io/deletion-protected'. The changes include the webhook implementation, registration in the controller, and updates to various templates to apply the protection label to relevant resources. I have reviewed the implementation and suggest addressing the potential bypass via DeleteCollection and improving the resource identification logic in the error message.
| if req.Operation != admissionv1.Delete { | ||
| return admission.Allowed("not a DELETE operation") | ||
| } |
There was a problem hiding this comment.
The webhook currently only handles the DELETE operation. While this covers most accidental deletions, DeleteCollection (e.g., kubectl delete configmaps -l cozystack.io/deletion-protected=true) will bypass this check because it returns admission.Allowed. Consider adding support for admissionv1.DeleteCollection or explicitly documenting this limitation.
| func kindToResourceArg(kind, namespace string) string { | ||
| switch kind { | ||
| case "Namespace": | ||
| return "namespace" | ||
| case "ConfigMap": | ||
| return "configmap -n " + namespace | ||
| case "HelmRelease": | ||
| return "helmrelease.helm.toolkit.fluxcd.io -n " + namespace | ||
| case "CustomResourceDefinition": | ||
| return "crd" | ||
| case "LinstorCluster": | ||
| return "linstorcluster.piraeus.io" | ||
| case "ClusterIssuer": | ||
| return "clusterissuer.cert-manager.io" | ||
| case "OCIRepository": | ||
| return "ocirepository.source.toolkit.fluxcd.io -n " + namespace | ||
| default: | ||
| if namespace != "" { | ||
| return kind + " -n " + namespace | ||
| } | ||
| return kind | ||
| } | ||
| } |
There was a problem hiding this comment.
The kindToResourceArg function uses the Kind string directly from the request. For some resources, kubectl might require the fully qualified name or a specific alias to correctly identify the resource type when applying labels. While the current mapping covers common cases, the default case (line 102) might produce suboptimal hints for less common types. Consider using lowercase for the resource type in the default case to better match standard kubectl usage.
func kindToResourceArg(kind, namespace string) string {
switch kind {
case "Namespace":
return "namespace"
case "ConfigMap":
return "configmap -n " + namespace
case "HelmRelease":
return "helmrelease.helm.toolkit.fluxcd.io -n " + namespace
case "CustomResourceDefinition":
return "crd"
case "LinstorCluster":
return "linstorcluster.piraeus.io"
case "ClusterIssuer":
return "clusterissuer.cert-manager.io"
case "OCIRepository":
return "ocirepository.source.toolkit.fluxcd.io -n " + namespace
default:
resource := strings.ToLower(kind)
if namespace != "" {
return resource + " -n " + namespace
}
return resource
}
}
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Thanks for the work on deletion protection — the design is solid (label-based + objectSelector filtering + fail-closed). A few things need to be addressed before this can merge.
Must fix
1. cozystack-version ConfigMap label won't be applied on upgrades
packages/core/platform/templates/cozystack-version.yaml renders only when the ConfigMap doesn't exist ({{- if not $configMap }}). On existing installations the ConfigMap already exists, so the template is skipped and the cozystack.io/deletion-protected label is never added. The resource stays unprotected.
CodeRabbit flagged this too — see its inline comment with a suggested fix.
2. Unquoted YAML annotation in webhook configuration
packages/system/lineage-controller-webhook/templates/validatingwebhookconfiguration.yaml:6:
cert-manager.io/inject-ca-from: {{ .Release.Namespace }}/lineage-controller-webhookThe templated value must be quoted to produce valid YAML:
cert-manager.io/inject-ca-from: "{{ .Release.Namespace }}/lineage-controller-webhook"3. DeleteCollection bypasses protection
The webhook only intercepts DELETE operations. A kubectl delete configmaps --all -n cozy-system (which issues a DeleteCollection request) will bypass the webhook entirely. Either add "DELETECOLLECTION" to the operations list in the webhook rules, or explicitly document this as a known limitation in the PR description.
Should fix
4. Test silently passes when Result is nil
internal/lineagecontrollerwebhook/validate_deletion_test.go:132:
if !tt.wantAllowed && resp.Result != nil && resp.Result.Code != tt.wantCode {If the webhook returns a denied response with resp.Result == nil, this assertion passes silently. Split into two checks: first assert resp.Result != nil, then check the code.
5. kindToResourceArg default case doesn't lowercase
For unknown resource kinds, the hint message will contain the Kind as-is (e.g., kubectl label SomeKind ...). kubectl expects lowercase resource types. The default branch should use strings.ToLower(kind).
General note
Please don't leave bot review comments (CodeRabbit, Gemini) unaddressed — either fix them or reply explaining why they don't apply. Unanswered bot comments make it hard to tell what's been considered and what's been missed.
|
Another approach planned |
…sionPolicy (#2650) ## What this PR does Adds a `ValidatingAdmissionPolicy` that blocks `DELETE` on critical platform objects labeled `platform.cozystack.io/no-delete=true`. Native, in-process CEL evaluation — no webhook DaemonSet, no Service, no TLS, no image to maintain. Requires Kubernetes 1.30+. This is an alternative to the closed #2402 (admission webhook approach). ### Protected objects (labeled in this PR) | Resource | Kind | Source | |---|---|---| | `cozy-system` | Namespace | `packages/core/installer/templates/cozy-system-{namespace,labels}.yaml` | | `tenant-root` | Namespace | `packages/system/cozystack-basics/templates/tenant-root.yaml` | | `tenant-root` | HelmRelease (ns: tenant-root) | same | | `cozystack-version` | ConfigMap (ns: cozy-system) | `packages/core/platform/templates/cozystack-version.yaml` | | `cozystack-packages` | OCIRepository (ns: cozy-system) | `packages/core/platform/templates/repository.yaml` | | `letsencrypt-prod`, `letsencrypt-stage`, `selfsigned-cluster-issuer` | ClusterIssuer | `packages/system/cert-manager-issuers/templates/cluster-issuers.yaml` | | `linstorcluster` | LinstorCluster | `packages/system/linstor/templates/cluster.yaml` | | `packages.cozystack.io`, `packagesources.cozystack.io` | CRD | label stamped in `internal/crdinstall/install.go` | ### Bypass ```bash kubectl label <kind> <name> platform.cozystack.io/no-delete- kubectl delete <kind> <name> ``` ### Why VAP instead of a webhook Same outcome as #2402 but with zero runtime infrastructure: no DaemonSet, Service, TLS certificate, or image. The kube-apiserver evaluates the CEL `expression: "false"` in-process and denies because `validationActions: [Deny]`. `objectSelector` on the binding scopes evaluation to labeled objects only — every other DELETE in the cluster is unaffected. ### Release note ```release-note feat(platform): add deletion-protection guardrail (ValidatingAdmissionPolicy) blocking DELETE on cozystack platform objects labeled `platform.cozystack.io/no-delete=true`. Bypass by removing the label first. Requires Kubernetes 1.30+. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Deletion-protection guardrail: objects labeled platform.cozystack.io/no-delete=true are denied DELETE requests; removal of the label allows deletion. * **Chores** * Platform migration to v43 backfills the no-delete label onto system resources and namespaces. * **Tools** * Added an "unprotect" teardown utility to remove the no-delete label for uninstall/disaster recovery. * **Documentation** * Helm NOTES now warns when the guardrail is inactive due to missing cluster API. * **Tests** * New tests and e2e checks validate deletion-protection behavior. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/cozystack/cozystack/pull/2650?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
What this PR does
Adds a validating admission webhook that prevents accidental deletion of critical cozystack platform resources. Deleting
tenant-rootcascades to all child tenants, deletingcozy-systemkills the operator, deletingLinstorClustertakes down all storage — there was no admission-level guardrail preventing this.Design
cozystack.io/deletion-protected: "true"are protectedProtected resources
tenant-rootcozy-systemcozystack-versioncozystack-packageslinstorclusterletsencrypt-prod,letsencrypt-stage,selfsigned-cluster-issuerpackages.cozystack.io,packagesources.cozystack.ioBypass mechanism
Two deliberate steps required:
Release note
Added deletion protection webhook preventing accidental deletion of critical platform resources (tenant-root, cozy-system, LinstorCluster, CRDs, ClusterIssuers, platform ConfigMap and OCIRepository). Protection is label-based and can be bypassed by removing the
cozystack.io/deletion-protectedlabel.Test plan
go buildfor lineage-controller-webhook binarygo test ./internal/crdinstall/passes with label injectionkubectl delete ns tenant-rootis denied🤖 Generated with Claude Code
Summary by CodeRabbit