Add more coverage for loader and strengthen type safety.

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,82 @@ 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)
+}
+
+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 +243,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)
 }