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