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.

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

Fix user visible files creation for windows #62375

Merged
merged 1 commit into from Apr 16, 2018

Conversation

@feiskyer
Copy link
Member

feiskyer commented Apr 11, 2018

What this PR does / why we need it:

Fix user visible files creation for windows. Without this, createUserVisibleFiles will get linkname with subpath included, and then symlink will fail. This is because "/" is used in pod spec (e.g. "new/path/data-1") while "" is used on Windows to get linkname.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #62338

Special notes for your reviewer:

Should also be cherry-picked to old releases.

Release note:

Fix user visible files creation for windows
@feiskyer

This comment has been minimized.

Copy link
Member Author

feiskyer commented Apr 11, 2018

/sig storage
/sig windows
/kind bug

@msau42

This comment has been minimized.

Copy link
Member

msau42 commented Apr 11, 2018

Can you add a unit test for this?

@feiskyer

This comment has been minimized.

Copy link
Member Author

feiskyer commented Apr 12, 2018

@msau42 Yep, of course.

@k8s-ci-robot k8s-ci-robot added size/M and removed size/XS labels Apr 12, 2018

@feiskyer

This comment has been minimized.

Copy link
Member Author

feiskyer commented Apr 12, 2018

/retest

@@ -414,6 +414,7 @@ func (w *AtomicWriter) writePayloadToDir(payload map[string]FileProjection, dir
// baz -> ..data/baz
func (w *AtomicWriter) createUserVisibleFiles(payload map[string]FileProjection) error {
for userVisiblePath := range payload {
userVisiblePath = filepath.Clean(userVisiblePath)

This comment has been minimized.

@msau42

msau42 Apr 12, 2018

Member

Can you explain how this solves the windows issue? It is not clear to me from the tests.

This comment has been minimized.

@feiskyer

feiskyer Apr 13, 2018

Author Member

This test is only for linux, so it's not clear for windows cases.

For windows containers, users may set path to foo/bar, while filepath.Clean() will convert it to foo\bar. The targetDir has already cleaned up to something like c:\var\lib\kubelet..., so following Index() logic will work now.

This comment has been minimized.

@msau42

msau42 Apr 13, 2018

Member

In the original bug report, the sample showed a container command that used Linux style path separators. Even if this is fixed, will the user still have to change their program?

This comment has been minimized.

@msau42

msau42 Apr 13, 2018

Member

Also should a windows test also be added?

This comment has been minimized.

@feiskyer

feiskyer Apr 13, 2018

Author Member

The unit tests on Windows are not running because of symlinks inside vendors. We may add this later if that could be resolved. @msau42 WDYT?

This comment has been minimized.

@msau42

msau42 Apr 13, 2018

Member

Ok sounds fine. I will do a final review soon

This comment has been minimized.

@msau42

msau42 Apr 13, 2018

Member

I see that validatePayload() in the very beginning of Write(), already does a clean on the user visible path. Do you know why it this additional clean is needed?

This comment has been minimized.

@feiskyer

feiskyer Apr 16, 2018

Author Member

@msau42 Thanks for reminding this. validatePayload() uses path.Clean(), but it doesn't convert slash to platform's Separator. It's reasonable to fix the issue there.

@feiskyer feiskyer force-pushed the feiskyer:visible-files branch from a87c6d5 to f12b8eb Apr 16, 2018

@k8s-ci-robot k8s-ci-robot added size/L and removed size/M labels Apr 16, 2018

@feiskyer

This comment has been minimized.

Copy link
Member Author

feiskyer commented Apr 16, 2018

@msau42 Fixed in validatePayload() and new unit tests added. PTAL

@msau42

This comment has been minimized.

Copy link
Member

msau42 commented Apr 16, 2018

/lgtm
Thanks! As a followup, it may be worth cleaning up all calls using path instead of filepath.

@saad-ali

This comment has been minimized.

Copy link
Member

saad-ali commented Apr 16, 2018

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Apr 16, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: feiskyer, msau42, saad-ali

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:

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

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Apr 16, 2018

Automatic merge from submit-queue (batch tested with PRs 62650, 62303, 62545, 62375). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 6fb3d3a into kubernetes:master Apr 16, 2018

15 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation feiskyer authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@feiskyer feiskyer deleted the feiskyer:visible-files branch Apr 17, 2018

k8s-github-robot pushed a commit that referenced this pull request Apr 18, 2018

Kubernetes Submit Queue
Merge pull request #62694 from feiskyer/clean-filepath
Automatic merge from submit-queue (batch tested with PRs 62694, 62569, 62646, 61633, 62433). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Use filepath.Clean() instead of path.Clean()

**What this PR does / why we need it**:

Use filepath.Clean() instead of path.Clean() across `pkg/volume`. This could fix potential issues for windows containers.

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
followup of #62375.

**Special notes for your reviewer**:

**Release note**:

```release-note
NONE
```

/assign @msau42

k8s-github-robot pushed a commit that referenced this pull request Apr 20, 2018

Kubernetes Submit Queue
Merge pull request #62689 from feiskyer/automated-cherry-pick-of-#623…
…75-upstream-release-1.10

Automatic merge from submit-queue.

Automated cherry pick of #62375: Fix use visible files creation for windows

Cherry pick of #62375 on release-1.10.

#62375: Fix use visible files creation for windows

k8s-github-robot pushed a commit that referenced this pull request Apr 20, 2018

Kubernetes Submit Queue
Merge pull request #62691 from feiskyer/automated-cherry-pick-of-#623…
…75-upstream-release-1.8

Automatic merge from submit-queue.

Automated cherry pick of #62375: Fix use visible files creation for windows

Cherry pick of #62375 on release-1.8.

#62375: Fix use visible files creation for windows

k8s-github-robot pushed a commit that referenced this pull request May 11, 2018

Kubernetes Submit Queue
Merge pull request #62690 from feiskyer/automated-cherry-pick-of-#623…
…75-upstream-release-1.9

Automatic merge from submit-queue.

Automated cherry pick of #62375: Fix use visible files creation for windows

Cherry pick of #62375 on release-1.9.

#62375: Fix use visible files creation for windows
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.