From 793577d044d6e335b8b497c058744ca27121489f Mon Sep 17 00:00:00 2001 From: jregan Date: Sun, 28 Oct 2018 12:32:33 -0700 Subject: [PATCH] Consult history in fileloader. Fixes #366 To reproduce #366, add ``` bases: - . ``` to `examples/helloWorld/kustomization.yaml`, attempt to build it, and enjoy the stack overflow. This PR fixes this by adding history to file loaders, allowing one to avoid cycles in overlay->base relationships. To make entry points clearer, this PR exposes only two public ways to make a fresh (no-history) loader * rooted at `/` * rooted at the process's current working directory. When making a new loader from an existing loader, retaining history along an overlay trace, the only allowed use is to go deeper into a file hierarchy, or go up and over to a never before visited sibling. This fix can probably be defeated by devious symbolic links. --- .../configmapfactory_test.go | 2 +- pkg/commands/build/build.go | 19 +-- pkg/commands/edit/add/configmap.go | 2 +- pkg/internal/loadertest/fakeloader.go | 12 +- pkg/loader/fileloader.go | 134 +++++++++++------- pkg/loader/fileloader_test.go | 53 ++++--- pkg/loader/githubloader.go | 2 +- pkg/loader/loader.go | 50 +------ pkg/resmap/factory_test.go | 4 +- pkg/transformers/config/factory_test.go | 2 +- 10 files changed, 140 insertions(+), 140 deletions(-) diff --git a/k8sdeps/configmapandsecret/configmapfactory_test.go b/k8sdeps/configmapandsecret/configmapfactory_test.go index 4d2214d7e4..161c31b657 100644 --- a/k8sdeps/configmapandsecret/configmapfactory_test.go +++ b/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 { diff --git a/pkg/commands/build/build.go b/pkg/commands/build/build.go index 11b9c97880..9c318482d6 100644 --- a/pkg/commands/build/build.go +++ b/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) } diff --git a/pkg/commands/edit/add/configmap.go b/pkg/commands/edit/add/configmap.go index 8bcec94381..6a5ace43a0 100644 --- a/pkg/commands/edit/add/configmap.go +++ b/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 diff --git a/pkg/internal/loadertest/fakeloader.go b/pkg/internal/loadertest/fakeloader.go index 66f2b5add0..1ac13917dd 100644 --- a/pkg/internal/loadertest/fakeloader.go +++ b/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. diff --git a/pkg/loader/fileloader.go b/pkg/loader/fileloader.go index 7a816348c3..3ab30d2e3d 100644 --- a/pkg/loader/fileloader.go +++ b/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) +func newLoaderOrDie(fSys fs.FileSystem, path string) *fileLoader { + l, err := newFileLoaderAt(fSys, path) + if err != nil { + log.Fatalf("unable to make loader at '%s'; %v", path, err) + } + return l } -// 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) +// 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 { - return false + return nil, fmt.Errorf( + "no absolute path for '%s' : %v", root, err) } - return filepath.IsAbs(fullFilePath) + if !fSys.IsDir(root) { + return nil, fmt.Errorf("absolute root '%s' must exist", root) + } + return &fileLoader{roots: []string{root}, fSys: fSys}, nil } -// 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") +// 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") } - // Special case current directory, expanding to full file path. - if location == currentDir { - currentDir, err := os.Getwd() - if err != nil { - return "", err - } - location = currentDir + if isRepoUrl(root) { + return newGithubLoader(root, l.fSys) } - // 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) + if filepath.IsAbs(root) { + return l.childLoaderAt(filepath.Clean(root)) } - return fullLocation, nil + // 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) } -// 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) - if err != nil { - fmt.Printf("Trouble in fulllocation: %v\n", err) +// 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 diff --git a/pkg/loader/fileloader_test.go b/pkg/loader/fileloader_test.go index 52fc78b378..63afbf5344 100644 --- a/pkg/loader/fileloader_test.go +++ b/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") diff --git a/pkg/loader/githubloader.go b/pkg/loader/githubloader.go index a062f34bae..1937d151d9 100644 --- a/pkg/loader/githubloader.go +++ b/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, diff --git a/pkg/loader/loader.go b/pkg/loader/loader.go index 8545374bb8..f3b9108027 100644 --- a/pkg/loader/loader.go +++ b/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) - } - - 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 +// NewLoader returns a Loader. +func NewLoader(root string, fSys fs.FileSystem) (ifc.Loader, error) { + if isRepoUrl(root) { + return newGithubLoader(root, fSys) } - 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) } diff --git a/pkg/resmap/factory_test.go b/pkg/resmap/factory_test.go index c35f046d3d..d8f3643cf2 100644 --- a/pkg/resmap/factory_test.go +++ b/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 { diff --git a/pkg/transformers/config/factory_test.go b/pkg/transformers/config/factory_test.go index 2274fcc1a1..e553157849 100644 --- a/pkg/transformers/config/factory_test.go +++ b/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{ {