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 subPath existence check to not follow symlink #48555

Merged
merged 1 commit into from
Jul 13, 2017
Merged

Fix subPath existence check to not follow symlink #48555

merged 1 commit into from
Jul 13, 2017

Conversation

redbaron
Copy link
Contributor

@redbaron redbaron commented Jul 6, 2017 •
edited
Loading

What this PR does / why we need it:
Volume mounting logic introduced in #43775 and #45623 checks
for subPath existence before attempting to create a directory,
should subPath not be present.

This breaks if subPath is a dangling symlink, os.Stat returns
"do not exist" status, yet os.MkdirAll can't create directory
as symlink is present at the given path.

This patch makes existence check to use os.Lstat which works for
normal files/directories as well as doesn't not attempt to follow
symlink, therefore it's "do not exist" status is more reliable when
making a decision whether to create directory or not.

subPath symlinks can be dangling in situations where kubelet is
running in a container itself with access to docker socket, such
as CoreOS's kubelet-wrapper script

Release note:

Fix pods failing to start when subPath is a dangling symlink from kubelet point of view, which can happen if it is running inside a container

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 6, 2017
@k8s-ci-robot
Copy link
Contributor

Hi @redbaron. Thanks for your PR.

I'm waiting for a kubernetes 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.

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. I understand the commands that are listed here.

@k8s-ci-robot 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 Jul 6, 2017
@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Jul 6, 2017
@wongma7
Copy link
Contributor

wongma7 commented Jul 6, 2017 •
edited
Loading

lgtm, thanks. given how many issues are popping up with my subpath code I'm debating having it not return an error when MkdirAll/chmod fails, so in those case it would behave like before my patches. Depends on whether we think it's better the pod starts with unexpected/incorrect permissions than not at all.

That said I really hope this is the end of the issues since I'd like to think 'symlink subpath + containerized kubelet' is the rarest edge case possible....

@redbaron
Copy link
Contributor Author

redbaron commented Jul 6, 2017

I think it is doing the right thing, issues will pop up either way, I wouldn't worry about it :)

@redbaron
Copy link
Contributor Author

redbaron commented Jul 6, 2017

/sig sig/node

@redbaron
Copy link
Contributor Author

redbaron commented Jul 7, 2017

How can I assign sig/node and sig/storage labels to draw their attention?

@gyliu513
Copy link
Contributor

gyliu513 commented Jul 7, 2017

/ok-to-test

@redbaron you can do as following to add labels

/sig node
/sig storage

@k8s-ci-robot k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. sig/storage Categorizes an issue or PR as relevant to SIG Storage. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 7, 2017
@redbaron
Fix subPath existence check to not follow symlink
020ec43 
Volume mounting logic introduced in kubernetes#43775 and kubernetes#45623 checks
for subPath existence before attempting to create a directory,
should subPath not be present.

This breaks if subPath is a dangling symlink, os.Stat returns
"do not exist" status, yet `os.MkdirAll` can't create directory
as symlink is present at the given path.

This patch makes existence check to use os.Lstat which works for
normal files/directories as well as doesn't not attempt to follow
symlink, therefore it's "do not exist" status is more reliable when
making a decision whether to create directory or not.

subPath symlinks can be dangling in situations where kubelet is
running in a container itself with access to docker socket, such
as CoreOS's kubelet-wrapper script
@redbaron redbaron force-pushed the hostPath-and-subPath-symlink branch from 632eef9 to 020ec43 Compare July 7, 2017 10:52
@mtaufen
Copy link
Contributor

mtaufen commented Jul 7, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 7, 2017
@mtaufen
Copy link
Contributor

mtaufen commented Jul 7, 2017

/retest

2 similar comments
@redbaron
Copy link
Contributor Author

redbaron commented Jul 7, 2017

/retest

@redbaron
Copy link
Contributor Author

redbaron commented Jul 7, 2017

/retest

@mtaufen
Copy link
Contributor

mtaufen commented Jul 12, 2017

@dchen1107 PTAL for approval

@dchen1107
Copy link
Member

/test pull-kubernetes-node-e2e

@dchen1107
Copy link
Member

/lgtm

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dchen1107, mtaufen, redbaron

Associated issue: 43775

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 12, 2017
@redbaron
Copy link
Contributor Author

/test pull-kubernetes-node-e2e

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 009858f into kubernetes:master Jul 13, 2017
@redbaron redbaron deleted the hostPath-and-subPath-symlink branch July 13, 2017 09:53
@redbaron
Copy link
Contributor Author

@mtaufen , do I need to submit separate PR to 1.7 branch or it will be cherry-picked automatically?

@k8s-cherrypick-bot
Copy link

Removing label cherrypick-candidate because no release milestone was set. This is an invalid state and thus this PR is not being considered for cherry-pick to any release branch. Please add an appropriate release milestone and then re-add the label.

@mtaufen mtaufen added this to the v1.7 milestone Jul 13, 2017
@mtaufen
Copy link
Contributor

mtaufen commented Jul 13, 2017

I added the cherrypick-candidate label.

@wojtek-t PTAL

@wojtek-t
Copy link
Member

@mtaufen - it seems like a reasonable cherrypick candidate. We have some issues with release branch now, but I will make sure this is cherrypicked before cutting 1.7.2

@wojtek-t wojtek-t mentioned this pull request Jul 18, 2017
Merged
@wojtek-t
Copy link
Member

Approved - cherrypick in #49089

@wojtek-t wojtek-t added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Jul 18, 2017
wojtek-t added a commit that referenced this pull request Jul 19, 2017
@wojtek-t
Merge pull request #49089 from wojtek-t/automated-cherry-pick-of-#485…
68e581e 
…55-upstream-release-1.7

Automated cherry pick of #48555 upstream release 1.7
@k8s-cherrypick-bot
Copy link

Commit found in the "release-1.7" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@mtaufen mtaufen

@Random-Liu Random-Liu

@dchen1107 dchen1107

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
v1.7
Development

Successfully merging this pull request may close these issues.
10 participants
@redbaron @k8s-ci-robot @wongma7 @gyliu513 @mtaufen @dchen1107 @k8s-github-robot @k8s-cherrypick-bot @wojtek-t @Random-Liu