[release/1.4] windows: Use GetFinalPathNameByHandle for ResolveSymbolicLink by kevpar · Pull Request #1636 · containerd/cri

kevpar · 2021-05-05T17:49:38Z

Backport of containerd/containerd#5411 for release/1.4. I also had to update some vendoring to bring in the right dependencies.

This change splits the definition of pkg/cri/os.ResolveSymbolicLink by
platform (windows/!windows), and switches to an alternate implementation
for Windows. This aims to fix the issue described in containerd/containerd#5405.

The previous implementation which just called filepath.EvalSymlinks has
historically had issues on Windows. One of these issues we were able to
fix in Go, but EvalSymlinks's behavior is not well specified on
Windows, and there could easily be more issues in the future, so it
seems prudent to move to a separate implementation for Windows.

The new implementation uses the Windows GetFinalPathNameByHandle API,
which takes a handle to an open file or directory and some flags, and
returns the "real" name for the object. See comments in the code for
details on the implementation.

I have tested this change with a variety of mounts and everything seems
to work as expected. Functions that make incorrect assumptions on what a
Windows path can look like may have some trouble with the \?\ path
syntax. For instance EvalSymlinks fails when given a \?\UNC\ path. For
this reason, the resolvePath implementation modifies the returned path
to translate to the more common form (\?\UNC\server\share ->
\server\share).

Signed-off-by: Kevin Parsons kevpar@microsoft.com

From containerd/containerd@b0d3b35

kevpar · 2021-05-05T19:55:25Z

The CI failures look unrelated. Maybe just hitting rate limits on gcr.io? I think it's safe to ignore them.

estesp · 2021-05-05T20:19:29Z

@mikebrow looks like release/1.4 doesn't have the magic update to the CRI test image(s) stuff we did on the main branch. Getting the 403 authorization failure on the old image layer. Can you help us figure out which pieces need backported here?

kevpar · 2021-05-13T08:30:51Z

@mikebrow any thoughts on what is needed here to help CI?

mikebrow · 2021-05-13T20:14:00Z

@mikebrow any thoughts on what is needed here to help CI?

@mikebrow looks like release/1.4 doesn't have the magic update to the CRI test image(s) stuff we did on the main branch. Getting the 403 authorization failure on the old image layer. Can you help us figure out which pieces need backported here?

thx almost forgot about this repo :-)

Let's merge this: #1638 brings critest back on-line then you can rebase..

mikebrow · 2021-05-13T20:47:33Z

kk you should be ok for critest.. after rebase on release/1.4

mikebrow · 2021-05-14T00:23:21Z

FYI also updated vendor, and cleaned up the node e2e test over here ... #1639 green lights!

fuweid · 2021-05-14T04:23:00Z

@kevpar need rebase here~

Updating with new versions needed to support using GetFinalPathNameByHandle. Signed-off-by: Kevin Parsons <kevpar@microsoft.com>

kevpar · 2021-05-14T09:24:48Z

@mikebrow thank you! I've rebased now.

mikebrow

just a couple nits

pkg/os/os_windows_test.go

pkg/os/os_windows.go

pkg/os/os_windows_test.go

kevpar · 2021-05-14T18:40:10Z

@mikebrow de-nit'd :)

pkg/os/os_unix.go

mikebrow

LGTM
just a new line missing

fuweid

Left some comments~

fuweid · 2021-05-18T06:05:37Z

pkg/os/os_windows.go

+// We could use os.Open if the path is a file, but it's easier to just use the same code for both.
+// Therefore, we call windows.CreateFile directly.
+func openPath(path string) (windows.Handle, error) {
+	u16, err := windows.UTF16PtrFromString(path)


~~Is it possible to use syscall.UTF16PtrFromString? I see stdlib provides the function. https://golang.org/pkg/syscall/?GOOS=windows#UTF16PtrFromString.~~

GetFinalPathNameByHandle is unavailable in stdlib syscall.

fuweid · 2021-05-18T06:13:19Z

pkg/os/os_windows.go

+	New: func() interface{} {
+		// Size of buffer chosen somewhat arbitrarily to accommodate a large number of path strings.
+		// MAX_PATH (260) + size of volume GUID prefix (49) + null terminator = 310.
+		b := make([]uint16, 310)


Suggest to add docs here https://docs.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation?tabs=cmd.

Starting in Windows 10, version 1607, MAX_PATH limitations have been removed from common Win32 file and directory functions.

Maybe we can use 4096 :)

Given this is a backport (of containerd/containerd@b0d3b35), my assumption is we shouldn't diverge from what was put into containerd master. For this and the other comments you have left, do you feel these should be addressed here?

I am fine with current change ~ just think 4096 looks better than 310

fuweid · 2021-05-18T06:16:19Z

pkg/os/os_windows.go

+// correct size for the call.
+func getFinalPathNameByHandle(h windows.Handle, flags uint32) (string, error) {
+	b := *(pool.Get().(*[]uint16))
+	defer func() { pool.Put(&b) }()


The b has been re-allocated. defer should be handled in some condition.

--> in for loop // If the buffer wasn't large enough, n will be the total size needed (including null terminator). // Resize and try again. if n > uint32(len(b) { b = make([]uint16, n) continue }

I'm not sure I understand your comment. Can you clarify what you're asking for?

Sorry for the late reply. A b is from pool.Get so that we should put the b back into the pool.
But the b has been changed when the size of original b is not large enough. In this case, it is not good to put the changed b into the pool.

pkg/os/os_windows.go

This change splits the definition of pkg/cri/os.ResolveSymbolicLink by platform (windows/!windows), and switches to an alternate implementation for Windows. This aims to fix the issue described in containerd/containerd#5405. The previous implementation which just called filepath.EvalSymlinks has historically had issues on Windows. One of these issues we were able to fix in Go, but EvalSymlinks's behavior is not well specified on Windows, and there could easily be more issues in the future, so it seems prudent to move to a separate implementation for Windows. The new implementation uses the Windows GetFinalPathNameByHandle API, which takes a handle to an open file or directory and some flags, and returns the "real" name for the object. See comments in the code for details on the implementation. I have tested this change with a variety of mounts and everything seems to work as expected. Functions that make incorrect assumptions on what a Windows path can look like may have some trouble with the \\?\ path syntax. For instance EvalSymlinks fails when given a \\?\UNC\ path. For this reason, the resolvePath implementation modifies the returned path to translate to the more common form (\\?\UNC\server\share -> \\server\share). Signed-off-by: Kevin Parsons <kevpar@microsoft.com> From containerd/containerd@b0d3b35

thaJeztah · 2021-05-26T16:55:42Z

/test pull-cri-containerd-node-e2e

fuweid

LGTM

kevpar · 2021-05-27T15:21:46Z

Thanks @fuweid!

k8s-ci-robot added the size/XXL label May 5, 2021

kevpar force-pushed the 1.4-symlink branch from 30a0187 to 4e9721e Compare May 5, 2021 19:30

kevpar mentioned this pull request May 6, 2021

windows: Use GetFinalPathNameByHandle for ResolveSymbolicLink containerd/containerd#5411

Merged

This was referenced May 13, 2021

updating critools to pickup new and moved test images #1637

Closed

moves up version of critest #1638

Merged

Update vendored dependencies

76356c4

Updating with new versions needed to support using GetFinalPathNameByHandle. Signed-off-by: Kevin Parsons <kevpar@microsoft.com>

kevpar force-pushed the 1.4-symlink branch from 4e9721e to c3199f9 Compare May 14, 2021 09:06

mikebrow reviewed May 14, 2021

View reviewed changes

pkg/os/os_windows_test.go Outdated Show resolved Hide resolved

pkg/os/os_windows.go Show resolved Hide resolved

pkg/os/os_windows_test.go Show resolved Hide resolved

kevpar force-pushed the 1.4-symlink branch from c3199f9 to 14ce983 Compare May 14, 2021 18:39

kevpar force-pushed the 1.4-symlink branch from 14ce983 to 8da928d Compare May 14, 2021 19:07

mikebrow reviewed May 17, 2021

View reviewed changes

pkg/os/os_unix.go Outdated Show resolved Hide resolved

mikebrow approved these changes May 17, 2021

View reviewed changes

mikebrow assigned fuweid and cpuguy83 May 17, 2021

fuweid reviewed May 18, 2021

View reviewed changes

kevpar force-pushed the 1.4-symlink branch from 8da928d to ee825af Compare May 26, 2021 16:33

fuweid approved these changes May 27, 2021

View reviewed changes

fuweid merged commit 4cc8ca2 into containerd:release/1.4 May 27, 2021

Conversation

kevpar commented May 5, 2021

Uh oh!

kevpar commented May 5, 2021

Uh oh!

estesp commented May 5, 2021

Uh oh!

kevpar commented May 13, 2021

Uh oh!

mikebrow commented May 13, 2021

Uh oh!

mikebrow commented May 13, 2021

Uh oh!

mikebrow commented May 14, 2021

Uh oh!

fuweid commented May 14, 2021

Uh oh!

kevpar commented May 14, 2021

Uh oh!

mikebrow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kevpar commented May 14, 2021

Uh oh!

Uh oh!

mikebrow left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fuweid left a comment

Choose a reason for hiding this comment

Uh oh!

fuweid May 18, 2021

Choose a reason for hiding this comment

Uh oh!

fuweid May 18, 2021

Choose a reason for hiding this comment

Uh oh!

fuweid May 18, 2021

Choose a reason for hiding this comment

Uh oh!

kevpar May 24, 2021

Choose a reason for hiding this comment

Uh oh!

fuweid May 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fuweid May 18, 2021

Choose a reason for hiding this comment

Uh oh!

kevpar May 24, 2021

Choose a reason for hiding this comment

Uh oh!

fuweid May 27, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thaJeztah commented May 26, 2021

Uh oh!

fuweid left a comment

Choose a reason for hiding this comment

Uh oh!

kevpar commented May 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

mikebrow left a comment •

edited

Loading

fuweid May 27, 2021 •

edited

Loading