From c3729672f9f8247a638f4397bdfdc78159a0d6b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Charles-Edouard=20Br=C3=A9t=C3=A9ch=C3=A9?= Date: Mon, 6 Feb 2023 13:49:04 +0100 Subject: [PATCH] refactor: make funcs part of engine struct to reduce parameter passing (#6235) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * refactor: make funcs part of engine struct to reduce parameter passing Signed-off-by: Charles-Edouard Brétéché * more Signed-off-by: Charles-Edouard Brétéché --------- Signed-off-by: Charles-Edouard Brétéché --- pkg/engine/background.go | 28 +++++++++------------------- pkg/engine/engine.go | 10 +++++----- pkg/engine/generation.go | 17 +++++------------ pkg/engine/imageVerify.go | 19 ++++++++----------- pkg/engine/imageVerifyValidate.go | 9 +++------ pkg/engine/imageVerify_test.go | 8 +++++--- pkg/engine/mutation.go | 16 ++++++---------- pkg/engine/mutation_test.go | 9 +++++---- pkg/engine/validation.go | 25 +++++++++---------------- pkg/engine/validation_test.go | 8 +++++--- 10 files changed, 60 insertions(+), 89 deletions(-) diff --git a/pkg/engine/background.go b/pkg/engine/background.go index 77fffbd9bd..475ccae1e3 100644 --- a/pkg/engine/background.go +++ b/pkg/engine/background.go @@ -6,7 +6,6 @@ import ( kyvernov1 "github.com/kyverno/kyverno/api/kyverno/v1" "github.com/kyverno/kyverno/pkg/autogen" - "github.com/kyverno/kyverno/pkg/config" engineapi "github.com/kyverno/kyverno/pkg/engine/api" "github.com/kyverno/kyverno/pkg/engine/utils" "github.com/kyverno/kyverno/pkg/engine/variables" @@ -18,22 +17,16 @@ import ( // - the caller has to check the ruleResponse to determine whether the path exist // // 2. returns the list of rules that are applicable on this policy and resource, if 1 succeed -func doApplyBackgroundChecks( - contextLoader engineapi.ContextLoaderFactory, - selector engineapi.PolicyExceptionSelector, +func (e *engine) applyBackgroundChecks( policyContext engineapi.PolicyContext, - cfg config.Configuration, ) (resp *engineapi.EngineResponse) { policyStartTime := time.Now() - return filterRules(contextLoader, selector, policyContext, policyStartTime, cfg) + return e.filterRules(policyContext, policyStartTime) } -func filterRules( - contextLoader engineapi.ContextLoaderFactory, - selector engineapi.PolicyExceptionSelector, +func (e *engine) filterRules( policyContext engineapi.PolicyContext, startTime time.Time, - cfg config.Configuration, ) *engineapi.EngineResponse { newResource := policyContext.NewResource() policy := policyContext.Policy() @@ -61,14 +54,14 @@ func filterRules( }, } - if cfg.ToFilter(kind, namespace, name) { + if e.configuration.ToFilter(kind, namespace, name) { logging.WithName("ApplyBackgroundChecks").Info("resource excluded", "kind", kind, "namespace", namespace, "name", name) return resp } applyRules := policy.GetSpec().GetApplyRules() for _, rule := range autogen.ComputeRules(policy) { - if ruleResp := filterRule(contextLoader, selector, rule, policyContext, cfg); ruleResp != nil { + if ruleResp := e.filterRule(rule, policyContext); ruleResp != nil { resp.PolicyResponse.Rules = append(resp.PolicyResponse.Rules, *ruleResp) if applyRules == kyvernov1.ApplyOne && ruleResp.Status != engineapi.RuleStatusSkip { break @@ -79,12 +72,9 @@ func filterRules( return resp } -func filterRule( - contextLoader engineapi.ContextLoaderFactory, - selector engineapi.PolicyExceptionSelector, +func (e *engine) filterRule( rule kyvernov1.Rule, policyContext engineapi.PolicyContext, - cfg config.Configuration, ) *engineapi.RuleResponse { if !rule.HasGenerate() && !rule.IsMutateExisting() { return nil @@ -96,7 +86,7 @@ func filterRule( subresourceGVKToAPIResource := GetSubresourceGVKToAPIResourceMap(kindsInPolicy, policyContext) // check if there is a corresponding policy exception - ruleResp := hasPolicyExceptions(logger, selector, policyContext, &rule, subresourceGVKToAPIResource, cfg) + ruleResp := hasPolicyExceptions(logger, e.exceptionSelector, policyContext, &rule, subresourceGVKToAPIResource, e.configuration) if ruleResp != nil { return ruleResp } @@ -113,7 +103,7 @@ func filterRule( oldResource := policyContext.OldResource() admissionInfo := policyContext.AdmissionInfo() ctx := policyContext.JSONContext() - excludeGroupRole := cfg.GetExcludeGroupRole() + excludeGroupRole := e.configuration.GetExcludeGroupRole() namespaceLabels := policyContext.NamespaceLabels() logger = logging.WithName(string(ruleType)).WithValues("policy", policy.GetName(), @@ -141,7 +131,7 @@ func filterRule( policyContext.JSONContext().Checkpoint() defer policyContext.JSONContext().Restore() - if err := LoadContext(context.TODO(), contextLoader, rule.Context, policyContext, rule.Name); err != nil { + if err := LoadContext(context.TODO(), e.contextLoader, rule.Context, policyContext, rule.Name); err != nil { logger.V(4).Info("cannot add external data to the context", "reason", err.Error()) return nil } diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index c4c1eebf60..a15808ffc0 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -31,14 +31,14 @@ func (e *engine) Validate( ctx context.Context, policyContext engineapi.PolicyContext, ) *engineapi.EngineResponse { - return doValidate(ctx, e.contextLoader, e.exceptionSelector, policyContext, e.configuration) + return e.validate(ctx, policyContext) } func (e *engine) Mutate( ctx context.Context, policyContext engineapi.PolicyContext, ) *engineapi.EngineResponse { - return doMutate(ctx, e.contextLoader, e.exceptionSelector, policyContext, e.configuration) + return e.mutate(ctx, policyContext) } func (e *engine) VerifyAndPatchImages( @@ -46,20 +46,20 @@ func (e *engine) VerifyAndPatchImages( rclient registryclient.Client, policyContext engineapi.PolicyContext, ) (*engineapi.EngineResponse, *engineapi.ImageVerificationMetadata) { - return doVerifyAndPatchImages(ctx, e.contextLoader, e.exceptionSelector, rclient, policyContext, e.configuration) + return e.verifyAndPatchImages(ctx, rclient, policyContext) } func (e *engine) ApplyBackgroundChecks( policyContext engineapi.PolicyContext, ) *engineapi.EngineResponse { - return doApplyBackgroundChecks(e.contextLoader, e.exceptionSelector, policyContext, e.configuration) + return e.applyBackgroundChecks(policyContext) } func (e *engine) GenerateResponse( policyContext engineapi.PolicyContext, gr kyvernov1beta1.UpdateRequest, ) *engineapi.EngineResponse { - return doGenerateResponse(e.contextLoader, e.exceptionSelector, policyContext, gr, e.configuration) + return e.generateResponse(policyContext, gr) } func (e *engine) ContextLoader( diff --git a/pkg/engine/generation.go b/pkg/engine/generation.go index b3e6ff4eaf..5ef86ad24f 100644 --- a/pkg/engine/generation.go +++ b/pkg/engine/generation.go @@ -5,31 +5,24 @@ import ( kyvernov1beta1 "github.com/kyverno/kyverno/api/kyverno/v1beta1" "github.com/kyverno/kyverno/pkg/autogen" - "github.com/kyverno/kyverno/pkg/config" engineapi "github.com/kyverno/kyverno/pkg/engine/api" "github.com/kyverno/kyverno/pkg/logging" "k8s.io/client-go/tools/cache" ) // GenerateResponse checks for validity of generate rule on the resource -func doGenerateResponse( - contextLoader engineapi.ContextLoaderFactory, - selector engineapi.PolicyExceptionSelector, +func (e *engine) generateResponse( policyContext engineapi.PolicyContext, gr kyvernov1beta1.UpdateRequest, - cfg config.Configuration, ) (resp *engineapi.EngineResponse) { policyStartTime := time.Now() - return filterGenerateRules(contextLoader, selector, policyContext, gr.Spec.Policy, policyStartTime, cfg) + return e.filterGenerateRules(policyContext, gr.Spec.Policy, policyStartTime) } -func filterGenerateRules( - contextLoader engineapi.ContextLoaderFactory, - selector engineapi.PolicyExceptionSelector, +func (e *engine) filterGenerateRules( policyContext engineapi.PolicyContext, policyNameKey string, startTime time.Time, - cfg config.Configuration, ) *engineapi.EngineResponse { newResource := policyContext.NewResource() kind := newResource.GetKind() @@ -59,13 +52,13 @@ func filterGenerateRules( }, }, } - if cfg.ToFilter(kind, namespace, name) { + if e.configuration.ToFilter(kind, namespace, name) { logging.WithName("Generate").Info("resource excluded", "kind", kind, "namespace", namespace, "name", name) return resp } for _, rule := range autogen.ComputeRules(policyContext.Policy()) { - if ruleResp := filterRule(contextLoader, selector, rule, policyContext, cfg); ruleResp != nil { + if ruleResp := e.filterRule(rule, policyContext); ruleResp != nil { resp.PolicyResponse.Rules = append(resp.PolicyResponse.Rules, *ruleResp) } } diff --git a/pkg/engine/imageVerify.go b/pkg/engine/imageVerify.go index 8a0ec0b80a..bb85c85bfa 100644 --- a/pkg/engine/imageVerify.go +++ b/pkg/engine/imageVerify.go @@ -29,13 +29,10 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ) -func doVerifyAndPatchImages( +func (e *engine) verifyAndPatchImages( ctx context.Context, - contextLoader engineapi.ContextLoaderFactory, - selector engineapi.PolicyExceptionSelector, rclient registryclient.Client, policyContext engineapi.PolicyContext, - cfg config.Configuration, ) (*engineapi.EngineResponse, *engineapi.ImageVerificationMetadata) { resp := &engineapi.EngineResponse{} @@ -74,12 +71,12 @@ func doVerifyAndPatchImages( kindsInPolicy := append(rule.MatchResources.GetKinds(), rule.ExcludeResources.GetKinds()...) subresourceGVKToAPIResource := GetSubresourceGVKToAPIResourceMap(kindsInPolicy, policyContext) - if !matches(logger, rule, policyContext, subresourceGVKToAPIResource, cfg) { + if !matches(logger, rule, policyContext, subresourceGVKToAPIResource, e.configuration) { return } // check if there is a corresponding policy exception - ruleResp := hasPolicyExceptions(logger, selector, policyContext, rule, subresourceGVKToAPIResource, cfg) + ruleResp := hasPolicyExceptions(logger, e.exceptionSelector, policyContext, rule, subresourceGVKToAPIResource, e.configuration) if ruleResp != nil { resp.PolicyResponse.Rules = append(resp.PolicyResponse.Rules, *ruleResp) return @@ -87,7 +84,7 @@ func doVerifyAndPatchImages( logger.V(3).Info("processing image verification rule", "ruleSelector", applyRules) - ruleImages, imageRefs, err := extractMatchingImages(policyContext, rule, cfg) + ruleImages, imageRefs, err := e.extractMatchingImages(policyContext, rule) if err != nil { appendResponse(resp, rule, fmt.Sprintf("failed to extract images: %s", err.Error()), engineapi.RuleStatusError) return @@ -103,7 +100,7 @@ func doVerifyAndPatchImages( } policyContext.JSONContext().Restore() - if err := LoadContext(ctx, contextLoader, rule.Context, policyContext, rule.Name); err != nil { + if err := LoadContext(ctx, e.contextLoader, rule.Context, policyContext, rule.Name); err != nil { appendResponse(resp, rule, fmt.Sprintf("failed to load context: %s", err.Error()), engineapi.RuleStatusError) return } @@ -124,7 +121,7 @@ func doVerifyAndPatchImages( } for _, imageVerify := range ruleCopy.VerifyImages { - iv.verify(ctx, imageVerify, ruleImages, cfg) + iv.verify(ctx, imageVerify, ruleImages, e.configuration) } }, ) @@ -155,7 +152,7 @@ func getMatchingImages(images map[string]map[string]apiutils.ImageInfo, rule *ky return imageInfos, strings.Join(imageRefs, ",") } -func extractMatchingImages(policyContext engineapi.PolicyContext, rule *kyvernov1.Rule, cfg config.Configuration) ([]apiutils.ImageInfo, string, error) { +func (e *engine) extractMatchingImages(policyContext engineapi.PolicyContext, rule *kyvernov1.Rule) ([]apiutils.ImageInfo, string, error) { var ( images map[string]map[string]apiutils.ImageInfo err error @@ -163,7 +160,7 @@ func extractMatchingImages(policyContext engineapi.PolicyContext, rule *kyvernov newResource := policyContext.NewResource() images = policyContext.JSONContext().ImageInfo() if rule.ImageExtractors != nil { - images, err = policyContext.JSONContext().GenerateCustomImageInfo(&newResource, rule.ImageExtractors, cfg) + images, err = policyContext.JSONContext().GenerateCustomImageInfo(&newResource, rule.ImageExtractors, e.configuration) if err != nil { // if we get an error while generating custom images from image extractors, // don't check for matching images in imageExtractors diff --git a/pkg/engine/imageVerifyValidate.go b/pkg/engine/imageVerifyValidate.go index 5c25d854db..f84feff1d7 100644 --- a/pkg/engine/imageVerifyValidate.go +++ b/pkg/engine/imageVerifyValidate.go @@ -8,33 +8,30 @@ import ( "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" apiutils "github.com/kyverno/kyverno/pkg/utils/api" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ) -func processImageValidationRule( +func (e *engine) processImageValidationRule( ctx context.Context, - contextLoader engineapi.ContextLoaderFactory, log logr.Logger, enginectx engineapi.PolicyContext, rule *kyvernov1.Rule, - cfg config.Configuration, ) *engineapi.RuleResponse { if isDeleteRequest(enginectx) { return nil } log = log.WithValues("rule", rule.Name) - matchingImages, _, err := extractMatchingImages(enginectx, rule, cfg) + matchingImages, _, err := e.extractMatchingImages(enginectx, rule) if err != nil { return ruleResponse(*rule, engineapi.Validation, err.Error(), engineapi.RuleStatusError) } if len(matchingImages) == 0 { return ruleResponse(*rule, engineapi.Validation, "image verified", engineapi.RuleStatusSkip) } - if err := LoadContext(ctx, contextLoader, rule.Context, enginectx, rule.Name); err != nil { + if err := LoadContext(ctx, e.contextLoader, rule.Context, enginectx, rule.Name); err != nil { if _, ok := err.(gojmespath.NotFoundError); ok { log.V(3).Info("failed to load context", "reason", err.Error()) } else { diff --git a/pkg/engine/imageVerify_test.go b/pkg/engine/imageVerify_test.go index 0da23710e1..1be96519c7 100644 --- a/pkg/engine/imageVerify_test.go +++ b/pkg/engine/imageVerify_test.go @@ -167,13 +167,15 @@ func testVerifyAndPatchImages( pContext engineapi.PolicyContext, cfg config.Configuration, ) (*engineapi.EngineResponse, *engineapi.ImageVerificationMetadata) { - return doVerifyAndPatchImages( - ctx, + e := NewEngine( + cfg, LegacyContextLoaderFactory(rclient, cmResolver), nil, + ) + return e.VerifyAndPatchImages( + ctx, rclient, pContext, - cfg, ) } diff --git a/pkg/engine/mutation.go b/pkg/engine/mutation.go index 3841c778af..1ce71bf6f2 100644 --- a/pkg/engine/mutation.go +++ b/pkg/engine/mutation.go @@ -10,7 +10,6 @@ import ( gojmespath "github.com/jmespath/go-jmespath" kyvernov1 "github.com/kyverno/kyverno/api/kyverno/v1" "github.com/kyverno/kyverno/pkg/autogen" - "github.com/kyverno/kyverno/pkg/config" engineapi "github.com/kyverno/kyverno/pkg/engine/api" "github.com/kyverno/kyverno/pkg/engine/mutate" "github.com/kyverno/kyverno/pkg/logging" @@ -22,12 +21,9 @@ import ( ) // Mutate performs mutation. Overlay first and then mutation patches -func doMutate( +func (e *engine) mutate( ctx context.Context, - contextLoader engineapi.ContextLoaderFactory, - selector engineapi.PolicyExceptionSelector, policyContext engineapi.PolicyContext, - cfg config.Configuration, ) (resp *engineapi.EngineResponse) { startTime := time.Now() policy := policyContext.Policy() @@ -65,8 +61,8 @@ func doMutate( func(ctx context.Context, span trace.Span) { logger := logger.WithValues("rule", rule.Name) var excludeResource []string - if len(cfg.GetExcludeGroupRole()) > 0 { - excludeResource = cfg.GetExcludeGroupRole() + if len(e.configuration.GetExcludeGroupRole()) > 0 { + excludeResource = e.configuration.GetExcludeGroupRole() } kindsInPolicy := append(rule.MatchResources.GetKinds(), rule.ExcludeResources.GetKinds()...) @@ -78,7 +74,7 @@ func doMutate( } // check if there is a corresponding policy exception - ruleResp := hasPolicyExceptions(logger, selector, policyContext, &computeRules[i], subresourceGVKToAPIResource, cfg) + ruleResp := hasPolicyExceptions(logger, e.exceptionSelector, policyContext, &computeRules[i], subresourceGVKToAPIResource, e.configuration) if ruleResp != nil { resp.PolicyResponse.Rules = append(resp.PolicyResponse.Rules, *ruleResp) return @@ -95,7 +91,7 @@ func doMutate( logger.Error(err, "failed to query resource object") } - if err := LoadContext(ctx, contextLoader, rule.Context, policyContext, rule.Name); err != nil { + if err := LoadContext(ctx, e.contextLoader, rule.Context, policyContext, rule.Name); err != nil { if _, ok := err.(gojmespath.NotFoundError); ok { logger.V(3).Info("failed to load context", "reason", err.Error()) } else { @@ -148,7 +144,7 @@ func doMutate( policyContext: policyContext, resource: patchedResource, log: logger, - contextLoader: contextLoader, + contextLoader: e.contextLoader, nesting: 0, } diff --git a/pkg/engine/mutation_test.go b/pkg/engine/mutation_test.go index 108dc0e1f6..14080858b0 100644 --- a/pkg/engine/mutation_test.go +++ b/pkg/engine/mutation_test.go @@ -10,7 +10,6 @@ import ( kyverno "github.com/kyverno/kyverno/api/kyverno/v1" "github.com/kyverno/kyverno/cmd/cli/kubectl-kyverno/utils/store" client "github.com/kyverno/kyverno/pkg/clients/dclient" - "github.com/kyverno/kyverno/pkg/config" engineapi "github.com/kyverno/kyverno/pkg/engine/api" enginecontext "github.com/kyverno/kyverno/pkg/engine/context" "github.com/kyverno/kyverno/pkg/registryclient" @@ -26,12 +25,14 @@ func testMutate( rclient registryclient.Client, pContext *PolicyContext, ) *engineapi.EngineResponse { - return doMutate( - ctx, + e := NewEngine( + cfg, LegacyContextLoaderFactory(rclient, nil), nil, + ) + return e.Mutate( + ctx, pContext, - config.NewDefaultConfiguration(), ) } diff --git a/pkg/engine/validation.go b/pkg/engine/validation.go index 0381e279ab..bd09c2179e 100644 --- a/pkg/engine/validation.go +++ b/pkg/engine/validation.go @@ -31,12 +31,9 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ) -func doValidate( +func (e *engine) validate( ctx context.Context, - contextLoader engineapi.ContextLoaderFactory, - selector engineapi.PolicyExceptionSelector, policyContext engineapi.PolicyContext, - cfg config.Configuration, ) (resp *engineapi.EngineResponse) { resp = &engineapi.EngineResponse{} startTime := time.Now() @@ -48,7 +45,7 @@ func doValidate( logger.V(4).Info("finished policy processing", "processingTime", resp.PolicyResponse.ProcessingTime.String(), "validationRulesApplied", resp.PolicyResponse.RulesAppliedCount) }() - resp = validateResource(ctx, contextLoader, selector, logger, policyContext, cfg) + resp = e.validateResource(ctx, logger, policyContext) resp.NamespaceLabels = policyContext.NamespaceLabels() return } @@ -99,13 +96,10 @@ func buildResponse(ctx engineapi.PolicyContext, resp *engineapi.EngineResponse, resp.PolicyResponse.Timestamp = startTime.Unix() } -func validateResource( +func (e *engine) validateResource( ctx context.Context, - contextLoader engineapi.ContextLoaderFactory, - selector engineapi.PolicyExceptionSelector, log logr.Logger, enginectx engineapi.PolicyContext, - cfg config.Configuration, ) *engineapi.EngineResponse { resp := &engineapi.EngineResponse{} @@ -148,20 +142,20 @@ func validateResource( kindsInPolicy := append(rule.MatchResources.GetKinds(), rule.ExcludeResources.GetKinds()...) subresourceGVKToAPIResource := GetSubresourceGVKToAPIResourceMap(kindsInPolicy, enginectx) - if !matches(log, rule, enginectx, subresourceGVKToAPIResource, cfg) { + if !matches(log, rule, enginectx, subresourceGVKToAPIResource, e.configuration) { return nil } // check if there is a corresponding policy exception - ruleResp := hasPolicyExceptions(log, selector, enginectx, rule, subresourceGVKToAPIResource, cfg) + ruleResp := hasPolicyExceptions(log, e.exceptionSelector, enginectx, rule, subresourceGVKToAPIResource, e.configuration) if ruleResp != nil { return ruleResp } log.V(3).Info("processing validation rule", "matchCount", matchCount, "applyRules", applyRules) enginectx.JSONContext().Reset() if hasValidate && !hasYAMLSignatureVerify { - return processValidationRule(ctx, contextLoader, log, enginectx, rule) + return e.processValidationRule(ctx, log, enginectx, rule) } else if hasValidateImage { - return processImageValidationRule(ctx, contextLoader, log, enginectx, rule, cfg) + return e.processImageValidationRule(ctx, log, enginectx, rule) } else if hasYAMLSignatureVerify { return processYAMLValidationRule(log, enginectx, rule) } @@ -179,14 +173,13 @@ func validateResource( return resp } -func processValidationRule( +func (e *engine) processValidationRule( ctx context.Context, - contextLoader engineapi.ContextLoaderFactory, log logr.Logger, policyContext engineapi.PolicyContext, rule *kyvernov1.Rule, ) *engineapi.RuleResponse { - v := newValidator(log, contextLoader, policyContext, rule) + v := newValidator(log, e.contextLoader, policyContext, rule) return v.validate(ctx) } diff --git a/pkg/engine/validation_test.go b/pkg/engine/validation_test.go index cefb6b362b..a683e734dd 100644 --- a/pkg/engine/validation_test.go +++ b/pkg/engine/validation_test.go @@ -20,12 +20,14 @@ import ( ) func testValidate(ctx context.Context, rclient registryclient.Client, pContext *PolicyContext, cfg config.Configuration) *engineapi.EngineResponse { - return doValidate( - ctx, + e := NewEngine( + cfg, LegacyContextLoaderFactory(rclient, nil), nil, + ) + return e.Validate( + ctx, pContext, - cfg, ) }