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

Correctly handle named pipe host mounts for Windows #69484

Merged
merged 5 commits into from Oct 30, 2018
Merged

Correctly handle named pipe host mounts for Windows #69484

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b4bb5dd
Correctly handle named pipe host mounts for Windows
ddebroy Oct 5, 2018
f8a69f1
Broaden scope of host path types to skip processing in Windows
ddebroy Oct 13, 2018
2e19f70
Improve comments for when hostPath in Windows needs to be transformed
ddebroy Oct 25, 2018
119e2a1
Address CR comments and add more tests
ddebroy Oct 26, 2018
5da66fd
Address code review comments
ddebroy Oct 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 6 additions & 7 deletions pkg/kubelet/kubelet_pods.go
View file
Open in desktop
Expand Up @@ -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
Expand Down Expand Up @@ -216,13 +215,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, ":") {
hostPath = "c:" + hostPath
}
if volumeutil.IsWindowsLocalPath(runtime.GOOS, hostPath) {
hostPath = "c:" + hostPath
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this call MakeAbsolutePath instead?

Copy link
Member Author

@ddebroy ddebroy Oct 27, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yujuhong yes, done.

}
if !filepath.IsAbs(containerPath) {

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)
}

Expand Down
41 changes: 39 additions & 2 deletions pkg/kubelet/kubelet_pods_windows_test.go
View file
Open in desktop
@@ -1,5 +1,3 @@
// +build windows

/*
Copyright 2017 The Kubernetes Authors.

Expand Down Expand Up @@ -50,13 +48,31 @@ 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,
},
},
}

podVolumes := kubecontainer.VolumeMap{
"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{
Expand Down Expand Up @@ -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)
}
32 changes: 32 additions & 0 deletions pkg/volume/util/util.go
View file
Open in desktop
Expand Up @@ -945,6 +945,38 @@ func CheckPersistentVolumeClaimModeBlock(pvc *v1.PersistentVolumeClaim) bool {
return utilfeature.DefaultFeatureGate.Enabled(features.BlockVolume) && pvc.Spec.VolumeMode != nil && *pvc.Spec.VolumeMode == v1.PersistentVolumeBlock
}

// 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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@ddebroy ddebroy Oct 25, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if goos != "windows" {
return false
}
// Check for UNC prefix \\
if strings.HasPrefix(path, `\\`) {
return true
}
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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should MakeAbsolutePath call this function instead?

Copy link
Member Author

@ddebroy ddebroy Oct 27, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

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" {
Expand Down
142 changes: 142 additions & 0 deletions pkg/volume/util/util_test.go
View file
Open in desktop
Expand Up @@ -2373,6 +2373,148 @@ func TestGetWindowsPath(t *testing.T) {
}
}

func TestIsWindowsUNCPath(t *testing.T) {
tests := []struct {
goos string
path string
isUNCPath bool
}{
{
goos: "linux",
path: `/usr/bin`,
isUNCPath: false,
},
{
goos: "linux",
path: `\\.\pipe\foo`,
isUNCPath: false,
},
{
goos: "windows",
path: `C:\foo`,
isUNCPath: false,
},
{
goos: "windows",
path: `\\server\share\foo`,
isUNCPath: 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 := IsWindowsUNCPath(test.goos, test.path)
if result != test.isUNCPath {
t.Errorf("IsWindowsUNCPath(%v) returned (%v), expected (%v)", test.path, result, test.isUNCPath)
}
}
}

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
Expand Down