mountpath should be absolute #48815
mountpath should be absolute #48815
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
Hi @dixudx. 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 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-github-robot
added
size/XS
| @@ -3045,7 +3045,6 @@ func TestValidateVolumeMounts(t *testing.T) { | |||
| {Name: "abc-123", MountPath: "/bab", SubPath: "baz"}, | |||
| {Name: "abc-123", MountPath: "/bac", SubPath: ".baz"}, | |||
| {Name: "abc-123", MountPath: "/bad", SubPath: "..baz"}, | |||
| {Name: "abc", MountPath: "c:/foo/bar"}, | |||
There was a problem hiding this comment.
isn't this absolute on a Windows machine?
There was a problem hiding this comment.
@sttts It's true. Apiserver can be run on various OS/Arch. Currently all the CIs are running on Linux. Such windows-like absolute path "c:/foo/bar" is considered as relative path on Linux.
| @@ -1760,6 +1760,9 @@ func ValidateVolumeMounts(mounts []api.VolumeMount, volumes sets.String, fldPath | |||
| if mountpoints.Has(mnt.MountPath) { | |||
| allErrs = append(allErrs, field.Invalid(idxPath.Child("mountPath"), mnt.MountPath, "must be unique")) | |||
| } | |||
| if !path.IsAbs(mnt.MountPath) { | |||
There was a problem hiding this comment.
what happens today if you use a non-absolute path? is it relative to the container root?
There was a problem hiding this comment.
@sttts From the Docker Official Doc,
The
container-dirmust always be an absolute path such as /src/docs.
So the container cannot be started up if you use a non-absolute path for mnt.MountPath, just as what was reported in #48749.
There was a problem hiding this comment.
Ic. I was worried that we disallow something in the API that used to work.
/cc @derekwaynecarr sounds reasonable. ptal.
|
/ok-to-test |
k8s-ci-robot
removed
the
needs-ok-to-test
|
ISTR fixing this in the past, but it must have been something else related. Shame on me. /lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dixudx, thockin Associated issue: 48749 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 |
k8s-github-robot
added
the
approved
|
/retest |
1 similar comment
|
/retest |
|
Automatic merge from submit-queue (batch tested with PRs 48082, 48815, 48901, 48824) |
What this PR does / why we need it:
Should validate the mountpath before mounting to container. Docker forbids mounting to a relative path inside the container.
Which issue this PR fixes : fixes #48749
Special notes for your reviewer:
Release note: