refactor: refactor cli filters and add unit tests (#8177)

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

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

@@ -1,54 +0,0 @@
-package test
-
-import (
-	"fmt"
-	"strings"
-
-	"github.com/kyverno/kyverno/cmd/cli/kubectl-kyverno/test/api"
-)
-
-type filter = func(api.TestResults) bool
-
-func noFilter(api.TestResults) bool {
-	return true
-}
-
-func parseFilter(in string) filter {
-	var filters []filter
-	if in != "" {
-		for _, t := range strings.Split(in, ",") {
-			parts := strings.Split(t, "=")
-			if len(parts) != 2 {
-				fmt.Printf("\n Invalid test-case-selector argument (%s). Selecting all test cases. \n", t)
-				return noFilter
-			}
-			key := strings.TrimSpace(parts[0])
-			value := strings.TrimSpace(parts[1])
-			switch key {
-			case "policy":
-				filters = append(filters, func(r api.TestResults) bool {
-					return r.Policy == "" || r.Policy == value
-				})
-			case "rule":
-				filters = append(filters, func(r api.TestResults) bool {
-					return r.Rule == "" || r.Rule == value
-				})
-			case "resource":
-				filters = append(filters, func(r api.TestResults) bool {
-					return r.Resource == "" || r.Resource == value
-				})
-			default:
-				fmt.Printf("\n Invalid parameter. Parameter can only be policy, rule or resource. Selecting all test cases \n")
-				return noFilter
-			}
-		}
-	}
-	return func(r api.TestResults) bool {
-		for _, filter := range filters {
-			if !filter(r) {
-				return false
-			}
-		}
-		return true
-	}
-}

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

@@ -13,6 +13,7 @@ import (
 	policyreportv1alpha2 "github.com/kyverno/kyverno/api/policyreport/v1alpha2"
 	"github.com/kyverno/kyverno/cmd/cli/kubectl-kyverno/test/api"
 	"github.com/kyverno/kyverno/cmd/cli/kubectl-kyverno/utils/common"
+	filterutils "github.com/kyverno/kyverno/cmd/cli/kubectl-kyverno/utils/filter"
 	pathutils "github.com/kyverno/kyverno/cmd/cli/kubectl-kyverno/utils/path"
 	sanitizederror "github.com/kyverno/kyverno/cmd/cli/kubectl-kyverno/utils/sanitizedError"
 	"github.com/kyverno/kyverno/cmd/cli/kubectl-kyverno/utils/store"
@@ -37,7 +38,7 @@ func applyPoliciesFromPath(
 	policyResourcePath string,
 	rc *resultCounts,
 	openApiManager openapi.Manager,
-	filter filter,
+	filter filterutils.Filter,
 	auditWarn bool,
 ) (map[string]policyreportv1alpha2.PolicyReportResult, []api.TestResults, error) {
 	engineResponses := make([]engineapi.EngineResponse, 0)
@@ -48,7 +49,7 @@ func applyPoliciesFromPath(
 
 	var filteredResults []api.TestResults
 	for _, res := range values.Results {
-		if filter(res) {
+		if filter.Apply(res) {
 			filteredResults = append(filteredResults, res)
 		}
 	}

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

@@ -7,6 +7,7 @@ import (
 	policyreportv1alpha2 "github.com/kyverno/kyverno/api/policyreport/v1alpha2"
 	"github.com/kyverno/kyverno/cmd/cli/kubectl-kyverno/test/api"
 	"github.com/kyverno/kyverno/cmd/cli/kubectl-kyverno/utils/color"
+	filterutils "github.com/kyverno/kyverno/cmd/cli/kubectl-kyverno/utils/filter"
 	"github.com/kyverno/kyverno/cmd/cli/kubectl-kyverno/utils/output/table"
 	sanitizederror "github.com/kyverno/kyverno/cmd/cli/kubectl-kyverno/utils/sanitizedError"
 	"github.com/kyverno/kyverno/cmd/cli/kubectl-kyverno/utils/store"
@@ -76,7 +77,14 @@ func testCommandExecute(
 		return rc, sanitizederror.NewWithError("a directory is required", err)
 	}
 	// parse filter
-	filter := parseFilter(testCase)
+	filter, errors := filterutils.ParseFilter(testCase)
+	if len(errors) > 0 {
+		fmt.Println()
+		fmt.Println("Filter errors:")
+		for _, e := range errors {
+			fmt.Printf("    %v \n", e.Error())
+		}
+	}
 	// init openapi manager
 	openApiManager, err := openapi.NewManager(log.Log)
 	if err != nil {

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

@@ -0,0 +1,94 @@
+package filter
+
+import (
+	"fmt"
+	"strings"
+
+	"github.com/kyverno/kyverno/cmd/cli/kubectl-kyverno/test/api"
+)
+
+type Filter interface {
+	Apply(api.TestResults) bool
+}
+
+type policy struct {
+	value string
+}
+
+func (f policy) Apply(result api.TestResults) bool {
+	if result.Policy == "" {
+		return true
+	}
+	if result.Policy == f.value {
+		return true
+	}
+	return false
+}
+
+type rule struct {
+	value string
+}
+
+func (f rule) Apply(result api.TestResults) bool {
+	if result.Rule == "" {
+		return true
+	}
+	if result.Rule == f.value {
+		return true
+	}
+	return false
+}
+
+type resource struct {
+	value string
+}
+
+func (f resource) Apply(result api.TestResults) bool {
+	if result.Resource == "" {
+		return true
+	}
+	if result.Resource == f.value {
+		return true
+	}
+	return false
+}
+
+type composite struct {
+	filters []Filter
+}
+
+func (f composite) Apply(result api.TestResults) bool {
+	for _, f := range f.filters {
+		if !f.Apply(result) {
+			return false
+		}
+	}
+	return true
+}
+
+func ParseFilter(in string) (Filter, []error) {
+	var filters []Filter
+	var errors []error
+	if in != "" {
+		for _, t := range strings.Split(in, ",") {
+			parts := strings.Split(t, "=")
+			if len(parts) != 2 {
+				errors = append(errors, fmt.Errorf("Invalid test-case-selector argument (%s). Parameter must be in the form `<key>=<value>`.", t))
+			} else {
+				key := strings.TrimSpace(parts[0])
+				value := strings.TrimSpace(parts[1])
+				switch key {
+				case "policy":
+					filters = append(filters, policy{value})
+				case "rule":
+					filters = append(filters, rule{value})
+				case "resource":
+					filters = append(filters, resource{value})
+				default:
+					errors = append(errors, fmt.Errorf("Invalid test-case-selector (%s). Parameter can only be policy, rule or resource.", t))
+				}
+			}
+		}
+	}
+	return composite{filters}, errors
+}

cmd/cli/kubectl-kyverno/utils/filter/filter_test.go

@@ -0,0 +1,288 @@
+package filter
+
+import (
+	"errors"
+	"reflect"
+	"testing"
+
+	"github.com/kyverno/kyverno/cmd/cli/kubectl-kyverno/test/api"
+)
+
+func Test_policy_Apply(t *testing.T) {
+	tests := []struct {
+		name   string
+		value  string
+		result api.TestResults
+		want   bool
+	}{{
+		name:   "empty result",
+		value:  "test",
+		result: api.TestResults{},
+		want:   true,
+	}, {
+		name:  "empty value",
+		value: "",
+		result: api.TestResults{
+			Policy: "test",
+		},
+		want: false,
+	}, {
+		name:   "empty value and result",
+		value:  "",
+		result: api.TestResults{},
+		want:   true,
+	}, {
+		name:  "match",
+		value: "test",
+		result: api.TestResults{
+			Policy: "test",
+		},
+		want: true,
+	}, {
+		name:  "no match",
+		value: "test",
+		result: api.TestResults{
+			Policy: "not-test",
+		},
+		want: false,
+	}}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			f := policy{
+				value: tt.value,
+			}
+			if got := f.Apply(tt.result); got != tt.want {
+				t.Errorf("policy.Apply() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
+
+func Test_rule_Apply(t *testing.T) {
+	tests := []struct {
+		name   string
+		value  string
+		result api.TestResults
+		want   bool
+	}{{
+		name:   "empty result",
+		value:  "test",
+		result: api.TestResults{},
+		want:   true,
+	}, {
+		name:  "empty value",
+		value: "",
+		result: api.TestResults{
+			Rule: "test",
+		},
+		want: false,
+	}, {
+		name:   "empty value and result",
+		value:  "",
+		result: api.TestResults{},
+		want:   true,
+	}, {
+		name:  "match",
+		value: "test",
+		result: api.TestResults{
+			Rule: "test",
+		},
+		want: true,
+	}, {
+		name:  "no match",
+		value: "test",
+		result: api.TestResults{
+			Rule: "not-test",
+		},
+		want: false,
+	}}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			f := rule{
+				value: tt.value,
+			}
+			if got := f.Apply(tt.result); got != tt.want {
+				t.Errorf("rule.Apply() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
+
+func Test_resource_Apply(t *testing.T) {
+	tests := []struct {
+		name   string
+		value  string
+		result api.TestResults
+		want   bool
+	}{{
+		name:   "empty result",
+		value:  "test",
+		result: api.TestResults{},
+		want:   true,
+	}, {
+		name:  "empty value",
+		value: "",
+		result: api.TestResults{
+			Resource: "test",
+		},
+		want: false,
+	}, {
+		name:   "empty value and result",
+		value:  "",
+		result: api.TestResults{},
+		want:   true,
+	}, {
+		name:  "match",
+		value: "test",
+		result: api.TestResults{
+			Resource: "test",
+		},
+		want: true,
+	}, {
+		name:  "no match",
+		value: "test",
+		result: api.TestResults{
+			Resource: "not-test",
+		},
+		want: false,
+	}}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			f := resource{
+				value: tt.value,
+			}
+			if got := f.Apply(tt.result); got != tt.want {
+				t.Errorf("resource.Apply() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
+
+func Test_composite_Apply(t *testing.T) {
+	tests := []struct {
+		name    string
+		filters []Filter
+		result  api.TestResults
+		want    bool
+	}{{
+		name:    "nil",
+		filters: nil,
+		result:  api.TestResults{},
+		want:    true,
+	}, {
+		name:    "empty",
+		filters: []Filter{},
+		result:  api.TestResults{},
+		want:    true,
+	}, {
+		name:    "policy match",
+		filters: []Filter{policy{"test"}},
+		result: api.TestResults{
+			Policy: "test",
+		},
+		want: true,
+	}, {
+		name:    "policy no match",
+		filters: []Filter{policy{"test"}},
+		result: api.TestResults{
+			Policy: "not-test",
+		},
+		want: false,
+	}, {
+		name:    "policy and resource match",
+		filters: []Filter{policy{"test"}, resource{"resource"}},
+		result: api.TestResults{
+			Policy:   "test",
+			Resource: "resource",
+		},
+		want: true,
+	}, {
+		name:    "policy match and resource no match",
+		filters: []Filter{policy{"test"}, resource{"resource"}},
+		result: api.TestResults{
+			Policy:   "test",
+			Resource: "not-resource",
+		},
+		want: false,
+	},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			f := composite{
+				filters: tt.filters,
+			}
+			if got := f.Apply(tt.result); got != tt.want {
+				t.Errorf("composite.Apply() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
+
+func TestParseFilter(t *testing.T) {
+	tests := []struct {
+		name   string
+		in     string
+		filter Filter
+		errors []error
+	}{{
+		name:   "empty",
+		in:     "",
+		filter: composite{},
+		errors: nil,
+	}, {
+		name:   "invalid key",
+		in:     "foo=bar",
+		filter: composite{},
+		errors: []error{
+			errors.New("Invalid test-case-selector (foo=bar). Parameter can only be policy, rule or resource."),
+		},
+	}, {
+		name:   "invalid arg",
+		in:     "policy",
+		filter: composite{},
+		errors: []error{
+			errors.New("Invalid test-case-selector argument (policy). Parameter must be in the form `<key>=<value>`."),
+		},
+	}, {
+		name:   "policy",
+		in:     "policy=test",
+		filter: composite{[]Filter{policy{"test"}}},
+		errors: nil,
+	}, {
+		name:   "rule",
+		in:     "rule=test",
+		filter: composite{[]Filter{rule{"test"}}},
+		errors: nil,
+	}, {
+		name:   "resource",
+		in:     "resource=test",
+		filter: composite{[]Filter{resource{"test"}}},
+		errors: nil,
+	}, {
+		name:   "policy, rule and resource",
+		in:     "policy=test,rule=test,resource=test",
+		filter: composite{[]Filter{policy{"test"}, rule{"test"}, resource{"test"}}},
+		errors: nil,
+	}, {
+		name:   "policy, rule, resource and errors",
+		in:     "policy=test,rule=test,foo=bar,resource=test,policy",
+		filter: composite{[]Filter{policy{"test"}, rule{"test"}, resource{"test"}}},
+		errors: []error{
+			errors.New("Invalid test-case-selector (foo=bar). Parameter can only be policy, rule or resource."),
+			errors.New("Invalid test-case-selector argument (policy). Parameter must be in the form `<key>=<value>`."),
+		},
+	},
+	// TODO: Add test cases.
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, got1 := ParseFilter(tt.in)
+			if !reflect.DeepEqual(got, tt.filter) {
+				t.Errorf("ParseFilter() got = %v, want %v", got, tt.filter)
+			}
+			if !reflect.DeepEqual(got1, tt.errors) {
+				t.Errorf("ParseFilter() got1 = %v, want %v", got1, tt.errors)
+			}
+		})
+	}
+}