From ac8cda5e37187a83d9a70c2acd91f32ad1189478 Mon Sep 17 00:00:00 2001 From: Niels de Vos Date: Mon, 17 Feb 2025 16:01:42 +0100 Subject: [PATCH] rbd: add validation to ToCSI() for rbdVolume and rbdSnapshot After an unfortnate timed restart of the provisioner, a volume that got cloned did not get a `rbdVolume.VolID` set. The `.VolID` is used as the CSI Volume Handle, and is a required attribute. The `rbdVolume` and `rbdSnapshot` structs have a `.ToCSI()` function that can do the validation of required attributes. This is now added, including unit-tests. Signed-off-by: Niels de Vos --- internal/rbd/controllerserver.go | 11 ++++ internal/rbd/controllerserver_test.go | 83 +++++++++++++++++++++++++- internal/rbd/snapshot.go | 7 +++ internal/rbd/snapshot_test.go | 86 +++++++++++++++++++++++++++ 4 files changed, 186 insertions(+), 1 deletion(-) create mode 100644 internal/rbd/snapshot_test.go diff --git a/internal/rbd/controllerserver.go b/internal/rbd/controllerserver.go index fab44ae30..ab99c0560 100644 --- a/internal/rbd/controllerserver.go +++ b/internal/rbd/controllerserver.go @@ -250,6 +250,17 @@ func (cs *ControllerServer) parseVolCreateRequest( } func (rbdVol *rbdVolume) ToCSI(ctx context.Context) (*csi.Volume, error) { + switch { + case rbdVol.VolID == "": + return nil, fmt.Errorf("%q does not have a volume-id set", rbdVol) + case rbdVol.Pool == "": + return nil, fmt.Errorf("%q does not have a pool set", rbdVol) + case rbdVol.JournalPool == "": + return nil, fmt.Errorf("%q does not have a journal-pool set", rbdVol) + case rbdVol.RbdImageName == "": + return nil, fmt.Errorf("%q does not have an image-name set", rbdVol) + } + vol := &csi.Volume{ VolumeId: rbdVol.VolID, CapacityBytes: rbdVol.VolSize, diff --git a/internal/rbd/controllerserver_test.go b/internal/rbd/controllerserver_test.go index b9acdae55..d7d55ca0d 100644 --- a/internal/rbd/controllerserver_test.go +++ b/internal/rbd/controllerserver_test.go @@ -16,7 +16,10 @@ limitations under the License. package rbd -import "testing" +import ( + "context" + "testing" +) func TestValidateStriping(t *testing.T) { t.Parallel() @@ -85,3 +88,81 @@ func TestValidateStriping(t *testing.T) { }) } } + +func TestToCSIVolume(t *testing.T) { + t.Parallel() + tests := []struct { + name string + rv *rbdVolume + wantErr bool + }{ + { + name: "all attributes set", + rv: &rbdVolume{ + rbdImage: rbdImage{ + VolID: "0001-unique-volume-id", + Pool: "ecpool", + JournalPool: "replicapool", + RbdImageName: "csi-vol-01234-5678-90abc", + }, + }, + wantErr: false, + }, + { + name: "missing volume-id", + rv: &rbdVolume{ + rbdImage: rbdImage{ + VolID: "", + Pool: "ecpool", + JournalPool: "replicapool", + RbdImageName: "csi-vol-01234-5678-90abc", + }, + }, + wantErr: true, + }, + { + name: "missing pool", + rv: &rbdVolume{ + rbdImage: rbdImage{ + VolID: "0001-unique-volume-id", + Pool: "", + JournalPool: "replicapool", + RbdImageName: "csi-vol-01234-5678-90abc", + }, + }, + wantErr: true, + }, + { + name: "missing journal-pool", + rv: &rbdVolume{ + rbdImage: rbdImage{ + VolID: "0001-unique-volume-id", + Pool: "ecpool", + JournalPool: "", + RbdImageName: "csi-vol-01234-5678-90abc", + }, + }, + wantErr: true, + }, + { + name: "missing image-name", + rv: &rbdVolume{ + rbdImage: rbdImage{ + VolID: "0001-unique-volume-id", + Pool: "ecpool", + JournalPool: "", + RbdImageName: "", + }, + }, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + if _, err := tt.rv.ToCSI(context.TODO()); (err != nil) != tt.wantErr { + t.Errorf("ToCSI() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} diff --git a/internal/rbd/snapshot.go b/internal/rbd/snapshot.go index d47d3d31c..e35a68ecb 100644 --- a/internal/rbd/snapshot.go +++ b/internal/rbd/snapshot.go @@ -147,6 +147,13 @@ func (rbdSnap *rbdSnapshot) toVolume() *rbdVolume { } func (rbdSnap *rbdSnapshot) ToCSI(ctx context.Context) (*csi.Snapshot, error) { + switch { + case rbdSnap.VolID == "": + return nil, fmt.Errorf("%q does not have a volume-id set", rbdSnap) + case rbdSnap.SourceVolumeID == "": + return nil, fmt.Errorf("%q does not have a source-volume-id set", rbdSnap) + } + created, err := rbdSnap.GetCreationTime(ctx) if err != nil { return nil, err diff --git a/internal/rbd/snapshot_test.go b/internal/rbd/snapshot_test.go new file mode 100644 index 000000000..0ce21b1fa --- /dev/null +++ b/internal/rbd/snapshot_test.go @@ -0,0 +1,86 @@ +/* +Copyright 2025 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 rbd + +import ( + "context" + "testing" + "time" +) + +func TestToCSISnapshot(t *testing.T) { + t.Parallel() + now := time.Now() + tests := []struct { + name string + rs *rbdSnapshot + wantErr bool + }{ + { + name: "all attributes set", + rs: &rbdSnapshot{ + rbdImage: rbdImage{ + VolID: "0001-unique-snapshot-id", + CreatedAt: &now, + }, + SourceVolumeID: "0001-unique-volume-id", + }, + wantErr: false, + }, + { + name: "missing volume-id", + rs: &rbdSnapshot{ + rbdImage: rbdImage{ + VolID: "", + CreatedAt: &now, + }, + SourceVolumeID: "0001-unique-volume-id", + }, + wantErr: true, + }, + { + name: "missing source-volume-id", + rs: &rbdSnapshot{ + rbdImage: rbdImage{ + VolID: "0001-unique-snapshot-id", + CreatedAt: &now, + }, + SourceVolumeID: "", + }, + wantErr: true, + }, + { + name: "missing creation-time", + rs: &rbdSnapshot{ + rbdImage: rbdImage{ + VolID: "0001-unique-snapshot-id", + CreatedAt: nil, + }, + SourceVolumeID: "0001-unique-volume-id", + }, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + if _, err := tt.rs.ToCSI(context.TODO()); (err != nil) != tt.wantErr { + t.Errorf("ToCSI() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +}