Skip to content

Commit

Permalink
Skip subdirectory creation for invalid volume ID
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@dabradley
dabradley committed Jan 12, 2024
1 parent 69b121b commit 697f860
Show file tree
Hide file tree
Showing 3 changed files with 119 additions and 65 deletions.
5 changes: 5 additions & 0 deletions docs/examples/pv_subdir.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
82 changes: 36 additions & 46 deletions pkg/azurelustre/nodeserver.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{}

Expand Down Expand Up @@ -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)
Expand All @@ -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",
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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]
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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
}
}

Expand All @@ -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
Expand Down
97 changes: 78 additions & 19 deletions pkg/azurelustre/nodeserver_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -1210,16 +1264,17 @@ 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",
"sub-dir": "testSubDir",
"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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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)
}
Expand Down

0 comments on commit 697f860

Please sign in to comment.