From cfb93ded950cedbeba87029d3ab99c5851c7dd17 Mon Sep 17 00:00:00 2001 From: Kiyoshi Mizumaru Date: Tue, 10 Nov 2026 19:15:46 +0900 Subject: [PATCH] Add namespace label selector for cross-namespace volume sharing Add the trident.netapp.io/shareToNamespaceSelector annotation for source PVCs. When it is set, a new subordinate volume is allowed only if the labels of its namespace match the Kubernetes label selector in the annotation. The namespace is read from the Kubernetes API when the subordinate volume configuration is validated, so removing a required label denies subsequent provisioning. Existing subordinate volumes and mounts are not affected. The selector is an alternative to trident.netapp.io/shareToNamespace. A source PVC that carries both annotations is rejected with an error that names the source PVC and asks the user to set only one of them. Empty or invalid selectors and namespace lookup failures also deny sharing. Behavior is unchanged when the selector annotation is absent, and the subordinate PVC still requires shareFromPVC and a TridentVolumeReference. Co-Authored-By: Claude Opus 5.5 --- .../controller_helpers/kubernetes/config.go | 1 + .../controller_helpers/kubernetes/helper.go | 59 +++- .../kubernetes/helper_test.go | 295 ++++++++++++++++++ 3 files changed, 346 insertions(+), 9 deletions(-) diff --git a/frontend/csi/controller_helpers/kubernetes/config.go b/frontend/csi/controller_helpers/kubernetes/config.go index 1240aab6f..b33b5eedc 100644 --- a/frontend/csi/controller_helpers/kubernetes/config.go +++ b/frontend/csi/controller_helpers/kubernetes/config.go @@ -73,6 +73,7 @@ const ( AnnVolumeCloneToNS = prefix + "/cloneToNamespace" AnnVolumeCloneFromNS = prefix + "/cloneFromNamespace" AnnVolumeShareToNS = prefix + "/shareToNamespace" + AnnVolumeShareToNSSelector = prefix + "/shareToNamespaceSelector" AnnReadOnlyClone = prefix + "/readOnlyClone" AnnLUKSEncryption = prefix + "/luksEncryption" // import only AnnSkipRecoveryQueue = prefix + "/skipRecoveryQueue" diff --git a/frontend/csi/controller_helpers/kubernetes/helper.go b/frontend/csi/controller_helpers/kubernetes/helper.go index 3d0e80386..8e4375f36 100644 --- a/frontend/csi/controller_helpers/kubernetes/helper.go +++ b/frontend/csi/controller_helpers/kubernetes/helper.go @@ -15,6 +15,7 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" "k8s.io/client-go/util/retry" "github.com/netapp/trident/config" @@ -523,16 +524,9 @@ func (h *helper) validateSubordinateVolumeConfig( return err } - // Ensure the source PVC has shared itself with the subordinate volume's namespace - sourceAnnotations := sourcePVC.Annotations - if sourceAnnotations == nil { - sourceAnnotations = make(map[string]string) - } - shareToAnnotation := sourceAnnotations[AnnVolumeShareToNS] - // Ensure the source PVC has been explicitly shared with the subordinate PVC namespace - if !h.matchNamespaceToAnnotation(subordinatePVC.Namespace, shareToAnnotation) { - return fmt.Errorf("subordinate volume source PVC is not shared with namespace %s", subordinatePVC.Namespace) + if err := h.validateNamespaceSharing(ctx, subordinatePVC.Namespace, sourcePVC); err != nil { + return err } // Ensure the subordinate volume and its source volume are the same storage class @@ -562,6 +556,53 @@ func (h *helper) validateSubordinateVolumeConfig( return nil } +// validateNamespaceSharing checks the source PVC's sharing policy at provisioning time. +// The source PVC authorizes namespaces either by name with shareToNamespace or by namespace labels with +// shareToNamespaceSelector. The two annotations are mutually exclusive. +func (h *helper) validateNamespaceSharing( + ctx context.Context, namespace string, sourcePVC *v1.PersistentVolumeClaim, +) error { + annotations := sourcePVC.Annotations + shareTo, hasShareTo := annotations[AnnVolumeShareToNS] + selectorValue, hasSelector := annotations[AnnVolumeShareToNSSelector] + + // Reject an ambiguous policy rather than guessing which annotation the user intended to apply. + if hasShareTo && hasSelector { + return fmt.Errorf("cannot share source PVC %s/%s with namespace %s: the %s and %s annotations are "+ + "mutually exclusive; set only one of them on the source PVC", + sourcePVC.Namespace, sourcePVC.Name, namespace, AnnVolumeShareToNS, AnnVolumeShareToNSSelector) + } + + if !hasSelector { + if !h.matchNamespaceToAnnotation(namespace, shareTo) { + return fmt.Errorf("subordinate volume source PVC is not shared with namespace %s", namespace) + } + return nil + } + + // Empty selectors match every label set in Kubernetes, so reject them for explicit sharing authorization. + if strings.TrimSpace(selectorValue) == "" { + return fmt.Errorf("%s annotation must contain a nonempty namespace label selector", AnnVolumeShareToNSSelector) + } + selector, err := labels.Parse(selectorValue) + if err != nil { + return fmt.Errorf("invalid %s annotation: %w", AnnVolumeShareToNSSelector, err) + } + if selector.Empty() { + return fmt.Errorf("%s annotation must contain a nonempty namespace label selector", AnnVolumeShareToNSSelector) + } + + // Read current namespace labels directly so a later provisioning attempt observes label removal. + targetNamespace, err := h.kubeClient.CoreV1().Namespaces().Get(ctx, namespace, metav1.GetOptions{}) + if err != nil { + return fmt.Errorf("could not get namespace %s to evaluate %s: %w", namespace, AnnVolumeShareToNSSelector, err) + } + if !selector.Matches(labels.Set(targetNamespace.Labels)) { + return fmt.Errorf("subordinate volume source PVC namespace selector does not match namespace %s", namespace) + } + return nil +} + func (h *helper) matchNamespaceToAnnotation(namespace, annotation string) bool { availableToNamespaces := strings.Split(annotation, ",") for _, availableToNamespace := range availableToNamespaces { diff --git a/frontend/csi/controller_helpers/kubernetes/helper_test.go b/frontend/csi/controller_helpers/kubernetes/helper_test.go index c15a3671e..89c59be74 100644 --- a/frontend/csi/controller_helpers/kubernetes/helper_test.go +++ b/frontend/csi/controller_helpers/kubernetes/helper_test.go @@ -9,10 +9,16 @@ import ( vsv1 "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumesnapshot/v1" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" v1 "k8s.io/api/core/v1" k8sstoragev1 "k8s.io/api/storage/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + k8sfake "k8s.io/client-go/kubernetes/fake" + k8stesting "k8s.io/client-go/testing" "k8s.io/client-go/tools/cache" "github.com/netapp/trident/config" @@ -22,6 +28,295 @@ import ( "github.com/netapp/trident/utils/errors" ) +func TestValidateNamespaceSharing(t *testing.T) { + tests := []struct { + name string + annotations map[string]string + labels map[string]string + errorText string + gets int + }{ + { + name: "legacy exact namespace", + annotations: map[string]string{AnnVolumeShareToNS: "other,target"}, + }, + { + name: "legacy wildcard", + annotations: map[string]string{AnnVolumeShareToNS: "*"}, + }, + { + name: "legacy denied", + annotations: map[string]string{AnnVolumeShareToNS: "other"}, + errorText: "not shared", + }, + { + name: "no sharing policy", + errorText: "not shared", + }, + { + name: "selector alone", + annotations: map[string]string{AnnVolumeShareToNSSelector: "team=blue"}, + labels: map[string]string{"team": "blue"}, + gets: 1, + }, + { + name: "selector mismatch", + annotations: map[string]string{AnnVolumeShareToNSSelector: "team=blue"}, + labels: map[string]string{"team": "red"}, + errorText: "does not match", + gets: 1, + }, + { + name: "missing label", + annotations: map[string]string{AnnVolumeShareToNSSelector: "team=blue"}, + errorText: "does not match", + gets: 1, + }, + { + name: "empty selector", + annotations: map[string]string{AnnVolumeShareToNSSelector: ""}, + errorText: "nonempty", + }, + { + name: "whitespace selector", + annotations: map[string]string{AnnVolumeShareToNSSelector: " \t\n"}, + errorText: "nonempty", + }, + { + name: "invalid selector", + annotations: map[string]string{AnnVolumeShareToNSSelector: "team in ("}, + errorText: "invalid", + }, + { + name: "comma only selector", + annotations: map[string]string{AnnVolumeShareToNSSelector: ","}, + errorText: "invalid", + }, + { + name: "set and existence expressions", + annotations: map[string]string{AnnVolumeShareToNSSelector: "team in (blue,green),approved,!blocked"}, + labels: map[string]string{"team": "green", "approved": ""}, + gets: 1, + }, + { + name: "negative expression follows Kubernetes semantics", + annotations: map[string]string{AnnVolumeShareToNSSelector: "team notin (red)"}, + gets: 1, + }, + { + name: "both annotations rejected even when both would allow", + annotations: map[string]string{ + AnnVolumeShareToNS: "target", + AnnVolumeShareToNSSelector: "team=blue", + }, + labels: map[string]string{"team": "blue"}, + errorText: "mutually exclusive", + }, + { + name: "both annotations rejected with wildcard", + annotations: map[string]string{ + AnnVolumeShareToNS: "*", + AnnVolumeShareToNSSelector: "team=blue", + }, + labels: map[string]string{"team": "blue"}, + errorText: "mutually exclusive", + }, + { + name: "both annotations rejected when namespace list would deny", + annotations: map[string]string{ + AnnVolumeShareToNS: "other", + AnnVolumeShareToNSSelector: "team=blue", + }, + labels: map[string]string{"team": "blue"}, + errorText: "mutually exclusive", + }, + { + name: "both annotations rejected with empty namespace list", + annotations: map[string]string{ + AnnVolumeShareToNS: "", + AnnVolumeShareToNSSelector: "team=blue", + }, + labels: map[string]string{"team": "blue"}, + errorText: "mutually exclusive", + }, + { + name: "both annotations rejected with empty selector", + annotations: map[string]string{ + AnnVolumeShareToNS: "*", + AnnVolumeShareToNSSelector: "", + }, + errorText: "mutually exclusive", + }, + { + name: "both annotations rejected with invalid selector", + annotations: map[string]string{ + AnnVolumeShareToNS: "*", + AnnVolumeShareToNSSelector: "team in (", + }, + errorText: "mutually exclusive", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + client := k8sfake.NewSimpleClientset(&v1.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: "target", Labels: tt.labels}, + }) + h := &helper{kubeClient: client} + source := &v1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{Name: "source", Namespace: "source-ns", Annotations: tt.annotations}, + } + err := h.validateNamespaceSharing(context.Background(), "target", source) + if tt.errorText == "" { + require.NoError(t, err) + } else { + require.ErrorContains(t, err, tt.errorText) + } + assert.Len(t, client.Actions(), tt.gets, "legacy, conflicting, and invalid policies must not require namespace API access") + }) + } +} + +func TestValidateNamespaceSharing_NamespaceErrors(t *testing.T) { + for _, apiErr := range []error{ + apierrors.NewNotFound(schema.GroupResource{Resource: "namespaces"}, "target"), + apierrors.NewForbidden(schema.GroupResource{Resource: "namespaces"}, "target", fmt.Errorf("denied")), + apierrors.NewTimeoutError("namespace lookup timed out", 1), + } { + t.Run(apiErr.Error(), func(t *testing.T) { + client := k8sfake.NewSimpleClientset() + client.PrependReactor("get", "namespaces", func(k8stesting.Action) (bool, runtime.Object, error) { + return true, nil, apiErr + }) + h := &helper{kubeClient: client} + source := &v1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "source", + Namespace: "source-ns", + Annotations: map[string]string{AnnVolumeShareToNSSelector: "team=blue"}, + }, + } + err := h.validateNamespaceSharing(context.Background(), "target", source) + require.ErrorContains(t, err, "could not get namespace target") + assert.ErrorIs(t, err, apiErr) + }) + } +} + +func TestValidateNamespaceSharing_ConflictMessage(t *testing.T) { + client := k8sfake.NewSimpleClientset() + h := &helper{kubeClient: client} + source := &v1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "source", + Namespace: "source-ns", + Annotations: map[string]string{ + AnnVolumeShareToNS: "target", + AnnVolumeShareToNSSelector: "team=blue", + }, + }, + } + err := h.validateNamespaceSharing(context.Background(), "target", source) + require.Error(t, err) + // The message is recorded as a PVC event, so it must identify the source PVC, the target namespace, + // both annotations, and the required action. + for _, text := range []string{ + "source-ns/source", "namespace target", AnnVolumeShareToNS + " and " + AnnVolumeShareToNSSelector, + "mutually exclusive", "set only one of them on the source PVC", + } { + assert.Contains(t, err.Error(), text) + } + assert.Empty(t, client.Actions()) +} + +func TestValidateSubordinateVolumeConfig_NamespaceSelector(t *testing.T) { + ctx := context.Background() + client := k8sfake.NewSimpleClientset(&v1.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: "target", Labels: map[string]string{"team": "blue"}}, + }) + h := &helper{ + kubeClient: client, + pvcIndexer: cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{}), + pvIndexer: cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{}), + vrefIndexer: cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{vrefIndex: TridentVolumeReferenceKeyFunc}), + } + source := &v1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "source", + Namespace: "source-ns", + Annotations: map[string]string{AnnVolumeShareToNSSelector: "team=blue"}, + }, + Spec: v1.PersistentVolumeClaimSpec{StorageClassName: new("shared"), VolumeName: "source-pv"}, + Status: v1.PersistentVolumeClaimStatus{Phase: v1.ClaimBound}, + } + pv := &v1.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{Name: "source-pv"}, + Spec: v1.PersistentVolumeSpec{ClaimRef: &v1.ObjectReference{Name: "source", Namespace: "source-ns"}}, + Status: v1.PersistentVolumeStatus{Phase: v1.VolumeBound}, + } + vref := &netappv1.TridentVolumeReference{ + ObjectMeta: metav1.ObjectMeta{Name: "reference", Namespace: "target"}, + Spec: netappv1.TridentVolumeReferenceSpec{PVCName: "source", PVCNamespace: "source-ns"}, + } + require.NoError(t, h.pvcIndexer.Add(source)) + require.NoError(t, h.pvIndexer.Add(pv)) + require.NoError(t, h.vrefIndexer.Add(vref)) + pvc := &v1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "subordinate", + Namespace: "target", + Annotations: map[string]string{AnnVolumeShareFromPVC: "source-ns/source"}, + }, + Spec: v1.PersistentVolumeClaimSpec{StorageClassName: new("shared")}, + } + config := &storage.VolumeConfig{} + require.NoError(t, h.validateSubordinateVolumeConfig(ctx, pvc, config)) + assert.Equal(t, "source-pv", config.ShareSourceVolume) + + // A matching selector does not bypass the existing source validation requirements. + wrongClass := pvc.DeepCopy() + wrongClass.Spec.StorageClassName = new("other") + require.ErrorContains(t, h.validateSubordinateVolumeConfig(ctx, wrongClass, &storage.VolumeConfig{}), "same storage class") + source.Status.Phase = v1.ClaimPending + require.NoError(t, h.pvcIndexer.Update(source)) + require.ErrorContains(t, h.validateSubordinateVolumeConfig(ctx, pvc, &storage.VolumeConfig{}), "source PVC is not bound") + source.Status.Phase = v1.ClaimBound + require.NoError(t, h.pvcIndexer.Update(source)) + pv.Spec.ClaimRef.Name = "unexpected" + require.NoError(t, h.pvIndexer.Update(pv)) + require.ErrorContains(t, h.validateSubordinateVolumeConfig(ctx, pvc, &storage.VolumeConfig{}), "expected PVC") + pv.Spec.ClaimRef.Name = "source" + require.NoError(t, h.pvIndexer.Update(pv)) + require.NoError(t, h.vrefIndexer.Delete(vref)) + require.ErrorContains(t, h.validateSubordinateVolumeConfig(ctx, pvc, &storage.VolumeConfig{}), "volume reference") + require.NoError(t, h.vrefIndexer.Add(vref)) + + // Adding shareToNamespace alongside the selector makes the source policy ambiguous and blocks provisioning. + source.Annotations[AnnVolumeShareToNS] = "target" + require.NoError(t, h.pvcIndexer.Update(source)) + conflictConfig := &storage.VolumeConfig{} + require.ErrorContains(t, h.validateSubordinateVolumeConfig(ctx, pvc, conflictConfig), "mutually exclusive") + assert.Empty(t, conflictConfig.ShareSourceVolume) + delete(source.Annotations, AnnVolumeShareToNS) + require.NoError(t, h.pvcIndexer.Update(source)) + require.NoError(t, h.validateSubordinateVolumeConfig(ctx, pvc, &storage.VolumeConfig{})) + + // Removing the label prevents subsequent provisioning even while the TVR and source caches remain unchanged. + namespace, err := client.CoreV1().Namespaces().Get(ctx, "target", metav1.GetOptions{}) + require.NoError(t, err) + namespace.Labels = nil + _, err = client.CoreV1().Namespaces().Update(ctx, namespace, metav1.UpdateOptions{}) + require.NoError(t, err) + nextConfig := &storage.VolumeConfig{} + require.ErrorContains(t, h.validateSubordinateVolumeConfig(ctx, pvc, nextConfig), "does not match") + assert.Empty(t, nextConfig.ShareSourceVolume) + assert.Equal(t, "source-pv", config.ShareSourceVolume, "existing provisioned volume configuration is unaffected") + + namespace.Labels = map[string]string{"team": "blue"} + _, err = client.CoreV1().Namespaces().Update(ctx, namespace, metav1.UpdateOptions{}) + require.NoError(t, err) + require.NoError(t, h.validateSubordinateVolumeConfig(ctx, pvc, &storage.VolumeConfig{})) +} + func TestGetCaseFoldedAnnotation(t *testing.T) { ann := map[string]string{ "key1": "value1",