CON-4645: fsRepair param does not validate the input and no error is reported, silently applies any values input#536
Conversation
…reported, silently applies any values input Signed-off-by: Rashi Jain <rashi.jain@hpe.com>
rashi-jain1
requested review from
datamattsson,
dileepds,
pavansshanbhag,
raunakkumar,
rohit-balakrishna,
sijeesh and
suneeth51
as code owners
| // Validate fsRepair value from volumeContext -- catches PVs that may have been | ||
| // created before this validation existed and have an invalid (non-boolean) value. | ||
| if fsRepairVal := volumeContext[fsRepairKey]; fsRepairVal != "" && fsRepairVal != trueKey && fsRepairVal != falseKey { | ||
| return nil, fmt.Errorf("invalid value %q for fsRepair parameter on volume %s, it must be either [true, false] (lowercase only)", fsRepairVal, volumeID) |
There was a problem hiding this comment.
We need to return the gRPC error code InvalidArgument here. But the issue is that the NodeStageVolume masks all the error codes into codes.Internal when responding to the CO. We should change this function and the calling function and fix it so that NodeStageVolume can pass err and the actual gRPC error code is prepared in the nodeStageVolume function. Please check the pattern used in NodeUnstageVolume/nodeUnstageVolume.
While you are changing this, please also fix the same in line no: 763, 771, 777
Signed-off-by: Rashi Jain <rashi.jain@hpe.com>
| mountInfo := getMountInfo(volumeID, volCap, publishContext, stagingMountPoint) | ||
|
|
||
| // Validate fsRepair value from volumeContext -- catches PVs that may have been | ||
| // created before this validation existed and have an invalid (non-boolean) value. |
There was a problem hiding this comment.
If there are customers who had configured this incorrectly before and without this check, it was working without throwing errors, adding this check will cause unnecessary regressions. I dont think, we should have this check.
CC: @dileepds , @datamattsson
There was a problem hiding this comment.
So, shall I remove this check?
There was a problem hiding this comment.
Yes, please remove this check.
| if err != nil { | ||
| log.Errorln(err.Error()) | ||
| return err | ||
| return status.Errorf(codes.Internal, "failed to retrieve encryption passphrase for volume: %s", err.Error()) |
There was a problem hiding this comment.
the issues also exist at line no: 211, 269, 305, 450, 486, 490, 506, 512, 515, 518. Please change these as well.
Also at line no: 252(isVolumeStaged already add a proper error code) no change needed.
Once you fix the above and add proper error codes in the stageVolume function, you should remove the error code masking at line no: 297.
Signed-off-by: Rashi Jain <rashi.jain@hpe.com>
| } | ||
| } | ||
|
|
||
| func contains(s, substr string) bool { |
There was a problem hiding this comment.
string module already has function for both contains and containsSubstring, why are we adding our own code. please remove these functions and use the inbuilt string functions.
datamattsson
left a comment
datamattsson
left a comment
There was a problem hiding this comment.
Besides my comment, can we validate "nfsResources" and "hostEncryption" the same way as they are string booleans too.
|
|
||
| // validateFsRepairParameter validates that the fsRepair StorageClass parameter, | ||
| // if specified, is strictly "true" or "false" (lowercase only). | ||
| func (driver *Driver) validateFsRepairParameter(createParameters map[string]string) error { |
There was a problem hiding this comment.
Can we use a generic library for boolean string validation instead?
There was a problem hiding this comment.
Done. Added validation for "nfsResources" and for "hostEncryption" we're already validating by "validateHostEncryptionParameters"
Signed-off-by: Rashi Jain <rashi.jain@hpe.com>
4 participants
BUG Link : https://jira.storage.hpecorp.net/browse/CON-4645
BUG Description: fsRepair param does not validate the input and no error is reported, silently applies any values input
BUG Fix :
Added a new validateFsRepairParameter() function that rejects any invalid value of fsRepair that is not exactly "true" or "false" (lowercase only)
Attached Logs on JIRA.