From 4efcc5bf9741a8b9c0428e322257bbc6e3e82014 Mon Sep 17 00:00:00 2001 From: Humble Chirammal Date: Fri, 3 Sep 2021 18:18:49 +0530 Subject: [PATCH 01/10] cleanup: simplify checkStaticVolume function and remove unwanted vars checkStaticVolume() in the reconcilePV function has been unwantedly introducing variables to confirm the pv spec is static or not. This patch simplify it and make a smaller footprint of the functions. Signed-off-by: Humble Chirammal --- .../persistentvolume/persistentvolume.go | 21 +++---------------- 1 file changed, 3 insertions(+), 18 deletions(-) diff --git a/internal/controller/persistentvolume/persistentvolume.go b/internal/controller/persistentvolume/persistentvolume.go index 0e81b58d5..269ec757c 100644 --- a/internal/controller/persistentvolume/persistentvolume.go +++ b/internal/controller/persistentvolume/persistentvolume.go @@ -19,7 +19,6 @@ import ( "context" "errors" "fmt" - "strconv" ctrl "github.com/ceph/ceph-csi/internal/controller" "github.com/ceph/ceph-csi/internal/rbd" @@ -126,19 +125,8 @@ func (r *ReconcilePersistentVolume) getCredentials( return cr, nil } -func checkStaticVolume(pv *corev1.PersistentVolume) (bool, error) { - static := false - var err error - - staticVol := pv.Spec.CSI.VolumeAttributes["staticVolume"] - if staticVol != "" { - static, err = strconv.ParseBool(staticVol) - if err != nil { - return false, fmt.Errorf("failed to parse preProvisionedVolume: %w", err) - } - } - - return static, nil +func checkStaticVolume(pv *corev1.PersistentVolume) bool { + return pv.Spec.CSI.VolumeAttributes["staticVolume"] == "true" } // storeVolumeIDInPV stores the new volumeID in PV object. @@ -178,10 +166,7 @@ func (r ReconcilePersistentVolume) reconcilePV(ctx context.Context, obj runtime. secretName := "" secretNamespace := "" // check static volume - static, err := checkStaticVolume(pv) - if err != nil { - return err - } + static := checkStaticVolume(pv) // if the volume is static, dont generate OMAP data if static { return nil From 73e2ffe8b851929d2cd44d9c132c28574454911e Mon Sep 17 00:00:00 2001 From: Madhu Rajanna Date: Mon, 6 Sep 2021 17:48:53 +0530 Subject: [PATCH 02/10] cleanup: move cephfs csi spec validation to validator moved the cephfs related validation like validating the input parameters sent in the GRPC request to a new file. Signed-off-by: Madhu Rajanna --- internal/cephfs/util.go | 88 --------------------------- internal/cephfs/validator.go | 112 +++++++++++++++++++++++++++++++++++ 2 files changed, 112 insertions(+), 88 deletions(-) create mode 100644 internal/cephfs/validator.go diff --git a/internal/cephfs/util.go b/internal/cephfs/util.go index ee1920bd1..05834a1c9 100644 --- a/internal/cephfs/util.go +++ b/internal/cephfs/util.go @@ -18,7 +18,6 @@ package cephfs import ( "context" - "fmt" "time" "github.com/ceph/ceph-csi/internal/util" @@ -27,8 +26,6 @@ import ( "github.com/container-storage-interface/spec/lib/go/csi" "github.com/golang/protobuf/ptypes" "github.com/golang/protobuf/ptypes/timestamp" - "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" ) type volumeID string @@ -39,91 +36,6 @@ func execCommandErr(ctx context.Context, program string, args ...string) error { return err } -// Controller service request validation. -func (cs *ControllerServer) validateCreateVolumeRequest(req *csi.CreateVolumeRequest) error { - if err := cs.Driver.ValidateControllerServiceRequest( - csi.ControllerServiceCapability_RPC_CREATE_DELETE_VOLUME); err != nil { - return fmt.Errorf("invalid CreateVolumeRequest: %w", err) - } - - if req.GetName() == "" { - return status.Error(codes.InvalidArgument, "volume Name cannot be empty") - } - - reqCaps := req.GetVolumeCapabilities() - if reqCaps == nil { - return status.Error(codes.InvalidArgument, "volume Capabilities cannot be empty") - } - - for _, capability := range reqCaps { - if capability.GetBlock() != nil { - return status.Error(codes.Unimplemented, "block volume not supported") - } - } - - // Allow readonly access mode for volume with content source - err := util.CheckReadOnlyManyIsSupported(req) - if err != nil { - return err - } - - if req.VolumeContentSource != nil { - volumeSource := req.VolumeContentSource - switch volumeSource.Type.(type) { - case *csi.VolumeContentSource_Snapshot: - snapshot := req.VolumeContentSource.GetSnapshot() - // CSI spec requires returning NOT_FOUND when the volumeSource is missing/incorrect. - if snapshot == nil { - return status.Error(codes.NotFound, "volume Snapshot cannot be empty") - } - if snapshot.GetSnapshotId() == "" { - return status.Error(codes.NotFound, "volume Snapshot ID cannot be empty") - } - case *csi.VolumeContentSource_Volume: - // CSI spec requires returning NOT_FOUND when the volumeSource is missing/incorrect. - vol := req.VolumeContentSource.GetVolume() - if vol == nil { - return status.Error(codes.NotFound, "volume cannot be empty") - } - if vol.GetVolumeId() == "" { - return status.Error(codes.NotFound, "volume ID cannot be empty") - } - - default: - return status.Error(codes.InvalidArgument, "unsupported volume data source") - } - } - - return nil -} - -func (cs *ControllerServer) validateDeleteVolumeRequest() error { - if err := cs.Driver.ValidateControllerServiceRequest( - csi.ControllerServiceCapability_RPC_CREATE_DELETE_VOLUME); err != nil { - return fmt.Errorf("invalid DeleteVolumeRequest: %w", err) - } - - return nil -} - -// Controller expand volume request validation. -func (cs *ControllerServer) validateExpandVolumeRequest(req *csi.ControllerExpandVolumeRequest) error { - if err := cs.Driver.ValidateControllerServiceRequest(csi.ControllerServiceCapability_RPC_EXPAND_VOLUME); err != nil { - return fmt.Errorf("invalid ExpandVolumeRequest: %w", err) - } - - if req.GetVolumeId() == "" { - return status.Error(codes.InvalidArgument, "Volume ID cannot be empty") - } - - capRange := req.GetCapacityRange() - if capRange == nil { - return status.Error(codes.InvalidArgument, "CapacityRange cannot be empty") - } - - return nil -} - func genSnapFromOptions(ctx context.Context, req *csi.CreateSnapshotRequest) (snap *cephfsSnapshot, err error) { cephfsSnap := &cephfsSnapshot{} cephfsSnap.RequestName = req.GetName() diff --git a/internal/cephfs/validator.go b/internal/cephfs/validator.go new file mode 100644 index 000000000..5d398574a --- /dev/null +++ b/internal/cephfs/validator.go @@ -0,0 +1,112 @@ +/* +Copyright 2021 The Ceph-CSI Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package cephfs + +import ( + "fmt" + + "github.com/ceph/ceph-csi/internal/util" + + "github.com/container-storage-interface/spec/lib/go/csi" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" +) + +// Controller service request validation. +func (cs *ControllerServer) validateCreateVolumeRequest(req *csi.CreateVolumeRequest) error { + if err := cs.Driver.ValidateControllerServiceRequest( + csi.ControllerServiceCapability_RPC_CREATE_DELETE_VOLUME); err != nil { + return fmt.Errorf("invalid CreateVolumeRequest: %w", err) + } + + if req.GetName() == "" { + return status.Error(codes.InvalidArgument, "volume Name cannot be empty") + } + + reqCaps := req.GetVolumeCapabilities() + if reqCaps == nil { + return status.Error(codes.InvalidArgument, "volume Capabilities cannot be empty") + } + + for _, capability := range reqCaps { + if capability.GetBlock() != nil { + return status.Error(codes.Unimplemented, "block volume not supported") + } + } + + // Allow readonly access mode for volume with content source + err := util.CheckReadOnlyManyIsSupported(req) + if err != nil { + return err + } + + if req.VolumeContentSource != nil { + volumeSource := req.VolumeContentSource + switch volumeSource.Type.(type) { + case *csi.VolumeContentSource_Snapshot: + snapshot := req.VolumeContentSource.GetSnapshot() + // CSI spec requires returning NOT_FOUND when the volumeSource is missing/incorrect. + if snapshot == nil { + return status.Error(codes.NotFound, "volume Snapshot cannot be empty") + } + if snapshot.GetSnapshotId() == "" { + return status.Error(codes.NotFound, "volume Snapshot ID cannot be empty") + } + case *csi.VolumeContentSource_Volume: + // CSI spec requires returning NOT_FOUND when the volumeSource is missing/incorrect. + vol := req.VolumeContentSource.GetVolume() + if vol == nil { + return status.Error(codes.NotFound, "volume cannot be empty") + } + if vol.GetVolumeId() == "" { + return status.Error(codes.NotFound, "volume ID cannot be empty") + } + + default: + return status.Error(codes.InvalidArgument, "unsupported volume data source") + } + } + + return nil +} + +func (cs *ControllerServer) validateDeleteVolumeRequest() error { + if err := cs.Driver.ValidateControllerServiceRequest( + csi.ControllerServiceCapability_RPC_CREATE_DELETE_VOLUME); err != nil { + return fmt.Errorf("invalid DeleteVolumeRequest: %w", err) + } + + return nil +} + +// Controller expand volume request validation. +func (cs *ControllerServer) validateExpandVolumeRequest(req *csi.ControllerExpandVolumeRequest) error { + if err := cs.Driver.ValidateControllerServiceRequest(csi.ControllerServiceCapability_RPC_EXPAND_VOLUME); err != nil { + return fmt.Errorf("invalid ExpandVolumeRequest: %w", err) + } + + if req.GetVolumeId() == "" { + return status.Error(codes.InvalidArgument, "Volume ID cannot be empty") + } + + capRange := req.GetCapacityRange() + if capRange == nil { + return status.Error(codes.InvalidArgument, "CapacityRange cannot be empty") + } + + return nil +} From 31696a6ce0ca656ba031210e20689e3562561c6c Mon Sep 17 00:00:00 2001 From: Madhu Rajanna Date: Mon, 6 Sep 2021 17:50:55 +0530 Subject: [PATCH 03/10] cleanup: move genSnapFromOptions to volumeoptions moved genSnapFromOptions function to volumeoptions.go which is more appropriated than util. Signed-off-by: Madhu Rajanna --- internal/cephfs/util.go | 19 ------------------- internal/cephfs/volumeoptions.go | 19 +++++++++++++++++++ 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/internal/cephfs/util.go b/internal/cephfs/util.go index 05834a1c9..42ecb03ce 100644 --- a/internal/cephfs/util.go +++ b/internal/cephfs/util.go @@ -23,7 +23,6 @@ import ( "github.com/ceph/ceph-csi/internal/util" "github.com/ceph/ceph-csi/internal/util/log" - "github.com/container-storage-interface/spec/lib/go/csi" "github.com/golang/protobuf/ptypes" "github.com/golang/protobuf/ptypes/timestamp" ) @@ -36,24 +35,6 @@ func execCommandErr(ctx context.Context, program string, args ...string) error { return err } -func genSnapFromOptions(ctx context.Context, req *csi.CreateSnapshotRequest) (snap *cephfsSnapshot, err error) { - cephfsSnap := &cephfsSnapshot{} - cephfsSnap.RequestName = req.GetName() - snapOptions := req.GetParameters() - - cephfsSnap.Monitors, cephfsSnap.ClusterID, err = util.GetMonsAndClusterID(snapOptions) - if err != nil { - log.ErrorLog(ctx, "failed getting mons (%s)", err) - - return nil, err - } - if namePrefix, ok := snapOptions["snapshotNamePrefix"]; ok { - cephfsSnap.NamePrefix = namePrefix - } - - return cephfsSnap, nil -} - func parseTime(ctx context.Context, createTime time.Time) (*timestamp.Timestamp, error) { tm, err := ptypes.TimestampProto(createTime) if err != nil { diff --git a/internal/cephfs/volumeoptions.go b/internal/cephfs/volumeoptions.go index 3a44cae22..ea62ecbe0 100644 --- a/internal/cephfs/volumeoptions.go +++ b/internal/cephfs/volumeoptions.go @@ -27,6 +27,7 @@ import ( cerrors "github.com/ceph/ceph-csi/internal/cephfs/errors" "github.com/ceph/ceph-csi/internal/util" + "github.com/ceph/ceph-csi/internal/util/log" ) type volumeOptions struct { @@ -586,3 +587,21 @@ func newSnapshotOptionsFromID( return &volOptions, &info, &sid, nil } + +func genSnapFromOptions(ctx context.Context, req *csi.CreateSnapshotRequest) (snap *cephfsSnapshot, err error) { + cephfsSnap := &cephfsSnapshot{} + cephfsSnap.RequestName = req.GetName() + snapOptions := req.GetParameters() + + cephfsSnap.Monitors, cephfsSnap.ClusterID, err = util.GetMonsAndClusterID(snapOptions) + if err != nil { + log.ErrorLog(ctx, "failed getting mons (%s)", err) + + return nil, err + } + if namePrefix, ok := snapOptions["snapshotNamePrefix"]; ok { + cephfsSnap.NamePrefix = namePrefix + } + + return cephfsSnap, nil +} From da70ed50dc5ef36c0ec7f162173bbb8363492e15 Mon Sep 17 00:00:00 2001 From: Madhu Rajanna Date: Mon, 6 Sep 2021 17:53:07 +0530 Subject: [PATCH 04/10] cleanup: move execCommandErr to volumemounter Moved execCommandErr to the volumemounter.go which is the only caller of this function and moving the execCommandErr helps in reducing the util file. Signed-off-by: Madhu Rajanna --- internal/cephfs/util.go | 7 ------- internal/cephfs/volumemounter.go | 6 ++++++ 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/internal/cephfs/util.go b/internal/cephfs/util.go index 42ecb03ce..49b63e48f 100644 --- a/internal/cephfs/util.go +++ b/internal/cephfs/util.go @@ -20,7 +20,6 @@ import ( "context" "time" - "github.com/ceph/ceph-csi/internal/util" "github.com/ceph/ceph-csi/internal/util/log" "github.com/golang/protobuf/ptypes" @@ -29,12 +28,6 @@ import ( type volumeID string -func execCommandErr(ctx context.Context, program string, args ...string) error { - _, _, err := util.ExecCommand(ctx, program, args...) - - return err -} - func parseTime(ctx context.Context, createTime time.Time) (*timestamp.Timestamp, error) { tm, err := ptypes.TimestampProto(createTime) if err != nil { diff --git a/internal/cephfs/volumemounter.go b/internal/cephfs/volumemounter.go index 04ad0f5b0..0fa4f4757 100644 --- a/internal/cephfs/volumemounter.go +++ b/internal/cephfs/volumemounter.go @@ -66,6 +66,12 @@ var ( } ) +func execCommandErr(ctx context.Context, program string, args ...string) error { + _, _, err := util.ExecCommand(ctx, program, args...) + + return err +} + // Load available ceph mounters installed on system into availableMounters // Called from driver.go's Run(). func loadAvailableMounters(conf *util.Config) error { From be7749c90e9e71924982192c3d41a665e6d4113a Mon Sep 17 00:00:00 2001 From: Madhu Rajanna Date: Mon, 6 Sep 2021 17:54:47 +0530 Subject: [PATCH 05/10] cleanup: move volumeID to the volumeoptions volumeID can be moved to the volumeOptions as most of the volume related helper functions are available on the volumeoptions.go Signed-off-by: Madhu Rajanna --- internal/cephfs/util.go | 2 -- internal/cephfs/volumeoptions.go | 2 ++ 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/cephfs/util.go b/internal/cephfs/util.go index 49b63e48f..11e949b2f 100644 --- a/internal/cephfs/util.go +++ b/internal/cephfs/util.go @@ -26,8 +26,6 @@ import ( "github.com/golang/protobuf/ptypes/timestamp" ) -type volumeID string - func parseTime(ctx context.Context, createTime time.Time) (*timestamp.Timestamp, error) { tm, err := ptypes.TimestampProto(createTime) if err != nil { diff --git a/internal/cephfs/volumeoptions.go b/internal/cephfs/volumeoptions.go index ea62ecbe0..8600664dc 100644 --- a/internal/cephfs/volumeoptions.go +++ b/internal/cephfs/volumeoptions.go @@ -30,6 +30,8 @@ import ( "github.com/ceph/ceph-csi/internal/util/log" ) +type volumeID string + type volumeOptions struct { TopologyPools *[]util.TopologyConstrainedPool TopologyRequirement *csi.TopologyRequirement From 8caeb409bb1bf8b63117e2e7ee690169b7066a3f Mon Sep 17 00:00:00 2001 From: Madhu Rajanna Date: Tue, 7 Sep 2021 12:03:29 +0530 Subject: [PATCH 06/10] cephfs: add comment for validateDeleteVolumeRequest added function comment for the validateDeleteVolumeRequest function. Signed-off-by: Madhu Rajanna --- internal/cephfs/validator.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/cephfs/validator.go b/internal/cephfs/validator.go index 5d398574a..86c5c970c 100644 --- a/internal/cephfs/validator.go +++ b/internal/cephfs/validator.go @@ -84,6 +84,7 @@ func (cs *ControllerServer) validateCreateVolumeRequest(req *csi.CreateVolumeReq return nil } +// validateDeleteVolumeRequest validates the Controller DeleteVolume request. func (cs *ControllerServer) validateDeleteVolumeRequest() error { if err := cs.Driver.ValidateControllerServiceRequest( csi.ControllerServiceCapability_RPC_CREATE_DELETE_VOLUME); err != nil { From 9fd51d9bec20a77c443dd6aea9d1ffd0460dc973 Mon Sep 17 00:00:00 2001 From: Madhu Rajanna Date: Tue, 7 Sep 2021 12:04:55 +0530 Subject: [PATCH 07/10] cephfs: add comment for validateCreateVolumeRequest added function comment for validateCreateVolumeRequest Signed-off-by: Madhu Rajanna --- internal/cephfs/validator.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/cephfs/validator.go b/internal/cephfs/validator.go index 86c5c970c..9fd154e49 100644 --- a/internal/cephfs/validator.go +++ b/internal/cephfs/validator.go @@ -26,7 +26,7 @@ import ( "google.golang.org/grpc/status" ) -// Controller service request validation. +// validateCreateVolumeRequest validates the Controller CreateVolume request. func (cs *ControllerServer) validateCreateVolumeRequest(req *csi.CreateVolumeRequest) error { if err := cs.Driver.ValidateControllerServiceRequest( csi.ControllerServiceCapability_RPC_CREATE_DELETE_VOLUME); err != nil { From 76f1b4249851016ded3933824b58258d21dd7a69 Mon Sep 17 00:00:00 2001 From: Madhu Rajanna Date: Tue, 7 Sep 2021 12:06:14 +0530 Subject: [PATCH 08/10] cephfs: correct comment for validateExpandVolumeRequest corrected the function comment for validateExpandVolumeRequest. Signed-off-by: Madhu Rajanna --- internal/cephfs/validator.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/cephfs/validator.go b/internal/cephfs/validator.go index 9fd154e49..07e38688c 100644 --- a/internal/cephfs/validator.go +++ b/internal/cephfs/validator.go @@ -94,7 +94,7 @@ func (cs *ControllerServer) validateDeleteVolumeRequest() error { return nil } -// Controller expand volume request validation. +// validateExpandVolumeRequest validates the Controller ExpandVolume request. func (cs *ControllerServer) validateExpandVolumeRequest(req *csi.ControllerExpandVolumeRequest) error { if err := cs.Driver.ValidateControllerServiceRequest(csi.ControllerServiceCapability_RPC_EXPAND_VOLUME); err != nil { return fmt.Errorf("invalid ExpandVolumeRequest: %w", err) From e99dd3dea4759887a051eda486ef1df5149ad689 Mon Sep 17 00:00:00 2001 From: Rakshith R Date: Tue, 7 Sep 2021 17:20:51 +0530 Subject: [PATCH 09/10] util: read ceph.conf by calling conn.ReadConfigFile(CephConfigPath) The configurations in cpeh.conf is not picked up by rados connection automatically, hence we need to call conn.ReadConfigFile before calling Connect(). Signed-off-by: Rakshith R --- internal/util/conn_pool.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/internal/util/conn_pool.go b/internal/util/conn_pool.go index 3987101dd..aca2bb482 100644 --- a/internal/util/conn_pool.go +++ b/internal/util/conn_pool.go @@ -145,6 +145,10 @@ func (cp *ConnPool) Get(monitors, user, keyfile string) (*rados.Conn, error) { return nil, fmt.Errorf("parsing cmdline args (%v) failed: %w", args, err) } + if err = conn.ReadConfigFile(CephConfigPath); err != nil { + return nil, fmt.Errorf("failed to read config file %q: %w", CephConfigPath, err) + } + err = conn.Connect() if err != nil { return nil, fmt.Errorf("connecting failed: %w", err) From 8c8f34cf7a98527ef6fb9324f1da7b0cf50a3445 Mon Sep 17 00:00:00 2001 From: Madhu Rajanna Date: Wed, 8 Sep 2021 13:28:30 +0530 Subject: [PATCH 10/10] rbd: set vaultAuthNamespace to vaultNamespace if empty When we read the csi-kms-connection-details configmap vaultAuthNamespace might not be set when we do the conversion the vaultAuthNamespace might be set to empty key and this commits check for the empty value of vaultAuthNamespace and set the vaultAuthNamespace to vaultNamespace. setting empty value for vaultAuthNamespace happened due to Marshalling at https://github.com/ceph/ceph-csi/blob/devel/ internal/kms/vault_tokens.go#L136-L139. Signed-off-by: Madhu Rajanna --- internal/kms/vault.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/internal/kms/vault.go b/internal/kms/vault.go index 6a7fc001c..d1e146538 100644 --- a/internal/kms/vault.go +++ b/internal/kms/vault.go @@ -192,11 +192,16 @@ func (vc *vaultConnection) initConnection(config map[string]interface{}) error { if errors.Is(err, errConfigOptionInvalid) { return err } - vaultAuthNamespace := vaultNamespace // optional, same as vaultNamespace + vaultAuthNamespace := "" err = setConfigString(&vaultAuthNamespace, config, "vaultAuthNamespace") if errors.Is(err, errConfigOptionInvalid) { return err } + // if the vaultAuthNamespace key is present and value is empty in config, set + // the optional vaultNamespace. + if vaultAuthNamespace == "" { + vaultAuthNamespace = vaultNamespace + } // set the option if the value was not invalid if firstInit || !errors.Is(err, errConfigOptionMissing) { vaultConfig[api.EnvVaultNamespace] = vaultAuthNamespace