windows: Use GetFinalPathNameByHandle for ResolveSymbolicLink by kevpar · Pull Request #5411 · containerd/containerd

kevpar · 2021-04-23T08:39:43Z

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 #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

pkg/os/os_windows.go

ambarve · 2021-04-23T22:57:55Z

LGTM!

kevpar · 2021-04-25T13:29:18Z

I've fixed up some logic, expanded the comments to explain better why we are doing it this way, and added some unit tests. However, I'm having trouble getting some of the more interesting tests to work currently, as they require creating/mounting VHDs. I initially used the Hyper-V PowerShell cmdlets, but those are not present on the CI machines, and my attempt to install them was not successful.

I'm looking into if there's another way I can do the VHD setup that is needed. Otherwise I'll probably need to take out some of the test cases, or find a way to put them behind a feature flag.

The implementation of resolvePath and intent behind what we are testing should still be suitable for review. @AkihiroSuda if you can take a look again that would be appreciated. :)

kevpar · 2021-04-27T01:00:36Z

Okay! Finally got the tests to run properly. Everything passes now except for pull-containerd-node-e2e, which looks unrelated.

The only issue I see is that to properly implement VHD formatting I had to detect the Windows OS version we are running on, which brings in the github.com/Microsoft/hcsshim/test module as a new dependency. I'm not sure if pulling this in to the main containerd module is desired, but this test doesn't look like it would make sense under integration/client, either.

I could remove the version detection given the CI only runs on WS2019, but that's not ideal since someone might need to run it locally, and we don't want to prevent the CI machines from being upgraded.

I could also move the github.com/Microsoft/hcsshim/test/functional/manifest package out of the test module, but that will require a new hcsshim release to update to, and that might complicate backporting this to v1.4.

theopenlab-ci · 2021-04-27T08:59:17Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 7m 50s (non-voting)
containerd-test-arm64 : SUCCESS in 23m 41s (non-voting)
containerd-integration-test-arm64 : SUCCESS in 29m 07s (non-voting)

mainred · 2021-04-28T14:46:02Z

Thanks for your effort, @kevpar .
Thanks, @AkihiroSuda for sharing so many valuable inputs in this PR, could you please take another look?

cc @estesp

pkg/os/os_windows_test.go

estesp · 2021-04-29T13:54:31Z

/test pull-containerd-node-e2e

theopenlab-ci · 2021-05-04T19:31:32Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 4m 58s (non-voting)
containerd-test-arm64 : SUCCESS in 5m 34s (non-voting)
containerd-integration-test-arm64 : SUCCESS in 23m 05s (non-voting)

kevpar · 2021-05-04T21:05:24Z

@estesp looks like this is properly signed off. Could you please help merge?

ibabou · 2021-05-05T21:00:24Z

@kevpar Is there a plan to get this cherry-picked into 1.4.x as well ? Or the only path will be into 1.5.1 ?
We currently facing issues with our csi driver on 1.4.4, so wants to see if we can get this backported.

kevpar · 2021-05-06T06:01:19Z

@kevpar Is there a plan to get this cherry-picked into 1.4.x as well ? Or the only path will be into 1.5.1 ?
We currently facing issues with our csi driver on 1.4.4, so wants to see if we can get this backported.

@ibabou yes, I have a PR out to backport to containerd/cri for 1.4 here: containerd/cri#1636

ibabou · 2021-05-06T06:06:28Z

@kevpar Is there a plan to get this cherry-picked into 1.4.x as well ? Or the only path will be into 1.5.1 ?
We currently facing issues with our csi driver on 1.4.4, so wants to see if we can get this backported.

@ibabou yes, I have a PR out to backport to containerd/cri for 1.4 here: containerd/cri#1636

That's awesome, thanks Kevin!

ibabou · 2021-05-26T19:39:45Z

@kevpar I don't see the change in release/1.5. Is there a pending thing related to it ? Seems both 1.5.1 and 1.5.2 got published without it ?

kevpar · 2021-05-26T19:51:02Z

@kevpar I don't see the change in release/1.5. Is there a pending thing related to it ? Seems both 1.5.1 and 1.5.2 got published without it ?

It looks like the backport PR (#5454) for 1.5 accidentally went into master instead of release/1.5. I'll send a new PR to get this in. :)

kevpar · 2021-05-26T19:57:41Z

@kevpar I don't see the change in release/1.5. Is there a pending thing related to it ? Seems both 1.5.1 and 1.5.2 got published without it ?

It looks like the backport PR (#5454) for 1.5 accidentally went into master instead of release/1.5. I'll send a new PR to get this in. :)

PR: #5537

ibabou · 2021-05-26T20:29:44Z

@kevpar I don't see the change in release/1.5. Is there a pending thing related to it ? Seems both 1.5.1 and 1.5.2 got published without it ?

It looks like the backport PR (#5454) for 1.5 accidentally went into master instead of release/1.5. I'll send a new PR to get this in. :)

PR: #5537

Yeah, I noticed too that the PR of the cherry-pick did seem to not have gone in release/1.5 :). Thanks a lot Kevin!

`filepath.EvalSymlinks` does not work well on Windows, and can enter infinite loops in certain situations and error out. Use Win32 API GetFinalPathNameByHandle to handle path resolution. Implementation based off on: containerd/containerd#5411 Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>

* Add `fs.ResolvePath` to resolve symbolic links `filepath.EvalSymlinks` does not work well on Windows, and can enter infinite loops in certain situations and error out. Use Win32 API GetFinalPathNameByHandle to handle path resolution. Implementation based off on: containerd/containerd#5411 Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com> * PR: types, documentation Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com> * remove unneded constant groups Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com> * Attempt normalized path first Update logic to try querying for normalized path initially, then use opened path if access is denied. Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com> --------- Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>

kevpar mentioned this pull request Apr 23, 2021

containerd fails to add a disk mount on Windows #5405

Closed

kevpar force-pushed the win-evalsymlinks branch from cf2152d to b6f688d Compare April 23, 2021 08:46

AkihiroSuda reviewed Apr 23, 2021

View reviewed changes

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

AkihiroSuda reviewed Apr 23, 2021

View reviewed changes

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

kevpar force-pushed the win-evalsymlinks branch 2 times, most recently from d506042 to 50b2d37 Compare April 23, 2021 09:15

ambarve reviewed Apr 23, 2021

View reviewed changes

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

kevpar force-pushed the win-evalsymlinks branch 2 times, most recently from be4dd63 to 4fc8e28 Compare April 25, 2021 12:34

kevpar force-pushed the win-evalsymlinks branch 9 times, most recently from 62ade5c to 001659e Compare April 27, 2021 00:21

kevpar force-pushed the win-evalsymlinks branch from 001659e to ee116ec Compare April 27, 2021 08:22

kzys reviewed Apr 28, 2021

View reviewed changes

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

containerd deleted a comment from theopenlab-ci bot Apr 29, 2021

kevpar force-pushed the win-evalsymlinks branch from 02734f8 to b0d3b35 Compare May 4, 2021 18:55

estesp merged commit 1e8ec32 into containerd:master May 5, 2021

estesp added the cherry-pick/1.5.x label May 5, 2021

estesp mentioned this pull request May 5, 2021

[release/1.5] cherry-pick: windows: Use GetFinalPathNameByHandle for ResolveSymbolicLink #5454

Merged

estesp added cherry-picked/1.5.x PR commits are cherry-picked into release/1.5 branch and removed cherry-pick/1.5.x labels May 5, 2021

kevpar mentioned this pull request May 5, 2021

[release/1.4] windows: Use GetFinalPathNameByHandle for ResolveSymbolicLink containerd/cri#1636

Merged

kevpar mentioned this pull request May 26, 2021

[release/1.5] cherry-pick: windows: Use GetFinalPathNameByHandle for ResolveSymbolicLink #5537

Merged

helsaawy mentioned this pull request Feb 6, 2023

Add fs.ResolvePath to resolve symbolic links microsoft/go-winio#275

Merged

helsaawy mentioned this pull request Apr 14, 2023

Add ResolvePath tests microsoft/go-winio#276

Merged

Conversation

kevpar commented Apr 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ambarve commented Apr 23, 2021

Uh oh!

kevpar commented Apr 25, 2021

Uh oh!

kevpar commented Apr 27, 2021

Uh oh!

theopenlab-ci bot commented Apr 27, 2021

Uh oh!

mainred commented Apr 28, 2021

Uh oh!

Uh oh!

estesp commented Apr 29, 2021

Uh oh!

theopenlab-ci bot commented May 4, 2021

Uh oh!

kevpar commented May 4, 2021

Uh oh!

ibabou commented May 5, 2021

Uh oh!

kevpar commented May 6, 2021

Uh oh!

ibabou commented May 6, 2021

Uh oh!

ibabou commented May 26, 2021

Uh oh!

kevpar commented May 26, 2021

Uh oh!

kevpar commented May 26, 2021

Uh oh!

ibabou commented May 26, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

kevpar commented Apr 23, 2021 •

edited

Loading