diff --git a/internal/nvmeof/controller/controllerserver.go b/internal/nvmeof/controller/controllerserver.go index 3fa5dde75cb..2c989cc2f14 100644 --- a/internal/nvmeof/controller/controllerserver.go +++ b/internal/nvmeof/controller/controllerserver.go @@ -431,9 +431,9 @@ func validateCreateVolumeRequest(req *csi.CreateVolumeRequest) error { } } // Validate listeners JSON if provided - listeners, err := parseListeners(params["listeners"]) + countOfListeners, err := validateListenersParameter(params["listeners"]) if err != nil { - return fmt.Errorf("invalid listeners parameter: %w", err) + return err } // Validate network mask if provided err = validateNetworkMask(params["networkMask"]) @@ -442,11 +442,11 @@ func validateCreateVolumeRequest(req *csi.CreateVolumeRequest) error { } networkMask := params["networkMask"] // Must have EITHER listeners XOR networkMask - if len(listeners) == 0 && networkMask == "" { + if countOfListeners == 0 && networkMask == "" { return errors.New("must specify either 'listeners' xor 'networkMask', but got neither") } - if len(listeners) > 0 && networkMask != "" { - return errors.New("must specify either 'listeners' xor 'networkMask',but got both") + if countOfListeners > 0 && networkMask != "" { + return errors.New("must specify either 'listeners' xor 'networkMask', but got both") } // Validate QoS parameters - cannot mix RBD and NVMe-oF QoS mutableParams := req.GetMutableParameters() @@ -487,7 +487,7 @@ func validatePublishVolumeRequest(req *csi.ControllerPublishVolumeRequest) error return util.ValidateControllerPublishVolumeRequest(req) } -// parseListeners parses the listeners JSON parameter and validates its contents. +// validateListenersParameter parses the listeners JSON parameter and validates its contents. // it possible to have zero listeners if networkMask is provided, // because in that case the gateway will automatically create listeners // for all interfaces in the specified network mask. @@ -507,29 +507,13 @@ func validatePublishVolumeRequest(req *csi.ControllerPublishVolumeRequest) error // } // // ]. -func parseListeners(listenersJSON string) ([]nvmeof.ListenerDetails, error) { - if listenersJSON == "" { // No "listeners" entry was provided - return []nvmeof.ListenerDetails{}, nil - } - var listeners []nvmeof.ListenerDetails - if err := json.Unmarshal([]byte(listenersJSON), &listeners); err != nil { - return nil, fmt.Errorf("failed to parse listeners JSON: %w", err) - } - - if len(listeners) == 0 { // At least one listener is required - return nil, errors.New("at least one listener must be specified") - } - - // Validate each listener - // Listener address can be empty. it will set to default 0.0.0.0 - // Port can be empty (will use default - 4420). - for i, listener := range listeners { - if listener.Hostname == "" { - return nil, fmt.Errorf("listener %d: missing required fields (hostname)", i) - } +func validateListenersParameter(listenersJSON string) (int, error) { + listeners, err := nvmeof.SetupListeners(listenersJSON) + if err != nil { + return -1, err } - return listeners, nil + return len(listeners), nil } // Validate network mask CIDR format. @@ -764,43 +748,10 @@ func (cs *Server) createNVMeoFResources( params := req.GetParameters() networkMask := params["networkMask"] - nvmeofData := &nvmeof.NVMeoFVolumeData{ - SubsystemNQN: params["subsystemNQN"], - NamespaceID: 0, // will be set after namespace creation, - NamespaceUUID: "", // will be set after namespace creation - ListenerInfo: nil, // will be set after listener creation or retrieval - GatewayManagementInfo: nvmeof.GatewayConfig{ - Address: params["nvmeofGatewayAddress"], - }, - Security: nvmeof.NVMeoFSecurityConfig{ - DhchapMode: params["dhchapMode"], - AuthenticationKMSID: params["authenticationKMSID"], - }, - } - if nvmeofGatewayPortStr := params["nvmeofGatewayPort"]; nvmeofGatewayPortStr != "" { - parsed, err := strconv.ParseUint(nvmeofGatewayPortStr, 10, 32) - if err != nil { - return nil, fmt.Errorf("invalid nvmeofGatewayPort %s: %w", nvmeofGatewayPortStr, err) - } - nvmeofData.GatewayManagementInfo.Port = uint32(parsed) - } + nvmeofData := &nvmeof.NVMeoFVolumeData{} - // setup listeners (if provided, otherwise it will be set by gateway based on network mask) - listeners, err := parseListeners(params["listeners"]) - if err != nil { - return nil, fmt.Errorf("failed to parse listeners: %w", err) - } - nvmeofData.ListenerInfo = listeners - // If listener port or address is empty, it will be set to default. - nvmeofData.SetListenersWithDefaults() - - // If dhchapMode was explicitly provided and is not "none", and authenticationKMSID is empty, - // use a default KMS ID - RBD metadata KMS. - // In production, users should always provide a KMS ID when using DH-CHAP. - if nvmeofData.Security.DhchapMode != nvmeof.DHCHAPEmpty && - nvmeofData.Security.DhchapMode != nvmeof.DHCHAPModeNone && - nvmeofData.Security.AuthenticationKMSID == "" { - nvmeofData.Security.AuthenticationKMSID = "metadata" + if err := nvmeofData.SetFromParameters(params); err != nil { + return nil, fmt.Errorf("failed to set NVMe-oF volume data: %w", err) } // extract Qos parameters if any diff --git a/internal/nvmeof/volume.go b/internal/nvmeof/volume.go index c0c1a4dda12..6d4da0cb910 100644 --- a/internal/nvmeof/volume.go +++ b/internal/nvmeof/volume.go @@ -16,6 +16,13 @@ limitations under the License. package nvmeof +import ( + "encoding/json" + "errors" + "fmt" + "strconv" +) + // NVMeoFVolumeData holds the data required for an NVMe-oF volume. type NVMeoFVolumeData struct { SubsystemNQN string @@ -44,3 +51,75 @@ func (v *NVMeoFVolumeData) SetListenersWithDefaults() { } } } + +// SetupListeners parses the listeners JSON and validates the required fields. +// It returns a slice of ListenerDetails or an error if the JSON is invalid +// or if required fields are missing. +func SetupListeners(listenersJSON string) ([]ListenerDetails, error) { + if listenersJSON == "" { // No "listeners" entry was provided + return []ListenerDetails{}, nil + } + var listeners []ListenerDetails + if err := json.Unmarshal([]byte(listenersJSON), &listeners); err != nil { + return nil, fmt.Errorf("failed to parse listeners JSON: %w", err) + } + + if len(listeners) == 0 { // At least one listener is required + return nil, errors.New("at least one listener must be specified") + } + + // Validate each listener + // Listener address can be empty. It will be set later to the default 0.0.0.0. + // Port can be empty (it will be set later to the default 4420). + for i, listener := range listeners { + if listener.Hostname == "" { + return nil, fmt.Errorf("listener %d: missing required fields (hostname)", i) + } + } + + return listeners, nil +} + +// SetFromParameters populates the NVMeoFVolumeData fields based on the provided parameters map. +// It extracts the subsystem NQN, gateway management info, security config, and +// listener info from the parameters. +// It also applies default values to listeners if necessary. +func (v *NVMeoFVolumeData) SetFromParameters(parameters map[string]string) error { + // set subsystem NQN + v.SubsystemNQN = parameters["subsystemNQN"] + + // set gw management info + if nvmeofGatewayPortStr := parameters["nvmeofGatewayPort"]; nvmeofGatewayPortStr != "" { + parsed, err := strconv.ParseUint(nvmeofGatewayPortStr, 10, 32) + if err != nil { + return fmt.Errorf("invalid nvmeofGatewayPort %s: %w", nvmeofGatewayPortStr, err) + } + v.GatewayManagementInfo.Port = uint32(parsed) + } + v.GatewayManagementInfo.Address = parameters["nvmeofGatewayAddress"] + + // set security config + v.Security.DhchapMode = parameters["dhchapMode"] + v.Security.AuthenticationKMSID = parameters["authenticationKMSID"] + + // If dhchapMode was explicitly provided and is not "none", and authenticationKMSID is empty, + // use a default KMS ID - RBD metadata KMS. + // In production, users should always provide a KMS ID when using DH-CHAP. + if v.Security.DhchapMode != DHCHAPEmpty && + v.Security.DhchapMode != DHCHAPModeNone && + v.Security.AuthenticationKMSID == "" { + v.Security.AuthenticationKMSID = "metadata" + } + + // set listeners + listeners, err := SetupListeners(parameters["listeners"]) + if err != nil { + return fmt.Errorf("failed to set up listeners: %w", err) + } + v.ListenerInfo = listeners + + // Apply default values to listeners if necessary + v.SetListenersWithDefaults() + + return nil +} diff --git a/internal/nvmeof/volume_test.go b/internal/nvmeof/volume_test.go index ca9027eb4f1..7123f50cc2a 100644 --- a/internal/nvmeof/volume_test.go +++ b/internal/nvmeof/volume_test.go @@ -50,3 +50,171 @@ func TestSetListenersWithDefaults(t *testing.T) { require.Equal(t, test.out, vol.ListenerInfo) } } + +func TestSetupListeners(t *testing.T) { + t.Parallel() + tests := []struct { + name string + jsonInput string + expected []ListenerDetails + expectError bool + }{ + { + name: "valid listeners JSON", + jsonInput: `[{"hostname": "nvmeof-gw-a", "port": 4420, "address": "10.92.3.12"}]`, + expected: []ListenerDetails{ + {Hostname: "nvmeof-gw-a", GatewayAddress: GatewayAddress{Port: 4420, Address: "10.92.3.12"}}, + }, + expectError: false, + }, + { + name: "invalid listeners JSON", + jsonInput: `invalid json`, + expected: nil, + expectError: true, + }, + { + name: "missing required fields", + jsonInput: `[{"port": 4420, "address": "10.92.3.12"}]`, + expected: nil, + expectError: true, + }, + // An empty listeners array is invalid because at least one listener is required. + // the listener label can be omitted, then network mask label must be provided. + { + name: "empty listeners array", + jsonInput: `[]`, + expected: nil, + expectError: true, + }, + { + name: "no listeners entry", + jsonInput: ``, + expected: []ListenerDetails{}, + expectError: false, + }, + } + + for _, test := range tests { + listeners, err := SetupListeners(test.jsonInput) + if test.expectError { + require.Error(t, err) + } else { + require.NoError(t, err) + require.Equal(t, test.expected, listeners) + } + } +} + +func TestSetFromParameters(t *testing.T) { + t.Parallel() + tests := []struct { + name string + params map[string]string + expected NVMeoFVolumeData + expectError bool + }{ + { + name: "valid parameters with listeners and gateway info", + params: map[string]string{ + "subsystemNQN": "nqn.2016-06.io.ceph:subsystem.test-integration", + "nvmeofGatewayAddress": "10.241.1.5", + "nvmeofGatewayPort": "5500", + "listeners": `[{"hostname": "nvmeof-gw-a", "port": 4420, "address": "10.92.3.12"}]`, + }, + expected: NVMeoFVolumeData{ + SubsystemNQN: "nqn.2016-06.io.ceph:subsystem.test-integration", + GatewayManagementInfo: GatewayConfig{ + Address: "10.241.1.5", + Port: 5500, + }, + ListenerInfo: []ListenerDetails{ + {Hostname: "nvmeof-gw-a", GatewayAddress: GatewayAddress{Port: 4420, Address: "10.92.3.12"}}, + }, + }, + expectError: false, + }, + { + name: "valid parameters with DH-CHAP authentication", + params: map[string]string{ + "subsystemNQN": "nqn.2016-06.io.ceph:subsystem.test-dhchap", + "nvmeofGatewayAddress": "10.241.1.6", + "nvmeofGatewayPort": "5500", + "dhchapMode": "bidirectional", + "authenticationKMSID": "vault-kms", + "listeners": `[{"hostname": "nvmeof-gw-b", "port": 4420, "address": "10.92.3.13"}]`, + }, + expected: NVMeoFVolumeData{ + SubsystemNQN: "nqn.2016-06.io.ceph:subsystem.test-dhchap", + GatewayManagementInfo: GatewayConfig{ + Address: "10.241.1.6", + Port: 5500, + }, + Security: NVMeoFSecurityConfig{ + DhchapMode: "bidirectional", + AuthenticationKMSID: "vault-kms", + }, + ListenerInfo: []ListenerDetails{ + {Hostname: "nvmeof-gw-b", GatewayAddress: GatewayAddress{Port: 4420, Address: "10.92.3.13"}}, + }, + }, + expectError: false, + }, + { + name: "valid parameters with default KMS type", + params: map[string]string{ + "subsystemNQN": "nqn.2016-06.io.ceph:subsystem.test-dhchap-default-kms", + "nvmeofGatewayAddress": "10.241.1.6", + "nvmeofGatewayPort": "5500", + "dhchapMode": "bidirectional", + "listeners": `[{"hostname": "nvmeof-gw-b", "port": 4420, "address": "10.92.3.13"}]`, + }, + expected: NVMeoFVolumeData{ + SubsystemNQN: "nqn.2016-06.io.ceph:subsystem.test-dhchap-default-kms", + GatewayManagementInfo: GatewayConfig{ + Address: "10.241.1.6", + Port: 5500, + }, + Security: NVMeoFSecurityConfig{ + DhchapMode: "bidirectional", + AuthenticationKMSID: "metadata", + }, + ListenerInfo: []ListenerDetails{ + {Hostname: "nvmeof-gw-b", GatewayAddress: GatewayAddress{Port: 4420, Address: "10.92.3.13"}}, + }, + }, + expectError: false, + }, + { + name: "invalid gateway port", + params: map[string]string{ + "subsystemNQN": "nqn.2016-06.io.ceph:subsystem.test-invalid-port", + "nvmeofGatewayAddress": "10.241.1.7", + "nvmeofGatewayPort": "invalid-port", + }, + expected: NVMeoFVolumeData{}, + expectError: true, + }, + { + name: "invalid listeners JSON", + params: map[string]string{ + "subsystemNQN": "nqn.2016-06.io.ceph:subsystem.test-invalid-listeners", + "nvmeofGatewayAddress": "10.241.1.8", + "nvmeofGatewayPort": "5500", + "listeners": `invalid-json`, + }, + expected: NVMeoFVolumeData{}, + expectError: true, + }, + } + for _, test := range tests { + vol := &NVMeoFVolumeData{} + err := vol.SetFromParameters(test.params) + if test.expectError { + require.Error(t, err) + } else { + require.NoError(t, err) + require.Equal(t, test.expected, *vol) + } + } +}