From 03220cd8a9b395940d5e30bc0de88cc756acbf22 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Charles-Edouard=20Br=C3=A9t=C3=A9ch=C3=A9?= Date: Mon, 27 Mar 2023 13:22:54 +0200 Subject: [PATCH] refactor: factorise rule handler invocation code (#6694) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * refactor: factorise rule handler invocation code Signed-off-by: Charles-Edouard Brétéché * fix Signed-off-by: Charles-Edouard Brétéché --------- Signed-off-by: Charles-Edouard Brétéché --- pkg/engine/background.go | 2 +- pkg/engine/engine.go | 60 +++++++++++++++++++ pkg/engine/exceptions.go | 8 +-- pkg/engine/handlers/handler.go | 1 - pkg/engine/handlers/manifest/k8smanifest.go | 1 - pkg/engine/handlers/mutation/mutate.go | 40 +------------ .../handlers/mutation/mutateexisting.go | 38 ------------ pkg/engine/imageVerify.go | 2 +- pkg/engine/mutation.go | 28 +++------ pkg/engine/validation.go | 3 +- 10 files changed, 77 insertions(+), 106 deletions(-) diff --git a/pkg/engine/background.go b/pkg/engine/background.go index 2908635180..a5f690f8ad 100644 --- a/pkg/engine/background.go +++ b/pkg/engine/background.go @@ -69,7 +69,7 @@ func (e *engine) filterRule( } // check if there is a corresponding policy exception - ruleResp := hasPolicyExceptions(logger, ruleType, e.exceptionSelector, policyContext, &rule, e.configuration) + ruleResp := hasPolicyExceptions(logger, ruleType, e.exceptionSelector, policyContext, rule, e.configuration) if ruleResp != nil { return ruleResp } diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index dc32578772..b5982dac3c 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -2,7 +2,10 @@ package engine import ( "context" + "fmt" + "github.com/go-logr/logr" + gojmespath "github.com/jmespath/go-jmespath" kyvernov1 "github.com/kyverno/kyverno/api/kyverno/v1" kyvernov1beta1 "github.com/kyverno/kyverno/api/kyverno/v1beta1" "github.com/kyverno/kyverno/pkg/clients/dclient" @@ -13,8 +16,12 @@ import ( "github.com/kyverno/kyverno/pkg/engine/handlers/manifest" "github.com/kyverno/kyverno/pkg/engine/handlers/mutation" "github.com/kyverno/kyverno/pkg/engine/internal" + engineutils "github.com/kyverno/kyverno/pkg/engine/utils" "github.com/kyverno/kyverno/pkg/logging" "github.com/kyverno/kyverno/pkg/registryclient" + "github.com/kyverno/kyverno/pkg/tracing" + "go.opentelemetry.io/otel/trace" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ) type engine struct { @@ -119,3 +126,56 @@ func (e *engine) ContextLoader( ) } } + +func (e *engine) invokeRuleHandler( + ctx context.Context, + logger logr.Logger, + handler handlers.Handler, + policyContext engineapi.PolicyContext, + resource unstructured.Unstructured, + rule kyvernov1.Rule, + polexFilter func(logr.Logger, engineapi.PolicyContext, kyvernov1.Rule) *engineapi.RuleResponse, +) (unstructured.Unstructured, []engineapi.RuleResponse) { + return tracing.ChildSpan2( + ctx, + "pkg/engine", + fmt.Sprintf("RULE %s", rule.Name), + func(ctx context.Context, span trace.Span) (unstructured.Unstructured, []engineapi.RuleResponse) { + // check if resource and rule match + var excludeResource []string + if len(e.configuration.GetExcludedGroups()) > 0 { + excludeResource = e.configuration.GetExcludedGroups() + } + gvk, subresource := policyContext.ResourceKind() + if err := engineutils.MatchesResourceDescription( + resource, + rule, + policyContext.AdmissionInfo(), + excludeResource, + policyContext.NamespaceLabels(), + policyContext.Policy().GetNamespace(), + gvk, + subresource, + ); err != nil { + logger.V(4).Info("rule not matched", "reason", err.Error()) + return resource, nil + } + // check if there's an exception + if ruleResp := polexFilter(logger, policyContext, rule); ruleResp != nil { + return resource, handlers.RuleResponses(ruleResp) + } + // load rule context + if err := internal.LoadContext(ctx, e, policyContext, rule); err != nil { + if _, ok := err.(gojmespath.NotFoundError); ok { + logger.V(3).Info("failed to load context", "reason", err.Error()) + } else { + logger.Error(err, "failed to load context") + } + // TODO: return error ? + return resource, nil + } + // process handler + return handler.Process(ctx, logger, policyContext, resource, rule) + }, + ) +} diff --git a/pkg/engine/exceptions.go b/pkg/engine/exceptions.go index ebcf14f8a0..f079e2f6b8 100644 --- a/pkg/engine/exceptions.go +++ b/pkg/engine/exceptions.go @@ -43,7 +43,7 @@ func findExceptions( func matchesException( selector engineapi.PolicyExceptionSelector, policyContext engineapi.PolicyContext, - rule *kyvernov1.Rule, + rule kyvernov1.Rule, cfg config.Configuration, ) (*kyvernov2alpha1.PolicyException, error) { candidates, err := findExceptions(selector, policyContext.Policy(), rule.Name) @@ -76,7 +76,7 @@ func hasPolicyExceptions( ruleType engineapi.RuleType, selector engineapi.PolicyExceptionSelector, ctx engineapi.PolicyContext, - rule *kyvernov1.Rule, + rule kyvernov1.Rule, cfg config.Configuration, ) *engineapi.RuleResponse { // if matches, check if there is a corresponding policy exception @@ -87,10 +87,10 @@ func hasPolicyExceptions( key, err := cache.MetaNamespaceKeyFunc(exception) if err != nil { log.Error(err, "failed to compute policy exception key", "namespace", exception.GetNamespace(), "name", exception.GetName()) - response = internal.RuleError(rule, ruleType, "failed to compute exception key", err) + response = internal.RuleError(&rule, ruleType, "failed to compute exception key", err) } else { log.V(3).Info("policy rule skipped due to policy exception", "exception", key) - response = internal.RuleSkip(rule, ruleType, "rule skipped due to policy exception "+key) + response = internal.RuleSkip(&rule, ruleType, "rule skipped due to policy exception "+key) response.Exception = exception } } diff --git a/pkg/engine/handlers/handler.go b/pkg/engine/handlers/handler.go index 6ca4d1aced..150e45eb6e 100644 --- a/pkg/engine/handlers/handler.go +++ b/pkg/engine/handlers/handler.go @@ -16,7 +16,6 @@ type Handler interface { engineapi.PolicyContext, unstructured.Unstructured, kyvernov1.Rule, - func(logr.Logger, engineapi.PolicyContext, kyvernov1.Rule) *engineapi.RuleResponse, ) (unstructured.Unstructured, []engineapi.RuleResponse) } diff --git a/pkg/engine/handlers/manifest/k8smanifest.go b/pkg/engine/handlers/manifest/k8smanifest.go index 449d519494..a6e2840b11 100644 --- a/pkg/engine/handlers/manifest/k8smanifest.go +++ b/pkg/engine/handlers/manifest/k8smanifest.go @@ -50,7 +50,6 @@ func (h handler) Process( policyContext engineapi.PolicyContext, resource unstructured.Unstructured, rule kyvernov1.Rule, - _ func(logr.Logger, engineapi.PolicyContext, kyvernov1.Rule) *engineapi.RuleResponse, ) (unstructured.Unstructured, []engineapi.RuleResponse) { if engineutils.IsDeleteRequest(policyContext) { return resource, nil diff --git a/pkg/engine/handlers/mutation/mutate.go b/pkg/engine/handlers/mutation/mutate.go index 4013d1353e..b04c48c2d3 100644 --- a/pkg/engine/handlers/mutation/mutate.go +++ b/pkg/engine/handlers/mutation/mutate.go @@ -4,13 +4,11 @@ import ( "context" "github.com/go-logr/logr" - gojmespath "github.com/jmespath/go-jmespath" kyvernov1 "github.com/kyverno/kyverno/api/kyverno/v1" "github.com/kyverno/kyverno/pkg/config" engineapi "github.com/kyverno/kyverno/pkg/engine/api" "github.com/kyverno/kyverno/pkg/engine/handlers" "github.com/kyverno/kyverno/pkg/engine/mutate" - engineutils "github.com/kyverno/kyverno/pkg/engine/utils" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ) @@ -36,56 +34,20 @@ func (h handler) Process( policyContext engineapi.PolicyContext, resource unstructured.Unstructured, rule kyvernov1.Rule, - polexFilter func(logr.Logger, engineapi.PolicyContext, kyvernov1.Rule) *engineapi.RuleResponse, ) (unstructured.Unstructured, []engineapi.RuleResponse) { policy := policyContext.Policy() contextLoader := h.contextLoader(policy, rule) - var excludeResource []string - if len(h.configuration.GetExcludedGroups()) > 0 { - excludeResource = h.configuration.GetExcludedGroups() - } - gvk, subresource := policyContext.ResourceKind() - if err := engineutils.MatchesResourceDescription( - resource, - rule, - policyContext.AdmissionInfo(), - excludeResource, - policyContext.NamespaceLabels(), - policyContext.Policy().GetNamespace(), - gvk, - subresource, - ); err != nil { - logger.V(4).Info("rule not matched", "reason", err.Error()) - return resource, nil - } - - // check if there is a corresponding policy exception - if ruleResp := polexFilter(logger, policyContext, rule); ruleResp != nil { - return resource, handlers.RuleResponses(ruleResp) - } - + _, subresource := policyContext.ResourceKind() logger.V(3).Info("processing mutate rule") - - if err := contextLoader(ctx, rule.Context, policyContext.JSONContext()); err != nil { - if _, ok := err.(gojmespath.NotFoundError); ok { - logger.V(3).Info("failed to load context", "reason", err.Error()) - } else { - logger.Error(err, "failed to load context") - } - // TODO: return error ? - return resource, nil - } var parentResourceGVR metav1.GroupVersionResource if subresource != "" { parentResourceGVR = policyContext.RequestResource() } - resourceInfo := resourceInfo{ unstructured: resource, subresource: subresource, parentResourceGVR: parentResourceGVR, } - // logger.V(4).Info("apply rule to resource", "resource namespace", patchedResource.unstructured.GetNamespace(), "resource name", patchedResource.unstructured.GetName()) var mutateResp *mutate.Response if rule.Mutation.ForEachMutation != nil { diff --git a/pkg/engine/handlers/mutation/mutateexisting.go b/pkg/engine/handlers/mutation/mutateexisting.go index 42a08cfd84..7fd4dff13e 100644 --- a/pkg/engine/handlers/mutation/mutateexisting.go +++ b/pkg/engine/handlers/mutation/mutateexisting.go @@ -4,7 +4,6 @@ import ( "context" "github.com/go-logr/logr" - gojmespath "github.com/jmespath/go-jmespath" kyvernov1 "github.com/kyverno/kyverno/api/kyverno/v1" "github.com/kyverno/kyverno/pkg/clients/dclient" "github.com/kyverno/kyverno/pkg/config" @@ -12,7 +11,6 @@ import ( "github.com/kyverno/kyverno/pkg/engine/handlers" "github.com/kyverno/kyverno/pkg/engine/internal" "github.com/kyverno/kyverno/pkg/engine/mutate" - engineutils "github.com/kyverno/kyverno/pkg/engine/utils" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ) @@ -40,47 +38,11 @@ func (h handlerExisting) Process( policyContext engineapi.PolicyContext, resource unstructured.Unstructured, rule kyvernov1.Rule, - polexFilter func(logr.Logger, engineapi.PolicyContext, kyvernov1.Rule) *engineapi.RuleResponse, ) (unstructured.Unstructured, []engineapi.RuleResponse) { policy := policyContext.Policy() contextLoader := h.contextLoader(policy, rule) var responses []engineapi.RuleResponse - var excludeResource []string - if len(h.configuration.GetExcludedGroups()) > 0 { - excludeResource = h.configuration.GetExcludedGroups() - } - gvk, subresource := policyContext.ResourceKind() - if err := engineutils.MatchesResourceDescription( - resource, - rule, - policyContext.AdmissionInfo(), - excludeResource, - policyContext.NamespaceLabels(), - policyContext.Policy().GetNamespace(), - gvk, - subresource, - ); err != nil { - logger.V(4).Info("rule not matched", "reason", err.Error()) - return resource, nil - } - - // check if there is a corresponding policy exception - if ruleResp := polexFilter(logger, policyContext, rule); ruleResp != nil { - return resource, handlers.RuleResponses(ruleResp) - } - logger.V(3).Info("processing mutate rule") - - if err := contextLoader(ctx, rule.Context, policyContext.JSONContext()); err != nil { - if _, ok := err.(gojmespath.NotFoundError); ok { - logger.V(3).Info("failed to load context", "reason", err.Error()) - } else { - logger.Error(err, "failed to load context") - } - // TODO: return error ? - return resource, nil - } - var patchedResources []resourceInfo targets, err := loadTargets(h.client, rule.Mutation.Targets, policyContext, logger) if err != nil { diff --git a/pkg/engine/imageVerify.go b/pkg/engine/imageVerify.go index 7803f2dfa9..f96b43ce25 100644 --- a/pkg/engine/imageVerify.go +++ b/pkg/engine/imageVerify.go @@ -84,7 +84,7 @@ func (e *engine) doVerifyAndPatch( } // check if there is a corresponding policy exception - ruleResp := hasPolicyExceptions(logger, engineapi.ImageVerify, e.exceptionSelector, policyContext, rule, e.configuration) + ruleResp := hasPolicyExceptions(logger, engineapi.ImageVerify, e.exceptionSelector, policyContext, *rule, e.configuration) if ruleResp != nil { resp.PolicyResponse.Rules = append(resp.PolicyResponse.Rules, *ruleResp) return diff --git a/pkg/engine/mutation.go b/pkg/engine/mutation.go index 93bcf6292c..4fd9fae1e0 100644 --- a/pkg/engine/mutation.go +++ b/pkg/engine/mutation.go @@ -2,7 +2,6 @@ package engine import ( "context" - "fmt" "time" "github.com/go-logr/logr" @@ -10,8 +9,6 @@ import ( "github.com/kyverno/kyverno/pkg/autogen" engineapi "github.com/kyverno/kyverno/pkg/engine/api" "github.com/kyverno/kyverno/pkg/engine/internal" - "github.com/kyverno/kyverno/pkg/tracing" - "go.opentelemetry.io/otel/trace" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ) @@ -34,25 +31,18 @@ func (e *engine) mutate( applyRules := policy.GetSpec().GetApplyRules() for _, rule := range autogen.ComputeRules(policy) { + logger := internal.LoggerWithRule(logger, rule) if !rule.HasMutate() { continue } - resource, ruleResp := tracing.ChildSpan2( - ctx, - "pkg/engine", - fmt.Sprintf("RULE %s", rule.Name), - func(ctx context.Context, span trace.Span) (unstructured.Unstructured, []engineapi.RuleResponse) { - logger := internal.LoggerWithRule(logger, rule) - polexFilter := func(logger logr.Logger, policyContext engineapi.PolicyContext, rule kyvernov1.Rule) *engineapi.RuleResponse { - return hasPolicyExceptions(logger, engineapi.Validation, e.exceptionSelector, policyContext, &rule, e.configuration) - } - if !policyContext.AdmissionOperation() && rule.IsMutateExisting() { - return e.mutateExistingHandler.Process(ctx, logger, policyContext, matchedResource, rule, polexFilter) - } else { - return e.mutateHandler.Process(ctx, logger, policyContext, matchedResource, rule, polexFilter) - } - }, - ) + polexFilter := func(logger logr.Logger, policyContext engineapi.PolicyContext, rule kyvernov1.Rule) *engineapi.RuleResponse { + return hasPolicyExceptions(logger, engineapi.Validation, e.exceptionSelector, policyContext, rule, e.configuration) + } + handler := e.mutateHandler + if !policyContext.AdmissionOperation() && rule.IsMutateExisting() { + handler = e.mutateExistingHandler + } + resource, ruleResp := e.invokeRuleHandler(ctx, logger, handler, policyContext, matchedResource, rule, polexFilter) matchedResource = resource for _, ruleResp := range ruleResp { ruleResp := ruleResp diff --git a/pkg/engine/validation.go b/pkg/engine/validation.go index a0429eba2f..132aac7c8f 100644 --- a/pkg/engine/validation.go +++ b/pkg/engine/validation.go @@ -80,7 +80,7 @@ func (e *engine) validateResource( return nil } // check if there is a corresponding policy exception - ruleResp := hasPolicyExceptions(logger, engineapi.Validation, e.exceptionSelector, policyContext, rule, e.configuration) + ruleResp := hasPolicyExceptions(logger, engineapi.Validation, e.exceptionSelector, policyContext, *rule, e.configuration) if ruleResp != nil { return handlers.RuleResponses(ruleResp) } @@ -96,7 +96,6 @@ func (e *engine) validateResource( policyContext, policyContext.NewResource(), *rule, - nil, ) return rr }