cleanup: check GetParameters() for nil in CSI requests - #6463
Draft
nixpanic wants to merge 7 commits into
Draft
Conversation
Add validation to check if req.GetParameters() returns nil before accessing the parameters map. This prevents potential nil pointer dereferences in the following functions: - CreateSnapshot in controllerserver.go - validation function in groupcontrollerserver.go - NewVolumeOptions in store/volumeoptions.go - NewVolumeGroupOptions in store/volumegroup.go Assisted-by: AskBob <askbob@ibm.com> Signed-off-by: Niels de Vos <ndevos@ibm.com>
Add validation to check if req.GetParameters() returns nil before accessing the parameters map. This prevents potential nil pointer dereferences in the following functions: - CreateSnapshot in controllerserver.go - genVolFromVolID in rbd_util.go Assisted-by: AskBob <askbob@ibm.com> Signed-off-by: Niels de Vos <ndevos@ibm.com>
Add validation to check if req.GetParameters() returns nil before accessing the parameters map in the network fence implementation. This prevents potential nil pointer dereferences. Assisted-by: AskBob <askbob@ibm.com> Signed-off-by: Niels de Vos <ndevos@ibm.com>
Add validation to check if req.GetParameters() returns nil before accessing the parameters map in the network fence implementation. This prevents potential nil pointer dereferences. Assisted-by: AskBob <askbob@ibm.com> Signed-off-by: Niels de Vos <ndevos@ibm.com>
Add validation to check if req.GetParameters() returns nil before accessing the parameters map in the following RBD csi-addons functions: - network_fence.go: FenceClusterNetwork and UnfenceClusterNetwork - replication.go: EnableVolumeReplication - volumegroup.go: CreateVolumeGroup This prevents potential nil pointer dereferences. Assisted-by: AskBob <askbob@ibm.com> Signed-off-by: Niels de Vos <ndevos@ibm.com>
Add validation to check if req.GetParameters() returns nil before accessing the parameters map in the CreateSnapshot function in controller/controllerserver.go. This prevents potential nil pointer dereferences. Assisted-by: AskBob <askbob@ibm.com> Signed-off-by: Niels de Vos <ndevos@ibm.com>
Add validation to check if req.GetParameters() returns nil before accessing the parameters map in the CreateVolumeGroup function in group_controllerserver.go. This prevents potential nil pointer dereferences. Assisted-by: AskBob <askbob@ibm.com> Signed-off-by: Niels de Vos <ndevos@ibm.com>
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent potential nil-map handling issues by explicitly checking GetParameters() results across several CSI and CSI-addons request handlers in Ceph-CSI (RBD, CephFS, NVMe-oF).
Changes:
- Added early
nilchecks forreq.GetParameters()in multiple controller and helper paths. - Returned
InvalidArgument/errors when parameters are nil, before continuing request validation/processing. - Updated various call sites to pass locally-captured
parametersinto downstream helpers/managers.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/rbd/group_controllerserver.go | Adds parameters nil-check before creating the RBD manager for volume group snapshot ops. |
| internal/rbd/controllerserver.go | Adds parameters nil-checks in create-volume and create-snapshot request validation. |
| internal/nvmeof/controller/controllerserver.go | Adds parameters nil-checks in NVMe-oF create-volume validation and NVMe-oF resource creation. |
| internal/csi-addons/rbd/volumegroup.go | Adds parameters nil-checks when creating volume groups and extracting flatten mode. |
| internal/csi-addons/rbd/replication.go | Adds parameters nil-checks across replication RPCs before using request parameters. |
| internal/csi-addons/rbd/network_fence.go | Adds parameters nil-checks for network fencing/unfencing operations. |
| internal/csi-addons/networkfence/fencing.go | Adds parameters nil-check in GetFenceClients before reading clusterID. |
| internal/csi-addons/cephfs/network_fence.go | Adds parameters nil-checks for CephFS network fencing/unfencing operations. |
| internal/cephfs/store/volumeoptions.go | Adds parameters nil-check in volume options construction. |
| internal/cephfs/store/volumegroup.go | Adds parameters nil-check in volume group options construction. |
| internal/cephfs/groupcontrollerserver.go | Adds parameters nil-check during volume group snapshot request validation. |
| internal/cephfs/controllerserver.go | Adds parameters nil-check before extracting cluster information during snapshot create. |
Suppressed comments (5)
internal/csi-addons/rbd/replication.go:359
- Nil maps are a valid zero value for protobuf map fields and are safe to index/iterate in Go. Returning an error for nil here changes behavior compared to an empty map (which would just use defaults). Consider normalizing nil to an empty map instead of rejecting the request.
parameters := req.GetParameters()
if parameters == nil {
return nil, status.Error(codes.InvalidArgument, "parameters cannot be nil")
}
internal/csi-addons/rbd/replication.go:522
- Nil protobuf maps are safe for reads and equivalent to empty maps for this code path. Rejecting nil here is an unnecessary behavior change; prefer normalizing nil to an empty map and letting subsequent logic apply defaults/required-key validation.
parameters := req.GetParameters()
if parameters == nil {
return nil, status.Error(codes.InvalidArgument, "parameters cannot be nil")
}
internal/csi-addons/rbd/replication.go:644
- A nil
parametersmap is a valid state for omitted protobuf map fields and is safe to read from. Returning an error here is stricter than necessary and makes nil behave differently from an empty map. Prefer normalizing nil tomap[string]string{}and continue.
parameters := req.GetParameters()
if parameters == nil {
return nil, status.Error(codes.InvalidArgument, "parameters cannot be nil")
}
internal/csi-addons/rbd/network_fence.go:104
- Same as
FenceClusterNetwork: nil protobuf maps are safe for reads and the existing validation already returns a specific error whenclusterIDis missing. Rejecting nil parameters is stricter and less informative. Prefer normalizing to an empty map and reuseparametersfor theNewNetworkFencecall.
parameters := req.GetParameters()
if parameters == nil {
return nil, status.Error(codes.InvalidArgument, "parameters cannot be nil")
}
internal/csi-addons/cephfs/network_fence.go:109
- Same as
FenceClusterNetwork: nil protobuf maps are safe for reads and the existing validation already reports missingclusterID. Rejecting nil parameters is stricter and less informative. Prefer normalizing to an empty map and reuseparametersfor theNewNetworkFencecall.
parameters := req.GetParameters()
if parameters == nil {
return nil, status.Error(codes.InvalidArgument, "parameters cannot be nil")
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+78
to
+81
| parameters := req.GetParameters() | ||
| if parameters == nil { | ||
| return nil, status.Error(codes.InvalidArgument, "parameters cannot be nil") | ||
| } |
Comment on lines
82
to
+85
| options := req.GetParameters() | ||
| if options == nil { | ||
| return status.Error(codes.InvalidArgument, "parameters cannot be nil") | ||
| } |
Comment on lines
1404
to
+1407
| options := req.GetParameters() | ||
| if options == nil { | ||
| return status.Error(codes.InvalidArgument, "parameters cannot be nil") | ||
| } |
Comment on lines
468
to
+471
| params := req.GetParameters() | ||
| if params == nil { | ||
| return errors.New("parameters cannot be nil") | ||
| } |
Comment on lines
803
to
+806
| params := req.GetParameters() | ||
| if params == nil { | ||
| return nil, errors.New("parameters cannot be nil") | ||
| } |
Comment on lines
+73
to
+76
| parameters := req.GetParameters() | ||
| if parameters == nil { | ||
| return nil, status.Error(codes.InvalidArgument, "parameters cannot be nil") | ||
| } |
Comment on lines
+776
to
+779
| parameters := req.GetParameters() | ||
| if parameters == nil { | ||
| return nil, status.Error(codes.InvalidArgument, "parameters cannot be nil") | ||
| } |
Comment on lines
279
to
+282
| volOptions := req.GetParameters() | ||
| if volOptions == nil { | ||
| return nil, fmt.Errorf("parameters cannot be nil") | ||
| } |
Comment on lines
47
to
+50
| volOptions := req.GetParameters() | ||
| if volOptions == nil { | ||
| return nil, fmt.Errorf("parameters cannot be nil") | ||
| } |
Comment on lines
63
to
+66
| param := req.GetParameters() | ||
| if param == nil { | ||
| return status.Error(codes.InvalidArgument, "parameters cannot be nil") | ||
| } |
iPraveenParihar
left a comment
iPraveenParihar
left a comment
Contributor
There was a problem hiding this comment.
May be copilot is overthinking 😜, nil map reads don't panic in Go, so these are for better error messages rather than crash prevention.
| } | ||
| } | ||
|
|
||
| interval, startTime := getSchedulingDetails(req.GetParameters()) |
Contributor
There was a problem hiding this comment.
Need check here too?
| } | ||
|
|
||
| // extract the flatten mode | ||
| flattenMode, err := getFlattenMode(ctx, req.GetParameters()) |
Contributor
There was a problem hiding this comment.
There are other replaces where we don't have nil check.
| if parameters == nil { | ||
| return nil, status.Error(codes.InvalidArgument, "parameters cannot be nil") | ||
| } | ||
| mgr := rbd.NewManager(rs.driverInstance, parameters, req.GetSecrets()) |
Contributor
There was a problem hiding this comment.
@nixpanic, How about moving the check into NewManager() ?
Member
Author
A, interesting! I was expecting this to panic, but it does not: package main
import (
"fmt"
)
func getMap() map[string]any {
return nil
}
func main() {
m := getMap()
v := m["panic"]
fmt.Printf("no panic happened, value: %v\n", v)
}Result: It seems that adding a new key/value to the map I'll have a think about how to progress with this. Better error reporting is helpful, but maybe #6458 is sufficient for that already. |
nixpanic
marked this pull request as draft
August 7, 2026 12:21
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Copilot reported a potential panic when there are no parameters passed in CSI
requests. It is unlikely that a StorageClass or other sources of requests do
not contain parameters, but technically users could do this and cause Ceph-CSI
to crash.
See-also: #6458 (comment)