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..a8807f8e --- /dev/null +++ b/charts/etcd-operator/templates/member-deletion-policy.yaml @@ -0,0 +1,99 @@ +{{- /* +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. + +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; + - 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. + +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. + +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:kube-controller-manager") + .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"] +{{- 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 new file mode 100644 index 00000000..fc20c12b --- /dev/null +++ b/charts/etcd-operator/tests/member_deletion_policy_test.yaml @@ -0,0 +1,165 @@ +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, the namespace controller and kube-controller-manager + 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 + # 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: + 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 + + # 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 04233fbb..d4ff0c56 100644 --- a/charts/etcd-operator/values.yaml +++ b/charts/etcd-operator/values.yaml @@ -9,6 +9,33 @@ 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. +# +# 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 + # -- 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..39d9f302 100644 --- a/docs/concepts.md +++ b/docs/concepts.md @@ -10,12 +10,39 @@ 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, 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. + +**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 `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..c6330930 100644 --- a/docs/installation.md +++ b/docs/installation.md @@ -311,6 +311,17 @@ kubectl delete etcdcluster.etcd-operator.cozystack.io --all -A # intentionally left in place. make undeploy +# 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: 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 diff --git a/test/e2e/kamaji_datastore_test.go b/test/e2e/kamaji_datastore_test.go index a93bd9de..cf8049f7 100644 --- a/test/e2e/kamaji_datastore_test.go +++ b/test/e2e/kamaji_datastore_test.go @@ -165,7 +165,25 @@ 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) + } + // 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.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) { t.Fatalf("delete member %s: %v", victim, err) }