fix: revert pv labeling to dircet api reads (#2085)

* fix: revert pv labeling to dircet api reads Signed-off-by: Oliver Baehler <oliver@sudo-i.net> * fix: revert pv labeling to dircet api reads Signed-off-by: Oliver Baehler <oliver@sudo-i.net> --------- Signed-off-by: Oliver Baehler <oliver@sudo-i.net>
2026-08-19 04:26:45 +00:00 · 2026-08-17 10:52:24 +02:00
parent 5e9080e7b6
commit 62d8d623ed
3 changed files with 468 additions and 18 deletions
@@ -5,6 +5,7 @@ package e2e

 import (
 	"context"
+	"fmt"

 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
@@ -219,6 +220,67 @@ var _ = Describe("preventing PersistentVolume cross-tenant mount", Ordered, Labe
 		}, defaultTimeoutInterval, defaultPollInterval).Should(HaveOccurred())
 	})

+	It("should repair empty and stale Tenant labels on claimed PersistentVolumes", Label("skip-on-openshift"), func() {
+		ns := NewNamespace("", map[string]string{
+			meta.TenantLabel: tnt1.GetName(),
+		})
+		NamespaceCreation(ns, tnt1.Spec.Owners[0].UserSpec, defaultTimeoutInterval).Should(Succeed())
+
+		volumes := make([]*corev1.PersistentVolume, 0, 2)
+		defer func() {
+			for _, pv := range volumes {
+				_ = k8sClient.Delete(context.Background(), pv)
+			}
+		}()
+
+		for index, value := range []string{"", tnt2.Name} {
+			pv := &corev1.PersistentVolume{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: fmt.Sprintf("repair-tenant-label-%d", index),
+					Labels: map[string]string{
+						meta.TenantLabel: value,
+					},
+				},
+				Spec: corev1.PersistentVolumeSpec{
+					Capacity: corev1.ResourceList{
+						corev1.ResourceStorage: resource.MustParse("1Gi"),
+					},
+					AccessModes: []corev1.PersistentVolumeAccessMode{
+						corev1.ReadWriteOnce,
+					},
+					PersistentVolumeReclaimPolicy: corev1.PersistentVolumeReclaimRetain,
+					StorageClassName:              "manual",
+					ClaimRef: &corev1.ObjectReference{
+						APIVersion: "v1",
+						Kind:       "PersistentVolumeClaim",
+						Namespace:  ns.Name,
+						Name:       fmt.Sprintf("repair-tenant-label-%d", index),
+					},
+					PersistentVolumeSource: corev1.PersistentVolumeSource{
+						HostPath: &corev1.HostPathVolumeSource{
+							Path: fmt.Sprintf("/tmp/capsule-e2e-repair-tenant-label-%d", index),
+						},
+					},
+				},
+			}
+			volumes = append(volumes, pv)
+
+			EventuallyCreation(func() error {
+				return k8sClient.Create(context.Background(), pv)
+			}).Should(Succeed())
+
+			Eventually(func(g Gomega) {
+				current := &corev1.PersistentVolume{}
+				g.Expect(k8sClient.Get(
+					context.Background(),
+					types.NamespacedName{Name: pv.Name},
+					current,
+				)).To(Succeed())
+				g.Expect(current.GetLabels()).To(HaveKeyWithValue(meta.TenantLabel, tnt1.Name))
+			}, defaultTimeoutInterval, defaultPollInterval).Should(Succeed())
+		}
+	})
+
 	It("should not add a selector when updating an already-bound dynamic PVC without selector", func() {
 		ns := NewNamespace("", map[string]string{
 			meta.TenantLabel: tnt1.GetName(),
@@ -5,13 +5,16 @@ package pv

 import (
 	"context"
+	"fmt"

 	corev1 "k8s.io/api/core/v1"
+	apiequality "k8s.io/apimachinery/pkg/api/equality"
 	"k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/client-go/util/retry"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/builder"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/event"
 	log2 "sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/predicate"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
@@ -24,6 +27,7 @@ import (

 type Controller struct {
 	client client.Client
+	reader client.Reader
 	label  string
 }

@@ -34,29 +38,50 @@ func (c *Controller) SetupWithManager(mgr ctrl.Manager, ctrlConfig utils.Control
 	}

 	c.client = mgr.GetClient()
+	c.reader = mgr.GetAPIReader()
 	c.label = label

 	return ctrl.NewControllerManagedBy(mgr).
 		Named("capsule/persistentvolumes").
-		For(&corev1.PersistentVolume{}, builder.WithPredicates(predicate.NewPredicateFuncs(func(object client.Object) bool {
-			pv, ok := object.(*corev1.PersistentVolume)
-			if !ok {
-				return false
-			}
-
-			if pv.Spec.ClaimRef == nil {
-				return false
-			}
-
-			labels := object.GetLabels()
-			_, ok = labels[c.label]
-
-			return !ok
-		}))).
+		For(&corev1.PersistentVolume{}, builder.WithPredicates(persistentVolumePredicate(c.label))).
 		WithOptions(ctrlConfig.Runtime.ToControllerOptions()).
 		Complete(c)
 }

+func persistentVolumePredicate(label string) predicate.Predicate {
+	return predicate.Funcs{
+		CreateFunc: func(e event.CreateEvent) bool {
+			pv, ok := e.Object.(*corev1.PersistentVolume)
+
+			return ok && pv.Spec.ClaimRef != nil
+		},
+		DeleteFunc: func(event.DeleteEvent) bool {
+			return false
+		},
+		UpdateFunc: func(e event.UpdateEvent) bool {
+			oldPV, oldOK := e.ObjectOld.(*corev1.PersistentVolume)
+
+			newPV, newOK := e.ObjectNew.(*corev1.PersistentVolume)
+			if !oldOK || !newOK || newPV.Spec.ClaimRef == nil {
+				return false
+			}
+
+			return !apiequality.Semantic.DeepEqual(oldPV.Spec.ClaimRef, newPV.Spec.ClaimRef) ||
+				persistentVolumeLabelChanged(oldPV, newPV, label)
+		},
+		GenericFunc: func(event.GenericEvent) bool {
+			return false
+		},
+	}
+}
+
+func persistentVolumeLabelChanged(oldPV, newPV *corev1.PersistentVolume, label string) bool {
+	oldValue, oldExists := oldPV.GetLabels()[label]
+	newValue, newExists := newPV.GetLabels()[label]
+
+	return oldExists != newExists || oldValue != newValue
+}
+
 func (c *Controller) Reconcile(ctx context.Context, request reconcile.Request) (reconcile.Result, error) {
 	log := log2.FromContext(ctx)

@@ -79,19 +104,23 @@ func (c *Controller) Reconcile(ctx context.Context, request reconcile.Request) (
 		return reconcile.Result{}, nil
 	}

-	tnt, err := tenant.GetTenantNameByStatusNamespace(ctx, c.client, persistentVolume.Spec.ClaimRef.Namespace)
+	tnt, err := c.resolveNamespaceTenant(ctx, persistentVolume.Spec.ClaimRef.Namespace)
 	if err != nil {
 		log.Error(err, "unable to retrieve Tenant from the claimRef")

 		return reconcile.Result{}, err
 	}

-	if tnt == "" {
+	if tnt == nil {
 		log.V(4).Info("skipping reconciliation, PV is claimed by a PVC not managed in a Tenant")

 		return reconcile.Result{}, nil
 	}

+	if persistentVolume.GetLabels()[c.label] == tnt.GetName() {
+		return reconcile.Result{}, nil
+	}
+
 	retryErr := retry.RetryOnConflict(retry.DefaultRetry, func() error {
 		pv := persistentVolume

@@ -99,12 +128,20 @@ func (c *Controller) Reconcile(ctx context.Context, request reconcile.Request) (
 			return err
 		}

+		if pv.Spec.ClaimRef == nil || pv.Spec.ClaimRef.Namespace != persistentVolume.Spec.ClaimRef.Namespace {
+			return nil
+		}
+
 		labels := pv.GetLabels()
 		if labels == nil {
 			labels = map[string]string{}
 		}

-		labels[c.label] = tnt
+		if labels[c.label] == tnt.GetName() {
+			return nil
+		}
+
+		labels[c.label] = tnt.GetName()

 		pv.SetLabels(labels)

@@ -118,3 +155,29 @@ func (c *Controller) Reconcile(ctx context.Context, request reconcile.Request) (

 	return reconcile.Result{}, nil
 }
+
+func (c *Controller) resolveNamespaceTenant(
+	ctx context.Context,
+	namespace string,
+) (*capsulev1beta2.Tenant, error) {
+	reader := c.reader
+	if reader == nil {
+		reader = c.client
+	}
+
+	ns := &corev1.Namespace{}
+	if err := reader.Get(ctx, client.ObjectKey{Name: namespace}, ns); err != nil {
+		if errors.IsNotFound(err) {
+			return nil, nil
+		}
+
+		return nil, fmt.Errorf("get claim namespace %q: %w", namespace, err)
+	}
+
+	tnt, err := tenant.ResolveNamespaceTenant(ctx, reader, ns)
+	if err != nil {
+		return nil, fmt.Errorf("resolve claim namespace %q: %w", namespace, err)
+	}
+
+	return tnt, nil
+}
@@ -0,0 +1,325 @@
+// Copyright 2020-2026 Project Capsule Authors
+// SPDX-License-Identifier: Apache-2.0
+
+package pv
+
+import (
+	"context"
+	"errors"
+	"fmt"
+	"testing"
+
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/types"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+	"sigs.k8s.io/controller-runtime/pkg/event"
+	"sigs.k8s.io/controller-runtime/pkg/reconcile"
+
+	capsulev1beta2 "github.com/projectcapsule/capsule/api/v1beta2"
+	"github.com/projectcapsule/capsule/pkg/api/meta"
+)
+
+func TestReconcileRepairsPersistentVolumeTenantLabelFromNamespace(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name        string
+		labels      map[string]string
+		wantUpdates int
+	}{
+		{
+			name:        "missing label",
+			wantUpdates: 1,
+		},
+		{
+			name:        "empty label",
+			labels:      map[string]string{meta.TenantLabel: ""},
+			wantUpdates: 1,
+		},
+		{
+			name:        "stale label",
+			labels:      map[string]string{meta.TenantLabel: "another-tenant"},
+			wantUpdates: 1,
+		},
+		{
+			name:        "current label",
+			labels:      map[string]string{meta.TenantLabel: "tenant-a"},
+			wantUpdates: 0,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			scheme := persistentVolumeTestScheme(t)
+			tnt := persistentVolumeTestTenant()
+			ns := persistentVolumeTestNamespace(tnt)
+			pv := persistentVolumeTestVolume(ns.Name, tt.labels)
+			base := fake.NewClientBuilder().WithScheme(scheme).WithObjects(tnt, ns, pv).Build()
+			counting := &updateCountingClient{Client: base}
+			controller := &Controller{
+				client: counting,
+				reader: base,
+				label:  meta.TenantLabel,
+			}
+
+			if _, err := controller.Reconcile(context.Background(), reconcile.Request{
+				NamespacedName: types.NamespacedName{Name: pv.Name},
+			}); err != nil {
+				t.Fatalf("Reconcile() error = %v", err)
+			}
+
+			updated := &corev1.PersistentVolume{}
+			if err := base.Get(context.Background(), client.ObjectKeyFromObject(pv), updated); err != nil {
+				t.Fatalf("get PersistentVolume: %v", err)
+			}
+
+			if got := updated.GetLabels()[meta.TenantLabel]; got != tnt.Name {
+				t.Fatalf("tenant label = %q, want %q", got, tnt.Name)
+			}
+
+			if counting.updates != tt.wantUpdates {
+				t.Fatalf("updates = %d, want %d", counting.updates, tt.wantUpdates)
+			}
+		})
+	}
+}
+
+func TestReconcileSkipsPersistentVolumeFromUnmanagedOrDeletedNamespace(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name      string
+		namespace *corev1.Namespace
+	}{
+		{
+			name: "unmanaged namespace",
+			namespace: &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{
+				Name: "unmanaged",
+			}},
+		},
+		{
+			name: "deleted namespace",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			scheme := persistentVolumeTestScheme(t)
+			objects := []client.Object{}
+			namespace := "deleted"
+			if tt.namespace != nil {
+				objects = append(objects, tt.namespace)
+				namespace = tt.namespace.Name
+			}
+
+			pv := persistentVolumeTestVolume(namespace, nil)
+			objects = append(objects, pv)
+			base := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objects...).Build()
+			counting := &updateCountingClient{Client: base}
+			controller := &Controller{client: counting, reader: base, label: meta.TenantLabel}
+
+			if _, err := controller.Reconcile(context.Background(), reconcile.Request{
+				NamespacedName: types.NamespacedName{Name: pv.Name},
+			}); err != nil {
+				t.Fatalf("Reconcile() error = %v", err)
+			}
+
+			if counting.updates != 0 {
+				t.Fatalf("updates = %d, want 0", counting.updates)
+			}
+		})
+	}
+}
+
+func TestReconcileRetriesInconsistentOrFailedNamespaceResolution(t *testing.T) {
+	t.Parallel()
+
+	t.Run("inconsistent ownership", func(t *testing.T) {
+		t.Parallel()
+
+		scheme := persistentVolumeTestScheme(t)
+		tnt := persistentVolumeTestTenant()
+		ns := persistentVolumeTestNamespace(tnt)
+		delete(ns.Labels, meta.TenantLabel)
+		pv := persistentVolumeTestVolume(ns.Name, nil)
+		base := fake.NewClientBuilder().WithScheme(scheme).WithObjects(tnt, ns, pv).Build()
+		controller := &Controller{client: base, reader: base, label: meta.TenantLabel}
+
+		_, err := controller.Reconcile(context.Background(), reconcile.Request{
+			NamespacedName: types.NamespacedName{Name: pv.Name},
+		})
+		if err == nil {
+			t.Fatal("Reconcile() error = nil, want inconsistent ownership error")
+		}
+	})
+
+	t.Run("reader failure", func(t *testing.T) {
+		t.Parallel()
+
+		scheme := persistentVolumeTestScheme(t)
+		pv := persistentVolumeTestVolume("tenant-a-ns", nil)
+		base := fake.NewClientBuilder().WithScheme(scheme).WithObjects(pv).Build()
+		controller := &Controller{
+			client: base,
+			reader: &namespaceFailingReader{
+				Reader: base,
+				err:    errors.New("temporary API failure"),
+			},
+			label: meta.TenantLabel,
+		}
+
+		_, err := controller.Reconcile(context.Background(), reconcile.Request{
+			NamespacedName: types.NamespacedName{Name: pv.Name},
+		})
+		if err == nil || !errors.Is(err, controller.reader.(*namespaceFailingReader).err) {
+			t.Fatalf("Reconcile() error = %v, want temporary API failure", err)
+		}
+	})
+}
+
+func TestPersistentVolumePredicate(t *testing.T) {
+	t.Parallel()
+
+	pred := persistentVolumePredicate(meta.TenantLabel)
+	claimed := persistentVolumeTestVolume("tenant-a-ns", map[string]string{meta.TenantLabel: "tenant-a"})
+	unclaimed := claimed.DeepCopy()
+	unclaimed.Spec.ClaimRef = nil
+
+	if !pred.Create(event.CreateEvent{Object: claimed}) {
+		t.Fatal("claimed PersistentVolume create should reconcile")
+	}
+
+	if pred.Create(event.CreateEvent{Object: unclaimed}) {
+		t.Fatal("unclaimed PersistentVolume create should not reconcile")
+	}
+
+	if pred.Delete(event.DeleteEvent{Object: claimed}) {
+		t.Fatal("PersistentVolume delete should not reconcile")
+	}
+
+	if pred.Generic(event.GenericEvent{Object: claimed}) {
+		t.Fatal("PersistentVolume generic event should not reconcile")
+	}
+
+	labelChanged := claimed.DeepCopy()
+	labelChanged.Labels[meta.TenantLabel] = ""
+	if !pred.Update(event.UpdateEvent{ObjectOld: claimed, ObjectNew: labelChanged}) {
+		t.Fatal("tenant label change should reconcile")
+	}
+
+	missingLabel := claimed.DeepCopy()
+	delete(missingLabel.Labels, meta.TenantLabel)
+	emptyLabel := missingLabel.DeepCopy()
+	emptyLabel.Labels[meta.TenantLabel] = ""
+
+	if !pred.Update(event.UpdateEvent{ObjectOld: missingLabel, ObjectNew: emptyLabel}) {
+		t.Fatal("adding an empty tenant label should reconcile")
+	}
+
+	if !pred.Update(event.UpdateEvent{ObjectOld: emptyLabel, ObjectNew: missingLabel}) {
+		t.Fatal("removing an empty tenant label should reconcile")
+	}
+
+	claimChanged := claimed.DeepCopy()
+	claimChanged.Spec.ClaimRef = claimChanged.Spec.ClaimRef.DeepCopy()
+	claimChanged.Spec.ClaimRef.Namespace = "tenant-b-ns"
+	if !pred.Update(event.UpdateEvent{ObjectOld: claimed, ObjectNew: claimChanged}) {
+		t.Fatal("claim reference change should reconcile")
+	}
+
+	unrelatedChange := claimed.DeepCopy()
+	unrelatedChange.Status.Phase = corev1.VolumeBound
+	if pred.Update(event.UpdateEvent{ObjectOld: claimed, ObjectNew: unrelatedChange}) {
+		t.Fatal("unrelated PersistentVolume update should not reconcile")
+	}
+}
+
+func persistentVolumeTestScheme(t *testing.T) *runtime.Scheme {
+	t.Helper()
+
+	scheme := runtime.NewScheme()
+	if err := corev1.AddToScheme(scheme); err != nil {
+		t.Fatalf("add core scheme: %v", err)
+	}
+
+	if err := capsulev1beta2.AddToScheme(scheme); err != nil {
+		t.Fatalf("add Capsule scheme: %v", err)
+	}
+
+	return scheme
+}
+
+func persistentVolumeTestTenant() *capsulev1beta2.Tenant {
+	return &capsulev1beta2.Tenant{ObjectMeta: metav1.ObjectMeta{
+		Name: "tenant-a",
+		UID:  types.UID("tenant-a-uid"),
+	}}
+}
+
+func persistentVolumeTestNamespace(tnt *capsulev1beta2.Tenant) *corev1.Namespace {
+	return &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{
+		Name:   "tenant-a-ns",
+		Labels: map[string]string{meta.TenantLabel: tnt.Name},
+		OwnerReferences: []metav1.OwnerReference{{
+			APIVersion: capsulev1beta2.GroupVersion.String(),
+			Kind:       "Tenant",
+			Name:       tnt.Name,
+			UID:        tnt.UID,
+		}},
+	}}
+}
+
+func persistentVolumeTestVolume(namespace string, labels map[string]string) *corev1.PersistentVolume {
+	return &corev1.PersistentVolume{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:   "pv-" + namespace,
+			Labels: labels,
+		},
+		Spec: corev1.PersistentVolumeSpec{ClaimRef: &corev1.ObjectReference{
+			Namespace: namespace,
+			Name:      "claim",
+		}},
+	}
+}
+
+type updateCountingClient struct {
+	client.Client
+
+	updates int
+}
+
+func (c *updateCountingClient) Update(
+	ctx context.Context,
+	obj client.Object,
+	opts ...client.UpdateOption,
+) error {
+	c.updates++
+
+	return c.Client.Update(ctx, obj, opts...)
+}
+
+type namespaceFailingReader struct {
+	client.Reader
+
+	err error
+}
+
+func (r *namespaceFailingReader) Get(
+	ctx context.Context,
+	key client.ObjectKey,
+	obj client.Object,
+	opts ...client.GetOption,
+) error {
+	if _, ok := obj.(*corev1.Namespace); ok {
+		return fmt.Errorf("get namespace: %w", r.err)
+	}
+
+	return r.Reader.Get(ctx, key, obj, opts...)
+}