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

Don't attempt to make and chmod subPath if it already exists #45623

Merged
merged 1 commit into from
May 12, 2017

Conversation

wongma7
Copy link
Contributor

@wongma7 wongma7 commented May 10, 2017

fixes #45613

#43775 fixed one bug and introduced another... I overlooked that subPaths can be files, in which case MkDirAll will simply fail and the pod will not able to start.

Regardless of whether it is a directory or a file, there is no need to introduce the MkdirAll->Chmod procedure if it exists, because if it exists, it should already have the correct permissions.

This needs to be cherry-picked into 1.6.

Fix pods failing to start if they specify a file as a volume subPath to mount.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 10, 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 labels May 10, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-ci-robot
Copy link
Contributor

Hi @wongma7. 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 @k8s-bot 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.

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 May 10, 2017
@gnufied
Copy link
Member

gnufied commented May 10, 2017

@k8s-bot ok to test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 10, 2017
@k8s-github-robot k8s-github-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels May 11, 2017
@wongma7
Copy link
Contributor Author

wongma7 commented May 11, 2017

@kubernetes/sig-node-pr-reviews pretty severe bug preventing pods from starting, please review :)

@wongma7
Copy link
Contributor Author

wongma7 commented May 11, 2017

/sig sig/node
@kubernetes/sig-node-pr-reviews

zzz

@saad-ali saad-ali 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. kind/bug Categorizes issue or PR as related to a bug. cherrypick-candidate labels May 11, 2017
@saad-ali saad-ali added this to the v1.6 milestone May 11, 2017
@saad-ali saad-ali added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label May 11, 2017
@rootfs
Copy link
Contributor

rootfs commented May 12, 2017

safe guard lgtm
/assign
/lgmt

@childsb
Copy link
Contributor

childsb commented May 12, 2017

This LGTM, i'll ask someone from node to approve.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 12, 2017
@derekwaynecarr
Copy link
Member

/approve

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr, wongma7

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 May 12, 2017
@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 816f8e2 into kubernetes:master May 12, 2017
k8s-github-robot pushed a commit that referenced this pull request May 17, 2017
…3-upstream-release-1.6

Automatic merge from submit-queue

Automated cherry pick of #45623

Cherry pick of #45623 on release-1.6.

#45623: Don't attempt to make and chmod subPath if it already exists
@k8s-cherrypick-bot
Copy link

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

@enisoc enisoc added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jun 12, 2017
redbaron added a commit to redbaron/kubernetes that referenced this pull request Jul 7, 2017
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
k8s-github-robot pushed a commit that referenced this pull request Jul 13, 2017
Automatic merge from submit-queue

Fix subPath existence check to not follow symlink

**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**:
```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
```
wojtek-t pushed a commit to wojtek-t/kubernetes that referenced this pull request Jul 18, 2017
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note-none Denotes a PR that doesn't merit a release note. 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
Development

Successfully merging this pull request may close these issues.

subPath = file bug