[manila-csi-plugin] support muilple share rules

Signed-off-by: moonek <[email protected]>
Signed-off-by: Carlos da Silva <[email protected]>

Author: moonek

docs/manila-csi-plugin/using-manila-csi-plugin.md

@@ -71,7 +71,7 @@ Parameter | Required | Description
 ----------|----------|------------
 `shareID` | if `shareName` is not given | The UUID of the share
 `shareName` | if `shareID` is not given | The name of the share
-`shareAccessID` | _yes_ | The UUID of the access rule for the share
+`shareAccessIDs` | _yes_ | The UUID of the access rule for the share
 `cephfs-mounter` | _no_ | Relevant for CephFS Manila shares. Specifies which mounting method to use with the CSI CephFS driver. Available options are `kernel` and `fuse`, defaults to `fuse`. See [CSI CephFS docs](https://github.com/ceph/ceph-csi/blob/csi-v1.0/docs/deploy-cephfs.md#configuration) for further information.
 `cephfs-kernelMountOptions` | _no_ | Relevant for CephFS Manila shares. Specifies mount options for CephFS kernel client. See [CSI CephFS docs](https://github.com/ceph/ceph-csi/blob/csi-v1.0/docs/deploy-cephfs.md#configuration) for further information.
 `cephfs-fuseMountOptions` | _no_ | Relevant for CephFS Manila shares. Specifies mount options for CephFS FUSE client. See [CSI CephFS docs](https://github.com/ceph/ceph-csi/blob/csi-v1.0/docs/deploy-cephfs.md#configuration) for further information.

examples/manila-csi-plugin/nfs/static-provisioning/preprovisioned-pvc.yaml

@@ -20,7 +20,7 @@ spec:
       namespace: default
     volumeAttributes:
       shareID: SHARE-UUID-GOES-HERE
-      shareAccessID: ACCESS-UUID-OF-THE-SHARE
+      shareAccessIDs: ACCESS-UUID-OF-THE-SHARE
 ---
 apiVersion: v1
 kind: PersistentVolumeClaim

pkg/csi/manila/adapters.go

@@ -19,6 +19,7 @@ package manila
 import (
 	"strings"
 
+	"github.com/gophercloud/gophercloud/v2/openstack/sharedfilesystems/v2/shares"
 	"k8s.io/cloud-provider-openstack/pkg/csi/manila/shareadapters"
 	"k8s.io/klog/v2"
 )
@@ -35,3 +36,29 @@ func getShareAdapter(proto string) shareadapters.ShareAdapter {
 
 	return nil
 }
+
+func getAccessRightBasedOnShareAdapter(shareAdapter shareadapters.ShareAdapter, accessRights []shares.AccessRight, shareAccessIDs string) (accessRight *shares.AccessRight) {
+	switch shareAdapter.(type) {
+	case *shareadapters.Cephfs:
+		shareAccessIDs := strings.Split(shareAccessIDs, ",")
+		for _, accessRightID := range shareAccessIDs {
+			for _, accessRight := range accessRights {
+				if accessRight.ID == accessRightID {
+					// TODO: we should add support for getting the node's own IP or Ceph
+					// user to avoid unnecessary access rights processing. All the node
+					// needs is one cephx user/key to mount the share, so we can return
+					// the first access right that matches the share access IDs list.
+					return &accessRight
+				}
+			}
+		}
+		klog.Fatalf("failed to find access rights %s for cephfs share", shareAccessIDs)
+	case *shareadapters.NFS:
+		// For NFS, we don't need to use an access right specifically. The controller is
+		// already making sure the access rules are properly created.
+		return nil
+	default:
+		klog.Fatalf("unknown share adapter type %T", shareAdapter)
+	}
+	return nil
+}

pkg/csi/manila/controllerserver.go

@@ -194,18 +194,24 @@ func (cs *controllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol
 
 	ad := getShareAdapter(shareOpts.Protocol)
 
-	accessRight, err := ad.GetOrGrantAccess(ctx, &shareadapters.GrantAccessArgs{Share: share, ManilaClient: manilaClient, Options: shareOpts})
+	accessRights, err := ad.GetOrGrantAccesses(ctx, &shareadapters.GrantAccessArgs{Share: share, ManilaClient: manilaClient, Options: shareOpts})
 	if err != nil {
 		if wait.Interrupted(err) {
-			return nil, status.Errorf(codes.DeadlineExceeded, "deadline exceeded while waiting for access rule %s for volume %s to become available", accessRight.ID, share.Name)
+			return nil, status.Errorf(codes.DeadlineExceeded, "deadline exceeded while waiting for access rules for volume %s to become available", share.Name)
 		}
 
 		return nil, status.Errorf(codes.Internal, "failed to grant access to volume %s: %v", share.Name, err)
 	}
 
+	var accessRightIDs []string
+	for _, ar := range accessRights {
+		accessRightIDs = append(accessRightIDs, ar.ID)
+	}
+	shareAccessIDs := strings.Join(accessRightIDs, ",")
+
 	volCtx := filterParametersForVolumeContext(params, options.NodeVolumeContextFields())
 	volCtx = util.SetMapIfNotEmpty(volCtx, "shareID", share.ID)
-	volCtx = util.SetMapIfNotEmpty(volCtx, "shareAccessID", accessRight.ID)
+	volCtx = util.SetMapIfNotEmpty(volCtx, "shareAccessIDs", shareAccessIDs)
 	volCtx = util.SetMapIfNotEmpty(volCtx, "groupID", share.ShareGroupID)
 	volCtx = util.SetMapIfNotEmpty(volCtx, "affinity", shareOpts.Affinity)
 	volCtx = util.SetMapIfNotEmpty(volCtx, "antiAffinity", shareOpts.AntiAffinity)

pkg/csi/manila/nodeserver.go

@@ -108,18 +108,6 @@ func (ns *nodeServer) buildVolumeContext(ctx context.Context, volID volumeID, sh
 		return nil, nil, status.Errorf(codes.Internal, "failed to list access rights for volume %s: %v", volID, err)
 	}
 
-	for i := range accessRights {
-		if accessRights[i].ID == shareOpts.ShareAccessID {
-			accessRight = &accessRights[i]
-			break
-		}
-	}
-
-	if accessRight == nil {
-		return nil, nil, status.Errorf(codes.InvalidArgument, "cannot find access right %s for volume %s",
-			shareOpts.ShareAccessID, volID)
-	}
-
 	// Retrieve list of all export locations for this share.
 	// Share adapter will try to choose the correct one for mounting.
 
@@ -131,6 +119,7 @@ func (ns *nodeServer) buildVolumeContext(ctx context.Context, volID volumeID, sh
 	// Build volume context for fwd plugin
 
 	sa := getShareAdapter(ns.d.shareProto)
+	accessRight = getAccessRightBasedOnShareAdapter(sa, accessRights, shareOpts.ShareAccessIDs)
 	opts := &shareadapters.VolumeContextArgs{
 		Locations: availableExportLocations,
 		Options:   shareOpts,

pkg/csi/manila/options/shareoptions.go

@@ -41,9 +41,9 @@ type ControllerVolumeContext struct {
 }
 
 type NodeVolumeContext struct {
-	ShareID       string `name:"shareID" value:"optionalIf:shareName=." precludes:"shareName"`
-	ShareName     string `name:"shareName" value:"optionalIf:shareID=." precludes:"shareID"`
-	ShareAccessID string `name:"shareAccessID"`
+	ShareID        string `name:"shareID" value:"optionalIf:shareName=." precludes:"shareName"`
+	ShareName      string `name:"shareName" value:"optionalIf:shareID=." precludes:"shareID"`
+	ShareAccessIDs string `name:"shareAccessIDs"`
 
 	// Adapter options
 

pkg/csi/manila/shareadapters/cephfs.go

@@ -19,6 +19,7 @@ package shareadapters
 import (
 	"context"
 	"fmt"
+	"strings"
 	"time"
 
 	"github.com/gophercloud/gophercloud/v2"
@@ -32,82 +33,89 @@ type Cephfs struct{}
 
 var _ ShareAdapter = &Cephfs{}
 
-func (Cephfs) GetOrGrantAccess(ctx context.Context, args *GrantAccessArgs) (accessRight *shares.AccessRight, err error) {
+func (Cephfs) GetOrGrantAccesses(ctx context.Context, args *GrantAccessArgs) ([]shares.AccessRight, error) {
 	// First, check if the access right exists or needs to be created
 
-	var rights []shares.AccessRight
-
-	accessTo := args.Options.CephfsClientID
-	if accessTo == "" {
-		accessTo = args.Share.Name
-	}
-
-	rights, err = args.ManilaClient.GetAccessRights(ctx, args.Share.ID)
+	rights, err := args.ManilaClient.GetAccessRights(ctx, args.Share.ID)
 	if err != nil {
 		if _, ok := err.(gophercloud.ErrResourceNotFound); !ok {
 			return nil, fmt.Errorf("failed to list access rights: %v", err)
 		}
-	} else {
-		// Try to find the access right
+	}
 
-		for _, r := range rights {
-			if r.AccessTo == accessTo && r.AccessType == "cephx" && r.AccessLevel == "rw" {
-				klog.V(4).Infof("cephx access right for share %s already exists", args.Share.Name)
+	accessToList := []string{args.Share.Name}
+	if args.Options.CephfsClientID != "" {
+		accessToList = strings.Split(args.Options.CephfsClientID, ",")
+	}
 
-				accessRight = &r
-				break
-			}
+	created := false
+	accessRightClient := accessToList[0]
+	var accessRight *shares.AccessRight
+
+	// Try to find the access right
+	for _, r := range rights {
+		if r.AccessTo == accessRightClient && r.AccessType == "cephx" && r.AccessLevel == "rw" {
+			klog.V(4).Infof("cephx access right for share %s already exists", args.Share.Name)
+			accessRight = &r
+			break
 		}
 	}
 
+	// Not found, create it
 	if accessRight == nil {
-		// Not found, create it
-
-		accessRight, err = args.ManilaClient.GrantAccess(ctx, args.Share.ID, shares.GrantAccessOpts{
+		result, err := args.ManilaClient.GrantAccess(ctx, args.Share.ID, shares.GrantAccessOpts{
 			AccessType:  "cephx",
 			AccessLevel: "rw",
-			AccessTo:    accessTo,
+			AccessTo:    accessRightClient,
 		})
-
 		if err != nil {
-			return
+			return nil, fmt.Errorf("failed to grant access right: %v", err)
 		}
+		if result.AccessKey == "" {
+			// Wait till a ceph key is assigned to the access right
+			backoff := wait.Backoff{
+				Duration: time.Second * 5,
+				Factor:   1.2,
+				Steps:    10,
+			}
+			wait_err := wait.ExponentialBackoff(backoff, func() (bool, error) {
+				rights, err := args.ManilaClient.GetAccessRights(ctx, args.Share.ID)
+				if err != nil {
+					return false, fmt.Errorf("error get access rights for share %s: %v", args.Share.ID, err)
+				}
+				if len(rights) == 0 {
+					return false, fmt.Errorf("cannot find the access right we've just created")
+				}
+				for _, r := range rights {
+					if r.AccessTo == accessRightClient && r.AccessKey != "" {
+						return true, nil
+					}
+				}
+				klog.V(4).Infof("Access key for %s is not set yet, retrying...", accessRightClient)
+				return false, nil
+			})
+			if wait_err != nil {
+				return nil, fmt.Errorf("timed out while attempting to get access rights for share %s: %v", args.Share.ID, err)
+			}
+		}
+		created = true
 	}
 
-	if accessRight.AccessKey != "" {
-		// The access right is ready
-		return
-	}
-
-	// Wait till a ceph key is assigned to the access right
-
-	backoff := wait.Backoff{
-		Duration: time.Second * 5,
-		Factor:   1.2,
-		Steps:    10,
-	}
-
-	return accessRight, wait.ExponentialBackoff(backoff, func() (bool, error) {
-		rights, err := args.ManilaClient.GetAccessRights(ctx, args.Share.ID)
+	// Search again because access rights have changed
+	if created {
+		rights, err = args.ManilaClient.GetAccessRights(ctx, args.Share.ID)
 		if err != nil {
-			return false, err
-		}
-
-		var accessRight *shares.AccessRight
-
-		for i := range rights {
-			if rights[i].AccessTo == accessTo {
-				accessRight = &rights[i]
-				break
+			if _, ok := err.(gophercloud.ErrResourceNotFound); !ok {
+				return nil, fmt.Errorf("failed to list access rights: %v", err)
 			}
 		}
-
-		if accessRight == nil {
-			return false, fmt.Errorf("cannot find the access right we've just created")
+		for _, r := range rights {
+			if r.AccessTo == accessRightClient && r.AccessKey != "" {
+				return []shares.AccessRight{r}, nil
+			}
 		}
-
-		return accessRight.AccessKey != "", nil
-	})
+	}
+	return nil, fmt.Errorf("access right not found after creation: %v", err)
 }
 
 func (Cephfs) BuildVolumeContext(args *VolumeContextArgs) (volumeContext map[string]string, err error) {

pkg/csi/manila/shareadapters/nfs.go

@@ -33,7 +33,7 @@ type NFS struct{}
 
 var _ ShareAdapter = &NFS{}
 
-func (NFS) GetOrGrantAccess(ctx context.Context, args *GrantAccessArgs) (*shares.AccessRight, error) {
+func (NFS) GetOrGrantAccesses(ctx context.Context, args *GrantAccessArgs) ([]shares.AccessRight, error) {
 	// First, check if the access right exists or needs to be created
 
 	rights, err := args.ManilaClient.GetAccessRights(ctx, args.Share.ID)
@@ -43,22 +43,43 @@ func (NFS) GetOrGrantAccess(ctx context.Context, args *GrantAccessArgs) (*shares
 		}
 	}
 
-	// Try to find the access right
-
-	for _, r := range rights {
-		if r.AccessTo == args.Options.NFSShareClient && r.AccessType == "ip" && r.AccessLevel == "rw" {
-			klog.V(4).Infof("IP access right for share %s already exists", args.Share.Name)
-			return &r, nil
+	accessToList := strings.Split(args.Options.NFSShareClient, ",")
+
+	created := false
+	for _, at := range accessToList {
+		// Try to find the access right
+		found := false
+		for _, r := range rights {
+			if r.AccessTo == at && r.AccessType == "ip" && r.AccessLevel == "rw" {
+				klog.V(4).Infof("IP access right %s for share %s already exists", at, args.Share.Name)
+				found = true
+				break
+			}
+		}
+		// Not found, create it
+		if !found {
+			_, err = args.ManilaClient.GrantAccess(ctx, args.Share.ID, shares.GrantAccessOpts{
+				AccessType:  "ip",
+				AccessLevel: "rw",
+				AccessTo:    at,
+			})
+			if err != nil {
+				return nil, fmt.Errorf("failed to grant access right: %v", err)
+			}
+			created = true
 		}
 	}
 
-	// Not found, create it
-
-	return args.ManilaClient.GrantAccess(ctx, args.Share.ID, shares.GrantAccessOpts{
-		AccessType:  "ip",
-		AccessLevel: "rw",
-		AccessTo:    args.Options.NFSShareClient,
-	})
+	// Search again because access rights have changed
+	if created {
+		rights, err = args.ManilaClient.GetAccessRights(ctx, args.Share.ID)
+		if err != nil {
+			if _, ok := err.(gophercloud.ErrResourceNotFound); !ok {
+				return nil, fmt.Errorf("failed to list access rights: %v", err)
+			}
+		}
+	}
+	return rights, nil
 }
 
 func (NFS) BuildVolumeContext(args *VolumeContextArgs) (volumeContext map[string]string, err error) {

pkg/csi/manila/shareadapters/shareadapter.go

@@ -46,7 +46,7 @@ type ShareAdapter interface {
 	// GetOrGrantAccess first tries to retrieve an access right for args.Share.
 	// An access right is created for the share in case it doesn't exist yet.
 	// Returns an existing or new access right for args.Share.
-	GetOrGrantAccess(ctx context.Context, args *GrantAccessArgs) (accessRight *shares.AccessRight, err error)
+	GetOrGrantAccesses(ctx context.Context, args *GrantAccessArgs) (accessRights []shares.AccessRight, err error)
 
 	// BuildVolumeContext builds a volume context map that's passed to NodeStageVolumeRequest and NodePublishVolumeRequest
 	BuildVolumeContext(args *VolumeContextArgs) (volumeContext map[string]string, err error)

tests/e2e/csi/manila/testdriver.go

@@ -197,8 +197,8 @@ func (d *manilaTestDriver) GetPersistentVolumeSource(readOnly bool, fsType strin
 			ReadOnly:     readOnly,
 			FSType:       fsType,
 			VolumeAttributes: map[string]string{
-				"shareID":       v.shareID,
-				"shareAccessID": v.accessID,
+				"shareID":        v.shareID,
+				"shareAccessIDs": v.accessID,
 			},
 			NodeStageSecretRef: &v1.SecretReference{
 				Name:      manilaSecretName,