Skip to content

Commit 0abce91

Browse files
torirevillavivakeram
torirevilla
and
vivakeram
authored
Export Policy Fix when VolConfig has no value
Ensure the correct export policy is used even when the volume config does not have a stored value. Co-authored-by: vivakeram <[email protected]>
1 parent cc2aaf6 commit 0abce91

File tree

7 files changed

+421
-49
lines changed

7 files changed

+421
-49
lines changed

‎storage_drivers/ontap/api/abstraction_rest.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -216,6 +216,7 @@ func (d OntapAPIREST) VolumeInfo(ctx context.Context, name string) (*Volume, err
216216
"type", "size", "comment", "aggregates", "nas", "guarantee",
217217
"snapshot_policy", "snapshot_directory_access_enabled",
218218
"space.snapshot.used", "space.snapshot.reserve_percent",
219+
"nas.export_policy.name",
219220
}
220221
volumeGetResponse, err := d.api.VolumeGetByName(ctx, name, fields)
221222
if err != nil {

‎storage_drivers/ontap/ontap_common.go‎

Lines changed: 26 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4490,21 +4490,33 @@ func deleteAutomaticSnapshot(
44904490
func removeExportPolicyRules(
44914491
ctx context.Context, exportPolicy string, publishInfo *tridentmodels.VolumePublishInfo, clientAPI api.OntapAPI,
44924492
) error {
4493+
fields := LogFields{
4494+
"Method": "removeExportPolicyRules",
4495+
"policyName": exportPolicy,
4496+
"nodeIPs": publishInfo.HostIP,
4497+
}
4498+
4499+
Logc(ctx).WithFields(fields).Debug(">>>> removeExportPolicyRules")
4500+
defer Logc(ctx).WithFields(fields).Debug("<<<< removeExportPolicyRules")
4501+
44934502
var removeRuleIndexes []int
44944503

44954504
nodeIPRules := make(map[string]struct{})
44964505
for _, ip := range publishInfo.HostIP {
4506+
ip = strings.TrimSpace(ip)
44974507
nodeIPRules[ip] = struct{}{}
44984508
}
4509+
44994510
// Get export policy rules from given policy
4500-
allExportRules, err := clientAPI.ExportRuleList(ctx, exportPolicy)
4511+
existingExportRules, err := clientAPI.ExportRuleList(ctx, exportPolicy)
45014512
if err != nil {
45024513
return err
45034514
}
4515+
Logc(ctx).WithField("existingExportRules", existingExportRules).Debug("Existing export policy rules.")
45044516

45054517
// Match list of rules to rule index based on clientMatch address
45064518
// ONTAP expects the rule index to delete
4507-
for clientMatch, ruleIndex := range allExportRules {
4519+
for clientMatch, ruleIndex := range existingExportRules {
45084520
// For the policy, match the node IP addresses to the clientMatch to remove the matched items.
45094521
// Example:
45104522
// trident_pvc_123 is attached to node1 and node2. The policy is being unpublished from node1.
@@ -4521,18 +4533,27 @@ func removeExportPolicyRules(
45214533
// index 1: "1.1.1.0, 1.1.1.1"
45224534
// index 2: "2.2.2.0, 2.2.2.2"
45234535
// For this example, index 1 will be removed.
4536+
4537+
// Add a ruleIndex for deletion only if ALL the IPs in the clientMatch are in the list of IPs we are trying
4538+
// to delete
4539+
allMatch := true
45244540
for _, singleClientMatch := range strings.Split(clientMatch, ",") {
4525-
if _, match := nodeIPRules[singleClientMatch]; match {
4526-
removeRuleIndexes = append(removeRuleIndexes, ruleIndex)
4541+
singleClientMatch = strings.TrimSpace(singleClientMatch)
4542+
if _, match := nodeIPRules[singleClientMatch]; !match {
4543+
allMatch = false
4544+
break
45274545
}
45284546
}
4547+
if allMatch {
4548+
removeRuleIndexes = append(removeRuleIndexes, ruleIndex)
4549+
}
45294550
}
45304551

45314552
// Attempt to remove node IP addresses from export policy rules
4553+
Logc(ctx).WithField("removeRuleIndexes", removeRuleIndexes).Debug("Rule indexes to remove.")
45324554
for _, ruleIndex := range removeRuleIndexes {
45334555
if err = clientAPI.ExportRuleDestroy(ctx, exportPolicy, ruleIndex); err != nil {
45344556
Logc(ctx).WithError(err).Error("Error deleting export policy rule.")
4535-
return err
45364557
}
45374558
}
45384559

‎storage_drivers/ontap/ontap_common_test.go‎

Lines changed: 205 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2569,7 +2569,7 @@ func TestEnsureNodeAccess(t *testing.T) {
25692569

25702570
assert.NoError(t, err)
25712571

2572-
// Test 2 - Test When policy doesn't exists
2572+
// Test 2 - Test When policy doesn't exist
25732573

25742574
mockAPI = mockapi.NewMockOntapAPI(mockCtrl)
25752575
mockRestClient := newMockRestClient(t)
@@ -2603,6 +2603,210 @@ func TestEnsureNodeAccess(t *testing.T) {
26032603
assert.Error(t, err)
26042604
}
26052605

2606+
func TestRemoveExportPolicyRules_Success(t *testing.T) {
2607+
ctx := context.TODO()
2608+
mockAPI := newMockOntapAPI(t)
2609+
exportPolicy := "testPolicy"
2610+
publishInfo := &tridentmodels.VolumePublishInfo{HostIP: []string{"1.1.1.1", "1.1.1.2"}}
2611+
2612+
mockAPI.EXPECT().ExportRuleList(ctx, exportPolicy).Return(map[string]int{"1.1.1.1": 1, "1.1.1.2": 2}, nil)
2613+
mockAPI.EXPECT().ExportRuleDestroy(ctx, exportPolicy, 1).Return(nil)
2614+
mockAPI.EXPECT().ExportRuleDestroy(ctx, exportPolicy, 2).Return(nil)
2615+
2616+
err := removeExportPolicyRules(ctx, exportPolicy, publishInfo, mockAPI)
2617+
assert.NoError(t, err)
2618+
}
2619+
2620+
func TestRemoveExportPolicyRules_ErrorInList(t *testing.T) {
2621+
ctx := context.TODO()
2622+
mockAPI := newMockOntapAPI(t)
2623+
exportPolicy := "testPolicy"
2624+
publishInfo := &tridentmodels.VolumePublishInfo{HostIP: []string{"1.1.1.1"}}
2625+
fakeErr := errors.New("fake error")
2626+
2627+
mockAPI.EXPECT().ExportRuleList(ctx, exportPolicy).Return(nil, fakeErr)
2628+
mockAPI.EXPECT().ExportRuleDestroy(ctx, exportPolicy, gomock.Any()).Times(0)
2629+
2630+
err := removeExportPolicyRules(ctx, exportPolicy, publishInfo, mockAPI)
2631+
assert.Error(t, err)
2632+
assert.Equal(t, fakeErr, err)
2633+
}
2634+
2635+
func TestRemoveExportPolicyRules_ErrorInFirstDestroy(t *testing.T) {
2636+
ctx := context.TODO()
2637+
mockAPI := newMockOntapAPI(t)
2638+
exportPolicy := "testPolicy"
2639+
publishInfo := &tridentmodels.VolumePublishInfo{HostIP: []string{"1.1.1.1", "2.2.2.2"}}
2640+
fakeErr := errors.New("fake destroy error")
2641+
2642+
mockAPI.EXPECT().ExportRuleList(ctx, exportPolicy).Return(map[string]int{"1.1.1.1": 1, "2.2.2.2": 2}, nil)
2643+
mockAPI.EXPECT().ExportRuleDestroy(ctx, exportPolicy, 1).Return(fakeErr)
2644+
mockAPI.EXPECT().ExportRuleDestroy(ctx, exportPolicy, 2).Return(nil)
2645+
2646+
err := removeExportPolicyRules(ctx, exportPolicy, publishInfo, mockAPI)
2647+
assert.NoError(t, err)
2648+
}
2649+
2650+
func TestRemoveExportPolicyRules_NoMatchingRules(t *testing.T) {
2651+
ctx := context.TODO()
2652+
mockAPI := newMockOntapAPI(t)
2653+
exportPolicy := "testPolicy"
2654+
publishInfo := &tridentmodels.VolumePublishInfo{HostIP: []string{"1.1.1.1"}}
2655+
2656+
mockAPI.EXPECT().ExportRuleList(ctx, exportPolicy).Return(map[string]int{"2.2.2.2": 1}, nil)
2657+
mockAPI.EXPECT().ExportRuleDestroy(ctx, exportPolicy, gomock.Any()).Times(0)
2658+
2659+
err := removeExportPolicyRules(ctx, exportPolicy, publishInfo, mockAPI)
2660+
assert.NoError(t, err)
2661+
}
2662+
2663+
func TestRemoveExportPolicyRules_AllIPsMatchInZapiRule(t *testing.T) {
2664+
ctx := context.TODO()
2665+
mockAPI := newMockOntapAPI(t)
2666+
exportPolicy := "testPolicy"
2667+
publishInfo := &tridentmodels.VolumePublishInfo{HostIP: []string{"1.1.1.2", "1.1.1.1"}}
2668+
2669+
mockAPI.EXPECT().ExportRuleList(ctx, exportPolicy).Return(map[string]int{"1.1.1.1, 1.1.1.2": 1}, nil)
2670+
mockAPI.EXPECT().ExportRuleDestroy(ctx, exportPolicy, 1).Return(nil)
2671+
2672+
err := removeExportPolicyRules(ctx, exportPolicy, publishInfo, mockAPI)
2673+
assert.NoError(t, err)
2674+
}
2675+
2676+
func TestRemoveExportPolicyRules_DuplicateIPsMatchInZapiRule(t *testing.T) {
2677+
ctx := context.TODO()
2678+
mockAPI := newMockOntapAPI(t)
2679+
exportPolicy := "testPolicy"
2680+
publishInfo := &tridentmodels.VolumePublishInfo{HostIP: []string{"1.1.1.1", "1.1.1.2"}}
2681+
2682+
mockAPI.EXPECT().ExportRuleList(ctx, exportPolicy).
2683+
Return(map[string]int{"1.1.1.1, 1.1.1.2": 1, "1.1.1.2, 1.1.1.1": 2}, nil)
2684+
mockAPI.EXPECT().ExportRuleDestroy(ctx, exportPolicy, 1).Return(nil).Times(1)
2685+
mockAPI.EXPECT().ExportRuleDestroy(ctx, exportPolicy, 2).Return(nil).Times(1)
2686+
2687+
err := removeExportPolicyRules(ctx, exportPolicy, publishInfo, mockAPI)
2688+
assert.NoError(t, err)
2689+
}
2690+
2691+
func TestRemoveExportPolicyRules_OneIPMatchInZapiRule(t *testing.T) {
2692+
ctx := context.TODO()
2693+
mockAPI := newMockOntapAPI(t)
2694+
exportPolicy := "testPolicy"
2695+
publishInfo := &tridentmodels.VolumePublishInfo{HostIP: []string{"1.1.1.2"}}
2696+
2697+
mockAPI.EXPECT().ExportRuleList(ctx, exportPolicy).Return(map[string]int{"1.1.1.1, 1.1.1.2": 1}, nil)
2698+
mockAPI.EXPECT().ExportRuleDestroy(ctx, exportPolicy, gomock.Any()).Times(0)
2699+
2700+
err := removeExportPolicyRules(ctx, exportPolicy, publishInfo, mockAPI)
2701+
assert.NoError(t, err)
2702+
}
2703+
2704+
func TestRemoveExportPolicyRules_NoIPMatchInZapiRule(t *testing.T) {
2705+
ctx := context.TODO()
2706+
mockAPI := newMockOntapAPI(t)
2707+
exportPolicy := "testPolicy"
2708+
publishInfo := &tridentmodels.VolumePublishInfo{HostIP: []string{"2.2.2.2", "2.2.2.3"}}
2709+
2710+
mockAPI.EXPECT().ExportRuleList(ctx, exportPolicy).Return(map[string]int{"1.1.1.1, 1.1.1.2": 1}, nil)
2711+
mockAPI.EXPECT().ExportRuleDestroy(ctx, exportPolicy, gomock.Any()).Times(0)
2712+
2713+
err := removeExportPolicyRules(ctx, exportPolicy, publishInfo, mockAPI)
2714+
assert.NoError(t, err)
2715+
}
2716+
2717+
func TestRemoveExportPolicyRules_EmptyHostIPList(t *testing.T) {
2718+
ctx := context.TODO()
2719+
mockAPI := newMockOntapAPI(t)
2720+
exportPolicy := "testPolicy"
2721+
publishInfo := &tridentmodels.VolumePublishInfo{HostIP: []string{}}
2722+
2723+
mockAPI.EXPECT().ExportRuleList(ctx, exportPolicy).Return(map[string]int{"1.1.1.1": 1}, nil)
2724+
mockAPI.EXPECT().ExportRuleDestroy(ctx, exportPolicy, gomock.Any()).Times(0)
2725+
2726+
err := removeExportPolicyRules(ctx, exportPolicy, publishInfo, mockAPI)
2727+
assert.NoError(t, err)
2728+
}
2729+
2730+
func TestRemoveExportPolicyRules_EmptyExportRuleList(t *testing.T) {
2731+
ctx := context.TODO()
2732+
mockAPI := newMockOntapAPI(t)
2733+
exportPolicy := "testPolicy"
2734+
publishInfo := &tridentmodels.VolumePublishInfo{HostIP: []string{"1.1.1.1"}}
2735+
2736+
mockAPI.EXPECT().ExportRuleList(ctx, exportPolicy).Return(map[string]int{}, nil)
2737+
mockAPI.EXPECT().ExportRuleDestroy(ctx, exportPolicy, gomock.Any()).Times(0)
2738+
2739+
err := removeExportPolicyRules(ctx, exportPolicy, publishInfo, mockAPI)
2740+
assert.NoError(t, err)
2741+
}
2742+
2743+
func TestRemoveExportPolicyRules_InvalidIPFormat(t *testing.T) {
2744+
ctx := context.TODO()
2745+
mockAPI := newMockOntapAPI(t)
2746+
exportPolicy := "testPolicy"
2747+
publishInfo := &tridentmodels.VolumePublishInfo{HostIP: []string{"invalidIP"}}
2748+
2749+
mockAPI.EXPECT().ExportRuleList(ctx, exportPolicy).Return(map[string]int{"1.1.1.1": 1}, nil)
2750+
mockAPI.EXPECT().ExportRuleDestroy(ctx, exportPolicy, gomock.Any()).Times(0)
2751+
2752+
err := removeExportPolicyRules(ctx, exportPolicy, publishInfo, mockAPI)
2753+
assert.NoError(t, err)
2754+
}
2755+
2756+
func TestRemoveExportPolicyRules_MixedValidAndInvalidIPs(t *testing.T) {
2757+
ctx := context.TODO()
2758+
mockAPI := newMockOntapAPI(t)
2759+
exportPolicy := "testPolicy"
2760+
publishInfo := &tridentmodels.VolumePublishInfo{HostIP: []string{"1.1.1.1", "invalidIP"}}
2761+
2762+
mockAPI.EXPECT().ExportRuleList(ctx, exportPolicy).Return(map[string]int{"1.1.1.1": 1}, nil)
2763+
mockAPI.EXPECT().ExportRuleDestroy(ctx, exportPolicy, 1).Return(nil)
2764+
2765+
err := removeExportPolicyRules(ctx, exportPolicy, publishInfo, mockAPI)
2766+
assert.NoError(t, err)
2767+
}
2768+
2769+
func TestRemoveExportPolicyRules_IPsWithSpaces(t *testing.T) {
2770+
ctx := context.TODO()
2771+
mockAPI := newMockOntapAPI(t)
2772+
exportPolicy := "testPolicy"
2773+
publishInfo := &tridentmodels.VolumePublishInfo{HostIP: []string{" 1.1.1.1 ", " 1.1.1.2 "}}
2774+
2775+
mockAPI.EXPECT().ExportRuleList(ctx, exportPolicy).Return(map[string]int{"1.1.1.1": 1, "1.1.1.2": 2}, nil)
2776+
mockAPI.EXPECT().ExportRuleDestroy(ctx, exportPolicy, 1).Return(nil)
2777+
mockAPI.EXPECT().ExportRuleDestroy(ctx, exportPolicy, 2).Return(nil)
2778+
2779+
err := removeExportPolicyRules(ctx, exportPolicy, publishInfo, mockAPI)
2780+
assert.NoError(t, err)
2781+
}
2782+
2783+
func TestRemoveExportPolicyRules_IPsWithMixedFormats(t *testing.T) {
2784+
ctx := context.TODO()
2785+
mockAPI := newMockOntapAPI(t)
2786+
exportPolicy := "testPolicy"
2787+
publishInfo := &tridentmodels.VolumePublishInfo{HostIP: []string{"1.1.1.1", "2001:db8::1"}}
2788+
2789+
mockAPI.EXPECT().ExportRuleList(ctx, exportPolicy).Return(map[string]int{"1.1.1.1": 1, "2001:db8::1": 2}, nil)
2790+
mockAPI.EXPECT().ExportRuleDestroy(ctx, exportPolicy, 1).Return(nil)
2791+
mockAPI.EXPECT().ExportRuleDestroy(ctx, exportPolicy, 2).Return(nil)
2792+
2793+
err := removeExportPolicyRules(ctx, exportPolicy, publishInfo, mockAPI)
2794+
assert.NoError(t, err)
2795+
}
2796+
2797+
func TestRemoveExportPolicyRules_IPsMatchInMixedFormats(t *testing.T) {
2798+
ctx := context.TODO()
2799+
mockAPI := newMockOntapAPI(t)
2800+
exportPolicy := "testPolicy"
2801+
publishInfo := &tridentmodels.VolumePublishInfo{HostIP: []string{"1.1.1.1"}}
2802+
2803+
mockAPI.EXPECT().ExportRuleList(ctx, exportPolicy).Return(map[string]int{"::ffff:1.1.1.1": 1}, nil)
2804+
mockAPI.EXPECT().ExportRuleDestroy(ctx, exportPolicy, gomock.Any()).Times(0)
2805+
2806+
err := removeExportPolicyRules(ctx, exportPolicy, publishInfo, mockAPI)
2807+
assert.NoError(t, err)
2808+
}
2809+
26062810
func TestDeleteExportPolicy(t *testing.T) {
26072811
// Test-1: Positive flow
26082812

‎storage_drivers/ontap/ontap_nas.go‎

Lines changed: 35 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -826,16 +826,21 @@ func (d *NASStorageDriver) Unpublish(
826826
return nil
827827
}
828828

829-
exists, err := d.API.VolumeExists(ctx, volConfig.InternalName)
829+
volume, err := d.API.VolumeInfo(ctx, volConfig.InternalName)
830830
if err != nil {
831831
Logc(ctx).WithError(err).Error("Error checking for existing volume.")
832832
return err
833833
}
834-
if !exists {
834+
if volume == nil {
835835
Logc(ctx).WithField("volume", volConfig.InternalName).Debug("Volume not found.")
836836
return errors.NotFoundError("volume not found")
837837
}
838838

839+
// Trident versions <23.04 may not have the exportPolicy field set in the volume config,
840+
// if this is the case we need to use the export policy from the volume info from ONTAP.
841+
if volConfig.ExportPolicy == "" {
842+
volConfig.ExportPolicy = volume.ExportPolicy
843+
}
839844
exportPolicy := volConfig.ExportPolicy
840845
if exportPolicy == volExportPolicyName {
841846
// Remove export policy rules matching the node IP address from the volume level policy
@@ -864,8 +869,8 @@ func (d *NASStorageDriver) Unpublish(
864869
}
865870
}
866871

867-
} else if exportPolicy == getExportPolicyName(publishInfo.BackendUUID) {
868-
// volume using backend-based export policy.
872+
} else {
873+
// Volume is using a backend-based policy or migrating to using autoExportPolicies.
869874
if len(publishInfo.Nodes) == 0 {
870875
if err = d.setVolToEmptyPolicy(ctx, volConfig.InternalName); err != nil {
871876
return err
@@ -929,11 +934,34 @@ func (d *NASStorageDriver) publishFlexVolShare(
929934
}
930935

931936
// Ensure the flexVol has the correct export policy and rules applied.
937+
backendPolicyName := getExportPolicyName(publishInfo.BackendUUID)
938+
flexVolPolicyName := flexvol
939+
// Trident versions <23.04 may not have the exportPolicy field set in the volume config,
940+
// if this is the case we need to use the export policy from the volume info from ONTAP.
941+
if volConfig.ExportPolicy != backendPolicyName && volConfig.ExportPolicy != flexvol &&
942+
volConfig.ExportPolicy != getEmptyExportPolicyName(*d.Config.StoragePrefix) {
943+
volume, err := d.API.VolumeInfo(ctx, flexvol)
944+
if err != nil {
945+
return err
946+
}
947+
if volume == nil {
948+
return errors.NotFoundError("volume not found")
949+
}
950+
volConfig.ExportPolicy = volume.ExportPolicy
951+
}
952+
932953
// If the flexVol already have backend policy, leave it as it is. We will have an opportunity to migrate it
933954
// to flexVol policy during unpublish.
934-
flexVolPolicyName := flexvol
935-
if volConfig.ExportPolicy == getExportPolicyName(publishInfo.BackendUUID) {
936-
flexVolPolicyName = volConfig.ExportPolicy
955+
switch volConfig.ExportPolicy {
956+
case getEmptyExportPolicyName(*d.Config.StoragePrefix), flexvol:
957+
flexVolPolicyName = flexvol
958+
case backendPolicyName:
959+
flexVolPolicyName = backendPolicyName
960+
default:
961+
// This can happen if the customer switched from autoExportPolicy=false to true and the volume is still using
962+
// default or user supplied export policy.
963+
Logc(ctx).Debugf("Export policy %s is not managed by Trident.", volConfig.ExportPolicy)
964+
return nil
937965
}
938966

939967
volConfig.ExportPolicy = flexVolPolicyName

‎storage_drivers/ontap/ontap_nas_qtree.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -866,8 +866,8 @@ func (d *NASQtreeStorageDriver) Unpublish(
866866
}
867867
}
868868

869-
} else if exportPolicy == getExportPolicyName(publishInfo.BackendUUID) {
870-
// Qtree using backend-based export policy.
869+
} else {
870+
// Qtree is using a backend-based policy or migrating to using autoExportPolicies.
871871
if len(publishInfo.Nodes) == 0 {
872872
if err = d.setQtreeToEmptyPolicy(ctx, qtreeName, qtree.Volume); err != nil {
873873
return err

0 commit comments

Comments
 (0)