feedback: address feedback and fix tests on windows

Author: tam7t

pkg/secrets-store/nodeserver_test.go

@@ -363,7 +363,7 @@ func TestNodeUnpublishVolume(t *testing.T) {
 			name: "Success for a mounted volume with a retry",
 			nodeUnpublishVolReq: csi.NodeUnpublishVolumeRequest{
 				VolumeId:   "testvolid1",
-				TargetPath: tmpdir.New(t, "", `*\\pods\\fakePod\\volumes\\kubernetes.io~csi\\myvol\\mount`),
+				TargetPath: tmpdir.New(t, "", `*mount`),
 			},
 			mountPoints:        []mount.MountPoint{},
 			shouldRetryUnmount: true,

pkg/secrets-store/provider_client_test.go

@@ -22,6 +22,7 @@ import (
 	"fmt"
 	"os"
 	"reflect"
+	"runtime"
 	"sync"
 	"testing"
 	"time"
@@ -64,6 +65,7 @@ func TestMountContent(t *testing.T) {
 		files          []*v1alpha1.File
 		// expectations
 		expectedFiles map[string]os.FileMode
+		skipon        string
 	}{
 		{
 			name:           "provider successful response (no files)",
@@ -77,13 +79,13 @@ func TestMountContent(t *testing.T) {
 			files: []*v1alpha1.File{
 				{
 					Path:     "foo",
-					Mode:     0644,
+					Mode:     0666,
 					Contents: []byte("foo"),
 				},
 			},
 			objectVersions: map[string]string{"foo": "v1"},
 			expectedFiles: map[string]os.FileMode{
-				"foo": 0644,
+				"foo": 0666,
 			},
 		},
 		{
@@ -92,23 +94,23 @@ func TestMountContent(t *testing.T) {
 			files: []*v1alpha1.File{
 				{
 					Path:     "foo",
-					Mode:     0644,
+					Mode:     0666,
 					Contents: []byte("foo"),
 				},
 				{
 					Path:     "bar",
-					Mode:     0777,
+					Mode:     0444,
 					Contents: []byte("bar"),
 				},
 			},
 			objectVersions: map[string]string{"foo": "v1"},
 			expectedFiles: map[string]os.FileMode{
-				"foo": 0644,
-				"bar": 0777,
+				"foo": 0666,
+				"bar": 0444,
 			},
 		},
 		{
-			name:       "provider response with nested files",
+			name:       "provider response with nested files (linux)",
 			permission: "777",
 			files: []*v1alpha1.File{
 				{
@@ -133,11 +135,46 @@ func TestMountContent(t *testing.T) {
 				"baz/bar": 0777,
 				"baz/qux": 0777,
 			},
+			skipon: "windows",
+		},
+		{
+			// note: this is a bit weird because the path `baz\bar` on windows
+			// should be a file `bar` nested in a folder `baz`. it _actually_
+			// works on linux though because the `\` character is just treated
+			// as part of the filename.
+			name:       "provider response with nested files (windows)",
+			permission: "777",
+			files: []*v1alpha1.File{
+				{
+					Path:     "foo",
+					Mode:     0444,
+					Contents: []byte("foo"),
+				},
+				{
+					Path:     "baz\\bar",
+					Mode:     0444,
+					Contents: []byte("bar"),
+				},
+				{
+					Path:     "baz\\qux",
+					Mode:     0666,
+					Contents: []byte("qux"),
+				},
+			},
+			objectVersions: map[string]string{"foo": "v1"},
+			expectedFiles: map[string]os.FileMode{
+				"foo":      0444,
+				"baz\\bar": 0444,
+				"baz\\qux": 0666,
+			},
 		},
 	}
 
 	for _, test := range cases {
 		t.Run(test.name, func(t *testing.T) {
+			if test.skipon == runtime.GOOS {
+				t.SkipNow()
+			}
 			socketPath := tmpdir.New(t, "", "ut")
 			targetPath := tmpdir.New(t, "", "ut")
 

pkg/util/fileutil/filesystem.go

@@ -36,14 +36,40 @@ var (
 func GetMountedFiles(targetPath string) (map[string]string, error) {
 	paths := make(map[string]string)
 
+	// visitor is called for each file walked.
+	// root is the root that will be walked, and base is the starting path
+	// relative to targetPath when walking began
+	visitor := func(root, base string) filepath.WalkFunc {
+		return func(path string, info os.FileInfo, err error) error {
+			// if there was an error walking path immediately propagate it
+			if err != nil {
+				return err
+			}
+
+			// do not include directories in result
+			if info.IsDir() {
+				return nil
+			}
+
+			// find the relative path of the root that was walked
+			rel, err := filepath.Rel(root, path)
+			if err != nil {
+				return err
+			}
+
+			paths[filepath.Join(base, rel)] = path
+			return nil
+		}
+	}
+
 	// atomic writer will write data to, but filepath.Walk does not follow
-	// symlinks
+	// symlinks. follow any symlinks in the targetPath and then walk every item.
 	d, err := os.ReadDir(targetPath)
 	if err != nil {
-		return paths, err
+		return nil, err
 	}
 
-	// for each item in the targetPath, walk that item
+	// for each file/directory in the targetPath, walk that entry
 	for _, entry := range d {
 		// skip the reserved paths of targetPath/..*
 		if strings.HasPrefix(entry.Name(), "..") {
@@ -52,7 +78,7 @@ func GetMountedFiles(targetPath string) (map[string]string, error) {
 
 		info, err := entry.Info()
 		if err != nil {
-			continue
+			return nil, err
 		}
 
 		// the path to the file relative to targetPath
@@ -61,67 +87,24 @@ func GetMountedFiles(targetPath string) (map[string]string, error) {
 
 		// the path before following the symlink
 		// i.e. targetPath/foo
-		p := filepath.Join(targetPath, info.Name())
+		root := filepath.Join(targetPath, base)
 
 		// for symlinks in the targetPath...
 		if info.Mode()&os.ModeSymlink != 0 {
 			// the resolved relative path
 			// i.e. ..data/foo
-			actual, err := os.Readlink(p)
+			actual, err := os.Readlink(root)
 			if err != nil {
-				continue
+				return nil, err
 			}
 
-			// the root path to walk
+			// update the root path to walk to be after following the symlink
 			// i.e. targetPath/..data/foo
-			root := filepath.Join(targetPath, actual)
-
-			if err := filepath.Walk(root, func(path string, info os.FileInfo, err error) error {
-				// if there was an error walking path immediately propagate it
-				if err != nil {
-					return err
-				}
-
-				// do not include directories in result
-				if info.IsDir() {
-					return nil
-				}
-
-				// We want the relative path before following the symbolic link.
-				// Compute the relative path within the symlink'd directory.
-				rel, err := filepath.Rel(root, path)
-				if err != nil {
-					return err
-				}
-				paths[filepath.Join(base, rel)] = path
-
-				return nil
-			}); err != nil {
-				return paths, err
-			}
-		} else {
-			if err := filepath.Walk(p, func(path string, info os.FileInfo, err error) error {
-				// if there was an error walking path immediately propagate it
-				if err != nil {
-					return err
-				}
-
-				// do not include directories in result
-				if info.IsDir() {
-					return nil
-				}
-
-				// determine relative path
-				rel, err := filepath.Rel(targetPath, path)
-				if err != nil {
-					return err
-				}
-				paths[rel] = path
+			root = filepath.Join(targetPath, actual)
+		}
 
-				return nil
-			}); err != nil {
-				return paths, err
-			}
+		if err := filepath.Walk(root, visitor(root, base)); err != nil {
+			return paths, err
 		}
 	}
 	return paths, nil

pkg/util/fileutil/filesystem_test.go

@@ -19,6 +19,7 @@ package fileutil
 import (
 	"os"
 	"path/filepath"
+	"sort"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -49,7 +50,13 @@ func TestGetMountedFiles(t *testing.T) {
 			name: "target path dir/file found",
 			targetPath: func(t *testing.T) string {
 				dir := tmpdir.New(t, "", "ut")
-				os.Create(filepath.Join(dir, "secret.txt"))
+				f, err := os.Create(filepath.Join(dir, "secret.txt"))
+				if err != nil {
+					t.Fatalf("error writing file: %s", err)
+				}
+				if err := f.Close(); err != nil {
+					t.Fatalf("error writing file: %s", err)
+				}
 				return dir
 			},
 			expectedErr: false,
@@ -59,12 +66,20 @@ func TestGetMountedFiles(t *testing.T) {
 			name: "target path dir/dir/file found",
 			targetPath: func(t *testing.T) string {
 				dir := tmpdir.New(t, "", "ut")
-				os.MkdirAll(filepath.Join(dir, "subdir"), 0700)
-				os.Create(filepath.Join(dir, "subdir", "secret.txt"))
+				if err := os.MkdirAll(filepath.Join(dir, "subdir"), 0700); err != nil {
+					t.Fatalf("could not make subdir: %s", err)
+				}
+				f, err := os.Create(filepath.Join(dir, "subdir", "secret.txt"))
+				if err != nil {
+					t.Fatalf("could not write file: %s", err)
+				}
+				if err := f.Close(); err != nil {
+					t.Fatalf("error writing file: %s", err)
+				}
 				return dir
 			},
 			expectedErr: false,
-			want:        []string{"subdir/secret.txt"},
+			want:        []string{filepath.Join("subdir", "secret.txt")},
 		},
 		{
 			name: "target path with atomic_writer symlinks",
@@ -79,14 +94,26 @@ func TestGetMountedFiles(t *testing.T) {
 						Data: []byte("foo"),
 						Mode: 0700,
 					},
+					"foo/baz.txt": {
+						Data: []byte("baz"),
+						Mode: 0700,
+					},
+					"foo.txt": {
+						Data: []byte("foo.txt"),
+						Mode: 0700,
+					},
 				})
 				if err != nil {
 					t.Fatalf("unable to write FileProjection: %s", err)
 				}
 				return dir
 			},
 			expectedErr: false,
-			want:        []string{"foo/bar.txt"},
+			want: []string{
+				"foo.txt",
+				filepath.Join("foo", "bar.txt"),
+				filepath.Join("foo", "baz.txt"),
+			},
 		},
 	}
 
@@ -101,6 +128,7 @@ func TestGetMountedFiles(t *testing.T) {
 			for k := range got {
 				gotKeys = append(gotKeys, k)
 			}
+			sort.Strings(gotKeys)
 
 			if diff := cmp.Diff(test.want, gotKeys, cmpopts.EquateEmpty()); diff != "" {
 				t.Errorf("GetMountedFiles() keys mismatch (-want +got):\n%s", diff)

pkg/util/fileutil/writer_test.go

@@ -23,6 +23,7 @@ import (
 	"io/fs"
 	"os"
 	"path/filepath"
+	"runtime"
 	"strings"
 	"testing"
 
@@ -34,6 +35,7 @@ func TestValidate_Success(t *testing.T) {
 	cases := []struct {
 		name    string
 		payload []*v1alpha1.File
+		skipon  string
 	}{
 		{
 			name: "valid double payload",
@@ -55,17 +57,31 @@ func TestValidate_Success(t *testing.T) {
 			},
 		},
 		{
-			name: "valid nested path",
+			name: "valid nested path (linux)",
 			payload: []*v1alpha1.File{
 				{
 					Path: "foo/bar",
 				},
 			},
+			skipon: "windows",
+		},
+		{
+			name: "valid nested path (windows)",
+			// note: on linux this will be treated as a file with name `foo\bar`
+			// not a file `bar` nested in the directory `foo`.
+			payload: []*v1alpha1.File{
+				{
+					Path: "foo\\bar",
+				},
+			},
 		},
 	}
 
 	for _, tc := range cases {
 		t.Run(tc.name, func(t *testing.T) {
+			if tc.skipon == runtime.GOOS {
+				t.SkipNow()
+			}
 			if err := Validate(tc.payload); err != nil {
 				t.Errorf("%v: unexpected error: %v", tc.name, err)
 			}
@@ -426,9 +442,18 @@ func readPayloads(path string, payloads []*v1alpha1.File) error {
 		if err != nil {
 			return err
 		}
-		if info.Mode() != fs.FileMode(p.Mode) {
-			return fmt.Errorf("unexpected file mode. got: %v, want: %v", info.Mode(), fs.FileMode(p.Mode))
+		if runtime.GOOS == "windows" {
+			// on windows only the 0200 bitmask is used by chmod
+			// https://golang.org/src/os/file.go?s=15847:15891#L522
+			if (info.Mode() & 0200) != (fs.FileMode(p.Mode) & 0200) {
+				return fmt.Errorf("unexpected file mode. got: %v, want: %v", info.Mode(), fs.FileMode(p.Mode))
+			}
+		} else {
+			if info.Mode() != fs.FileMode(p.Mode) {
+				return fmt.Errorf("unexpected file mode. got: %v, want: %v", info.Mode(), fs.FileMode(p.Mode))
+			}
 		}
+
 		contents, err := os.ReadFile(fp)
 		if err != nil {
 			return err