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.

Sign up for GitHub

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

Jump to bottom

Add ability to pass format options in mount-utils #112877

Merged
merged 1 commit into from Nov 7, 2022
Merged

Add ability to pass format options in mount-utils #112877

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 9 additions & 1 deletion staging/src/k8s.io/mount-utils/mount.go
View file
Open in desktop
Expand Up @@ -165,7 +165,15 @@ func (mounter *SafeFormatAndMount) FormatAndMount(source string, target string,
// be used by callers that pass sensitive material (like passwords) as mount
// options.
func (mounter *SafeFormatAndMount) FormatAndMountSensitive(source string, target string, fstype string, options []string, sensitiveOptions []string) error {
return mounter.formatAndMountSensitive(source, target, fstype, options, sensitiveOptions)
return mounter.FormatAndMountSensitiveWithFormatOptions(source, target, fstype, options, sensitiveOptions, nil /* formatOptions */)
}

// FormatAndMountSensitiveWithFormatOptions behaves exactly the same as
// FormatAndMountSensitive, but allows for options to be passed when the disk
// is formatted. These options are NOT validated in any way and should never
// come directly from untrusted user input as that would be an injection risk.
func (mounter *SafeFormatAndMount) FormatAndMountSensitiveWithFormatOptions(source string, target string, fstype string, options []string, sensitiveOptions []string, formatOptions []string) error {
Copy link

Choose a reason for hiding this comment

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

Need to add the method to the interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which interface? FormatAndMount and FormatAndMountSensitive don't seem to be on any interface.

return mounter.formatAndMountSensitive(source, target, fstype, options, sensitiveOptions, formatOptions)
}

// getMountRefsByDev finds all references to the device provided
Expand Down
3 changes: 2 additions & 1 deletion staging/src/k8s.io/mount-utils/mount_linux.go
View file
Open in desktop
Expand Up @@ -469,7 +469,7 @@ func (mounter *SafeFormatAndMount) checkAndRepairFilesystem(source string) error
}

// formatAndMount uses unix utils to format and mount the given disk
func (mounter *SafeFormatAndMount) formatAndMountSensitive(source string, target string, fstype string, options []string, sensitiveOptions []string) error {
func (mounter *SafeFormatAndMount) formatAndMountSensitive(source string, target string, fstype string, options []string, sensitiveOptions []string, formatOptions []string) error {
readOnly := false
for _, option := range options {
if option == "ro" {
Expand Down Expand Up @@ -521,6 +521,7 @@ func (mounter *SafeFormatAndMount) formatAndMountSensitive(source string, target
source,
}
}
args = append(formatOptions, args...)

klog.Infof("Disk %q appears to be unformatted, attempting to format as type: %q with options: %v", source, fstype, args)
output, err := mounter.Exec.Command("mkfs."+fstype, args...).CombinedOutput()
Expand Down
2 changes: 1 addition & 1 deletion staging/src/k8s.io/mount-utils/mount_unsupported.go
View file
Open in desktop
Expand Up @@ -90,7 +90,7 @@ func (mounter *Mounter) GetMountRefs(pathname string) ([]string, error) {
return nil, errUnsupported
}

func (mounter *SafeFormatAndMount) formatAndMountSensitive(source string, target string, fstype string, options []string, sensitiveOptions []string) error {
func (mounter *SafeFormatAndMount) formatAndMountSensitive(source string, target string, fstype string, options []string, sensitiveOptions []string, formatOptions []string) error {
return mounter.Interface.Mount(source, target, fstype, options)
}

Expand Down
8 changes: 6 additions & 2 deletions staging/src/k8s.io/mount-utils/mount_windows.go
View file
Open in desktop
Expand Up @@ -273,7 +273,7 @@ func (mounter *Mounter) GetMountRefs(pathname string) ([]string, error) {
return []string{pathname}, nil
}

func (mounter *SafeFormatAndMount) formatAndMountSensitive(source string, target string, fstype string, options []string, sensitiveOptions []string) error {
func (mounter *SafeFormatAndMount) formatAndMountSensitive(source string, target string, fstype string, options []string, sensitiveOptions []string, formatOptions []string) error {
// Try to mount the disk
klog.V(4).Infof("Attempting to formatAndMount disk: %s %s %s", fstype, source, target)

Expand All @@ -288,8 +288,12 @@ func (mounter *SafeFormatAndMount) formatAndMountSensitive(source string, target
}

// format disk if it is unformatted(raw)
formatOptionsUnwrapped := ""
if len(formatOptions) > 0 {
formatOptionsUnwrapped = " " + strings.Join(formatOptions, " ")
}
cmd := fmt.Sprintf("Get-Disk -Number %s | Where partitionstyle -eq 'raw' | Initialize-Disk -PartitionStyle GPT -PassThru"+
" | New-Partition -UseMaximumSize | Format-Volume -FileSystem %s -Confirm:$false", source, fstype)
" | New-Partition -UseMaximumSize | Format-Volume -FileSystem %s -Confirm:$false%s", source, fstype, formatOptionsUnwrapped)
Copy link
Member

Choose a reason for hiding this comment

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

I think this will work for an intree driver but not for a CSI driver that uses CSI Proxy to do the format & mount

Copy link
Member

Choose a reason for hiding this comment

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

Is there a way how to pass such parameters through CSI proxy?
We can document that this will work only when a CSI driver is running as a "privileged container", without the proxy. Will it work then?

Copy link
Member

Choose a reason for hiding this comment

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

Yes there's a way to pass parameters to CSI Proxy, CSI Drivers use the API exposed by CSI Proxy, currently it formats only with the volumeId https://github.com/kubernetes-csi/csi-proxy/blob/master/client/api/volume/v1/api.proto#L22 in the proto definition but as we're in the process to move to HostProcess containers this would need to go here instead: https://github.com/kubernetes-csi/csi-proxy/blob/4624937be58bfd46f5ebe6d8f213f58414275e45/pkg/volume/volume.go#L140

After reading the PR in more detail it's doing the following in Windows:

	formatOptionsUnwrapped := ""
	if len(formatOptions) > 0 {
		formatOptionsUnwrapped = " " + strings.Join(formatOptions, " ")
	}

Is the expectation of the format additional options to work in Windows? I don't think they will work even in intree code, I'd suggest removing this part for Windows and only use it in Linux.

To enable something similar in Windows there needs to be a mapping of a mkfs format option to a Format-Volume format option.

Copy link
Member

Choose a reason for hiding this comment

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

Good questions!

Is the expectation of the format additional options to work in Windows?

Yes, I would say so. The code as it's suggested in this PR moves this responsibility to the caller (= CSI driver / in-tree volume plugin / user). It must provide the right options for the right OS and the right fsType. mount-utils should be IMO small and stupid in this regard.

@ConnorJC3, so... if user wants a ntfs volume that will be usable both on Windows and Linux nodes, they will need to provide both mkfs and Format-Volume options, because they don't know what node will format the volume (i.e. if a Windows or Linux Pod gets the PV first). How will the StorageClass.Parameters will look like? Will be the driver really smart enough to choose the right options? It is solvable, but it's more complex that I initially thought.

I don't think they will work even in intree code, I'd suggest removing this part for Windows and only use it in Linux.

I am not a Windows person, @ConnorJC3, does it sound right? Can you check it, update the PR as suggested and update csi-proxy?

Copy link
Contributor Author

@ConnorJC3 ConnorJC3 Nov 2, 2022 •
edited

Choose a reason for hiding this comment

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

I am not a Windows person, @ConnorJC3, does it sound right? Can you check it, update the PR as suggested and update csi-proxy?

So as far as I know, there are 2 options here:

  1. Use a privileged container and directly manipulate the volumes. If a driver is doing this (and uses mount-utils), it should work.
  2. Use the csi-proxy daemon. Basically everybody does this currently, because up until recently privileged containers on Windows did not exist. We (as in the aws-ebs-csi-driver) plan to eventually move to using them, but have to wait for now until all our supported platforms can use them.

In case number 2, changes would have to be made to the CSI Proxy in order to support this for drivers. However, I'm not sure if it's worth doing so given that the CSI Proxy should be on its way out anyways.

As mount-utils will become the de-facto place for this implementation as privileged Windows containers are phased in, we should keep it here.

Copy link
Contributor Author

@ConnorJC3 ConnorJC3 Nov 2, 2022 •
edited

Choose a reason for hiding this comment

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

@ConnorJC3, so... if user wants a ntfs volume that will be usable both on Windows and Linux nodes, they will need to provide both mkfs and Format-Volume options, because they don't know what node will format the volume (i.e. if a Windows or Linux Pod gets the PV first). How will the StorageClass.Parameters will look like? Will be the driver really smart enough to choose the right options? It is solvable, but it's more complex that I initially thought.

I'm not sure if this is a huge problem, because this should already be true with mount options, and nobody's complained about those as far as I know. Also, this is more of a discussion that makes sense on the CSI Driver side, mount-utils just provides the implementation for formatting and mounting volumes. That said, I see 2 options:

  1. If a CSI Driver is creating the format options (say, for example, the driver had an option for block size and mapped that to the relevant format option), then it could just do it correctly for each platform.
  2. If the user is directly supplying the format options, then the driver would probably need to take a set of options for both linux and windows, and use the correct one depending on which platform the formatting ran on.

Copy link
Member

Choose a reason for hiding this comment

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

As mount-utils will become the de-facto place for this implementation as privileged Windows containers are phased in, we should keep it here.

That's my understanding too, but again, I know very little about Windows. @mauriciopoppe, is the current PR correct then?

Copy link
Member

Choose a reason for hiding this comment

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

As mount-utils will become the de-facto place for this implementation as privileged Windows containers are phased in, we should keep it here

This is still up for discussion, I think it's worth talking about it with the community.

In case number 2, changes would have to be made to the CSI Proxy in order to support this for drivers. However, I'm not sure if it's worth doing so given that the CSI Proxy should be on its way out anyways.

With HostProcess containers CSI Proxy is going to become a library and will be used in the CSI Drivers like mount-utils , this is tracked in this KEP kubernetes/enhancements#3641, @ddebroy and I had a discussion about it in the CSI Windows meeting, quoting some part of it: "reasons to keep CSI Proxy independent, CSI Proxy is more than mount e.g. ops for SMB, iSCSI, broader in the number of problems it solves, it's like storage utils than mount-utils."

I think I should take back what I said in #112877 (comment) about it not working in Windows, I think it'll work and enable sending additional flags which is useful and it's all in the control of the caller which should have Windows/Linux variations in their mount code.

/remove-hold

if output, err := mounter.Exec.Command("powershell", "/c", cmd).CombinedOutput(); err != nil {
return fmt.Errorf("diskMount: format disk failed, error: %v, output: %q", err, string(output))
}
Expand Down
14 changes: 13 additions & 1 deletion staging/src/k8s.io/mount-utils/safe_format_and_mount_test.go
View file
Open in desktop
Expand Up @@ -68,6 +68,7 @@ func TestSafeFormatAndMount(t *testing.T) {
fstype string
mountOptions []string
sensitiveMountOptions []string
formatOptions []string
execScripts []ExecArgs
mountErrs []error
expErrorType MountErrorType
Expand Down Expand Up @@ -213,6 +214,15 @@ func TestSafeFormatAndMount(t *testing.T) {
},
expErrorType: FormatFailed,
},
{
description: "Test that 'blkid' is called and confirms unformatted disk, format passes, mount passes (with format options)",
fstype: "ext4",
formatOptions: []string{"-b", "1024"},
execScripts: []ExecArgs{
{"blkid", []string{"-p", "-s", "TYPE", "-s", "PTTYPE", "-o", "export", "/dev/foo"}, "", &testingexec.FakeExitError{Status: 2}},
{"mkfs.ext4", []string{"-b", "1024", "-F", "-m0", "/dev/foo"}, "", nil},
},
},
}

for _, test := range tests {
Expand All @@ -233,7 +243,9 @@ func TestSafeFormatAndMount(t *testing.T) {
device := "/dev/foo"
dest := mntDir
var err error
if len(test.sensitiveMountOptions) == 0 {
if len(test.formatOptions) > 0 {
err = mounter.FormatAndMountSensitiveWithFormatOptions(device, dest, test.fstype, test.mountOptions, test.sensitiveMountOptions, test.formatOptions)
} else if len(test.sensitiveMountOptions) == 0 {
err = mounter.FormatAndMount(device, dest, test.fstype, test.mountOptions)
} else {
err = mounter.FormatAndMountSensitive(device, dest, test.fstype, test.mountOptions, test.sensitiveMountOptions)
Expand Down