From 9df572271a5b1cbcbcfb74b19f9ee9d6f33225fa Mon Sep 17 00:00:00 2001 From: MaBing Date: Mon, 28 Sep 2026 15:43:50 +0800 Subject: [PATCH] fix(dataset): enforce inherited reference grants Check every ancestor's policy against each downstream namespace so multi-hop references cannot bypass an intermediate denial or read-only grant. Reconcile transitive dependents when namespace labels change, and reuse compiled selectors during resolution. Existing reference chains may become denied or read-only after upgrade if an ancestor does not grant an intermediate namespace. --- ...-repository-share-access-implementation.md | 21 +++ .../controller/dataset/dataset_controller.go | 74 +++++---- .../dataset/dataset_controller_test.go | 41 +++++ .../dataset/mount_policy_additional_test.go | 62 +++++++- pkg/mountpolicy/policy.go | 141 +++++++++++++----- pkg/mountpolicy/policy_resolution_test.go | 97 ++++++++++++ pkg/mountpolicy/policy_test.go | 4 +- 7 files changed, 364 insertions(+), 76 deletions(-) create mode 100644 pkg/mountpolicy/policy_resolution_test.go diff --git a/docs/dataset-repository-share-access-implementation.md b/docs/dataset-repository-share-access-implementation.md index 10a272a..6661301 100644 --- a/docs/dataset-repository-share-access-implementation.md +++ b/docs/dataset-repository-share-access-implementation.md @@ -131,6 +131,25 @@ For C's final namespace: Do not treat an upstream legacy `status.readOnly=false` as authorization; resolve the current chain again. +Each ancestor must authorize every distinct namespace below it in the reference chain, including the final namespace. For `target/C -> b/B -> a/A`, check B's grant to `target`, A's grant to `target`, and A's grant to `b`. For longer chains, apply the same rule to each intermediate reference's effective permission. Repeated namespaces do not require repeated checks. Validate and parse each source policy once per resolution, then reuse its selectors for these checks. + +### 3.3 Upgrade Compatibility + +Intermediate-namespace authorization extends the previous final-namespace checks. Existing references can become denied or read-only even when the root source still grants read-write access to the final namespace: + +| Reference path | Relevant grants | Result | +| --- | --- | --- | +| `target/C -> a/A` | A grants `target` read-write | Read-write; namespace `b` is not on this path | +| `target/C -> b/B -> a/A` | A and B grant `target` read-write; A denies `b` | Denied | +| `target/C -> b/B -> a/A` | A and B grant `target` read-write; A grants `b` read-only | Read-only | +| `target/C -> b/B -> a/A` | All three required grants are read-write | Read-write | + +These results assume valid source identities and PVC/PV bindings. A denial sets `MountPolicy=False` and conservatively sets `status.readOnly=true`; this does not permit read-only mounting. Denial messages identify the final Dataset, the ancestor source, and the namespace whose grant failed. B's own cached MountPolicy is not an authorization check for C. + +Before upgrading, inspect existing reference chains and their current namespace labels. Ensure every required intermediate grant matches the intended access. If an intermediate namespace should not receive access, create a new direct reference to an authorized source when appropriate; the existing reference URI is immutable. Consumers must call `Verify` before creating Pods and handle a denied or changed grant. This verification does not revoke mounts in already-running Pods. + +Namespace label events propagate to all transitive reference dependents because those labels now affect downstream permissions. Deploy this propagation with the intermediate-namespace checks. If those checks are reverted, reassess and normally remove the propagation change too; leaving it in place causes extra reconciliation but does not itself tighten authorization. + ## 4. Dataset Controller Modify `internal/controller/dataset/dataset_controller.go`. @@ -183,6 +202,8 @@ Add at least the following tests: - When JSON/unstructured representation omits `status.ReadOnly=false`, `Verify` may return writable only with `MountPolicy=True` and matching `observedGeneration`. - Read-only wins where overlapping rules match the same namespace. - Multi-level references: upstream read-only, upstream not authorizing the final namespace, fully read-write chains, cycles, and excessive depth. +- Intermediate namespace grants: deny and read-only must propagate despite read-write grants to the final namespace; direct references remain independent of namespaces outside their chain. Cover repeated namespaces and 32-level chains with both shared and distinct namespaces. +- Namespace label events enqueue references in that namespace and all transitive dependents exactly once, excluding unrelated references and namespaces without references. - Effective permission is reconciled again after a Ready PVC when namespace workspace labels or source policy changes. - A REFERENCE that enters Failed due to a temporary source/policy error automatically recovers through periodic requeue even without an additional watch event. - A PVC-preparation failure (such as a source without a PVC or a PVC not bound to a PV) must set `MountPolicy=False` and must not leave an old condition behind. diff --git a/internal/controller/dataset/dataset_controller.go b/internal/controller/dataset/dataset_controller.go index 69c6d34..71fa2a4 100644 --- a/internal/controller/dataset/dataset_controller.go +++ b/internal/controller/dataset/dataset_controller.go @@ -1315,50 +1315,60 @@ func (r *DatasetReconciler) enqueueReferenceDatasets(ctx context.Context, object requests = append(requests, reconcile.Request{NamespacedName: key}) } + var queue []client.ObjectKey switch changed := object.(type) { case *corev1.Namespace: - // Namespace labels affect only references whose final target is this namespace. + // Namespace labels affect references in this namespace and every + // downstream reference that inherits their grants. for i := range list.Items { ds := &list.Items[i] if ds.Namespace == changed.Name && ds.Spec.Source.Type == datasetv1alpha1.DatasetTypeReference && !kubeutils.IsDeleted(ds) { - appendRequest(client.ObjectKeyFromObject(ds)) + key := client.ObjectKeyFromObject(ds) + appendRequest(key) + queue = append(queue, key) } } - return requests case *datasetv1alpha1.Dataset: - // Walk the reverse reference graph so source changes reach direct and - // transitive dependents without requeueing unrelated references. - reverse := make(map[client.ObjectKey][]client.ObjectKey) - for i := range list.Items { - ds := &list.Items[i] - if ds.Spec.Source.Type != datasetv1alpha1.DatasetTypeReference || kubeutils.IsDeleted(ds) { - continue - } - source, err := mountpolicy.ParseReference(ds.Spec.Source.URI) - if err != nil { - continue - } - reverse[source] = append(reverse[source], client.ObjectKeyFromObject(ds)) + // For() already enqueues the changed Dataset itself; this watch only + // enqueues its dependents. Namespace events must enqueue both. + queue = append(queue, client.ObjectKeyFromObject(changed)) + default: + return nil + } + + if len(queue) == 0 { + return requests + } + + // Walk the reverse reference graph so Dataset and Namespace changes + // reach transitive dependents without requeueing unrelated references. + reverse := make(map[client.ObjectKey][]client.ObjectKey) + for i := range list.Items { + ds := &list.Items[i] + if ds.Spec.Source.Type != datasetv1alpha1.DatasetTypeReference || kubeutils.IsDeleted(ds) { + continue + } + source, err := mountpolicy.ParseReference(ds.Spec.Source.URI) + if err != nil { + continue } + reverse[source] = append(reverse[source], client.ObjectKeyFromObject(ds)) + } - queue := []client.ObjectKey{client.ObjectKeyFromObject(changed)} - visited := make(map[client.ObjectKey]struct{}) - for len(queue) > 0 { - key := queue[0] - queue = queue[1:] - if _, ok := visited[key]; ok { - continue - } - visited[key] = struct{}{} - for _, dependent := range reverse[key] { - appendRequest(dependent) - queue = append(queue, dependent) - } + visited := make(map[client.ObjectKey]struct{}) + for len(queue) > 0 { + key := queue[0] + queue = queue[1:] + if _, ok := visited[key]; ok { + continue + } + visited[key] = struct{}{} + for _, dependent := range reverse[key] { + appendRequest(dependent) + queue = append(queue, dependent) } - return requests - default: - return nil } + return requests } func dependencyDatasetChanged(update event.UpdateEvent) bool { diff --git a/internal/controller/dataset/dataset_controller_test.go b/internal/controller/dataset/dataset_controller_test.go index 0ccad1e..0b2f7e0 100644 --- a/internal/controller/dataset/dataset_controller_test.go +++ b/internal/controller/dataset/dataset_controller_test.go @@ -154,6 +154,47 @@ func TestDatasetReconciler_enqueueReferenceDatasetsScopesDependencies(t *testing require.True(t, dependencyDatasetChanged(event.UpdateEvent{ObjectOld: oldDataset, ObjectNew: newDataset})) } +func TestDatasetReconciler_enqueueNamespaceTransitiveReferences(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, datasetv1alpha1.AddToScheme(scheme)) + require.NoError(t, corev1.AddToScheme(scheme)) + + reference := func(namespace, name, uri string) *datasetv1alpha1.Dataset { + return &datasetv1alpha1.Dataset{ + ObjectMeta: metav1.ObjectMeta{Namespace: namespace, Name: name}, + Spec: datasetv1alpha1.DatasetSpec{Source: datasetv1alpha1.DatasetSource{ + Type: datasetv1alpha1.DatasetTypeReference, URI: uri, + }}, + } + } + // Both intermediate references are in the changed namespace. Their shared + // descendants must be enqueued once, including those multiple hops away. + first := reference("middle", "first", "dataset://origin/source") + second := reference("middle", "second", "dataset://middle/first") + third := reference("target", "third", "dataset://middle/second") + leaf := reference("target", "leaf", "dataset://target/third") + unrelated := reference("other", "unrelated", "dataset://origin/source") + source := &datasetv1alpha1.Dataset{ObjectMeta: metav1.ObjectMeta{Namespace: "origin", Name: "source"}} + reconciler := &DatasetReconciler{Client: fake.NewClientBuilder().WithScheme(scheme). + WithObjects(source, first, second, third, leaf, unrelated).Build(), Scheme: scheme} + + requests := reconciler.enqueueReferenceDatasets(context.Background(), &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: "middle", Labels: map[string]string{"workspace": "changed"}}, + }) + require.ElementsMatch(t, []client.ObjectKey{ + {Namespace: "middle", Name: "first"}, + {Namespace: "middle", Name: "second"}, + {Namespace: "target", Name: "third"}, + {Namespace: "target", Name: "leaf"}, + }, requestKeys(requests)) + + // A source namespace's labels do not affect grants to other namespaces. + requests = reconciler.enqueueReferenceDatasets(context.Background(), &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: "origin"}, + }) + require.Empty(t, requests) +} + func requestKeys(requests []reconcile.Request) []client.ObjectKey { keys := make([]client.ObjectKey, 0, len(requests)) for _, request := range requests { diff --git a/internal/controller/dataset/mount_policy_additional_test.go b/internal/controller/dataset/mount_policy_additional_test.go index 978f4fd..b5701b1 100644 --- a/internal/controller/dataset/mount_policy_additional_test.go +++ b/internal/controller/dataset/mount_policy_additional_test.go @@ -31,7 +31,10 @@ func TestReconcileMountPolicyStoresFullSourceChain(t *testing.T) { c.Spec.Share = false objects := append(referenceTestStorage(a, nil), referenceTestStorage(b, a)...) objects = append(objects, referenceTestStorage(c, b)...) - objects = append(objects, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "target", Labels: map[string]string{"workspace": "one"}}}) + objects = append(objects, + &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "target", Labels: map[string]string{"workspace": "one"}}}, + &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "b", Labels: map[string]string{"workspace": "one"}}}, + ) reconciler := &DatasetReconciler{Client: fake.NewClientBuilder().WithScheme(scheme).WithObjects(objects...).Build(), Scheme: scheme} require.NoError(t, reconciler.reconcileMountPolicy(ctx, c)) @@ -92,3 +95,60 @@ func referenceTestStorage(ds, parent *datasetv1alpha1.Dataset) []client.Object { } return []client.Object{ds, pvc, pv} } + +func TestReconcileMountPolicyInheritsIntermediateGrants(t *testing.T) { + for _, mode := range []mountpolicy.GrantResult{mountpolicy.ReadOnly, mountpolicy.ReadWrite, mountpolicy.Denied} { + t.Run(string(mode), func(t *testing.T) { + ctx := context.Background() + scheme := runtime.NewScheme() + require.NoError(t, datasetv1alpha1.AddToScheme(scheme)) + require.NoError(t, corev1.AddToScheme(scheme)) + + a := referenceTestSource("a", "a", "a-uid", "a-pvc", "a-pvc-uid", "a-pv", "a-pv-uid", nil, nil) + a.Spec.Share = true + // A grants the final namespace RW but can restrict B's namespace. + if mode != mountpolicy.Denied { + access := datasetv1alpha1.AccessModeReadWrite + if mode == mountpolicy.ReadOnly { + access = datasetv1alpha1.AccessModeReadOnly + } + a.Spec.ShareAccess.Rules = append(a.Spec.ShareAccess.Rules, datasetv1alpha1.ShareAccessRule{ + NamespaceSelector: metav1.LabelSelector{MatchLabels: map[string]string{"workspace": "two"}}, + AccessMode: access, + }) + } + b := referenceTestSource("b", "b", "b-uid", "b-pvc", "b-pvc-uid", "b-pv", "b-pv-uid", a, + &datasetv1alpha1.DatasetSource{Type: datasetv1alpha1.DatasetTypeReference, URI: "dataset://a/a"}) + c := referenceTestSource("c", "target", "c-uid", "c-pvc", "c-pvc-uid", "c-pv", "c-pv-uid", b, + &datasetv1alpha1.DatasetSource{Type: datasetv1alpha1.DatasetTypeReference, URI: "dataset://b/b"}) + objects := append(referenceTestStorage(a, nil), referenceTestStorage(b, a)...) + objects = append(objects, referenceTestStorage(c, b)...) + objects = append(objects, + &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "target", Labels: map[string]string{"workspace": "one"}}}, + &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "b", Labels: map[string]string{"workspace": "two"}}}, + ) + reconciler := &DatasetReconciler{Client: fake.NewClientBuilder().WithScheme(scheme).WithObjects(objects...).Build(), Scheme: scheme} + err := reconciler.reconcileMountPolicy(ctx, c) + reconciler.setMountPolicyCondition(c, err) + if mode == mountpolicy.Denied { + require.ErrorContains(t, err, "not shared to namespace b") + require.ErrorContains(t, err, "reference dataset target/c") + require.ErrorContains(t, err, "source dataset a/a") + require.True(t, c.Status.ReadOnly) + _, err = mountpolicy.Verify(ctx, reconciler.Client, c) + require.Error(t, err) + return + } + require.NoError(t, err) + require.Equal(t, mode == mountpolicy.ReadOnly, c.Status.ReadOnly) + verified, err := mountpolicy.Verify(ctx, reconciler.Client, c) + require.NoError(t, err) + require.Equal(t, c.Status.ReadOnly, verified.ReadOnly) + if mode == mountpolicy.ReadOnly { + c.Status.ReadOnly = false // stale status from the v0.1.14 controller + _, err = mountpolicy.Verify(ctx, reconciler.Client, c) + require.ErrorContains(t, err, "mount access mode no longer matches source policy") + } + }) + } +} diff --git a/pkg/mountpolicy/policy.go b/pkg/mountpolicy/policy.go index 9639f40..1ec1b50 100644 --- a/pkg/mountpolicy/policy.go +++ b/pkg/mountpolicy/policy.go @@ -53,36 +53,59 @@ type Resolution struct { // schema validation (for example, a fake client, an old API server, or an // imported object). func Validate(ds *datasetv1alpha1.Dataset) error { + _, err := compilePolicy(ds) + return err +} + +type compiledRule struct { + selector labels.Selector + mode datasetv1alpha1.AccessMode +} + +type compiledPolicy struct { + namespaceSelector labels.Selector + rules []compiledRule +} + +// compilePolicy validates every rule before any grant is evaluated. Resolve +// reuses the parsed selectors for every distinct target of the same source. +func compilePolicy(ds *datasetv1alpha1.Dataset) (*compiledPolicy, error) { if ds == nil { - return fmt.Errorf("dataset is required") + return nil, fmt.Errorf("dataset is required") } if ds.Spec.Source.Type == datasetv1alpha1.DatasetTypeReference && ds.Spec.VolumeClaimRef != nil { - return fmt.Errorf("REFERENCE dataset cannot set volumeClaimRef") + return nil, fmt.Errorf("REFERENCE dataset cannot set volumeClaimRef") } + compiled := &compiledPolicy{namespaceSelector: labels.Everything()} if ds.Spec.ShareToNamespaceSelector != nil && !emptySelector(ds.Spec.ShareToNamespaceSelector) { - if _, err := metav1.LabelSelectorAsSelector(ds.Spec.ShareToNamespaceSelector); err != nil { - return fmt.Errorf("shareToNamespaceSelector is invalid: %w", err) + selector, err := metav1.LabelSelectorAsSelector(ds.Spec.ShareToNamespaceSelector) + if err != nil { + return nil, fmt.Errorf("shareToNamespaceSelector is invalid: %w", err) } + compiled.namespaceSelector = selector } policy := ds.Spec.ShareAccess if policy == nil { - return nil + return compiled, nil } if len(policy.Rules) == 0 { - return fmt.Errorf("shareAccess.rules must contain at least one rule when shareAccess is configured") + return nil, fmt.Errorf("shareAccess.rules must contain at least one rule when shareAccess is configured") } + compiled.rules = make([]compiledRule, 0, len(policy.Rules)) for i, rule := range policy.Rules { if rule.AccessMode != datasetv1alpha1.AccessModeReadOnly && rule.AccessMode != datasetv1alpha1.AccessModeReadWrite { - return fmt.Errorf("shareAccess.rules[%d].accessMode %q is invalid", i, rule.AccessMode) + return nil, fmt.Errorf("shareAccess.rules[%d].accessMode %q is invalid", i, rule.AccessMode) } if emptySelector(&rule.NamespaceSelector) { - return fmt.Errorf("shareAccess.rules[%d].namespaceSelector must not be empty", i) + return nil, fmt.Errorf("shareAccess.rules[%d].namespaceSelector must not be empty", i) } - if _, err := metav1.LabelSelectorAsSelector(&rule.NamespaceSelector); err != nil { - return fmt.Errorf("shareAccess.rules[%d].namespaceSelector is invalid: %w", i, err) + selector, err := metav1.LabelSelectorAsSelector(&rule.NamespaceSelector) + if err != nil { + return nil, fmt.Errorf("shareAccess.rules[%d].namespaceSelector is invalid: %w", i, err) } + compiled.rules = append(compiled.rules, compiledRule{selector: selector, mode: rule.AccessMode}) } - return nil + return compiled, nil } // Grant computes the access that source grants to targetNamespace. Namespace @@ -98,41 +121,38 @@ func Grant(ctx context.Context, reader client.Reader, source *datasetv1alpha1.Da if source.DeletionTimestamp != nil || !source.Spec.Share { return Denied, nil } - if err := Validate(source); err != nil { + policy, err := compilePolicy(source) + if err != nil { return Denied, err } + return policy.grant(ctx, reader, targetNamespace) +} +func (p *compiledPolicy) grant(ctx context.Context, reader client.Reader, targetNamespace string) (GrantResult, error) { + if targetNamespace == "" { + return Denied, fmt.Errorf("target namespace is required") + } ns := &corev1.Namespace{} if err := reader.Get(ctx, client.ObjectKey{Name: targetNamespace}, ns); err != nil { return Denied, fmt.Errorf("get target namespace %q: %w", targetNamespace, err) } - if source.Spec.ShareToNamespaceSelector != nil && !emptySelector(source.Spec.ShareToNamespaceSelector) { - selector, err := metav1.LabelSelectorAsSelector(source.Spec.ShareToNamespaceSelector) - if err != nil { - return Denied, fmt.Errorf("parse shareToNamespaceSelector: %w", err) - } - if !selector.Matches(labels.Set(ns.Labels)) { - return Denied, nil - } + namespaceLabels := labels.Set(ns.Labels) + if !p.namespaceSelector.Matches(namespaceLabels) { + return Denied, nil } - // A nil policy is the legacy sharing contract: shared references are - // readable, never writable. An explicit policy has no fallback rule. - if source.Spec.ShareAccess == nil { + // Nil rules preserve legacy read-only sharing. Explicit empty policies + // have already been rejected by compilePolicy. + if p.rules == nil { return ReadOnly, nil } - grant := Denied - for i, rule := range source.Spec.ShareAccess.Rules { - selector, err := metav1.LabelSelectorAsSelector(&rule.NamespaceSelector) - if err != nil { - return Denied, fmt.Errorf("parse shareAccess.rules[%d].namespaceSelector: %w", i, err) - } - if !selector.Matches(labels.Set(ns.Labels)) { + for _, rule := range p.rules { + if !rule.selector.Matches(namespaceLabels) { continue } // ReadOnly wins over every ReadWrite match. - if rule.AccessMode == datasetv1alpha1.AccessModeReadOnly { + if rule.mode == datasetv1alpha1.AccessModeReadOnly { return ReadOnly, nil } grant = ReadWrite @@ -140,9 +160,34 @@ func Grant(ctx context.Context, reader client.Reader, source *datasetv1alpha1.Da return grant, nil } -// Resolve re-resolves every reference edge against the final Dataset namespace. -// It does not use upstream status.ReadOnly: status is a cached outcome and -// cannot grant access after a source policy changes. +// namespaceCachingReader keeps namespace identities consistent during one +// resolution while checking every ancestor against each intermediate. +// It is local to one sequential Resolve call and is not concurrency-safe. +// All namespace reads in this path use *corev1.Namespace. +type namespaceCachingReader struct { + client.Reader + namespaces map[string]*corev1.Namespace +} + +func (r *namespaceCachingReader) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + target, ok := obj.(*corev1.Namespace) + if !ok { + return r.Reader.Get(ctx, key, obj, opts...) + } + if cached, found := r.namespaces[key.Name]; found { + cached.DeepCopyInto(target) + return nil + } + if err := r.Reader.Get(ctx, key, target, opts...); err != nil { + return err + } + r.namespaces[key.Name] = target.DeepCopy() + return nil +} + +// Resolve checks every ancestor against the final namespace and all intervening +// reference namespaces. Any inherited denial or read-only grant is preserved. +// Cached upstream status.ReadOnly never grants access. func Resolve(ctx context.Context, reader client.Reader, ds *datasetv1alpha1.Dataset) (*Resolution, error) { if ds == nil || ds.Spec.Source.Type != datasetv1alpha1.DatasetTypeReference { return nil, fmt.Errorf("a REFERENCE dataset is required") @@ -151,6 +196,9 @@ func Resolve(ctx context.Context, reader client.Reader, ds *datasetv1alpha1.Data return nil, err } + reader = &namespaceCachingReader{Reader: reader, namespaces: make(map[string]*corev1.Namespace)} + targetNamespaces := []string{ds.Namespace} + namespaceSeen := map[string]struct{}{ds.Namespace: {}} seen := map[string]struct{}{datasetKey(ds.Namespace, ds.Name): {}} current := ds result := &Resolution{} @@ -175,23 +223,34 @@ func Resolve(ctx context.Context, reader client.Reader, ds *datasetv1alpha1.Data if source.DeletionTimestamp != nil { return nil, fmt.Errorf("source dataset %s/%s is deleting", source.Namespace, source.Name) } - grant, err := Grant(ctx, reader, source, ds.Namespace) + if !source.Spec.Share { + return nil, fmt.Errorf("reference dataset %s/%s: source dataset %s/%s is not shared to namespace %s", ds.Namespace, ds.Name, source.Namespace, source.Name, ds.Namespace) + } + policy, err := compilePolicy(source) if err != nil { - return nil, err + return nil, fmt.Errorf("reference dataset %s/%s: source dataset %s/%s: %w", ds.Namespace, ds.Name, source.Namespace, source.Name, err) } - if grant == Denied { - return nil, fmt.Errorf("source dataset %s/%s is not shared to namespace %s", source.Namespace, source.Name, ds.Namespace) + for _, namespace := range targetNamespaces { + grant, err := policy.grant(ctx, reader, namespace) + if err != nil { + return nil, err + } + if grant == Denied { + return nil, fmt.Errorf("reference dataset %s/%s: source dataset %s/%s is not shared to namespace %s in its reference chain", ds.Namespace, ds.Name, source.Namespace, source.Name, namespace) + } + result.ReadOnly = result.ReadOnly || grant == ReadOnly } result.Sources = append(result.Sources, source) - if grant == ReadOnly { - result.ReadOnly = true - } if source.Spec.Source.Type != datasetv1alpha1.DatasetTypeReference { return result, nil } if source.Spec.VolumeClaimRef != nil { return nil, fmt.Errorf("source REFERENCE dataset %s/%s sets volumeClaimRef", source.Namespace, source.Name) } + if _, found := namespaceSeen[source.Namespace]; !found { + namespaceSeen[source.Namespace] = struct{}{} + targetNamespaces = append(targetNamespaces, source.Namespace) + } current = source } } diff --git a/pkg/mountpolicy/policy_resolution_test.go b/pkg/mountpolicy/policy_resolution_test.go new file mode 100644 index 0000000..07ba5f3 --- /dev/null +++ b/pkg/mountpolicy/policy_resolution_test.go @@ -0,0 +1,97 @@ +package mountpolicy + +import ( + "context" + "fmt" + "testing" + + datasetv1alpha1 "github.com/BaizeAI/dataset/api/dataset/v1alpha1" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + 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" +) + +func resolutionChain(depth int, distinctNamespaces bool) (*datasetv1alpha1.Dataset, []client.Object) { + var objects []client.Object + var previous *datasetv1alpha1.Dataset + for i := 0; i <= depth; i++ { + namespace := "shared" + if distinctNamespaces { + namespace = fmt.Sprintf("ns-%d", i) + } + if i == 0 || distinctNamespaces { + objects = append(objects, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{ + Name: namespace, Labels: map[string]string{"workspace": "one"}, + }}) + } + ds := &datasetv1alpha1.Dataset{ + ObjectMeta: metav1.ObjectMeta{Namespace: namespace, Name: fmt.Sprintf("dataset-%d", i)}, + Spec: datasetv1alpha1.DatasetSpec{ + Share: true, ShareAccess: policy(datasetv1alpha1.AccessModeReadWrite, "one"), + Source: datasetv1alpha1.DatasetSource{Type: datasetv1alpha1.DatasetTypeManual}, + }, + } + if previous != nil { + ds.Spec.Source = datasetv1alpha1.DatasetSource{ + Type: datasetv1alpha1.DatasetTypeReference, + URI: fmt.Sprintf("dataset://%s/%s", previous.Namespace, previous.Name), + } + } + objects = append(objects, ds) + previous = ds + } + return previous, objects +} + +func TestResolveMaximumDepthNamespaces(t *testing.T) { + for _, distinct := range []bool{false, true} { + t.Run(fmt.Sprintf("distinct=%t", distinct), func(t *testing.T) { + leaf, objects := resolutionChain(MaxReferenceDepth, distinct) + resolution, err := Resolve(context.Background(), newReader(t, objects...), leaf) + require.NoError(t, err) + require.Len(t, resolution.Sources, MaxReferenceDepth) + require.False(t, resolution.ReadOnly) + }) + } +} + +func TestGrantValidatesAllRulesBeforeMatching(t *testing.T) { + source := &datasetv1alpha1.Dataset{Spec: datasetv1alpha1.DatasetSpec{ + Share: true, ShareAccess: policy(datasetv1alpha1.AccessModeReadOnly, "one"), + }} + source.Spec.ShareAccess.Rules = append(source.Spec.ShareAccess.Rules, datasetv1alpha1.ShareAccessRule{ + AccessMode: datasetv1alpha1.AccessModeReadWrite, + NamespaceSelector: metav1.LabelSelector{MatchExpressions: []metav1.LabelSelectorRequirement{{ + Key: "workspace", Operator: metav1.LabelSelectorOperator("invalid"), + }}}, + }) + reader := newReader(t, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{ + Name: "target", Labels: map[string]string{"workspace": "one"}, + }}) + grant, err := Grant(context.Background(), reader, source, "target") + require.ErrorContains(t, err, "shareAccess.rules[1].namespaceSelector is invalid") + require.Equal(t, Denied, grant) +} + +func BenchmarkResolveNamespaces(b *testing.B) { + for _, distinct := range []bool{false, true} { + b.Run(fmt.Sprintf("distinct=%t", distinct), func(b *testing.B) { + leaf, objects := resolutionChain(MaxReferenceDepth, distinct) + // Use the same fake API reader for both layouts and all iterations. + scheme := runtime.NewScheme() + require.NoError(b, datasetv1alpha1.AddToScheme(scheme)) + require.NoError(b, corev1.AddToScheme(scheme)) + reader := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objects...).Build() + b.ReportAllocs() + b.ResetTimer() + for i := 0; i < b.N; i++ { + if _, err := Resolve(context.Background(), reader, leaf); err != nil { + b.Fatal(err) + } + } + }) + } +} diff --git a/pkg/mountpolicy/policy_test.go b/pkg/mountpolicy/policy_test.go index 8b8bfc7..905ee38 100644 --- a/pkg/mountpolicy/policy_test.go +++ b/pkg/mountpolicy/policy_test.go @@ -102,7 +102,7 @@ func TestResolveRequiresEverySourceAndReadonlyWins(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Name: "leaf", Namespace: "target"}, Spec: datasetv1alpha1.DatasetSpec{Source: datasetv1alpha1.DatasetSource{Type: datasetv1alpha1.DatasetTypeReference, URI: "dataset://middle/middle"}}, } - reader := newReader(t, target, root, middle) + reader := newReader(t, target, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "middle", Labels: map[string]string{"workspace": "one"}}}, root, middle) resolution, err := Resolve(ctx, reader, leaf) require.NoError(t, err) require.True(t, resolution.ReadOnly) @@ -110,7 +110,7 @@ func TestResolveRequiresEverySourceAndReadonlyWins(t *testing.T) { require.Equal(t, []string{"middle", "root"}, []string{resolution.Sources[0].Name, resolution.Sources[1].Name}) root.Spec.ShareAccess = policy(datasetv1alpha1.AccessModeReadWrite, "other") - reader = newReader(t, target, root, middle) + reader = newReader(t, target, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "middle", Labels: map[string]string{"workspace": "one"}}}, root, middle) _, err = Resolve(ctx, reader, leaf) require.ErrorContains(t, err, "not shared") }