Merge pull request #726 from monopole/refactorLoader

Add more coverage for loader and strengthen type safety

pkg/fs/confirmeddir.go

@@ -0,0 +1,73 @@
+/*
+Copyright 2018 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 fs
+
+import (
+	"path/filepath"
+	"strings"
+)
+
+// ConfirmedDir is a clean, absolute, delinkified path
+// that was confirmed to point to an existing directory.
+type ConfirmedDir string
+
+// HasPrefix returns true if the directory argument
+// is a prefix of self (d) from the point of view of
+// a file system.
+//
+// I.e., it's true if the argument equals or contains
+// self (d) in a file path sense.
+//
+// HasPrefix emulates the semantics of strings.HasPrefix
+// such that the following are true:
+//
+//   strings.HasPrefix("foobar", "foobar")
+//   strings.HasPrefix("foobar", "foo")
+//   strings.HasPrefix("foobar", "")
+//
+//   d := fSys.ConfirmDir("/foo/bar")
+//   d.HasPrefix("/foo/bar")
+//   d.HasPrefix("/foo")
+//   d.HasPrefix("/")
+//
+// Not contacting a file system here to check for
+// actual path existence.
+//
+// This is tested on linux, but will have trouble
+// on other operating systems.
+// TODO(monopole) Refactor when #golang/go/18358 closes.
+// See also:
+//   https://github.com/golang/go/issues/18358
+//   https://github.com/golang/dep/issues/296
+//   https://github.com/golang/dep/blob/master/internal/fs/fs.go#L33
+//   https://codereview.appspot.com/5712045
+func (d ConfirmedDir) HasPrefix(path ConfirmedDir) bool {
+	if path.String() == string(filepath.Separator) || path == d {
+		return true
+	}
+	return strings.HasPrefix(
+		string(d),
+		string(path)+string(filepath.Separator))
+}
+
+func (d ConfirmedDir) Join(path string) string {
+	return filepath.Join(string(d), path)
+}
+
+func (d ConfirmedDir) String() string {
+	return string(d)
+}

pkg/fs/confirmeddir_test.go

@@ -0,0 +1,103 @@
+/*
+Copyright 2018 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 fs
+
+import (
+	"testing"
+)
+
+func TestJoin(t *testing.T) {
+	fSys := MakeFakeFS()
+	err := fSys.Mkdir("/foo")
+	if err != nil {
+		t.Fatalf("unexpected err: %v", err)
+	}
+	d, f, err := fSys.CleanedAbs("/foo")
+	if err != nil {
+		t.Fatalf("unexpected err: %v", err)
+	}
+	if f != "" {
+		t.Fatalf("unexpected file: %v", f)
+	}
+	if d.Join("bar") != "/foo/bar" {
+		t.Fatalf("expected join %s", d.Join("bar"))
+	}
+}
+
+func TestHasPrefix_Slash(t *testing.T) {
+	d, f, err := MakeFakeFS().CleanedAbs("/")
+	if err != nil {
+		t.Fatalf("unexpected err: %v", err)
+	}
+	if f != "" {
+		t.Fatalf("unexpected file: %v", f)
+	}
+	if d.HasPrefix("/hey") {
+		t.Fatalf("should be false")
+	}
+	if !d.HasPrefix("/") {
+		t.Fatalf("/ should have the prefix /")
+	}
+}
+
+func TestHasPrefix_SlashFoo(t *testing.T) {
+	fSys := MakeFakeFS()
+	err := fSys.Mkdir("/foo")
+	if err != nil {
+		t.Fatalf("unexpected err: %v", err)
+	}
+	d, _, err := fSys.CleanedAbs("/foo")
+	if err != nil {
+		t.Fatalf("unexpected err: %v", err)
+	}
+	if d.HasPrefix("/fo") {
+		t.Fatalf("/foo does not have path prefix /fo")
+	}
+	if d.HasPrefix("/fod") {
+		t.Fatalf("/foo does not have path prefix /fod")
+	}
+	if !d.HasPrefix("/foo") {
+		t.Fatalf("/foo should have prefix /foo")
+	}
+}
+
+func TestHasPrefix_SlashFooBar(t *testing.T) {
+	fSys := MakeFakeFS()
+	err := fSys.MkdirAll("/foo/bar")
+	if err != nil {
+		t.Fatalf("unexpected err: %v", err)
+	}
+	d, _, err := fSys.CleanedAbs("/foo/bar")
+	if err != nil {
+		t.Fatalf("unexpected err: %v", err)
+	}
+	if d.HasPrefix("/fo") {
+		t.Fatalf("/foo/bar does not have path prefix /fo")
+	}
+	if d.HasPrefix("/foobar") {
+		t.Fatalf("/foo/bar does not have path prefix /foobar")
+	}
+	if !d.HasPrefix("/foo/bar") {
+		t.Fatalf("/foo/bar should have prefix /foo/bar")
+	}
+	if !d.HasPrefix("/foo") {
+		t.Fatalf("/foo/bar should have prefix /foo")
+	}
+	if !d.HasPrefix("/") {
+		t.Fatalf("/foo/bar should have prefix /")
+	}
+}

pkg/fs/fakefs.go

@@ -102,16 +102,16 @@ func (fs *fakeFs) Open(name string) (File, error) {
 	return fs.m[name], nil
 }
 
-// CleanedAbs does nothing and cannot fail.
-func (fs *fakeFs) CleanedAbs(path string) (string, string, error) {
+// CleanedAbs cannot fail.
+func (fs *fakeFs) CleanedAbs(path string) (ConfirmedDir, string, error) {
 	if fs.IsDir(path) {
-		return path, "", nil
+		return ConfirmedDir(path), "", nil
 	}
 	d := filepath.Dir(path)
 	if d == path {
-		return d, "", nil
+		return ConfirmedDir(d), "", nil
 	}
-	return d, filepath.Base(path), nil
+	return ConfirmedDir(d), filepath.Base(path), nil
 }
 
 // Exists returns true if file is known.

pkg/fs/fs.go

@@ -30,7 +30,7 @@ type FileSystem interface {
 	RemoveAll(name string) error
 	Open(name string) (File, error)
 	IsDir(name string) bool
-	CleanedAbs(path string) (string, string, error)
+	CleanedAbs(path string) (ConfirmedDir, string, error)
 	Exists(name string) bool
 	Glob(pattern string) ([]string, error)
 	ReadFile(name string) ([]byte, error)

pkg/fs/realfs.go

@@ -60,7 +60,8 @@ func (realFS) Open(name string) (File, error) { return os.Open(name) }
 // and file components.  If the entire path is
 // a directory, the file component is an empty
 // string.
-func (x realFS) CleanedAbs(path string) (string, string, error) {
+func (x realFS) CleanedAbs(
+	path string) (ConfirmedDir, string, error) {
 	absRoot, err := filepath.Abs(path)
 	if err != nil {
 		return "", "", fmt.Errorf(
@@ -72,7 +73,7 @@ func (x realFS) CleanedAbs(path string) (string, string, error) {
 			"evalsymlink failure on '%s' : %v", path, err)
 	}
 	if x.IsDir(deLinked) {
-		return deLinked, "", nil
+		return ConfirmedDir(deLinked), "", nil
 	}
 	d := filepath.Dir(deLinked)
 	if !x.IsDir(d) {
@@ -89,7 +90,7 @@ func (x realFS) CleanedAbs(path string) (string, string, error) {
 		log.Fatalf("these should be equal: '%s', '%s'",
 			filepath.Join(d, f), deLinked)
 	}
-	return d, f, nil
+	return ConfirmedDir(d), f, nil
 }
 
 // Exists returns true if os.Stat succeeds.

pkg/fs/realfs_test.go

@@ -52,7 +52,7 @@ func TestCleanedAbs_1(t *testing.T) {
 	if err != nil {
 		t.Fatalf("unexpected err=%v", err)
 	}
-	if d != wd {
+	if d.String() != wd {
 		t.Fatalf("unexpected d=%s", d)
 	}
 	if f != "" {
@@ -90,7 +90,7 @@ func TestCleanedAbs_3(t *testing.T) {
 	if err != nil {
 		t.Fatalf("unexpected err=%v", err)
 	}
-	if d != testDir {
+	if d.String() != testDir {
 		t.Fatalf("unexpected d=%s", d)
 	}
 	if f != "foo" {
@@ -119,7 +119,7 @@ func TestCleanedAbs_4(t *testing.T) {
 	if err != nil {
 		t.Fatalf("unexpected err=%v", err)
 	}
-	if d != filepath.Join(testDir, "d1", "d2") {
+	if d.String() != filepath.Join(testDir, "d1", "d2") {
 		t.Fatalf("unexpected d=%s", d)
 	}
 	if f != "" {
@@ -131,7 +131,7 @@ func TestCleanedAbs_4(t *testing.T) {
 	if err != nil {
 		t.Fatalf("unexpected err=%v", err)
 	}
-	if d != filepath.Join(testDir, "d1", "d2") {
+	if d.String() != filepath.Join(testDir, "d1", "d2") {
 		t.Fatalf("unexpected d=%s", d)
 	}
 	if f != "bar" {

pkg/loader/fileloader.go

@@ -78,10 +78,16 @@ import (
 // from /etc/passwd will fail.
 //
 type fileLoader struct {
-	// Previously visited absolute directory paths.
-	// Tracked to avoid cycles.
-	// The last entry is the current root.
-	roots []string
+	// Loader that spawned this loader.
+	// Used to avoid cycles.
+	referrer *fileLoader
+	// An absolute, cleaned path to a directory.
+	// The Load function reads from this directory,
+	// or directories below it.
+	root fs.ConfirmedDir
+	// URI, if any, used for a download into root.
+	// TODO(monopole): use non-string type.
+	uri string
 	// File system utilities.
 	fSys fs.FileSystem
 	// Used to clone repositories.
@@ -103,12 +109,12 @@ func NewFileLoaderAtRoot(fSys fs.FileSystem) *fileLoader {
 // Root returns the absolute path that is prepended to any
 // relative paths used in Load.
 func (l *fileLoader) Root() string {
-	return l.roots[len(l.roots)-1]
+	return l.root.String()
 }
 
 func newLoaderOrDie(fSys fs.FileSystem, path string) *fileLoader {
 	l, err := newFileLoaderAt(
-		path, fSys, []string{}, simpleGitCloner)
+		path, fSys, nil, simpleGitCloner)
 	if err != nil {
 		log.Fatalf("unable to make loader at '%s'; %v", path, err)
 	}
@@ -117,34 +123,33 @@ func newLoaderOrDie(fSys fs.FileSystem, path string) *fileLoader {
 
 // newFileLoaderAt returns a new fileLoader with given root.
 func newFileLoaderAt(
-	root string, fSys fs.FileSystem,
-	roots []string, cloner gitCloner) (*fileLoader, error) {
-	if root == "" {
+	possibleRoot string, fSys fs.FileSystem,
+	referrer *fileLoader, cloner gitCloner) (*fileLoader, error) {
+	if possibleRoot == "" {
 		return nil, fmt.Errorf(
 			"loader root cannot be empty")
 	}
-	absRoot, f, err := fSys.CleanedAbs(root)
+	root, f, err := fSys.CleanedAbs(possibleRoot)
 	if err != nil {
 		return nil, fmt.Errorf(
-			"absolute path error in '%s' : %v", root, err)
+			"absolute path error in '%s' : %v", possibleRoot, err)
 	}
 	if f != "" {
 		return nil, fmt.Errorf(
 			"got file '%s', but '%s' must be a directory to be a root",
-			f, root)
+			f, possibleRoot)
 	}
-	if err := isPathEqualToOrAbove(absRoot, roots); err != nil {
-		return nil, err
-	}
-	if !fSys.IsDir(absRoot) {
-		return nil, fmt.Errorf(
-			"'%s' does not exist or is not a directory", absRoot)
+	if referrer != nil {
+		if err := referrer.errIfArgEqualOrHigher(root); err != nil {
+			return nil, err
+		}
 	}
 	return &fileLoader{
-		roots:   append(roots, absRoot),
-		fSys:    fSys,
-		cloner:  cloner,
-		cleaner: func() error { return nil },
+		root:     root,
+		referrer: referrer,
+		fSys:     fSys,
+		cloner:   cloner,
+		cleaner:  func() error { return nil },
 	}, nil
 }
 
@@ -155,68 +160,86 @@ func (l *fileLoader) New(path string) (ifc.Loader, error) {
 		return nil, fmt.Errorf("new root cannot be empty")
 	}
 	if isRepoUrl(path) {
-		// This works well enough for purpose at hand to detect
-		// previously visited URLs and thus avoid cycles.
-		if err := isPathEqualToOrAbove(path, l.roots); err != nil {
+		// Avoid cycles.
+		if err := l.errIfPreviouslySeenUri(path); err != nil {
 			return nil, err
 		}
-		return newGitLoader(path, l.fSys, l.roots, l.cloner)
+		return newGitLoader(path, l.fSys, l.referrer, l.cloner)
 	}
 	if filepath.IsAbs(path) {
 		return nil, fmt.Errorf("new root '%s' cannot be absolute", path)
 	}
 	return newFileLoaderAt(
-		filepath.Join(l.Root(), path), l.fSys, l.roots, l.cloner)
+		l.root.Join(path), l.fSys, l, l.cloner)
 }
 
 // newGitLoader returns a new Loader pinned to a temporary
 // directory holding a cloned git repo.
 func newGitLoader(
-	root string, fSys fs.FileSystem,
-	roots []string, cloner gitCloner) (ifc.Loader, error) {
-	tmpDirForRepo, pathInRepo, err := cloner(root)
+	uri string, fSys fs.FileSystem,
+	referrer *fileLoader, cloner gitCloner) (ifc.Loader, error) {
+	tmpDirForRepo, pathInRepo, err := cloner(uri)
 	if err != nil {
 		return nil, err
 	}
-	trueRoot := filepath.Join(tmpDirForRepo, pathInRepo)
-	if !fSys.IsDir(trueRoot) {
+	root, f, err := fSys.CleanedAbs(
+		filepath.Join(tmpDirForRepo, pathInRepo))
+	if err != nil {
+		return nil, err
+	}
+	if f != "" {
 		return nil, fmt.Errorf(
-			"something wrong cloning '%s'; unable to find '%s'",
-			root, trueRoot)
+			"'%s' refers to file '%s'; expecting directory", pathInRepo, f)
 	}
 	return &fileLoader{
-		roots:   append(roots, root, trueRoot),
-		fSys:    fSys,
-		cloner:  cloner,
-		cleaner: func() error { return fSys.RemoveAll(tmpDirForRepo) },
+		root:     root,
+		referrer: referrer,
+		uri:      uri,
+		fSys:     fSys,
+		cloner:   cloner,
+		cleaner:  func() error { return fSys.RemoveAll(tmpDirForRepo) },
 	}, nil
 }
 
-// isPathEqualToOrAbove tests whether the 1st argument,
-// viewed as a path to a directory, is equal to or above
-// any of the paths in the 2nd argument.  It's assumed
-// that all paths are cleaned, delinkified and absolute.
-func isPathEqualToOrAbove(path string, roots []string) error {
-	terminated := path + string(filepath.Separator)
-	for _, r := range roots {
-		if r == path || strings.HasPrefix(r, terminated) {
-			return fmt.Errorf(
-				"cycle detected: new root '%s' contains previous root '%s'",
-				path, r)
-		}
+// errIfArgEqualOrHigher tests whether the argument,
+// is equal to or above the root of any ancestor.
+func (l *fileLoader) errIfArgEqualOrHigher(
+	candidateRoot fs.ConfirmedDir) error {
+	if l.root.HasPrefix(candidateRoot) {
+		return fmt.Errorf(
+			"cycle detected: candidate root '%s' contains visited root '%s'",
+			candidateRoot, l.root)
 	}
-	return nil
+	if l.referrer == nil {
+		return nil
+	}
+	return l.referrer.errIfArgEqualOrHigher(candidateRoot)
+}
+
+// TODO(monopole): Distinguish branches?
+// I.e. Allow a distinction between git URI with
+// path foo and tag bar and a git URI with the same
+// path but a different tag?
+func (l *fileLoader) errIfPreviouslySeenUri(uri string) error {
+	if strings.HasPrefix(l.uri, uri) {
+		return fmt.Errorf(
+			"cycle detected: URI '%s' referenced by previous URI '%s'",
+			uri, l.uri)
+	}
+	if l.referrer == nil {
+		return nil
+	}
+	return l.referrer.errIfPreviouslySeenUri(uri)
 }
 
 // Load returns content of file at the given relative path,
 // else an error.  The path must refer to a file in or
-// below the current Root().
+// below the current root.
 func (l *fileLoader) Load(path string) ([]byte, error) {
 	if filepath.IsAbs(path) {
 		return nil, l.loadOutOfBounds(path)
 	}
-	d, f, err := l.fSys.CleanedAbs(
-		filepath.Join(l.Root(), path))
+	d, f, err := l.fSys.CleanedAbs(l.root.Join(path))
 	if err != nil {
 		return nil, err
 	}
@@ -224,41 +247,16 @@ func (l *fileLoader) Load(path string) ([]byte, error) {
 		return nil, fmt.Errorf(
 			"'%s' must be a file (got d='%s')", path, d)
 	}
-	path = filepath.Join(d, f)
-	if !l.isInOrBelowRoot(path) {
+	if !d.HasPrefix(l.root) {
 		return nil, l.loadOutOfBounds(path)
 	}
-	return l.fSys.ReadFile(path)
-}
-
-// isInOrBelowRoot is true if the argument is in or
-// below Root() from purely a path perspective (no
-// check for actual file existence). For this to work,
-// both the given argument "path" and l.Root() must
-// be cleaned, absolute paths, and l.Root() must be
-// a directory.  Both conditions enforced elsewhere.
-//
-// This is tested on linux, but will have trouble
-// on other operating systems.  As soon as related
-// work is completed in the core filepath package,
-// this code should be refactored to use it.
-// See:
-//   https://github.com/golang/go/issues/18355
-//   https://github.com/golang/dep/issues/296
-//   https://github.com/golang/dep/blob/master/internal/fs/fs.go#L33
-//   https://codereview.appspot.com/5712045
-func (l *fileLoader) isInOrBelowRoot(path string) bool {
-	if l.Root() == string(filepath.Separator) {
-		return true
-	}
-	return strings.HasPrefix(
-		path, l.Root()+string(filepath.Separator))
+	return l.fSys.ReadFile(d.Join(f))
 }
 
 func (l *fileLoader) loadOutOfBounds(path string) error {
 	return fmt.Errorf(
 		"security; file '%s' is not in or below '%s'",
-		path, l.Root())
+		path, l.root)
 }
 
 // Cleanup runs the cleaner.

pkg/loader/fileloader_test.go

@@ -63,15 +63,15 @@ func MakeFakeFs(td []testData) fs.FileSystem {
 
 func TestNewFileLoaderAt_DemandsDirectory(t *testing.T) {
 	fSys := MakeFakeFs(testCases)
-	_, err := newFileLoaderAt("/foo", fSys, []string{}, nil)
+	_, err := newFileLoaderAt("/foo", fSys, nil, nil)
 	if err != nil {
 		t.Fatalf("Unexpected error - a directory should work.")
 	}
-	_, err = newFileLoaderAt("/foo/project", fSys, []string{}, nil)
+	_, err = newFileLoaderAt("/foo/project", fSys, nil, nil)
 	if err != nil {
 		t.Fatalf("Unexpected error - a directory should work.")
 	}
-	_, err = newFileLoaderAt("/foo/project/fileA.yaml", fSys, []string{}, nil)
+	_, err = newFileLoaderAt("/foo/project/fileA.yaml", fSys, nil, nil)
 	if err == nil {
 		t.Fatalf("Expected error - a file should not work.")
 	}

pkg/loader/gitcloner_test.go

@@ -167,7 +167,7 @@ func TestGitLoader(t *testing.T) {
 whatever
 `))
 	l, err := newGitLoader(
-		url, fSys, []string{},
+		url, fSys, nil,
 		makeFakeGitCloner(t, fSys, coRoot))
 	if err != nil {
 		t.Fatalf("unexpected err: %v\n", err)
@@ -177,10 +177,10 @@ whatever
 			coRoot+"/"+pathInRepo, l.Root())
 	}
 	if _, err = l.New(url); err == nil {
-		t.Fatalf("expected cycle error")
+		t.Fatalf("expected cycle error 1")
 	}
 	if _, err = l.New(rootUrl + "/" + "foo"); err == nil {
-		t.Fatalf("expected cycle error")
+		t.Fatalf("expected cycle error 2")
 	}
 
 	pathInRepo = "foo/overlay"

pkg/loader/loader.go

@@ -26,8 +26,8 @@ import (
 func NewLoader(root string, fSys fs.FileSystem) (ifc.Loader, error) {
 	if isRepoUrl(root) {
 		return newGitLoader(
-			root, fSys, []string{}, simpleGitCloner)
+			root, fSys, nil, simpleGitCloner)
 	}
 	return newFileLoaderAt(
-		root, fSys, []string{}, simpleGitCloner)
+		root, fSys, nil, simpleGitCloner)
 }