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 <[email protected]>
ceph · Feb 18, 2025 · f4c123b · f4c123b
1 parent b05d467
commit f4c123b
Show file tree

Hide file tree

Showing 4 changed files with 186 additions and 1 deletion.
diff --git 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
@@ -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
@@ -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
@@ -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)
+			}
+		})
+	}
+}