Merge pull request #1452 from ConnorJC3/custom-block-sizes

Support specifying block size for filesystem format
kubernetes-sigs · Jan 5, 2023 · 862fe33 · 862fe33
2 parents dbf04e4 + c3cf17f
commit 862fe33
Show file tree

Hide file tree

Showing 12 changed files with 190 additions and 45 deletions.
diff --git a/docs/parameters.md b/docs/parameters.md
@@ -14,6 +14,7 @@ The AWS EBS CSI Driver supports [tagging](tagging.md) through `StorageClass.para
 | "encrypted"                  | true, false                                        | false   | Whether the volume should be encrypted or not. Valid values are "true" or "false".                                                                                                                                                                                                                                                                                                             |
 | "blockExpress"               | true, false                                        | false   | Enables the creation of [io2 Block Express volumes](https://aws.amazon.com/ebs/provisioned-iops/#Introducing_io2_Block_Express) by increasing the IOPS limit for io2 volumes to 256000. Volumes created with more than 64000 IOPS will fail to mount on instances that do not support io2 Block Express.                                                                                       |
 | "kmsKeyId"                   |                                                    |         | The full ARN of the key to use when encrypting the volume. If not specified, AWS will use the default KMS key for the region the volume is in. This will be an auto-generated key called `/aws/ebs` if not changed.                                                                                                                                                                            |
+| "blockSize"                  |                                                    |         | The block size to use when formatting the underlying filesystem. Only supported on linux nodes and with fstype `ext2`, `ext3`, `ext4`, or `xfs`.                                    |
 
 **Appendix**
 * `gp3` is currently not supported on outposts. Outpost customers need to use a different type for their volumes.

diff --git a/pkg/driver/constants.go b/pkg/driver/constants.go
@@ -72,6 +72,9 @@ const (
 	// BlockExpressKey increases the iops limit for io2 volumes to the block express limit
 	BlockExpressKey = "blockexpress"
 
+	// BlockSizeKey configures the block size when formatting a volume
+	BlockSizeKey = "blocksize"
+
 	// TagKeyPrefix contains the prefix of a volume parameter that designates it as
 	// a tag to be attached to the resource
 	TagKeyPrefix = "tagSpecification"
@@ -127,3 +130,24 @@ const (
 	//DefaultBlockSize represents the default block size (4KB)
 	DefaultBlockSize = 4096
 )
+
+// constants for fstypes
+const (
+	// FSTypeExt2 represents the ext2 filesystem type
+	FSTypeExt2 = "ext2"
+	// FSTypeExt3 represents the ext3 filesystem type
+	FSTypeExt3 = "ext3"
+	// FSTypeExt4 represents the ext4 filesystem type
+	FSTypeExt4 = "ext4"
+	// FSTypeXfs represents the xfs filesystem type
+	FSTypeXfs = "xfs"
+	// FSTypeNtfs represents the ntfs filesystem type
+	FSTypeNtfs = "ntfs"
+)
+
+// BlockSizeExcludedFSTypes contains the filesystems that a custom block size is *NOT* supported on
+var (
+	BlockSizeExcludedFSTypes = map[string]struct{}{
+		FSTypeNtfs: {},
+	}
+)
diff --git a/pkg/driver/controller.go b/pkg/driver/controller.go
@@ -132,6 +132,7 @@ func (d *controllerService) CreateVolume(ctx context.Context, req *csi.CreateVol
 			cloud.VolumeNameTagKey:   volName,
 			cloud.AwsEbsDriverTagKey: isManagedByDriver,
 		}
+		blockSize string
 	)
 
 	tProps := new(template.Props)
@@ -178,6 +179,12 @@ func (d *controllerService) CreateVolume(ctx context.Context, req *csi.CreateVol
 			if value == "true" {
 				blockExpress = true
 			}
+		case BlockSizeKey:
+			_, err = strconv.Atoi(value)
+			if err != nil {
+				return nil, status.Errorf(codes.InvalidArgument, "Could not parse blockSize (%s): %v", value, err)
+			}
+			blockSize = value
 		default:
 			if strings.HasPrefix(key, TagKeyPrefix) {
 				scTags = append(scTags, value)
@@ -187,6 +194,26 @@ func (d *controllerService) CreateVolume(ctx context.Context, req *csi.CreateVol
 		}
 	}
 
+	if len(blockSize) > 0 {
+		for _, volCap := range req.GetVolumeCapabilities() {
+			switch volCap.GetAccessType().(type) {
+			case *csi.VolumeCapability_Block:
+				return nil, status.Error(codes.InvalidArgument, "Cannot use block size with block volume")
+			}
+
+			mountVolume := volCap.GetMount()
+			if mountVolume == nil {
+				return nil, status.Error(codes.InvalidArgument, "CreateVolume: mount is nil within volume capability")
+			}
+
+			fsType := mountVolume.GetFsType()
+
+			if _, ok := BlockSizeExcludedFSTypes[fsType]; ok {
+				return nil, status.Errorf(codes.InvalidArgument, "Cannot use block size with fstype %s", fsType)
+			}
+		}
+	}
+
 	if volumeType == cloud.VolumeTypeIO1 {
 		if iopsPerGB == 0 {
 			return nil, status.Errorf(codes.InvalidArgument, "The parameter IOPSPerGB must be specified for io1 volumes")
@@ -265,7 +292,7 @@ func (d *controllerService) CreateVolume(ctx context.Context, req *csi.CreateVol
 		}
 		return nil, status.Errorf(errCode, "Could not create volume %q: %v", volName, err)
 	}
-	return newCreateVolumeResponse(disk), nil
+	return newCreateVolumeResponse(disk, blockSize), nil
 }
 
 func validateCreateVolumeRequest(req *csi.CreateVolumeRequest) error {
@@ -754,7 +781,7 @@ func getOutpostArn(requirement *csi.TopologyRequirement) string {
 	return ""
 }
 
-func newCreateVolumeResponse(disk *cloud.Disk) *csi.CreateVolumeResponse {
+func newCreateVolumeResponse(disk *cloud.Disk, blockSize string) *csi.CreateVolumeResponse {
 	var src *csi.VolumeContentSource
 	if disk.SnapshotID != "" {
 		src = &csi.VolumeContentSource{
@@ -777,11 +804,16 @@ func newCreateVolumeResponse(disk *cloud.Disk) *csi.CreateVolumeResponse {
 		segments[AwsOutpostIDKey] = strings.ReplaceAll(arn.Resource, "outpost/", "")
 	}
 
+	context := map[string]string{}
+	if len(blockSize) > 0 {
+		context[BlockSizeKey] = blockSize
+	}
+
 	return &csi.CreateVolumeResponse{
 		Volume: &csi.Volume{
 			VolumeId:      disk.VolumeID,
 			CapacityBytes: util.GiBToBytes(disk.CapacityGiB),
-			VolumeContext: map[string]string{},
+			VolumeContext: context,
 			AccessibleTopology: []*csi.Topology{
 				{
 					Segments: segments,

diff --git a/pkg/driver/controller_test.go b/pkg/driver/controller_test.go
@@ -1585,6 +1585,57 @@ func TestCreateVolume(t *testing.T) {
 				checkExpectedErrorCode(t, err, codes.AlreadyExists)
 			},
 		},
+		{
+			name: "success with block size",
+			testFunc: func(t *testing.T) {
+				req := &csi.CreateVolumeRequest{
+					Name:               "random-vol-name",
+					CapacityRange:      stdCapRange,
+					VolumeCapabilities: stdVolCap,
+					Parameters: map[string]string{
+						BlockSizeKey: "4096",
+					},
+				}
+
+				ctx := context.Background()
+
+				mockDisk := &cloud.Disk{
+					VolumeID:         req.Name,
+					AvailabilityZone: expZone,
+					CapacityGiB:      util.BytesToGiB(stdVolSize),
+				}
+
+				mockCtl := gomock.NewController(t)
+				defer mockCtl.Finish()
+
+				mockCloud := cloud.NewMockCloud(mockCtl)
+				mockCloud.EXPECT().CreateDisk(gomock.Eq(ctx), gomock.Eq(req.Name), gomock.Any()).Return(mockDisk, nil)
+
+				awsDriver := controllerService{
+					cloud:         mockCloud,
+					inFlight:      internal.NewInFlight(),
+					driverOptions: &DriverOptions{},
+				}
+
+				response, err := awsDriver.CreateVolume(ctx, req)
+				if err != nil {
+					srvErr, ok := status.FromError(err)
+					if !ok {
+						t.Fatalf("Could not get error status code from error: %v", srvErr)
+					}
+					t.Fatalf("Unexpected error: %v", srvErr.Code())
+				}
+
+				context := response.Volume.VolumeContext
+				if blockSize, ok := context[BlockSizeKey]; ok {
+					if blockSize != "4096" {
+						t.Fatalf("Invalid %s in VolumeContext (got %s expected 4096)", BlockSizeKey, blockSize)
+					}
+				} else {
+					t.Fatalf("Missing key %s in VolumeContext", BlockSizeKey)
+				}
+			},
+		},
 	}
 
 	for _, tc := range testCases {

diff --git a/pkg/driver/mock_mount.go b/pkg/driver/mock_mount.go
diff --git a/pkg/driver/mount.go b/pkg/driver/mount.go
@@ -32,7 +32,7 @@ import (
 type Mounter interface {
 	mountutils.Interface
 
-	FormatAndMount(source string, target string, fstype string, options []string) error
+	FormatAndMountSensitiveWithFormatOptions(source string, target string, fstype string, options []string, sensitiveOptions []string, formatOptions []string) error
 	IsCorruptedMnt(err error) bool
 	GetDeviceNameFromMount(mountPath string) (string, int, error)
 	MakeFile(path string) error

diff --git a/pkg/driver/mount_windows.go b/pkg/driver/mount_windows.go
@@ -27,12 +27,12 @@ import (
 	"regexp"
 )
 
-func (m NodeMounter) FormatAndMount(source string, target string, fstype string, options []string) error {
+func (m NodeMounter) FormatAndMountSensitiveWithFormatOptions(source string, target string, fstype string, options []string, sensitiveOptions []string, formatOptions []string) error {
 	proxyMounter, ok := m.SafeFormatAndMount.Interface.(*mounter.CSIProxyMounter)
 	if !ok {
 		return fmt.Errorf("failed to cast mounter to csi proxy mounter")
 	}
-	return proxyMounter.FormatAndMount(source, target, fstype, options)
+	return proxyMounter.FormatAndMountSensitiveWithFormatOptions(source, target, fstype, options, sensitiveOptions, formatOptions)
 }
 
 // GetDeviceNameFromMount returns the volume ID for a mount path.

diff --git a/pkg/driver/node.go b/pkg/driver/node.go
@@ -21,6 +21,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"strconv"
 	"strings"
 
 	csi "github.com/container-storage-interface/spec/lib/go/csi"
@@ -34,17 +35,6 @@ import (
 )
 
 const (
-	// FSTypeExt2 represents the ext2 filesystem type
-	FSTypeExt2 = "ext2"
-	// FSTypeExt3 represents the ext3 filesystem type
-	FSTypeExt3 = "ext3"
-	// FSTypeExt4 represents the ext4 filesystem type
-	FSTypeExt4 = "ext4"
-	// FSTypeXfs represents the xfs filesystem type
-	FSTypeXfs = "xfs"
-	// FSTypeNtfs represents the ntfs filesystem type
-	FSTypeNtfs = "ntfs"
-
 	// default file system type to be used when it is not provided
 	defaultFsType = FSTypeExt4
 
@@ -155,6 +145,24 @@ func (d *nodeService) NodeStageVolume(ctx context.Context, req *csi.NodeStageVol
 		return nil, status.Errorf(codes.InvalidArgument, "NodeStageVolume: invalid fstype %s", fsType)
 	}
 
+	context := req.GetVolumeContext()
+	blockSize, ok := context[BlockSizeKey]
+	if ok {
+		// This check is already performed on the controller side
+		// However, because it is potentially security-sensitive, we redo it here to be safe
+		_, err := strconv.Atoi(blockSize)
+		if err != nil {
+			return nil, status.Errorf(codes.InvalidArgument, "Invalid blockSize (aborting!): %v", err)
+		}
+
+		// In the case that the default fstype does not support custom block sizes we could
+		// be using an invalid fstype, so recheck that here
+		if _, ok = BlockSizeExcludedFSTypes[strings.ToLower(fsType)]; ok {
+			return nil, status.Errorf(codes.InvalidArgument, "Cannot use block size with fstype %s", fsType)
+		}
+
+	}
+
 	mountOptions := collectMountOptions(fsType, mountVolume.MountFlags)
 
 	if ok = d.inFlight.Insert(volumeID); !ok {
@@ -219,7 +227,11 @@ func (d *nodeService) NodeStageVolume(ctx context.Context, req *csi.NodeStageVol
 
 	// FormatAndMount will format only if needed
 	klog.V(4).Infof("NodeStageVolume: formatting %s and mounting at %s with fstype %s", source, target, fsType)
-	err = d.mounter.FormatAndMount(source, target, fsType, mountOptions)
+	formatOptions := []string{}
+	if len(blockSize) > 0 {
+		formatOptions = append(formatOptions, "-b", blockSize)
+	}
+	err = d.mounter.FormatAndMountSensitiveWithFormatOptions(source, target, fsType, mountOptions, nil, formatOptions)
 	if err != nil {
 		msg := fmt.Sprintf("could not format %q and mount it at %q: %v", source, target, err)
 		return nil, status.Error(codes.Internal, msg)