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 issue in evaluating symlink path for subpath #93707

Merged
merged 1 commit into from Aug 16, 2020
Merged

Fix issue in evaluating symlink path for subpath #93707

merged 1 commit into from Aug 16, 2020

Conversation

jingxu97
Copy link
Contributor

@jingxu97 jingxu97 commented Aug 5, 2020 •
edited

After removing driver letter assignment during disk partition and formatting, we found an issue that golang filepath.EvalSymlnik no longer works. See bug golang/go#39786

We had used a simple powershell command (get-item -path %p).Target to get the symlink, but it does not address all the cases. If there is a subpath under symlink, this command does not resolve upper path symlink. During evaluating symlink, we need to make sure the returned target path does not contain any symlink.

This PR tries to recursively resolve symlink in the path until it checking all intermediate dirs.

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

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

@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. 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. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 5, 2020
@k8s-ci-robot k8s-ci-robot added sig/storage Categorizes an issue or PR as relevant to SIG Storage. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Aug 5, 2020
@jingxu97
Copy link
Contributor Author

jingxu97 commented Aug 5, 2020

/test pull-kubernetes-e2e-windows-gce

1 similar comment
@jingxu97
Copy link
Contributor Author

jingxu97 commented Aug 5, 2020

/test pull-kubernetes-e2e-windows-gce

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 5, 2020
@jingxu97
Copy link
Contributor Author

jingxu97 commented Aug 5, 2020

/test pull-kubernetes-e2e-windows-gce

2 similar comments
@jingxu97
Copy link
Contributor Author

jingxu97 commented Aug 6, 2020

/test pull-kubernetes-e2e-windows-gce

@jingxu97
Copy link
Contributor Author

jingxu97 commented Aug 6, 2020

/test pull-kubernetes-e2e-windows-gce

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 6, 2020
@jingxu97 jingxu97 changed the title WIP Fix issue in evaluating symlink path for subpath Aug 6, 2020
@jingxu97
Copy link
Contributor Author

jingxu97 commented Aug 6, 2020

cc @msau42

@jingxu97
Copy link
Contributor Author

jingxu97 commented Aug 6, 2020

/retest

msau42
msau42 reviewed Aug 6, 2020
View reviewed changes
Copy link
Member

@msau42 msau42 left a comment

Choose a reason for hiding this comment

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

IIUC, since Get-Item isn't returning the proper path, we check until we find a parent directory that resolves correctly, and then append the rest of the path back. But doesn't this basically bypass the symlink resolution/verification?

pkg/volume/util/subpath/subpath_windows.go Outdated
@@ -62,6 +62,32 @@ func evalPath(path string) (linkedPath string, err error) {
if isVolumePrefix(linkedPath) {
return path, err
}
// The following is a workaround solution for issue https://github.com/kubernetes/kubernetes/issues/93759
if _, err = os.Lstat(linkedPath); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Should we specifically look for not exists error?

Copy link
Member

Choose a reason for hiding this comment

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

What happens if the linkedPath really does exist because of name collision between the directory name of the subpath and a real directory in the system? For example, your subpath was "/var/lib/kubelet/..../volumes/x/programs", and "C://programs" actually does exist?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that case, the following check will fail
if !mount.PathWithinBase(currentFullPath, volumePath) {
errorResult = fmt.Errorf("SubPath %q not within volume path %q", currentFullPath, volumePath)
break
}
https://github.com/kubernetes/kubernetes/blob/feaaf468ed83e214dd12a7d3c4fbf5177165c405/pkg/volume/util/subpath/subpath_windows.go#L170

@jingxu97
Copy link
Contributor Author

jingxu97 commented Aug 7, 2020

/test pull-kubernetes-e2e-windows-gce

1 similar comment
@jingxu97
Copy link
Contributor Author

jingxu97 commented Aug 7, 2020

/test pull-kubernetes-e2e-windows-gce

@jingxu97
Copy link
Contributor Author

jingxu97 commented Aug 7, 2020

resolution/verification?

I think I didn't change the original flow. Basically instead of using a power shell command (which might take some time to execute), here use a few more commands to get the path. Even if user change symlink during or after this process, we only allow subpath which is under base path. The lock is implemented after we eval the subpath ?

kubernetes/pkg/volume/util/subpath/subpath_windows.go

Line 99 in 9881440

func lockAndCheckSubPathWithoutSymlink(volumePath, subPath string) ([]uintptr, error) {

@msau42
Copy link
Member

msau42 commented Aug 7, 2020

My understanding is that we skip evaluating the parts of the path that don't resolve correctly, which means that the path that gets returned from this function could still contain symlinks?

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Aug 15, 2020
@jingxu97 jingxu97 added the kind/bug Categorizes issue or PR as related to a bug. label Aug 15, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-kind Indicates a PR lacks a `kind/foo` label and requires one. label Aug 15, 2020
@jingxu97 jingxu97 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 15, 2020
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 15, 2020
@kubernetes kubernetes deleted a comment from k8s-ci-robot Aug 15, 2020
@jingxu97
Copy link
Contributor Author

/test pull-kubernetes-e2e-windows-gce

@jingxu97
Copy link
Contributor Author

/test pull-kubernetes-e2e-windows-gce

@jingxu97
Copy link
Contributor Author

/retest

@jingxu97
Copy link
Contributor Author

/retest
/test pull-kubernetes-e2e-windows-gce

andyzhangx
andyzhangx approved these changes Aug 15, 2020
View reviewed changes
Copy link
Member

@andyzhangx andyzhangx left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 15, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@andyzhangx
Copy link
Member

/priority important-soon

@k8s-ci-robot k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Aug 15, 2020
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

3 similar comments
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@jingxu97 jingxu97 added this to the v1.19 milestone Aug 16, 2020
@k8s-ci-robot k8s-ci-robot merged commit ebbbc57 into kubernetes:master Aug 16, 2020
@jingxu97
Copy link
Contributor Author

should consider to cherrypick to 1.18

@jingxu97 jingxu97 mentioned this pull request Nov 20, 2020
Merged
k8s-ci-robot added a commit that referenced this pull request Dec 1, 2020
@k8s-ci-robot
Merge pull request #96729 from jingxu97/automated-cherry-pick-of-#92328-
cc92637 
#93707-upstream-release-1.18

Automated cherry pick of #92328: Replace evalSymlink() with powershell command for #93707: Fix issue in evaluating symlink path for subpath
@jingxu97 jingxu97 mentioned this pull request Feb 23, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msau42 msau42 msau42 left review comments

@andyzhangx andyzhangx andyzhangx approved these changes

@jsafrane jsafrane Awaiting requested review from jsafrane

@oomichi oomichi Awaiting requested review from oomichi

Assignees

@andyzhangx andyzhangx

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.19
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@jingxu97 @msau42 @k8s-ci-robot @andyzhangx @fejta-bot @oomichi