From c46b6b11bddb18668617849d26b853f6a20b81af Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Thu, 30 Jul 2026 10:58:08 +0200 Subject: [PATCH 1/3] feat(chart): deny EtcdMember deletion at admission MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The docs have said since their first version that EtcdMember objects are created and deleted by the cluster controller and users should not touch them. Nothing enforced it, and the accident is unrecoverable: a member's PVC is controller-owned by it, so deleting the CR takes the data volume with it while the finalizer removes the member from etcd on the way out. Delete every member of a cluster and it dismembers itself with nothing left to restore from. One kubectl delete, a GitOps prune of an unexpected object, or a cleanup script sweeping CRs by label is enough. Install a ValidatingAdmissionPolicy denying DELETE on etcdmembers, except for the operator's ServiceAccount (scale-down and crash-loop replacement delete members deliberately), the garbage collector (an EtcdCluster deletion must still cascade), the namespace controller (namespace deletion must not hang), and anything in memberDeletionProtection.additionalAllowedUsers. Break-glass without uninstalling: annotate the member with etcd-operator.cozystack.io/allow-deletion=true. Requires Kubernetes 1.30+; memberDeletionProtection.enabled=false installs without the guard. Deliberately not gated on .Capabilities — that is empty for a plain helm template, so gating there would silently drop the policy from the rendered release manifests. Docs: state in the API model that deleting a member destroys its data (the sentence previously said only "create or edit"), document the guard and the break-glass path, add the annotation step to the broken-member recovery recipe it would otherwise block, and remove the policy before the CRDs during a manual teardown — deleting a CRD makes the apiserver delete every member, which the policy would deny. Assisted-By: Claude Signed-off-by: Andrei Kvapil --- .../templates/member-deletion-policy.yaml | 74 ++++++++++ .../tests/member_deletion_policy_test.yaml | 128 ++++++++++++++++++ charts/etcd-operator/values.yaml | 22 +++ docs/concepts.md | 27 +++- docs/installation.md | 7 + docs/operations.md | 10 +- 6 files changed, 265 insertions(+), 3 deletions(-) create mode 100644 charts/etcd-operator/templates/member-deletion-policy.yaml create mode 100644 charts/etcd-operator/tests/member_deletion_policy_test.yaml diff --git a/charts/etcd-operator/templates/member-deletion-policy.yaml b/charts/etcd-operator/templates/member-deletion-policy.yaml new file mode 100644 index 00000000..03a99161 --- /dev/null +++ b/charts/etcd-operator/templates/member-deletion-policy.yaml @@ -0,0 +1,74 @@ +{{- /* +Enforces at the API boundary the contract the docs have carried since day one: +EtcdMember objects are created and deleted by the operator, not by users. + +Deleting one is not a recoverable mistake. Its data PVC is controller-owned by +the member, so the volume — and on a Delete-reclaim StorageClass the data +itself — goes with it, while the member's finalizer removes the member from +etcd on the way out. A cluster whose members are deleted one by one therefore +dismembers itself and leaves nothing to restore from. + +Denied here rather than survived in the controllers: the accident (a stray +kubectl delete, a GitOps prune, a cleanup script sweeping CRs) is what needs to +stop, and the requester learns why at the moment they try it. + +Allowed through: + - the operator's own ServiceAccount — scale-down and crash-loop replacement + delete members deliberately; + - the garbage collector — cascade from a deleted EtcdCluster must still work; + - the namespace controller — deleting a namespace must not hang. + +Break-glass without uninstalling the policy: annotate the member with +etcd-operator.cozystack.io/allow-deletion=true, then delete it. + +Requires Kubernetes 1.30+ (ValidatingAdmissionPolicy GA). On an older +apiserver the install fails with "no matches for kind +ValidatingAdmissionPolicy"; set memberDeletionProtection.enabled=false to +install without the guard. Deliberately not gated on .Capabilities: that is +empty for a plain `helm template`, so gating there would silently drop the +policy from the rendered release manifests. +*/ -}} +{{- if .Values.memberDeletionProtection.enabled }} +{{- $allowed := concat + (list + (printf "system:serviceaccount:%s:%s" .Release.Namespace (include "etcd-operator.serviceAccountName" .)) + "system:serviceaccount:kube-system:generic-garbage-collector" + "system:serviceaccount:kube-system:namespace-controller") + .Values.memberDeletionProtection.additionalAllowedUsers }} +apiVersion: admissionregistration.k8s.io/v1 +kind: ValidatingAdmissionPolicy +metadata: + name: {{ include "etcd-operator.fullname" . }}-protect-members + labels: + {{- include "etcd-operator.labels" . | nindent 4 }} +spec: + failurePolicy: Fail + matchConstraints: + resourceRules: + - apiGroups: ["etcd-operator.cozystack.io"] + apiVersions: ["*"] + operations: ["DELETE"] + resources: ["etcdmembers"] + validations: + - expression: >- + request.userInfo.username in {{ $allowed | toJson }} + || (has(oldObject.metadata.annotations) + && "etcd-operator.cozystack.io/allow-deletion" in oldObject.metadata.annotations + && oldObject.metadata.annotations["etcd-operator.cozystack.io/allow-deletion"] == "true") + reason: Forbidden + messageExpression: >- + "EtcdMember " + oldObject.metadata.name + " is managed exclusively by etcd-operator, and deleting it + removes the member from etcd and releases its data volume — the data is not recoverable from here. + Scale the EtcdCluster instead. If you really mean it, annotate the member with + etcd-operator.cozystack.io/allow-deletion=true first." +--- +apiVersion: admissionregistration.k8s.io/v1 +kind: ValidatingAdmissionPolicyBinding +metadata: + name: {{ include "etcd-operator.fullname" . }}-protect-members + labels: + {{- include "etcd-operator.labels" . | nindent 4 }} +spec: + policyName: {{ include "etcd-operator.fullname" . }}-protect-members + validationActions: ["Deny"] +{{- end }} diff --git a/charts/etcd-operator/tests/member_deletion_policy_test.yaml b/charts/etcd-operator/tests/member_deletion_policy_test.yaml new file mode 100644 index 00000000..b82f0b5d --- /dev/null +++ b/charts/etcd-operator/tests/member_deletion_policy_test.yaml @@ -0,0 +1,128 @@ +suite: EtcdMember deletion is denied at admission + +# The policy is the enforcement point for a contract the docs have always +# stated: EtcdMember objects belong to the operator. What matters in these +# assertions is not the YAML shape but who keeps the ability to delete — +# getting that list wrong either wedges cluster/namespace deletion (too +# strict) or leaves the hole open (too loose). + +templates: + - member-deletion-policy.yaml + +tests: + - it: is installed by default + asserts: + - hasDocuments: + count: 2 + - containsDocument: + apiVersion: admissionregistration.k8s.io/v1 + kind: ValidatingAdmissionPolicy + name: RELEASE-NAME-etcd-operator-protect-members + documentIndex: 0 + - containsDocument: + apiVersion: admissionregistration.k8s.io/v1 + kind: ValidatingAdmissionPolicyBinding + name: RELEASE-NAME-etcd-operator-protect-members + documentIndex: 1 + # The binding must name the policy it enforces — a typo here installs a + # policy that matches nothing and silently protects nothing. + - equal: + path: spec.policyName + value: RELEASE-NAME-etcd-operator-protect-members + documentIndex: 1 + + - it: matches only DELETE on etcdmembers + documentSelector: + path: kind + value: ValidatingAdmissionPolicy + asserts: + - equal: + path: spec.matchConstraints.resourceRules[0].operations + value: ["DELETE"] + - equal: + path: spec.matchConstraints.resourceRules[0].resources + value: ["etcdmembers"] + - equal: + path: spec.matchConstraints.resourceRules[0].apiGroups + value: ["etcd-operator.cozystack.io"] + + - it: denies rather than warns, and fails closed + asserts: + - equal: + path: spec.validationActions + value: ["Deny"] + documentSelector: + path: kind + value: ValidatingAdmissionPolicyBinding + - equal: + path: spec.failurePolicy + value: Fail + documentSelector: + path: kind + value: ValidatingAdmissionPolicy + + # The operator deletes members itself on scale-down and crash-loop + # replacement; the GC has to cascade from a deleted EtcdCluster; the + # namespace controller has to finish a namespace deletion. Denying any of + # the three turns a routine operation into a wedge. + - it: still allows the operator, the garbage collector and the namespace controller + documentSelector: + path: kind + value: ValidatingAdmissionPolicy + asserts: + - matchRegex: + path: spec.validations[0].expression + pattern: system:serviceaccount:NAMESPACE:RELEASE-NAME-etcd-operator + - matchRegex: + path: spec.validations[0].expression + pattern: system:serviceaccount:kube-system:generic-garbage-collector + - matchRegex: + path: spec.validations[0].expression + pattern: system:serviceaccount:kube-system:namespace-controller + + - it: honours the break-glass annotation + documentSelector: + path: kind + value: ValidatingAdmissionPolicy + asserts: + - matchRegex: + path: spec.validations[0].expression + pattern: etcd-operator\.cozystack\.io/allow-deletion + + # A platform whose own controller legitimately reaps members needs a way in + # that does not mean disabling the guard outright. + - it: accepts additional allowed users + set: + memberDeletionProtection: + additionalAllowedUsers: + - system:serviceaccount:platform:reaper + documentSelector: + path: kind + value: ValidatingAdmissionPolicy + asserts: + - matchRegex: + path: spec.validations[0].expression + pattern: system:serviceaccount:platform:reaper + + # Kubernetes below 1.30 has no ValidatingAdmissionPolicy; the chart must + # still install there. + - it: can be switched off for older apiservers + set: + memberDeletionProtection: + enabled: false + asserts: + - hasDocuments: + count: 0 + + - it: names the ServiceAccount the release actually uses + set: + serviceAccount: + create: false + name: byo-sa + documentSelector: + path: kind + value: ValidatingAdmissionPolicy + asserts: + - matchRegex: + path: spec.validations[0].expression + pattern: system:serviceaccount:NAMESPACE:byo-sa diff --git a/charts/etcd-operator/values.yaml b/charts/etcd-operator/values.yaml index 04233fbb..3311feae 100644 --- a/charts/etcd-operator/values.yaml +++ b/charts/etcd-operator/values.yaml @@ -9,6 +9,28 @@ crds: # EtcdCluster and its data. keep: true +# Deny DELETE on EtcdMember objects at the API boundary, except for the +# operator itself, the garbage collector (so a deleted EtcdCluster still +# cascades) and the namespace controller (so namespace deletion does not hang). +# +# Deleting an EtcdMember by hand is not a recoverable mistake: its data PVC is +# controller-owned by the member, so the volume goes with it, while the +# member's finalizer removes the member from etcd on the way out. The docs have +# always said these objects are the operator's; this enforces it. +# +# Break-glass without uninstalling: annotate the member with +# etcd-operator.cozystack.io/allow-deletion=true, then delete it. +# +# Requires Kubernetes 1.30+ (ValidatingAdmissionPolicy GA). Set to false on +# older apiservers. +memberDeletionProtection: + # -- Install the ValidatingAdmissionPolicy protecting EtcdMember objects. + enabled: true + # -- Extra usernames allowed to delete EtcdMembers, in apiserver form + # (e.g. "system:serviceaccount::" or a user name). For platforms + # whose own controllers legitimately reap these objects. + additionalAllowedUsers: [] + # -- Render a Namespace object. Off by default (real `helm install` uses # --create-namespace); build-dist-manifests turns it on so the rendered # kubectl-apply manifest is self-contained. diff --git a/docs/concepts.md b/docs/concepts.md index 4abc8d81..d239b22a 100644 --- a/docs/concepts.md +++ b/docs/concepts.md @@ -10,12 +10,37 @@ Two custom resources, one of them user-facing. **`EtcdCluster`** — the user-facing object. It captures cluster-wide intent: replica count, etcd version, per-member storage size, a progress deadline. This is the only resource users normally touch. -**`EtcdMember`** — one per etcd member. Created and deleted by the cluster controller. Each `EtcdMember` owns its Pod and PVC. Users should not create or edit these directly. +**`EtcdMember`** — one per etcd member. Created and deleted by the cluster controller. Each `EtcdMember` owns its Pod and PVC. Users should not create, edit or **delete** these directly. + +Deleting one by hand is not a recoverable mistake: the member's PVC is controller-owned by it, so the data volume is removed with the CR (and on a `Delete`-reclaim StorageClass the data itself), while the member's finalizer removes the member from etcd on the way out. Delete every member of a cluster and it dismembers itself, leaving nothing to restore from. Scale the `EtcdCluster` instead — see [member deletion is denied at admission](#member-deletion-is-denied-at-admission). There is **no StatefulSet**. Each member's Pod and PVC are reconciled independently by the member controller. The motivation is protocol awareness: scale-up adds a member as a learner first and only promotes once it's caught up; scale-down runs `MemberRemove` via a finalizer before reclaiming the Pod; pod restarts reuse the existing data dir and rejoin with the same etcd-side member ID. None of these flows fit StatefulSet's "all replicas are one fungible workload" model. The cluster controller decides *which* members exist and orchestrates the etcd-side state machine (`MemberAddAsLearner` / `MemberPromote` / `MemberRemove`). The member controller decides *how* a member becomes real — Pod, PVC, etcd flags — and reports observed facts (member ID, readiness) back up to its CR's status. +## Member deletion is denied at admission + +The chart installs a `ValidatingAdmissionPolicy` that rejects `DELETE` on `etcdmembers` for everyone except: + +- the operator's own ServiceAccount — scale-down and crash-loop replacement delete members deliberately; +- `system:serviceaccount:kube-system:generic-garbage-collector` — a deleted `EtcdCluster` must still cascade to its members; +- `system:serviceaccount:kube-system:namespace-controller` — deleting a namespace must not hang; +- anything listed in `memberDeletionProtection.additionalAllowedUsers`, for platforms whose own controllers legitimately reap these objects. + +The rejection message says what would have happened and how to proceed deliberately. Break-glass without uninstalling the policy: + +```sh +kubectl annotate etcdmember.etcd-operator.cozystack.io -n \ + etcd-operator.cozystack.io/allow-deletion=true +kubectl delete etcdmember.etcd-operator.cozystack.io -n +``` + +The guard exists because the accident it prevents is unrecoverable and easy: one `kubectl delete`, a GitOps prune of an unexpected object, a cleanup script sweeping CRs by label. The controllers cannot make that survivable — a member's data volume is bound to its identity, and a replacement member gets a fresh name and UID — so the event is stopped at the boundary instead. + +Requires Kubernetes 1.30+ (`ValidatingAdmissionPolicy` GA). On older apiservers, install with `memberDeletionProtection.enabled=false`; the operator behaves as before, without the guard. + +**Uninstalling:** remove the policy before removing the CRDs. `helm uninstall` does this in the right order, but a manual teardown that deletes the CRDs first will find member deletion denied and stall. + ## Member naming `EtcdMember` CRs are created with `ObjectMeta.GenerateName="-"`. Each member's name is an apiserver-assigned random suffix (e.g. `mycluster-7xq2k`). Names are not predictable, and that is deliberate — the previous design used `-` and tied cluster identity to ordinal reuse across incarnations, which is exactly the trap to avoid for stateful systems. Now: diff --git a/docs/installation.md b/docs/installation.md index d8c6ab4c..c7d00250 100644 --- a/docs/installation.md +++ b/docs/installation.md @@ -311,6 +311,13 @@ kubectl delete etcdcluster.etcd-operator.cozystack.io --all -A # intentionally left in place. make undeploy +# Remove the member-deletion guard before the CRDs. Removing the CRD makes the +# apiserver delete every EtcdMember, and the policy would deny those requests — +# leaving the CRD stuck in Terminating. `helm uninstall` above already removed +# it; this is for teardowns that skipped Helm: +kubectl delete validatingadmissionpolicybinding etcd-operator-protect-members --ignore-not-found +kubectl delete validatingadmissionpolicy etcd-operator-protect-members --ignore-not-found + # Remove the CRDs too (only after all EtcdClusters are gone) — deleting them # cascade-deletes every remaining EtcdCluster: kubectl delete crd etcdclusters.etcd-operator.cozystack.io \ diff --git a/docs/operations.md b/docs/operations.md index 4c5a7975..dd1892bb 100644 --- a/docs/operations.md +++ b/docs/operations.md @@ -214,14 +214,20 @@ Manual recovery: ```sh # 1. Identify the broken member. kubectl get etcdmember.etcd-operator.cozystack.io -n -# 2. Delete it — the finalizer runs MemberRemove against peers, then GC takes +# 2. Deleting a member is denied by default (see concepts: member deletion is +# denied at admission) — this is the deliberate exception, so unlock it: +kubectl annotate etcdmember.etcd-operator.cozystack.io -n \ + etcd-operator.cozystack.io/allow-deletion=true +# 3. Delete it — the finalizer runs MemberRemove against peers, then GC takes # the Pod and PVC. Quorum holds because we remove before adding. kubectl delete etcdmember.etcd-operator.cozystack.io -n -# 3. The cluster controller's next reconcile observes current < desired and +# 4. The cluster controller's next reconcile observes current < desired and # scales up automatically — a new member is added with GenerateName and # fresh storage. ``` +The annotation step is the point of the guard: this recovery discards a data volume on purpose, and typing that out is what separates it from the same command issued by accident. + This sequence preserves quorum if you have an odd number of voters and only one is broken. If multiple voters are broken simultaneously, quorum is lost and you can't `MemberRemove` cleanly. In that case the recovery is to delete the EtcdCluster, recreate it, and restore from a snapshot — see [Restoring a cluster from a snapshot](#restoring-a-cluster-from-a-snapshot). Snapshots only exist if you have been taking `EtcdSnapshot`s, so set that up *before* you need it. ## Taking a snapshot From d7886dc12f88dd9b19be1067497e9cfd3dc64ee8 Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Thu, 30 Jul 2026 11:48:24 +0200 Subject: [PATCH 2/3] test(e2e): take the break-glass path when deleting a member MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Kamaji datastore test deletes EtcdMembers on purpose to exercise MemberRemove plus the GenerateName replacement. With member deletion denied at admission that request is rejected — correctly: the guard exists to stop exactly this command when it is issued by accident. Annotate the member with etcd-operator.cozystack.io/allow-deletion=true first, which is the documented path for a deliberate deletion. Done in the test rather than by exempting its identity in the policy, so the e2e stays honest about what a human has to do. Assisted-By: Claude Signed-off-by: Andrei Kvapil --- test/e2e/kamaji_datastore_test.go | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/test/e2e/kamaji_datastore_test.go b/test/e2e/kamaji_datastore_test.go index a93bd9de..790cb2a9 100644 --- a/test/e2e/kamaji_datastore_test.go +++ b/test/e2e/kamaji_datastore_test.go @@ -165,7 +165,22 @@ func TestKamajiDataStore(t *testing.T) { t.Logf("original members: %v", original) for _, victim := range original { t.Logf("deleting EtcdMember %q (operator does MemberRemove + a GenerateName replacement)", victim) - m := &etcdv1alpha2.EtcdMember{ObjectMeta: metav1.ObjectMeta{Namespace: e2eNamespace, Name: victim}} + // Member deletion is denied at admission — the guard exists precisely to + // stop this command when it is issued by accident. This test issues it on + // purpose, so it takes the documented break-glass path: annotate first. + // Doing it here rather than exempting the test's identity in the policy + // keeps the e2e honest about what a human has to do. + m := &etcdv1alpha2.EtcdMember{} + if err := kube.Get(ctx, client.ObjectKey{Namespace: e2eNamespace, Name: victim}, m); err != nil { + t.Fatalf("get member %s before deletion: %v", victim, err) + } + if m.Annotations == nil { + m.Annotations = map[string]string{} + } + m.Annotations["etcd-operator.cozystack.io/allow-deletion"] = "true" + if err := kube.Update(ctx, m); err != nil { + t.Fatalf("annotate member %s for deletion: %v", victim, err) + } if err := kube.Delete(ctx, m); err != nil && !apierrors.IsNotFound(err) { t.Fatalf("delete member %s: %v", victim, err) } From 02286c445aaa9c11703ec8c0268b750abd5d2fb0 Mon Sep 17 00:00:00 2001 From: Andrey Kolkov Date: Mon, 17 Aug 2026 11:46:46 +0400 Subject: [PATCH 3/3] fix(chart): address review on member-deletion protection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Allow system:kube-controller-manager: on clusters without --use-service-account-credentials, GC and namespace cleanup delete as that user, not the per-controller SAs. Covered in the unittest. - Scope the binding to manager.watchNamespaces when set (namespaceSelector on kubernetes.io/metadata.name) so multiple namespace-scoped releases don't deny each other's members; document the cluster-wide singleton constraint. - Correct the CRD-teardown story: apiextensions cleans up CR instances in-process during CRD deletion, bypassing admission, so the policy neither denies those deletes nor stalls the CRD — and CRD-deletion-driven member removal is a documented known gap the guard does not cover (crds.keep guards that path). Not verified on kind here; framed as the honest limitation. - Rewrite the always-render rationale: capability detection would make the guard's absence silent; absence must be loud and explicitly opted out of. - Drop "a GitOps prune" from the threat framing; state the contract positively (nothing manages EtcdMembers declaratively). - installation.md teardown: delete the policy by label, not a hardcoded name. - e2e break-glass: annotate via a MergeFrom patch, not Get+Update (409-proof). Signed-off-by: Andrey Kolkov Co-Authored-By: Claude Opus 4.8 (1M context) --- .../templates/member-deletion-policy.yaml | 41 +++++++++++++++---- .../tests/member_deletion_policy_test.yaml | 39 +++++++++++++++++- charts/etcd-operator/values.yaml | 5 +++ docs/concepts.md | 6 ++- docs/installation.md | 16 +++++--- test/e2e/kamaji_datastore_test.go | 5 ++- 6 files changed, 94 insertions(+), 18 deletions(-) diff --git a/charts/etcd-operator/templates/member-deletion-policy.yaml b/charts/etcd-operator/templates/member-deletion-policy.yaml index 03a99161..a8807f8e 100644 --- a/charts/etcd-operator/templates/member-deletion-policy.yaml +++ b/charts/etcd-operator/templates/member-deletion-policy.yaml @@ -8,15 +8,20 @@ itself — goes with it, while the member's finalizer removes the member from etcd on the way out. A cluster whose members are deleted one by one therefore dismembers itself and leaves nothing to restore from. -Denied here rather than survived in the controllers: the accident (a stray -kubectl delete, a GitOps prune, a cleanup script sweeping CRs) is what needs to -stop, and the requester learns why at the moment they try it. +Nothing manages EtcdMember objects declaratively (the only sanctioned +non-operator writer is cmd/etcd-migrate, which creates and never deletes), so a +DELETE from anywhere else is always an accident or a tracking misconfiguration +— stopped at the boundary, with the requester told why. Allowed through: - the operator's own ServiceAccount — scale-down and crash-loop replacement delete members deliberately; - the garbage collector — cascade from a deleted EtcdCluster must still work; - - the namespace controller — deleting a namespace must not hang. + - the namespace controller — deleting a namespace must not hang; + - kube-controller-manager itself — on clusters run without + --use-service-account-credentials the GC and namespace-cleanup deletes + authenticate as the user system:kube-controller-manager rather than the + per-controller ServiceAccounts above. Break-glass without uninstalling the policy: annotate the member with etcd-operator.cozystack.io/allow-deletion=true, then delete it. @@ -24,16 +29,22 @@ etcd-operator.cozystack.io/allow-deletion=true, then delete it. Requires Kubernetes 1.30+ (ValidatingAdmissionPolicy GA). On an older apiserver the install fails with "no matches for kind ValidatingAdmissionPolicy"; set memberDeletionProtection.enabled=false to -install without the guard. Deliberately not gated on .Capabilities: that is -empty for a plain `helm template`, so gating there would silently drop the -policy from the rendered release manifests. +install without the guard. + +Rendered unconditionally (NOT gated on .Capabilities.APIVersions): capability +detection would make a security guard's absence silent — a pre-1.30 `helm +install`, or a GitOps render carrying the destination's API versions, would +quietly omit it. Absence of a guard against an unrecoverable accident must be +loud and explicit: always render, and opt out via +memberDeletionProtection.enabled=false. */ -}} {{- if .Values.memberDeletionProtection.enabled }} {{- $allowed := concat (list (printf "system:serviceaccount:%s:%s" .Release.Namespace (include "etcd-operator.serviceAccountName" .)) "system:serviceaccount:kube-system:generic-garbage-collector" - "system:serviceaccount:kube-system:namespace-controller") + "system:serviceaccount:kube-system:namespace-controller" + "system:kube-controller-manager") .Values.memberDeletionProtection.additionalAllowedUsers }} apiVersion: admissionregistration.k8s.io/v1 kind: ValidatingAdmissionPolicy @@ -71,4 +82,18 @@ metadata: spec: policyName: {{ include "etcd-operator.fullname" . }}-protect-members validationActions: ["Deny"] +{{- with .Values.manager.watchNamespaces }} + # Scope to the watched namespaces so multiple namespace-scoped releases don't + # deny each other's members (admission is deny-wins; each allowlist holds only + # its own operator SA). Cluster-wide install must be a singleton — see values.yaml. + matchResources: + namespaceSelector: + matchExpressions: + - key: kubernetes.io/metadata.name + operator: In + values: + {{- range . }} + - {{ . | quote }} + {{- end }} +{{- end }} {{- end }} diff --git a/charts/etcd-operator/tests/member_deletion_policy_test.yaml b/charts/etcd-operator/tests/member_deletion_policy_test.yaml index b82f0b5d..fc20c12b 100644 --- a/charts/etcd-operator/tests/member_deletion_policy_test.yaml +++ b/charts/etcd-operator/tests/member_deletion_policy_test.yaml @@ -65,7 +65,7 @@ tests: # replacement; the GC has to cascade from a deleted EtcdCluster; the # namespace controller has to finish a namespace deletion. Denying any of # the three turns a routine operation into a wedge. - - it: still allows the operator, the garbage collector and the namespace controller + - it: still allows the operator, the garbage collector, the namespace controller and kube-controller-manager documentSelector: path: kind value: ValidatingAdmissionPolicy @@ -79,6 +79,11 @@ tests: - matchRegex: path: spec.validations[0].expression pattern: system:serviceaccount:kube-system:namespace-controller + # Clusters without --use-service-account-credentials run GC and namespace + # cleanup as the user system:kube-controller-manager, not per-controller SAs. + - matchRegex: + path: spec.validations[0].expression + pattern: system:kube-controller-manager - it: honours the break-glass annotation documentSelector: @@ -126,3 +131,35 @@ tests: - matchRegex: path: spec.validations[0].expression pattern: system:serviceaccount:NAMESPACE:byo-sa + + # Cluster-wide by default (no watchNamespaces): the binding must match every + # namespace, so it carries no matchResources scoping. + - it: binds cluster-wide when the operator watches all namespaces + documentSelector: + path: kind + value: ValidatingAdmissionPolicyBinding + asserts: + - notExists: + path: spec.matchResources + + # Namespace-scoped operator: the binding must scope to the watched namespaces + # so two scoped releases don't deny each other's members. + - it: scopes the binding to the watched namespaces when set + set: + manager: + watchNamespaces: + - team-a + - team-b + documentSelector: + path: kind + value: ValidatingAdmissionPolicyBinding + asserts: + - equal: + path: spec.matchResources.namespaceSelector.matchExpressions[0].key + value: kubernetes.io/metadata.name + - equal: + path: spec.matchResources.namespaceSelector.matchExpressions[0].operator + value: In + - equal: + path: spec.matchResources.namespaceSelector.matchExpressions[0].values + value: [team-a, team-b] diff --git a/charts/etcd-operator/values.yaml b/charts/etcd-operator/values.yaml index 3311feae..d4ff0c56 100644 --- a/charts/etcd-operator/values.yaml +++ b/charts/etcd-operator/values.yaml @@ -23,6 +23,11 @@ crds: # # Requires Kubernetes 1.30+ (ValidatingAdmissionPolicy GA). Set to false on # older apiservers. +# +# Scope: with manager.watchNamespaces set, the binding is scoped to those +# namespaces so scoped releases coexist. Cluster-wide (watchNamespaces empty) +# the install must be a singleton — a second cluster-wide release would deny +# this operator's own scale-down/replacement (admission is deny-wins). memberDeletionProtection: # -- Install the ValidatingAdmissionPolicy protecting EtcdMember objects. enabled: true diff --git a/docs/concepts.md b/docs/concepts.md index d239b22a..39d9f302 100644 --- a/docs/concepts.md +++ b/docs/concepts.md @@ -35,11 +35,13 @@ kubectl annotate etcdmember.etcd-operator.cozystack.io -n \ kubectl delete etcdmember.etcd-operator.cozystack.io -n ``` -The guard exists because the accident it prevents is unrecoverable and easy: one `kubectl delete`, a GitOps prune of an unexpected object, a cleanup script sweeping CRs by label. The controllers cannot make that survivable — a member's data volume is bound to its identity, and a replacement member gets a fresh name and UID — so the event is stopped at the boundary instead. +The guard exists because the accident it prevents is unrecoverable and easy, and because nothing legitimately manages `EtcdMember` objects declaratively — the only sanctioned non-operator writer is `cmd/etcd-migrate`, which creates and never deletes. So a DELETE from anywhere else — a stray `kubectl delete`, a cleanup script sweeping CRs by label, a GitOps tool that wrongly tracks these objects (a tracking misconfiguration, not a workflow to support) — is always an accident. The controllers cannot make it survivable (a member's data volume is bound to its identity, and a replacement gets a fresh name and UID), so the event is stopped at the boundary instead. Requires Kubernetes 1.30+ (`ValidatingAdmissionPolicy` GA). On older apiservers, install with `memberDeletionProtection.enabled=false`; the operator behaves as before, without the guard. -**Uninstalling:** remove the policy before removing the CRDs. `helm uninstall` does this in the right order, but a manual teardown that deletes the CRDs first will find member deletion denied and stall. +**Known limitation:** the guard does not cover member removal driven by *CRD* deletion. When the `etcdmembers` CRD is deleted, apiextensions cleans up the CR instances in-process, through the storage layer rather than the authenticated request path — the same route that keeps admission webhooks from firing for CRs deleted during CRD deletion — so admission (and this policy) never sees those deletes. `crds.keep=true` (the default) is what actually protects against that path. + +**Uninstalling:** `helm uninstall` removes the policy for you. A manual teardown should remove the policy too (see the [teardown runbook](installation.md#teardown)); ordering it before the CRD deletion is tidy but not load-bearing — per the limitation above, CRD cleanup bypasses the policy rather than stalling on it. ## Member naming diff --git a/docs/installation.md b/docs/installation.md index c7d00250..c6330930 100644 --- a/docs/installation.md +++ b/docs/installation.md @@ -311,12 +311,16 @@ kubectl delete etcdcluster.etcd-operator.cozystack.io --all -A # intentionally left in place. make undeploy -# Remove the member-deletion guard before the CRDs. Removing the CRD makes the -# apiserver delete every EtcdMember, and the policy would deny those requests — -# leaving the CRD stuck in Terminating. `helm uninstall` above already removed -# it; this is for teardowns that skipped Helm: -kubectl delete validatingadmissionpolicybinding etcd-operator-protect-members --ignore-not-found -kubectl delete validatingadmissionpolicy etcd-operator-protect-members --ignore-not-found +# Remove the member-deletion guard. `helm uninstall` above already removed it; +# this is for teardowns that skipped Helm. Order does not actually matter for +# the CRD: apiextensions cleans up CR instances in-process during CRD deletion, +# which bypasses admission, so the policy neither denies those deletes nor +# stalls the CRD (this is also why CRD-deletion-driven member removal is a +# known gap the guard does not cover — see concepts.md). Delete by label so it +# works regardless of the release name (add app.kubernetes.io/instance= +# to disambiguate when several releases are installed): +kubectl delete validatingadmissionpolicybinding,validatingadmissionpolicy \ + -l app.kubernetes.io/name=etcd-operator --ignore-not-found # Remove the CRDs too (only after all EtcdClusters are gone) — deleting them # cascade-deletes every remaining EtcdCluster: diff --git a/test/e2e/kamaji_datastore_test.go b/test/e2e/kamaji_datastore_test.go index 790cb2a9..cf8049f7 100644 --- a/test/e2e/kamaji_datastore_test.go +++ b/test/e2e/kamaji_datastore_test.go @@ -174,11 +174,14 @@ func TestKamajiDataStore(t *testing.T) { if err := kube.Get(ctx, client.ObjectKey{Namespace: e2eNamespace, Name: victim}, m); err != nil { t.Fatalf("get member %s before deletion: %v", victim, err) } + // Patch (not Get+Update) so a concurrent member-controller status write + // can't 409 and fail the test spuriously. + base := m.DeepCopy() if m.Annotations == nil { m.Annotations = map[string]string{} } m.Annotations["etcd-operator.cozystack.io/allow-deletion"] = "true" - if err := kube.Update(ctx, m); err != nil { + if err := kube.Patch(ctx, m, client.MergeFrom(base)); err != nil { t.Fatalf("annotate member %s for deletion: %v", victim, err) } if err := kube.Delete(ctx, m); err != nil && !apierrors.IsNotFound(err) {