fix: add trailing backslash to remote path when linking to SMB shares #98

marosset · 2020-10-19T23:33:50Z

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change

/kind bug

/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:

This PR addresses an issue where golang fails to properly evaluate symlinks on windows if the link source is remote file share AND there is no trailing backslash (ex: \server\share will not work but \server\share\ will work).

Which issue(s) this PR fixes:

Fixes # kubernetes/kubernetes#94213

Special notes for your reviewer:
This issue is only seen on containerd (not Docker).
Kevin Parsons (@kevpar) investigated this and believes it is becasue docker does not call EvalSymLinks but containerd does.

We'll follow up with a golang bug for this too.

Does this PR introduce a user-facing change?:

Working around some issues with how golang evaluates symlink targets by appending a trailing \ when creating symlinks to SMB shares.

k8s-ci-robot · 2020-10-19T23:33:57Z

Welcome @marosset!

It looks like this is your first PR to kubernetes-csi/csi-proxy 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-csi/csi-proxy has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2020-10-19T23:33:58Z

Hi @marosset. Thanks for your PR.

I'm waiting for a kubernetes-csi member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

kevpar · 2020-10-19T23:50:40Z

internal/os/smb/api.go

@@ -38,6 +38,13 @@ func (APIImplementor) IsSmbMapped(remotePath string) (bool, error) {
 // alpha to merge the paths.
 // TODO (for beta release): Merge the link paths - os.Symlink and Powershell link path.
 func (APIImplementor) NewSmbLink(remotePath, localPath string) error {
+
+	if !strings.HasSuffix(remotePath, "\\") {


Do we expect this code path to ever be used for a file symlink? If so, adding a slash to the end would break it.

We technically only need the slash added if the path is to the share root, also (e.g. \\server\share doesn't work currently, but \\server\share\dir does).

On the node I was testing these changes out on only the symlink created against the root share used this codepath.

Here is the state of some symlinks created on that node

azureuser@1876k8s00000000 c:\k>dir "c:\var\lib\kubelet\pods\7819fd8b-cdbd-4292-8554-769ad4af543a\volumes\kubernetes.io~csi\pvc-ee3f6a92-f195-4d9e-841b-6966e6839829" Volume in drive C is Windows Volume Serial Number is CA78-414E Directory of c:\var\lib\kubelet\pods\7819fd8b-cdbd-4292-8554-769ad4af543a\volumes\kubernetes.io~csi\pvc-ee3f6a92-f195-4d9e-841b-6966e6839829 10/19/2020 11:11 PM <DIR> . 10/19/2020 11:11 PM <DIR> .. 10/19/2020 11:11 PM <SYMLINKD> mount [c:\var\lib\kubelet\plugins\kubernetes.io\csi\pv\pvc-ee3f6a92-f195-4d9e-841b-6966e6839829\globalmount] 10/19/2020 11:10 PM 340 vol_data.json 1 File(s) 340 bytes 3 Dir(s) 117,465,772,032 bytes free azureuser@1876k8s00000000 c:\k>dir c:\var\lib\kubelet\plugins\kubernetes.io\csi\pv\pvc-ee3f6a92-f195-4d9e-841b-6966e6839829 Volume in drive C is Windows Volume Serial Number is CA78-414E Directory of c:\var\lib\kubelet\plugins\kubernetes.io\csi\pv\pvc-ee3f6a92-f195-4d9e-841b-6966e6839829 10/19/2020 11:11 PM <DIR> . 10/19/2020 11:11 PM <DIR> .. 10/19/2020 11:11 PM <SYMLINKD> globalmount [\\faf486191df3c4f0fad548f.file.core.windows.net\pvc-ee3f6a92-f195-4d9e-841b-6966e6839829\] 10/19/2020 11:10 PM 135 vol_data.json 1 File(s) 135 bytes 3 Dir(s) 117,465,772,032 bytes free

I believe this codepath will only be used for creating links to SMB shares.
Will try and confirm.

@andyzhangx

Also want to verify there should be no problem adding trailing backslash in any case. Is there any possibility that remote path is a file?

smb remote path is a folder, this PR lgtm, thanks

ddebroy · 2020-10-20T05:45:03Z

/ok-to-test

a0f195c Merge pull request kubernetes-csi#106 from msau42/fix-canary 7100c12 Only set staging registry when running canary job b3c65f9 Merge pull request kubernetes-csi#99 from msau42/add-release-process e53f3e8 Merge pull request kubernetes-csi#103 from msau42/fix-canary d129462 Document new method for adding CI jobs are new K8s versions e73c2ce Use staging registry for canary tests 2c09846 Add cleanup instructions to release-notes generation 60e1cd3 Merge pull request kubernetes-csi#98 from pohly/kubernetes-1-19-fixes 0979c09 prow.sh: fix E2E suite for Kubernetes >= 1.18 3b4a2f1 prow.sh: fix installing Go for Kubernetes 1.19.0 1fbb636 Merge pull request kubernetes-csi#97 from pohly/go-1.15 82d108a switch to Go 1.15 d8a2530 Merge pull request kubernetes-csi#95 from msau42/add-release-process 843bddc Add steps on promoting release images 0345a83 Merge pull request kubernetes-csi#94 from linux-on-ibm-z/bump-timeout 1fdf2d5 cloud build: bump timeout in Prow job 41ec6d1 Merge pull request kubernetes-csi#93 from animeshk08/patch-1 5a54e67 filter-junit: Fix gofmt error 0676fcb Merge pull request kubernetes-csi#92 from animeshk08/patch-1 36ea4ff filter-junit: Fix golint error f5a4203 Merge pull request kubernetes-csi#91 from cyb70289/arm64 43e50d6 prow.sh: enable building arm64 image 0d5bd84 Merge pull request kubernetes-csi#90 from pohly/k8s-staging-sig-storage 3df86b7 cloud build: k8s-staging-sig-storage c5fd961 Merge pull request kubernetes-csi#89 from pohly/cloud-build-binfmt db0c2a7 cloud build: initialize support for running commands in Dockerfile be902f4 Merge pull request kubernetes-csi#88 from pohly/multiarch-windows-fix 340e082 build.make: optional inclusion of Windows in multiarch images 5231f05 build.make: properly declare push-multiarch 4569f27 build.make: fix push-multiarch ambiguity 17dde9e Merge pull request kubernetes-csi#87 from pohly/cloud-build bd41690 cloud build: initial set of shared files 9084fec Merge pull request kubernetes-csi#81 from msau42/add-release-process 6f2322e Update patch release notes generation command 0fcc3b1 Merge pull request kubernetes-csi#78 from ggriffiths/fix_csi_snapshotter_rbac_version_set d8c76fe Support local snapshot RBAC for pull jobs c1bdf5b Merge pull request kubernetes-csi#80 from msau42/add-release-process ea1f94a update release tools instructions git-subtree-dir: release-tools git-subtree-split: a0f195cc2ddc2a1f07d4d3e46fc08187db358f94

jingxu97 · 2020-10-21T14:13:18Z

/lgtm

jingxu97 · 2020-10-21T19:33:49Z

/approve

k8s-ci-robot · 2020-10-21T19:33:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jingxu97, marosset

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~~ [jingxu97]

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

fix: add trailing backslash to remote path when linking to SMB shares

76fce59

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 19, 2020

k8s-ci-robot requested review from kkmsft and PatrickLang October 19, 2020 23:33

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 19, 2020

k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Oct 19, 2020

kevpar reviewed Oct 19, 2020

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 20, 2020

ddebroy requested a review from jingxu97 October 20, 2020 05:45

k8s-ci-robot assigned jingxu97 Oct 21, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 21, 2020

k8s-ci-robot merged commit 4322716 into kubernetes-csi:master Oct 21, 2020

This was referenced Nov 10, 2020

fixing issue where SMB share paths cannot resolve with CRI-containerD on Windows kubernetes/kubernetes#96396

Merged

Support CRI-ContainerD on Windows kubernetes/enhancements#1001

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add trailing backslash to remote path when linking to SMB shares #98

fix: add trailing backslash to remote path when linking to SMB shares #98

marosset commented Oct 19, 2020 •

edited

Loading

k8s-ci-robot commented Oct 19, 2020

k8s-ci-robot commented Oct 19, 2020

kevpar Oct 19, 2020

kevpar Oct 19, 2020

marosset Oct 19, 2020

marosset Oct 19, 2020

jingxu97 Oct 20, 2020

andyzhangx Oct 21, 2020

ddebroy commented Oct 20, 2020

jingxu97 commented Oct 21, 2020

jingxu97 commented Oct 21, 2020

k8s-ci-robot commented Oct 21, 2020

fix: add trailing backslash to remote path when linking to SMB shares #98

fix: add trailing backslash to remote path when linking to SMB shares #98

Conversation

marosset commented Oct 19, 2020 • edited Loading

k8s-ci-robot commented Oct 19, 2020

k8s-ci-robot commented Oct 19, 2020

kevpar Oct 19, 2020

Choose a reason for hiding this comment

kevpar Oct 19, 2020

Choose a reason for hiding this comment

marosset Oct 19, 2020

Choose a reason for hiding this comment

marosset Oct 19, 2020

Choose a reason for hiding this comment

jingxu97 Oct 20, 2020

Choose a reason for hiding this comment

andyzhangx Oct 21, 2020

Choose a reason for hiding this comment

ddebroy commented Oct 20, 2020

jingxu97 commented Oct 21, 2020

jingxu97 commented Oct 21, 2020

k8s-ci-robot commented Oct 21, 2020

marosset commented Oct 19, 2020 •

edited

Loading