Merge pull request #570 from monopole/hotYoga

Add test coverage to gitloader.

pkg/internal/loadertest/fakeloader.go

@@ -30,14 +30,13 @@ type FakeLoader struct {
 	delegate ifc.Loader
 }
 
-// NewFakeLoader returns a Loader that delegates calls, and encapsulates
+// NewFakeLoader returns a Loader that uses a fake filesystem.
-// a fake file system that the Loader reads from. "initialDir" parameter
+// The argument should be an absolute file path.
-// must be an full, absolute directory (trailing slash doesn't matter).
 func NewFakeLoader(initialDir string) FakeLoader {
 	// Create fake filesystem and inject it into initial Loader.
 	fSys := fs.MakeFakeFS()
 	fSys.Mkdir(initialDir)
-	ldr, err := loader.NewFileLoaderAtRoot(fSys).New(initialDir)
+	ldr, err := loader.NewLoader(initialDir, fSys)
 	if err != nil {
 		log.Fatalf("Unable to make loader: %v", err)
 	}

pkg/loader/fileloader.go

@@ -26,17 +26,67 @@ import (
 	"sigs.k8s.io/kustomize/pkg/ifc"
 )
 
-// fileLoader loads files from a file system.
+const enforceRelocatable = false
-// It has a notion of a current working directory, called 'root',
+
-// that is independent from the current working directory of the
+// fileLoader loads files, returning an array of bytes.
-// process. When it loads a file from a relative path, the load
+// It also enforces two kustomization requirements:
-// is done relative to this root, not the process CWD.
+//
+//   1) relocatable
+//
+//      A kustomization and the resources, bases,
+//      patches, etc. that it depends on should be
+//      relocatable, so all path specifications
+//      must be relative, not absolute.  The paths
+//      are taken relative to the location of the
+//      kusttomization file.
+//
+//   2) acyclic
+//
+//      There should be no cycles in overlay to base
+//      relationships, including no cycles between
+//      git repositories.
+//
+// The loader has a notion of a current working directory
+// (CWD), called 'root', that is independent of the CWD
+// of the process.  When `Load` is called with a file path
+// argument, the load is done relative to this root,
+// not relative to the process CWD.
+//
+// The loader offers a `New` method returning a new loader
+// with a new root.  The new root can be one of two things,
+// a remote git repo URL, or a directory specified relative
+// to the current root.  In the former case, the remote
+// repository is locally cloned, and the new loader is
+// rooted on a path in that clone.
+//
+// Crucially, a root history is used to so that New fails
+// if its argument either matches or is a parent of the
+// current or any previously used root.
+//
+// This disallows:
+//
+//  * A base that is a git repository that, in turn,
+//    specifies a base repository seen previously
+//    in the loading process (a cycle).
+//
+//  * An overlay depending on a base positioned at or
+//    above it.  I.e. '../foo' is OK, but '.', '..',
+//    '../..', etc. are disallowed.  Allowing such a
+//    base has no advantages and encourage cycles,
+//    particularly if some future change were to
+//    introduce globbing to file specifications in
+//    the kustomization file.
+//
 type fileLoader struct {
-	// Previously visited directories, tracked to avoid cycles.
+	// Previously visited directories, tracked to
-	// The last entry is the current root.
+	// avoid cycles.  The last entry is the current root.
 	roots []string
 	// File system utilities.
 	fSys fs.FileSystem
+	// Used to clone repositories.
+	cloner gitCloner
+	// Used to clean up, as needed.
+	cleaner func() error
 }
 
 // NewFileLoaderAtCwd returns a loader that loads from ".".
@@ -49,14 +99,15 @@ func NewFileLoaderAtRoot(fSys fs.FileSystem) *fileLoader {
 	return newLoaderOrDie(fSys, "/")
 }
 
-// Root returns the absolute path that is prepended to any relative paths
+// Root returns the absolute path that is prepended to any
-// used in Load.
+// relative paths used in Load.
 func (l *fileLoader) Root() string {
 	return l.roots[len(l.roots)-1]
 }
 
 func newLoaderOrDie(fSys fs.FileSystem, path string) *fileLoader {
-	l, err := newFileLoaderAt(fSys, path)
+	l, err := newFileLoaderAt(
+		path, fSys, []string{}, hashicorpGitCloner)
 	if err != nil {
 		log.Fatalf("unable to make loader at '%s'; %v", path, err)
 	}
@@ -64,55 +115,82 @@ func newLoaderOrDie(fSys fs.FileSystem, path string) *fileLoader {
 }
 
 // newFileLoaderAt returns a new fileLoader with given root.
-func newFileLoaderAt(fSys fs.FileSystem, root string) (*fileLoader, error) {
+func newFileLoaderAt(
+	root string, fSys fs.FileSystem,
+	roots []string, cloner gitCloner) (*fileLoader, error) {
+	if root == "" {
+		return nil, fmt.Errorf(
+			"loader root cannot be empty")
+	}
 	root, err := filepath.Abs(root)
 	if err != nil {
 		return nil, fmt.Errorf(
-			"no absolute path for '%s' : %v", root, err)
+			"absolute path error in '%s' : %v", root, err)
 	}
 	if !fSys.IsDir(root) {
-		return nil, fmt.Errorf("absolute root '%s' must exist", root)
+		return nil, fmt.Errorf("absolute root dir '%s' does not exist", root)
 	}
-	return &fileLoader{roots: []string{root}, fSys: fSys}, nil
+	return &fileLoader{
+		roots:   append(roots, root),
+		fSys:    fSys,
+		cloner:  cloner,
+		cleaner: func() error { return nil },
+	}, nil
 }
 
-// Returns a new Loader, which might be rooted relative to current loader.
+// New returns a new Loader, rooted relative to current loader,
+// or rooted in a temp directory holding a git repo clone.
 func (l *fileLoader) New(root string) (ifc.Loader, error) {
 	if root == "" {
 		return nil, fmt.Errorf("new root cannot be empty")
 	}
 	if isRepoUrl(root) {
-		return newGithubLoader(root, l.fSys)
+		if err := l.seenBefore(root); err != nil {
+			return nil, err
+		}
+		return newGitLoader(root, l.fSys, l.roots, l.cloner)
 	}
-	if filepath.IsAbs(root) {
+	if enforceRelocatable && filepath.IsAbs(root) {
-		return l.childLoaderAt(filepath.Clean(root))
+		return nil, fmt.Errorf("new root '%s' cannot be absolute", root)
 	}
-	// Get absolute path to squeeze out "..", ".", etc. to check for cycles.
+	// Get absolute path to squeeze out "..", ".", etc.
+	// to facilitate the seenBefore test.
 	absRoot, err := filepath.Abs(filepath.Join(l.Root(), root))
 	if err != nil {
 		return nil, fmt.Errorf(
-			"problem joining '%s' and '%s': %v", l.Root(), root, err)
+			"problem joining '%s' to '%s': %v", l.Root(), root, err)
 	}
-	return l.childLoaderAt(absRoot)
+	if err := l.seenBefore(absRoot); err != nil {
-}
-
-// childLoaderAt returns a new fileLoader with given root.
-func (l *fileLoader) childLoaderAt(root string) (*fileLoader, error) {
-	if !l.fSys.IsDir(root) {
-		return nil, fmt.Errorf("absolute root '%s' must exist", root)
-	}
-	if err := l.seenBefore(root); err != nil {
 		return nil, err
 	}
-	return &fileLoader{roots: append(l.roots, root), fSys: l.fSys}, nil
+	return newFileLoaderAt(absRoot, l.fSys, l.roots, 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)
+	if err != nil {
+		return nil, err
+	}
+	trueRoot := filepath.Join(tmpDirForRepo, pathInRepo)
+	if !fSys.IsDir(trueRoot) {
+		return nil, fmt.Errorf(
+			"something wrong cloning '%s'; unable to find '%s'",
+			root, trueRoot)
+	}
+	return &fileLoader{
+		roots:   append(roots, root, trueRoot),
+		fSys:    fSys,
+		cloner:  cloner,
+		cleaner: func() error { return fSys.RemoveAll(tmpDirForRepo) },
+	}, nil
 }
 
 // seenBefore tests whether the current or any previously
 // visited root begins with the given path.
-// This disallows an overlay from depending on a base positioned
-// above it.  There's no good reason to allow this, and to disallow
-// it avoid cycles, especially if some future change re-introduces
-// globbing to resource and base specification.
 func (l *fileLoader) seenBefore(path string) error {
 	for _, r := range l.roots {
 		if strings.HasPrefix(r, path) {
@@ -126,13 +204,18 @@ func (l *fileLoader) seenBefore(path string) error {
 
 // Load returns content of file at the given relative path.
 func (l *fileLoader) Load(path string) ([]byte, error) {
-	if !filepath.IsAbs(path) {
+	if filepath.IsAbs(path) {
+		if enforceRelocatable {
+			return nil, fmt.Errorf(
+				"must use relative path; '%s' is absolute", path)
+		}
+	} else {
 		path = filepath.Join(l.Root(), path)
 	}
 	return l.fSys.ReadFile(path)
 }
 
-// Cleanup does nothing
+// Cleanup runs the cleaner.
 func (l *fileLoader) Cleanup() error {
-	return nil
+	return l.cleaner()
 }

pkg/loader/githubloader.go

@@ -20,64 +20,35 @@ import (
 	"io/ioutil"
 	"os"
 	"path/filepath"
-	"sigs.k8s.io/kustomize/pkg/ifc"
 	"strings"
 
 	"github.com/hashicorp/go-getter"
-
-	"sigs.k8s.io/kustomize/pkg/fs"
 )
 
-// githubLoader loads files from a checkout github repo
+// gitCloner is a function that can clone a git repo.
-type githubLoader struct {
+type gitCloner func(url string) (
-	repo string
+	// Directory where the repo is cloned to.
-	// targetDir will hold the local repo
+	checkoutDir string,
-	targetDir string
+	// Relative path in the checkoutDir to location
-	// checkoutDir is for the whole repository
+	// of kustomization file.
-	checkoutDir string
+	pathInCoDir string,
-	loader      *fileLoader
+	// Any error encountered when cloning.
+	err error)
+
+func makeTmpDir() (string, error) {
+	return ioutil.TempDir("", "kustomize-")
 }
 
-// Root returns the root location for this Loader.
+func hashicorpGitCloner(repoUrl string) (
-func (l *githubLoader) Root() string {
+	checkoutDir string, pathInCoDir string, err error) {
-	return l.targetDir
+	dir, err := makeTmpDir()
-}
-
-// New delegates to fileLoader.New
-func (l *githubLoader) New(newRoot string) (ifc.Loader, error) {
-	return l.loader.New(newRoot)
-}
-
-// Load delegates to fileLoader.Load
-func (l *githubLoader) Load(location string) ([]byte, error) {
-	return l.loader.Load(location)
-}
-
-// Cleanup removes the checked out repo
-func (l *githubLoader) Cleanup() error {
-	return os.RemoveAll(l.checkoutDir)
-}
-
-// newGithubLoader returns a new fileLoader with given github Url.
-func newGithubLoader(repoUrl string, fs fs.FileSystem) (*githubLoader, error) {
-	dir, err := ioutil.TempDir("", "kustomize-")
 	if err != nil {
-		return nil, err
+		return
 	}
-	repodir := filepath.Join(dir, "repo")
+	checkoutDir = filepath.Join(dir, "repo")
-	src, subdir := getter.SourceDirSubdir(repoUrl)
+	url, pathInCoDir := getter.SourceDirSubdir(repoUrl)
-	err = checkout(src, repodir)
+	err = checkout(url, checkoutDir)
-	if err != nil {
+	return
-		return nil, err
-	}
-	target := filepath.Join(repodir, subdir)
-	l, _ := newFileLoaderAt(fs, target)
-	return &githubLoader{
-		repo:        repoUrl,
-		targetDir:   target,
-		checkoutDir: repodir,
-		loader:      l,
-	}, nil
 }
 
 // isRepoUrl checks if a string is a repo Url

pkg/loader/githubloader_test.go

@@ -17,7 +17,11 @@ limitations under the License.
 package loader
 
 import (
+	"strings"
 	"testing"
+
+	"sigs.k8s.io/kustomize/pkg/constants"
+	"sigs.k8s.io/kustomize/pkg/fs"
 )
 
 func TestIsRepoURL(t *testing.T) {
@@ -70,3 +74,97 @@ func TestIsRepoURL(t *testing.T) {
 		}
 	}
 }
+
+func splitOnNthSlash(v string, n int) (string, string) {
+	left := ""
+	for i := 0; i < n; i++ {
+		k := strings.Index(v, "/")
+		if k < 0 {
+			break
+		}
+		left = left + v[:k+1]
+		v = v[k+1:]
+	}
+	return left[:len(left)-1], v
+}
+
+func TestSplit(t *testing.T) {
+	path := "a/b/c/d/e/f/g"
+	if left, right := splitOnNthSlash(path, 2); left != "a/b" || right != "c/d/e/f/g" {
+		t.Fatalf("got left='%s', right='%s'", left, right)
+	}
+	if left, right := splitOnNthSlash(path, 3); left != "a/b/c" || right != "d/e/f/g" {
+		t.Fatalf("got left='%s', right='%s'", left, right)
+	}
+	if left, right := splitOnNthSlash(path, 6); left != "a/b/c/d/e/f" || right != "g" {
+		t.Fatalf("got left='%s', right='%s'", left, right)
+	}
+}
+
+// makeFakeGitCloner returns a cloner that ignores the
+// URL argument and returns a path in a fake file system
+// that should already hold the 'repo' contents.
+func makeFakeGitCloner(t *testing.T, fSys fs.FileSystem, coRoot string) gitCloner {
+	if !fSys.IsDir(coRoot) {
+		t.Fatalf("expecting a directory at '%s'", coRoot)
+	}
+	return func(url string) (
+		checkoutDir string, pathInCoDir string, err error) {
+		_, path := splitOnNthSlash(url, 3)
+		if !fSys.IsDir(coRoot + "/" + path) {
+			t.Fatalf("expecting a directory at '%s'/'%s'",
+				coRoot, path)
+		}
+		return coRoot, path, nil
+	}
+}
+
+func TestGitLoader(t *testing.T) {
+	rootUrl := "github.com/someOrg/someRepo"
+	pathInRepo := "foo/base"
+	url := rootUrl + "/" + pathInRepo
+	if !isRepoUrl(url) {
+		t.Fatalf("'%s' should be accepted as a repo url", url)
+	}
+
+	coRoot := "/tmp"
+	fSys := fs.MakeFakeFS()
+	fSys.MkdirAll(coRoot)
+	fSys.MkdirAll(coRoot + "/" + pathInRepo)
+	fSys.WriteFile(
+		coRoot+"/"+pathInRepo+"/"+constants.KustomizationFileName,
+		[]byte(`
+whatever
+`))
+	l, err := newGitLoader(
+		url, fSys, []string{},
+		makeFakeGitCloner(t, fSys, coRoot))
+	if err != nil {
+		t.Fatalf("unexpected err: %v\n", err)
+	}
+	if coRoot+"/"+pathInRepo != l.Root() {
+		t.Fatalf("expected root '%s', got '%s'\n",
+			coRoot+"/"+pathInRepo, l.Root())
+	}
+	if _, err = l.New(url); err == nil {
+		t.Fatalf("expected cycle error")
+	}
+	if _, err = l.New(rootUrl + "/" + "foo"); err == nil {
+		t.Fatalf("expected cycle error")
+	}
+
+	pathInRepo = "foo/overlay"
+	fSys.MkdirAll(coRoot + "/" + pathInRepo)
+	url = rootUrl + "/" + pathInRepo
+	if !isRepoUrl(url) {
+		t.Fatalf("'%s' should be accepted as a repo url", url)
+	}
+	l2, err := l.New(url)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if coRoot+"/"+pathInRepo != l2.Root() {
+		t.Fatalf("expected root '%s', got '%s'\n",
+			coRoot+"/"+pathInRepo, l2.Root())
+	}
+}

pkg/loader/loader.go

@@ -25,7 +25,9 @@ import (
 // NewLoader returns a Loader.
 func NewLoader(root string, fSys fs.FileSystem) (ifc.Loader, error) {
 	if isRepoUrl(root) {
-		return newGithubLoader(root, fSys)
+		return newGitLoader(
+			root, fSys, []string{}, hashicorpGitCloner)
 	}
-	return newFileLoaderAt(fSys, root)
+	return newFileLoaderAt(
+		root, fSys, []string{}, hashicorpGitCloner)
 }