From 62d8d623ede28ad0b22d039a48c83a9f622a83ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20B=C3=A4hler?= <26610571+oliverbaehler@users.noreply.github.com> Date: Mon, 17 Aug 2026 10:52:24 +0200 Subject: [PATCH] fix: revert pv labeling to dircet api reads (#2085) * fix: revert pv labeling to dircet api reads Signed-off-by: Oliver Baehler * fix: revert pv labeling to dircet api reads Signed-off-by: Oliver Baehler --------- Signed-off-by: Oliver Baehler --- e2e/storage_pv_cross_tenant_mount_test.go | 62 ++++ internal/controllers/pv/controller.go | 99 +++++-- internal/controllers/pv/controller_test.go | 325 +++++++++++++++++++++ 3 files changed, 468 insertions(+), 18 deletions(-) create mode 100644 internal/controllers/pv/controller_test.go diff --git a/e2e/storage_pv_cross_tenant_mount_test.go b/e2e/storage_pv_cross_tenant_mount_test.go index c0038f92..0fbb39a7 100644 --- a/e2e/storage_pv_cross_tenant_mount_test.go +++ b/e2e/storage_pv_cross_tenant_mount_test.go @@ -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(), diff --git a/internal/controllers/pv/controller.go b/internal/controllers/pv/controller.go index c3e33084..7a0896bc 100644 --- a/internal/controllers/pv/controller.go +++ b/internal/controllers/pv/controller.go @@ -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 +} diff --git a/internal/controllers/pv/controller_test.go b/internal/controllers/pv/controller_test.go new file mode 100644 index 00000000..c3e75b51 --- /dev/null +++ b/internal/controllers/pv/controller_test.go @@ -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...) +}