refactor: cli test loading (#8244)

Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>

cmd/cli/kubectl-kyverno/fix/test/command.go

@@ -5,6 +5,7 @@ import (
 	"os"
 	"path/filepath"
 
+	testutils "github.com/kyverno/kyverno/cmd/cli/kubectl-kyverno/utils/test"
 	"github.com/spf13/cobra"
 	"sigs.k8s.io/yaml"
 )
@@ -17,27 +18,27 @@ func Command() *cobra.Command {
 		Short:   "Fix inconsistencies and deprecated usage in Kyverno test files.",
 		Example: "",
 		RunE: func(cmd *cobra.Command, args []string) error {
-			var testCases []testCase
+			var testCases []testutils.TestCase
 			for _, arg := range args {
-				tests, err := loadTests(arg, fileName)
+				tests, err := testutils.LoadTests(arg, fileName)
 				if err != nil {
 					return err
 				}
 				testCases = append(testCases, tests...)
 			}
 			for _, testCase := range testCases {
-				fmt.Printf("Processing test file (%s)...", testCase.path)
+				fmt.Printf("Processing test file (%s)...", testCase.Path)
 				fmt.Println()
-				if testCase.err != nil {
-					fmt.Printf("  ERROR: loading test file (%s): %s", testCase.path, testCase.err)
+				if testCase.Err != nil {
+					fmt.Printf("  ERROR: loading test file (%s): %s", testCase.Path, testCase.Err)
 					fmt.Println()
 					continue
 				}
-				test := testCase.test
+				test := testCase.Test
 				needsSave := false
 				if test.Name == "" {
 					fmt.Println("  WARNING: name is not set")
-					test.Name = filepath.Base(testCase.path)
+					test.Name = filepath.Base(testCase.Path)
 					needsSave = true
 				}
 				if len(test.Policies) == 0 {
@@ -68,7 +69,7 @@ func Command() *cobra.Command {
 					}
 				}
 				if save && needsSave {
-					fmt.Printf("  Saving test file (%s)...", testCase.path)
+					fmt.Printf("  Saving test file (%s)...", testCase.Path)
 					fmt.Println()
 					yamlBytes, err := yaml.Marshal(test)
 					if err != nil {
@@ -76,8 +77,8 @@ func Command() *cobra.Command {
 						fmt.Println()
 						continue
 					}
-					if err := os.WriteFile(testCase.path, yamlBytes, os.ModePerm); err != nil {
-						fmt.Printf("    ERROR: saving test file (%s): %s", testCase.path, err)
+					if err := os.WriteFile(testCase.Path, yamlBytes, os.ModePerm); err != nil {
+						fmt.Printf("    ERROR: saving test file (%s): %s", testCase.Path, err)
 						fmt.Println()
 						continue
 					}

cmd/cli/kubectl-kyverno/fix/test/load.go

@@ -1,61 +0,0 @@
-package test
-
-import (
-	"os"
-	"path/filepath"
-
-	"github.com/kyverno/kyverno/cmd/cli/kubectl-kyverno/test/api"
-	"k8s.io/apimachinery/pkg/util/yaml"
-)
-
-type testCase struct {
-	path string
-	test *api.Test
-	err  error
-}
-
-func loadTests(dirPath string, fileName string) ([]testCase, error) {
-	return loadLocalTest(filepath.Clean(dirPath), fileName)
-}
-
-func loadLocalTest(path string, fileName string) ([]testCase, error) {
-	var tests []testCase
-	files, err := os.ReadDir(path)
-	if err != nil {
-		return nil, err
-	}
-	for _, file := range files {
-		if file.IsDir() {
-			ps, err := loadLocalTest(filepath.Join(path, file.Name()), fileName)
-			if err != nil {
-				return nil, err
-			}
-			tests = append(tests, ps...)
-		} else if file.Name() == fileName {
-			tests = append(tests, loadTest(path, file.Name()))
-		}
-	}
-	return tests, nil
-}
-
-func loadTest(dirPath string, fileName string) testCase {
-	path := filepath.Join(dirPath, fileName)
-	yamlBytes, err := os.ReadFile(path) // #nosec G304
-	if err != nil {
-		return testCase{
-			path: path,
-			err:  err,
-		}
-	}
-	var test api.Test
-	if err := yaml.UnmarshalStrict(yamlBytes, &test); err != nil {
-		return testCase{
-			path: path,
-			err:  err,
-		}
-	}
-	return testCase{
-		path: path,
-		test: &test,
-	}
-}

cmd/cli/kubectl-kyverno/test/command.go

@@ -3,6 +3,7 @@ package test
 import (
 	"fmt"
 	"os"
+	"path/filepath"
 
 	"github.com/go-git/go-billy/v5"
 	policyreportv1alpha2 "github.com/kyverno/kyverno/api/policyreport/v1alpha2"
@@ -92,19 +93,25 @@ func testCommandExecute(
 		return rc, fmt.Errorf("unable to create open api controller, %w", err)
 	}
 	// load tests
-	fs, policies, errors := loadTests(dirPath, fileName, gitBranch)
-	if len(policies) == 0 {
+	fs, tests, err := loadTests(dirPath, fileName, gitBranch)
+	if err != nil {
+		fmt.Println()
+		fmt.Println("Error loading tests:")
+		fmt.Printf("    %s\n", err)
+		return rc, err
+	}
+	if len(tests) == 0 {
 		fmt.Println()
 		fmt.Println("No test yamls available")
 	}
-	if len(errors) > 0 {
+	if errs := tests.Errors(); len(errs) > 0 {
 		fmt.Println()
 		fmt.Println("Test errors:")
-		for _, e := range errors {
+		for _, e := range errs {
 			fmt.Printf("    %v \n", e.Error())
 		}
 	}
-	if len(policies) == 0 {
+	if len(tests) == 0 {
 		if len(errors) == 0 {
 			os.Exit(0)
 		} else {
@@ -113,19 +120,20 @@ func testCommandExecute(
 	}
 	rc = &resultCounts{}
 	var table table.Table
-	for _, p := range policies {
+	for _, p := range tests {
+		resourcePath := filepath.Dir(p.Path)
 		if tests, responses, err := applyPoliciesFromPath(
 			fs,
-			p.test,
+			p.Test,
 			fs != nil,
-			p.resourcePath,
+			resourcePath,
 			rc,
 			openApiManager,
 			filter,
 			false,
 		); err != nil {
 			return rc, sanitizederror.NewWithError("failed to apply test command", err)
-		} else if t, err := printTestResult(tests, responses, rc, failOnly, detailedResults, fs, p.resourcePath); err != nil {
+		} else if t, err := printTestResult(tests, responses, rc, failOnly, detailedResults, fs, resourcePath); err != nil {
 			return rc, sanitizederror.NewWithError("failed to print test result:", err)
 		} else {
 			table.AddFailed(t.RawRows...)

cmd/cli/kubectl-kyverno/test/load.go

@@ -2,55 +2,42 @@ package test
 
 import (
 	"fmt"
-	"io"
 	"net/url"
-	"os"
-	"path"
 	"path/filepath"
 	"sort"
 	"strings"
 
 	"github.com/go-git/go-billy/v5"
 	"github.com/go-git/go-billy/v5/memfs"
-	"github.com/kyverno/kyverno/cmd/cli/kubectl-kyverno/test/api"
 	sanitizederror "github.com/kyverno/kyverno/cmd/cli/kubectl-kyverno/utils/sanitizedError"
+	"github.com/kyverno/kyverno/cmd/cli/kubectl-kyverno/utils/source"
+	testutils "github.com/kyverno/kyverno/cmd/cli/kubectl-kyverno/utils/test"
 	gitutils "github.com/kyverno/kyverno/pkg/utils/git"
-	"k8s.io/apimachinery/pkg/util/yaml"
-	"sigs.k8s.io/controller-runtime/pkg/log"
 )
 
-type testCase struct {
-	test         *api.Test
-	resourcePath string
-}
-
-func loadTests(
-	dirPath []string,
-	fileName string,
-	gitBranch string,
-) (billy.Filesystem, []testCase, []error) {
-	var tests []testCase
-	var errors []error
-	if strings.Contains(dirPath[0], "https://") {
+func loadTests(dirPath []string, fileName string, gitBranch string) (billy.Filesystem, testutils.TestCases, error) {
+	var tests []testutils.TestCase
+	// TODO support multiple paths
+	path := dirPath[0]
+	if source.IsGit(path) {
 		fs := memfs.New()
-		if gitURL, err := url.Parse(dirPath[0]); err != nil {
-			errors = append(errors, sanitizederror.NewWithError("failed to parse URL", err))
+		gitURL, err := url.Parse(path)
+		if err != nil {
+			return nil, nil, err
 		} else {
 			pathElems := strings.Split(gitURL.Path[1:], "/")
 			if len(pathElems) <= 1 {
-				err := fmt.Errorf("invalid URL path %s - expected https://github.com/:owner/:repository/:branch (without --git-branch flag) OR https://github.com/:owner/:repository/:directory (with --git-branch flag)", gitURL.Path)
-				fmt.Printf("Error: failed to parse URL \nCause: %s\n", err)
-				os.Exit(1)
+				return nil, nil, fmt.Errorf("invalid URL path %s - expected https://github.com/:owner/:repository/:branch (without --git-branch flag) OR https://github.com/:owner/:repository/:directory (with --git-branch flag)", gitURL.Path)
 			}
 			gitURL.Path = strings.Join([]string{pathElems[0], pathElems[1]}, "/")
 			repoURL := gitURL.String()
 			var gitPathToYamls string
 			if gitBranch == "" {
 				gitPathToYamls = "/"
-				if string(dirPath[0][len(dirPath[0])-1]) == "/" {
-					gitBranch = strings.ReplaceAll(dirPath[0], repoURL+"/", "")
+				if string(path[len(path)-1]) == "/" {
+					gitBranch = strings.ReplaceAll(path, repoURL+"/", "")
 				} else {
-					gitBranch = strings.ReplaceAll(dirPath[0], repoURL, "")
+					gitBranch = strings.ReplaceAll(path, repoURL, "")
 				}
 				if gitBranch == "" {
 					gitBranch = "main"
@@ -58,97 +45,30 @@ func loadTests(
 					gitBranch = gitBranch[1:]
 				}
 			} else {
-				if string(dirPath[0][len(dirPath[0])-1]) == "/" {
-					gitPathToYamls = strings.ReplaceAll(dirPath[0], repoURL+"/", "/")
+				if string(path[len(path)-1]) == "/" {
+					gitPathToYamls = strings.ReplaceAll(path, repoURL+"/", "/")
 				} else {
-					gitPathToYamls = strings.ReplaceAll(dirPath[0], repoURL, "/")
+					gitPathToYamls = strings.ReplaceAll(path, repoURL, "/")
 				}
 			}
-			_, cloneErr := gitutils.Clone(repoURL, fs, gitBranch)
-			if cloneErr != nil {
-				fmt.Printf("Error: failed to clone repository \nCause: %s\n", cloneErr)
-				log.Log.V(3).Info(fmt.Sprintf("failed to clone repository  %v as it is not valid", repoURL), "error", cloneErr)
-				os.Exit(1)
+			if _, err := gitutils.Clone(repoURL, fs, gitBranch); err != nil {
+				return nil, nil, fmt.Errorf("Error: failed to clone repository \nCause: %s\n", err)
 			}
-			if yamlFiles, err := gitutils.ListYamls(fs, gitPathToYamls); err != nil {
-				errors = append(errors, sanitizederror.NewWithError("failed to list YAMLs in repository", err))
-			} else {
-				sort.Strings(yamlFiles)
-				for _, yamlFilePath := range yamlFiles {
-					file, err := fs.Open(yamlFilePath)
-					if err != nil {
-						errors = append(errors, sanitizederror.NewWithError("Error: failed to open file", err))
-						continue
-					}
-					if path.Base(file.Name()) == fileName {
-						resoucePath := strings.Trim(yamlFilePath, fileName)
-						yamlBytes, err := io.ReadAll(file)
-						if err != nil {
-							errors = append(errors, fmt.Errorf("failed to read file (%s)", err))
-							continue
-						}
-						test, err := loadTest(yamlBytes)
-						if err != nil {
-							errors = append(errors, fmt.Errorf("failed to load test file (%s)", err))
-							continue
-						}
-						tests = append(tests, testCase{
-							test:         test,
-							resourcePath: resoucePath,
-						})
-					}
+			yamlFiles, err := gitutils.ListYamls(fs, gitPathToYamls)
+			if err != nil {
+				return nil, nil, sanitizederror.NewWithError("failed to list YAMLs in repository", err)
+			}
+			sort.Strings(yamlFiles)
+			for _, yamlFilePath := range yamlFiles {
+				if filepath.Base(yamlFilePath) == fileName {
+					// resoucePath := strings.Trim(yamlFilePath, fileName)
+					tests = append(tests, testutils.LoadTest(fs, yamlFilePath))
 				}
 			}
 		}
-		return fs, tests, errors
+		return fs, tests, nil
 	} else {
-		path := filepath.Clean(dirPath[0])
-		tests, errors = loadLocalTest(path, fileName)
-		return nil, tests, errors
+		tests, err := testutils.LoadTests(path, fileName)
+		return nil, tests, err
 	}
 }
-
-func loadLocalTest(
-	path string,
-	fileName string,
-) ([]testCase, []error) {
-	var policies []testCase
-	var errors []error
-	files, err := os.ReadDir(path)
-	if err != nil {
-		errors = append(errors, fmt.Errorf("failed to read %v: %v", path, err.Error()))
-	} else {
-		for _, file := range files {
-			if file.IsDir() {
-				ps, errs := loadLocalTest(filepath.Join(path, file.Name()), fileName)
-				policies = append(policies, ps...)
-				errors = append(errors, errs...)
-			} else if file.Name() == fileName {
-				// We accept the risk of including files here as we read the test dir only.
-				yamlBytes, err := os.ReadFile(filepath.Join(path, file.Name())) // #nosec G304
-				if err != nil {
-					errors = append(errors, fmt.Errorf("unable to read yaml (%s)", err))
-					continue
-				}
-				test, err := loadTest(yamlBytes)
-				if err != nil {
-					errors = append(errors, fmt.Errorf("failed to load test file (%s)", err))
-					continue
-				}
-				policies = append(policies, testCase{
-					test:         test,
-					resourcePath: path,
-				})
-			}
-		}
-	}
-	return policies, errors
-}
-
-func loadTest(data []byte) (*api.Test, error) {
-	var test api.Test
-	if err := yaml.UnmarshalStrict(data, &test); err != nil {
-		return nil, err
-	}
-	return &test, nil
-}

cmd/cli/kubectl-kyverno/test/load_test.go

@@ -1,63 +1,63 @@
 package test
 
-import (
-	"reflect"
-	"testing"
+// import (
+// 	"reflect"
+// 	"testing"
 
-	"github.com/kyverno/kyverno/cmd/cli/kubectl-kyverno/test/api"
-)
+// 	"github.com/kyverno/kyverno/cmd/cli/kubectl-kyverno/test/api"
+// )
 
-func Test_loadTest(t *testing.T) {
-	tests := []struct {
-		name    string
-		data    []byte
-		want    *api.Test
-		wantErr bool
-	}{{
-		name: "invalid schema",
-		data: []byte(`
-- name: mytest
-  policies:
-    - pol.yaml
-  resources:
-    - pod.yaml
-  results:
-  - policy: evil-policy-match-foreign-pods
-    rule: evil-validation
-    resource: nginx
-    status: pass
-`),
-		want:    nil,
-		wantErr: true,
-	}, {
-		name: "unknown field",
-		data: []byte(`
-name: mytest
-policies:
-  - pol.yaml
-resources:
-  - pod.yaml
-results:
-- policy: evil-policy-match-foreign-pods
-  rule: evil-validation
-  resource: nginx
-  foo: bar
-  result: pass
-`),
-		want:    nil,
-		wantErr: true,
-	},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			got, err := loadTest(tt.data)
-			if (err != nil) != tt.wantErr {
-				t.Errorf("loadTest() error = %v, wantErr %v", err, tt.wantErr)
-				return
-			}
-			if !reflect.DeepEqual(got, tt.want) {
-				t.Errorf("loadTest() = %v, want %v", got, tt.want)
-			}
-		})
-	}
-}
+// func Test_loadTest(t *testing.T) {
+// 	tests := []struct {
+// 		name    string
+// 		data    []byte
+// 		want    *api.Test
+// 		wantErr bool
+// 	}{{
+// 		name: "invalid schema",
+// 		data: []byte(`
+// - name: mytest
+//   policies:
+//     - pol.yaml
+//   resources:
+//     - pod.yaml
+//   results:
+//   - policy: evil-policy-match-foreign-pods
+//     rule: evil-validation
+//     resource: nginx
+//     status: pass
+// `),
+// 		want:    nil,
+// 		wantErr: true,
+// 	}, {
+// 		name: "unknown field",
+// 		data: []byte(`
+// name: mytest
+// policies:
+//   - pol.yaml
+// resources:
+//   - pod.yaml
+// results:
+// - policy: evil-policy-match-foreign-pods
+//   rule: evil-validation
+//   resource: nginx
+//   foo: bar
+//   result: pass
+// `),
+// 		want:    nil,
+// 		wantErr: true,
+// 	},
+// 	}
+// 	for _, tt := range tests {
+// 		t.Run(tt.name, func(t *testing.T) {
+// 			got, err := loadTest(tt.data)
+// 			if (err != nil) != tt.wantErr {
+// 				t.Errorf("loadTest() error = %v, wantErr %v", err, tt.wantErr)
+// 				return
+// 			}
+// 			if !reflect.DeepEqual(got, tt.want) {
+// 				t.Errorf("loadTest() = %v, want %v", got, tt.want)
+// 			}
+// 		})
+// 	}
+// }

cmd/cli/kubectl-kyverno/utils/test/test.go

@@ -0,0 +1,94 @@
+package test
+
+import (
+	"io"
+	"os"
+	"path/filepath"
+
+	"github.com/go-git/go-billy/v5"
+	"github.com/kyverno/kyverno/cmd/cli/kubectl-kyverno/test/api"
+	"k8s.io/apimachinery/pkg/util/yaml"
+)
+
+type TestCase struct {
+	Path string
+	Test *api.Test
+	Err  error
+}
+
+type TestCases []TestCase
+
+func (tc TestCases) Errors() []error {
+	var errors []error
+	for _, test := range tc {
+		if test.Err != nil {
+			errors = append(errors, test.Err)
+		}
+	}
+	return errors
+}
+
+func LoadTests(dirPath string, fileName string) (TestCases, error) {
+	return loadLocalTest(filepath.Clean(dirPath), fileName)
+}
+
+func loadLocalTest(path string, fileName string) (TestCases, error) {
+	var tests []TestCase
+	files, err := os.ReadDir(path)
+	if err != nil {
+		return nil, err
+	}
+	for _, file := range files {
+		if file.IsDir() {
+			ps, err := loadLocalTest(filepath.Join(path, file.Name()), fileName)
+			if err != nil {
+				return nil, err
+			}
+			tests = append(tests, ps...)
+		} else if file.Name() == fileName {
+			tests = append(tests, LoadTest(nil, filepath.Join(path, fileName)))
+		}
+	}
+	return tests, nil
+}
+
+func LoadTest(fs billy.Filesystem, path string) TestCase {
+	var yamlBytes []byte
+	if fs != nil {
+		file, err := fs.Open(path)
+		if err != nil {
+			return TestCase{
+				Path: path,
+				Err:  err,
+			}
+		}
+		data, err := io.ReadAll(file)
+		if err != nil {
+			return TestCase{
+				Path: path,
+				Err:  err,
+			}
+		}
+		yamlBytes = data
+	} else {
+		data, err := os.ReadFile(path) // #nosec G304
+		if err != nil {
+			return TestCase{
+				Path: path,
+				Err:  err,
+			}
+		}
+		yamlBytes = data
+	}
+	var test api.Test
+	if err := yaml.UnmarshalStrict(yamlBytes, &test); err != nil {
+		return TestCase{
+			Path: path,
+			Err:  err,
+		}
+	}
+	return TestCase{
+		Path: path,
+		Test: &test,
+	}
+}