From 06d4628339321392830ce181c53c7f15a6f2daa9 Mon Sep 17 00:00:00 2001 From: notsrch <57882515+notsrch@users.noreply.github.com> Date: Fri, 4 Sep 2026 12:25:33 -0500 Subject: [PATCH] Repair a node's export policy rules when it registers With autoExportPolicy enabled, the ONTAP NAS drivers write a node's export policy rules only in that node's own ControllerPublishVolume. Nothing adds or corrects a rule afterwards: ReconcileVolumeNodeAccess is a no-op for ontap-nas-economy and ontap-nas, the periodic node-access loop touches only the backend-wide policy, and Kubernetes does not repeat ControllerPublishVolume while a VolumeAttachment exists. A node whose rule goes missing or stale (IP change, out-of-band edit, or the rule wipe in #1179) fails every mount of the volume until an operator deletes its VolumeAttachments or edits the policy on the array. This change adds the repair path and runs it when a node registers, which is the one event a rebooted or re-addressed node always produces. - Implement ReconcileVolumeNodeAccess for ontap-nas-economy and ontap-nas as an add-only repair of the volume's own per-qtree or per-volume policy for the given nodes. It never removes a rule or changes which policy is assigned, and it skips volumes that are not on a Trident-managed per-volume policy. Economy read-only clones are repaired against their source qtree's policy; ontap-nas clones have no policy of their own and are skipped. - Extract the rule-adding half of the publish path into ensureNodeAccessRulesForPolicy, which takes a node list, so publish and repair share one code path. - In both cores, after AddNode registers the node, a background task collects every volume the node is published to (subordinates resolve to their share source) and repairs each one for every node published to it. Failures are logged and retried at the next registration. Only backends that enforce publications take part. - Remove the classic core's per-volume loop from the synchronous reconcile; it only ever reached no-op driver implementations. The repair is event-driven only. There is no periodic pass, so a rule lost while every node stays up is repaired at the next registration of any node published to that volume. A periodic pass can be layered on the same helpers later. Verified on a live ONTAP cluster: a qtree rule deleted on the array was restored 21 s after the node's Trident pod restarted; the periodic loop alone left it missing. Fixes #1181. Co-Authored-By: Claude Fable 5.1 --- CHANGELOG.md | 1 + core/common.go | 40 +++ core/concurrent_core.go | 138 +++++++++ core/concurrent_core_test.go | 150 ++++++++++ core/orchestrator_core.go | 127 +++++--- core/orchestrator_core_test.go | 272 ++++++++++++++---- storage_drivers/ontap/ontap_common.go | 121 +++----- storage_drivers/ontap/ontap_common_test.go | 229 +++++++++++++++ storage_drivers/ontap/ontap_nas.go | 46 ++- storage_drivers/ontap/ontap_nas_qtree.go | 59 +++- storage_drivers/ontap/ontap_nas_qtree_test.go | 213 ++++++++++++++ storage_drivers/ontap/ontap_nas_test.go | 191 +++++++++++- 12 files changed, 1388 insertions(+), 199 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4913decf3..c98d9f529 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ - **Kubernetes:** Fixed race conditions and state handling for concurrent publish/unpublish, clone, cache, and backend update operations. - **Kubernetes:** Fixed export policy race conditions and concurrent publish/unpublish handling for subordinate volumes and read-only clones in ONTAP-NAS and ONTAP-NAS-Economy drivers. +- **Kubernetes:** Fixed ONTAP-NAS and ONTAP-NAS-Economy drivers with `autoExportPolicy` never restoring a published node's export policy rules after the node changed IP address or the rules were removed; the rules are now repaired for every volume published to a node when that node registers (Issue [#1181](https://github.com/NetApp/trident/issues/1181)). - **Kubernetes:** Fixed ONTAP-NAS-Economy FlexVol discovery after MetroCluster failover when snapshot policies differ (Issue [#1082](https://github.com/NetApp/trident/issues/1082)). - **Kubernetes:** Fixed ONTAP-NAS-Economy volume delete retries that interrupted long-running qtree deletes (Issue [#1121](https://github.com/NetApp/trident/issues/1121)). - **Kubernetes:** Fixed ONTAP-SAN and ONTAP-SAN-Economy import and resize behavior, including `fsType` validation, volume metadata handling, and autogrow mode behavior. diff --git a/core/common.go b/core/common.go index 9905ab4c3..e207cbc25 100644 --- a/core/common.go +++ b/core/common.go @@ -307,6 +307,46 @@ func generateVolumePublication(volName string, publishInfo *models.VolumePublish return vp } +// volumeNodeAccessRepair asks a backend's driver to add any missing export rules on one volume's +// policy for the nodes currently published to it. volConfig is a private copy, so the driver call +// can run without holding any orchestrator lock. +type volumeNodeAccessRepair struct { + backend storage.Backend + volConfig *storage.VolumeConfig + nodes []*models.Node +} + +// applyNodeAccessRepairs runs each repair through the backend's driver, stopping early if ctx is +// cancelled. The driver is called directly rather than through the backend wrapper, whose +// "already up to date" short-circuit can be flipped by a concurrent publish while the repairs are +// in flight. Failures are logged and skipped: the next registration of any node published to the +// volume retries the repair. +func applyNodeAccessRepairs(ctx context.Context, nodeName string, repairs []volumeNodeAccessRepair) { + for _, repair := range repairs { + if ctx.Err() != nil { + return + } + err := repair.backend.Driver().ReconcileVolumeNodeAccess(ctx, repair.volConfig, repair.nodes) + if err != nil { + Logc(ctx).WithError(err).WithFields(LogFields{ + "node": nodeName, + "volume": repair.volConfig.Name, + "backend": repair.backend.Name(), + }).Warn("Could not repair export rules for a volume published to the node.") + } + } +} + +// backendCanRepairNodeAccess reports whether b hosts per-volume export policies worth repairing: +// it enforces publications and is still serving volumes. +func backendCanRepairNodeAccess(b storage.Backend) bool { + if !b.CanEnablePublishEnforcement() { + return false + } + state := b.State() + return state.IsOnline() || state.IsDeleting() +} + // isDockerPluginMode returns true if the ENV variable config.DockerPluginModeEnvVariable is set func isDockerPluginMode() bool { return os.Getenv(config.DockerPluginModeEnvVariable) != "" diff --git a/core/concurrent_core.go b/core/concurrent_core.go index 30f2b6663..1dfebcee9 100644 --- a/core/concurrent_core.go +++ b/core/concurrent_core.go @@ -7381,9 +7381,147 @@ func (o *ConcurrentTridentOrchestrator) AddNode( if invalidateErr := o.invalidateAllBackendNodeAccess(ctx); invalidateErr != nil { Logc(ctx).WithError(invalidateErr).Error("Could not invalidate backend node access.") } + + // A node's per-volume export rules are otherwise written only by its own ControllerPublishVolume, + // which Kubernetes never repeats while a VolumeAttachment exists. Registration is the event a + // rebooted or re-addressed node always produces, so repair its volumes' rules here, off the + // request path and detached from the request's cancellation. + nodeName := node.Name + o.bgTasks.launch(context.WithoutCancel(ctx), func(ctx context.Context) { + o.repairNodeAccessForNode(ctx, nodeName) + }) return } +// repairNodeAccessForNode adds any missing export rules, for every node concerned, on every volume +// that nodeName is published to. It reads what it needs from the cache and calls the drivers with +// no cache lock held. +func (o *ConcurrentTridentOrchestrator) repairNodeAccessForNode(ctx context.Context, nodeName string) { + repairs, err := o.nodeAccessRepairsForNode(ctx, nodeName) + if err != nil { + Logc(ctx).WithError(err).WithField("node", nodeName).Warn( + "Could not collect the volumes published to the node for export rule repair.") + return + } + if len(repairs) == 0 { + return + } + Logc(ctx).WithFields(LogFields{ + "node": nodeName, + "volumes": len(repairs), + }).Debug("Repairing export rules for volumes published to the node.") + applyNodeAccessRepairs(ctx, nodeName, repairs) +} + +// nodeAccessRepairsForNode collects one repair per volume that nodeName is published to. A +// subordinate publication resolves to the share-source volume whose policy grants the access, and +// each repair carries every node published to that volume, so a shared volume is repaired for all +// of its nodes at once. Only backends that enforce publications take part. +func (o *ConcurrentTridentOrchestrator) nodeAccessRepairsForNode( + ctx context.Context, nodeName string, +) ([]volumeNodeAccessRepair, error) { + _, results, unlocker, err := db.Lock(ctx, db.Query( + db.ListVolumePublications(), db.ListNodes(), db.ListSubordinateVolumes())) + if err != nil { + unlocker() + return nil, err + } + publications, allNodes, subordinates := results[0].VolumePublications, results[0].Nodes, + results[0].SubordinateVolumes + unlocker() + + shareSourceByName := make(map[string]string, len(subordinates)) + for _, subordinate := range subordinates { + shareSourceByName[subordinate.Config.Name] = subordinate.Config.ShareSourceVolume + } + nodesByName := make(map[string]*models.Node, len(allNodes)) + for _, n := range allNodes { + nodesByName[n.Name] = n + } + + // Every node published to each hosting volume, and the hosting volumes this node is published to. + nodeNamesByVolume := make(map[string]map[string]struct{}) + targetVolumes := make(map[string]struct{}) + for _, pub := range publications { + hosting := pub.VolumeName + if source, ok := shareSourceByName[hosting]; ok { + hosting = source + } + if nodeNamesByVolume[hosting] == nil { + nodeNamesByVolume[hosting] = make(map[string]struct{}) + } + nodeNamesByVolume[hosting][pub.NodeName] = struct{}{} + if pub.NodeName == nodeName { + targetVolumes[hosting] = struct{}{} + } + } + if len(targetVolumes) == 0 { + return nil, nil + } + + volumeQueries := make([][]db.Subquery, 0, len(targetVolumes)) + for name := range targetVolumes { + volumeQueries = append(volumeQueries, db.Query(db.ReadVolume(name))) + } + _, results, unlocker, err = db.Lock(ctx, volumeQueries...) + if err != nil { + unlocker() + return nil, err + } + volumes := make([]*storage.Volume, 0, len(results)) + backendUUIDs := make(map[string]struct{}) + for _, result := range results { + if volume := result.Volume.Read; volume != nil { + volumes = append(volumes, volume) + backendUUIDs[volume.BackendUUID] = struct{}{} + } + } + unlocker() + if len(volumes) == 0 { + return nil, nil + } + + backendQueries := make([][]db.Subquery, 0, len(backendUUIDs)) + for backendUUID := range backendUUIDs { + backendQueries = append(backendQueries, db.Query(db.ReadBackend(backendUUID))) + } + _, results, unlocker, err = db.Lock(ctx, backendQueries...) + if err != nil { + unlocker() + return nil, err + } + backendsByUUID := make(map[string]storage.Backend, len(results)) + for _, result := range results { + if backend := result.Backend.Read; backend != nil { + backendsByUUID[backend.BackendUUID()] = backend + } + } + unlocker() + + sort.Slice(volumes, func(i, j int) bool { return volumes[i].Config.Name < volumes[j].Config.Name }) + repairs := make([]volumeNodeAccessRepair, 0, len(volumes)) + for _, volume := range volumes { + backend, ok := backendsByUUID[volume.BackendUUID] + if !ok || !backendCanRepairNodeAccess(backend) { + continue + } + nodeNames := make([]string, 0, len(nodeNamesByVolume[volume.Config.Name])) + for name := range nodeNamesByVolume[volume.Config.Name] { + nodeNames = append(nodeNames, name) + } + sort.Strings(nodeNames) + nodes := make([]*models.Node, 0, len(nodeNames)) + for _, name := range nodeNames { + if node := nodesByName[name]; node != nil { + nodes = append(nodes, node) + } + } + // ReadVolume hands back a deep copy, so the config is already private to this repair. + repairs = append(repairs, volumeNodeAccessRepair{backend: backend, volConfig: volume.Config, nodes: nodes}) + } + return repairs, nil +} + // UpdateNode updates the publication state of a node. It does not create a new node if it does not exist. func (o *ConcurrentTridentOrchestrator) UpdateNode( ctx context.Context, nodeName string, flags *models.NodePublicationStateFlags, diff --git a/core/concurrent_core_test.go b/core/concurrent_core_test.go index dd950af69..c7ae7d1ff 100644 --- a/core/concurrent_core_test.go +++ b/core/concurrent_core_test.go @@ -13687,6 +13687,9 @@ func TestAddNodeConcurrentCore(t *testing.T) { o := getConcurrentOrchestrator() o.storeClient = mockStoreClient o.lastNodeRegistrationTime = time.Time{} + // AddNode launches a background repair that reads the cache; stop it before the next + // case reinitializes the cache underneath it. + defer o.bgTasks.stop() err := o.AddNode(testCtx, tt.newNode, func(_, _, _ string) {}) @@ -13704,6 +13707,153 @@ func TestAddNodeConcurrentCore(t *testing.T) { } } +func TestNodeAccessRepairsForNodeConcurrentCore(t *testing.T) { + const enforcingUUID, plainUUID = "uuid-enforcing", "uuid-plain" + + tests := []struct { + name string + node string + publications []*models.VolumePublication + expectVolumes []string + expectNodes []string + }{ + { + name: "RepairCarriesEveryNodePublishedToTheVolume", + node: "nodeA", + publications: []*models.VolumePublication{ + getFakeVolumePublication("vol1", "nodeA"), + getFakeVolumePublication("vol1", "nodeB"), + }, + expectVolumes: []string{"vol1"}, + expectNodes: []string{"nodeA", "nodeB"}, + }, + { + name: "SubordinatePublicationResolvesToTheSourceVolume", + node: "nodeA", + publications: []*models.VolumePublication{ + getFakeVolumePublication("sub1", "nodeA"), + getFakeVolumePublication("vol1", "nodeB"), + }, + expectVolumes: []string{"vol1"}, + expectNodes: []string{"nodeA", "nodeB"}, + }, + { + name: "SourceAndSubordinateProduceOneRepair", + node: "nodeA", + publications: []*models.VolumePublication{ + getFakeVolumePublication("vol1", "nodeA"), + getFakeVolumePublication("sub1", "nodeA"), + }, + expectVolumes: []string{"vol1"}, + expectNodes: []string{"nodeA"}, + }, + { + name: "UnregisteredNodeIsLeftOut", + node: "nodeA", + publications: []*models.VolumePublication{ + getFakeVolumePublication("vol1", "nodeA"), + getFakeVolumePublication("vol1", "ghost"), + }, + expectVolumes: []string{"vol1"}, + expectNodes: []string{"nodeA"}, + }, + { + name: "BackendWithoutPublishEnforcementIsSkipped", + node: "nodeA", + publications: []*models.VolumePublication{getFakeVolumePublication("vol2", "nodeA")}, + expectVolumes: []string{}, + }, + { + name: "OtherNodesPublicationsAreNotRepaired", + node: "nodeA", + publications: []*models.VolumePublication{getFakeVolumePublication("vol1", "nodeB")}, + expectVolumes: []string{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + db.Initialize() + mockCtrl := gomock.NewController(t) + enforcing := getMockBackend(mockCtrl, "enforcing", enforcingUUID) + enforcing.EXPECT().CanEnablePublishEnforcement().Return(true).AnyTimes() + plain := getMockBackend(mockCtrl, "plain", plainUUID) + plain.EXPECT().CanEnablePublishEnforcement().Return(false).AnyTimes() + addBackendsToCache(t, enforcing, plain) + + source := getFakeVolume("vol1", enforcingUUID) + source.Config.ExportPolicy = "vol1" + addVolumesToCache(t, source, getFakeVolume("vol2", plainUUID)) + addSubordinateVolumesToCache(t, &storage.Volume{ + Config: &storage.VolumeConfig{Name: "sub1", ShareSourceVolume: "vol1"}, + State: storage.VolumeStateSubordinate, + }) + addNodesToCache(t, getFakeNode("nodeA"), getFakeNode("nodeB")) + addVolumePublicationsToCache(t, tt.publications...) + + repairs, err := getConcurrentOrchestrator().nodeAccessRepairsForNode(testCtx, tt.node) + require.NoError(t, err) + + volumes := make([]string, 0, len(repairs)) + for _, r := range repairs { + volumes = append(volumes, r.volConfig.Name) + } + assert.ElementsMatch(t, tt.expectVolumes, volumes) + if len(tt.expectVolumes) == 1 { + assert.Same(t, enforcing, repairs[0].backend) + assert.ElementsMatch(t, tt.expectNodes, nodeNamesOf(repairs[0].nodes)) + assert.Equal(t, source.Config, repairs[0].volConfig) + assert.NotSame(t, source.Config, repairs[0].volConfig, "the driver must get a private copy") + } + }) + } +} + +// Registering a node must repair the export rules of the volumes it is published to, for every +// node published to them, without holding any cache lock while the driver talks to ONTAP. +func TestAddNodeConcurrentCore_RepairsExportRulesOfPublishedVolumes(t *testing.T) { + const backendUUID = "uuid1" + db.Initialize() + mockCtrl := gomock.NewController(t) + + mockStoreClient := mockpersistentstore.NewMockStoreClient(mockCtrl) + mockStoreClient.EXPECT().AddOrUpdateNode(gomock.Any(), gomock.Any()).Return(nil) + + driver := mockstorage.NewMockDriver(mockCtrl) + backend := getMockBackend(mockCtrl, "backend1", backendUUID) + backend.EXPECT().CanEnablePublishEnforcement().Return(true).AnyTimes() + backend.EXPECT().Driver().Return(driver).AnyTimes() + backend.EXPECT().InvalidateNodeAccess().AnyTimes() + addBackendsToCache(t, backend) + + volume := getFakeVolume("vol1", backendUUID) + volume.Config.ExportPolicy = "vol1" + addVolumesToCache(t, volume) + addNodesToCache(t, getFakeNode("nodeB")) + addVolumePublicationsToCache(t, + getFakeVolumePublication("vol1", "nodeA"), getFakeVolumePublication("vol1", "nodeB")) + + repaired := make(chan []*models.Node, 1) + driver.EXPECT().ReconcileVolumeNodeAccess(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn( + func(_ context.Context, volConfig *storage.VolumeConfig, nodes []*models.Node) error { + assert.Equal(t, "vol1", volConfig.Name) + repaired <- nodes + return nil + }) + + o := getConcurrentOrchestrator() + o.storeClient = mockStoreClient + require.NoError(t, o.AddNode(testCtx, &models.Node{Name: "nodeA", IPs: []string{"10.0.0.5"}}, func(_, _, _ string) {})) + defer o.bgTasks.stop() + + select { + case nodes := <-repaired: + assert.ElementsMatch(t, []string{"nodeA", "nodeB"}, nodeNamesOf(nodes)) + case <-time.After(5 * time.Second): + t.Fatal("registering the node did not repair its published volume") + } +} + func TestUpdateNodeConcurrentCore(t *testing.T) { tests := []struct { name string diff --git a/core/orchestrator_core.go b/core/orchestrator_core.go index 91933aa6a..6d91dfea7 100644 --- a/core/orchestrator_core.go +++ b/core/orchestrator_core.go @@ -6609,38 +6609,7 @@ func (o *TridentOrchestrator) reconcileNodeAccessOnBackend(ctx context.Context, var nodes []*models.Node if b.CanEnablePublishEnforcement() { - nodes = o.publishedNodesForBackend(b) - volToNodePublications := o.volumePublicationsForBackend(b) - - nodeMap := make(map[string]*models.Node) - for _, n := range nodes { - nodeMap[n.Name] = n - } - - // reconcile every volume if publish enforcement is enabled for the backend - for volName, volPublications := range volToNodePublications { - volume, ok := o.volumes[volName] - if !ok { - continue - } - - volNodes := make([]*models.Node, 0) - for _, pub := range volPublications { - if node, found := nodeMap[pub.NodeName]; found { - volNodes = append(volNodes, node) - } - } - - if err := b.ReconcileVolumeNodeAccess(ctx, volume.Config, volNodes); err != nil { - Logc(ctx).WithError(err).WithFields(LogFields{ - "volume": volume.Config.Name, - "nodes": volNodes, - }).Error("Unable to reconcile node access for volume.") - return err - } - } - } else { nodes = o.nodes.List() } @@ -6675,18 +6644,6 @@ func (o *TridentOrchestrator) publishedNodesForBackend(b storage.Backend) []*mod return nodes } -func (o *TridentOrchestrator) volumePublicationsForBackend(b storage.Backend) map[string][]*models.VolumePublication { - volumes := b.Volumes() - volumeToNodePublications := make(map[string][]*models.VolumePublication) - - volumes.Range(func(k, _ interface{}) bool { - volName := k.(string) - volumeToNodePublications[volName] = o.volumePublications.ListPublicationsForVolume(volName) - return true - }) - return volumeToNodePublications -} - func (o *TridentOrchestrator) reconcileBackendState(ctx context.Context, b storage.Backend) error { Logc(ctx).WithField("backend", b.Name()).Debug(">>>>>> reconcileBackendState") defer Logc(ctx).WithField("backend", b.Name()).Debug("<<<<<<< reconcileBackendState") @@ -6900,6 +6857,90 @@ func (o *TridentOrchestrator) AddNode( o.lastNodeRegistration = time.Now() o.invalidateAllBackendNodeAccess() + + // A node's per-volume export rules are otherwise written only by its own ControllerPublishVolume, + // which Kubernetes never repeats while a VolumeAttachment exists. Registration is the event a + // rebooted or re-addressed node always produces, so repair its volumes' rules here, off the + // request path and detached from the request's cancellation. + nodeName := node.Name + o.bgTasks.launch(context.WithoutCancel(ctx), func(ctx context.Context) { + o.repairNodeAccessForNode(ctx, nodeName) + }) + return nil +} + +// repairNodeAccessForNode adds any missing export rules, for every node concerned, on every volume +// that nodeName is published to. It snapshots what it needs under the orchestrator lock and calls +// the drivers with the lock released. +func (o *TridentOrchestrator) repairNodeAccessForNode(ctx context.Context, nodeName string) { + repairs := o.nodeAccessRepairsForNode(ctx, nodeName) + if len(repairs) == 0 { + return + } + Logc(ctx).WithFields(LogFields{ + "node": nodeName, + "volumes": len(repairs), + }).Debug("Repairing export rules for volumes published to the node.") + applyNodeAccessRepairs(ctx, nodeName, repairs) +} + +// nodeAccessRepairsForNode collects one repair per volume that nodeName is published to. A +// subordinate publication resolves to the share-source volume whose policy grants the access, and +// each repair carries every node published to that volume, so a shared volume is repaired for all +// of its nodes at once. Only backends that enforce publications take part. +func (o *TridentOrchestrator) nodeAccessRepairsForNode(ctx context.Context, nodeName string) []volumeNodeAccessRepair { + o.mutex.Lock() + defer o.mutex.Unlock() + + repairs := make([]volumeNodeAccessRepair, 0) + seenVolumes := make(map[string]struct{}) + for _, pub := range o.volumePublications.ListPublicationsForNode(nodeName) { + volume := o.hostingVolume(pub.VolumeName) + if volume == nil { + continue + } + if _, done := seenVolumes[volume.Config.Name]; done { + continue + } + seenVolumes[volume.Config.Name] = struct{}{} + + backend, ok := o.backends[volume.BackendUUID] + if !ok || !backendCanRepairNodeAccess(backend) { + continue + } + + nodes := make([]*models.Node, 0) + seenNodes := make(map[string]struct{}) + for _, volPub := range o.listVolumePublicationsForVolumeAndSubordinates(ctx, volume.Config.Name) { + if _, done := seenNodes[volPub.NodeName]; done { + continue + } + seenNodes[volPub.NodeName] = struct{}{} + if node := o.nodes.Get(volPub.NodeName); node != nil { + nodes = append(nodes, node) + } + } + + repairs = append(repairs, volumeNodeAccessRepair{ + backend: backend, + volConfig: volume.Config.ConstructClone(), + nodes: nodes, + }) + } + return repairs +} + +// hostingVolume returns the volume whose storage backs volumeName: the volume itself, or the share +// source of a subordinate volume. Nil when neither is known. +func (o *TridentOrchestrator) hostingVolume(volumeName string) *storage.Volume { + if volume, ok := o.volumes[volumeName]; ok { + return volume + } + if subordinate, ok := o.subordinateVolumes[volumeName]; ok { + if source, ok := o.volumes[subordinate.Config.ShareSourceVolume]; ok { + return source + } + } return nil } diff --git a/core/orchestrator_core_test.go b/core/orchestrator_core_test.go index a5510c303..85f1df308 100644 --- a/core/orchestrator_core_test.go +++ b/core/orchestrator_core_test.go @@ -4667,6 +4667,210 @@ func TestAddNode(t *testing.T) { } } +func nodeNamesOf(nodes []*models.Node) []string { + names := make([]string, 0, len(nodes)) + for _, n := range nodes { + names = append(names, n.Name) + } + return names +} + +func TestNodeAccessRepairsForNode(t *testing.T) { + const enforcingUUID, plainUUID = "uuid-enforcing", "uuid-plain" + newVolumes := func() (source, plain, subordinate *storage.Volume) { + source = &storage.Volume{ + Config: &storage.VolumeConfig{Name: "vol1", InternalName: "vol1", ExportPolicy: "vol1"}, + BackendUUID: enforcingUUID, + } + source.Config.SubordinateVolumes = map[string]interface{}{"sub1": nil} + plain = &storage.Volume{Config: &storage.VolumeConfig{Name: "vol2"}, BackendUUID: plainUUID} + subordinate = &storage.Volume{ + Config: &storage.VolumeConfig{Name: "sub1", ShareSourceVolume: "vol1"}, + State: storage.VolumeStateSubordinate, + } + return + } + + tests := []struct { + name string + node string + publications []*models.VolumePublication + expectVolumes []string + expectNodes []string + }{ + { + name: "RepairCarriesEveryNodePublishedToTheVolume", + node: "nodeA", + publications: []*models.VolumePublication{ + {VolumeName: "vol1", NodeName: "nodeA"}, + {VolumeName: "vol1", NodeName: "nodeB"}, + }, + expectVolumes: []string{"vol1"}, + expectNodes: []string{"nodeA", "nodeB"}, + }, + { + name: "SubordinatePublicationResolvesToTheSourceVolume", + node: "nodeA", + publications: []*models.VolumePublication{ + {VolumeName: "sub1", NodeName: "nodeA"}, + {VolumeName: "vol1", NodeName: "nodeB"}, + }, + expectVolumes: []string{"vol1"}, + expectNodes: []string{"nodeA", "nodeB"}, + }, + { + name: "SourceAndSubordinateProduceOneRepair", + node: "nodeA", + publications: []*models.VolumePublication{ + {VolumeName: "vol1", NodeName: "nodeA"}, + {VolumeName: "sub1", NodeName: "nodeA"}, + }, + expectVolumes: []string{"vol1"}, + expectNodes: []string{"nodeA"}, + }, + { + name: "UnregisteredNodeIsLeftOut", + node: "nodeA", + publications: []*models.VolumePublication{ + {VolumeName: "vol1", NodeName: "nodeA"}, + {VolumeName: "vol1", NodeName: "ghost"}, + }, + expectVolumes: []string{"vol1"}, + expectNodes: []string{"nodeA"}, + }, + { + name: "BackendWithoutPublishEnforcementIsSkipped", + node: "nodeA", + publications: []*models.VolumePublication{{VolumeName: "vol2", NodeName: "nodeA"}}, + expectVolumes: []string{}, + }, + { + name: "UnknownVolumeIsSkipped", + node: "nodeA", + publications: []*models.VolumePublication{{VolumeName: "vol9", NodeName: "nodeA"}}, + expectVolumes: []string{}, + }, + { + name: "OtherNodesPublicationsAreNotRepaired", + node: "nodeA", + publications: []*models.VolumePublication{{VolumeName: "vol1", NodeName: "nodeB"}}, + expectVolumes: []string{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mockCtrl := gomock.NewController(t) + enforcing := mockstorage.NewMockBackend(mockCtrl) + enforcing.EXPECT().CanEnablePublishEnforcement().Return(true).AnyTimes() + enforcing.EXPECT().State().Return(storage.Online).AnyTimes() + plainBackend := mockstorage.NewMockBackend(mockCtrl) + plainBackend.EXPECT().CanEnablePublishEnforcement().Return(false).AnyTimes() + + source, plain, subordinate := newVolumes() + o := getOrchestrator(t, false) + o.backends[enforcingUUID] = enforcing + o.backends[plainUUID] = plainBackend + o.volumes = map[string]*storage.Volume{"vol1": source, "vol2": plain} + o.subordinateVolumes = map[string]*storage.Volume{"sub1": subordinate} + o.nodes.Set("nodeA", &models.Node{Name: "nodeA"}) + o.nodes.Set("nodeB", &models.Node{Name: "nodeB"}) + for _, pub := range tt.publications { + o.volumePublications.Set(pub.VolumeName, pub.NodeName, pub) + } + + repairs := o.nodeAccessRepairsForNode(coreCtx, tt.node) + + volumes := make([]string, 0, len(repairs)) + for _, r := range repairs { + volumes = append(volumes, r.volConfig.Name) + } + assert.ElementsMatch(t, tt.expectVolumes, volumes) + if len(tt.expectVolumes) == 1 { + assert.Same(t, enforcing, repairs[0].backend) + assert.ElementsMatch(t, tt.expectNodes, nodeNamesOf(repairs[0].nodes)) + assert.Equal(t, source.Config, repairs[0].volConfig) + assert.NotSame(t, source.Config, repairs[0].volConfig, "the driver must get a private copy") + } + }) + } +} + +func TestApplyNodeAccessRepairs(t *testing.T) { + mockCtrl := gomock.NewController(t) + driver := mockstorage.NewMockDriver(mockCtrl) + backend := mockstorage.NewMockBackend(mockCtrl) + backend.EXPECT().Driver().Return(driver).AnyTimes() + backend.EXPECT().Name().Return("backend1").AnyTimes() + + nodes := []*models.Node{{Name: "nodeA"}} + first := &storage.VolumeConfig{Name: "vol1"} + second := &storage.VolumeConfig{Name: "vol2"} + repairs := []volumeNodeAccessRepair{ + {backend: backend, volConfig: first, nodes: nodes}, + {backend: backend, volConfig: second, nodes: nodes}, + } + + t.Run("EveryRepairReachesTheDriverEvenAfterAFailure", func(t *testing.T) { + driver.EXPECT().ReconcileVolumeNodeAccess(gomock.Any(), first, nodes).Return(errors.New("ontap down")) + driver.EXPECT().ReconcileVolumeNodeAccess(gomock.Any(), second, nodes).Return(nil) + + applyNodeAccessRepairs(coreCtx, "nodeA", repairs) + }) + + t.Run("CancelledContextStopsBeforeCallingTheDriver", func(t *testing.T) { + cancelled, cancel := context.WithCancel(coreCtx) + cancel() + + applyNodeAccessRepairs(cancelled, "nodeA", repairs) + }) +} + +// Registering a node must repair the export rules of the volumes it is published to, for every +// node published to them, without holding the orchestrator lock while the driver talks to ONTAP. +func TestAddNode_RepairsExportRulesOfPublishedVolumes(t *testing.T) { + const backendUUID = "uuid1" + mockCtrl := gomock.NewController(t) + driver := mockstorage.NewMockDriver(mockCtrl) + backend := mockstorage.NewMockBackend(mockCtrl) + backend.EXPECT().Driver().Return(driver).AnyTimes() + backend.EXPECT().Name().Return("backend1").AnyTimes() + backend.EXPECT().BackendUUID().Return(backendUUID).AnyTimes() + backend.EXPECT().GetDriverName().Return("ontap-nas-economy").AnyTimes() + backend.EXPECT().State().Return(storage.Online).AnyTimes() + backend.EXPECT().CanEnablePublishEnforcement().Return(true).AnyTimes() + backend.EXPECT().InvalidateNodeAccess().AnyTimes() + + volume := &storage.Volume{ + Config: &storage.VolumeConfig{Name: "vol1", InternalName: "vol1", ExportPolicy: "vol1"}, + BackendUUID: backendUUID, + } + o := getOrchestrator(t, false) + o.backends[backendUUID] = backend + o.volumes["vol1"] = volume + o.nodes.Set("nodeB", &models.Node{Name: "nodeB"}) + o.volumePublications.Set("vol1", "nodeA", &models.VolumePublication{VolumeName: "vol1", NodeName: "nodeA"}) + o.volumePublications.Set("vol1", "nodeB", &models.VolumePublication{VolumeName: "vol1", NodeName: "nodeB"}) + + repaired := make(chan []*models.Node, 1) + driver.EXPECT().ReconcileVolumeNodeAccess(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn( + func(_ context.Context, volConfig *storage.VolumeConfig, nodes []*models.Node) error { + assert.Equal(t, "vol1", volConfig.Name) + repaired <- nodes + return nil + }) + + require.NoError(t, o.AddNode(ctx(), &models.Node{Name: "nodeA", IPs: []string{"10.0.0.5"}}, nil)) + defer o.bgTasks.stop() + + select { + case nodes := <-repaired: + assert.ElementsMatch(t, []string{"nodeA", "nodeB"}, nodeNamesOf(nodes)) + case <-time.After(5 * time.Second): + t.Fatal("registering the node did not repair its published volume") + } +} + func TestUpdateNode(t *testing.T) { nodeName := "FakeNode" @@ -6010,10 +6214,9 @@ func TestPublishVolume(t *testing.T) { return nil }) mockBackend.EXPECT().ReconcileNodeAccess(coreCtx, gomock.Any(), gomock.Any()).Return(nil) - mockBackend.EXPECT().ReconcileVolumeNodeAccess(coreCtx, gomock.Any(), gomock.Any()).Return(nil) mockBackend.EXPECT().SetNodeAccessUpToDate() mockBackend.EXPECT().Volumes().Return(makeSyncMap(map[string]*storage.Volume{volumeName: volume})). - Times(2) + Times(1) mockBackend.EXPECT().PublishVolume(coreCtx, gomock.Any(), gomock.Any()).Return(nil) mockBackend.EXPECT().CanEnablePublishEnforcement().Return(true).Times(2) mockStoreClient.EXPECT().UpdateVolume(coreCtx, volume).Return(nil) @@ -6036,10 +6239,9 @@ func TestPublishVolume(t *testing.T) { ) { mockStoreClient.EXPECT().AddVolumePublication(coreCtx, gomock.Any()).Return(nil) mockBackend.EXPECT().ReconcileNodeAccess(coreCtx, gomock.Any(), gomock.Any()).Return(nil) - mockBackend.EXPECT().ReconcileVolumeNodeAccess(coreCtx, gomock.Any(), gomock.Any()).Return(nil) mockBackend.EXPECT().SetNodeAccessUpToDate() mockBackend.EXPECT().Volumes().Return(makeSyncMap(map[string]*storage.Volume{volumeName: volume})). - Times(2) + Times(1) mockBackend.EXPECT().PublishVolume(coreCtx, gomock.Any(), gomock.Any()).Return(nil) mockBackend.EXPECT().CanEnablePublishEnforcement().Return(true).Times(2) mockStoreClient.EXPECT().UpdateVolume(coreCtx, volume).Return(nil) @@ -6062,10 +6264,9 @@ func TestPublishVolume(t *testing.T) { ) { mockStoreClient.EXPECT().AddVolumePublication(coreCtx, gomock.Any()).Return(nil) mockBackend.EXPECT().ReconcileNodeAccess(coreCtx, gomock.Any(), gomock.Any()).Return(nil) - mockBackend.EXPECT().ReconcileVolumeNodeAccess(coreCtx, gomock.Any(), gomock.Any()).Return(nil) mockBackend.EXPECT().SetNodeAccessUpToDate() mockBackend.EXPECT().Volumes().Return(makeSyncMap(map[string]*storage.Volume{volumeName: volume})). - Times(2) + Times(1) mockBackend.EXPECT().PublishVolume(coreCtx, gomock.Any(), gomock.Any()).Return(nil) mockBackend.EXPECT().CanEnablePublishEnforcement().Return(true).Times(2) mockStoreClient.EXPECT().UpdateVolume(coreCtx, volume).Return(nil) @@ -6088,10 +6289,9 @@ func TestPublishVolume(t *testing.T) { ) { mockStoreClient.EXPECT().AddVolumePublication(coreCtx, gomock.Any()).Return(nil) mockBackend.EXPECT().ReconcileNodeAccess(coreCtx, gomock.Any(), gomock.Any()).Return(nil) - mockBackend.EXPECT().ReconcileVolumeNodeAccess(coreCtx, gomock.Any(), gomock.Any()).Return(nil) mockBackend.EXPECT().SetNodeAccessUpToDate() mockBackend.EXPECT().Volumes().Return(makeSyncMap(map[string]*storage.Volume{volumeName: volume})). - Times(2) + Times(1) mockBackend.EXPECT().PublishVolume(coreCtx, gomock.Any(), gomock.Any()).Return(nil) mockBackend.EXPECT().CanEnablePublishEnforcement().Return(true).Times(2) mockStoreClient.EXPECT().UpdateVolume(coreCtx, volume).Return(nil) @@ -6170,10 +6370,9 @@ func TestPublishVolume(t *testing.T) { mockBackend.EXPECT().EnablePublishEnforcement(coreCtx, gomock.Any()).Return( errors.UnsupportedError("unsupported error")) mockBackend.EXPECT().ReconcileNodeAccess(coreCtx, gomock.Any(), gomock.Any()).Return(nil) - mockBackend.EXPECT().ReconcileVolumeNodeAccess(coreCtx, gomock.Any(), gomock.Any()).Return(nil) mockBackend.EXPECT().SetNodeAccessUpToDate() mockBackend.EXPECT().Volumes().Return(makeSyncMap(map[string]*storage.Volume{volumeName: volume})). - Times(2) + Times(1) mockBackend.EXPECT().PublishVolume(coreCtx, gomock.Any(), gomock.Any()).Return(nil) mockStoreClient.EXPECT().UpdateVolume(coreCtx, volume).Return(nil) }, @@ -6222,9 +6421,8 @@ func TestPublishVolume(t *testing.T) { mockBackend.EXPECT().CanEnablePublishEnforcement().Return(true).Times(2) mockBackend.EXPECT().ReconcileNodeAccess(coreCtx, gomock.Any(), gomock.Any()). Return(fmt.Errorf("some error")) - mockBackend.EXPECT().ReconcileVolumeNodeAccess(coreCtx, gomock.Any(), gomock.Any()).Return(nil) mockBackend.EXPECT().Volumes().Return(makeSyncMap(map[string]*storage.Volume{volumeName: volume})). - Times(2) + Times(1) mockBackend.EXPECT().Name().Return("").AnyTimes() }, wantErr: assert.Error, @@ -6250,10 +6448,9 @@ func TestPublishVolume(t *testing.T) { return nil }) mockBackend.EXPECT().ReconcileNodeAccess(coreCtx, gomock.Any(), gomock.Any()).Return(nil) - mockBackend.EXPECT().ReconcileVolumeNodeAccess(coreCtx, gomock.Any(), gomock.Any()).Return(nil) mockBackend.EXPECT().SetNodeAccessUpToDate() mockBackend.EXPECT().Volumes().Return(makeSyncMap(map[string]*storage.Volume{volumeName: volume})). - Times(2) + Times(1) mockBackend.EXPECT().PublishVolume(coreCtx, gomock.Any(), gomock.Any()).Return(fmt.Errorf("some error")) mockBackend.EXPECT().CanEnablePublishEnforcement().Return(true).Times(2) }, @@ -6280,10 +6477,9 @@ func TestPublishVolume(t *testing.T) { return nil }) mockBackend.EXPECT().ReconcileNodeAccess(coreCtx, gomock.Any(), gomock.Any()).Return(nil) - mockBackend.EXPECT().ReconcileVolumeNodeAccess(coreCtx, gomock.Any(), gomock.Any()).Return(nil) mockBackend.EXPECT().SetNodeAccessUpToDate() mockBackend.EXPECT().Volumes().Return(makeSyncMap(map[string]*storage.Volume{volumeName: volume})). - Times(2) + Times(1) mockBackend.EXPECT().PublishVolume(coreCtx, gomock.Any(), gomock.Any()).Return(nil) mockBackend.EXPECT().CanEnablePublishEnforcement().Return(true).Times(2) mockStoreClient.EXPECT().UpdateVolume(coreCtx, volume).Return(fmt.Errorf("some error")) @@ -6309,10 +6505,9 @@ func TestPublishVolume(t *testing.T) { ) { mockStoreClient.EXPECT().AddVolumePublication(coreCtx, gomock.Any()).Return(nil) mockBackend.EXPECT().ReconcileNodeAccess(coreCtx, gomock.Any(), gomock.Any()).Return(nil) - mockBackend.EXPECT().ReconcileVolumeNodeAccess(coreCtx, gomock.Any(), gomock.Any()).Return(nil) mockBackend.EXPECT().SetNodeAccessUpToDate() mockBackend.EXPECT().Volumes().Return(makeSyncMap(map[string]*storage.Volume{volumeName: volume})). - Times(2) + Times(1) mockBackend.EXPECT().PublishVolume(coreCtx, gomock.Any(), gomock.Any()).Return(nil) mockBackend.EXPECT().CanEnablePublishEnforcement().Return(true).Times(2) mockStoreClient.EXPECT().UpdateVolume(coreCtx, volume).Return(nil) @@ -6475,9 +6670,8 @@ func TestPublishVolume_UpdateExistingVP(t *testing.T) { return nil }) mockBackend.EXPECT().ReconcileNodeAccess(coreCtx, gomock.Any(), gomock.Any()).Return(nil) - mockBackend.EXPECT().ReconcileVolumeNodeAccess(coreCtx, gomock.Any(), gomock.Any()).Return(nil) mockBackend.EXPECT().SetNodeAccessUpToDate() - mockBackend.EXPECT().Volumes().Return(makeSyncMap(map[string]*storage.Volume{volumeName: volume})).Times(2) + mockBackend.EXPECT().Volumes().Return(makeSyncMap(map[string]*storage.Volume{volumeName: volume})).Times(1) mockBackend.EXPECT().PublishVolume(coreCtx, gomock.Any(), gomock.Any()).Return(nil) mockBackend.EXPECT().CanEnablePublishEnforcement().Return(true).Times(2) mockStoreClient.EXPECT().UpdateVolume(coreCtx, volume).Return(nil) @@ -6498,9 +6692,8 @@ func TestPublishVolume_UpdateExistingVP(t *testing.T) { // Second publish - should sync fields from volume to VP mockStoreClient.EXPECT().UpdateVolumePublication(coreCtx, gomock.Any()).Return(nil) // This is the key call we're testing mockBackend.EXPECT().ReconcileNodeAccess(coreCtx, gomock.Any(), gomock.Any()).Return(nil) - mockBackend.EXPECT().ReconcileVolumeNodeAccess(coreCtx, gomock.Any(), gomock.Any()).Return(nil) mockBackend.EXPECT().SetNodeAccessUpToDate() - mockBackend.EXPECT().Volumes().Return(makeSyncMap(map[string]*storage.Volume{volumeName: volume})).Times(2) + mockBackend.EXPECT().Volumes().Return(makeSyncMap(map[string]*storage.Volume{volumeName: volume})).Times(1) mockBackend.EXPECT().PublishVolume(coreCtx, gomock.Any(), gomock.Any()).Return(nil) mockBackend.EXPECT().CanEnablePublishEnforcement().Return(true).Times(2) mockStoreClient.EXPECT().UpdateVolume(coreCtx, volume).Return(nil) @@ -10860,41 +11053,6 @@ func TestPublishedNodesForBackend(t *testing.T) { assert.Equal(t, expectedNodes, actualNodes) } -func TestVolumePublicationsForBackend(t *testing.T) { - mockCtrl := gomock.NewController(t) - mockBackend := mockstorage.NewMockBackend(mockCtrl) - o := getOrchestrator(t, false) - - // orchestrator has nodeA and nodeB, backend has published to nodeA - o.nodes.Set("nodeA", &models.Node{Name: "nodeA"}) - o.nodes.Set("nodeB", &models.Node{Name: "nodeB"}) - o.volumePublications.Set("vol1", "nodeA", &models.VolumePublication{ - NodeName: "nodeA", - VolumeName: "vol1", - }) - o.volumePublications.Set("vol3", "nodeB", &models.VolumePublication{ - NodeName: "nodeB", - VolumeName: "vol3", - }) - mockBackend.EXPECT().Volumes().Return(makeSyncMap(map[string]*storage.Volume{ - "vol1": {Config: &storage.VolumeConfig{Name: "vol1", ExportPolicy: "pol1"}}, - "vol2": {Config: &storage.VolumeConfig{Name: "vol2", ExportPolicy: "pol2"}}, - })) - - expectedVolToPubs := map[string][]*models.VolumePublication{ - "vol1": { - { - NodeName: "nodeA", - VolumeName: "vol1", - }, - }, - "vol2": {}, - } - - actualVolToPubs := o.volumePublicationsForBackend(mockBackend) - assert.Equal(t, expectedVolToPubs, actualVolToPubs) -} - func TestReconcileBackendState(t *testing.T) { // Set fake values backendUUID := "1234" diff --git a/storage_drivers/ontap/ontap_common.go b/storage_drivers/ontap/ontap_common.go index f5d7b35d3..32dd75e3a 100644 --- a/storage_drivers/ontap/ontap_common.go +++ b/storage_drivers/ontap/ontap_common.go @@ -453,84 +453,7 @@ func ensureNodeAccessForPolicy( exportPolicyMutex.Lock(policyName) defer exportPolicyMutex.Unlock(policyName) - if exists, err := clientAPI.ExportPolicyExists(ctx, policyName); err != nil { - return err - } else if !exists { - Logc(ctx).WithField("exportPolicy", policyName).Debug("Export policy missing, will create it.") - - if err = clientAPI.ExportPolicyCreate(ctx, policyName); err != nil { - return err - } - } - - desiredRules, err := network.FilterIPs(ctx, targetNode.IPs, config.AutoExportCIDRs) - if err != nil { - err = fmt.Errorf("unable to determine desired export policy rules; %v", err) - Logc(ctx).Error(err) - return err - } - Logc(ctx).WithField("desiredRules", desiredRules).Debug("Desired export policy rules.") - - // first grab all existing rules - existingRules, err := clientAPI.ExportRuleList(ctx, policyName) - if err != nil { - // Could not list rules, just log it, no action required. - Logc(ctx).WithField("error", err).Debug("Export policy rules could not be listed.") - } - Logc(ctx).WithField("existingRules", existingRules).Debug("Existing export policy rules.") - - for _, desiredRule := range desiredRules { - desiredRule = strings.TrimSpace(desiredRule) - - desiredIP := net.ParseIP(desiredRule) - if desiredIP == nil { - Logc(ctx).WithField("desiredRule", desiredRule).Debug("Invalid desired rule IP") - continue - } - - // Loop through the existing rules one by one and compare to make sure we cover the scenario where the - // existing rule is of format "1.1.1.1, 2.2.2.2" and the desired rule is format "1.1.1.1". - // This can happen because of the difference in how ONTAP ZAPI and ONTAP REST creates export rule. - - ruleFound := false - for _, existingRule := range existingRules { - existingIPs := strings.Split(existingRule, ",") - - for _, ip := range existingIPs { - ip = strings.TrimSpace(ip) - - existingIP := net.ParseIP(ip) - if existingIP == nil { - Logc(ctx).WithField("existingRule", existingRule).Debug("Invalid existing rule IP") - continue - } - - if existingIP.Equal(desiredIP) { - ruleFound = true - break - } - } - - if ruleFound { - break - } - } - - // Rule does not exist, so create it - if !ruleFound { - if err = clientAPI.ExportRuleCreate(ctx, policyName, desiredRule, config.NASType); err != nil { - // Check if error is that the export policy rule already exist error - if errors.IsAlreadyExistsError(err) { - Logc(ctx).WithField("desiredRule", desiredRule).WithError(err).Debug( - "Export policy rule already exists") - continue - } - return err - } - } - } - - return nil + return ensureNodeAccessRulesForPolicy(ctx, []*tridentmodels.Node{targetNode}, clientAPI, config, policyName) } // ensureNodeAccessForPolicyAndApply ensures an export policy exists with the correct rules for the target node, @@ -558,6 +481,36 @@ func ensureNodeAccessForPolicyAndApply( exportPolicyMutex.Lock(policyName) defer exportPolicyMutex.Unlock(policyName) + if err := ensureNodeAccessRulesForPolicy( + ctx, []*tridentmodels.Node{targetNode}, clientAPI, config, policyName, + ); err != nil { + return err + } + + // Apply the policy while still holding the lock + if applyPolicy != nil { + if err := applyPolicy(); err != nil { + return err + } + } + + return nil +} + +// ensureNodeAccessRulesForPolicy ensures that the export policy identified by policyName exists (creating it if +// necessary) and that it contains rules granting access for every IP address of the given nodes, filtered by +// config.AutoExportCIDRs. +// +// This function only ADDS rules: it never deletes any existing rule, and it never changes which export policy +// is assigned to any volume or qtree. Callers that need to remove stale rules should use +// reconcileExportPolicyRules instead; that function acquires exportPolicyMutex for policyName itself, so it +// MUST be called WITHOUT already holding that lock. +// NOTE: Caller MUST hold the exportPolicyMutex for policyName before calling this function. +// NOTE: Every element of nodes must be non-nil; a nil element will panic in getDesiredExportPolicyRules. +func ensureNodeAccessRulesForPolicy( + ctx context.Context, nodes []*tridentmodels.Node, clientAPI api.OntapAPI, + config *drivers.OntapStorageDriverConfig, policyName string, +) error { if exists, err := clientAPI.ExportPolicyExists(ctx, policyName); err != nil { return err } else if !exists { @@ -568,12 +521,15 @@ func ensureNodeAccessForPolicyAndApply( } } - desiredRules, err := network.FilterIPs(ctx, targetNode.IPs, config.AutoExportCIDRs) + desiredRules, err := getDesiredExportPolicyRules(ctx, nodes, config) if err != nil { err = fmt.Errorf("unable to determine desired export policy rules; %v", err) Logc(ctx).Error(err) return err } + // getDesiredExportPolicyRules unions the rules via a map, so its iteration order is random; sort to keep + // rule-creation order deterministic. + sort.Strings(desiredRules) Logc(ctx).WithField("desiredRules", desiredRules).Debug("Desired export policy rules.") // first grab all existing rules @@ -635,13 +591,6 @@ func ensureNodeAccessForPolicyAndApply( } } - // Apply the policy while still holding the lock - if applyPolicy != nil { - if err = applyPolicy(); err != nil { - return err - } - } - return nil } diff --git a/storage_drivers/ontap/ontap_common_test.go b/storage_drivers/ontap/ontap_common_test.go index fc059fdd2..68cbffa44 100644 --- a/storage_drivers/ontap/ontap_common_test.go +++ b/storage_drivers/ontap/ontap_common_test.go @@ -5043,6 +5043,235 @@ func TestEnsureNodeAccessForPolicyAndApply_PolicyCreatedAndApplied(t *testing.T) assert.True(t, applyPolicyCalled, "applyPolicy callback should have been called after policy creation") } +func TestEnsureNodeAccessRulesForPolicy_MultiNodeUnionDedupe(t *testing.T) { + ctx := context.Background() + policyName := "trident-fakeUUID" + mockCtrl := gomock.NewController(t) + mockAPI := mockapi.NewMockOntapAPI(mockCtrl) + + // 10.0.0.2 appears on both nodes and must only be created once. + nodes := []*tridentmodels.Node{ + {Name: "node1", IPs: []string{"10.0.0.1", "10.0.0.2"}}, + {Name: "node2", IPs: []string{"10.0.0.2", "10.0.0.3"}}, + } + config := &drivers.OntapStorageDriverConfig{AutoExportCIDRs: []string{"10.0.0.0/24"}} + + mockAPI.EXPECT().ExportPolicyExists(ctx, policyName).Times(1).Return(true, nil) + mockAPI.EXPECT().ExportRuleList(ctx, policyName).Times(1).Return(make(map[int]string), nil) + // Rules must be created in ascending IP order, which only holds if the desired rules are sorted after + // getDesiredExportPolicyRules unions them (its map-based iteration order is otherwise random). InOrder + // pins this so removing the sort in the helper fails this test. + gomock.InOrder( + mockAPI.EXPECT().ExportRuleCreate(ctx, policyName, "10.0.0.1", gomock.Any()).Times(1).Return(nil), + mockAPI.EXPECT().ExportRuleCreate(ctx, policyName, "10.0.0.2", gomock.Any()).Times(1).Return(nil), + mockAPI.EXPECT().ExportRuleCreate(ctx, policyName, "10.0.0.3", gomock.Any()).Times(1).Return(nil), + ) + // This helper only adds rules; it must never destroy one. + mockAPI.EXPECT().ExportRuleDestroy(gomock.Any(), gomock.Any(), gomock.Any()).Times(0) + + err := ensureNodeAccessRulesForPolicy(ctx, nodes, mockAPI, config, policyName) + assert.NoError(t, err) +} + +func TestEnsureNodeAccessRulesForPolicy_CIDRFiltering(t *testing.T) { + ctx := context.Background() + policyName := "trident-fakeUUID" + mockCtrl := gomock.NewController(t) + mockAPI := mockapi.NewMockOntapAPI(mockCtrl) + + nodes := []*tridentmodels.Node{ + {Name: "node1", IPs: []string{"10.0.0.1", "192.168.1.1"}}, + } + config := &drivers.OntapStorageDriverConfig{AutoExportCIDRs: []string{"10.0.0.0/24"}} + + mockAPI.EXPECT().ExportPolicyExists(ctx, policyName).Times(1).Return(true, nil) + mockAPI.EXPECT().ExportRuleList(ctx, policyName).Times(1).Return(make(map[int]string), nil) + // 192.168.1.1 is outside AutoExportCIDRs and is filtered out before it ever reaches ExportRuleCreate, so no + // expectation is registered for it; an unexpected call for it would fail the test. + mockAPI.EXPECT().ExportRuleCreate(ctx, policyName, "10.0.0.1", gomock.Any()).Times(1).Return(nil) + + err := ensureNodeAccessRulesForPolicy(ctx, nodes, mockAPI, config, policyName) + assert.NoError(t, err) +} + +func TestEnsureNodeAccessRulesForPolicy_PolicyMissingCreatesThenAddsRules(t *testing.T) { + ctx := context.Background() + policyName := "trident-fakeUUID" + mockCtrl := gomock.NewController(t) + mockAPI := mockapi.NewMockOntapAPI(mockCtrl) + + nodeIP := "10.0.0.1" + nodes := []*tridentmodels.Node{{Name: "node1", IPs: []string{nodeIP}}} + config := &drivers.OntapStorageDriverConfig{AutoExportCIDRs: []string{"10.0.0.0/24"}} + + mockAPI.EXPECT().ExportPolicyExists(ctx, policyName).Times(1).Return(false, nil) + mockAPI.EXPECT().ExportPolicyCreate(ctx, policyName).Times(1).Return(nil) + ruleListCall := mockAPI.EXPECT().ExportRuleList(ctx, policyName).Times(1).Return(make(map[int]string), nil) + mockAPI.EXPECT().ExportRuleCreate(ctx, policyName, nodeIP, gomock.Any()).After(ruleListCall).Times(1).Return(nil) + + err := ensureNodeAccessRulesForPolicy(ctx, nodes, mockAPI, config, policyName) + assert.NoError(t, err) +} + +func TestEnsureNodeAccessRulesForPolicy_ExistingCommaFormatRuleNotRecreated(t *testing.T) { + ctx := context.Background() + policyName := "trident-fakeUUID" + mockCtrl := gomock.NewController(t) + mockAPI := mockapi.NewMockOntapAPI(mockCtrl) + + nodes := []*tridentmodels.Node{ + {Name: "node1", IPs: []string{"10.1.1.26"}}, + {Name: "node2", IPs: []string{"10.1.1.27"}}, + } + config := &drivers.OntapStorageDriverConfig{AutoExportCIDRs: []string{"0.0.0.0/0"}} + + // Existing ZAPI-style rule combines both IPs into a single comma-separated entry. + zapiExportRule := map[int]string{1: "10.1.1.26, 10.1.1.27"} + + mockAPI.EXPECT().ExportPolicyExists(ctx, policyName).Times(1).Return(true, nil) + mockAPI.EXPECT().ExportRuleList(ctx, policyName).Times(1).Return(zapiExportRule, nil) + mockAPI.EXPECT().ExportRuleCreate(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(0) + + err := ensureNodeAccessRulesForPolicy(ctx, nodes, mockAPI, config, policyName) + assert.NoError(t, err) +} + +func TestEnsureNodeAccessRulesForPolicy_ExportPolicyExistsErrorPropagated(t *testing.T) { + ctx := context.Background() + policyName := "trident-fakeUUID" + mockCtrl := gomock.NewController(t) + mockAPI := mockapi.NewMockOntapAPI(mockCtrl) + + nodes := []*tridentmodels.Node{{Name: "node1", IPs: []string{"10.0.0.1"}}} + config := &drivers.OntapStorageDriverConfig{AutoExportCIDRs: []string{"10.0.0.0/24"}} + + expectedErr := errors.New("API error checking export policy existence") + mockAPI.EXPECT().ExportPolicyExists(ctx, policyName).Times(1).Return(false, expectedErr) + mockAPI.EXPECT().ExportPolicyCreate(gomock.Any(), gomock.Any()).Times(0) + mockAPI.EXPECT().ExportRuleList(gomock.Any(), gomock.Any()).Times(0) + mockAPI.EXPECT().ExportRuleCreate(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(0) + + err := ensureNodeAccessRulesForPolicy(ctx, nodes, mockAPI, config, policyName) + assert.Error(t, err) + assert.Equal(t, expectedErr, err) +} + +func TestEnsureNodeAccessRulesForPolicy_ExportRuleCreateErrorPropagated(t *testing.T) { + ctx := context.Background() + policyName := "trident-fakeUUID" + mockCtrl := gomock.NewController(t) + mockAPI := mockapi.NewMockOntapAPI(mockCtrl) + + nodeIP := "10.0.0.1" + nodes := []*tridentmodels.Node{{Name: "node1", IPs: []string{nodeIP}}} + config := &drivers.OntapStorageDriverConfig{AutoExportCIDRs: []string{"10.0.0.0/24"}} + + expectedErr := errors.New("generic export rule create failure") + mockAPI.EXPECT().ExportPolicyExists(ctx, policyName).Times(1).Return(true, nil) + mockAPI.EXPECT().ExportRuleList(ctx, policyName).Times(1).Return(make(map[int]string), nil) + mockAPI.EXPECT().ExportRuleCreate(ctx, policyName, nodeIP, gomock.Any()).Times(1).Return(expectedErr) + + err := ensureNodeAccessRulesForPolicy(ctx, nodes, mockAPI, config, policyName) + assert.Error(t, err) + assert.Equal(t, expectedErr, err) +} + +func TestEnsureNodeAccessRulesForPolicy_ExportRuleCreateAlreadyExistsTolerated(t *testing.T) { + ctx := context.Background() + policyName := "trident-fakeUUID" + mockCtrl := gomock.NewController(t) + mockAPI := mockapi.NewMockOntapAPI(mockCtrl) + + // Rules are created in sorted order, so "10.0.0.1" is attempted before "10.0.0.2". + nodes := []*tridentmodels.Node{{Name: "node1", IPs: []string{"10.0.0.1", "10.0.0.2"}}} + config := &drivers.OntapStorageDriverConfig{AutoExportCIDRs: []string{"10.0.0.0/24"}} + + mockAPI.EXPECT().ExportPolicyExists(ctx, policyName).Times(1).Return(true, nil) + mockAPI.EXPECT().ExportRuleList(ctx, policyName).Times(1).Return(make(map[int]string), nil) + gomock.InOrder( + mockAPI.EXPECT().ExportRuleCreate(ctx, policyName, "10.0.0.1", gomock.Any()).Times(1). + Return(errors.AlreadyExistsError("rule already exists")), + mockAPI.EXPECT().ExportRuleCreate(ctx, policyName, "10.0.0.2", gomock.Any()).Times(1).Return(nil), + ) + + err := ensureNodeAccessRulesForPolicy(ctx, nodes, mockAPI, config, policyName) + assert.NoError(t, err) +} + +func TestEnsureNodeAccessRulesForPolicy_ExportRuleListErrorTolerated(t *testing.T) { + ctx := context.Background() + policyName := "trident-fakeUUID" + mockCtrl := gomock.NewController(t) + mockAPI := mockapi.NewMockOntapAPI(mockCtrl) + + nodes := []*tridentmodels.Node{{Name: "node1", IPs: []string{"10.0.0.1", "10.0.0.2"}}} + config := &drivers.OntapStorageDriverConfig{AutoExportCIDRs: []string{"10.0.0.0/24"}} + + mockAPI.EXPECT().ExportPolicyExists(ctx, policyName).Times(1).Return(true, nil) + mockAPI.EXPECT().ExportRuleList(ctx, policyName).Times(1).Return(nil, errors.New("ontap error")) + mockAPI.EXPECT().ExportRuleCreate(ctx, policyName, "10.0.0.1", gomock.Any()).Times(1).Return(nil) + mockAPI.EXPECT().ExportRuleCreate(ctx, policyName, "10.0.0.2", gomock.Any()).Times(1).Return(nil) + + err := ensureNodeAccessRulesForPolicy(ctx, nodes, mockAPI, config, policyName) + assert.NoError(t, err) +} + +func TestEnsureNodeAccessRulesForPolicy_EmptyNodesSlice(t *testing.T) { + ctx := context.Background() + policyName := "trident-fakeUUID" + mockCtrl := gomock.NewController(t) + mockAPI := mockapi.NewMockOntapAPI(mockCtrl) + + nodes := []*tridentmodels.Node{} + config := &drivers.OntapStorageDriverConfig{AutoExportCIDRs: []string{"10.0.0.0/24"}} + + mockAPI.EXPECT().ExportPolicyExists(ctx, policyName).Times(1).Return(true, nil) + mockAPI.EXPECT().ExportRuleList(ctx, policyName).Times(1).Return(make(map[int]string), nil) + mockAPI.EXPECT().ExportRuleCreate(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(0) + + err := ensureNodeAccessRulesForPolicy(ctx, nodes, mockAPI, config, policyName) + assert.NoError(t, err) +} + +func TestEnsureNodeAccessRulesForPolicy_EmptyNodesPolicyMissing(t *testing.T) { + ctx := context.Background() + policyName := "trident-fakeUUID" + mockCtrl := gomock.NewController(t) + mockAPI := mockapi.NewMockOntapAPI(mockCtrl) + + nodes := []*tridentmodels.Node{} + config := &drivers.OntapStorageDriverConfig{AutoExportCIDRs: []string{"10.0.0.0/24"}} + + // Even with no nodes, a missing policy must still be created (deny-all until rules are added). + mockAPI.EXPECT().ExportPolicyExists(ctx, policyName).Times(1).Return(false, nil) + mockAPI.EXPECT().ExportPolicyCreate(ctx, policyName).Times(1).Return(nil) + mockAPI.EXPECT().ExportRuleList(ctx, policyName).Times(1).Return(make(map[int]string), nil) + mockAPI.EXPECT().ExportRuleCreate(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(0) + + err := ensureNodeAccessRulesForPolicy(ctx, nodes, mockAPI, config, policyName) + assert.NoError(t, err) +} + +func TestEnsureNodeAccessRulesForPolicy_DesiredRulesErrorPropagated(t *testing.T) { + ctx := context.Background() + policyName := "trident-fakeUUID" + mockCtrl := gomock.NewController(t) + mockAPI := mockapi.NewMockOntapAPI(mockCtrl) + + nodes := []*tridentmodels.Node{{Name: "node1", IPs: []string{"192.168.1.1"}}} + // /35 is not a valid IPv4 prefix length, so getDesiredExportPolicyRules (via network.FilterIPs) fails + // before any rule listing or creation is attempted. + config := &drivers.OntapStorageDriverConfig{AutoExportCIDRs: []string{"192.168.1.0/35"}} + + mockAPI.EXPECT().ExportPolicyExists(ctx, policyName).Times(1).Return(true, nil) + mockAPI.EXPECT().ExportRuleList(gomock.Any(), gomock.Any()).Times(0) + mockAPI.EXPECT().ExportRuleCreate(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(0) + + err := ensureNodeAccessRulesForPolicy(ctx, nodes, mockAPI, config, policyName) + assert.Error(t, err) + assert.Contains(t, err.Error(), "unable to determine desired export policy rules") +} + func TestIsDefaultAuthTypeOfType(t *testing.T) { response := api.IscsiInitiatorAuth{ AuthType: "fakeAuthType", diff --git a/storage_drivers/ontap/ontap_nas.go b/storage_drivers/ontap/ontap_nas.go index aa9aa039f..3c9eb55d1 100644 --- a/storage_drivers/ontap/ontap_nas.go +++ b/storage_drivers/ontap/ontap_nas.go @@ -1873,8 +1873,14 @@ func (d *NASStorageDriver) reconcileNodeAccessForBackendPolicy( return nil } +// ReconcileVolumeNodeAccess adds any missing export rules for the published nodes to the volume's own +// per-volume export policy. It never removes a rule or changes which policy is assigned. Volumes that +// are not on a Trident-managed per-volume policy (still on the backend policy, unpublished, or on a +// customer-managed policy) are skipped, as are read-only clones, which never get a policy of their own +// on this driver. The recorded ExportPolicy is trusted rather than re-read from ONTAP; a stale record +// only delays repair until the volume's next publish or unpublish. func (d *NASStorageDriver) ReconcileVolumeNodeAccess( - ctx context.Context, _ *storage.VolumeConfig, _ []*models.Node, + ctx context.Context, volConfig *storage.VolumeConfig, nodes []*models.Node, ) error { fields := LogFields{ "Method": "ReconcileVolumeNodeAccess", @@ -1883,7 +1889,43 @@ func (d *NASStorageDriver) ReconcileVolumeNodeAccess( Logd(ctx, d.Name(), d.Config.DebugTraceFlags["method"]).WithFields(fields).Trace(">>>> ReconcileVolumeNodeAccess") defer Logd(ctx, d.Name(), d.Config.DebugTraceFlags["method"]).WithFields(fields).Trace("<<<< ReconcileVolumeNodeAccess") - return nil + skipFields := LogFields{ + "volume": volConfig.InternalName, + "exportPolicy": volConfig.ExportPolicy, + } + + if !d.Config.AutoExportPolicy { + Logc(ctx).WithFields(skipFields).Debug( + "Auto export policies are not turned on; skipping export policy reconciliation.") + return nil + } + + if len(nodes) == 0 { + Logc(ctx).WithFields(skipFields).Debug( + "No published nodes to reconcile; skipping export policy reconciliation.") + return nil + } + + if volConfig.InternalName == "" { + Logc(ctx).WithFields(skipFields).Debug( + "Volume has no internal name; skipping export policy reconciliation.") + return nil + } + + if volConfig.ExportPolicy != volConfig.InternalName { + Logc(ctx).WithFields(skipFields).Debug( + "Volume is not on a per-volume export policy; skipping export policy reconciliation.") + return nil + } + + policyName := volConfig.ExportPolicy + + // ensureNodeAccessRulesForPolicy requires the caller to hold the export policy lock; it only adds missing + // rules and never assigns or deletes a policy. + exportPolicyMutex.Lock(policyName) + defer exportPolicyMutex.Unlock(policyName) + + return ensureNodeAccessRulesForPolicy(ctx, nodes, d.API, &d.Config, policyName) } // GetBackendState returns the reason if SVM is offline, and a flag to indicate if there is change diff --git a/storage_drivers/ontap/ontap_nas_qtree.go b/storage_drivers/ontap/ontap_nas_qtree.go index ced589710..90c5ec4db 100644 --- a/storage_drivers/ontap/ontap_nas_qtree.go +++ b/storage_drivers/ontap/ontap_nas_qtree.go @@ -2918,7 +2918,19 @@ func (d *NASQtreeStorageDriver) ReconcileNodeAccess( return reconcileNASNodeAccess(ctx, nodes, &d.Config, d.API, backendPolicyName) } -func (d *NASQtreeStorageDriver) ReconcileVolumeNodeAccess(ctx context.Context, volConfig *storage.VolumeConfig, nodes []*models.Node) error { +// ReconcileVolumeNodeAccess repairs (add-only) the volume's qtree-level export policy so it grants access to +// every currently published node; it never removes a rule or changes which policy is assigned. Read-only +// clones have no export policy of their own -- they share their source qtree's policy and are reconciled +// against it. It is a no-op when auto export policies are disabled, there are no published nodes, the volume +// has no internal name, or the volume is not (yet) on a Trident-managed per-qtree policy -- e.g. it is still +// on the backend policy, unpublished, predates the per-qtree policy field, or uses a customer-managed policy. +// This method trusts the volume's recorded ExportPolicy instead of querying ONTAP for it; if that record is +// stale (rare crash or out-of-band edit), the volume is skipped until its next publish/unpublish, or an +// unreferenced policy may be recreated (possibly including rules for nodes that have since unpublished) -- +// an accepted trade-off to keep the reconcile cost bounded. +func (d *NASQtreeStorageDriver) ReconcileVolumeNodeAccess( + ctx context.Context, volConfig *storage.VolumeConfig, nodes []*models.Node, +) error { fields := LogFields{ "Method": "ReconcileVolumeNodeAccess", "Type": "NASQtreeStorageDriver", @@ -2926,7 +2938,50 @@ func (d *NASQtreeStorageDriver) ReconcileVolumeNodeAccess(ctx context.Context, v Logd(ctx, d.Name(), d.Config.DebugTraceFlags["method"]).WithFields(fields).Trace(">>>> ReconcileVolumeNodeAccess") defer Logd(ctx, d.Name(), d.Config.DebugTraceFlags["method"]).WithFields(fields).Trace("<<<< ReconcileVolumeNodeAccess") - return nil + skipFields := LogFields{ + "volume": volConfig.InternalName, + "exportPolicy": volConfig.ExportPolicy, + } + + if !d.Config.AutoExportPolicy { + Logc(ctx).WithFields(skipFields).Debug( + "Auto export policies are not turned on; skipping export policy reconciliation.") + return nil + } + + if len(nodes) == 0 { + Logc(ctx).WithFields(skipFields).Debug( + "No published nodes to reconcile; skipping export policy reconciliation.") + return nil + } + + if volConfig.InternalName == "" { + Logc(ctx).WithFields(skipFields).Debug( + "Volume has no internal name; skipping export policy reconciliation.") + return nil + } + + // A volume is on a Trident-managed per-qtree policy either because it owns that policy outright, or + // because it is a read-only clone sharing its source qtree's policy (RO clones never get their own + // export policy; Publish/publishQtreeShare leaves volConfig.ExportPolicy pointed at the source qtree). + onOwnPolicy := volConfig.ExportPolicy == volConfig.InternalName + onSourcePolicy := volConfig.ReadOnlyClone && volConfig.CloneSourceVolumeInternal != "" && + volConfig.ExportPolicy == volConfig.CloneSourceVolumeInternal + + if !onOwnPolicy && !onSourcePolicy { + Logc(ctx).WithFields(skipFields).Debug( + "Volume is not on a qtree-level export policy; skipping export policy reconciliation.") + return nil + } + + policyName := volConfig.ExportPolicy + + // ensureNodeAccessRulesForPolicy requires the caller to hold the export policy lock; it only adds missing + // rules and never assigns or deletes a policy. + exportPolicyMutex.Lock(policyName) + defer exportPolicyMutex.Unlock(policyName) + + return ensureNodeAccessRulesForPolicy(ctx, nodes, d.API, &d.Config, policyName) } // GetBackendState returns the reason if SVM is offline, and a flag to indicate if there is change diff --git a/storage_drivers/ontap/ontap_nas_qtree_test.go b/storage_drivers/ontap/ontap_nas_qtree_test.go index 9add2aae2..c956f1f0f 100644 --- a/storage_drivers/ontap/ontap_nas_qtree_test.go +++ b/storage_drivers/ontap/ontap_nas_qtree_test.go @@ -4418,6 +4418,219 @@ func TestReconcileNodeAccess(t *testing.T) { assert.NoError(t, err, "Reconcile node access failed") } +func TestNASQtreeStorageDriverReconcileVolumeNodeAccess_AutoExportPolicyDisabled(t *testing.T) { + // Strict mock, zero expectations registered: auto export policy is off, so no ONTAP call of any kind + // should ever be made, regardless of nodes/policy state. Any call would fail the test as unexpected. + _, driver := newMockOntapNasQtreeDriver(t) + driver.Config.AutoExportPolicy = false + driver.Config.AutoExportCIDRs = []string{"10.0.0.0/24"} + + volConfig := &storage.VolumeConfig{InternalName: "trident_pvc_x", ExportPolicy: "trident_pvc_x"} + nodes := []*models.Node{{Name: "node-1", IPs: []string{"10.0.0.1"}}} + + err := driver.ReconcileVolumeNodeAccess(ctx, volConfig, nodes) + assert.NoError(t, err) +} + +func TestNASQtreeStorageDriverReconcileVolumeNodeAccess_EmptyNodes(t *testing.T) { + // No published nodes: an add-only reconciler has nothing to add, so it must not call ONTAP. + _, driver := newMockOntapNasQtreeDriver(t) + driver.Config.AutoExportPolicy = true + driver.Config.AutoExportCIDRs = []string{"10.0.0.0/24"} + + volConfig := &storage.VolumeConfig{InternalName: "trident_pvc_x", ExportPolicy: "trident_pvc_x"} + nodes := []*models.Node{} + + err := driver.ReconcileVolumeNodeAccess(ctx, volConfig, nodes) + assert.NoError(t, err) +} + +func TestNASQtreeStorageDriverReconcileVolumeNodeAccess_BackendPolicy(t *testing.T) { + // Volume is still on the backend policy; that is repaired by ReconcileNodeAccess, not this method. + _, driver := newMockOntapNasQtreeDriver(t) + driver.Config.AutoExportPolicy = true + driver.Config.AutoExportCIDRs = []string{"10.0.0.0/24"} + + backendPolicy := getExportPolicyName(BackendUUID) + volConfig := &storage.VolumeConfig{InternalName: "trident_pvc_x", ExportPolicy: backendPolicy} + nodes := []*models.Node{{Name: "node-1", IPs: []string{"10.0.0.1"}}} + + err := driver.ReconcileVolumeNodeAccess(ctx, volConfig, nodes) + assert.NoError(t, err) +} + +func TestNASQtreeStorageDriverReconcileVolumeNodeAccess_EmptyExportPolicyName(t *testing.T) { + // Volume is unpublished (on the empty policy); nothing to reconcile until it is published. + _, driver := newMockOntapNasQtreeDriver(t) + driver.Config.AutoExportPolicy = true + driver.Config.AutoExportCIDRs = []string{"10.0.0.0/24"} + + emptyPolicy := getEmptyExportPolicyName(*driver.Config.StoragePrefix) + volConfig := &storage.VolumeConfig{InternalName: "trident_pvc_x", ExportPolicy: emptyPolicy} + nodes := []*models.Node{{Name: "node-1", IPs: []string{"10.0.0.1"}}} + + err := driver.ReconcileVolumeNodeAccess(ctx, volConfig, nodes) + assert.NoError(t, err) +} + +func TestNASQtreeStorageDriverReconcileVolumeNodeAccess_UnmanagedPolicy(t *testing.T) { + // Customer-managed export policy; Trident must never touch it. + _, driver := newMockOntapNasQtreeDriver(t) + driver.Config.AutoExportPolicy = true + driver.Config.AutoExportCIDRs = []string{"10.0.0.0/24"} + + volConfig := &storage.VolumeConfig{InternalName: "trident_pvc_x", ExportPolicy: "default"} + nodes := []*models.Node{{Name: "node-1", IPs: []string{"10.0.0.1"}}} + + err := driver.ReconcileVolumeNodeAccess(ctx, volConfig, nodes) + assert.NoError(t, err) +} + +func TestNASQtreeStorageDriverReconcileVolumeNodeAccess_UnsetExportPolicy(t *testing.T) { + // Pre-23.04 upgrade case: ExportPolicy was never populated. Heals at the next publish/unpublish, not here. + _, driver := newMockOntapNasQtreeDriver(t) + driver.Config.AutoExportPolicy = true + driver.Config.AutoExportCIDRs = []string{"10.0.0.0/24"} + + volConfig := &storage.VolumeConfig{InternalName: "trident_pvc_x", ExportPolicy: ""} + nodes := []*models.Node{{Name: "node-1", IPs: []string{"10.0.0.1"}}} + + err := driver.ReconcileVolumeNodeAccess(ctx, volConfig, nodes) + assert.NoError(t, err) +} + +func TestNASQtreeStorageDriverReconcileVolumeNodeAccess_EmptyInternalName(t *testing.T) { + // Pins that the InternalName == "" guard runs before the policy-equality arms: without it, the dangerous + // combination InternalName == "" && ExportPolicy == "" would satisfy volConfig.ExportPolicy == + // volConfig.InternalName and go on to ensure a policy literally named "". + _, driver := newMockOntapNasQtreeDriver(t) + driver.Config.AutoExportPolicy = true + driver.Config.AutoExportCIDRs = []string{"10.0.0.0/24"} + + volConfig := &storage.VolumeConfig{InternalName: "", ExportPolicy: "", ReadOnlyClone: false} + nodes := []*models.Node{{Name: "node-1", IPs: []string{"10.0.0.1"}}} + + err := driver.ReconcileVolumeNodeAccess(ctx, volConfig, nodes) + assert.NoError(t, err) +} + +func TestNASQtreeStorageDriverReconcileVolumeNodeAccess_ROCloneUnmanaged(t *testing.T) { + // A read-only clone whose ExportPolicy is a customer-managed (or otherwise unrelated) policy -- not the + // source qtree's policy -- must be skipped just like a non-clone volume on an unmanaged policy. + _, driver := newMockOntapNasQtreeDriver(t) + driver.Config.AutoExportPolicy = true + driver.Config.AutoExportCIDRs = []string{"10.0.0.0/24"} + + volConfig := &storage.VolumeConfig{ + InternalName: "trident_pvc_clone", + ExportPolicy: "default", + ReadOnlyClone: true, + CloneSourceVolumeInternal: "trident_pvc_src", + } + nodes := []*models.Node{{Name: "node-1", IPs: []string{"10.0.0.1"}}} + + err := driver.ReconcileVolumeNodeAccess(ctx, volConfig, nodes) + assert.NoError(t, err) +} + +func TestNASQtreeStorageDriverReconcileVolumeNodeAccess_HappyPathMultiNode(t *testing.T) { + mockAPI, driver := newMockOntapNasQtreeDriver(t) + driver.Config.AutoExportPolicy = true + driver.Config.AutoExportCIDRs = []string{"10.0.0.0/24"} + + policyName := "trident_pvc_x" + volConfig := &storage.VolumeConfig{InternalName: policyName, ExportPolicy: policyName} + nodes := []*models.Node{ + {Name: "node-1", IPs: []string{"10.0.0.5"}}, + {Name: "node-2", IPs: []string{"10.0.0.6"}}, + } + + // One stale rule is already present (e.g. left over from a node that is no longer published); the + // add-only helper must leave it alone and only create the two rules the current nodes are missing. + mockAPI.EXPECT().ExportPolicyExists(ctx, policyName).Times(1).Return(true, nil) + mockAPI.EXPECT().ExportRuleList(ctx, policyName).Times(1).Return(map[int]string{1: "192.168.9.9"}, nil) + gomock.InOrder( + mockAPI.EXPECT().ExportRuleCreate(ctx, policyName, "10.0.0.5", gomock.Any()).Times(1).Return(nil), + mockAPI.EXPECT().ExportRuleCreate(ctx, policyName, "10.0.0.6", gomock.Any()).Times(1).Return(nil), + ) + // Pin add-only + no-assignment at the driver level: this method must never destroy a rule or (re)assign + // the policy to the qtree. + mockAPI.EXPECT().ExportRuleDestroy(gomock.Any(), gomock.Any(), gomock.Any()).Times(0) + mockAPI.EXPECT().QtreeModifyExportPolicy(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(0) + + err := driver.ReconcileVolumeNodeAccess(ctx, volConfig, nodes) + assert.NoError(t, err) +} + +func TestNASQtreeStorageDriverReconcileVolumeNodeAccess_ROClone(t *testing.T) { + // A published read-only clone has no export policy of its own: publishQtreeShare leaves its ExportPolicy + // pointed at the source qtree's policy. Reconciliation must run against that shared source-named policy, + // using the clone's own published node/IP, and must never destroy a rule or reassign the policy. + mockAPI, driver := newMockOntapNasQtreeDriver(t) + driver.Config.AutoExportPolicy = true + driver.Config.AutoExportCIDRs = []string{"10.0.0.0/24"} + + sourcePolicy := "trident_pvc_src" + volConfig := &storage.VolumeConfig{ + InternalName: "trident_pvc_clone", + ExportPolicy: sourcePolicy, + ReadOnlyClone: true, + CloneSourceVolumeInternal: sourcePolicy, + } + nodes := []*models.Node{{Name: "node-1", IPs: []string{"10.0.0.9"}}} + + mockAPI.EXPECT().ExportPolicyExists(ctx, sourcePolicy).Times(1).Return(true, nil) + mockAPI.EXPECT().ExportRuleList(ctx, sourcePolicy).Times(1).Return(make(map[int]string), nil) + mockAPI.EXPECT().ExportRuleCreate(ctx, sourcePolicy, "10.0.0.9", gomock.Any()).Times(1).Return(nil) + mockAPI.EXPECT().ExportRuleDestroy(gomock.Any(), gomock.Any(), gomock.Any()).Times(0) + mockAPI.EXPECT().QtreeModifyExportPolicy(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(0) + + err := driver.ReconcileVolumeNodeAccess(ctx, volConfig, nodes) + assert.NoError(t, err) +} + +func TestNASQtreeStorageDriverReconcileVolumeNodeAccess_PolicyMissingRecreated(t *testing.T) { + // #1180-adjacent recovery: the policy was deleted out-of-band while the qtree still references it by name. + mockAPI, driver := newMockOntapNasQtreeDriver(t) + driver.Config.AutoExportPolicy = true + driver.Config.AutoExportCIDRs = []string{"10.0.0.0/24"} + + policyName := "trident_pvc_y" + volConfig := &storage.VolumeConfig{InternalName: policyName, ExportPolicy: policyName} + nodes := []*models.Node{{Name: "node-1", IPs: []string{"10.0.0.7"}}} + + // The whole sequence is order-pinned so an inverted implementation (e.g. creating the rule before checking + // or creating the policy) fails this test, not just an implementation that merely calls each method once. + gomock.InOrder( + mockAPI.EXPECT().ExportPolicyExists(ctx, policyName).Times(1).Return(false, nil), + mockAPI.EXPECT().ExportPolicyCreate(ctx, policyName).Times(1).Return(nil), + mockAPI.EXPECT().ExportRuleList(ctx, policyName).Times(1).Return(make(map[int]string), nil), + mockAPI.EXPECT().ExportRuleCreate(ctx, policyName, "10.0.0.7", gomock.Any()).Times(1).Return(nil), + ) + + err := driver.ReconcileVolumeNodeAccess(ctx, volConfig, nodes) + assert.NoError(t, err) +} + +func TestNASQtreeStorageDriverReconcileVolumeNodeAccess_HelperErrorPropagated(t *testing.T) { + mockAPI, driver := newMockOntapNasQtreeDriver(t) + driver.Config.AutoExportPolicy = true + driver.Config.AutoExportCIDRs = []string{"10.0.0.0/24"} + + policyName := "trident_pvc_z" + volConfig := &storage.VolumeConfig{InternalName: policyName, ExportPolicy: policyName} + nodes := []*models.Node{{Name: "node-1", IPs: []string{"10.0.0.8"}}} + + mockAPI.EXPECT().ExportPolicyExists(ctx, policyName).Times(1).Return(false, mockError) + mockAPI.EXPECT().ExportPolicyCreate(gomock.Any(), gomock.Any()).Times(0) + mockAPI.EXPECT().ExportRuleList(gomock.Any(), gomock.Any()).Times(0) + mockAPI.EXPECT().ExportRuleCreate(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(0) + + err := driver.ReconcileVolumeNodeAccess(ctx, volConfig, nodes) + assert.Error(t, err) + assert.Equal(t, mockError, err) +} + func TestEnsureSMBShare_Success_WithSMBShareInConfig(t *testing.T) { volName := "vol1" mockAPI, driver := newMockOntapNasQtreeDriver(t) diff --git a/storage_drivers/ontap/ontap_nas_test.go b/storage_drivers/ontap/ontap_nas_test.go index 2a78e7217..50b5ad564 100644 --- a/storage_drivers/ontap/ontap_nas_test.go +++ b/storage_drivers/ontap/ontap_nas_test.go @@ -6573,27 +6573,200 @@ func TestOntapNASDriverGetConfig(t *testing.T) { assert.NotNil(t, config) } -func TestOntapNASDriverReconcileVolumeNodeAccess(t *testing.T) { - ctx := context.Background() +func TestOntapNASDriverReconcileVolumeNodeAccess_AutoExportPolicyDisabled(t *testing.T) { + // Strict mock, zero expectations registered: auto export policy is off, so no ONTAP call of any kind + // should ever be made, regardless of nodes/policy state. Any call would fail the test as unexpected. + _, driver := newMockOntapNASDriverWithSVM(t, "SVM1") + driver.Config.AutoExportPolicy = false + driver.Config.AutoExportCIDRs = []string{"10.0.0.0/24"} + + volConfig := &storage.VolumeConfig{InternalName: "trident_pvc_x", ExportPolicy: "trident_pvc_x"} + nodes := []*models.Node{{Name: "node-1", IPs: []string{"10.0.0.1"}}} + + err := driver.ReconcileVolumeNodeAccess(ctx, volConfig, nodes) + assert.NoError(t, err) +} + +func TestOntapNASDriverReconcileVolumeNodeAccess_EmptyNodes(t *testing.T) { + // No published nodes: an add-only reconciler has nothing to add, so it must not call ONTAP. + _, driver := newMockOntapNASDriverWithSVM(t, "SVM1") + driver.Config.AutoExportPolicy = true + driver.Config.AutoExportCIDRs = []string{"10.0.0.0/24"} + + volConfig := &storage.VolumeConfig{InternalName: "trident_pvc_x", ExportPolicy: "trident_pvc_x"} + nodes := []*models.Node{} + + err := driver.ReconcileVolumeNodeAccess(ctx, volConfig, nodes) + assert.NoError(t, err) +} + +func TestOntapNASDriverReconcileVolumeNodeAccess_EmptyInternalName(t *testing.T) { + // Pins that the InternalName == "" guard runs before the policy-equality check: without it, the + // dangerous combination InternalName == "" && ExportPolicy == "" would satisfy volConfig.ExportPolicy == + // volConfig.InternalName and go on to ensure a policy literally named "". + _, driver := newMockOntapNASDriverWithSVM(t, "SVM1") + driver.Config.AutoExportPolicy = true + driver.Config.AutoExportCIDRs = []string{"10.0.0.0/24"} + + volConfig := &storage.VolumeConfig{InternalName: "", ExportPolicy: ""} + nodes := []*models.Node{{Name: "node-1", IPs: []string{"10.0.0.1"}}} + + err := driver.ReconcileVolumeNodeAccess(ctx, volConfig, nodes) + assert.NoError(t, err) +} + +func TestOntapNASDriverReconcileVolumeNodeAccess_BackendPolicy(t *testing.T) { + // Volume is still on the backend policy; that is repaired by ReconcileNodeAccess, not this method. + _, driver := newMockOntapNASDriverWithSVM(t, "SVM1") + driver.Config.AutoExportPolicy = true + driver.Config.AutoExportCIDRs = []string{"10.0.0.0/24"} + + backendPolicy := getExportPolicyName(BackendUUID) + volConfig := &storage.VolumeConfig{InternalName: "trident_pvc_x", ExportPolicy: backendPolicy} + nodes := []*models.Node{{Name: "node-1", IPs: []string{"10.0.0.1"}}} + + err := driver.ReconcileVolumeNodeAccess(ctx, volConfig, nodes) + assert.NoError(t, err) +} + +func TestOntapNASDriverReconcileVolumeNodeAccess_EmptyExportPolicyName(t *testing.T) { + // Volume is unpublished (on the empty policy); nothing to reconcile until it is published. _, driver := newMockOntapNASDriverWithSVM(t, "SVM1") + driver.Config.AutoExportPolicy = true + driver.Config.AutoExportCIDRs = []string{"10.0.0.0/24"} + + emptyPolicy := getEmptyExportPolicyName(*driver.Config.StoragePrefix) + volConfig := &storage.VolumeConfig{InternalName: "trident_pvc_x", ExportPolicy: emptyPolicy} + nodes := []*models.Node{{Name: "node-1", IPs: []string{"10.0.0.1"}}} + + err := driver.ReconcileVolumeNodeAccess(ctx, volConfig, nodes) + assert.NoError(t, err) +} + +func TestOntapNASDriverReconcileVolumeNodeAccess_UnmanagedPolicy(t *testing.T) { + // Customer-managed export policy; Trident must never touch it. + _, driver := newMockOntapNASDriverWithSVM(t, "SVM1") + driver.Config.AutoExportPolicy = true + driver.Config.AutoExportCIDRs = []string{"10.0.0.0/24"} + + volConfig := &storage.VolumeConfig{InternalName: "trident_pvc_x", ExportPolicy: "default"} + nodes := []*models.Node{{Name: "node-1", IPs: []string{"10.0.0.1"}}} + + err := driver.ReconcileVolumeNodeAccess(ctx, volConfig, nodes) + assert.NoError(t, err) +} + +func TestOntapNASDriverReconcileVolumeNodeAccess_UnsetExportPolicy(t *testing.T) { + // Pre-23.04 upgrade case: ExportPolicy was never populated. Heals at the next publish/unpublish, not here. + _, driver := newMockOntapNASDriverWithSVM(t, "SVM1") + driver.Config.AutoExportPolicy = true + driver.Config.AutoExportCIDRs = []string{"10.0.0.0/24"} + + volConfig := &storage.VolumeConfig{InternalName: "trident_pvc_x", ExportPolicy: ""} + nodes := []*models.Node{{Name: "node-1", IPs: []string{"10.0.0.1"}}} + + err := driver.ReconcileVolumeNodeAccess(ctx, volConfig, nodes) + assert.NoError(t, err) +} + +func TestOntapNASDriverReconcileVolumeNodeAccess_ROCloneSkipped(t *testing.T) { + // Pins that ontap-nas deliberately has NO economy-style shared-source-policy arm. ExportPolicy is set + // equal to CloneSourceVolumeInternal -- the one value that would satisfy the economy driver's RO-clone + // arm (ReadOnlyClone && ExportPolicy == CloneSourceVolumeInternal) -- specifically so that this test + // fails the moment someone copies that arm onto this driver, forcing them to confront the difference: + // on this driver, core clears a read-only clone's ExportPolicy to "" (core/orchestrator_core.go) and + // CreateClone never provisions a FlexVol for it, so there is no backend object and no recorded policy + // name to reconcile against in the first place. In practice ExportPolicy stays "" for a real RO clone + // (see UnsetExportPolicy); this fixture is a deliberately adversarial value to pin the no-arm decision, + // not a state this driver actually produces. + _, driver := newMockOntapNASDriverWithSVM(t, "SVM1") + driver.Config.AutoExportPolicy = true + driver.Config.AutoExportCIDRs = []string{"10.0.0.0/24"} volConfig := &storage.VolumeConfig{ - Name: "test-volume", - InternalName: "trident_test_volume", + InternalName: "trident_pvc_clone", + ExportPolicy: "trident_pvc_src", + ReadOnlyClone: true, + CloneSourceVolumeInternal: "trident_pvc_src", } + nodes := []*models.Node{{Name: "node-1", IPs: []string{"10.0.0.1"}}} + + err := driver.ReconcileVolumeNodeAccess(ctx, volConfig, nodes) + assert.NoError(t, err) +} +func TestOntapNASDriverReconcileVolumeNodeAccess_HappyPathMultiNode(t *testing.T) { + mockAPI, driver := newMockOntapNASDriverWithSVM(t, "SVM1") + driver.Config.AutoExportPolicy = true + driver.Config.AutoExportCIDRs = []string{"10.0.0.0/24"} + + policyName := "trident_pvc_x" + volConfig := &storage.VolumeConfig{InternalName: policyName, ExportPolicy: policyName} nodes := []*models.Node{ - { - Name: "node1", - }, - } + {Name: "node-1", IPs: []string{"10.0.0.5"}}, + {Name: "node-2", IPs: []string{"10.0.0.6"}}, + } + + // One stale, out-of-CIDR rule is already present (e.g. left over from a node that is no longer + // published); the add-only helper must leave it alone and only create the two rules the current nodes + // are missing. + mockAPI.EXPECT().ExportPolicyExists(ctx, policyName).Times(1).Return(true, nil) + mockAPI.EXPECT().ExportRuleList(ctx, policyName).Times(1).Return(map[int]string{1: "203.0.113.5"}, nil) + gomock.InOrder( + mockAPI.EXPECT().ExportRuleCreate(ctx, policyName, "10.0.0.5", gomock.Any()).Times(1).Return(nil), + mockAPI.EXPECT().ExportRuleCreate(ctx, policyName, "10.0.0.6", gomock.Any()).Times(1).Return(nil), + ) + // Pin add-only + no-assignment at the driver level: this method must never destroy a rule or (re)assign + // the policy to the volume. + mockAPI.EXPECT().ExportRuleDestroy(gomock.Any(), gomock.Any(), gomock.Any()).Times(0) + mockAPI.EXPECT().VolumeModifyExportPolicy(gomock.Any(), gomock.Any(), gomock.Any()).Times(0) err := driver.ReconcileVolumeNodeAccess(ctx, volConfig, nodes) + assert.NoError(t, err) +} - // This function returns nil in NAS driver - testing for coverage +func TestOntapNASDriverReconcileVolumeNodeAccess_PolicyMissingRecreated(t *testing.T) { + // #1180-adjacent recovery: the policy was deleted out-of-band while the volume still references it by + // name. The whole sequence is order-pinned so an inverted implementation fails this test, not just one + // that merely calls each method once. + mockAPI, driver := newMockOntapNASDriverWithSVM(t, "SVM1") + driver.Config.AutoExportPolicy = true + driver.Config.AutoExportCIDRs = []string{"10.0.0.0/24"} + + policyName := "trident_pvc_y" + volConfig := &storage.VolumeConfig{InternalName: policyName, ExportPolicy: policyName} + nodes := []*models.Node{{Name: "node-1", IPs: []string{"10.0.0.7"}}} + + gomock.InOrder( + mockAPI.EXPECT().ExportPolicyExists(ctx, policyName).Times(1).Return(false, nil), + mockAPI.EXPECT().ExportPolicyCreate(ctx, policyName).Times(1).Return(nil), + mockAPI.EXPECT().ExportRuleList(ctx, policyName).Times(1).Return(make(map[int]string), nil), + mockAPI.EXPECT().ExportRuleCreate(ctx, policyName, "10.0.0.7", gomock.Any()).Times(1).Return(nil), + ) + + err := driver.ReconcileVolumeNodeAccess(ctx, volConfig, nodes) assert.NoError(t, err) } +func TestOntapNASDriverReconcileVolumeNodeAccess_HelperErrorPropagated(t *testing.T) { + mockAPI, driver := newMockOntapNASDriverWithSVM(t, "SVM1") + driver.Config.AutoExportPolicy = true + driver.Config.AutoExportCIDRs = []string{"10.0.0.0/24"} + + policyName := "trident_pvc_z" + volConfig := &storage.VolumeConfig{InternalName: policyName, ExportPolicy: policyName} + nodes := []*models.Node{{Name: "node-1", IPs: []string{"10.0.0.8"}}} + + mockAPI.EXPECT().ExportPolicyExists(ctx, policyName).Times(1).Return(false, mockError) + mockAPI.EXPECT().ExportPolicyCreate(gomock.Any(), gomock.Any()).Times(0) + mockAPI.EXPECT().ExportRuleList(gomock.Any(), gomock.Any()).Times(0) + mockAPI.EXPECT().ExportRuleCreate(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(0) + + err := driver.ReconcileVolumeNodeAccess(ctx, volConfig, nodes) + assert.Error(t, err) + assert.Equal(t, mockError, err) +} + func TestOntapNASDriverEnablePublishEnforcement(t *testing.T) { ctx := context.Background() _, driver := newMockOntapNASDriverWithSVM(t, "SVM1")