Repository: ceph/ceph-csi
Pull request #2135 (2021-06-05 14:46:36)

Fix regression when no image features are provided
# Describe what this PR does #
There is an issue that if the storage class was originally created with no image features (which worked fine), it would fail when upgrading the Ceph CSI.

The regression was introduced by this commit:

https://github.com/ceph/ceph-csi/commit/d8f7b38d3dac9718695ff6c33328f5f6cfce2c2f

Several users have filed issues about this, see listed in related issues below.

## Is there anything that requires special attention ##

This is a regression which really hurts when upgrading (and only noticed when remounting volumes).  It's incredibly hard to fix in production because PVs are read-only.  Users could potentially lose their data if they try and force a replace as the PV is recreated.

## Related issues ##

Fixes: #1937 
Fixes: #2049 

## Future concerns ##

None for now.

mnaser: I've added a failing test, I'm investigating the fix now, but it's 100% a problem with that test, seeing locally:

```
$ go test ./internal/rbd
--- FAIL: TestValidateImageFeatures (0.00s)
    rbd_util_test.go:134: 
                Error Trace:    rbd_util_test.go:134
                Error:          Expected nil, but got: &errors.errorString{s:"invalid feature "}
                Test:           TestValidateImageFeatures
FAIL
FAIL    github.com/ceph/ceph-csi/internal/rbd   0.048s
FAIL
```
mnaser: OK, so it seems like an empty string being split will result in at least one item:

```
package main

import (
	"fmt"
	"strings"
)

func main() {
	arr := strings.Split("", ",")
	for x, _ := range arr {
		fmt.Print(x)
	}
}
```
mnaser: This is ready and good to go, I believe it should be a quite high priority fix as it's a regression.
nixpanic: Thanks for the PR! Please note that we expect commit messages to have a prefix of the affected component. In this case, you can add `rbd: ` as a prefix. Please update the commit messages and force-push the updated commits to your fork+branch again.

More details about this can be found in the [development guide](https://github.com/ceph/ceph-csi/blob/devel/docs/development-guide.md).
mnaser: Thanks for the reviews.  I've made the required modifications..
nixpanic: /retest ci/centos/mini-e2e-helm/k8s-1.19
Madhu-1: 
/retest ci/centos/mini-e2e-helm/k8s-1.19
Madhu-1: /retest ci/centos/mini-e2e-helm/k8s-1.19
humblec: /retest ci/centos/k8s-e2e-external-storage/1.20
Madhu-1: /retest all
nixpanic: @mergifyio rebase
mergify[bot]: **Command `rebase`: success**
> **Branch has been successfully rebased**
> 


Patch:
diff --git a/internal/rbd/rbd_util.go b/internal/rbd/rbd_util.go
index c11c6468b..0053c76ed 100644
--- a/internal/rbd/rbd_util.go
+++ b/internal/rbd/rbd_util.go
@@ -993,6 +993,13 @@ func genVolFromVolumeOptions(ctx context.Context, volOptions, credentials map[st
 }
 
 func (rv *rbdVolume) validateImageFeatures(imageFeatures string) error {
+	// It is possible for image features to be an empty string which
+	// the Go split function would return a single item array with
+	// an empty string, causing a failure when trying to validate
+	// the features.
+	if strings.TrimSpace(imageFeatures) == "" {
+		return nil
+	}
 	arr := strings.Split(imageFeatures, ",")
 	featureSet := sets.NewString(arr...)
 	for _, f := range arr {


Test patch:
diff --git a/internal/rbd/rbd_util_test.go b/internal/rbd/rbd_util_test.go
index b8122585a..166cb460b 100644
--- a/internal/rbd/rbd_util_test.go
+++ b/internal/rbd/rbd_util_test.go
@@ -51,6 +51,14 @@ func TestValidateImageFeatures(t *testing.T) {
 		isErr         bool
 		errMsg        string
 	}{
+		{
+			"",
+			&rbdVolume{
+				Mounter: rbdDefaultMounter,
+			},
+			false,
+			"",
+		},
 		{
 			"layering",
 			&rbdVolume{