Merge pull request #520 from monopole/loadingWithHistory

Consult history in fileloader.

k8sdeps/configmapandsecret/configmapfactory_test.go

@@ -131,7 +131,7 @@ func TestConstructConfigMap(t *testing.T) {
 	fSys := fs.MakeFakeFS()
 	fSys.WriteFile("/configmap/app.env", []byte("DB_USERNAME=admin\nDB_PASSWORD=somepw\n"))
 	fSys.WriteFile("/configmap/app-init.ini", []byte("FOO=bar\nBAR=baz\n"))
-	f := NewConfigMapFactory(fSys, loader.NewFileLoader(fSys))
+	f := NewConfigMapFactory(fSys, loader.NewFileLoaderAtRoot(fSys))
 	for _, tc := range testCases {
 		cm, err := f.MakeConfigMap(&tc.input, tc.options)
 		if err != nil {

pkg/commands/build/build.go

@@ -120,9 +120,8 @@ func (o *buildOptions) Validate(args []string, p string, fs fs.FileSystem) error
 // RunBuild runs build command.
 func (o *buildOptions) RunBuild(
 	out io.Writer, fSys fs.FileSystem,
-	rf *resmap.Factory,
-	ptf transformer.Factory) error {
-	rootLoader, err := loader.NewLoader(o.kustomizationPath, "", fSys)
+	rf *resmap.Factory, ptf transformer.Factory) error {
+	ldr, err := loader.NewLoader(o.kustomizationPath, fSys)
 	if err != nil {
 		return err
 	}
@@ -130,11 +129,8 @@ func (o *buildOptions) RunBuild(
 	if err != nil {
 		return err
 	}
-	defer rootLoader.Cleanup()
-	kt, err := target.NewKustTarget(
-		rootLoader, fSys,
-		rf,
-		ptf, tc)
+	defer ldr.Cleanup()
+	kt, err := target.NewKustTarget(ldr, fSys, rf, ptf, tc)
 	if err != nil {
 		return err
 	}
@@ -161,10 +157,5 @@ func makeTransformerconfig(
 	if paths == nil || len(paths) == 0 {
 		return config.NewFactory(nil).DefaultConfig(), nil
 	}
-	ldr, err := loader.NewLoader(".", "", fSys)
-	if err != nil {
-		return nil, errors.Wrap(
-			err, "cannot create transformer configuration loader")
-	}
-	return config.NewFactory(ldr).FromFiles(paths)
+	return config.NewFactory(loader.NewFileLoaderAtCwd(fSys)).FromFiles(paths)
 }

pkg/commands/edit/add/configmap.go

@@ -67,7 +67,7 @@ func newCmdAddConfigMap(fSys fs.FileSystem, kf ifc.KunstructuredFactory) *cobra.
 			}
 
 			// Add the flagsAndArgs map to the kustomization file.
-			kf.Set(fSys, loader.NewFileLoader(fSys))
+			kf.Set(fSys, loader.NewFileLoaderAtCwd(fSys))
 			err = addConfigMap(kustomization, flagsAndArgs, kf)
 			if err != nil {
 				return err

pkg/internal/loadertest/fakeloader.go

@@ -18,6 +18,7 @@ limitations under the License.
 package loadertest
 
 import (
+	"log"
 	"sigs.k8s.io/kustomize/pkg/fs"
 	"sigs.k8s.io/kustomize/pkg/ifc"
 	"sigs.k8s.io/kustomize/pkg/loader"
@@ -34,10 +35,13 @@ type FakeLoader struct {
 // 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.
-	fakefs := fs.MakeFakeFS()
-	rootLoader := loader.NewFileLoader(fakefs)
-	ldr, _ := rootLoader.New(initialDir)
-	return FakeLoader{fs: fakefs, delegate: ldr}
+	fSys := fs.MakeFakeFS()
+	fSys.Mkdir(initialDir)
+	ldr, err := loader.NewFileLoaderAtRoot(fSys).New(initialDir)
+	if err != nil {
+		log.Fatalf("Unable to make loader: %v", err)
+	}
+	return FakeLoader{fs: fSys, delegate: ldr}
 }
 
 // AddFile adds a fake file to the file system.

pkg/loader/fileloader.go

@@ -18,90 +18,118 @@ package loader
 
 import (
 	"fmt"
-	"os"
+	"log"
 	"path/filepath"
+	"strings"
 
 	"sigs.k8s.io/kustomize/pkg/fs"
 	"sigs.k8s.io/kustomize/pkg/ifc"
 )
 
-const currentDir = "."
-
 // 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.
 type fileLoader struct {
-	root string
+	// Previously visited directories, tracked to avoid cycles.
+	// The last entry is the current root.
+	roots []string
+	// File system utilities.
 	fSys fs.FileSystem
 }
 
-// NewFileLoader returns a new fileLoader.
-func NewFileLoader(fSys fs.FileSystem) *fileLoader {
-	return newFileLoaderAtRoot("", fSys)
+// NewFileLoaderAtCwd returns a loader that loads from ".".
+func NewFileLoaderAtCwd(fSys fs.FileSystem) *fileLoader {
+	return newLoaderOrDie(fSys, ".")
 }
 
-// newFileLoaderAtRoot returns a new fileLoader with given root.
-func newFileLoaderAtRoot(root string, fs fs.FileSystem) *fileLoader {
-	return &fileLoader{root: root, fSys: fs}
+// NewFileLoaderAtRoot returns a loader that loads from "/".
+func NewFileLoaderAtRoot(fSys fs.FileSystem) *fileLoader {
+	return newLoaderOrDie(fSys, "/")
 }
 
-// Root returns the root location for this Loader.
+// Root returns the absolute path that is prepended to any relative paths
+// used in Load.
 func (l *fileLoader) Root() string {
-	return l.root
+	return l.roots[len(l.roots)-1]
 }
 
-// Returns a new Loader rooted at newRoot. "newRoot" MUST be
-// a directory (not a file). The directory can have a trailing
-// slash or not.
-// Example: "/home/seans/project" or "/home/seans/project/"
-// NOT "/home/seans/project/file.yaml".
-func (l *fileLoader) New(newRoot string) (ifc.Loader, error) {
-	return NewLoader(newRoot, l.root, l.fSys)
-}
-
-// IsAbsPath return true if the location calculated with the root
-// and location params a full file path.
-func (l *fileLoader) IsAbsPath(root string, location string) bool {
-	fullFilePath, err := l.fullLocation(root, location)
+func newLoaderOrDie(fSys fs.FileSystem, path string) *fileLoader {
+	l, err := newFileLoaderAt(fSys, path)
 	if err != nil {
-		return false
+		log.Fatalf("unable to make loader at '%s'; %v", path, err)
 	}
-	return filepath.IsAbs(fullFilePath)
+	return l
 }
 
-// fullLocation returns some notion of a full path.
-// If location is a full file path, then ignore root. If location is relative, then
-// join the root path with the location path. Either root or location can be empty,
-// but not both. Special case for ".": Expands to current working directory.
-// Example: "/home/seans/project", "subdir/bar" -> "/home/seans/project/subdir/bar".
-func (l *fileLoader) fullLocation(root string, location string) (string, error) {
-	// First, validate the parameters
-	if len(root) == 0 && len(location) == 0 {
-		return "", fmt.Errorf("unable to calculate full location: root and location empty")
-	}
-	// Special case current directory, expanding to full file path.
-	if location == currentDir {
-		currentDir, err := os.Getwd()
-		if err != nil {
-			return "", err
-		}
-		location = currentDir
-	}
-	// Assume the location is a full file path. If not, then join root with location.
-	fullLocation := location
-	if !filepath.IsAbs(location) {
-		fullLocation = filepath.Join(root, location)
-	}
-	return fullLocation, nil
-}
-
-// Load returns the bytes from reading a file at fullFilePath.
-// Implements the Loader interface.
-func (l *fileLoader) Load(location string) ([]byte, error) {
-	fullLocation, err := l.fullLocation(l.root, location)
+// newFileLoaderAt returns a new fileLoader with given root.
+func newFileLoaderAt(fSys fs.FileSystem, root string) (*fileLoader, error) {
+	root, err := filepath.Abs(root)
 	if err != nil {
-		fmt.Printf("Trouble in fulllocation: %v\n", err)
+		return nil, fmt.Errorf(
+			"no absolute path for '%s' : %v", root, err)
+	}
+	if !fSys.IsDir(root) {
+		return nil, fmt.Errorf("absolute root '%s' must exist", root)
+	}
+	return &fileLoader{roots: []string{root}, fSys: fSys}, nil
+}
+
+// Returns a new Loader, which might be rooted relative to current loader.
+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 filepath.IsAbs(root) {
+		return l.childLoaderAt(filepath.Clean(root))
+	}
+	// Get absolute path to squeeze out "..", ".", etc. to check for cycles.
+	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)
+	}
+	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 {
 		return nil, err
 	}
-	return l.fSys.ReadFile(fullLocation)
+	return &fileLoader{roots: append(l.roots, root), fSys: l.fSys}, 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) {
+			return fmt.Errorf(
+				"cycle detected: new root '%s' contains previous root '%s'",
+				path, r)
+		}
+	}
+	return nil
+}
+
+// Load returns content of file at the given relative path.
+func (l *fileLoader) Load(path string) ([]byte, error) {
+	if !filepath.IsAbs(path) {
+		path = filepath.Join(l.Root(), path)
+	}
+	return l.fSys.ReadFile(path)
 }
 
 // Cleanup does nothing

pkg/loader/fileloader_test.go

@@ -57,8 +57,8 @@ func MakeFakeFs(td []testData) fs.FileSystem {
 }
 
 func TestLoaderLoad(t *testing.T) {
-	l1 := NewFileLoader(MakeFakeFs(testCases))
-	if "" != l1.Root() {
+	l1 := NewFileLoaderAtRoot(MakeFakeFs(testCases))
+	if "/" != l1.Root() {
 		t.Fatalf("incorrect root: '%s'\n", l1.Root())
 	}
 	for _, x := range testCases {
@@ -96,7 +96,7 @@ func TestLoaderLoad(t *testing.T) {
 }
 
 func TestLoaderNewSubDir(t *testing.T) {
-	l1, err := NewFileLoader(MakeFakeFs(testCases)).New("/foo/project")
+	l1, err := NewFileLoaderAtRoot(MakeFakeFs(testCases)).New("/foo/project")
 	if err != nil {
 		t.Fatalf("unexpected err: %v\n", err)
 	}
@@ -118,7 +118,7 @@ func TestLoaderNewSubDir(t *testing.T) {
 }
 
 func TestLoaderBadRelative(t *testing.T) {
-	l1, err := NewFileLoader(MakeFakeFs(testCases)).New("/foo/project/subdir1")
+	l1, err := NewFileLoaderAtRoot(MakeFakeFs(testCases)).New("/foo/project/subdir1")
 	if err != nil {
 		t.Fatalf("unexpected err: %v\n", err)
 	}
@@ -128,30 +128,38 @@ func TestLoaderBadRelative(t *testing.T) {
 
 	// Cannot cd into a file.
 	l2, err := l1.New("fileB.yaml")
-	// TODO: this should be an error.
-	if err != nil {
-		t.Fatalf("unexpected err %v", err)
+	if err == nil {
+		t.Fatalf("expected err, but got root %s", l2.Root())
 	}
 
-	// It's not okay to stay put.
+	// It's not okay to stay at the same place.
 	l2, err = l1.New(".")
-	// TODO: this should be an error.
-	if err != nil {
-		t.Fatalf("unexpected err %v", err)
+	if err == nil {
+		t.Fatalf("expected err, but got root %s", l2.Root())
 	}
 
 	// It's not okay to go up and back down into same place.
 	l2, err = l1.New("../subdir1")
-	// TODO: this should be an error.
-	if err != nil {
-		t.Fatalf("unexpected err %v", err)
+	if err == nil {
+		t.Fatalf("expected err, but got root %s", l2.Root())
 	}
 
-	// It's not okay to go up.
+	// It's not okay to go up via a relative path.
 	l2, err = l1.New("..")
-	// TODO: this should be an error.
-	if err != nil {
-		t.Fatalf("unexpected err %v", err)
+	if err == nil {
+		t.Fatalf("expected err, but got root %s", l2.Root())
+	}
+
+	// It's not okay to go up via an absolute path.
+	l2, err = l1.New("/foo/project")
+	if err == nil {
+		t.Fatalf("expected err, but got root %s", l2.Root())
+	}
+
+	// It's not okay to go to the root.
+	l2, err = l1.New("/")
+	if err == nil {
+		t.Fatalf("expected err, but got root %s", l2.Root())
 	}
 
 	// It's okay to go up and down to a sibling.
@@ -170,10 +178,17 @@ func TestLoaderBadRelative(t *testing.T) {
 	if !reflect.DeepEqual([]byte(x.expectedContent), b) {
 		t.Fatalf("in load expected %s, but got %s", x.expectedContent, b)
 	}
+
+	// It's not OK to go over to a previously visited directory.
+	// Must disallow going back and forth in a cycle.
+	l1, err = l2.New("../subdir1")
+	if err == nil {
+		t.Fatalf("expected err, but got root %s", l1.Root())
+	}
 }
 
 func TestLoaderMisc(t *testing.T) {
-	l := NewFileLoader(MakeFakeFs(testCases))
+	l := NewFileLoaderAtRoot(MakeFakeFs(testCases))
 	_, err := l.New("")
 	if err == nil {
 		t.Fatalf("Expected error for empty root location not returned")

pkg/loader/githubloader.go

@@ -71,7 +71,7 @@ func newGithubLoader(repoUrl string, fs fs.FileSystem) (*githubLoader, error) {
 		return nil, err
 	}
 	target := filepath.Join(repodir, subdir)
-	l := newFileLoaderAtRoot(target, fs)
+	l, _ := newFileLoaderAt(fs, target)
 	return &githubLoader{
 		repo:        repoUrl,
 		targetDir:   target,

pkg/loader/loader.go

@@ -18,52 +18,14 @@ limitations under the License.
 package loader
 
 import (
-	"fmt"
-	"path/filepath"
-	"sigs.k8s.io/kustomize/pkg/ifc"
-
 	"sigs.k8s.io/kustomize/pkg/fs"
+	"sigs.k8s.io/kustomize/pkg/ifc"
 )
 
-// NewLoader returns a Loader given a target
-// The target can be a local disk directory or a github Url
-func NewLoader(target, r string, fSys fs.FileSystem) (ifc.Loader, error) {
-	if !isValidLoaderPath(target, r) {
-		return nil, fmt.Errorf("Not valid path: root='%s', loc='%s'\n", r, target)
+// NewLoader returns a Loader.
+func NewLoader(root string, fSys fs.FileSystem) (ifc.Loader, error) {
+	if isRepoUrl(root) {
+		return newGithubLoader(root, fSys)
 	}
-
-	if !isLocalTarget(target, fSys) && isRepoUrl(target) {
-		return newGithubLoader(target, fSys)
-	}
-
-	l := newFileLoaderAtRoot(r, fSys)
-	if isRootLoaderPath(r) {
-		absPath, err := filepath.Abs(target)
-		if err != nil {
-			return nil, err
-		}
-		target = absPath
-	}
-
-	if !l.IsAbsPath(l.root, target) {
-		return nil, fmt.Errorf("Not abs path: l.root='%s', loc='%s'\n", l.root, target)
-	}
-	root, err := l.fullLocation(l.root, target)
-	if err != nil {
-		return nil, err
-	}
-	return newFileLoaderAtRoot(root, l.fSys), nil
-}
-
-func isValidLoaderPath(target, root string) bool {
-	return target != "" || root != ""
-}
-
-func isRootLoaderPath(root string) bool {
-	return root == ""
-}
-
-// isLocalTarget checks if a file exists in the filesystem
-func isLocalTarget(s string, fs fs.FileSystem) bool {
-	return fs.Exists(s)
+	return newFileLoaderAt(fSys, root)
 }

pkg/resmap/factory_test.go

@@ -270,7 +270,7 @@ func TestNewResMapFromSecretArgs(t *testing.T) {
 	}
 	fakeFs := fs.MakeFakeFS()
 	fakeFs.Mkdir(".")
-	rmF.Set(fakeFs, loader.NewFileLoader(fakeFs))
+	rmF.Set(fakeFs, loader.NewFileLoaderAtRoot(fakeFs))
 	actual, err := rmF.NewResMapFromSecretArgs(secrets, nil)
 
 	if err != nil {
@@ -326,7 +326,7 @@ func TestSecretTimeout(t *testing.T) {
 	}
 	fakeFs := fs.MakeFakeFS()
 	fakeFs.Mkdir(".")
-	rmF.Set(fakeFs, loader.NewFileLoader(fakeFs))
+	rmF.Set(fakeFs, loader.NewFileLoaderAtRoot(fakeFs))
 	_, err := rmF.NewResMapFromSecretArgs(secrets, nil)
 
 	if err == nil {

pkg/transformers/config/factory_test.go

@@ -39,7 +39,7 @@ namePrefix:
 `
 	fakeFS := fs.MakeFakeFS()
 	fakeFS.WriteFile("/transformerconfig/test/config.yaml", []byte(transformerConfig))
-	ldr := loader.NewFileLoader(fakeFS)
+	ldr := loader.NewFileLoaderAtRoot(fakeFS)
 	expected := &TransformerConfig{
 		NamePrefix: []FieldSpec{
 			{