[manila-csi-plugin] support muilple share rules (#2915)

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

Author: silvacarloss

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

@@ -60,8 +60,8 @@ Parameter | Required | Description
 `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.
-`cephfs-clientID` | _no_ | Relevant for CephFS Manila shares. Specifies the cephx client ID when creating an access rule for the provisioned share. The same cephx client ID may be shared with multiple Manila shares. If no value is provided, client ID for the provisioned Manila share will be set to some unique value (PersistentVolume name).
-`nfs-shareClient` | _no_ | Relevant for NFS Manila shares. Specifies what address has access to the NFS share. Defaults to `0.0.0.0/0`, i.e. anyone.
+`cephfs-clientID` | _no_ | Relevant for CephFS Manila shares. Specifies the cephx client ID when creating an access rule for the provisioned share. The same cephx client ID may be shared with multiple Manila shares. If providing access to multiple cephx client IDs, set it as a comma separated list. If no value is provided, client ID for the provisioned Manila share will be set to some unique value (PersistentVolume name).
+`nfs-shareClient` | _no_ | Relevant for NFS Manila shares. Specifies what address has access to the NFS share. Use a comma separated list for granting access to multiple IP addresses or subnets. Defaults to `0.0.0.0/0`, i.e. anyone.
 
 ### Node Service volume context
 
@@ -71,7 +71,8 @@ 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
+`shareAccessID` | _no_ | The UUID of the access rule for the share. This parameter is being deprecated and replaced by `shareAccessIDs`.
+`shareAccessIDs` | _yes_ | Comma separated UUIDs of access rules 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: COMMA-SEPARATED-ACCESS-UUIDS-OF-THE-SHARE
 ---
 apiVersion: v1
 kind: PersistentVolumeClaim

pkg/csi/manila/adapters.go

@@ -19,6 +19,8 @@ package manila
 import (
 	"strings"
 
+	"github.com/gophercloud/gophercloud/v2/openstack/sharedfilesystems/v2/shares"
+	"k8s.io/cloud-provider-openstack/pkg/csi/manila/options"
 	"k8s.io/cloud-provider-openstack/pkg/csi/manila/shareadapters"
 	"k8s.io/klog/v2"
 )
@@ -35,3 +37,40 @@ func getShareAdapter(proto string) shareadapters.ShareAdapter {
 
 	return nil
 }
+
+func getAccessIDs(shareOpts *options.NodeVolumeContext) []string {
+	if shareOpts.ShareAccessIDs != "" {
+		// Split by comma if multiple
+		return strings.Split(shareOpts.ShareAccessIDs, ",")
+	} else if shareOpts.ShareAccessID != "" {
+		// Backwards compatibility: treat as single-element list
+		return []string{shareOpts.ShareAccessID}
+	}
+	return nil
+}
+
+func getAccessRightBasedOnShareAdapter(shareAdapter shareadapters.ShareAdapter, accessRights []shares.AccessRight, shareOpts *options.NodeVolumeContext) (accessRight *shares.AccessRight) {
+	switch shareAdapter.(type) {
+	case *shareadapters.Cephfs:
+		shareAccessIDs := getAccessIDs(shareOpts)
+		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)
 	opts := &shareadapters.VolumeContextArgs{
 		Locations: availableExportLocations,
 		Share:     share,

pkg/csi/manila/options/shareoptions.go

@@ -41,9 +41,10 @@ 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"`
+	ShareAccessID  string `name:"shareAccessID" value:"optionalIf:shareAccessIDs=." precludes:"shareAccessIDs"` // Keep this for backwards compatibility
+	ShareAccessIDs string `name:"shareAccessIDs" value:"optionalIf:shareAccessID=." precludes:"shareAccessID"`
 
 	// Adapter options
 

pkg/csi/manila/shareadapters/cephfs.go

@@ -34,82 +34,77 @@ 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
-			}
+	// TODO: add support for getting the exact client ID that the node will use.
+	// For now, we use the first client ID in the list and it should be enough,
+	// considering our context with the nodes.
+	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 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)
-		if err != nil {
-			return false, err
-		}
-
-		var accessRight *shares.AccessRight
-
-		for i := range rights {
-			if rights[i].AccessTo == accessTo {
-				accessRight = &rights[i]
-				break
+		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 != "" {
+						accessRight = &r
+						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)
 			}
 		}
+	}
+	return []shares.AccessRight{*accessRight}, nil
 
-		if accessRight == nil {
-			return false, fmt.Errorf("cannot find the access right we've just created")
-		}
-
-		return accessRight.AccessKey != "", nil
-	})
 }
 
 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,33 @@ 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, ",")
+
+	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 {
+			right, 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)
+			}
+			rights = append(rights, *right)
 		}
 	}
 
-	// Not found, create it
-
-	return args.ManilaClient.GrantAccess(ctx, args.Share.ID, shares.GrantAccessOpts{
-		AccessType:  "ip",
-		AccessLevel: "rw",
-		AccessTo:    args.Options.NFSShareClient,
-	})
+	return rights, nil
 }
 
 func (NFS) BuildVolumeContext(args *VolumeContextArgs) (volumeContext map[string]string, err error) {

pkg/csi/manila/shareadapters/shareadapter.go

@@ -46,10 +46,11 @@ type SecretArgs struct {
 }
 
 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.
+	// GetOrGrantAccesses first tries to retrieve the list of access rights for args.Share.
+	// It iterates over the list of access clients that should have access to the share considering nfs-shareClient or cephfs-clientID.
+	// The 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,