feat(defrag): make EtcdDefrag spec.clusterRef and spec.rule immutable

api/v1alpha2/defrag_cel_test.go

@@ -0,0 +1,173 @@
+/*
+Copyright 2023 Timofey Larkin.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+*/
+
+package v1alpha2_test
+
+import (
+	"context"
+	"strings"
+	"testing"
+
+	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/resource"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	ctrlclient "sigs.k8s.io/controller-runtime/pkg/client"
+
+	lll "github.com/cozystack/etcd-operator/api/v1alpha2"
+)
+
+func defrag(name string, rule *lll.DefragRule) *lll.EtcdDefrag {
+	return &lll.EtcdDefrag{
+		ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: "default"},
+		Spec: lll.EtcdDefragSpec{
+			ClusterRef: corev1.LocalObjectReference{Name: "c1"},
+			Rule:       rule,
+		},
+	}
+}
+
+func freeSpaceRule(q string) *lll.DefragRule {
+	v := resource.MustParse(q)
+	return &lll.DefragRule{FreeSpaceAbove: &v}
+}
+
+// clusterRef and rule are the immutable request; ttlSecondsAfterFinished is a
+// retention knob that must stay editable after the run finishes.
+func TestCEL_EtcdDefragSpecImmutable(t *testing.T) {
+	skipIfNoEnvtest(t)
+	ctx := context.Background()
+
+	t.Run("create accepted", func(t *testing.T) {
+		d := defrag("df-create", freeSpaceRule("256Mi"))
+		if err := k8s.Create(ctx, d); err != nil {
+			t.Fatalf("Create rejected unexpectedly: %v", err)
+		}
+		t.Cleanup(func() { _ = k8s.Delete(ctx, d) })
+	})
+
+	t.Run("cannot mutate clusterRef", func(t *testing.T) {
+		d := defrag("df-clusterref", nil)
+		if err := k8s.Create(ctx, d); err != nil {
+			t.Fatalf("Create: %v", err)
+		}
+		t.Cleanup(func() { _ = k8s.Delete(ctx, d) })
+
+		got := &lll.EtcdDefrag{}
+		if err := k8s.Get(ctx, ctrlclient.ObjectKeyFromObject(d), got); err != nil {
+			t.Fatalf("Get: %v", err)
+		}
+		got.Spec.ClusterRef.Name = "c2"
+		err := k8s.Update(ctx, got)
+		if err == nil {
+			t.Fatal("apiserver accepted mutating spec.clusterRef; expected rejection")
+		}
+		if !strings.Contains(err.Error(), "spec.clusterRef is immutable") {
+			t.Fatalf("error did not mention the clusterRef rule: %v", err)
+		}
+	})
+
+	t.Run("cannot mutate rule", func(t *testing.T) {
+		d := defrag("df-rule-mutate", freeSpaceRule("256Mi"))
+		if err := k8s.Create(ctx, d); err != nil {
+			t.Fatalf("Create: %v", err)
+		}
+		t.Cleanup(func() { _ = k8s.Delete(ctx, d) })
+
+		got := &lll.EtcdDefrag{}
+		if err := k8s.Get(ctx, ctrlclient.ObjectKeyFromObject(d), got); err != nil {
+			t.Fatalf("Get: %v", err)
+		}
+		got.Spec.Rule = freeSpaceRule("512Mi")
+		err := k8s.Update(ctx, got)
+		if err == nil {
+			t.Fatal("apiserver accepted mutating spec.rule; expected rejection")
+		}
+		if !strings.Contains(err.Error(), "spec.rule is immutable") {
+			t.Fatalf("error did not mention the rule immutability: %v", err)
+		}
+	})
+
+	t.Run("cannot add rule", func(t *testing.T) {
+		d := defrag("df-rule-add", nil)
+		if err := k8s.Create(ctx, d); err != nil {
+			t.Fatalf("Create: %v", err)
+		}
+		t.Cleanup(func() { _ = k8s.Delete(ctx, d) })
+
+		got := &lll.EtcdDefrag{}
+		if err := k8s.Get(ctx, ctrlclient.ObjectKeyFromObject(d), got); err != nil {
+			t.Fatalf("Get: %v", err)
+		}
+		got.Spec.Rule = freeSpaceRule("256Mi")
+		err := k8s.Update(ctx, got)
+		if err == nil {
+			t.Fatal("apiserver accepted adding spec.rule; expected rejection")
+		}
+		if !strings.Contains(err.Error(), "cannot be added to or removed") {
+			t.Fatalf("error did not mention the add/remove rule: %v", err)
+		}
+	})
+
+	t.Run("cannot remove rule", func(t *testing.T) {
+		d := defrag("df-rule-remove", freeSpaceRule("256Mi"))
+		if err := k8s.Create(ctx, d); err != nil {
+			t.Fatalf("Create: %v", err)
+		}
+		t.Cleanup(func() { _ = k8s.Delete(ctx, d) })
+
+		got := &lll.EtcdDefrag{}
+		if err := k8s.Get(ctx, ctrlclient.ObjectKeyFromObject(d), got); err != nil {
+			t.Fatalf("Get: %v", err)
+		}
+		got.Spec.Rule = nil
+		err := k8s.Update(ctx, got)
+		if err == nil {
+			t.Fatal("apiserver accepted removing spec.rule; expected rejection")
+		}
+		if !strings.Contains(err.Error(), "cannot be added to or removed") {
+			t.Fatalf("error did not mention the add/remove rule: %v", err)
+		}
+	})
+
+	t.Run("ttlSecondsAfterFinished stays mutable", func(t *testing.T) {
+		d := defrag("df-ttl", freeSpaceRule("256Mi"))
+		if err := k8s.Create(ctx, d); err != nil {
+			t.Fatalf("Create: %v", err)
+		}
+		t.Cleanup(func() { _ = k8s.Delete(ctx, d) })
+
+		got := &lll.EtcdDefrag{}
+		if err := k8s.Get(ctx, ctrlclient.ObjectKeyFromObject(d), got); err != nil {
+			t.Fatalf("Get: %v", err)
+		}
+		ttl := int32(3600)
+		got.Spec.TTLSecondsAfterFinished = &ttl
+		if err := k8s.Update(ctx, got); err != nil {
+			t.Fatalf("apiserver rejected setting spec.ttlSecondsAfterFinished; it must stay mutable: %v", err)
+		}
+	})
+
+	t.Run("status remains mutable", func(t *testing.T) {
+		d := defrag("df-status", freeSpaceRule("256Mi"))
+		if err := k8s.Create(ctx, d); err != nil {
+			t.Fatalf("Create: %v", err)
+		}
+		t.Cleanup(func() { _ = k8s.Delete(ctx, d) })
+
+		got := &lll.EtcdDefrag{}
+		if err := k8s.Get(ctx, ctrlclient.ObjectKeyFromObject(d), got); err != nil {
+			t.Fatalf("Get: %v", err)
+		}
+		got.Status.Phase = lll.EtcdDefragPhasePending
+		if err := k8s.Status().Update(ctx, got); err != nil {
+			t.Fatalf("apiserver rejected a status update; status must stay mutable: %v", err)
+		}
+	})
+}

api/v1alpha2/etcddefrag_types.go

@@ -25,7 +25,15 @@ import (
 // EtcdDefragSpec is the desired state of an EtcdDefrag: a one-shot request to
 // defragment an EtcdCluster's members.
 //
+// clusterRef and rule are the request itself and are immutable: the controller
+// consults them once and never re-runs, so editing them post-create would
+// silently do nothing. ttlSecondsAfterFinished stays mutable — it is a
+// record-retention knob, not part of the request (see Job.spec.ttlSecondsAfterFinished).
+//
 // +kubebuilder:validation:XValidation:rule="size(self.clusterRef.name) != 0",message="spec.clusterRef.name is required"
+// +kubebuilder:validation:XValidation:rule="self.clusterRef == oldSelf.clusterRef",message="spec.clusterRef is immutable; create a new EtcdDefrag to defragment a different cluster"
+// +kubebuilder:validation:XValidation:rule="has(self.rule) == has(oldSelf.rule)",message="spec.rule cannot be added to or removed from an existing EtcdDefrag; create a new EtcdDefrag to change what it defragments"
+// +kubebuilder:validation:XValidation:rule="!has(self.rule) || !has(oldSelf.rule) || self.rule == oldSelf.rule",message="spec.rule is immutable; create a new EtcdDefrag to change what it defragments"
 type EtcdDefragSpec struct {
 	// ClusterRef names the EtcdCluster (same namespace) to defragment.
 	ClusterRef corev1.LocalObjectReference `json:"clusterRef"`

charts/etcd-operator/crd-bases/etcd-operator.cozystack.io_etcddefrags.yaml

@@ -57,6 +57,11 @@ spec:
             description: |-
               EtcdDefragSpec is the desired state of an EtcdDefrag: a one-shot request to
               defragment an EtcdCluster's members.
+
+              clusterRef and rule are the request itself and are immutable: the controller
+              consults them once and never re-runs, so editing them post-create would
+              silently do nothing. ttlSecondsAfterFinished stays mutable — it is a
+              record-retention knob, not part of the request (see Job.spec.ttlSecondsAfterFinished).
             properties:
               clusterRef:
                 description: ClusterRef names the EtcdCluster (same namespace) to
@@ -144,6 +149,15 @@ spec:
             x-kubernetes-validations:
             - message: spec.clusterRef.name is required
               rule: size(self.clusterRef.name) != 0
+            - message: spec.clusterRef is immutable; create a new EtcdDefrag to defragment
+                a different cluster
+              rule: self.clusterRef == oldSelf.clusterRef
+            - message: spec.rule cannot be added to or removed from an existing EtcdDefrag;
+                create a new EtcdDefrag to change what it defragments
+              rule: has(self.rule) == has(oldSelf.rule)
+            - message: spec.rule is immutable; create a new EtcdDefrag to change what
+                it defragments
+              rule: '!has(self.rule) || !has(oldSelf.rule) || self.rule == oldSelf.rule'
           status:
             description: EtcdDefragStatus is the observed state of an EtcdDefrag.
             properties: