Merge pull request #5177 from annasong20/test-accumulate-remote-file

Test accumulateResources errors for remote files

api/krusty/accumulation_test.go

@@ -4,12 +4,16 @@
 package krusty_test
 
 import (
+	"fmt"
 	"path/filepath"
+	"regexp"
 	"strings"
 	"testing"
 
+	"github.com/stretchr/testify/require"
 	. "sigs.k8s.io/kustomize/api/internal/target"
 	"sigs.k8s.io/kustomize/api/konfig"
+	"sigs.k8s.io/kustomize/api/krusty"
 	kusttest_test "sigs.k8s.io/kustomize/api/testutils/kusttest"
 )
 
@@ -160,3 +164,72 @@ spec:
     secretName: cert-tls
 `)
 }
+
+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
+		errFile, errDir string
+	}
+	buildError := func(tc testcase) string {
+		const (
+			prefix            = "accumulating resources"
+			filePrefixf       = "accumulating resources from '%s'"
+			fileWrapperIfDirf = "accumulation err='%s'"
+			separator         = ": "
+		)
+		parts := []string{
+			prefix,
+			strings.Join([]string{
+				fmt.Sprintf(filePrefixf, regexp.QuoteMeta(tc.resource)),
+				tc.errFile,
+			}, separator),
+		}
+		if tc.errDir != "" {
+			parts[1] = fmt.Sprintf(fileWrapperIfDirf, parts[1])
+			parts = append(parts, tc.errDir)
+		}
+		return strings.Join(parts, separator)
+	}
+	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`,
+		},
+		{
+			name:     "remote file qualifies as repo",
+			resource: "https://raw.githubusercontent.com/kubernetes-sigs/kustomize/kustomize/v5.0.0/examples/helloWorld/configMap",
+			// 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.
+			errFile: "URL is a git repository",
+			errDir:  `failed to run \S+/git fetch --depth=1 .+`,
+		},
+	} {
+		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))
+			b := krusty.MakeKustomizer(krusty.MakeDefaultOptions())
+			_, err := b.Run(fSys, tmpDir.String())
+			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
+}

api/krusty/remoteloader_test.go

@@ -10,7 +10,6 @@ import (
 	"io"
 	"os"
 	"os/exec"
-	"path/filepath"
 	"strings"
 	"testing"
 
@@ -19,7 +18,7 @@ import (
 	"sigs.k8s.io/kustomize/api/krusty"
 	"sigs.k8s.io/kustomize/api/loader"
 	"sigs.k8s.io/kustomize/api/resmap"
-	"sigs.k8s.io/kustomize/kyaml/filesys"
+	kusttest_test "sigs.k8s.io/kustomize/api/testutils/kusttest"
 	"sigs.k8s.io/kustomize/kyaml/yaml"
 )
 
@@ -264,7 +263,7 @@ resources:
 			}
 
 			kust := strings.ReplaceAll(test.kustomization, "$ROOT", repos.root)
-			fSys, tmpDir := createKustDir(t, kust)
+			fSys, tmpDir := kusttest_test.CreateKustDir(t, kust)
 
 			b := krusty.MakeKustomizer(krusty.MakeDefaultOptions())
 			m, err := b.Run(
@@ -368,7 +367,7 @@ resources:
 			if test.beforeTest != nil {
 				test.beforeTest(t)
 			}
-			fSys, tmpDir := createKustDir(t, test.kustomization)
+			fSys, tmpDir := kusttest_test.CreateKustDir(t, test.kustomization)
 
 			b := krusty.MakeKustomizer(krusty.MakeDefaultOptions())
 			m, err := b.Run(
@@ -424,16 +423,6 @@ func configureGitSSHCommand(t *testing.T) {
 	})
 }
 
-func createKustDir(t *testing.T, content string) (filesys.FileSystem, filesys.ConfirmedDir) {
-	t.Helper()
-
-	fSys := filesys.MakeFsOnDisk()
-	tmpDir, err := filesys.NewTmpConfirmedDir()
-	require.NoError(t, err)
-	require.NoError(t, fSys.WriteFile(filepath.Join(tmpDir.String(), "kustomization.yaml"), []byte(content)))
-	return fSys, tmpDir
-}
-
 func checkYaml(t *testing.T, actual resmap.ResMap, expected string) {
 	t.Helper()
 

api/loader/fileloader_test.go

@@ -196,13 +196,31 @@ func TestLoaderBadRelative(t *testing.T) {
 	require.Error(err)
 }
 
-func TestLoaderMisc(t *testing.T) {
-	l := makeLoader()
-	_, err := l.New("")
+func TestNewEmptyLoader(t *testing.T) {
+	_, err := makeLoader().New("")
 	require.Error(t, err)
+}
 
-	_, err = l.New("https://google.com/project")
-	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)
+	})
+	// 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))
+		fSys, dir := setupOnDisk(t)
+		require.NoError(t, fSys.MkdirAll(dir.Join("https:/domain")))
+		ldr, err := newLoaderOrDie(RestrictionRootOnly,
+			fSys,
+			dir.String(),
+		).New(remoteFileLikeRoot)
+		require.NoError(t, err)
+		require.Empty(t, ldr.Repo())
+	})
 }
 
 const (
@@ -212,17 +230,17 @@ const (
 
 // Create a structure like this
 //
-//   /tmp/kustomize-test-random
-//   ├── base
-//   │   ├── okayData
-//   │   ├── symLinkToOkayData -> okayData
-//   │   └── symLinkToExteriorData -> ../exteriorData
-//   └── exteriorData
-//
+//	/tmp/kustomize-test-random
+//	├── base
+//	│   ├── okayData
+//	│   ├── symLinkToOkayData -> okayData
+//	│   └── symLinkToExteriorData -> ../exteriorData
+//	└── exteriorData
 func commonSetupForLoaderRestrictionTest(t *testing.T) (string, filesys.FileSystem) {
 	t.Helper()
-	dir := t.TempDir()
-	fSys := filesys.MakeFsOnDisk()
+	fSys, tmpDir := setupOnDisk(t)
+	dir := tmpDir.String()
+
 	fSys.Mkdir(filepath.Join(dir, "base"))
 
 	fSys.WriteFile(
@@ -446,12 +464,8 @@ func TestLoaderDisallowsLocalBaseFromRemoteOverlay(t *testing.T) {
 }
 
 func TestLoaderDisallowsRemoteBaseExitRepo(t *testing.T) {
-	fSys := filesys.MakeFsOnDisk()
-	dir, err := filesys.NewTmpConfirmedDir()
-	require.NoError(t, err)
-	t.Cleanup(func() {
-		_ = fSys.RemoveAll(dir.String())
-	})
+	fSys, dir := setupOnDisk(t)
+
 	repo := dir.Join("repo")
 	require.NoError(t, fSys.Mkdir(repo))
 
@@ -618,3 +632,17 @@ func TestLoaderHTTP(t *testing.T) {
 		require.Error(err)
 	}
 }
+
+// setupOnDisk sets up a file system on disk and directory that is cleaned after
+// test completion.
+func setupOnDisk(t *testing.T) (filesys.FileSystem, filesys.ConfirmedDir) {
+	t.Helper()
+
+	fSys := filesys.MakeFsOnDisk()
+	dir, err := filesys.NewTmpConfirmedDir()
+	require.NoError(t, err)
+	t.Cleanup(func() {
+		_ = fSys.RemoveAll(dir.String())
+	})
+	return fSys, dir
+}

api/testutils/kusttest/ondisk.go

@@ -0,0 +1,29 @@
+// Copyright 2023 The Kubernetes Authors.
+// SPDX-License-Identifier: Apache-2.0
+
+package kusttest_test
+
+import (
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"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
+// test completion.
+func CreateKustDir(t *testing.T, content string) (filesys.FileSystem, filesys.ConfirmedDir) {
+	t.Helper()
+
+	fSys := filesys.MakeFsOnDisk()
+	tmpDir, 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()))
+	})
+	return fSys, tmpDir
+}