rbd: make image features as optional parameter

Makes the rbd images features in the storageclass
as optional so that default image features of librbd
can be used. and also kept the option to user
to specify the image features in the storageclass.

Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>

internal/rbd/controllerserver.go

@@ -115,8 +115,8 @@ func (cs *ControllerServer) parseVolCreateRequest(
 			"multi node access modes are only supported on rbd `block` type volumes")
 	}
 
-	if imageFeatures, ok := req.GetParameters()["imageFeatures"]; checkImageFeatures(imageFeatures, ok, true) {
-		return nil, status.Error(codes.InvalidArgument, "missing required parameter imageFeatures")
+	if imageFeatures, ok := req.GetParameters()["imageFeatures"]; !checkValidImageFeatures(imageFeatures, ok) {
+		return nil, status.Error(codes.InvalidArgument, "empty imageFeatures parameter")
 	}
 
 	// if it's NOT SINGLE_NODE_WRITER, and it's BLOCK we'll set the parameter to ignore the in-use checks

internal/rbd/nodeserver.go

@@ -215,12 +215,33 @@ func populateRbdVol(
 		rv.RbdImageName = imageAttributes.ImageName
 	}
 
+	err = rv.Connect(cr)
+	if err != nil {
+		log.ErrorLog(ctx, "failed to connect to volume %s: %v", rv, err)
+
+		return nil, status.Error(codes.Internal, err.Error())
+	}
+	// in case of any error call Destroy for cleanup.
+	defer func() {
+		if err != nil {
+			rv.Destroy()
+		}
+	}()
+	// get the image details from the ceph cluster.
+	err = rv.getImageInfo()
+	if err != nil {
+		log.ErrorLog(ctx, "failed to get image details %s: %v", rv, err)
+
+		return nil, status.Error(codes.Internal, err.Error())
+	}
+
 	if req.GetVolumeContext()["mounter"] == rbdDefaultMounter &&
-		!isKrbdFeatureSupported(ctx, req.GetVolumeContext()["imageFeatures"]) {
+		!isKrbdFeatureSupported(ctx, strings.Join(rv.ImageFeatureSet.Names(), ",")) {
 		if !parseBoolOption(ctx, req.GetVolumeContext(), tryOtherMounters, false) {
 			log.ErrorLog(ctx, "unsupported krbd Feature, set `tryOtherMounters:true` or fix krbd driver")
+			err = errors.New("unsupported krbd Feature")
 
-			return nil, status.Errorf(codes.Internal, "unsupported krbd Feature")
+			return nil, status.Error(codes.Internal, err.Error())
 		}
 		// fallback to rbd-nbd,
 		rv.Mounter = rbdNbdMounter
@@ -299,24 +320,10 @@ func (ns *NodeServer) NodeStageVolume(
 	}
 
 	isStaticVol := parseBoolOption(ctx, req.GetVolumeContext(), staticVol, false)
-
-	// throw error when imageFeatures parameter is missing or empty
-	// for backward compatibility, ignore error for non-static volumes from older cephcsi version
-	if imageFeatures, ok := req.GetVolumeContext()["imageFeatures"]; checkImageFeatures(imageFeatures, ok, isStaticVol) {
-		return nil, status.Error(codes.InvalidArgument, "missing required parameter imageFeatures")
-	}
-
 	rv, err := populateRbdVol(ctx, req, cr, req.GetSecrets())
 	if err != nil {
 		return nil, err
 	}
-
-	err = rv.Connect(cr)
-	if err != nil {
-		log.ErrorLog(ctx, "failed to connect to volume %s: %v", rv, err)
-
-		return nil, status.Error(codes.Internal, err.Error())
-	}
 	defer rv.Destroy()
 
 	if isHealer {

internal/rbd/rbd_util.go

@@ -471,11 +471,10 @@ func (ri *rbdImage) isInUse() (bool, error) {
 	return len(watchers) > defaultWatchers, nil
 }
 
-// checkImageFeatures check presence of imageFeatures parameter. It returns true when
-// there imageFeatures is missing or empty, skips missing parameter for non-static volumes
-// for backward compatibility.
-func checkImageFeatures(imageFeatures string, ok, static bool) bool {
-	return static && (!ok || imageFeatures == "")
+// checkValidImageFeatures check presence of imageFeatures parameter. It returns false when
+// there imageFeatures is present and empty.
+func checkValidImageFeatures(imageFeatures string, ok bool) bool {
+	return !(ok && imageFeatures == "")
 }
 
 // isNotMountPoint checks whether MountPoint does not exists and

internal/rbd/rbd_util_test.go

@@ -354,3 +354,35 @@ func TestIsKrbdFeatureSupported(t *testing.T) {
 		})
 	}
 }
+
+func Test_checkValidImageFeatures(t *testing.T) {
+	t.Parallel()
+	tests := []struct {
+		name          string
+		imageFeatures string
+		ok            bool
+		want          bool
+	}{
+		{
+			name:          "test for valid image features",
+			imageFeatures: "layering,exclusive-lock,object-map,fast-diff,deep-flatten",
+			ok:            true,
+			want:          true,
+		},
+		{
+			name:          "test for empty image features",
+			imageFeatures: "",
+			ok:            true,
+			want:          false,
+		},
+	}
+	for _, tt := range tests {
+		tc := tt
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			if got := checkValidImageFeatures(tc.imageFeatures, tc.ok); got != tc.want {
+				t.Errorf("checkValidImageFeatures() = %v, want %v", got, tc.want)
+			}
+		})
+	}
+}