From da4e8810079e2a98a3f4caad83618c53014d703b Mon Sep 17 00:00:00 2001 From: Anna Song Date: Sat, 1 Jul 2023 11:46:49 -0700 Subject: [PATCH] Add accumulateResources error tests for local files (#5225) * Add accumulateResources error tests for local files. Add tests demonstrating accumulateResources errors when the resource is a local file. Works to address #4807. * Improve readability --- api/krusty/accumulation_test.go | 143 +++++++++++++++++++++++-------- api/loader/fileloader_test.go | 52 ++++++++--- api/testutils/kusttest/ondisk.go | 22 +++-- 3 files changed, 164 insertions(+), 53 deletions(-) diff --git a/api/krusty/accumulation_test.go b/api/krusty/accumulation_test.go index e8c8b55523..00bb7aa3a9 100644 --- a/api/krusty/accumulation_test.go +++ b/api/krusty/accumulation_test.go @@ -17,6 +17,18 @@ import ( kusttest_test "sigs.k8s.io/kustomize/api/testutils/kusttest" ) +const validResource = ` +apiVersion: v1 +kind: Service +metadata: + name: myService +spec: + selector: + backend: bungie + ports: + - port: 7002 +` + func TestTargetMustHaveKustomizationFile(t *testing.T) { th := kusttest_test.MakeHarness(t) th.WriteF("service.yaml", ` @@ -63,17 +75,7 @@ func TestBaseMustHaveKustomizationFile(t *testing.T) { resources: - base `) - th.WriteF("base/service.yaml", ` -apiVersion: v1 -kind: Service -metadata: - name: myService -spec: - selector: - backend: bungie - ports: - - port: 7002 -`) + th.WriteF("base/service.yaml", validResource) err := th.RunWithErr(".", th.MakeDefaultOptions()) if err == nil { t.Fatalf("expected an error") @@ -167,12 +169,35 @@ spec: func TestAccumulateResourcesErrors(t *testing.T) { type testcase struct { - name string - resource string - // regex error message that is output when kustomize tries to - // accumulate resource as file and dir, respectively + name string + resource string + isAbsolute bool + files map[string]string + // errFile, errDir are regex for the expected error message output + // when kustomize tries to accumulate resource as file and dir, + // respectively. The test substitutes occurrences of "%s" in the + // error strings with the absolute path where kustomize looks for it. errFile, errDir string } + populateAbsolutePaths := func(tc testcase, dir string) testcase { + filePaths := make(map[string]string, len(tc.files)+1) + for file, content := range tc.files { + filePaths[filepath.Join(dir, file)] = content + } + resourcePath := filepath.Join(dir, tc.resource) + if tc.isAbsolute { + tc.resource = resourcePath + } + filePaths[filepath.Join(dir, "kustomization.yaml")] = fmt.Sprintf(` +resources: +- %s +`, tc.resource) + tc.files = filePaths + regPath := regexp.QuoteMeta(resourcePath) + tc.errFile = strings.ReplaceAll(tc.errFile, "%s", regPath) + tc.errDir = strings.ReplaceAll(tc.errDir, "%s", regPath) + return tc + } buildError := func(tc testcase) string { const ( prefix = "accumulating resources" @@ -196,39 +221,89 @@ func TestAccumulateResourcesErrors(t *testing.T) { for _, test := range []testcase{ { name: "remote file not considered repo", - // This url is too short to be considered a remote repo. - resource: "https://raw.githubusercontent.com/kustomize", - // It is acceptable that the error for a remote file-like reference - // (that is not long enough to be considered a repo) does not - // indicate said reference is not a local directory. - // Though it is possible for the remote file-like reference to be - // a local directory, it is very unlikely. - errFile: `HTTP Error: status code 400 \(Bad Request\)\z`, + // The example.com second-level domain is reserved and + // safe to access, see RFC 2606. + resource: "https://example.com/segments-too-few-to-be-repo", + // It's acceptable for the error output of a remote file-like + // resource to not indicate the resource's status as a + // local directory. Though it is possible for a remote file-like + // resource to be a local directory, it is very unlikely. + errFile: `HTTP Error: status code 404 \(Not Found\)\z`, }, { name: "remote file qualifies as repo", - resource: "https://raw.githubusercontent.com/kubernetes-sigs/kustomize/kustomize/v5.0.0/examples/helloWorld/configMap", + resource: "https://example.com/long/enough/to/have/org/and/repo", // TODO(4788): This error message is technically wrong. Just - // because we fail to GET a reference does not mean the reference is - // not a remote file. We should return the GET status code instead. + // because we fail to GET a resource does not mean the resource is + // not a remote file. We should return the GET status code as well. errFile: "URL is a git repository", errDir: `failed to run \S+/git fetch --depth=1 .+`, }, + { + name: "local file qualifies as repo", + // The .example top level domain is reserved for example purposes, + // see RFC 2606. + resource: "package@v1.28.0.example/configs/base", + errFile: `evalsymlink failure on '%s' .+`, + errDir: `failed to run \S+/git fetch --depth=1 .+`, + }, + { + name: "relative path does not exist", + resource: "file-or-directory", + errFile: `evalsymlink failure on '%s' .+`, + errDir: `must build at directory: not a valid directory: evalsymlink failure .+`, + }, + { + name: "absolute path does not exist", + resource: "file-or-directory", + isAbsolute: true, + errFile: `evalsymlink failure on '%s' .+`, + errDir: `new root '%s' cannot be absolute`, + }, + { + name: "relative file violates restrictions", + resource: "../base/resource.yaml", + files: map[string]string{ + "../base/resource.yaml": validResource, + }, + errFile: "security; file '%s' is not in or below .+", + // TODO(4348): Over-inclusion of directory error message when we + // know resource is file. + errDir: "must build at directory: '%s': file is not directory", + }, + { + name: "absolute file violates restrictions", + resource: "../base/resource.yaml", + isAbsolute: true, + files: map[string]string{ + "../base/resource.yaml": validResource, + }, + errFile: "security; file '%s' is not in or below .+", + // TODO(4348): Over-inclusion of directory error message when we + // know resource is file. + errDir: `new root '%s' cannot be absolute`, + }, } { t.Run(test.name, func(t *testing.T) { - // should use real file system to indicate that we are creating - // new temporary directories on disk when we attempt to fetch repos - fSys, tmpDir := kusttest_test.CreateKustDir(t, fmt.Sprintf(` -resources: -- %s -`, test.resource)) + // Should use real file system to indicate that we are creating + // new temporary directories on disk when we attempt to fetch repos. + fs, tmpDir := kusttest_test.Setup(t) + root := tmpDir.Join("root") + require.NoError(t, fs.Mkdir(root)) + + test = populateAbsolutePaths(test, root) + for file, content := range test.files { + dir := filepath.Dir(file) + require.NoError(t, fs.MkdirAll(dir)) + require.NoError(t, fs.WriteFile(file, []byte(content))) + } + b := krusty.MakeKustomizer(krusty.MakeDefaultOptions()) - _, err := b.Run(fSys, tmpDir.String()) + _, err := b.Run(fs, root) require.Regexp(t, buildError(test), err.Error()) }) } // TODO(annasong): add tests that check accumulateResources errors for - // - local files // - repos // - local directories // - files that yield malformed yaml errors diff --git a/api/loader/fileloader_test.go b/api/loader/fileloader_test.go index df5729cfef..44572f4be7 100644 --- a/api/loader/fileloader_test.go +++ b/api/loader/fileloader_test.go @@ -31,6 +31,11 @@ func TestIsRemoteFile(t *testing.T) { "https://raw.githubusercontent.com/kubernetes-sigs/kustomize/master/examples/helloWorld/configMap.yaml", true, }, + "malformed https": { + // TODO(annasong): Maybe we want to fix this. Needs more research. + "https:/raw.githubusercontent.com/kubernetes-sigs/kustomize/master/examples/helloWorld/configMap.yaml", + true, + }, "https dir": { "https://github.com/kubernetes-sigs/kustomize//examples/helloWorld/", true, @@ -201,23 +206,44 @@ func TestNewEmptyLoader(t *testing.T) { require.Error(t, err) } -func TestNewLoaderHTTP(t *testing.T) { - t.Run("doesn't exist", func(t *testing.T) { - _, err := makeLoader().New("https://google.com/project") - require.Error(t, err) +func TestNewRemoteLoaderDoesNotExist(t *testing.T) { + _, err := makeLoader().New("https://example.com/org/repo") + require.ErrorContains(t, err, "fetch") +} + +func TestLoaderLocalScheme(t *testing.T) { + // It is unlikely but possible for a reference with a url scheme to + // actually refer to a local file or directory. + t.Run("file", func(t *testing.T) { + fSys, dir := setupOnDisk(t) + parts := []string{ + "ssh:", + "resource.yaml", + } + require.NoError(t, fSys.Mkdir(dir.Join(parts[0]))) + const content = "resource config" + require.NoError(t, fSys.WriteFile( + dir.Join(filepath.Join(parts...)), + []byte(content), + )) + actualContent, err := newLoaderOrDie(RestrictionRootOnly, + fSys, + dir.String(), + ).Load(strings.Join(parts, "//")) + require.NoError(t, err) + require.Equal(t, content, string(actualContent)) }) - // Though it is unlikely and we do not weigh this use case in our designs, - // it is possible for a reference that is considered a remote file to - // actually be a directory - t.Run("exists", func(t *testing.T) { - const remoteFileLikeRoot = "https://domain" - require.True(t, IsRemoteFile(remoteFileLikeRoot)) + t.Run("directory", func(t *testing.T) { fSys, dir := setupOnDisk(t) - require.NoError(t, fSys.MkdirAll(dir.Join("https:/domain"))) + parts := []string{ + "https:", + "root", + } + require.NoError(t, fSys.MkdirAll(dir.Join(filepath.Join(parts...)))) ldr, err := newLoaderOrDie(RestrictionRootOnly, fSys, dir.String(), - ).New(remoteFileLikeRoot) + ).New(strings.Join(parts, "//")) require.NoError(t, err) require.Empty(t, ldr.Repo()) }) @@ -635,6 +661,8 @@ func TestLoaderHTTP(t *testing.T) { // setupOnDisk sets up a file system on disk and directory that is cleaned after // test completion. +// TODO(annasong): Move all loader tests that require real file system into +// api/krusty. func setupOnDisk(t *testing.T) (filesys.FileSystem, filesys.ConfirmedDir) { t.Helper() diff --git a/api/testutils/kusttest/ondisk.go b/api/testutils/kusttest/ondisk.go index f2bc2e8165..9a02299a6d 100644 --- a/api/testutils/kusttest/ondisk.go +++ b/api/testutils/kusttest/ondisk.go @@ -11,19 +11,27 @@ import ( "sigs.k8s.io/kustomize/kyaml/filesys" ) -// CreateKustDir creates a file system on disk and a new directory -// that holds a kustomization file with content. The directory is removed on +// Setup sets up a file system on disk and directory that is cleaned after // test completion. -func CreateKustDir(t *testing.T, content string) (filesys.FileSystem, filesys.ConfirmedDir) { +func Setup(t *testing.T) (filesys.FileSystem, filesys.ConfirmedDir) { t.Helper() fSys := filesys.MakeFsOnDisk() - tmpDir, err := filesys.NewTmpConfirmedDir() + dir, err := filesys.NewTmpConfirmedDir() require.NoError(t, err) - require.NoError(t, fSys.WriteFile(filepath.Join(tmpDir.String(), "kustomization.yaml"), []byte(content))) - t.Cleanup(func() { - require.NoError(t, fSys.RemoveAll(tmpDir.String())) + _ = fSys.RemoveAll(dir.String()) }) + return fSys, dir +} + +// CreateKustDir creates a file system on disk and a new directory +// that holds a kustomization file with content. The directory is removed on +// test completion. +func CreateKustDir(t *testing.T, content string) (filesys.FileSystem, filesys.ConfirmedDir) { + t.Helper() + + fSys, tmpDir := Setup(t) + require.NoError(t, fSys.WriteFile(filepath.Join(tmpDir.String(), "kustomization.yaml"), []byte(content))) return fSys, tmpDir }