fix: do not mutate on update and bound pvcs (#2073)

* fix: do not mutate on update and bound pvcs

Signed-off-by: Oliver Baehler <oliver@sudo-i.net>

* fix: do not mutate on update and bound pvcs

Signed-off-by: Oliver Baehler <oliver@sudo-i.net>

---------

Signed-off-by: Oliver Baehler <oliver@sudo-i.net>

internal/webhook/pvc/handler.go

@@ -51,6 +51,14 @@ func requiresPVCSpecValidation(
 	pvc *corev1.PersistentVolumeClaim,
 	oldPVC *corev1.PersistentVolumeClaim,
 ) bool {
+	// A bound PVC's volume binding fields, including its selector, are immutable.
+	// Reapplying the tenant selector during an update would make otherwise valid
+	// metadata or resize updates fail for claims created before Capsule enforced it.
+	if req.Operation == admissionv1.Update &&
+		isBoundPVC(oldPVC) {
+		return false
+	}
+
 	// Finalizer cleanup must remain possible after a bound PV has disappeared.
 	// Continue validating any update that changes the PVC spec.
 	if req.Operation != admissionv1.Update ||
@@ -62,3 +70,7 @@ func requiresPVCSpecValidation(
 
 	return !apiequality.Semantic.DeepEqual(pvc.Spec, oldPVC.Spec)
 }
+
+func isBoundPVC(pvc *corev1.PersistentVolumeClaim) bool {
+	return pvc != nil && pvc.Status.Phase == corev1.ClaimBound
+}

internal/webhook/pvc/handler_test.go

@@ -10,11 +10,15 @@ import (
 
 	admissionv1 "k8s.io/api/admission/v1"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
+
+	capsulev1beta2 "github.com/projectcapsule/capsule/api/v1beta2"
+	"github.com/projectcapsule/capsule/pkg/runtime/handlers"
 )
 
 func TestMutatingHandlerSkipsDynamicClaimsBeforeTenantLookup(t *testing.T) {
@@ -78,6 +82,114 @@ func TestValidatingHandlerSkipsTerminatingClaimWithUnchangedSpec(t *testing.T) {
 	}
 }
 
+func TestHandlersSkipBoundClaimsBeforeTenantLookup(t *testing.T) {
+	t.Parallel()
+
+	scheme := runtime.NewScheme()
+	if err := corev1.AddToScheme(scheme); err != nil {
+		t.Fatal(err)
+	}
+
+	oldPVC := &corev1.PersistentVolumeClaim{
+		ObjectMeta: metav1.ObjectMeta{Name: "nginx-logs"},
+		Spec: corev1.PersistentVolumeClaimSpec{
+			Selector: &metav1.LabelSelector{MatchLabels: map[string]string{
+				"velero.io/dynamic-pv-restore": "test.nginx-logs.sg75p",
+			}},
+			VolumeName: "pvc-f9bc7e8d",
+		},
+		Status: corev1.PersistentVolumeClaimStatus{Phase: corev1.ClaimBound},
+	}
+	newPVC := oldPVC.DeepCopy()
+	newPVC.Labels = map[string]string{"test": "test"}
+
+	tests := []struct {
+		name    string
+		handler handlers.Handler
+	}{
+		{
+			name:    "mutating",
+			handler: MutatingHandler(PersistentVolumeMutatingVolume()),
+		},
+		{
+			name:    "validating",
+			handler: Handler(PersistentVolumeValidatingVolume()),
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			base := fake.NewClientBuilder().WithScheme(scheme).Build()
+			reader := &pvcCountingReader{Reader: base}
+			request := pvcUpdateAdmissionRequest(t, oldPVC, newPVC)
+
+			if response := tt.handler.OnUpdate(nil, reader, admission.NewDecoder(scheme), nil)(
+				context.Background(),
+				request,
+			); response != nil {
+				t.Fatalf("response = %#v, want nil", response)
+			}
+
+			if reader.gets != 0 {
+				t.Fatalf("tenant lookup gets = %d, want 0 for a bound claim", reader.gets)
+			}
+		})
+	}
+}
+
+func TestVolumeHooksSkipBoundClaimUpdates(t *testing.T) {
+	t.Parallel()
+
+	scheme := runtime.NewScheme()
+	if err := corev1.AddToScheme(scheme); err != nil {
+		t.Fatal(err)
+	}
+
+	oldPVC := &corev1.PersistentVolumeClaim{
+		ObjectMeta: metav1.ObjectMeta{Name: "nginx-logs"},
+		Spec: corev1.PersistentVolumeClaimSpec{
+			Selector: &metav1.LabelSelector{MatchLabels: map[string]string{
+				"velero.io/dynamic-pv-restore": "test.nginx-logs.sg75p",
+			}},
+			VolumeName: "pvc-f9bc7e8d",
+		},
+		Status: corev1.PersistentVolumeClaimStatus{Phase: corev1.ClaimBound},
+	}
+	newPVC := oldPVC.DeepCopy()
+	newPVC.Labels = map[string]string{"test": "test"}
+	tnt := &capsulev1beta2.Tenant{ObjectMeta: metav1.ObjectMeta{Name: "test"}}
+	request := pvcUpdateAdmissionRequest(t, oldPVC, newPVC)
+	decoder := admission.NewDecoder(scheme)
+
+	if response := PersistentVolumeMutatingVolume().OnUpdate(
+		nil,
+		nil,
+		oldPVC,
+		newPVC,
+		decoder,
+		nil,
+		tnt,
+	)(context.Background(), request); response != nil {
+		t.Fatalf("mutating response = %#v, want nil", response)
+	}
+
+	if len(newPVC.Spec.Selector.MatchExpressions) != 0 {
+		t.Fatalf("selector expressions = %#v, want unchanged", newPVC.Spec.Selector.MatchExpressions)
+	}
+
+	if response := PersistentVolumeValidatingVolume().OnUpdate(
+		nil,
+		nil,
+		oldPVC,
+		newPVC,
+		decoder,
+		nil,
+		tnt,
+	)(context.Background(), request); response != nil {
+		t.Fatalf("validating response = %#v, want nil", response)
+	}
+}
+
 func TestRequiresPVCSpecValidation(t *testing.T) {
 	t.Parallel()
 
@@ -90,6 +202,12 @@ func TestRequiresPVCSpecValidation(t *testing.T) {
 	changed.Spec.VolumeName = "salusa"
 	active := terminating.DeepCopy()
 	active.DeletionTimestamp = nil
+	bound := active.DeepCopy()
+	bound.Status.Phase = corev1.ClaimBound
+	resizedBound := bound.DeepCopy()
+	resizedBound.Spec.Resources.Requests = corev1.ResourceList{
+		corev1.ResourceStorage: resource.MustParse("2Gi"),
+	}
 
 	tests := []struct {
 		name      string
@@ -111,6 +229,20 @@ func TestRequiresPVCSpecValidation(t *testing.T) {
 			pvc:       active,
 			want:      true,
 		},
+		{
+			name:      "bound metadata update",
+			operation: admissionv1.Update,
+			oldPVC:    bound.DeepCopy(),
+			pvc:       bound,
+			want:      false,
+		},
+		{
+			name:      "bound resize",
+			operation: admissionv1.Update,
+			oldPVC:    bound.DeepCopy(),
+			pvc:       resizedBound,
+			want:      false,
+		},
 		{
 			name:      "terminating finalizer update",
 			operation: admissionv1.Update,

internal/webhook/pvc/pvc_mutating_volume.go

@@ -67,6 +67,10 @@ func (h persistentVolumeMutatingVolume) OnUpdate(
 	tnt *capsulev1beta2.Tenant,
 ) handlers.Func {
 	return func(ctx context.Context, req admission.Request) *admission.Response {
+		if isBoundPVC(oldPVC) {
+			return nil
+		}
+
 		if newPVC == nil || tnt == nil {
 			return nil
 		}

internal/webhook/pvc/pvc_validating_volume.go

@@ -55,6 +55,10 @@ func (h persistentVolumeValidatingVolume) OnUpdate(
 	tnt *capsulev1beta2.Tenant,
 ) handlers.Func {
 	return func(ctx context.Context, req admission.Request) *admission.Response {
+		if isBoundPVC(oldPVC) {
+			return nil
+		}
+
 		if err := validatePVCSelector(newPVC, tnt); err != nil {
 			return ad.ErroredResponse(err)
 		}