Skip to content

Commit

Permalink
Merge pull request #156 from dabradley/skip-subdir-invalid-id
Browse files Browse the repository at this point in the history 
Skip subdirectory creation for invalid volume ID
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot authored Jan 16, 2024
2 parents 8854426 + 697f860 commit b46b1dd
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 b46b1dd

Please sign in to comment.