From 697f860c32fefbc9a0adf8f16444229d6c495b2a Mon Sep 17 00:00:00 2001 From: "David A. Bradley" Date: Tue, 2 Jan 2024 08:55:35 -0500 Subject: [PATCH] Skip subdirectory creation for invalid volume ID The only reasonable way that the unpublish command can determine whether to keep or delete the subdirectory, if given, is to bake this information into the volume ID, as the original volume context is not passed to volume unpublish calls. For dynamically created volumes, this is always consistent. However, if the end user defines their own volumes, but does not follow the expected volume handle / volume ID form, that information is not available. In this instance, if the user sets the retain-sub-dir value on a manually created volume to 'false', we will error out and refuse to create the volume. If they request that the subdirectory be retained, we will treat the volume as we do a read-only volume, where we will attempt to mount the given subdirectory, but will not do any logic associated with creating or deleting that subdirectory. As with a read-only volume, if that subdirectory does not exist on the cluster, the mount will fail with a reasonable error. --- docs/examples/pv_subdir.yaml | 5 ++ pkg/azurelustre/nodeserver.go | 82 +++++++++++-------------- pkg/azurelustre/nodeserver_test.go | 97 ++++++++++++++++++++++++------ 3 files changed, 119 insertions(+), 65 deletions(-) diff --git a/docs/examples/pv_subdir.yaml b/docs/examples/pv_subdir.yaml index 47e138fee..075b01295 100644 --- a/docs/examples/pv_subdir.yaml +++ b/docs/examples/pv_subdir.yaml @@ -27,6 +27,11 @@ spec: # This parameter is required if "sub-dir" is provided retain-sub-dir: "false" # Make sure this VolumeID is unique in the cluster + # volumeHandle format: {volume-name}#{fs-name}#{mgs-ip-address}#{sub-dir}#{retain-sub-dir} + # Example: vol_1#lustrefs#0.0.0.0#example_sub_dir#true + # If the value does not match this format, the driver will not attempt to create + # the configured subdirectory. Further, if retain-sub-dir is 'false' in that + # case, any attempt to mount this volume will fail. volumeHandle: ${UNIQUE_IDENTIFIER_VOLUME_ID} # "Delete" is not supported in static provisioning PV mountOptions: diff --git a/pkg/azurelustre/nodeserver.go b/pkg/azurelustre/nodeserver.go index 4eebfd726..b0f856564 100644 --- a/pkg/azurelustre/nodeserver.go +++ b/pkg/azurelustre/nodeserver.go @@ -73,12 +73,27 @@ func (d *Driver) NodePublishVolume( "Volume context must be provided") } - vol, err := newLustreVolume(volumeID, context) + volName := "" + skipCreateForInvalidVolumeID := false + + volFromID, err := getLustreVolFromID(volumeID) + if err != nil { + klog.Warningf("error parsing volume ID '%v'", err) + // If we can't parse the volume ID for the information now, we won't be able to + // do so while unpublishing to know whether to delete it, so skip it + skipCreateForInvalidVolumeID = true + } else { + volName = volFromID.name + } + + vol, err := newLustreVolume(volumeID, volName, context) if err != nil { return nil, err } - vol.id = volumeID + if volFromID != nil && *volFromID != *vol { + klog.Warningf("volume context does not match values in volume ID for volume %q", volumeID) + } subDirReplaceMap := map[string]string{} @@ -136,11 +151,18 @@ func (d *Driver) NodePublishVolume( } if len(vol.subDir) > 0 && !d.enableAzureLustreMockMount { - if readOnly && !vol.retainSubDir { - return nil, status.Error( - codes.InvalidArgument, - "Context retain-sub-dir must be true for a sub-dir on a read-only volume", - ) + if !vol.retainSubDir { + if readOnly { + return nil, status.Error( + codes.InvalidArgument, + "Context retain-sub-dir must be true for a sub-dir on a read-only volume", + ) + } else if skipCreateForInvalidVolumeID { + return nil, status.Error( + codes.InvalidArgument, + "Context retain-sub-dir must be true for volume with invalid ID", + ) + } } interpolatedSubDir := replaceWithMap(vol.subDir, subDirReplaceMap) @@ -154,6 +176,8 @@ func (d *Driver) NodePublishVolume( if readOnly { klog.V(2).Info("NodePublishVolume: not attempting to create sub-dir on read-only volume, assuming existing path") + } else if skipCreateForInvalidVolumeID { + klog.V(2).Info("NodePublishVolume: not attempting to create sub-dir for invalid volume ID, assuming existing path") } else { klog.V(2).Infof( "NodePublishVolume: sub-dir will be created at %q", @@ -285,7 +309,7 @@ func (d *Driver) NodeUnpublishVolume( vol, err := getLustreVolFromID(volumeID) if err != nil { - klog.V(2).Infof("failed to parse volume id %q for sub-dir cleanup, skipping", volumeID) + klog.V(2).Infof("failed to parse volume ID %q for sub-dir cleanup, skipping", volumeID) } else if len(vol.subDir) > 0 && !vol.retainSubDir { cleanupSubDir = true sourceRoot = getSourceString(vol.mgsIPAddress, vol.azureLustreName) @@ -762,7 +786,7 @@ type lustreVolume struct { func getLustreVolFromID(id string) (*lustreVolume, error) { segments := strings.Split(id, separator) if len(segments) < 3 { - return nil, fmt.Errorf("could not split volume id %q into lustre name and ip address", id) + return nil, fmt.Errorf("could not split volume ID %q into lustre name and ip address", id) } name := segments[0] @@ -793,11 +817,10 @@ func getLustreVolFromID(id string) (*lustreVolume, error) { } // Convert context parameters to a lustreVolume -func newLustreVolume(volumeID string, params map[string]string) (*lustreVolume, error) { +func newLustreVolume(volumeID string, volumeName string, params map[string]string) (*lustreVolume, error) { var mgsIPAddress, azureLustreName, subDir string // Shouldn't attempt to delete anything unless sub-dir is actually specified retainSubDir := true - subDirReplaceMap := map[string]string{} // validate parameters (case-insensitive). for k, v := range params { switch strings.ToLower(k) { @@ -839,20 +862,6 @@ func newLustreVolume(volumeID string, params map[string]string) (*lustreVolume, "Context sub-dir must be provided when retain-sub-dir is provided", ) } - case podNameKey: - subDirReplaceMap[podNameMetadata] = v - case podNamespaceKey: - subDirReplaceMap[podNamespaceMetadata] = v - case podUIDKey: - subDirReplaceMap[podUIDMetadata] = v - case serviceAccountNameKey: - subDirReplaceMap[serviceAccountNameMetadata] = v - case pvcNamespaceKey: - subDirReplaceMap[pvcNamespaceMetadata] = v - case pvcNameKey: - subDirReplaceMap[pvcNameMetadata] = v - case pvNameKey: - subDirReplaceMap[pvNameMetadata] = v } } @@ -871,32 +880,13 @@ func newLustreVolume(volumeID string, params map[string]string) (*lustreVolume, ) } - volName := "" - - volFromID, err := getLustreVolFromID(volumeID) - if err != nil { - klog.Warningf("error parsing volume id '%v'", err) - } else { - volName = volFromID.name - } - vol := &lustreVolume{ - name: volName, + name: volumeName, mgsIPAddress: mgsIPAddress, azureLustreName: azureLustreName, subDir: subDir, retainSubDir: retainSubDir, - id: fmt.Sprintf( - volumeIDTemplate, - volName, - azureLustreName, - mgsIPAddress, - subDir, - retainSubDir), - } - - if volFromID != nil && *volFromID != *vol { - klog.Warningf("volume context does not match values in volume id for volume %q", volumeID) + id: volumeID, } return vol, nil diff --git a/pkg/azurelustre/nodeserver_test.go b/pkg/azurelustre/nodeserver_test.go index 539b0d855..3cea7b0ea 100644 --- a/pkg/azurelustre/nodeserver_test.go +++ b/pkg/azurelustre/nodeserver_test.go @@ -347,6 +347,38 @@ func TestNodePublishVolume(t *testing.T) { {Action: "mount", Target: "target_test", Source: "1.1.1.1@tcp:/lustrefs/testSubDir", FSType: "lustre"}, }, }, + { + desc: "Unexpected volume ID skips sub-dir creation", + req: csi.NodePublishVolumeRequest{ + VolumeCapability: &csi.VolumeCapability{AccessMode: &volumeCap, AccessType: &csi.VolumeCapability_Mount{ + Mount: &csi.VolumeCapability_MountVolume{MountFlags: []string{"noatime", "flock"}}, + }}, + VolumeId: "vol_1", + TargetPath: targetTest, + VolumeContext: map[string]string{"mgs-ip-address": "1.1.1.1", "fs-name": "lustrefs", "sub-dir": subDir, "retain-sub-dir": "true"}, + Readonly: false, + }, + expectedErr: nil, + expectedMountpoints: []mount.MountPoint{{Device: "1.1.1.1@tcp:/lustrefs/testSubDir", Path: "target_test", Type: "lustre", Opts: []string{"noatime", "flock"}}}, + expectedMountActions: []mount.FakeAction{ + {Action: "mount", Target: "target_test", Source: "1.1.1.1@tcp:/lustrefs/testSubDir", FSType: "lustre"}, + }, + }, + { + desc: "Unexpected volume ID fails if retain-sub-dir is not true", + req: csi.NodePublishVolumeRequest{ + VolumeCapability: &csi.VolumeCapability{AccessMode: &volumeCap, AccessType: &csi.VolumeCapability_Mount{ + Mount: &csi.VolumeCapability_MountVolume{MountFlags: []string{"noatime", "flock"}}, + }}, + VolumeId: "vol_1", + TargetPath: targetTest, + VolumeContext: map[string]string{"mgs-ip-address": "1.1.1.1", "fs-name": "lustrefs", "sub-dir": subDir, "retain-sub-dir": "false"}, + Readonly: false, + }, + expectedErr: status.Error(codes.InvalidArgument, "Context retain-sub-dir must be true for volume with invalid ID"), + expectedMountpoints: nil, + expectedMountActions: []mount.FakeAction{}, + }, { desc: "Valid mount options with slashes in paths", req: csi.NodePublishVolumeRequest{ @@ -776,6 +808,24 @@ func TestNodeUnpublishVolume(t *testing.T) { {Action: "unmount", Target: workingMountDir + "/target_test", Source: "", FSType: ""}, }, }, + { + desc: "Valid request with unexpected ID skips cleanup", + setup: func(d *Driver) { + err = makeDir(targetTest) + assert.NoError(t, err) + err = makeDir(filepath.Join(workingMountDir, targetTest, subDir)) + assert.NoError(t, err) + err = d.mounter.Mount("1.1.1.1@tcp:/lustrefs/"+subDir, targetTest, "lustre", []string{"noatime", "flock"}) + assert.NoError(t, err) + }, + req: csi.NodeUnpublishVolumeRequest{TargetPath: targetTest, VolumeId: "vol_1"}, + expectedErr: nil, + expectExistingSubDir: true, + expectedMountpoints: []mount.MountPoint{}, + expectedMountActions: []mount.FakeAction{ + {Action: "unmount", Target: "target_test", Source: "", FSType: ""}, + }, + }, } // Setup @@ -1030,7 +1080,7 @@ func TestGetLustreVolFromID(t *testing.T) { desc: "incorrect volume id", volumeID: "vol_1", expectedLustreVolume: nil, - expectedErr: errors.New("could not split volume id \"vol_1\" into lustre name and ip address"), + expectedErr: errors.New("could not split volume ID \"vol_1\" into lustre name and ip address"), }, { desc: "incorrect retain-sub-dir", @@ -1153,13 +1203,15 @@ func TestNewLustreVolume(t *testing.T) { cases := []struct { desc string id string + volName string params map[string]string expectedLustreVolume *lustreVolume expectedErr error }{ { - desc: "valid context, no sub-dir", - id: "vol_1#lustrefs#1.1.1.1##true", + desc: "valid context, no sub-dir", + id: "vol_1#lustrefs#1.1.1.1##true", + volName: "vol_1", params: map[string]string{ "mgs-ip-address": "1.1.1.1", "fs-name": "lustrefs", @@ -1174,8 +1226,9 @@ func TestNewLustreVolume(t *testing.T) { }, }, { - desc: "valid context with sub-dir", - id: "vol_1#lustrefs#1.1.1.1#testSubDir#false", + desc: "valid context with sub-dir", + id: "vol_1#lustrefs#1.1.1.1#testSubDir#false", + volName: "vol_1", params: map[string]string{ "mgs-ip-address": "1.1.1.1", "fs-name": "lustrefs", @@ -1192,8 +1245,9 @@ func TestNewLustreVolume(t *testing.T) { }, }, { - desc: "invalid parameter is ignored", - id: "vol_1#lustrefs#1.1.1.1##true", + desc: "invalid parameter is ignored", + id: "vol_1#lustrefs#1.1.1.1##true", + volName: "vol_1", params: map[string]string{ "mgs-ip-address": "1.1.1.1", "fs-name": "lustrefs", @@ -1210,8 +1264,9 @@ func TestNewLustreVolume(t *testing.T) { expectedErr: nil, }, { - desc: "incorrect volume id is ignored for context", - id: "vol_1#otherfs#2.2.2.2#otherSubDir#false", + desc: "incorrect volume id is ignored for context", + id: "vol_1#otherfs#2.2.2.2#otherSubDir#false", + volName: "vol_1", params: map[string]string{ "mgs-ip-address": "1.1.1.1", "fs-name": "lustrefs", @@ -1219,7 +1274,7 @@ func TestNewLustreVolume(t *testing.T) { "retain-sub-dir": "true", }, expectedLustreVolume: &lustreVolume{ - id: "vol_1#lustrefs#1.1.1.1#testSubDir#true", + id: "vol_1#otherfs#2.2.2.2#otherSubDir#false", name: "vol_1", azureLustreName: "lustrefs", mgsIPAddress: "1.1.1.1", @@ -1229,8 +1284,9 @@ func TestNewLustreVolume(t *testing.T) { expectedErr: nil, }, { - desc: "sub-dir cannot be empty", - id: "vol_1#lustrefs#1.1.1.1##false", + desc: "sub-dir cannot be empty", + id: "vol_1#lustrefs#1.1.1.1##false", + volName: "vol_1", params: map[string]string{ "mgs-ip-address": "1.1.1.1", "fs-name": "lustrefs", @@ -1240,8 +1296,9 @@ func TestNewLustreVolume(t *testing.T) { expectedErr: status.Error(codes.InvalidArgument, "Context sub-dir must not be empty or root if provided"), }, { - desc: "sub-dir cannot be root", - id: "vol_1#lustrefs#1.1.1.1#/#false", + desc: "sub-dir cannot be root", + id: "vol_1#lustrefs#1.1.1.1#/#false", + volName: "vol_1", params: map[string]string{ "mgs-ip-address": "1.1.1.1", "fs-name": "lustrefs", @@ -1251,8 +1308,9 @@ func TestNewLustreVolume(t *testing.T) { expectedErr: status.Error(codes.InvalidArgument, "Context sub-dir must not be empty or root if provided"), }, { - desc: "must have retain-sub-dir with sub-dir", - id: "vol_1#lustrefs#1.1.1.1#testSubDir#", + desc: "must have retain-sub-dir with sub-dir", + id: "vol_1#lustrefs#1.1.1.1#testSubDir#", + volName: "vol_1", params: map[string]string{ "mgs-ip-address": "1.1.1.1", "fs-name": "lustrefs", @@ -1261,8 +1319,9 @@ func TestNewLustreVolume(t *testing.T) { expectedErr: status.Error(codes.InvalidArgument, "Context retain-sub-dir must be provided when sub-dir is provided"), }, { - desc: "sub-dir cannot be root", - id: "vol_1#lustrefs#1.1.1.1##false", + desc: "sub-dir cannot be root", + id: "vol_1#lustrefs#1.1.1.1##false", + volName: "vol_1", params: map[string]string{ "mgs-ip-address": "1.1.1.1", "fs-name": "lustrefs", @@ -1275,7 +1334,7 @@ func TestNewLustreVolume(t *testing.T) { for _, test := range cases { test := test // pin t.Run(test.desc, func(t *testing.T) { - vol, err := newLustreVolume(test.id, test.params) + vol, err := newLustreVolume(test.id, test.volName, test.params) if !reflect.DeepEqual(err, test.expectedErr) { t.Errorf("[test: %s] Unexpected error: %v, expected error: %v", test.desc, err, test.expectedErr) }