From b4bb5dd430671e4d8938868d571104b5131bfffc Mon Sep 17 00:00:00 2001 From: Deep Debroy Date: Fri, 5 Oct 2018 16:46:04 -0700 Subject: [PATCH 1/5] Correctly handle named pipe host mounts for Windows Signed-off-by: Deep Debroy --- pkg/kubelet/kubelet_pods.go | 7 +++--- pkg/volume/util/util.go | 7 ++++++ pkg/volume/util/util_test.go | 41 ++++++++++++++++++++++++++++++++++++ 3 files changed, 52 insertions(+), 3 deletions(-) diff --git a/pkg/kubelet/kubelet_pods.go b/pkg/kubelet/kubelet_pods.go index a00b934df6..20f92997d8 100644 --- a/pkg/kubelet/kubelet_pods.go +++ b/pkg/kubelet/kubelet_pods.go @@ -215,14 +215,15 @@ 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:/ + // Docker Volume Mounts fail on Windows if it is not of the form C:/ nor a named pipe starting with \\.\pipe\ 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, ":") { hostPath = "c:" + hostPath } } - if !filepath.IsAbs(containerPath) { + // IsAbs returns false for named pipes (\\.\pipe\...) in Windows. So check for it specifically and skip MakeAbsolutePath + if !volumeutil.IsWindowsNamedPipe(runtime.GOOS, containerPath) && !filepath.IsAbs(containerPath) { containerPath = volumeutil.MakeAbsolutePath(runtime.GOOS, containerPath) } diff --git a/pkg/volume/util/util.go b/pkg/volume/util/util.go index 72d9a781e7..2426d06610 100644 --- a/pkg/volume/util/util.go +++ b/pkg/volume/util/util.go @@ -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\`) { + return true + } + return false +} + // MakeAbsolutePath convert path to absolute path according to GOOS func MakeAbsolutePath(goos, path string) string { if goos != "windows" { diff --git a/pkg/volume/util/util_test.go b/pkg/volume/util/util_test.go index b1f83ac37c..2cddc632bb 100644 --- a/pkg/volume/util/util_test.go +++ b/pkg/volume/util/util_test.go @@ -2373,6 +2373,47 @@ func TestGetWindowsPath(t *testing.T) { } } +func TestIsWindowsNamedPipe(t *testing.T) { + tests := []struct { + goos string + path string + isNamedPipe bool + }{ + { + goos: "linux", + path: `/usr/bin`, + isNamedPipe: false, + }, + { + goos: "linux", + path: `\\.\pipe\foo`, + isNamedPipe: false, + }, + { + goos: "windows", + path: `C:\foo`, + isNamedPipe: false, + }, + { + goos: "windows", + path: `\\.\invalid`, + isNamedPipe: false, + }, + { + goos: "windows", + path: `\\.\pipe\valid_pipe`, + isNamedPipe: true, + }, + } + + for _, test := range tests { + result := IsWindowsNamedPipe(test.goos, test.path) + if result != test.isNamedPipe { + t.Errorf("IsWindowsNamedPipe(%v) returned (%v), expected (%v)", test.path, result, test.isNamedPipe) + } + } +} + func TestMakeAbsolutePath(t *testing.T) { tests := []struct { goos string From f8a69f10864c8936bfb652e8204e96a682f73f8f Mon Sep 17 00:00:00 2001 From: Deep Debroy Date: Fri, 12 Oct 2018 19:52:50 -0700 Subject: [PATCH 2/5] Broaden scope of host path types to skip processing in Windows Signed-off-by: Deep Debroy --- pkg/kubelet/kubelet_pods.go | 9 +++--- pkg/volume/util/util.go | 11 ++++++-- pkg/volume/util/util_test.go | 54 +++++++++++++++++++++--------------- 3 files changed, 46 insertions(+), 28 deletions(-) diff --git a/pkg/kubelet/kubelet_pods.go b/pkg/kubelet/kubelet_pods.go index 20f92997d8..0cda842d89 100644 --- a/pkg/kubelet/kubelet_pods.go +++ b/pkg/kubelet/kubelet_pods.go @@ -215,15 +215,16 @@ 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:/ nor a named pipe starting with \\.\pipe\ + // Docker Volume Mounts fail on Windows if it is not of the form C:/ containerPath := mount.MountPath if runtime.GOOS == "windows" { - if !volumeutil.IsWindowsNamedPipe(runtime.GOOS, hostPath) && (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) { hostPath = "c:" + hostPath } } - // IsAbs returns false for named pipes (\\.\pipe\...) in Windows. So check for it specifically and skip MakeAbsolutePath - if !volumeutil.IsWindowsNamedPipe(runtime.GOOS, containerPath) && !filepath.IsAbs(containerPath) { + // IsAbs returns false for UNC path/SMB shares/named pipes in Windows. So check for those specifically and skip MakeAbsolutePath + if !volumeutil.IsWindowsUNCPath(runtime.GOOS, containerPath) && !filepath.IsAbs(containerPath) { containerPath = volumeutil.MakeAbsolutePath(runtime.GOOS, containerPath) } diff --git a/pkg/volume/util/util.go b/pkg/volume/util/util.go index 2426d06610..e3ae1bf700 100644 --- a/pkg/volume/util/util.go +++ b/pkg/volume/util/util.go @@ -945,8 +945,15 @@ 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\`) { +// 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 { + if goos != "windows" { + return false + } + // Check for UNC prefix \\ + if strings.HasPrefix(path, `\\`) { return true } return false diff --git a/pkg/volume/util/util_test.go b/pkg/volume/util/util_test.go index 2cddc632bb..1f90fb6721 100644 --- a/pkg/volume/util/util_test.go +++ b/pkg/volume/util/util_test.go @@ -2373,43 +2373,53 @@ func TestGetWindowsPath(t *testing.T) { } } -func TestIsWindowsNamedPipe(t *testing.T) { +func TestIsWindowsUNCPath(t *testing.T) { tests := []struct { - goos string - path string - isNamedPipe bool + goos string + path string + isUNCPath bool }{ { - goos: "linux", - path: `/usr/bin`, - isNamedPipe: false, + goos: "linux", + path: `/usr/bin`, + isUNCPath: false, }, { - goos: "linux", - path: `\\.\pipe\foo`, - isNamedPipe: false, + goos: "linux", + path: `\\.\pipe\foo`, + isUNCPath: false, }, { - goos: "windows", - path: `C:\foo`, - isNamedPipe: false, + goos: "windows", + path: `C:\foo`, + isUNCPath: false, }, { - goos: "windows", - path: `\\.\invalid`, - isNamedPipe: false, + goos: "windows", + path: `\\server\share\foo`, + isUNCPath: true, }, { - goos: "windows", - path: `\\.\pipe\valid_pipe`, - isNamedPipe: true, + goos: "windows", + path: `\\?\server\share`, + isUNCPath: true, + }, + { + goos: "windows", + path: `\\?\c:\`, + isUNCPath: true, + }, + { + goos: "windows", + path: `\\.\pipe\valid_pipe`, + isUNCPath: true, }, } for _, test := range tests { - result := IsWindowsNamedPipe(test.goos, test.path) - if result != test.isNamedPipe { - t.Errorf("IsWindowsNamedPipe(%v) returned (%v), expected (%v)", test.path, result, test.isNamedPipe) + result := IsWindowsUNCPath(test.goos, test.path) + if result != test.isUNCPath { + t.Errorf("IsWindowsUNCPath(%v) returned (%v), expected (%v)", test.path, result, test.isUNCPath) } } } From 2e19f709221fc33e93d40e374450821119e7953b Mon Sep 17 00:00:00 2001 From: Deep Debroy Date: Thu, 25 Oct 2018 13:58:16 -0700 Subject: [PATCH 3/5] Improve comments for when hostPath in Windows needs to be transformed Signed-off-by: Deep Debroy --- pkg/kubelet/kubelet_pods.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/kubelet/kubelet_pods.go b/pkg/kubelet/kubelet_pods.go index 0cda842d89..925da6e98a 100644 --- a/pkg/kubelet/kubelet_pods.go +++ b/pkg/kubelet/kubelet_pods.go @@ -219,6 +219,8 @@ func makeMounts(pod *v1.Pod, podDir string, container *v1.Container, hostName, h containerPath := mount.MountPath if runtime.GOOS == "windows" { // Append C: only if it looks like a local path. Do not process UNC path/SMB shares/named pipes + // NOTE: strings.HasPrefix(hostPath, "\\") checks for prefix "\" as in "\foo\bar" and not "\\foo\bar" + // IsWindowsUNCPath checks for prefix "\\" as in "\\foo\bar" and not "\foo\bar" if (strings.HasPrefix(hostPath, "/") || strings.HasPrefix(hostPath, "\\")) && !strings.Contains(hostPath, ":") && !volumeutil.IsWindowsUNCPath(runtime.GOOS, hostPath) { hostPath = "c:" + hostPath } From 119e2a1d435301814bd75230f555070f94f5529e Mon Sep 17 00:00:00 2001 From: Deep Debroy Date: Fri, 26 Oct 2018 00:29:27 -0700 Subject: [PATCH 4/5] Address CR comments and add more tests Signed-off-by: Deep Debroy --- pkg/kubelet/kubelet_pods.go | 13 ++-- pkg/kubelet/kubelet_pods_windows_test.go | 41 ++++++++++- pkg/volume/util/util.go | 18 +++++ pkg/volume/util/util_test.go | 91 ++++++++++++++++++++++++ 4 files changed, 152 insertions(+), 11 deletions(-) diff --git a/pkg/kubelet/kubelet_pods.go b/pkg/kubelet/kubelet_pods.go index 925da6e98a..5a429108ff 100644 --- a/pkg/kubelet/kubelet_pods.go +++ b/pkg/kubelet/kubelet_pods.go @@ -128,7 +128,6 @@ func (kl *Kubelet) makeBlockVolumes(pod *v1.Pod, container *v1.Container, podVol // makeMounts determines the mount points for the given container. func makeMounts(pod *v1.Pod, podDir string, container *v1.Container, hostName, hostDomain, podIP string, podVolumes kubecontainer.VolumeMap, mounter mountutil.Interface, expandEnvs []kubecontainer.EnvVar) ([]kubecontainer.Mount, func(), error) { - // Kubernetes only mounts on /etc/hosts if: // - container is not an infrastructure (pause) container // - container is not already mounting on /etc/hosts @@ -216,15 +215,11 @@ 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" { - // Append C: only if it looks like a local path. Do not process UNC path/SMB shares/named pipes - // NOTE: strings.HasPrefix(hostPath, "\\") checks for prefix "\" as in "\foo\bar" and not "\\foo\bar" - // IsWindowsUNCPath checks for prefix "\\" as in "\\foo\bar" and not "\foo\bar" - if (strings.HasPrefix(hostPath, "/") || strings.HasPrefix(hostPath, "\\")) && !strings.Contains(hostPath, ":") && !volumeutil.IsWindowsUNCPath(runtime.GOOS, hostPath) { - hostPath = "c:" + hostPath - } + if volumeutil.IsWindowsLocalPath(runtime.GOOS, hostPath) { + hostPath = "c:" + hostPath } + + containerPath := mount.MountPath // IsAbs returns false for UNC path/SMB shares/named pipes in Windows. So check for those specifically and skip MakeAbsolutePath if !volumeutil.IsWindowsUNCPath(runtime.GOOS, containerPath) && !filepath.IsAbs(containerPath) { containerPath = volumeutil.MakeAbsolutePath(runtime.GOOS, containerPath) diff --git a/pkg/kubelet/kubelet_pods_windows_test.go b/pkg/kubelet/kubelet_pods_windows_test.go index 628c2ecdd7..41a2cf6dc0 100644 --- a/pkg/kubelet/kubelet_pods_windows_test.go +++ b/pkg/kubelet/kubelet_pods_windows_test.go @@ -1,5 +1,3 @@ -// +build windows - /* Copyright 2017 The Kubernetes Authors. @@ -50,6 +48,21 @@ func TestMakeMountsWindows(t *testing.T) { Name: "disk5", ReadOnly: false, }, + { + MountPath: `\mnt\path6`, + Name: "disk6", + ReadOnly: false, + }, + { + MountPath: `/mnt/path7`, + Name: "disk7", + ReadOnly: false, + }, + { + MountPath: `\\.\pipe\pipe1`, + Name: "pipe1", + ReadOnly: false, + }, }, } @@ -57,6 +70,9 @@ func TestMakeMountsWindows(t *testing.T) { "disk": kubecontainer.VolumeInfo{Mounter: &stubVolume{path: "c:/mnt/disk"}}, "disk4": kubecontainer.VolumeInfo{Mounter: &stubVolume{path: "c:/mnt/host"}}, "disk5": kubecontainer.VolumeInfo{Mounter: &stubVolume{path: "c:/var/lib/kubelet/podID/volumes/empty/disk5"}}, + "disk6": kubecontainer.VolumeInfo{Mounter: &stubVolume{path: `/mnt/disk6`}}, + "disk7": kubecontainer.VolumeInfo{Mounter: &stubVolume{path: `\mnt\disk7`}}, + "pipe1": kubecontainer.VolumeInfo{Mounter: &stubVolume{path: `\\.\pipe\pipe1`}}, } pod := v1.Pod{ @@ -97,6 +113,27 @@ func TestMakeMountsWindows(t *testing.T) { ReadOnly: false, SELinuxRelabel: false, }, + { + Name: "disk6", + ContainerPath: `c:\mnt\path6`, + HostPath: `c:/mnt/disk6`, + ReadOnly: false, + SELinuxRelabel: false, + }, + { + Name: "disk7", + ContainerPath: `c:/mnt/path7`, + HostPath: `c:\mnt\disk7`, + ReadOnly: false, + SELinuxRelabel: false, + }, + { + Name: "pipe1", + ContainerPath: `\\.\pipe\pipe1`, + HostPath: `\\.\pipe\pipe1`, + ReadOnly: false, + SELinuxRelabel: false, + }, } assert.Equal(t, expectedMounts, mounts, "mounts of container %+v", container) } diff --git a/pkg/volume/util/util.go b/pkg/volume/util/util.go index e3ae1bf700..a9569ef7ab 100644 --- a/pkg/volume/util/util.go +++ b/pkg/volume/util/util.go @@ -959,6 +959,24 @@ func IsWindowsUNCPath(goos, path string) bool { return false } +// IsWindowsLocalPath checks if path is a local path +// prefixed with "/" or "\" like "/foo/bar" or "\foo\bar" +func IsWindowsLocalPath(goos, path string) bool { + if goos != "windows" { + return false + } + if IsWindowsUNCPath(goos, path) { + return false + } + if strings.Contains(path, ":") { + return false + } + if !(strings.HasPrefix(path, `/`) || strings.HasPrefix(path, `\`)) { + return false + } + return true +} + // MakeAbsolutePath convert path to absolute path according to GOOS func MakeAbsolutePath(goos, path string) string { if goos != "windows" { diff --git a/pkg/volume/util/util_test.go b/pkg/volume/util/util_test.go index 1f90fb6721..d579599adb 100644 --- a/pkg/volume/util/util_test.go +++ b/pkg/volume/util/util_test.go @@ -2424,6 +2424,97 @@ func TestIsWindowsUNCPath(t *testing.T) { } } +func TestIsWindowsLocalPath(t *testing.T) { + tests := []struct { + goos string + path string + isWindowsLocalPath bool + }{ + { + goos: "linux", + path: `/usr/bin`, + isWindowsLocalPath: false, + }, + { + goos: "linux", + path: `\\.\pipe\foo`, + isWindowsLocalPath: false, + }, + { + goos: "windows", + path: `C:\foo`, + isWindowsLocalPath: false, + }, + { + goos: "windows", + path: `:\foo`, + isWindowsLocalPath: false, + }, + { + goos: "windows", + path: `X:\foo`, + isWindowsLocalPath: false, + }, + { + goos: "windows", + path: `\\server\share\foo`, + isWindowsLocalPath: false, + }, + { + goos: "windows", + path: `\\?\server\share`, + isWindowsLocalPath: false, + }, + { + goos: "windows", + path: `\\?\c:\`, + isWindowsLocalPath: false, + }, + { + goos: "windows", + path: `\\.\pipe\valid_pipe`, + isWindowsLocalPath: false, + }, + { + goos: "windows", + path: `foo`, + isWindowsLocalPath: false, + }, + { + goos: "windows", + path: `:foo`, + isWindowsLocalPath: false, + }, + { + goos: "windows", + path: `\foo`, + isWindowsLocalPath: true, + }, + { + goos: "windows", + path: `\foo\bar`, + isWindowsLocalPath: true, + }, + { + goos: "windows", + path: `/foo`, + isWindowsLocalPath: true, + }, + { + goos: "windows", + path: `/foo/bar`, + isWindowsLocalPath: true, + }, + } + + for _, test := range tests { + result := IsWindowsLocalPath(test.goos, test.path) + if result != test.isWindowsLocalPath { + t.Errorf("isWindowsLocalPath(%v) returned (%v), expected (%v)", test.path, result, test.isWindowsLocalPath) + } + } +} + func TestMakeAbsolutePath(t *testing.T) { tests := []struct { goos string From 5da66fd65fef91ac23cd3951cdf513881a238acd Mon Sep 17 00:00:00 2001 From: Deep Debroy Date: Sat, 27 Oct 2018 00:31:16 -0700 Subject: [PATCH 5/5] Address code review comments Signed-off-by: Deep Debroy --- pkg/kubelet/kubelet_pods.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/kubelet/kubelet_pods.go b/pkg/kubelet/kubelet_pods.go index 5a429108ff..eb519c3f38 100644 --- a/pkg/kubelet/kubelet_pods.go +++ b/pkg/kubelet/kubelet_pods.go @@ -216,7 +216,7 @@ 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:/ if volumeutil.IsWindowsLocalPath(runtime.GOOS, hostPath) { - hostPath = "c:" + hostPath + hostPath = volumeutil.MakeAbsolutePath(runtime.GOOS, hostPath) } containerPath := mount.MountPath