Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support specifying block size for filesystem format #1452

Merged
merged 1 commit into from
Jan 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/parameters.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
24 changes: 24 additions & 0 deletions pkg/driver/constants.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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: {},
}
)
38 changes: 35 additions & 3 deletions pkg/driver/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand All @@ -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")
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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{
Expand All @@ -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,
Expand Down
51 changes: 51 additions & 0 deletions pkg/driver/controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
12 changes: 6 additions & 6 deletions pkg/driver/mock_mount.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion pkg/driver/mount.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions pkg/driver/mount_windows.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
36 changes: 24 additions & 12 deletions pkg/driver/node.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"fmt"
"os"
"path/filepath"
"strconv"
"strings"

csi "github.com/container-storage-interface/spec/lib/go/csi"
Expand All @@ -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

Expand Down Expand Up @@ -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)
Comment on lines +151 to +153
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But really, neither place should be doing this, because it is not your interface. mkfs.xfs allows suffixes (e.g. "4k"), while this code will reject them. No big deal I guess. The user will figure out what's happening and work around it. But what about some future hypothetical file system that takes "small", "medium", and "large" instead of a number?

Copy link
Contributor Author

@ConnorJC3 ConnorJC3 Dec 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can try to make the validation smarter, but I'm of the opinion we should just leave it dumb. As explained above, we cannot accept arbitrary values here, because it leads to an RCE (and even when it doesn't, a significant security risk). We'd have to rebuild the validation for every filesystem ourselves, and getting it wrong means exposing our users/customers to security holes.

All existing filesystems I'm aware of accept this value as an integer. It's theoretically possible, but I strongly doubt any future filesystem will change that.

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 {
Expand Down Expand Up @@ -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)
Expand Down
Loading