fix: TODOs in cli (#8333)

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

cmd/cli/kubectl-kyverno/commands/apply/command.go

@@ -447,12 +447,15 @@ func printSkippedAndInvalidPolicies(skipInvalidPolicies SkippedInvalidPolicies)
 }
 
 func printReport(engineResponses []engineapi.EngineResponse, auditWarn bool) {
-	clustered, namespaced := report.ComputePolicyReports(auditWarn, engineResponses...)
+	clustered, namespaced, err := report.ComputePolicyReports(auditWarn, engineResponses...)
+	if err != nil {
+		fmt.Println("Error: failed to compute policy reports")
+	}
 	if len(clustered) > 0 || len(namespaced) > 0 {
 		fmt.Println(divider)
 		fmt.Println("POLICY REPORT:")
 		fmt.Println(divider)
-		report := report.MergeClusterReports(clustered, namespaced)
+		report := report.MergeClusterReports(clustered)
 		yamlReport, _ := yaml.Marshal(report)
 		fmt.Println(string(yamlReport))
 	} else {

cmd/cli/kubectl-kyverno/commands/apply/command_test.go

@@ -317,10 +317,10 @@ func Test_Apply(t *testing.T) {
 		_, _, _, responses, err := tc.config.applyCommandHelper()
 		assert.NilError(t, err, desc)
 
-		clustered, _ := report.ComputePolicyReports(tc.config.AuditWarn, responses...)
+		clustered, _, _ := report.ComputePolicyReports(tc.config.AuditWarn, responses...)
 		assert.Assert(t, len(clustered) > 0, "policy reports should not be empty: %s", desc)
 		combined := []policyreportv1alpha2.ClusterPolicyReport{
-			report.MergeClusterReports(clustered, nil),
+			report.MergeClusterReports(clustered),
 		}
 		assert.Equal(t, len(combined), len(tc.expectedPolicyReports))
 		for i, resp := range combined {

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

@@ -179,7 +179,10 @@ func checkResult(test testapi.TestResults, fs billy.Filesystem, resoucePath stri
 			return false, "Generated resource didn't match the generated resource in the test result", "Resource diff"
 		}
 	}
-	result := report.ComputePolicyReportResult(false, response, rule)
+	result, err := report.ComputePolicyReportResult(false, response, rule)
+	if err != nil {
+		return false, err.Error(), "Error"
+	}
 	if result.Result != expected {
 		return false, result.Message, fmt.Sprintf("Want %s, got %s", expected, result.Result)
 	}

cmd/cli/kubectl-kyverno/output/table/table_test.go

@@ -185,9 +185,7 @@ func TestTable_Rows(t *testing.T) {
 			},
 			Message: "message2",
 		}},
-	},
+	}}
-	// TODO: Add test cases.
-	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			tr := &Table{

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

@@ -28,7 +28,7 @@ func Load(fs billy.Filesystem, resourcePath string, paths ...string) ([]kyvernov
 			}
 			pols = append(pols, p...)
 			vaps = append(vaps, v...)
-		} else if fs != nil /* && source.IsGit(path) */ { // TODO source.IsGit(path)
+		} else if fs != nil {
 			p, v, err := gitLoad(fs, filepath.Join(resourcePath, path))
 			if err != nil {
 				return nil, nil, err

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

@@ -8,17 +8,20 @@ import (
 	reportutils "github.com/kyverno/kyverno/pkg/utils/report"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/client-go/tools/cache"
 )
 
-func ComputePolicyReportResult(auditWarn bool, engineResponse engineapi.EngineResponse, ruleResponse engineapi.RuleResponse) policyreportv1alpha2.PolicyReportResult {
+func ComputePolicyReportResult(auditWarn bool, engineResponse engineapi.EngineResponse, ruleResponse engineapi.RuleResponse) (policyreportv1alpha2.PolicyReportResult, error) {
 	policy := engineResponse.Policy()
-	policyName := policy.GetName()
+	policyName, err := cache.MetaNamespaceKeyFunc(policy.MetaObject())
+	if err != nil {
+		return policyreportv1alpha2.PolicyReportResult{}, err
+	}
 	audit := engineResponse.GetValidationFailureAction().Audit()
 	scored := annotations.Scored(policy.GetAnnotations())
 	category := annotations.Category(policy.GetAnnotations())
 	severity := annotations.Severity(policy.GetAnnotations())
 	result := policyreportv1alpha2.PolicyReportResult{
-		// TODO policy name looks wrong, it should consider the namespace too
 		Policy: policyName,
 		Resources: []corev1.ObjectReference{
 			{
@@ -54,10 +57,10 @@ func ComputePolicyReportResult(auditWarn bool, engineResponse engineapi.EngineRe
 	result.Message = ruleResponse.Message()
 	result.Source = kyverno.ValueKyvernoApp
 	result.Timestamp = metav1.Timestamp{Seconds: ruleResponse.Stats().Timestamp()}
-	return result
+	return result, nil
 }
 
-func ComputePolicyReportResultsPerPolicy(auditWarn bool, engineResponses ...engineapi.EngineResponse) map[engineapi.GenericPolicy][]policyreportv1alpha2.PolicyReportResult {
+func ComputePolicyReportResultsPerPolicy(auditWarn bool, engineResponses ...engineapi.EngineResponse) (map[engineapi.GenericPolicy][]policyreportv1alpha2.PolicyReportResult, error) {
 	results := map[engineapi.GenericPolicy][]policyreportv1alpha2.PolicyReportResult{}
 	for _, engineResponse := range engineResponses {
 		if len(engineResponse.PolicyResponse.Rules) == 0 {
@@ -69,20 +72,26 @@ func ComputePolicyReportResultsPerPolicy(auditWarn bool, engineResponses ...engi
 			// if ruleResponse.RuleType() != engineapi.Validation && ruleResponse.RuleType() != engineapi.ImageVerify {
 			// 	continue
 			// }
-			result := ComputePolicyReportResult(auditWarn, engineResponse, ruleResponse)
+			result, err := ComputePolicyReportResult(auditWarn, engineResponse, ruleResponse)
+			if err != nil {
+				return nil, err
+			}
 			results[policy] = append(results[policy], result)
 		}
 	}
 	if len(results) == 0 {
-		return nil
+		return nil, nil
 	}
-	return results
+	return results, nil
 }
 
-func ComputePolicyReports(auditWarn bool, engineResponses ...engineapi.EngineResponse) ([]policyreportv1alpha2.ClusterPolicyReport, []policyreportv1alpha2.PolicyReport) {
+func ComputePolicyReports(auditWarn bool, engineResponses ...engineapi.EngineResponse) ([]policyreportv1alpha2.ClusterPolicyReport, []policyreportv1alpha2.PolicyReport, error) {
 	var clustered []policyreportv1alpha2.ClusterPolicyReport
 	var namespaced []policyreportv1alpha2.PolicyReport
-	perPolicyResults := ComputePolicyReportResultsPerPolicy(auditWarn, engineResponses...)
+	perPolicyResults, err := ComputePolicyReportResultsPerPolicy(auditWarn, engineResponses...)
+	if err != nil {
+		return nil, nil, err
+	}
 	for policy, results := range perPolicyResults {
 		if policy.GetNamespace() == "" {
 			report := policyreportv1alpha2.ClusterPolicyReport{
@@ -109,21 +118,14 @@ func ComputePolicyReports(auditWarn bool, engineResponses ...engineapi.EngineRes
 			namespaced = append(namespaced, report)
 		}
 	}
-	return clustered, namespaced
+	return clustered, namespaced, nil
 }
 
-func MergeClusterReports(clustered []policyreportv1alpha2.ClusterPolicyReport, namespaced []policyreportv1alpha2.PolicyReport) policyreportv1alpha2.ClusterPolicyReport {
+func MergeClusterReports(clustered []policyreportv1alpha2.ClusterPolicyReport) policyreportv1alpha2.ClusterPolicyReport {
 	var results []policyreportv1alpha2.PolicyReportResult
 	for _, report := range clustered {
 		results = append(results, report.Results...)
 	}
-	// TODO why this ?
-	for _, report := range namespaced {
-		if report.GetNamespace() != "" {
-			continue
-		}
-		results = append(results, report.Results...)
-	}
 	return policyreportv1alpha2.ClusterPolicyReport{
 		TypeMeta: metav1.TypeMeta{
 			Kind:       "ClusterPolicyReport",

cmd/cli/kubectl-kyverno/report/report_test.go

@@ -34,7 +34,8 @@ func TestComputeClusterPolicyReports(t *testing.T) {
 			"validation rule 'pods-require-limits' passed.",
 		),
 	)
-	clustered, namespaced := ComputePolicyReports(false, er)
+	clustered, namespaced, err := ComputePolicyReports(false, er)
+	assert.NilError(t, err)
 	assert.Equal(t, len(clustered), 1)
 	assert.Equal(t, len(namespaced), 0)
 	{
@@ -68,7 +69,8 @@ func TestComputePolicyReports(t *testing.T) {
 			"validation rule 'pods-require-limits' passed.",
 		),
 	)
-	clustered, namespaced := ComputePolicyReports(false, er)
+	clustered, namespaced, err := ComputePolicyReports(false, er)
+	assert.NilError(t, err)
 	assert.Equal(t, len(clustered), 0)
 	assert.Equal(t, len(namespaced), 1)
 	{
@@ -102,7 +104,8 @@ func TestComputePolicyReportResultsPerPolicyOld(t *testing.T) {
 			"validation rule 'pods-require-limits' passed.",
 		),
 	)
-	results := ComputePolicyReportResultsPerPolicy(false, er)
+	results, err := ComputePolicyReportResultsPerPolicy(false, er)
+	assert.NilError(t, err)
 	for _, result := range results {
 		assert.Equal(t, len(result), 2)
 		for _, r := range result {
@@ -146,73 +149,18 @@ func TestMergeClusterReport(t *testing.T) {
 			},
 		},
 	}}
-	namespaced := []policyreportv1alpha2.PolicyReport{{
-		TypeMeta: metav1.TypeMeta{
-			Kind:       "PolicyReport",
-			APIVersion: report.SchemeGroupVersion.String(),
-		},
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      "ns-polr-1",
-			Namespace: "ns-polr",
-		},
-		Results: []policyreportv1alpha2.PolicyReportResult{
-			{
-				Policy:    "ns-polr-1",
-				Result:    report.StatusPass,
-				Resources: make([]corev1.ObjectReference, 10),
-			},
-		},
-	}, {
-		TypeMeta: metav1.TypeMeta{
-			Kind:       "PolicyReport",
-			APIVersion: report.SchemeGroupVersion.String(),
-		},
-		ObjectMeta: metav1.ObjectMeta{
-			Name: "ns-polr-2",
-		},
-		Results: []policyreportv1alpha2.PolicyReportResult{
-			{
-				Policy:    "ns-polr-2",
-				Result:    report.StatusPass,
-				Resources: make([]corev1.ObjectReference, 5),
-			},
-		},
-	}, {
-		TypeMeta: metav1.TypeMeta{
-			Kind:       "PolicyReport",
-			APIVersion: report.SchemeGroupVersion.String(),
-		},
-		ObjectMeta: metav1.ObjectMeta{
-			Name: "polr-3",
-		},
-		Results: []policyreportv1alpha2.PolicyReportResult{
-			{
-				Policy:    "polr-3",
-				Result:    report.StatusPass,
-				Resources: make([]corev1.ObjectReference, 1),
-			},
-		},
-	}}
 	expectedResults := []policyreportv1alpha2.PolicyReportResult{{
 		Policy: "cpolr-4",
 		Result: report.StatusFail,
 	}, {
 		Policy: "cpolr-5",
 		Result: report.StatusFail,
-	}, {
-		Policy:    "ns-polr-2",
-		Result:    report.StatusPass,
-		Resources: make([]corev1.ObjectReference, 5),
-	}, {
-		Policy:    "polr-3",
-		Result:    report.StatusPass,
-		Resources: make([]corev1.ObjectReference, 1),
 	}}
-	cpolr := MergeClusterReports(clustered, namespaced)
+	cpolr := MergeClusterReports(clustered)
 	assert.Equal(t, cpolr.APIVersion, report.SchemeGroupVersion.String())
 	assert.Equal(t, cpolr.Kind, "ClusterPolicyReport")
 	assert.DeepEqual(t, cpolr.Results, expectedResults)
-	assert.Equal(t, cpolr.Summary.Pass, 2)
+	assert.Equal(t, cpolr.Summary.Pass, 0)
 	assert.Equal(t, cpolr.Summary.Fail, 2)
 }
 
@@ -326,7 +274,8 @@ func TestComputePolicyReportResult(t *testing.T) {
 	}}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got := ComputePolicyReportResult(tt.auditWarn, tt.engineResponse, tt.ruleResponse)
+			got, err := ComputePolicyReportResult(tt.auditWarn, tt.engineResponse, tt.ruleResponse)
+			assert.NilError(t, err)
 			got.Timestamp = metav1.Timestamp{}
 			if !reflect.DeepEqual(got, tt.want) {
 				t.Errorf("ComputePolicyReportResult() = %v, want %v", got, tt.want)
@@ -351,7 +300,9 @@ func TestComputePolicyReportResultsPerPolicy(t *testing.T) {
 	}}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			if got := ComputePolicyReportResultsPerPolicy(tt.auditWarn, tt.engineResponses...); !reflect.DeepEqual(got, tt.want) {
+			got, err := ComputePolicyReportResultsPerPolicy(tt.auditWarn, tt.engineResponses...)
+			assert.NilError(t, err)
+			if !reflect.DeepEqual(got, tt.want) {
 				t.Errorf("ComputePolicyReportResultsPerPolicy() = %v, want %v", got, tt.want)
 			}
 		})

cmd/cli/kubectl-kyverno/variables/parse.go

@@ -2,6 +2,8 @@ package variables
 
 import (
 	"strings"
+
+	"github.com/kyverno/kyverno/cmd/cli/kubectl-kyverno/log"
 )
 
 func parse(vars ...string) map[string]string {
@@ -10,21 +12,21 @@ func parse(vars ...string) map[string]string {
 		variable = strings.TrimSpace(variable)
 		kvs := strings.Split(variable, "=")
 		if len(kvs) != 2 {
-			// TODO warning
+			log.Log.Info("ignored variable", "variable", variable)
 			continue
 		}
 		key := strings.TrimSpace(kvs[0])
 		value := strings.TrimSpace(kvs[1])
 		if len(value) == 0 || len(key) == 0 {
-			// TODO log
+			log.Log.Info("ignored variable", "variable", variable)
 			continue
 		}
 		if strings.Contains(key, "request.object.") {
-			// TODO log
+			log.Log.Info("ignored variable (contains `request.object.`)", "variable", variable)
 			continue
 		}
 		if result[key] != "" {
-			// TODO log
+			log.Log.Info("ignored variable (duplicated)", "variable", variable)
 			continue
 		}
 		result[key] = value

pkg/engine/api/policy.go

@@ -3,6 +3,7 @@ package api
 import (
 	kyvernov1 "github.com/kyverno/kyverno/api/kyverno/v1"
 	"k8s.io/api/admissionregistration/v1alpha1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 // PolicyType represents the type of a policy
@@ -34,6 +35,8 @@ type GenericPolicy interface {
 	GetAnnotations() map[string]string
 	// IsNamespaced indicates if the policy is namespace scoped
 	IsNamespaced() bool
+	// MetaObject provides an object compatible with metav1.Object
+	MetaObject() metav1.Object
 }
 
 type KyvernoPolicy struct {
@@ -72,6 +75,10 @@ func (p *KyvernoPolicy) IsNamespaced() bool {
 	return p.policy.IsNamespaced()
 }
 
+func (p *KyvernoPolicy) MetaObject() metav1.Object {
+	return p.policy
+}
+
 func NewKyvernoPolicy(pol kyvernov1.PolicyInterface) GenericPolicy {
 	return &KyvernoPolicy{
 		policy: pol,
@@ -114,6 +121,10 @@ func (p *ValidatingAdmissionPolicy) IsNamespaced() bool {
 	return false
 }
 
+func (p *ValidatingAdmissionPolicy) MetaObject() metav1.Object {
+	return &p.policy
+}
+
 func NewValidatingAdmissionPolicy(pol v1alpha1.ValidatingAdmissionPolicy) GenericPolicy {
 	return &ValidatingAdmissionPolicy{
 		policy: pol,