Improved testing to allow 'skip' status and fail if tested results do not exist (#1881)

* Improved testing to allow 'skip' status and fail if tested results do not exist

Signed-off-by: Trey Dockendorf <tdockendorf@osc.edu>

* Ensure exit 0 is seen as failure when should be failure

Signed-off-by: Trey Dockendorf <tdockendorf@osc.edu>

Makefile

@@ -176,10 +176,12 @@ test-e2e:
 	$(eval export E2E="")
 
 #Test TestCmd Policy
-run_testcmd_policy:
-	go build -o  kyvernoctl cmd/cli/kubectl-kyverno/main.go
-	./kyvernoctl test https://github.com/kyverno/policies/main
-	./kyvernoctl test ./test/cli/test
+run_testcmd_policy: cli
+	$(PWD)/$(CLI_PATH)/kyverno test https://github.com/kyverno/policies/main
+	$(PWD)/$(CLI_PATH)/kyverno test ./test/cli/test
+	$(PWD)/$(CLI_PATH)/kyverno test ./test/cli/test-fail/missing-policy && exit 1 || exit 0
+	$(PWD)/$(CLI_PATH)/kyverno test ./test/cli/test-fail/missing-rule && exit 1 || exit 0
+	$(PWD)/$(CLI_PATH)/kyverno test ./test/cli/test-fail/missing-resource && exit 1 || exit 0
 
 # godownloader create downloading script for kyverno-cli
 godownloader:

pkg/kyverno/test/command.go

@@ -26,6 +26,7 @@ import (
 	"github.com/kyverno/kyverno/pkg/openapi"
 	policy2 "github.com/kyverno/kyverno/pkg/policy"
 	"github.com/kyverno/kyverno/pkg/policyreport"
+	util "github.com/kyverno/kyverno/pkg/utils"
 	"github.com/lensesio/tableprinter"
 	"github.com/spf13/cobra"
 	corev1 "k8s.io/api/core/v1"
@@ -76,10 +77,10 @@ type SkippedPolicy struct {
 }
 
 type TestResults struct {
-	Policy   string `json:"policy"`
-	Rule     string `json:"rule"`
-	Status   string `json:"status"`
-	Resource string `json:"resource"`
+	Policy   string              `json:"policy"`
+	Rule     string              `json:"rule"`
+	Status   report.PolicyStatus `json:"status"`
+	Resource string              `json:"resource"`
 }
 
 type ReportResult struct {
@@ -107,6 +108,7 @@ type Values struct {
 }
 
 type resultCounts struct {
+	skip int
 	pass int
 	fail int
 }
@@ -219,26 +221,52 @@ func getLocalDirTestFiles(fs billy.Filesystem, path, fileName, valuesFile string
 	return errors
 }
 
-func buildPolicyResults(resps []*response.EngineResponse) map[string][]interface{} {
-	results := make(map[string][]interface{})
+func buildPolicyResults(resps []*response.EngineResponse, testResults []TestResults) map[string]report.PolicyReportResult {
+	results := make(map[string]report.PolicyReportResult)
 	infos := policyreport.GeneratePRsFromEngineResponse(resps, log.Log)
+	for _, resp := range resps {
+		policyName := resp.PolicyResponse.Policy
+		resourceName := resp.PolicyResponse.Resource.Name
+		var rules []string
+		for _, rule := range resp.PolicyResponse.Rules {
+			rules = append(rules, rule.Name)
+		}
+		result := report.PolicyReportResult{
+			Policy: policyName,
+			Resources: []*corev1.ObjectReference{
+				{
+					Name: resourceName,
+				},
+			},
+		}
+		for _, test := range testResults {
+			if test.Policy == policyName && test.Resource == resourceName {
+				if !util.ContainsString(rules, test.Rule) {
+					result.Status = report.StatusSkip
+				}
+				resultsKey := fmt.Sprintf("%s-%s-%s", test.Policy, test.Rule, test.Resource)
+				if _, ok := results[resultsKey]; !ok {
+					results[resultsKey] = result
+				}
+			}
+		}
+	}
 	for _, info := range infos {
 		for _, infoResult := range info.Results {
 			for _, rule := range infoResult.Rules {
 				if rule.Type != utils.Validation.String() {
 					continue
 				}
-				result := report.PolicyReportResult{
-					Policy: info.PolicyName,
-					Resources: []*corev1.ObjectReference{
-						{
-							Name: infoResult.Resource.Name,
-						},
-					},
+				var result report.PolicyReportResult
+				resultsKey := fmt.Sprintf("%s-%s-%s", info.PolicyName, rule.Name, infoResult.Resource.Name)
+				if val, ok := results[resultsKey]; ok {
+					result = val
+				} else {
+					continue
 				}
 				result.Rule = rule.Name
 				result.Status = report.PolicyStatus(rule.Check)
-				results[rule.Name] = append(results[rule.Name], result)
+				results[resultsKey] = result
 			}
 		}
 	}
@@ -357,7 +385,7 @@ func applyPoliciesFromPath(fs billy.Filesystem, policyBytes []byte, valuesFile s
 			validateEngineResponses = append(validateEngineResponses, validateErs)
 		}
 	}
-	resultsMap := buildPolicyResults(validateEngineResponses)
+	resultsMap := buildPolicyResults(validateEngineResponses, values.Results)
 	resultErr := printTestResult(resultsMap, values.Results, rc)
 	if resultErr != nil {
 		return sanitizederror.NewWithError("Unable to genrate result. Error:", resultErr)
@@ -365,9 +393,10 @@ func applyPoliciesFromPath(fs billy.Filesystem, policyBytes []byte, valuesFile s
 	return
 }
 
-func printTestResult(resps map[string][]interface{}, testResults []TestResults, rc *resultCounts) error {
+func printTestResult(resps map[string]report.PolicyReportResult, testResults []TestResults, rc *resultCounts) error {
 	printer := tableprinter.New(os.Stdout)
 	table := []*Table{}
+	boldGreen := color.New(color.FgGreen).Add(color.Bold)
 	boldRed := color.New(color.FgRed).Add(color.Bold)
 	boldYellow := color.New(color.FgYellow).Add(color.Bold)
 	boldFgCyan := color.New(color.FgCyan).Add(color.Bold)
@@ -375,32 +404,27 @@ func printTestResult(resps map[string][]interface{}, testResults []TestResults,
 		res := new(Table)
 		res.ID = i + 1
 		res.Resource = boldFgCyan.Sprintf(v.Resource) + " with " + boldFgCyan.Sprintf(v.Policy) + "/" + boldFgCyan.Sprintf(v.Rule)
-		n := resps[v.Rule]
-		data, _ := json.Marshal(n)
-		valuesBytes, err := yaml.ToJSON(data)
-		if err != nil {
-			return sanitizederror.NewWithError("failed to convert json", err)
+		resultKey := fmt.Sprintf("%s-%s-%s", v.Policy, v.Rule, v.Resource)
+		var testRes report.PolicyReportResult
+		if val, ok := resps[resultKey]; ok {
+			testRes = val
+		} else {
+			res.Result = boldYellow.Sprintf("Not found")
+			rc.fail++
+			table = append(table, res)
+			continue
 		}
-		var r []ReportResult
-		json.Unmarshal(valuesBytes, &r)
-		res.Result = boldYellow.Sprintf("Not found")
-		if len(r) != 0 {
-			var resource TestResults
-			for _, testRes := range r {
-				if testRes.Resources[0].Name == v.Resource {
-					resource.Policy = testRes.Policy
-					resource.Rule = testRes.Rule
-					resource.Status = testRes.Status
-					resource.Resource = testRes.Resources[0].Name
-					if v == resource {
-						res.Result = "Pass"
-						rc.pass++
-					} else {
-						res.Result = boldRed.Sprintf("Fail")
-						rc.fail++
-					}
-				}
+		if testRes.Status == v.Status {
+			if testRes.Status == report.StatusSkip {
+				res.Result = boldGreen.Sprintf("Skip")
+				rc.skip++
+			} else {
+				res.Result = boldGreen.Sprintf("Pass")
+				rc.pass++
 			}
+		} else {
+			res.Result = boldRed.Sprintf("Fail")
+			rc.fail++
 		}
 		table = append(table, res)
 	}

test/cli/test-fail/missing-policy/policy.yaml

@@ -0,0 +1,39 @@
+apiVersion: kyverno.io/v1
+kind: ClusterPolicy
+metadata:
+  name: disallow-latest-tag
+  annotations:
+    policies.kyverno.io/category: Best Practices
+    policies.kyverno.io/description: >-
+      The ':latest' tag is mutable and can lead to unexpected errors if the 
+      image changes. A best practice is to use an immutable tag that maps to 
+      a specific version of an application pod.      
+spec:
+  validationFailureAction: audit
+  rules:
+  - name: require-image-tag
+    match:
+      resources:
+        kinds:
+        - Pod
+        namespaces:
+        - test
+    validate:
+      message: "An image tag is required."  
+      pattern:
+        spec:
+          containers:
+          - image: "*:*"
+  - name: validate-image-tag
+    match:
+      resources:
+        kinds:
+        - Pod
+        namespaces:
+        - test
+    validate:
+      message: "Using a mutable image tag e.g. 'latest' is not allowed."
+      pattern:
+        spec:
+          containers:
+          - image: "!*:latest"

test/cli/test-fail/missing-policy/resources.yaml

@@ -0,0 +1,11 @@
+apiVersion: v1
+kind: Pod
+metadata:
+  name: test-ignore
+  namespace: default
+  labels:
+    app: app
+spec: 
+  containers:
+  - name: nginx
+    image: nginx:latest

test/cli/test-fail/missing-policy/test.yaml

@@ -0,0 +1,10 @@
+name: test-simple
+policies:
+  - policy.yaml
+resources:
+  - resources.yaml
+results:
+  - policy: missing
+    rule: validate-image-tag
+    resource: test
+    status: pass

test/cli/test-fail/missing-resource/policy.yaml

@@ -0,0 +1,39 @@
+apiVersion: kyverno.io/v1
+kind: ClusterPolicy
+metadata:
+  name: disallow-latest-tag
+  annotations:
+    policies.kyverno.io/category: Best Practices
+    policies.kyverno.io/description: >-
+      The ':latest' tag is mutable and can lead to unexpected errors if the 
+      image changes. A best practice is to use an immutable tag that maps to 
+      a specific version of an application pod.      
+spec:
+  validationFailureAction: audit
+  rules:
+  - name: require-image-tag
+    match:
+      resources:
+        kinds:
+        - Pod
+        namespaces:
+        - test
+    validate:
+      message: "An image tag is required."  
+      pattern:
+        spec:
+          containers:
+          - image: "*:*"
+  - name: validate-image-tag
+    match:
+      resources:
+        kinds:
+        - Pod
+        namespaces:
+        - test
+    validate:
+      message: "Using a mutable image tag e.g. 'latest' is not allowed."
+      pattern:
+        spec:
+          containers:
+          - image: "!*:latest"

test/cli/test-fail/missing-resource/resources.yaml

@@ -0,0 +1,11 @@
+apiVersion: v1
+kind: Pod
+metadata:
+  name: test-ignore
+  namespace: default
+  labels:
+    app: app
+spec: 
+  containers:
+  - name: nginx
+    image: nginx:latest

test/cli/test-fail/missing-resource/test.yaml

@@ -0,0 +1,10 @@
+name: test-simple
+policies:
+  - policy.yaml
+resources:
+  - resources.yaml
+results:
+  - policy: disallow-latest-tag
+    rule: validate-image-tag
+    resource: missing
+    status: pass

test/cli/test-fail/missing-rule/policy.yaml

@@ -0,0 +1,39 @@
+apiVersion: kyverno.io/v1
+kind: ClusterPolicy
+metadata:
+  name: disallow-latest-tag
+  annotations:
+    policies.kyverno.io/category: Best Practices
+    policies.kyverno.io/description: >-
+      The ':latest' tag is mutable and can lead to unexpected errors if the 
+      image changes. A best practice is to use an immutable tag that maps to 
+      a specific version of an application pod.      
+spec:
+  validationFailureAction: audit
+  rules:
+  - name: require-image-tag
+    match:
+      resources:
+        kinds:
+        - Pod
+        namespaces:
+        - test
+    validate:
+      message: "An image tag is required."  
+      pattern:
+        spec:
+          containers:
+          - image: "*:*"
+  - name: validate-image-tag
+    match:
+      resources:
+        kinds:
+        - Pod
+        namespaces:
+        - test
+    validate:
+      message: "Using a mutable image tag e.g. 'latest' is not allowed."
+      pattern:
+        spec:
+          containers:
+          - image: "!*:latest"

test/cli/test-fail/missing-rule/resources.yaml

@@ -0,0 +1,11 @@
+apiVersion: v1
+kind: Pod
+metadata:
+  name: test-ignore
+  namespace: default
+  labels:
+    app: app
+spec: 
+  containers:
+  - name: nginx
+    image: nginx:latest

test/cli/test-fail/missing-rule/test.yaml

@@ -0,0 +1,10 @@
+name: test-simple
+policies:
+  - policy.yaml
+resources:
+  - resources.yaml
+results:
+  - policy: disallow-latest-tag
+    rule: missing
+    resource: test
+    status: pass

test/cli/test/simple/policy.yaml

@@ -27,6 +27,8 @@ spec:
       resources:
         kinds:
         - Pod
+        namespaces:
+        - test
     validate:
       message: "Using a mutable image tag e.g. 'latest' is not allowed."
       pattern:

test/cli/test/simple/resources.yaml

@@ -1,10 +1,11 @@
 apiVersion: v1
 kind: Pod
 metadata:
-  name: test-web
+  name: test-require-image-tag-pass
+  namespace: test
   labels:
     app: app
-spec: 
+spec:
   containers:
   - name: nginx
     image: nginx:latest
@@ -12,10 +13,47 @@ spec:
 apiVersion: v1
 kind: Pod
 metadata:
-  name: test-app
+  name: test-require-image-tag-fail
+  namespace: test
   labels:
     app: app
-spec: 
+spec:
+  containers:
+  - name: nginx
+    image: nginx
+---
+apiVersion: v1
+kind: Pod
+metadata:
+  name: test-validate-image-tag-ignore
+  namespace: default
+  labels:
+    app: app
+spec:
+  containers:
+  - name: nginx
+    image: nginx:latest
+---
+apiVersion: v1
+kind: Pod
+metadata:
+  name: test-validate-image-tag-fail
+  namespace: test
+  labels:
+    app: app
+spec:
+  containers:
+  - name: nginx
+    image: nginx:latest
+---
+apiVersion: v1
+kind: Pod
+metadata:
+  name: test-validate-image-tag-pass
+  namespace: test
+  labels:
+    app: app
+spec:
   containers:
   - name: nginx
     image: nginx:1.12

test/cli/test/simple/test.yaml

@@ -5,10 +5,22 @@ resources:
   - resources.yaml
 results:
   - policy: disallow-latest-tag
-    rule: validate-image-tag
-    resource: test-web
+    rule: require-image-tag
+    resource: test-require-image-tag-pass
+    status: pass
+  - policy: disallow-latest-tag
+    rule: require-image-tag
+    resource: test-require-image-tag-fail
     status: fail
   - policy: disallow-latest-tag
     rule: validate-image-tag
-    resource: test-app
+    resource: test-validate-image-tag-ignore
+    status: skip
+  - policy: disallow-latest-tag
+    rule: validate-image-tag
+    resource: test-validate-image-tag-fail
+    status: fail
+  - policy: disallow-latest-tag
+    rule: validate-image-tag
+    resource: test-validate-image-tag-pass
     status: pass