cleanup: validate unknown parameters in ControllerModifyVolume - #6458
cleanup: validate unknown parameters in ControllerModifyVolume#6458nixpanic wants to merge 2 commits into
Conversation
nixpanic
added
component/nfs
3471981 to
3f7d560
Compare
Pull request has been modified.
3f7d560 to
218c870
Compare
| // Validate that only known parameters are provided (before any I/O) | ||
| knownParams := []string{ | ||
| nfs.ParameterServer, | ||
| nfs.ParameterClients, | ||
| } | ||
|
|
||
| for param := range req.GetMutableParameters() { | ||
| if !slices.Contains(knownParams, param) { | ||
| return nil, status.Errorf(codes.InvalidArgument, | ||
| "unknown mutable parameter: %s", param) | ||
| } | ||
| } |
There was a problem hiding this comment.
we need to have similar check before creating the subvolume or else we need a defer to cleanup the subvolume if anything fails after creation, i prefer to have input validation before loop of create/delete
There was a problem hiding this comment.
Currently CephFS does not support mutable parameters. But, well, #6390 is almost there now too. I'll add the extra checks/filtering in CreateVolume too, similar as what is done for nvmeof.
There was a problem hiding this comment.
This is done in the last update of this PR.
218c870 to
6eca8d5
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds explicit validation for unknown mutable_parameters keys in ControllerModifyVolume (and NVMe-oF CreateVolume request validation), to avoid silently accepting typos/mistakes in VolumeAttributesClass mutable parameters.
Changes:
- NVMe-oF: validate
ControllerModifyVolumeandCreateVolumerequestmutable_parametersto reject unknown keys. - NVMe-oF: avoid passing
MutableParametersthrough to the RBD backendCreateVolumecall. - NFS: avoid passing
MutableParametersthrough to the CephFS backendCreateVolumecall; validateControllerModifyVolumemutable parameters.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/nvmeof/controller/controllerserver.go | Adds allowlist validation for NVMe-oF mutable parameters and strips mutable parameters from the RBD backend create request. |
| internal/nfs/controller/controllerserver.go | Strips mutable parameters from the CephFS backend create request and adds allowlist validation for NFS mutable parameters. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
The NFS Controller gets parameters with CreateVolume, but should not pass these parameters on to CephFS CreateVolume. However, those parameters are critical for NFS functioning correctly, so those parameters need to be stored in the VolumeContext of the volume that the CephFS CreateVolume procedure returns. Working on correcting this. |
20e6097 to
5977885
Compare
Pull request has been modified.
|
/retest ci/centos/mini-e2e/k8s-1.37/cephfs |
|
@iPraveenParihar and @Madhu-1 , this PR should be ready now. NFS (and CephFS) passes e2e now too. Once this is merged, run |
|
@Mergifyio rebase |
🛑 The pull request rule doesn't match anymoreDetailsThis action has been cancelled. |
Filter out NFS-specific mutable parameters (server, clients) before passing the request to the CephFS backend. NFS handles these parameters separately through ControllerModifyVolume. This prevents the CephFS backend from receiving parameters it doesn't understand, which would cause validation errors. Assisted-by: AskBob <askbob@ibm.com> Signed-off-by: Niels de Vos <ndevos@ibm.com>
Add validation for unknown mutable parameters in ControllerModifyVolume to catch configuration errors early. Create a shared nvmeofMutableParams slice to define all valid NVMe-oF mutable parameters in one place, eliminating code duplication. Filter out NVMe-oF-specific mutable parameters before passing the request to the RBD backend. NVMe-oF handles these parameters separately through ControllerModifyVolume. This prevents the RBD backend from receiving parameters it doesn't understand, which would cause validation errors. Remove redundant rbd.HasQoSParams() checks that became dead code after the validation was moved earlier in the flow. Assisted-by: AskBob <askbob@ibm.com> Signed-off-by: Niels de Vos <ndevos@ibm.com>
5977885 to
b0daf72
Compare
|
/test ci/centos/k8s-e2e-external-storage/1.34 |
|
/test ci/centos/mini-e2e-helm/k8s-1.34 |
|
/test ci/centos/k8s-e2e-external-storage/1.35 |
|
/test ci/centos/mini-e2e/k8s-1.34 |
|
/test ci/centos/mini-e2e-helm/k8s-1.35 |
|
/test ci/centos/mini-e2e/k8s-1.35 |
|
/test ci/centos/k8s-e2e-external-storage/1.36 |
|
/test ci/centos/mini-e2e-helm/k8s-1.36 |
|
/test ci/centos/mini-e2e/k8s-1.36 |
|
/test ci/centos/upgrade-tests-cephfs |
|
/test ci/centos/upgrade-tests-rbd |
ceph-csi-bot
added
ci/in-progress/e2e
mergify
Bot
removed
the
ci/in-progress/e2e
Add validation to reject unknown mutable parameters in ControllerModifyVolume.
Previously, unknown parameters were silently ignored, which could lead to
confusion when users specify incorrect parameter names.
Related: #6443