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

fix: adopt go1.23 behavior change in mount point parsing on Windows#2 #129370

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: adopt go1.23 behavior change in mount point parsing on Windows#2 #129370

wants to merge 1 commit into from
21 −1

Conversation

andyzhangx
Copy link
Member

What type of PR is this?

/kind bug
/priority important-soon
/sig storage
/sig windows
/triage accepted

What this PR does / why we need it:

fix: adopt go1.23 behavior change in mount point parsing on Windows, this PR is a proceeding fix of #129368

btw, getAllParentLinks func is not used anymore, so I cleaned it up in this PR also.

if a disk or smb file share is mounted to a path, from go 1.23, the path is not a ModeSymlink any more, it's ModeIrregular instead, check details here: golang/go#63703 (comment)

I also tested by myself on Windows machine:

  • build following code with golang 1.23 (without any godebug symbol) and got following result:
	stat, err := os.Lstat(path)
	if err != nil {
		fmt.Printf("Error: %v", err)
		return
	}
	if stat.Mode()&os.ModeSymlink != 0 {
		fmt.Printf("%s is a symbolic link\n", path)
	}
	if stat.Mode()&os.ModeDir != 0 {
		fmt.Printf("%s is a directory\n", path)
	}
	if stat.Mode()&os.ModeIrregular != 0 {
		fmt.Printf("%s is a non-regular file\n", path)
	}
	if stat.Mode()&os.ModeDevice != 0 {
		fmt.Printf("%s is a device file\n", path)
	}

C:\>lstat.exe c:\var\lib\kubelet\plugins\kubernetes.io\csi\disk.csi.azure.com\05ee7e7cbf99f002f3c7b9a253a090cc282c52731210e29ec4a50e42e29a226a\globalmount
c:\var\lib\kubelet\plugins\kubernetes.io\csi\disk.csi.azure.com\05ee7e7cbf99f002f3c7b9a253a090cc282c52731210e29ec4a50e42e29a226a\globalmount is a non-regular file

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

fix: adopt go1.23 behavior change in mount point parsing on Windows

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

fix: adopt go1.23 behavior change in mount point parsing on Windows

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/windows Categorizes an issue or PR as relevant to SIG Windows. triage/accepted Indicates an issue or PR is ready to be actively worked on. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 23, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: andyzhangx
Once this PR has been reviewed and has the lgtm label, please assign liggitt for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

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

@andyzhangx andyzhangx changed the title fix: adopt go1.23 behavior change in mount point parsing on Windows fix: adopt go1.23 behavior change in mount point parsing on Windows#2 Dec 23, 2024
@andyzhangx
Copy link
Member Author

/test pull-kubernetes-e2e-capz-azure-file
/test pull-kubernetes-e2e-capz-azure-file-windows
/test pull-kubernetes-e2e-capz-azure-disk
/test pull-kubernetes-e2e-capz-azure-disk-windows

@andyzhangx andyzhangx mentioned this pull request Dec 24, 2024
Open
carlory
carlory reviewed Dec 24, 2024
View reviewed changes
pkg/volume/util/subpath/subpath_windows.go
@@ -342,6 348,10 @@ func doSafeMakeDir(pathname string, base string, perm os.FileMode) error {
if stat.Mode()&os.ModeSymlink != 0 {
return fmt.Errorf("subpath %q is an unexpected symlink after Mkdir", currentPath)
}
// go1.23 behavior change: https://github.com/golang/go/issues/63703#issuecomment-2538631458
if stat.Mode()&os.ModeIrregular != 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to change this file? https://github.com/kubernetes/kubernetes/blob/master/test/images/agnhost/mounttest/mt_utils_windows.go#L80

Copy link
Member Author

Choose a reason for hiding this comment

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

in the end, yes, and that's only for test, so we could change it later

msau42
msau42 reviewed Dec 30, 2024
View reviewed changes
pkg/util/filesystem/defaultfs.go
@@ -90,7 90,7 @@ func MkdirAllWithPathCheck(path string, perm os.FileMode) error {
// 1. for Unix/Linux OS, check if the path is directory.
// 2. for windows NTFS, check if the path is symlink instead of directory.
if dir.IsDir() ||
(runtime.GOOS == "windows" && (dir.Mode()&os.ModeSymlink != 0)) {
(runtime.GOOS == "windows" && (dir.Mode()&os.ModeSymlink != 0 || dir.Mode()&os.ModeIrregular != 0)) {
Copy link
Member

Choose a reason for hiding this comment

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

What other files can be irregular? Do we need to be more precise and explicitly check for mount point (golang/go#63703 (comment))?

Copy link
Contributor

Choose a reason for hiding this comment

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

Same question here

Copy link
Member Author

Choose a reason for hiding this comment

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

@qmuntal do you have golang code example how to get junction file type or dir.Mode()&os.ModeIrregular is enough?

the function GetFileInformationByHandleEx could get following fileinfo type, but junction file type is not inside it, not sure how to do that per doc: https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-getfileinformationbyhandleex

FileInformationClass value	lpFileInformation type
FileBasicInfo (0)	FILE_BASIC_INFO
FileStandardInfo (1)	FILE_STANDARD_INFO
FileNameInfo (2)	FILE_NAME_INFO
FileStreamInfo (7)	FILE_STREAM_INFO
FileCompressionInfo (8)	FILE_COMPRESSION_INFO
FileAttributeTagInfo (9)	FILE_ATTRIBUTE_TAG_INFO
FileIdBothDirectoryInfo (0xa)	FILE_ID_BOTH_DIR_INFO
FileIdBothDirectoryRestartInfo (0xb)	FILE_ID_BOTH_DIR_INFO
FileRemoteProtocolInfo (0xd)	FILE_REMOTE_PROTOCOL_INFO
FileFullDirectoryInfo (0xe)	FILE_FULL_DIR_INFO
FileFullDirectoryRestartInfo (0xf)	FILE_FULL_DIR_INFO
FileStorageInfo (0x10)	FILE_STORAGE_INFO
FileAlignmentInfo (0x11)	FILE_ALIGNMENT_INFO
FileIdInfo (0x12)	FILE_ID_INFO
FileIdExtdDirectoryInfo (0x13)	FILE_ID_EXTD_DIR_INFO
FileIdExtdDirectoryRestartInfo (0x14)	FILE_ID_EXTD_DIR_INFO

Copy link

Choose a reason for hiding this comment

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

You can get the tag by using the FileAttributeTagInfo class. Junction reparse points are identified by the IO_REPARSE_TAG_MOUNT_POINT tag.

Copy link

Choose a reason for hiding this comment

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

Example (pseudocode)

h, _ := syscall.CreateFile(name, 0, 0, nil, syscall.OPEN_EXISTING,  syscall.FILE_FLAG_BACKUP_SEMANTICS | syscall.FILE_FLAG_OPEN_REPARSE_POINT, 0)

var ti windows.FILE_ATTRIBUTE_TAG_INFO
windows.GetFileInformationByHandleEx(h, windows.FileAttributeTagInfo, (*byte)(unsafe.Pointer(&ti)), uint32(unsafe.Sizeof(ti)))

if ti.ReparseTag == windows.IO_REPARSE_TAG_MOUNT_POINT {
  // This is a reparse tag!
}

Copy link
Member Author

Choose a reason for hiding this comment

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

that needs to open the file syscall.CreateFile, it may break if I open every file to check the attributes ...

Copy link
Member Author

Choose a reason for hiding this comment

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

also do we really need such complex method in IsLikelyNotMountPoint func?

	// IsLikelyNotMountPoint uses heuristics to determine if a directory
	// is not a mountpoint.
	// It should return ErrNotExist when the directory does not exist.
	// IsLikelyNotMountPoint does NOT properly detect all mountpoint types
	// most notably linux bind mounts and symbolic link. For callers that do not
	// care about such situations, this is a faster alternative to calling List()
	// and scanning that output.
	IsLikelyNotMountPoint(file string) (bool, error)

Copy link

Choose a reason for hiding this comment

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

that needs to open the file syscall.CreateFile, it may break if I open every file to check the attributes ...

You should only call it if os.ModeIrregular is set, which most probably will be a reparse point.

also do we really need such complex method in IsLikelyNotMountPoint func?

I don't have the answer to this question, depends on what the callers expects.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just in case you did not notice this: https://learn.microsoft.com/en-us/windows/win32/fileio/determining-whether-a-directory-is-a-volume-mount-point

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carlory carlory carlory left review comments

@msau42 msau42 msau42 left review comments

@zylxjtu zylxjtu zylxjtu left review comments

@qmuntal qmuntal qmuntal left review comments

@jingxu97 jingxu97 Awaiting requested review from jingxu97

@saad-ali saad-ali Awaiting requested review from saad-ali

Assignees
No one assigned
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/windows Categorizes an issue or PR as relevant to SIG Windows. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
[sig-windows] PR tracking
Status: No status
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@andyzhangx @k8s-ci-robot @qmuntal @zylxjtu @msau42 @carlory