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",