Support specifying block size for filesystem format

[ConnorJC3]

Is this a bug fix or adding new feature?

New feature

What is this PR about? / Why do we need it?

Adds support to specify the block size during filesystem format. Currently not supported on windows (or for NTFS volumes at all), that can change once the csi-proxy is replaced with privileged Windows containers.

What testing is done?

Manual/New unit tests/CI

[ConnorJC3]

/hold we might want to wait to merge this for a non-RC release of mount-utils, hold for now

[rdpsin]

Different filesystems have different valid block sizes. E.g., ext4 will only accept 1K, 2K or 4K block sizes. Should the driver check for that?

[rdpsin]

Why?

[ConnorJC3]

Shortly after this, the block size is directly inserted into formatOptions and passed to mount-utils. An attacker that could theoretically control blockSize here could use that on linux to insert arbitrary mkfs parameters (a potential security threat). On Windows, it's a complete takeover: formatOptions is concatenated into the command.

In theory, this value is checked in the controller, but I'm not 100% confident there's no way to induce a value here without going through the controller (on the contrary, I'm pretty confident it is possible by directly manipulating CSIVolume objects).

Checking that it looks like an integer here is practically free and makes 100% sure no malicious values ever get passed to formatOptions

[wmesard]

It's not our interface, so I'm not sure how we can validate the input. Is it a number of bytes? Number of pages? Does the underlying command allow a "k" suffix? What about "Ki"? Does it allow negative values? mkfs.ext4 does, mkfs.xfs does not.

Unless you're saying that we should make it our interface, validate the input, and take on the job of converting it to whatever format is appropriate for the underlying file system. I don't think we should do that.

[torredil]

Different filesystems have different valid block sizes. E.g., ext4 will only accept 1K, 2K or 4K block sizes. Should the driver check for that?

mkfs rounds up to a valid block size.

StorageClass:

apiVersion: storage.k8s.io/v1
kind: StorageClass
metadata:
  name: ebs-sc
provisioner: ebs.csi.aws.com
volumeBindingMode: WaitForFirstConsumer
parameters:
  blocksize: "3535"
  csi.storage.k8s.io/fstype: ext4
----------------------------------------

$ kubectl describe pv

Name:              pvc-bd9fc0a3-6481-4811-8ad4-b2022b8241c8
Labels:            <none>
Annotations:       pv.kubernetes.io/provisioned-by: ebs.csi.aws.com
Finalizers:        [kubernetes.io/pv-protection external-attacher/ebs-csi-aws-com]
StorageClass:      ebs-sc
Status:            Bound
Claim:             default/ebs-claim
Reclaim Policy:    Delete
Access Modes:      RWO
VolumeMode:        Filesystem
Capacity:          4Gi
Node Affinity:
  Required Terms:
    Term 0:        topology.ebs.csi.aws.com/zone in [us-east-1d]
Message:
Source:
    Type:              CSI (a Container Storage Interface (CSI) volume source)
    Driver:            ebs.csi.aws.com
    FSType:            ext4
    VolumeHandle:      vol-0d6ec6acb3bbdbdd7
    ReadOnly:          false
    VolumeAttributes:      blocksize=3535
                           storage.kubernetes.io/csiProvisionerIdentity=1669217011030-8081-ebs.csi.aws.com
Events:                <none>
----------------------------------------

$ kubectl logs ebs-csi-node-qtqpt -n kube-system -c ebs-plugin

I1123 19:07:54.622264       1 mount_linux.go:528] Disk "/dev/nvme1n1" appears to be unformatted, attempting to format as type: "ext4" with options: [-b 3535 -F -m0 /dev/nvme1n1]
I1123 19:07:55.111667       1 mount_linux.go:538] Disk successfully formatted (mkfs): ext4 - /dev/nvme1n1 /var/lib/kubelet/plugins/kubernetes.io/csi/ebs.csi.aws.com/20ca089c5b265bd40055b47b3b882793681fd817a59484c5f1bf70f8bd5a6523/globalmount 

$ root@app:/# tune2fs -l /dev/nvme0n1p1 |grep '^Block size:'
Block size:               4096

[andyzhangx]

/test all

[wmesard]

Different filesystems have different valid block sizes. E.g., ext4 will only accept 1K, 2K or 4K block sizes. Should the driver check for that?

That seems like a violation of separation of concerns. Do these file systems have a stable interface to query their supported block sizes? Are we going to update the CSI driver when the FS or the underlying hardware adds support for new block sizes? How will we know if software and hardware on the node supports the new block size?

Also, that list only applies to x86. ext4 on ARM supports larger block sizes today.

Let components do its own input validation. As a caller, just make sure you are handling and reporting the errors correctly.

[ConnorJC3]

/remove-hold

I believe I've addressed all the existing feedback, this PR is ready for a fresh round of review and otherwise ready to merge.

[wmesard]

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?

[ConnorJC3]

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.

[torredil]

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: torredil, wmesard

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [torredil]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment