Skip to content

Commit 593b074

Browse files
alloydsaalloydsa
authored
Secure SMB implementation for Ontap-Nas Clone
1 parent 5a21be5 commit 593b074

File tree

9 files changed

+414
-17
lines changed

9 files changed

+414
-17
lines changed

‎core/orchestrator_core.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2370,6 +2370,11 @@ func (o *TridentOrchestrator) cloneVolumeInitial(
23702370
if volumeConfig.SplitOnClone != "" {
23712371
cloneConfig.SplitOnClone = volumeConfig.SplitOnClone
23722372
}
2373+
// Override this value only if SecureSMB is enabled in clone volume's config
2374+
if volumeConfig.SecureSMBEnabled {
2375+
cloneConfig.SecureSMBEnabled = volumeConfig.SecureSMBEnabled
2376+
cloneConfig.SMBShareACL = volumeConfig.SMBShareACL
2377+
}
23732378
cloneConfig.ReadOnlyClone = volumeConfig.ReadOnlyClone
23742379
cloneConfig.Namespace = volumeConfig.Namespace
23752380
cloneConfig.RequestName = volumeConfig.RequestName

‎storage/backend.go‎

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -791,13 +791,10 @@ func (b *StorageBackend) RemoveVolume(ctx context.Context, volConfig *VolumeConf
791791
return err
792792
}
793793

794-
// If it's a RO clone, no need to delete the volume in the driver
795-
if !volConfig.ReadOnlyClone {
796-
if err := b.driver.Destroy(ctx, volConfig); err != nil {
797-
// TODO: Check the error being returned once the nDVP throws errors
798-
// for volumes that aren't found.
799-
return err
800-
}
794+
if err := b.driver.Destroy(ctx, volConfig); err != nil {
795+
// TODO: Check the error being returned once the nDVP throws errors
796+
// for volumes that aren't found.
797+
return err
801798
}
802799

803800
b.RemoveCachedVolume(volConfig.Name)

‎storage_drivers/azure/azure_anf.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1553,6 +1553,12 @@ func (d *NASStorageDriver) Destroy(ctx context.Context, volConfig *storage.Volum
15531553
"Type": "NASStorageDriver",
15541554
"name": name,
15551555
}
1556+
1557+
// If it's a RO clone, no need to delete the volume
1558+
if volConfig.ReadOnlyClone {
1559+
return nil
1560+
}
1561+
15561562
Logd(ctx, d.Name(), d.Config.DebugTraceFlags["method"]).WithFields(fields).Trace(">>>> Destroy")
15571563
defer Logd(ctx, d.Name(), d.Config.DebugTraceFlags["method"]).WithFields(fields).Trace("<<<< Destroy")
15581564

‎storage_drivers/gcp/gcp_gcnv.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1340,6 +1340,12 @@ func (d *NASStorageDriver) Destroy(ctx context.Context, volConfig *storage.Volum
13401340
"Type": "NASStorageDriver",
13411341
"name": name,
13421342
}
1343+
1344+
// If it's a RO clone, no need to delete the volume
1345+
if volConfig.ReadOnlyClone {
1346+
return nil
1347+
}
1348+
13431349
Logd(ctx, d.Name(), d.Config.DebugTraceFlags["method"]).WithFields(fields).Trace(">>>> Destroy")
13441350
defer Logd(ctx, d.Name(), d.Config.DebugTraceFlags["method"]).WithFields(fields).Trace("<<<< Destroy")
13451351

‎storage_drivers/ontap/ontap_common.go‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4336,8 +4336,13 @@ func ConstructOntapNASVolumeAccessPath(
43364336
}
43374337

43384338
if volConfig.ReadOnlyClone {
4339-
completeVolumePath = fmt.Sprintf("%s\\%s\\%s\\%s", smbSharePath, volConfig.CloneSourceVolumeInternal,
4340-
"~snapshot", volConfig.CloneSourceSnapshot)
4339+
if volConfig.SecureSMBEnabled {
4340+
completeVolumePath = fmt.Sprintf("%s\\%s\\%s\\%s", smbSharePath, volConfig.InternalName, "~snapshot",
4341+
volConfig.CloneSourceSnapshot)
4342+
} else {
4343+
completeVolumePath = fmt.Sprintf("%s\\%s\\%s\\%s", smbSharePath, volConfig.CloneSourceVolumeInternal,
4344+
"~snapshot", volConfig.CloneSourceSnapshot)
4345+
}
43414346
} else {
43424347
// If the user does not specify an SMB Share, Trident creates it with the same name as the flexvol volume name.
43434348
completeVolumePath = smbSharePath + volumeName

‎storage_drivers/ontap/ontap_common_test.go‎

Lines changed: 33 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2405,7 +2405,7 @@ func TestConstructOntapNASVolumeAccessPath_SecureSMBDisabled(t *testing.T) {
24052405
for _, test := range tests {
24062406
t.Run(test.smbShare, func(t *testing.T) {
24072407
result := ConstructOntapNASVolumeAccessPath(ctx, test.smbShare, test.volName, volConfig, test.protocol)
2408-
assert.Equal(t, test.expectedPath, result, "unable to construct Ontap-NAS volume access path")
2408+
assert.Equal(t, test.expectedPath, result, "the constructed Ontap-NAS volume access path is incorrect")
24092409
})
24102410
}
24112411
}
@@ -2431,7 +2431,7 @@ func TestConstructOntapNASVolumeAccessPath_SecureSMBEnabled(t *testing.T) {
24312431
for _, test := range tests {
24322432
t.Run(test.smbShare, func(t *testing.T) {
24332433
result := ConstructOntapNASVolumeAccessPath(ctx, test.smbShare, test.volName, volConfig, test.protocol)
2434-
assert.Equal(t, test.expectedPath, result, "unable to construct Ontap-NAS volume access path")
2434+
assert.Equal(t, test.expectedPath, result, "the constructed Ontap-NAS volume access path is incorrect")
24352435
})
24362436
}
24372437
}
@@ -2459,7 +2459,35 @@ func TestConstructOntapNASVolumeAccessPath_ROClone(t *testing.T) {
24592459
for _, test := range tests {
24602460
t.Run(test.smbShare, func(t *testing.T) {
24612461
result := ConstructOntapNASVolumeAccessPath(ctx, test.smbShare, "/vol", volConfig, test.protocol)
2462-
assert.Equal(t, test.expectedPath, result, "unable to construct Ontap-NAS volume access path")
2462+
assert.Equal(t, test.expectedPath, result, "the constructed Ontap-NAS volume access path is incorrect")
2463+
})
2464+
}
2465+
}
2466+
2467+
func TestConstructOntapNASVolumeAccessPath_ROCloneSecureSMBEnabled(t *testing.T) {
2468+
ctx := context.Background()
2469+
2470+
volConfig := &storage.VolumeConfig{
2471+
InternalName: "vol",
2472+
ReadOnlyClone: true,
2473+
CloneSourceVolumeInternal: "sourceVol",
2474+
CloneSourceSnapshot: "snapshot-abcd-1234-wxyz",
2475+
SecureSMBEnabled: true,
2476+
}
2477+
2478+
tests := []struct {
2479+
smbShare string
2480+
protocol string
2481+
expectedPath string
2482+
}{
2483+
{"test_share", "smb", "\\vol\\~snapshot\\snapshot-abcd-1234-wxyz"},
2484+
{"", "smb", "\\vol\\~snapshot\\snapshot-abcd-1234-wxyz"},
2485+
}
2486+
2487+
for _, test := range tests {
2488+
t.Run(test.smbShare, func(t *testing.T) {
2489+
result := ConstructOntapNASVolumeAccessPath(ctx, test.smbShare, "/vol", volConfig, test.protocol)
2490+
assert.Equal(t, test.expectedPath, result, "the constructed Ontap-NAS volume access path is incorrect")
24632491
})
24642492
}
24652493
}
@@ -2478,7 +2506,7 @@ func TestConstructOntapNASFlexGroupSMBVolumePath(t *testing.T) {
24782506
for _, test := range tests {
24792507
t.Run(test.smbShare, func(t *testing.T) {
24802508
result := ConstructOntapNASFlexGroupSMBVolumePath(ctx, test.smbShare, "vol")
2481-
assert.Equal(t, test.expectedPath, result, "unable to construct Ontap-NAS-QTree SMB volume path")
2509+
assert.Equal(t, test.expectedPath, result, "the constructed Ontap-NAS-QTree SMB volume access path is incorrect")
24822510
})
24832511
}
24842512
}
@@ -2576,7 +2604,7 @@ func TestConstructOntapNASQTreeVolumePath(t *testing.T) {
25762604
for _, test := range tests {
25772605
t.Run(test.smbShare, func(t *testing.T) {
25782606
result := ConstructOntapNASQTreeVolumePath(ctx, test.smbShare, "flex-vol", test.volConfig, test.protocol)
2579-
assert.Equal(t, test.expectedPath, result, "unable to construct Ontap-NAS-QTree SMB volume path")
2607+
assert.Equal(t, test.expectedPath, result, "the constructed Ontap-NAS-QTree SMB volume path is incorrect")
25802608
})
25812609
}
25822610
}

‎storage_drivers/ontap/ontap_nas.go‎

Lines changed: 38 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -497,8 +497,9 @@ func (d *NASStorageDriver) CreateClone(
497497
// If we still don't have splitOnClone value then HERE we check for value in the source PV's Storage/Virtual Pool
498498
// If the value for "splitOnClone" is still empty then HERE we set it to backend config's SplitOnClone value
499499

500-
// Attempt to get splitOnClone value based on storagePool (source Volume's StoragePool)
500+
// Attempt to get splitOnClone value and adAdminUser value based on storagePool (source Volume's StoragePool)
501501
var storagePoolSplitOnCloneVal string
502+
var adAdminUser string
502503

503504
var labelErr error
504505
if storage.IsStoragePoolUnset(storagePool) {
@@ -511,22 +512,46 @@ func (d *NASStorageDriver) CreateClone(
511512
if labelErr != nil {
512513
return labelErr
513514
}
515+
if d.Config.NASType == sa.SMB {
516+
adAdminUser = d.Config.ADAdminUser
517+
}
514518
} else {
515519
if labels, labelErr = ConstructLabelsFromConfigs(ctx, storagePool, cloneVolConfig,
516520
d.Config.CommonStorageDriverConfig, api.MaxNASLabelLength); labelErr != nil {
517521
return labelErr
518522
}
519523
storagePoolSplitOnCloneVal = storagePool.InternalAttributes()[SplitOnClone]
524+
525+
if d.Config.NASType == sa.SMB {
526+
// Update the clone volume config with the admin user details from the storage pool
527+
adAdminUser = storagePool.InternalAttributes()[ADAdminUser]
528+
}
529+
}
530+
531+
if d.Config.NASType == sa.SMB {
532+
if adAdminUser != "" {
533+
if _, exists := cloneVolConfig.SMBShareACL[adAdminUser]; !exists {
534+
cloneVolConfig.SMBShareACL[adAdminUser] = ADAdminUserPermission
535+
}
536+
}
520537
}
521538

522539
if cloneVolConfig.ReadOnlyClone {
523540
if flexvol.SnapshotDir == nil {
524541
return fmt.Errorf("snapshot directory access is undefined on flexvol %s", flexvol.Name)
525542
}
543+
526544
if *flexvol.SnapshotDir == false {
527545
return fmt.Errorf("snapshot directory access is set to %t and readOnly clone is set to %t ",
528546
*flexvol.SnapshotDir, cloneVolConfig.ReadOnlyClone)
529547
}
548+
549+
if d.Config.NASType == sa.SMB && cloneVolConfig.SecureSMBEnabled {
550+
if err := d.EnsureSMBShare(ctx, cloneVolConfig.InternalName, "/"+cloneVolConfig.CloneSourceVolumeInternal,
551+
cloneVolConfig.SMBShareACL, cloneVolConfig.SecureSMBEnabled); err != nil {
552+
return err
553+
}
554+
}
530555
return nil
531556
}
532557

@@ -577,9 +602,9 @@ func (d *NASStorageDriver) CreateClone(
577602

578603
cloneVolConfig.ExportPolicy = exportPolicy
579604

580-
// TODO: enable secure SMB share for clone volumes
581605
if d.Config.NASType == sa.SMB {
582-
if err := d.EnsureSMBShare(ctx, cloneVolConfig.InternalName, "/"+cloneVolConfig.InternalName, cloneVolConfig.SMBShareACL, false); err != nil {
606+
if err := d.EnsureSMBShare(ctx, cloneVolConfig.InternalName, "/"+cloneVolConfig.InternalName,
607+
cloneVolConfig.SMBShareACL, cloneVolConfig.SecureSMBEnabled); err != nil {
583608
return err
584609
}
585610
}
@@ -598,6 +623,16 @@ func (d *NASStorageDriver) Destroy(ctx context.Context, volConfig *storage.Volum
598623
Logd(ctx, d.Name(), d.Config.DebugTraceFlags["method"]).WithFields(fields).Trace(">>>> Destroy")
599624
defer Logd(ctx, d.Name(), d.Config.DebugTraceFlags["method"]).WithFields(fields).Trace("<<<< Destroy")
600625

626+
// Handle ReadOnlyClone case early
627+
if volConfig.ReadOnlyClone {
628+
if d.Config.NASType == sa.SMB && volConfig.SecureSMBEnabled {
629+
if err := d.DestroySMBShare(ctx, name); err != nil {
630+
return err
631+
}
632+
}
633+
return nil // Return early for all ReadOnlyClone cases
634+
}
635+
601636
// TODO: If this is the parent of one or more clones, those clones have to split from this
602637
// volume before it can be deleted, which means separate copies of those volumes.
603638
// If there are a lot of clones on this volume, that could seriously balloon the amount of

‎storage_drivers/ontap/ontap_nas_qtree.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -574,6 +574,12 @@ func (d *NASQtreeStorageDriver) Destroy(ctx context.Context, volConfig *storage.
574574
"Type": "NASQtreeStorageDriver",
575575
"name": name,
576576
}
577+
578+
// If it's a RO clone, no need to delete the volume
579+
if volConfig.ReadOnlyClone {
580+
return nil
581+
}
582+
577583
Logd(ctx, d.Name(), d.Config.DebugTraceFlags["method"]).WithFields(fields).Trace(">>>> Destroy")
578584
defer Logd(ctx, d.Name(), d.Config.DebugTraceFlags["method"]).WithFields(fields).Trace("<<<< Destroy")
579585

0 commit comments

Comments
 (0)