Merge pull request #62018 from andyzhangx/local-windows-path-fix

Automatic merge from submit-queue (batch tested with PRs 61147, 62236, 62018). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

fix local volume absolute path issue on Windows

**What this PR does / why we need it**:
remove IsAbs validation on local volume since it does not work on windows cluster, Windows absolute path `D:` is not allowed in local volume, the [validation](https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/core/validation/validation.go#L1386) happens on both master and agent node, while for windows cluster, the master is Linux and agent is Windows, so `path.IsAbs()` func will not work all in both nodes. 
**Instead**, this PR use `MakeAbsolutePath` func to convert `local.path` value in kubelet, it supports both linux and windows styple. 

**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 #62016

**Special notes for your reviewer**:

**Release note**:

```
fix local volume absolute path issue on Windows
```
/sig storage
/sig windows

pkg/apis/core/validation/validation.go

@@ -1383,9 +1383,6 @@ func validateLocalVolumeSource(ls *core.LocalVolumeSource, fldPath *field.Path)
 		return allErrs
 	}
 
-	if !path.IsAbs(ls.Path) {
-		allErrs = append(allErrs, field.Invalid(fldPath, ls.Path, "must be an absolute path"))
-	}
 	allErrs = append(allErrs, validatePathNoBacksteps(ls.Path, fldPath.Child("path"))...)
 	return allErrs
 }

pkg/apis/core/validation/validation_test.go

@@ -521,8 +521,8 @@ func TestValidateLocalVolumes(t *testing.T) {
 			volume: testVolume("foo", "",
 				testLocalVolume("/foo/..", simpleVolumeNodeAffinity("foo", "bar"))),
 		},
-		"invalid-local-volume-relative-path": {
-			isExpectedFailure: true,
+		"valid-local-volume-relative-path": {
+			isExpectedFailure: false,
 			volume: testVolume("foo", "",
 				testLocalVolume("foo", simpleVolumeNodeAffinity("foo", "bar"))),
 		},

pkg/kubelet/kubelet_pods.go

@@ -95,23 +95,6 @@ func (kl *Kubelet) GetActivePods() []*v1.Pod {
 	return activePods
 }
 
-func makeAbsolutePath(goos, path string) string {
-	if goos != "windows" {
-		return "/" + path
-	}
-	// These are all for windows
-	// If there is a colon, give up.
-	if strings.Contains(path, ":") {
-		return path
-	}
-	// If there is a slash, but no drive, add 'c:'
-	if strings.HasPrefix(path, "/") || strings.HasPrefix(path, "\\") {
-		return "c:" + path
-	}
-	// Otherwise, add 'c:\'
-	return "c:\\" + path
-}
-
 // makeBlockVolumes maps the raw block devices specified in the path of the container
 // Experimental
 func (kl *Kubelet) makeBlockVolumes(pod *v1.Pod, container *v1.Container, podVolumes kubecontainer.VolumeMap, blkutil volumepathhandler.BlockVolumePathHandler) ([]kubecontainer.DeviceInfo, error) {
@@ -240,7 +223,7 @@ func makeMounts(pod *v1.Pod, podDir string, container *v1.Container, hostName, h
 			}
 		}
 		if !filepath.IsAbs(containerPath) {
-			containerPath = makeAbsolutePath(runtime.GOOS, containerPath)
+			containerPath = volumeutil.MakeAbsolutePath(runtime.GOOS, containerPath)
 		}
 
 		propagation, err := translateMountPropagation(mount.MountPropagation)

pkg/kubelet/kubelet_pods_test.go

@@ -51,52 +51,6 @@ import (
 	volumetest "k8s.io/kubernetes/pkg/volume/testing"
 )
 
-func TestMakeAbsolutePath(t *testing.T) {
-	tests := []struct {
-		goos         string
-		path         string
-		expectedPath string
-		name         string
-	}{
-		{
-			goos:         "linux",
-			path:         "non-absolute/path",
-			expectedPath: "/non-absolute/path",
-			name:         "basic linux",
-		},
-		{
-			goos:         "windows",
-			path:         "some\\path",
-			expectedPath: "c:\\some\\path",
-			name:         "basic windows",
-		},
-		{
-			goos:         "windows",
-			path:         "/some/path",
-			expectedPath: "c:/some/path",
-			name:         "linux path on windows",
-		},
-		{
-			goos:         "windows",
-			path:         "\\some\\path",
-			expectedPath: "c:\\some\\path",
-			name:         "windows path no drive",
-		},
-		{
-			goos:         "windows",
-			path:         "\\:\\some\\path",
-			expectedPath: "\\:\\some\\path",
-			name:         "windows path with colon",
-		},
-	}
-	for _, test := range tests {
-		path := makeAbsolutePath(test.goos, test.path)
-		if path != test.expectedPath {
-			t.Errorf("[%s] Expected %s saw %s", test.name, test.expectedPath, path)
-		}
-	}
-}
-
 func TestMakeMounts(t *testing.T) {
 	bTrue := true
 	propagationHostToContainer := v1.MountPropagationHostToContainer

pkg/volume/local/BUILD

@@ -32,6 +32,10 @@ go_test(
             "local_test.go",
         ],
         "@io_bazel_rules_go//go/platform:linux": [
+            "local_linux_test.go",
+            "local_test.go",
+        ],
+        "@io_bazel_rules_go//go/platform:windows": [
             "local_test.go",
         ],
         "//conditions:default": [],
@@ -54,6 +58,14 @@ go_test(
             "//vendor/k8s.io/apimachinery/pkg/types:go_default_library",
             "//vendor/k8s.io/client-go/util/testing:go_default_library",
         ],
+        "@io_bazel_rules_go//go/platform:windows": [
+            "//pkg/volume:go_default_library",
+            "//pkg/volume/testing:go_default_library",
+            "//vendor/k8s.io/api/core/v1:go_default_library",
+            "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
+            "//vendor/k8s.io/apimachinery/pkg/types:go_default_library",
+            "//vendor/k8s.io/client-go/util/testing:go_default_library",
+        ],
         "//conditions:default": [],
     }),
 )

pkg/volume/local/local.go

@@ -311,7 +311,8 @@ func (m *localVolumeMounter) SetUpAt(dir string, fsGroup *int64) error {
 	}
 
 	glog.V(4).Infof("attempting to mount %s", dir)
-	err = m.mounter.Mount(m.globalPath, dir, "", options)
+	globalPath := util.MakeAbsolutePath(runtime.GOOS, m.globalPath)
+	err = m.mounter.Mount(globalPath, dir, "", options)
 	if err != nil {
 		glog.Errorf("Mount of volume %s failed: %v", dir, err)
 		notMnt, mntErr := m.mounter.IsNotMountPoint(dir)
@@ -374,8 +375,9 @@ var _ volume.BlockVolumeMapper = &localVolumeMapper{}
 
 // SetUpDevice provides physical device path for the local PV.
 func (m *localVolumeMapper) SetUpDevice() (string, error) {
-	glog.V(4).Infof("SetupDevice returning path %s", m.globalPath)
-	return m.globalPath, nil
+	globalPath := util.MakeAbsolutePath(runtime.GOOS, m.globalPath)
+	glog.V(4).Infof("SetupDevice returning path %s", globalPath)
+	return globalPath, nil
 }
 
 // localVolumeUnmapper implements the BlockVolumeUnmapper interface for local volumes.

pkg/volume/local/local_linux_test.go

@@ -0,0 +1,68 @@
+// +build linux darwin
+
+/*
+Copyright 2017 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package local
+
+import (
+	"os"
+	"syscall"
+	"testing"
+
+	"k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
+)
+
+func TestFSGroupMount(t *testing.T) {
+	tmpDir, plug := getPlugin(t)
+	defer os.RemoveAll(tmpDir)
+	info, err := os.Stat(tmpDir)
+	if err != nil {
+		t.Errorf("Error getting stats for %s (%v)", tmpDir, err)
+	}
+	s := info.Sys().(*syscall.Stat_t)
+	if s == nil {
+		t.Errorf("Error getting stats for %s (%v)", tmpDir, err)
+	}
+	fsGroup1 := int64(s.Gid)
+	fsGroup2 := fsGroup1 + 1
+	pod1 := &v1.Pod{ObjectMeta: metav1.ObjectMeta{UID: types.UID("poduid")}}
+	pod1.Spec.SecurityContext = &v1.PodSecurityContext{
+		FSGroup: &fsGroup1,
+	}
+	pod2 := &v1.Pod{ObjectMeta: metav1.ObjectMeta{UID: types.UID("poduid")}}
+	pod2.Spec.SecurityContext = &v1.PodSecurityContext{
+		FSGroup: &fsGroup2,
+	}
+	err = testFSGroupMount(plug, pod1, tmpDir, fsGroup1)
+	if err != nil {
+		t.Errorf("Failed to make a new Mounter: %v", err)
+	}
+	err = testFSGroupMount(plug, pod2, tmpDir, fsGroup2)
+	if err != nil {
+		t.Errorf("Failed to make a new Mounter: %v", err)
+	}
+	//Checking if GID of tmpDir has not been changed by mounting it by second pod
+	s = info.Sys().(*syscall.Stat_t)
+	if s == nil {
+		t.Errorf("Error getting stats for %s (%v)", tmpDir, err)
+	}
+	if fsGroup1 != int64(s.Gid) {
+		t.Errorf("Old Gid %d for volume %s got overwritten by new Gid %d", fsGroup1, tmpDir, int64(s.Gid))
+	}
+}

pkg/volume/local/local_test.go

@@ -1,4 +1,4 @@
-// +build linux darwin
+// +build linux darwin windows
 
 /*
 Copyright 2017 The Kubernetes Authors.
@@ -22,7 +22,7 @@ import (
 	"fmt"
 	"os"
 	"path"
-	"syscall"
+	"runtime"
 	"testing"
 
 	"k8s.io/api/core/v1"
@@ -199,11 +199,15 @@ func TestMountUnmount(t *testing.T) {
 	if err := mounter.SetUp(nil); err != nil {
 		t.Errorf("Expected success, got: %v", err)
 	}
-	if _, err := os.Stat(path); err != nil {
-		if os.IsNotExist(err) {
-			t.Errorf("SetUp() failed, volume path not created: %s", path)
-		} else {
-			t.Errorf("SetUp() failed: %v", err)
+
+	if runtime.GOOS != "windows" {
+		// skip this check in windows since the "bind mount" logic is implemented differently in mount_wiondows.go
+		if _, err := os.Stat(path); err != nil {
+			if os.IsNotExist(err) {
+				t.Errorf("SetUp() failed, volume path not created: %s", path)
+			} else {
+				t.Errorf("SetUp() failed: %v", err)
+			}
 		}
 	}
 
@@ -260,6 +264,7 @@ func TestMapUnmap(t *testing.T) {
 	if err != nil {
 		t.Errorf("Failed to SetUpDevice, err: %v", err)
 	}
+
 	if _, err := os.Stat(devPath); err != nil {
 		if os.IsNotExist(err) {
 			t.Errorf("SetUpDevice() failed, volume path not created: %s", devPath)
@@ -302,45 +307,6 @@ func testFSGroupMount(plug volume.VolumePlugin, pod *v1.Pod, tmpDir string, fsGr
 	return nil
 }
 
-func TestFSGroupMount(t *testing.T) {
-	tmpDir, plug := getPlugin(t)
-	defer os.RemoveAll(tmpDir)
-	info, err := os.Stat(tmpDir)
-	if err != nil {
-		t.Errorf("Error getting stats for %s (%v)", tmpDir, err)
-	}
-	s := info.Sys().(*syscall.Stat_t)
-	if s == nil {
-		t.Errorf("Error getting stats for %s (%v)", tmpDir, err)
-	}
-	fsGroup1 := int64(s.Gid)
-	fsGroup2 := fsGroup1 + 1
-	pod1 := &v1.Pod{ObjectMeta: metav1.ObjectMeta{UID: types.UID("poduid")}}
-	pod1.Spec.SecurityContext = &v1.PodSecurityContext{
-		FSGroup: &fsGroup1,
-	}
-	pod2 := &v1.Pod{ObjectMeta: metav1.ObjectMeta{UID: types.UID("poduid")}}
-	pod2.Spec.SecurityContext = &v1.PodSecurityContext{
-		FSGroup: &fsGroup2,
-	}
-	err = testFSGroupMount(plug, pod1, tmpDir, fsGroup1)
-	if err != nil {
-		t.Errorf("Failed to make a new Mounter: %v", err)
-	}
-	err = testFSGroupMount(plug, pod2, tmpDir, fsGroup2)
-	if err != nil {
-		t.Errorf("Failed to make a new Mounter: %v", err)
-	}
-	//Checking if GID of tmpDir has not been changed by mounting it by second pod
-	s = info.Sys().(*syscall.Stat_t)
-	if s == nil {
-		t.Errorf("Error getting stats for %s (%v)", tmpDir, err)
-	}
-	if fsGroup1 != int64(s.Gid) {
-		t.Errorf("Old Gid %d for volume %s got overwritten by new Gid %d", fsGroup1, tmpDir, int64(s.Gid))
-	}
-}
-
 func TestConstructVolumeSpec(t *testing.T) {
 	tmpDir, plug := getPlugin(t)
 	defer os.RemoveAll(tmpDir)

pkg/volume/util/util.go

@@ -21,6 +21,7 @@ import (
 	"io/ioutil"
 	"os"
 	"path"
+	"path/filepath"
 	"strings"
 	"syscall"
 
@@ -721,3 +722,21 @@ func CheckVolumeModeFilesystem(volumeSpec *volume.Spec) (bool, error) {
 	}
 	return true, nil
 }
+
+// MakeAbsolutePath convert path to absolute path according to GOOS
+func MakeAbsolutePath(goos, path string) string {
+	if goos != "windows" {
+		return filepath.Clean("/" + path)
+	}
+	// These are all for windows
+	// If there is a colon, give up.
+	if strings.Contains(path, ":") {
+		return path
+	}
+	// If there is a slash, but no drive, add 'c:'
+	if strings.HasPrefix(path, "/") || strings.HasPrefix(path, "\\") {
+		return "c:" + path
+	}
+	// Otherwise, add 'c:\'
+	return "c:\\" + path
+}

pkg/volume/util/util_test.go

@@ -19,6 +19,7 @@ package util
 import (
 	"io/ioutil"
 	"os"
+	"runtime"
 	"testing"
 
 	"k8s.io/api/core/v1"
@@ -976,3 +977,57 @@ func TestGetWindowsPath(t *testing.T) {
 		}
 	}
 }
+
+func TestMakeAbsolutePath(t *testing.T) {
+	tests := []struct {
+		goos         string
+		path         string
+		expectedPath string
+		name         string
+	}{
+		{
+			goos:         "linux",
+			path:         "non-absolute/path",
+			expectedPath: "/non-absolute/path",
+			name:         "linux non-absolute path",
+		},
+		{
+			goos:         "linux",
+			path:         "/absolute/path",
+			expectedPath: "/absolute/path",
+			name:         "linux absolute path",
+		},
+		{
+			goos:         "windows",
+			path:         "some\\path",
+			expectedPath: "c:\\some\\path",
+			name:         "basic windows",
+		},
+		{
+			goos:         "windows",
+			path:         "/some/path",
+			expectedPath: "c:/some/path",
+			name:         "linux path on windows",
+		},
+		{
+			goos:         "windows",
+			path:         "\\some\\path",
+			expectedPath: "c:\\some\\path",
+			name:         "windows path no drive",
+		},
+		{
+			goos:         "windows",
+			path:         "\\:\\some\\path",
+			expectedPath: "\\:\\some\\path",
+			name:         "windows path with colon",
+		},
+	}
+	for _, test := range tests {
+		if runtime.GOOS == test.goos {
+			path := MakeAbsolutePath(test.goos, test.path)
+			if path != test.expectedPath {
+				t.Errorf("[%s] Expected %s saw %s", test.name, test.expectedPath, path)
+			}
+		}
+	}
+}