From 4daa65551641ec329259d8374e895aefd075eb6e Mon Sep 17 00:00:00 2001 From: jregan Date: Thu, 22 Nov 2018 08:27:25 -0800 Subject: [PATCH] Add test coverage to gitloader. --- pkg/internal/loadertest/fakeloader.go | 7 +- pkg/loader/fileloader.go | 157 ++++++++++++++++++++------ pkg/loader/githubloader.go | 69 ++++------- pkg/loader/githubloader_test.go | 98 ++++++++++++++++ pkg/loader/loader.go | 6 +- 5 files changed, 245 insertions(+), 92 deletions(-) diff --git a/pkg/internal/loadertest/fakeloader.go b/pkg/internal/loadertest/fakeloader.go index 1ac13917d..c882c239e 100644 --- a/pkg/internal/loadertest/fakeloader.go +++ b/pkg/internal/loadertest/fakeloader.go @@ -30,14 +30,13 @@ type FakeLoader struct { delegate ifc.Loader } -// NewFakeLoader returns a Loader that delegates calls, and encapsulates -// a fake file system that the Loader reads from. "initialDir" parameter -// must be an full, absolute directory (trailing slash doesn't matter). +// NewFakeLoader returns a Loader that uses a fake filesystem. +// The argument should be an absolute file path. 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) } diff --git a/pkg/loader/fileloader.go b/pkg/loader/fileloader.go index 3ab30d2e3..044049673 100644 --- a/pkg/loader/fileloader.go +++ b/pkg/loader/fileloader.go @@ -26,17 +26,67 @@ import ( "sigs.k8s.io/kustomize/pkg/ifc" ) -// fileLoader loads files from a file system. -// It has a notion of a current working directory, called 'root', -// that is independent from the current working directory of the -// process. When it loads a file from a relative path, the load -// is done relative to this root, not the process CWD. +const enforceRelocatable = false + +// fileLoader loads files, returning an array of bytes. +// It also enforces two kustomization requirements: +// +// 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. - // The last entry is the current root. + // Previously visited directories, tracked to + // 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 -// used in Load. +// 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] } 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) { - return l.childLoaderAt(filepath.Clean(root)) + if enforceRelocatable && filepath.IsAbs(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) -} - -// 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 { + if err := l.seenBefore(absRoot); 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() } diff --git a/pkg/loader/githubloader.go b/pkg/loader/githubloader.go index 1937d151d..c0e5a254e 100644 --- a/pkg/loader/githubloader.go +++ b/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 -type githubLoader struct { - repo string - // targetDir will hold the local repo - targetDir string - // checkoutDir is for the whole repository - checkoutDir string - loader *fileLoader +// gitCloner is a function that can clone a git repo. +type gitCloner func(url string) ( + // Directory where the repo is cloned to. + checkoutDir string, + // Relative path in the checkoutDir to location + // of kustomization file. + pathInCoDir string, + // Any error encountered when cloning. + err error) + +func makeTmpDir() (string, error) { + return ioutil.TempDir("", "kustomize-") } -// Root returns the root location for this Loader. -func (l *githubLoader) Root() string { - return l.targetDir -} - -// 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-") +func hashicorpGitCloner(repoUrl string) ( + checkoutDir string, pathInCoDir string, err error) { + dir, err := makeTmpDir() if err != nil { - return nil, err + return } - repodir := filepath.Join(dir, "repo") - src, subdir := getter.SourceDirSubdir(repoUrl) - err = checkout(src, repodir) - if err != nil { - return nil, err - } - target := filepath.Join(repodir, subdir) - l, _ := newFileLoaderAt(fs, target) - return &githubLoader{ - repo: repoUrl, - targetDir: target, - checkoutDir: repodir, - loader: l, - }, nil + checkoutDir = filepath.Join(dir, "repo") + url, pathInCoDir := getter.SourceDirSubdir(repoUrl) + err = checkout(url, checkoutDir) + return } // isRepoUrl checks if a string is a repo Url diff --git a/pkg/loader/githubloader_test.go b/pkg/loader/githubloader_test.go index 69b873b6f..1b1ac002a 100644 --- a/pkg/loader/githubloader_test.go +++ b/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()) + } +} diff --git a/pkg/loader/loader.go b/pkg/loader/loader.go index f3b910802..ba97a0040 100644 --- a/pkg/loader/loader.go +++ b/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) }