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

Correctly handle named pipe host mounts for Windows #69484

Merged
merged 5 commits into from Oct 30, 2018

Conversation

@ddebroy
Member

ddebroy commented Oct 5, 2018

What this PR does / why we need it:
Windows named pipes in host mounts were not being correctly handled and passed through to the container runtime. This PR adds support to detect named pipes in Windows and skip the processing associated with regular file system based host mount paths in Windows.

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 # #67147

Special notes for your reviewer:
Tested the following spec spawns up the pod container on Windows with the mount paths to \\.\pipe\docker_pipe correctly configured:

apiVersion: v1
kind: Pod
metadata:
  name: docker-pipe-windows
spec:
  nodeSelector:
    beta.kubernetes.io/os: windows
  containers:
    - name: main
      image: microsoft/windowsservercore:1803
      command:
        - powershell
        - Start-Sleep
        - "-s 3600"
      volumeMounts:
        - mountPath: \\.\pipe\docker-engine
          name: docker-pipe
  volumes:
    - name: docker-pipe
      hostPath: 
        path: \\.\pipe\docker-engine
        type: null
  restartPolicy: Never

docker inspect output for above container:

...
        "Mounts": [
            {
                "Type": "npipe",
                "Source": "\\\\.\\pipe\\docker-engine",
                "Destination": "\\\\.\\pipe\\docker-engine",
                "Mode": "",
                "RW": true,
                "Propagation": ""
            },
            {
                "Type": "bind",
                "Source": "c:\\var\\lib\\kubelet\\pods\\1e664368-c8f7-11e8-97bc-000d3a35a802\\volumes\\kubernetes.io~secret\\default-token-5kttv",
                "Destination": "c:\\var\\run\\secrets\\kubernetes.io\\serviceaccount",
                "Mode": "",
                "RW": false,
                "Propagation": ""
            }
        ],
...

Release note:

Handle Windows named pipes in host mounts.
Correctly handle named pipe host mounts for Windows
Signed-off-by: Deep Debroy <ddebroy@docker.com>
@ddebroy

This comment has been minimized.

Member

ddebroy commented Oct 5, 2018

/sig storage

@ddebroy

This comment has been minimized.

Member

ddebroy commented Oct 5, 2018

/assign @dchen1107

@ddebroy

This comment has been minimized.

Member

ddebroy commented Oct 5, 2018

@ddebroy

This comment has been minimized.

Member

ddebroy commented Oct 6, 2018

/kind bug

@k8s-ci-robot k8s-ci-robot added kind/bug and removed needs-kind labels Oct 6, 2018

@ddebroy

This comment has been minimized.

Member

ddebroy commented Oct 6, 2018

/test pull-kubernetes-verify

@ddebroy

This comment has been minimized.

Member

ddebroy commented Oct 6, 2018

/test pull-kubernetes-local-e2e-containerized

@feiskyer

LGTM

@PatrickLang Would you like also take a look?

@feiskyer

This comment has been minimized.

Member

feiskyer commented Oct 6, 2018

/retest

@@ -945,6 +945,13 @@ func CheckPersistentVolumeClaimModeBlock(pvc *v1.PersistentVolumeClaim) bool {
return utilfeature.DefaultFeatureGate.Enabled(features.BlockVolume) && pvc.Spec.VolumeMode != nil && *pvc.Spec.VolumeMode == v1.PersistentVolumeBlock
}
func IsWindowsNamedPipe(goos, path string) bool {
if goos == "windows" && strings.HasPrefix(path, `\\.\pipe\`) {

This comment has been minimized.

@andyzhangx

andyzhangx Oct 8, 2018

Member

@PatrickLang @atomaras Does named pipe also starts with \\.\pipe\? what about upper case like \\.\Pipe\ ?

This comment has been minimized.

@andyzhangx

andyzhangx Oct 8, 2018

Member

It also supports like this: \.\pipe\ ...

docker run -it --rm -v \.\pipe\docker_engine:\.\pipe\docker_engine microsoft:windowsservercore:1803 powershell

This comment has been minimized.

@ddebroy

ddebroy Oct 8, 2018

Member

I think the logic here needs to be aligned with logic in the runtime. Here are some references:
https://github.com/moby/moby/blob/master/volume/mounts/windows_parser.go#L44
and
https://github.com/moby/moby/blob/master/libcontainerd/client_local_windows.go#L303
Both consider \\.\pipe as the standard named pipe prefix for local pipes on Windows.

@andyzhangx, \.\pipe\... is rejected by Docker:

C:\k>docker run -ti -v \.\pipe\docker_engine:\.\pipe\docker_engine microsoft/windowsservercore:1803
docker: Error response from daemon: invalid volume specification: '\.\pipe\docker_engine:\.\pipe\docker_engine'.

This comment has been minimized.

@PatrickLang

PatrickLang Oct 10, 2018

The underlying CreateFileW() is case insensitive by default, so \\.\pipe\foo == \\.\Pipe\foo
https://docs.microsoft.com/en-us/windows/desktop/api/fileapi/nf-fileapi-createfilew

If it's constrained in the lower layer (moby / containerd), then the same check should be used here. I think it's ok as-is

@bertinatto

This comment has been minimized.

Member

bertinatto commented Oct 8, 2018

I seems like pull-kubernetes-local-e2e-containerized is failing because of #69465 .

@ddebroy

This comment has been minimized.

Member

ddebroy commented Oct 9, 2018

/test pull-kubernetes-local-e2e-containerized

@ddebroy

This comment has been minimized.

Member

ddebroy commented Oct 10, 2018

/retest

@PatrickLang

This comment has been minimized.

PatrickLang commented Oct 10, 2018

I talked to @ddebroy on Slack, and this requires Docker EE-basic 18.03 or later. If you're testing on an Azure VM, you need to be sure to upgrade Docker first. This is tracked at Azure/acs-engine#3852

containerPath := mount.MountPath
if runtime.GOOS == "windows" {
if (strings.HasPrefix(hostPath, "/") || strings.HasPrefix(hostPath, "\\")) && !strings.Contains(hostPath, ":") {
if !volumeutil.IsWindowsNamedPipe(runtime.GOOS, hostPath) && (strings.HasPrefix(hostPath, "/") || strings.HasPrefix(hostPath, "\\")) && !strings.Contains(hostPath, ":") {

This comment has been minimized.

@PatrickLang

PatrickLang Oct 10, 2018

nit - can you preserve the parenthesis so that the grouping is still easy to see?

@ddebroy

This comment has been minimized.

Member

ddebroy commented Oct 17, 2018

@dchen1107 can you please take a look?

@ddebroy

This comment has been minimized.

Member

ddebroy commented Oct 17, 2018

@ddebroy

This comment has been minimized.

Member

ddebroy commented Oct 17, 2018

@Random-Liu @saad-ali PTAL if this looks good to be approved. Not sure if @dchen1107 (who was originally assigned) is online/offline.

@saad-ali

Couple of minor comments.

/approve

// IsWindowsUNCPath checks if path is prefixed with \\
// This can be used to skip any processing of paths
// that point to SMB shares, local named pipes and local UNC path
func IsWindowsUNCPath(goos, path string) bool {

This comment has been minimized.

@saad-ali

saad-ali Oct 19, 2018

Member

nit, why is goos a parameter and not generated inside this method?

This comment has been minimized.

@ddebroy

ddebroy Oct 25, 2018

Member

goos as a parameter allows for an easy way to unit-test the function to make sure in other OS environments, there are no unexpected results.

@@ -218,11 +218,13 @@ func makeMounts(pod *v1.Pod, podDir string, container *v1.Container, hostName, h
// Docker Volume Mounts fail on Windows if it is not of the form C:/
containerPath := mount.MountPath
if runtime.GOOS == "windows" {
if (strings.HasPrefix(hostPath, "/") || strings.HasPrefix(hostPath, "\\")) && !strings.Contains(hostPath, ":") {
// Append C: only if it looks like a local path. Do not process UNC path/SMB shares/named pipes
if (strings.HasPrefix(hostPath, "/") || strings.HasPrefix(hostPath, "\\")) && !strings.Contains(hostPath, ":") && !volumeutil.IsWindowsUNCPath(runtime.GOOS, hostPath) {

This comment has been minimized.

@saad-ali

saad-ali Oct 19, 2018

Member

IsWindowsUNCPath is checking for prefix \ e.g. \foo\bar which is different from what strings.HasPrefix(hostPath, "\") is checking for: \ vs \

That is very subtle. Add a comment explaining this, so folks are less likely to "fix" this in the future by mistake. Also is there any unit test that can be added to kubelet_pods_tests.go for this to catch regressions?

@PatrickLang

This comment has been minimized.

PatrickLang commented Oct 23, 2018

For other reviewers - this change allows Windows mounts that are equivalent to /var/run/docker.sock on Linux. The usual caveats around allowing HostPath mounts should apply to this too.

https://kubernetes.io/docs/concepts/policy/pod-security-policy/#volumes-and-file-systems

@PatrickLang

This comment has been minimized.

PatrickLang commented Oct 23, 2018

/lgtm
/approve

@PatrickLang

This comment has been minimized.

PatrickLang commented Oct 23, 2018

/assign @yujuhong
can you approve or assign to Dawn if you can't?

Improve comments for when hostPath in Windows needs to be transformed
Signed-off-by: Deep Debroy <ddebroy@docker.com>

@k8s-ci-robot k8s-ci-robot removed the lgtm label Oct 25, 2018

@yujuhong

This comment has been minimized.

Contributor

yujuhong commented Oct 25, 2018

/assign @yujuhong
can you approve or assign to Dawn if you can't?

Reviewed and left comments.

Address CR comments and add more tests
Signed-off-by: Deep Debroy <ddebroy@docker.com>

@k8s-ci-robot k8s-ci-robot added size/L and removed size/M labels Oct 26, 2018

@ddebroy

This comment has been minimized.

Member

ddebroy commented Oct 26, 2018

@yujuhong @PatrickLang @saad-ali thanks for reviewing so far. I have addressed the review comments ... PTAL.

// IsWindowsLocalPath checks if path is a local path
// prefixed with "/" or "\" like "/foo/bar" or "\foo\bar"
func IsWindowsLocalPath(goos, path string) bool {

This comment has been minimized.

@yujuhong

yujuhong Oct 26, 2018

Contributor

Should MakeAbsolutePath call this function instead?

This comment has been minimized.

@ddebroy

ddebroy Oct 27, 2018

Member

@yujuhong I would prefer to keep MakeAbsolutePath as is for now in the context of this PR. The scope of this PR is to enable named pipe mounting support in Windows and that code path skips MakeAbsolutePath. MakeAbsolutePath is called in a couple of contexts (not involving named pipes) and maybe refactored in a separate PR later?

hostPath = "c:" + hostPath
}
if volumeutil.IsWindowsLocalPath(runtime.GOOS, hostPath) {
hostPath = "c:" + hostPath

This comment has been minimized.

@yujuhong

yujuhong Oct 26, 2018

Contributor

Should this call MakeAbsolutePath instead?

This comment has been minimized.

@ddebroy

ddebroy Oct 27, 2018

Member

@yujuhong yes, done.

Address code review comments
Signed-off-by: Deep Debroy <ddebroy@docker.com>
@ddebroy

This comment has been minimized.

Member

ddebroy commented Oct 27, 2018

/retest

@ddebroy

This comment has been minimized.

Member

ddebroy commented Oct 30, 2018

@yujuhong PTAL

@yujuhong

This comment has been minimized.

Contributor

yujuhong commented Oct 30, 2018

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Oct 30, 2018

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Oct 30, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ddebroy, PatrickLang, saad-ali, yujuhong

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-ci-robot k8s-ci-robot merged commit 63a7e06 into kubernetes:master Oct 30, 2018

18 checks passed

cla/linuxfoundation ddebroy 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-100-performance 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-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment