From de00c78513016cd218992b2f82d9376b8ebcc746 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Charles-Edouard=20Br=C3=A9t=C3=A9ch=C3=A9?= Date: Thu, 6 Apr 2023 00:55:42 +0200 Subject: [PATCH] refactor: simplify engine responses (#6804) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Charles-Edouard Brétéché --- cmd/cli/kubectl-kyverno/apply/report_test.go | 41 +++++++++++--------- pkg/controllers/report/utils/events.go | 10 ++--- pkg/engine/api/policyresponse.go | 16 ++++---- pkg/engine/image_verify.go | 5 +-- pkg/engine/mutation.go | 5 +-- pkg/engine/validation.go | 5 +-- pkg/event/events.go | 8 ++-- pkg/webhooks/utils/event.go | 10 ++--- 8 files changed, 49 insertions(+), 51 deletions(-) diff --git a/cmd/cli/kubectl-kyverno/apply/report_test.go b/cmd/cli/kubectl-kyverno/apply/report_test.go index 8a26aeb525..34ea473f93 100644 --- a/cmd/cli/kubectl-kyverno/apply/report_test.go +++ b/cmd/cli/kubectl-kyverno/apply/report_test.go @@ -92,15 +92,18 @@ func Test_buildPolicyReports(t *testing.T) { er := engineapi.EngineResponse{} er.Policy = &policy - er.PolicyResponse.Add(engineapi.ExecutionStats{}, *engineapi.RuleFail( - "pods-require-account", - engineapi.Validation, - "validation error: User pods must include an account for charging. Rule pods-require-account failed at path /metadata/labels/"), - ) - er.PolicyResponse.Add(engineapi.ExecutionStats{}, *engineapi.RulePass( - "pods-require-limits", - engineapi.Validation, - "validation rule 'pods-require-limits' passed."), + er.PolicyResponse.Add( + engineapi.ExecutionStats{}, + *engineapi.RuleFail( + "pods-require-account", + engineapi.Validation, + "validation error: User pods must include an account for charging. Rule pods-require-account failed at path /metadata/labels/", + ), + *engineapi.RulePass( + "pods-require-limits", + engineapi.Validation, + "validation rule 'pods-require-limits' passed.", + ), ) info := kyvCommon.ProcessValidateEngineResponse(&policy, &er, "", rc, true, false) @@ -137,15 +140,17 @@ func Test_buildPolicyResults(t *testing.T) { er := engineapi.EngineResponse{} er.Policy = &policy - er.PolicyResponse.Add(engineapi.ExecutionStats{}, *engineapi.RuleFail( - "pods-require-account", - engineapi.Validation, - "validation error: User pods must include an account for charging. Rule pods-require-account failed at path /metadata/labels/"), - ) - er.PolicyResponse.Add(engineapi.ExecutionStats{}, *engineapi.RulePass( - "pods-require-limits", - engineapi.Validation, - "validation rule 'pods-require-limits' passed."), + er.PolicyResponse.Add( + engineapi.ExecutionStats{}, *engineapi.RuleFail( + "pods-require-account", + engineapi.Validation, + "validation error: User pods must include an account for charging. Rule pods-require-account failed at path /metadata/labels/", + ), + *engineapi.RulePass( + "pods-require-limits", + engineapi.Validation, + "validation rule 'pods-require-limits' passed.", + ), ) info := kyvCommon.ProcessValidateEngineResponse(&policy, &er, "", rc, true, false) diff --git a/pkg/controllers/report/utils/events.go b/pkg/controllers/report/utils/events.go index 52952c0897..a24c80fa38 100644 --- a/pkg/controllers/report/utils/events.go +++ b/pkg/controllers/report/utils/events.go @@ -33,9 +33,9 @@ func generateSuccessEvents(log logr.Logger, ers ...engineapi.EngineResponse) (ev func generateExceptionEvents(log logr.Logger, ers ...engineapi.EngineResponse) (eventInfos []event.Info) { for _, er := range ers { - for i, ruleResp := range er.PolicyResponse.Rules { + for _, ruleResp := range er.PolicyResponse.Rules { if ruleResp.Status() == engineapi.RuleStatusSkip && ruleResp.IsException() { - eventInfos = append(eventInfos, event.NewPolicyExceptionEvents(er, &er.PolicyResponse.Rules[i], event.PolicyController)...) + eventInfos = append(eventInfos, event.NewPolicyExceptionEvents(er, ruleResp, event.PolicyController)...) } } } @@ -57,11 +57,11 @@ func generateFailEventsPerEr(log logr.Logger, er engineapi.EngineResponse) []eve "namespace", er.Resource.GetNamespace(), "name", er.Resource.GetName(), ) - for i, rule := range er.PolicyResponse.Rules { + for _, rule := range er.PolicyResponse.Rules { if rule.Status() != engineapi.RuleStatusPass && rule.Status() != engineapi.RuleStatusSkip { - eventResource := event.NewResourceViolationEvent(event.PolicyController, event.PolicyViolation, er, &er.PolicyResponse.Rules[i]) + eventResource := event.NewResourceViolationEvent(event.PolicyController, event.PolicyViolation, er, rule) eventInfos = append(eventInfos, eventResource) - eventPolicy := event.NewPolicyFailEvent(event.PolicyController, event.PolicyViolation, er, &er.PolicyResponse.Rules[i], false) + eventPolicy := event.NewPolicyFailEvent(event.PolicyController, event.PolicyViolation, er, rule, false) eventInfos = append(eventInfos, eventPolicy) } } diff --git a/pkg/engine/api/policyresponse.go b/pkg/engine/api/policyresponse.go index c1bab1b832..dcaad1128e 100644 --- a/pkg/engine/api/policyresponse.go +++ b/pkg/engine/api/policyresponse.go @@ -8,13 +8,15 @@ type PolicyResponse struct { Rules []RuleResponse } -func (pr *PolicyResponse) Add(stats ExecutionStats, response RuleResponse) { - pr.Rules = append(pr.Rules, response.WithStats(stats)) - status := response.Status() - if status == RuleStatusPass || status == RuleStatusFail { - pr.Stats.RulesAppliedCount++ - } else if status == RuleStatusError { - pr.Stats.RulesErrorCount++ +func (pr *PolicyResponse) Add(stats ExecutionStats, responses ...RuleResponse) { + for _, response := range responses { + pr.Rules = append(pr.Rules, response.WithStats(stats)) + status := response.Status() + if status == RuleStatusPass || status == RuleStatusFail { + pr.Stats.RulesAppliedCount++ + } else if status == RuleStatusError { + pr.Stats.RulesErrorCount++ + } } } diff --git a/pkg/engine/image_verify.go b/pkg/engine/image_verify.go index dc1fdc5cee..6f2de4f7fb 100644 --- a/pkg/engine/image_verify.go +++ b/pkg/engine/image_verify.go @@ -53,10 +53,7 @@ func (e *engine) verifyAndPatchImages( engineapi.ImageVerify, ) matchedResource = resource - stats := engineapi.NewExecutionStats(startTime, time.Now()) - for _, ruleResp := range ruleResp { - resp.Add(stats, ruleResp) - } + resp.Add(engineapi.NewExecutionStats(startTime, time.Now()), ruleResp...) if applyRules == kyvernov1.ApplyOne && resp.Stats.RulesAppliedCount > 0 { break } diff --git a/pkg/engine/mutation.go b/pkg/engine/mutation.go index 6964c198ec..4c8eac6969 100644 --- a/pkg/engine/mutation.go +++ b/pkg/engine/mutation.go @@ -50,10 +50,7 @@ func (e *engine) mutate( engineapi.Mutation, ) matchedResource = resource - stats := engineapi.NewExecutionStats(startTime, time.Now()) - for _, ruleResp := range ruleResp { - resp.Add(stats, ruleResp) - } + resp.Add(engineapi.NewExecutionStats(startTime, time.Now()), ruleResp...) if applyRules == kyvernov1.ApplyOne && resp.Stats.RulesAppliedCount > 0 { break } diff --git a/pkg/engine/validation.go b/pkg/engine/validation.go index 01f7b498a9..62c6a5dd69 100644 --- a/pkg/engine/validation.go +++ b/pkg/engine/validation.go @@ -68,10 +68,7 @@ func (e *engine) validate( engineapi.Validation, ) matchedResource = resource - stats := engineapi.NewExecutionStats(startTime, time.Now()) - for _, ruleResp := range ruleResp { - resp.Add(stats, ruleResp) - } + resp.Add(engineapi.NewExecutionStats(startTime, time.Now()), ruleResp...) if applyRules == kyvernov1.ApplyOne && resp.Stats.RulesAppliedCount > 0 { break } diff --git a/pkg/event/events.go b/pkg/event/events.go index 4950e9bef2..acf62697f7 100644 --- a/pkg/event/events.go +++ b/pkg/event/events.go @@ -9,7 +9,7 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ) -func NewPolicyFailEvent(source Source, reason Reason, engineResponse engineapi.EngineResponse, ruleResp *engineapi.RuleResponse, blocked bool) Info { +func NewPolicyFailEvent(source Source, reason Reason, engineResponse engineapi.EngineResponse, ruleResp engineapi.RuleResponse, blocked bool) Info { return Info{ Kind: getPolicyKind(engineResponse.Policy), Name: engineResponse.Policy.GetName(), @@ -20,7 +20,7 @@ func NewPolicyFailEvent(source Source, reason Reason, engineResponse engineapi.E } } -func buildPolicyEventMessage(resp *engineapi.RuleResponse, resource engineapi.ResourceSpec, blocked bool) string { +func buildPolicyEventMessage(resp engineapi.RuleResponse, resource engineapi.ResourceSpec, blocked bool) string { var b strings.Builder if resource.Namespace != "" { fmt.Fprintf(&b, "%s %s/%s", resource.Kind, resource.Namespace, resource.Name) @@ -68,7 +68,7 @@ func NewPolicyAppliedEvent(source Source, engineResponse engineapi.EngineRespons } } -func NewResourceViolationEvent(source Source, reason Reason, engineResponse engineapi.EngineResponse, ruleResp *engineapi.RuleResponse) Info { +func NewResourceViolationEvent(source Source, reason Reason, engineResponse engineapi.EngineResponse, ruleResp engineapi.RuleResponse) Info { var bldr strings.Builder defer bldr.Reset() @@ -123,7 +123,7 @@ func NewBackgroundSuccessEvent(policy, rule string, source Source, r *unstructur return events } -func NewPolicyExceptionEvents(engineResponse engineapi.EngineResponse, ruleResp *engineapi.RuleResponse, source Source) []Info { +func NewPolicyExceptionEvents(engineResponse engineapi.EngineResponse, ruleResp engineapi.RuleResponse, source Source) []Info { exception := ruleResp.Exception() exceptionName, exceptionNamespace := exception.GetName(), exception.GetNamespace() policyMessage := fmt.Sprintf("resource %s was skipped from rule %s due to policy exception %s/%s", resourceKey(engineResponse.PatchedResource), ruleResp.Name(), exceptionNamespace, exceptionName) diff --git a/pkg/webhooks/utils/event.go b/pkg/webhooks/utils/event.go index d3678d4828..f4522397dd 100644 --- a/pkg/webhooks/utils/event.go +++ b/pkg/webhooks/utils/event.go @@ -20,20 +20,20 @@ func GenerateEvents(engineResponses []engineapi.EngineResponse, blocked bool) [] continue } if !er.IsSuccessful() { - for i, ruleResp := range er.PolicyResponse.Rules { + for _, ruleResp := range er.PolicyResponse.Rules { if ruleResp.Status() == engineapi.RuleStatusFail || ruleResp.Status() == engineapi.RuleStatusError { - e := event.NewPolicyFailEvent(event.AdmissionController, event.PolicyViolation, er, &er.PolicyResponse.Rules[i], blocked) + e := event.NewPolicyFailEvent(event.AdmissionController, event.PolicyViolation, er, ruleResp, blocked) events = append(events, e) } if !blocked { - e := event.NewResourceViolationEvent(event.AdmissionController, event.PolicyViolation, er, &er.PolicyResponse.Rules[i]) + e := event.NewResourceViolationEvent(event.AdmissionController, event.PolicyViolation, er, ruleResp) events = append(events, e) } } } else if er.IsSkipped() { // Handle PolicyException Event - for i, ruleResp := range er.PolicyResponse.Rules { + for _, ruleResp := range er.PolicyResponse.Rules { if ruleResp.Status() == engineapi.RuleStatusSkip && !blocked && ruleResp.IsException() { - events = append(events, event.NewPolicyExceptionEvents(er, &er.PolicyResponse.Rules[i], event.AdmissionController)...) + events = append(events, event.NewPolicyExceptionEvents(er, ruleResp, event.AdmissionController)...) } } } else if !er.IsSkipped() {