[manila-csi-plugin] Support mutable NFS access rules

charts/manila-csi-plugin/templates/controllerplugin-rules-clusterrole.yaml

@@ -27,6 +27,9 @@ rules:
   - apiGroups: ["storage.k8s.io"]
     resources: ["storageclasses"]
     verbs: ["get", "list", "watch"]
+  - apiGroups: ["storage.k8s.io"]
+    resources: ["volumeattributesclasses"]
+    verbs: ["get", "list", "watch"]
   - apiGroups: ["storage.k8s.io"]
     resources: ["csinodes"]
     verbs: ["get", "list", "watch"]

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

@@ -64,6 +64,27 @@ Parameter | Required | Description
 `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.
 
+### Mutable NFS access rules
+
+For NFS shares on Kubernetes 1.34 or newer, `nfs-shareClient` can be changed after
+provisioning by assigning a `VolumeAttributesClass` to the PVC. The value is treated as the desired set of `rw`
+IP access rules: missing rules are granted before obsolete rules are revoked. Other
+access types and `ro` rules are left unchanged.
+
+```yaml
+apiVersion: storage.k8s.io/v1
+kind: VolumeAttributesClass
+metadata:
+  name: manila-nfs-private
+driverName: nfs.manila.csi.openstack.org
+parameters:
+  nfs-shareClient: 10.0.0.0/24,192.0.2.10
+```
+
+Set `spec.volumeAttributesClassName` on the PVC to this class. To change the list
+again, create another `VolumeAttributesClass` and update the PVC to refer to it.
+An empty `nfs-shareClient` value revokes all `rw` IP access rules.
+
 ### Node Service volume context
 
 _Kubernetes PV CSI volume attributes for pre-provisioned volumes_

pkg/csi/manila/controllerserver.go

@@ -19,6 +19,8 @@ package manila
 import (
 	"context"
 	"encoding/json"
+	"fmt"
+	"net"
 	"strings"
 	"sync"
 
@@ -91,6 +93,19 @@ func (cs *controllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol
 	if params == nil {
 		params = make(map[string]string)
 	}
+	if cs.d.shareProto != "NFS" && len(req.GetMutableParameters()) != 0 {
+		return nil, status.Error(codes.InvalidArgument, "mutable parameters are only supported for NFS volumes")
+	}
+	for key, value := range req.GetMutableParameters() {
+		if key != "nfs-shareClient" {
+			return nil, status.Errorf(codes.InvalidArgument, "unsupported mutable parameter %q", key)
+		}
+		accessToList, err := parseNFSShareClients(value)
+		if err != nil {
+			return nil, status.Errorf(codes.InvalidArgument, "invalid nfs-shareClient: %v", err)
+		}
+		params[key] = strings.Join(accessToList, ",")
+	}
 
 	params["protocol"] = cs.d.shareProto
 
@@ -227,8 +242,79 @@ func (cs *controllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol
 	}, nil
 }
 
-func (d *controllerServer) ControllerModifyVolume(ctx context.Context, req *csi.ControllerModifyVolumeRequest) (*csi.ControllerModifyVolumeResponse, error) {
-	return nil, status.Error(codes.Unimplemented, "")
+func (cs *controllerServer) ControllerModifyVolume(ctx context.Context, req *csi.ControllerModifyVolumeRequest) (*csi.ControllerModifyVolumeResponse, error) {
+	if cs.d.shareProto != "NFS" {
+		return nil, status.Error(codes.InvalidArgument, "mutable parameters are only supported for NFS volumes")
+	}
+	if req.GetVolumeId() == "" {
+		return nil, status.Error(codes.InvalidArgument, "volume ID cannot be empty")
+	}
+	if len(req.GetSecrets()) == 0 {
+		return nil, status.Error(codes.InvalidArgument, "secrets cannot be nil or empty")
+	}
+
+	mutableParameters := req.GetMutableParameters()
+	if len(mutableParameters) == 0 {
+		return nil, status.Error(codes.InvalidArgument, "mutable parameters cannot be nil or empty")
+	}
+	for key := range mutableParameters {
+		if key != "nfs-shareClient" {
+			return nil, status.Errorf(codes.InvalidArgument, "unsupported mutable parameter %q", key)
+		}
+	}
+
+	accessToList, err := parseNFSShareClients(mutableParameters["nfs-shareClient"])
+	if err != nil {
+		return nil, status.Errorf(codes.InvalidArgument, "invalid nfs-shareClient: %v", err)
+	}
+
+	osOpts, err := options.NewOpenstackOptions(req.GetSecrets())
+	if err != nil {
+		return nil, status.Errorf(codes.InvalidArgument, "invalid OpenStack secrets: %v", err)
+	}
+
+	manilaClient, err := cs.d.manilaClientBuilder.New(ctx, osOpts)
+	if err != nil {
+		return nil, status.Errorf(codes.Unauthenticated, "failed to create Manila v2 client: %v", err)
+	}
+
+	share, err := manilaClient.GetShareByID(ctx, req.GetVolumeId())
+	if err != nil {
+		if clouderrors.IsNotFound(err) {
+			return nil, status.Errorf(codes.NotFound, "volume %s not found: %v", req.GetVolumeId(), err)
+		}
+		return nil, status.Errorf(codes.Internal, "failed to retrieve volume %s: %v", req.GetVolumeId(), err)
+	}
+	if !compareProtocol(share.ShareProto, "NFS") {
+		return nil, status.Errorf(codes.InvalidArgument, "nfs-shareClient cannot be applied to a %s volume", share.ShareProto)
+	}
+
+	if _, isPending := pendingVolumes.LoadOrStore(share.Name, true); isPending {
+		return nil, status.Errorf(codes.Aborted, "volume %s is already being processed", share.Name)
+	}
+	defer pendingVolumes.Delete(share.Name)
+
+	if err := (shareadapters.NFS{}).ReconcileAccesses(ctx, manilaClient, share.ID, accessToList); err != nil {
+		if wait.Interrupted(err) {
+			return nil, status.Errorf(codes.DeadlineExceeded, "deadline exceeded while updating access rules for volume %s", share.Name)
+		}
+		return nil, status.Errorf(codes.Internal, "failed to update access rules for volume %s: %v", share.Name, err)
+	}
+
+	return &csi.ControllerModifyVolumeResponse{}, nil
+}
+
+func parseNFSShareClients(value string) ([]string, error) {
+	clients := util.Unique(util.SplitTrim(value, ','))
+	for _, client := range clients {
+		if net.ParseIP(client) != nil {
+			continue
+		}
+		if _, _, err := net.ParseCIDR(client); err != nil {
+			return nil, fmt.Errorf("%q is not an IP address or CIDR", client)
+		}
+	}
+	return clients, nil
 }
 
 func (cs *controllerServer) DeleteVolume(ctx context.Context, req *csi.DeleteVolumeRequest) (*csi.DeleteVolumeResponse, error) {

pkg/csi/manila/controllerserver_test.go

@@ -14,12 +14,128 @@ limitations under the License.
 package manila
 
 import (
+	"context"
 	"fmt"
+	"reflect"
 	"testing"
 
+	csispec "github.com/container-storage-interface/spec/lib/go/csi"
+	"github.com/gophercloud/gophercloud/v2/openstack/sharedfilesystems/v2/shares"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
+	"k8s.io/cloud-provider-openstack/pkg/client"
 	"k8s.io/cloud-provider-openstack/pkg/csi"
+	"k8s.io/cloud-provider-openstack/pkg/csi/manila/manilaclient"
 )
 
+func TestParseNFSShareClients(t *testing.T) {
+	tests := []struct {
+		name    string
+		value   string
+		want    []string
+		wantErr bool
+	}{
+		{name: "addresses and CIDRs", value: " 10.0.0.1, 192.0.2.0/24,2001:db8::1 ", want: []string{"10.0.0.1", "192.0.2.0/24", "2001:db8::1"}},
+		{name: "duplicates", value: "10.0.0.1,10.0.0.1", want: []string{"10.0.0.1"}},
+		{name: "empty revokes all", value: "", want: []string{}},
+		{name: "invalid", value: "not-an-address", wantErr: true},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			got, err := parseNFSShareClients(tc.value)
+			if (err != nil) != tc.wantErr {
+				t.Fatalf("unexpected error: %v", err)
+			}
+			if !reflect.DeepEqual(got, tc.want) {
+				t.Fatalf("got %#v, want %#v", got, tc.want)
+			}
+		})
+	}
+}
+
+type modifyVolumeTestClient struct {
+	manilaclient.Interface
+	share        *shares.Share
+	getCalls     int
+	granted      []shares.GrantAccessOpts
+	revoked      []string
+	accessRights [][]shares.AccessRight
+}
+
+func (c *modifyVolumeTestClient) GetShareByID(context.Context, string) (*shares.Share, error) {
+	return c.share, nil
+}
+
+func (c *modifyVolumeTestClient) GetAccessRights(context.Context, string) ([]shares.AccessRight, error) {
+	idx := c.getCalls
+	if idx >= len(c.accessRights) {
+		idx = len(c.accessRights) - 1
+	}
+	c.getCalls++
+	return c.accessRights[idx], nil
+}
+
+func (c *modifyVolumeTestClient) GrantAccess(_ context.Context, _ string, opts shares.GrantAccessOptsBuilder) (*shares.AccessRight, error) {
+	grantOpts := opts.(shares.GrantAccessOpts)
+	c.granted = append(c.granted, grantOpts)
+	return &shares.AccessRight{ID: "new-rule"}, nil
+}
+
+func (c *modifyVolumeTestClient) RevokeAccess(_ context.Context, _ string, accessID string) error {
+	c.revoked = append(c.revoked, accessID)
+	return nil
+}
+
+type modifyVolumeTestBuilder struct {
+	client manilaclient.Interface
+}
+
+func (b *modifyVolumeTestBuilder) New(context.Context, *client.AuthOpts) (manilaclient.Interface, error) {
+	return b.client, nil
+}
+
+func TestControllerModifyVolumeReconcilesNFSAccessRules(t *testing.T) {
+	manilaClient := &modifyVolumeTestClient{
+		share: &shares.Share{ID: "share-1", Name: "pvc-share", ShareProto: "NFS"},
+		accessRights: [][]shares.AccessRight{
+			{{ID: "old-rule", AccessType: "ip", AccessLevel: "rw", AccessTo: "192.0.2.0/24", State: "active"}},
+			{{ID: "new-rule", AccessType: "ip", AccessLevel: "rw", AccessTo: "10.0.0.0/24", State: "active"}},
+		},
+	}
+	server := &controllerServer{d: &Driver{
+		shareProto:          "NFS",
+		manilaClientBuilder: &modifyVolumeTestBuilder{client: manilaClient},
+	}}
+
+	_, err := server.ControllerModifyVolume(context.Background(), &csispec.ControllerModifyVolumeRequest{
+		VolumeId:          "share-1",
+		Secrets:           map[string]string{"os-region": "RegionOne"},
+		MutableParameters: map[string]string{"nfs-shareClient": "10.0.0.0/24"},
+	})
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if len(manilaClient.granted) != 1 || manilaClient.granted[0].AccessTo != "10.0.0.0/24" {
+		t.Fatalf("unexpected grants: %#v", manilaClient.granted)
+	}
+	if !reflect.DeepEqual(manilaClient.revoked, []string{"old-rule"}) {
+		t.Fatalf("unexpected revocations: %#v", manilaClient.revoked)
+	}
+}
+
+func TestControllerModifyVolumeRejectsUnsupportedParameter(t *testing.T) {
+	server := &controllerServer{d: &Driver{shareProto: "NFS"}}
+	_, err := server.ControllerModifyVolume(context.Background(), &csispec.ControllerModifyVolumeRequest{
+		VolumeId:          "share-1",
+		Secrets:           map[string]string{"os-region": "RegionOne"},
+		MutableParameters: map[string]string{"unsupported": "value"},
+	})
+	if status.Code(err) != codes.InvalidArgument {
+		t.Fatalf("got %v, want InvalidArgument", err)
+	}
+}
+
 func TestPrepareShareMetadata(t *testing.T) {
 	ts := []struct {
 		allVolumeParams     map[string]string

pkg/csi/manila/driver.go

@@ -117,7 +117,7 @@ type nonBlockingGRPCServer struct {
 }
 
 const (
-	specVersion   = "1.8.0"
+	specVersion   = "1.12.0"
 	driverVersion = "0.9.0"
 	topologyKey   = "topology.manila.csi.openstack.org/zone"
 )
@@ -189,11 +189,15 @@ func NewDriver(o *DriverOpts) (*Driver, error) {
 func (d *Driver) SetupControllerService() error {
 	klog.Info("Providing controller service")
 
-	d.addControllerServiceCapabilities([]csi.ControllerServiceCapability_RPC_Type{
+	controllerCapabilities := []csi.ControllerServiceCapability_RPC_Type{
 		csi.ControllerServiceCapability_RPC_CREATE_DELETE_VOLUME,
 		csi.ControllerServiceCapability_RPC_CREATE_DELETE_SNAPSHOT,
 		csi.ControllerServiceCapability_RPC_EXPAND_VOLUME,
-	})
+	}
+	if d.shareProto == "NFS" {
+		controllerCapabilities = append(controllerCapabilities, csi.ControllerServiceCapability_RPC_MODIFY_VOLUME)
+	}
+	d.addControllerServiceCapabilities(controllerCapabilities)
 
 	d.addVolumeCapabilityAccessModes([]csi.VolumeCapability_AccessMode_Mode{
 		csi.VolumeCapability_AccessMode_MULTI_NODE_MULTI_WRITER,

pkg/csi/manila/driver_test.go

@@ -134,3 +134,31 @@ func TestInitProxiedDriverRetryOnUnavailable(t *testing.T) {
 		t.Errorf("expected 4 ProbeForever calls (3 Unavailable + 1 success), got %d", idClient.calls)
 	}
 }
+
+func TestSetupControllerServiceModifyVolumeCapability(t *testing.T) {
+	for _, tc := range []struct {
+		protocol string
+		want     bool
+	}{
+		{protocol: "NFS", want: true},
+		{protocol: "CEPHFS", want: false},
+	} {
+		t.Run(tc.protocol, func(t *testing.T) {
+			d := &Driver{shareProto: tc.protocol}
+			if err := d.SetupControllerService(); err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+
+			got := false
+			for _, capability := range d.cscaps {
+				if capability.GetRpc().GetType() == csi.ControllerServiceCapability_RPC_MODIFY_VOLUME {
+					got = true
+					break
+				}
+			}
+			if got != tc.want {
+				t.Fatalf("MODIFY_VOLUME capability = %t, want %t", got, tc.want)
+			}
+		})
+	}
+}

pkg/csi/manila/shareadapters/accessrights.go

@@ -32,6 +32,8 @@ const (
 	accessRuleStateError       = "error"
 	accessRuleStateQueuedApply = "queued_to_apply"
 	accessRuleStateApplying    = "applying"
+	accessRuleStateQueuedDeny  = "queued_to_deny"
+	accessRuleStateDenying     = "denying"
 
 	waitForAccessRuleTimeout = 3
 	waitForAccessRuleRetries = 10
@@ -92,3 +94,40 @@ func waitForAccessRuleActive(ctx context.Context, manilaClient manilaclient.Inte
 
 	return result, nil
 }
+
+func waitForAccessRuleDeleted(ctx context.Context, manilaClient manilaclient.Interface, shareID, accessID string) error {
+	backoff := wait.Backoff{
+		Duration: time.Second * waitForAccessRuleTimeout,
+		Factor:   1.2,
+		Steps:    waitForAccessRuleRetries,
+	}
+
+	err := wait.ExponentialBackoffWithContext(ctx, backoff, func(ctx context.Context) (bool, error) {
+		rights, err := manilaClient.GetAccessRights(ctx, shareID)
+		if err != nil {
+			return false, fmt.Errorf("failed to get access rights for share %s: %v", shareID, err)
+		}
+
+		for i := range rights {
+			if rights[i].ID != accessID {
+				continue
+			}
+
+			switch rights[i].State {
+			case accessRuleStateActive, accessRuleStateQueuedDeny, accessRuleStateDenying:
+				klog.V(4).Infof("access rule %s for share %s is in state %s, waiting for deletion...", accessID, shareID, rights[i].State)
+				return false, nil
+			case accessRuleStateError:
+				return false, fmt.Errorf("access rule %s for share %s is in error state while being revoked", accessID, shareID)
+			default:
+				return false, fmt.Errorf("access rule %s for share %s is in unexpected state %s while being revoked", accessID, shareID, rights[i].State)
+			}
+		}
+
+		return true, nil
+	})
+	if wait.Interrupted(err) {
+		return fmt.Errorf("timed out waiting for access rule %s for share %s to be deleted", accessID, shareID)
+	}
+	return err
+}