From 43811733dc717d16934c8fe13dfe2c03e1cf43ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Charles-Edouard=20Br=C3=A9t=C3=A9ch=C3=A9?= Date: Wed, 29 Mar 2023 19:44:09 +0200 Subject: [PATCH] refactor: remove rules pointer (#6722) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Charles-Edouard Brétéché --- pkg/engine/background.go | 2 +- pkg/engine/engine.go | 4 +- pkg/engine/exceptions.go | 4 +- .../handlers/mutation/mutate_existing.go | 2 +- .../handlers/validation/validate_image.go | 10 ++-- .../handlers/validation/validate_manifest.go | 4 +- .../handlers/validation/validate_resource.go | 46 +++++++++---------- pkg/engine/imageVerify.go | 21 ++++----- pkg/engine/internal/imageverifier.go | 14 +++--- pkg/engine/internal/response.go | 12 ++--- 10 files changed, 57 insertions(+), 62 deletions(-) diff --git a/pkg/engine/background.go b/pkg/engine/background.go index 007fa2bb2a..15e1fd5d32 100644 --- a/pkg/engine/background.go +++ b/pkg/engine/background.go @@ -129,7 +129,7 @@ func (e *engine) filterRule( // evaluate pre-conditions if !variables.EvaluateConditions(logger, ctx, copyConditions) { logger.V(4).Info("skip rule as preconditions are not met", "rule", ruleCopy.Name) - return internal.RuleSkip(ruleCopy, ruleType, "") + return internal.RuleSkip(*ruleCopy, ruleType, "") } // build rule Response diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index 90932a92c4..b388cb8dbb 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -208,10 +208,10 @@ func (e *engine) invokeRuleHandler( // check preconditions preconditionsPassed, err := internal.CheckPreconditions(logger, policyContext, rule.GetAnyAllConditions()) if err != nil { - return resource, handlers.RuleResponses(internal.RuleError(&rule, ruleType, "failed to evaluate preconditions", err)) + return resource, handlers.RuleResponses(internal.RuleError(rule, ruleType, "failed to evaluate preconditions", err)) } if !preconditionsPassed { - return resource, handlers.RuleResponses(internal.RuleSkip(&rule, ruleType, "preconditions not met")) + return resource, handlers.RuleResponses(internal.RuleSkip(rule, ruleType, "preconditions not met")) } // process handler return handler.Process(ctx, logger, policyContext, resource, rule) diff --git a/pkg/engine/exceptions.go b/pkg/engine/exceptions.go index 78c247ef4d..cd0aad8b6d 100644 --- a/pkg/engine/exceptions.go +++ b/pkg/engine/exceptions.go @@ -85,10 +85,10 @@ func (e *engine) hasPolicyExceptions( key, err := cache.MetaNamespaceKeyFunc(exception) if err != nil { logger.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 { logger.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/mutation/mutate_existing.go b/pkg/engine/handlers/mutation/mutate_existing.go index 5d202b8c11..409afefb71 100644 --- a/pkg/engine/handlers/mutation/mutate_existing.go +++ b/pkg/engine/handlers/mutation/mutate_existing.go @@ -42,7 +42,7 @@ func (h mutateExistingHandler) Process( var patchedResources []resourceInfo targets, err := loadTargets(h.client, rule.Mutation.Targets, policyContext, logger) if err != nil { - rr := internal.RuleError(&rule, engineapi.Mutation, "", err) + rr := internal.RuleError(rule, engineapi.Mutation, "", err) responses = append(responses, *rr) } else { patchedResources = append(patchedResources, targets...) diff --git a/pkg/engine/handlers/validation/validate_image.go b/pkg/engine/handlers/validation/validate_image.go index 1359e8803c..0712b61bed 100644 --- a/pkg/engine/handlers/validation/validate_image.go +++ b/pkg/engine/handlers/validation/validate_image.go @@ -44,21 +44,21 @@ func (h validateImageHandler) Process( h.configuration, ) if err != nil { - return resource, handlers.RuleResponses(internal.RuleError(&rule, engineapi.Validation, "", err)) + return resource, handlers.RuleResponses(internal.RuleError(rule, engineapi.Validation, "", err)) } if len(matchingImages) == 0 { - return resource, handlers.RuleResponses(internal.RuleSkip(&rule, engineapi.Validation, "image verified")) + return resource, handlers.RuleResponses(internal.RuleSkip(rule, engineapi.Validation, "image verified")) } preconditionsPassed, err := internal.CheckPreconditions(logger, policyContext, rule.RawAnyAllConditions) if err != nil { - return resource, handlers.RuleResponses(internal.RuleError(&rule, engineapi.Validation, "failed to evaluate preconditions", err)) + return resource, handlers.RuleResponses(internal.RuleError(rule, engineapi.Validation, "failed to evaluate preconditions", err)) } if !preconditionsPassed { if policyContext.Policy().GetSpec().ValidationFailureAction.Audit() { return resource, nil } - return resource, handlers.RuleResponses(internal.RuleSkip(&rule, engineapi.Validation, "preconditions not met")) + return resource, handlers.RuleResponses(internal.RuleSkip(rule, engineapi.Validation, "preconditions not met")) } for _, v := range rule.VerifyImages { imageVerify := v.Convert() @@ -79,7 +79,7 @@ func (h validateImageHandler) Process( } } logger.V(4).Info("validated image", "rule", rule.Name) - return resource, handlers.RuleResponses(internal.RulePass(&rule, engineapi.Validation, "image verified")) + return resource, handlers.RuleResponses(internal.RulePass(rule, engineapi.Validation, "image verified")) } func validateImage(ctx engineapi.PolicyContext, imageVerify *kyvernov1.ImageVerification, name string, imageInfo apiutils.ImageInfo, log logr.Logger) error { diff --git a/pkg/engine/handlers/validation/validate_manifest.go b/pkg/engine/handlers/validation/validate_manifest.go index d263fad886..f43087e73f 100644 --- a/pkg/engine/handlers/validation/validate_manifest.go +++ b/pkg/engine/handlers/validation/validate_manifest.go @@ -57,13 +57,13 @@ func (h validateManifestHandler) Process( verified, reason, err := h.verifyManifest(ctx, logger, policyContext, *rule.Validation.Manifests) if err != nil { logger.V(3).Info("verifyManifest return err", "error", err.Error()) - return resource, handlers.RuleResponses(internal.RuleError(&rule, engineapi.Validation, "error occurred during manifest verification", err)) + return resource, handlers.RuleResponses(internal.RuleError(rule, engineapi.Validation, "error occurred during manifest verification", err)) } logger.V(3).Info("verifyManifest result", "verified", strconv.FormatBool(verified), "reason", reason) if !verified { return resource, handlers.RuleResponses(internal.RuleResponse(rule, engineapi.Validation, reason, engineapi.RuleStatusFail)) } - return resource, handlers.RuleResponses(internal.RulePass(&rule, engineapi.Validation, reason)) + return resource, handlers.RuleResponses(internal.RulePass(rule, engineapi.Validation, reason)) } func (h validateManifestHandler) verifyManifest( diff --git a/pkg/engine/handlers/validation/validate_resource.go b/pkg/engine/handlers/validation/validate_resource.go index 61a2b1b632..659a70d90d 100644 --- a/pkg/engine/handlers/validation/validate_resource.go +++ b/pkg/engine/handlers/validation/validate_resource.go @@ -47,14 +47,14 @@ func (h validateResourceHandler) Process( ) (unstructured.Unstructured, []engineapi.RuleResponse) { policy := policyContext.Policy() contextLoader := h.contextLoader(policy, rule) - v := newValidator(logger, contextLoader, policyContext, &rule) + v := newValidator(logger, contextLoader, policyContext, rule) return resource, handlers.RuleResponses(v.validate(ctx)) } type validator struct { log logr.Logger policyContext engineapi.PolicyContext - rule *kyvernov1.Rule + rule kyvernov1.Rule contextEntries []kyvernov1.ContextEntry anyAllConditions apiextensions.JSON pattern apiextensions.JSON @@ -66,20 +66,19 @@ type validator struct { nesting int } -func newValidator(log logr.Logger, contextLoader engineapi.EngineContextLoader, ctx engineapi.PolicyContext, rule *kyvernov1.Rule) *validator { - ruleCopy := rule.DeepCopy() +func newValidator(log logr.Logger, contextLoader engineapi.EngineContextLoader, ctx engineapi.PolicyContext, rule kyvernov1.Rule) *validator { return &validator{ log: log, - rule: ruleCopy, + rule: rule, policyContext: ctx, contextLoader: contextLoader, - contextEntries: ruleCopy.Context, - anyAllConditions: ruleCopy.GetAnyAllConditions(), - pattern: ruleCopy.Validation.GetPattern(), - anyPattern: ruleCopy.Validation.GetAnyPattern(), - deny: ruleCopy.Validation.Deny, - podSecurity: ruleCopy.Validation.PodSecurity, - forEach: ruleCopy.Validation.ForEachValidation, + contextEntries: rule.Context, + anyAllConditions: rule.GetAnyAllConditions(), + pattern: rule.Validation.GetPattern(), + anyPattern: rule.Validation.GetAnyPattern(), + deny: rule.Validation.Deny, + podSecurity: rule.Validation.PodSecurity, + forEach: rule.Validation.ForEachValidation, } } @@ -87,11 +86,10 @@ func newForEachValidator( foreach kyvernov1.ForEachValidation, contextLoader engineapi.EngineContextLoader, nesting int, - rule *kyvernov1.Rule, + rule kyvernov1.Rule, ctx engineapi.PolicyContext, log logr.Logger, ) (*validator, error) { - ruleCopy := rule.DeepCopy() anyAllConditions, err := datautils.ToMap(foreach.AnyAllConditions) if err != nil { return nil, fmt.Errorf("failed to convert ruleCopy.Validation.ForEachValidation.AnyAllConditions: %w", err) @@ -105,7 +103,7 @@ func newForEachValidator( return &validator{ log: log, policyContext: ctx, - rule: ruleCopy, + rule: rule, contextLoader: contextLoader, contextEntries: foreach.Context, anyAllConditions: anyAllConditions, @@ -219,10 +217,10 @@ func (v *validator) validateElements(ctx context.Context, foreach kyvernov1.ForE continue } msg := fmt.Sprintf("validation failure: %v", r.Message) - return internal.RuleResponse(*v.rule, engineapi.Validation, msg, r.Status), applyCount + return internal.RuleResponse(v.rule, engineapi.Validation, msg, r.Status), applyCount } msg := fmt.Sprintf("validation failure: %v", r.Message) - return internal.RuleResponse(*v.rule, engineapi.Validation, msg, r.Status), applyCount + return internal.RuleResponse(v.rule, engineapi.Validation, msg, r.Status), applyCount } applyCount++ @@ -250,7 +248,7 @@ func (v *validator) validateDeny() *engineapi.RuleResponse { return internal.RuleError(v.rule, engineapi.Validation, "failed to check deny preconditions", err) } else { if deny { - return internal.RuleResponse(*v.rule, engineapi.Validation, v.getDenyMessage(deny), engineapi.RuleStatusFail) + return internal.RuleResponse(v.rule, engineapi.Validation, v.getDenyMessage(deny), engineapi.RuleStatusFail) } return internal.RulePass(v.rule, engineapi.Validation, v.getDenyMessage(deny)) } @@ -357,7 +355,7 @@ func (v *validator) validatePodSecurity() *engineapi.RuleResponse { return rspn } else { msg := fmt.Sprintf(`Validation rule '%s' failed. It violates PodSecurity "%s:%s": %s`, v.rule.Name, v.podSecurity.Level, v.podSecurity.Version, pss.FormatChecksPrint(pssChecks)) - rspn := internal.RuleResponse(*v.rule, engineapi.Validation, msg, engineapi.RuleStatusFail) + rspn := internal.RuleResponse(v.rule, engineapi.Validation, msg, engineapi.RuleStatusFail) rspn.PodSecurityChecks = podSecurityChecks return rspn } @@ -389,13 +387,13 @@ func (v *validator) validatePatterns(resource unstructured.Unstructured) *engine } if pe.Path == "" { - return internal.RuleResponse(*v.rule, engineapi.Validation, v.buildErrorMessage(err, ""), engineapi.RuleStatusError) + return internal.RuleResponse(v.rule, engineapi.Validation, v.buildErrorMessage(err, ""), engineapi.RuleStatusError) } - return internal.RuleResponse(*v.rule, engineapi.Validation, v.buildErrorMessage(err, pe.Path), engineapi.RuleStatusFail) + return internal.RuleResponse(v.rule, engineapi.Validation, v.buildErrorMessage(err, pe.Path), engineapi.RuleStatusFail) } - return internal.RuleResponse(*v.rule, engineapi.Validation, v.buildErrorMessage(err, pe.Path), engineapi.RuleStatusError) + return internal.RuleResponse(v.rule, engineapi.Validation, v.buildErrorMessage(err, pe.Path), engineapi.RuleStatusError) } v.log.V(4).Info("successfully processed rule") @@ -454,7 +452,7 @@ func (v *validator) validatePatterns(resource unstructured.Unstructured) *engine v.log.V(4).Info(fmt.Sprintf("Validation rule '%s' failed. %s", v.rule.Name, errorStr)) msg := buildAnyPatternErrorMessage(v.rule, errorStr) - return internal.RuleResponse(*v.rule, engineapi.Validation, msg, engineapi.RuleStatusFail) + return internal.RuleResponse(v.rule, engineapi.Validation, msg, engineapi.RuleStatusFail) } } @@ -504,7 +502,7 @@ func (v *validator) buildErrorMessage(err error, path string) string { } } -func buildAnyPatternErrorMessage(rule *kyvernov1.Rule, errors []string) string { +func buildAnyPatternErrorMessage(rule kyvernov1.Rule, errors []string) string { errStr := strings.Join(errors, " ") if rule.Validation.Message == "" { return fmt.Sprintf("validation error: %s", errStr) diff --git a/pkg/engine/imageVerify.go b/pkg/engine/imageVerify.go index 165a704212..fc62c06ee6 100644 --- a/pkg/engine/imageVerify.go +++ b/pkg/engine/imageVerify.go @@ -40,12 +40,9 @@ func (e *engine) verifyAndPatchImages( defer policyContext.JSONContext().Restore() ivm := engineapi.ImageVerificationMetadata{} - rules := autogen.ComputeRules(policyContext.Policy()) applyRules := policy.GetSpec().GetApplyRules() - for i := range rules { - rule := &rules[i] - + for _, rule := range autogen.ComputeRules(policyContext.Policy()) { tracing.ChildSpan( ctx, "pkg/engine", @@ -67,7 +64,7 @@ func (e *engine) doVerifyAndPatch( ctx context.Context, logger logr.Logger, policyContext engineapi.PolicyContext, - rule *kyvernov1.Rule, + rule kyvernov1.Rule, resp *engineapi.EngineResponse, ivm *engineapi.ImageVerificationMetadata, ) { @@ -75,15 +72,15 @@ func (e *engine) doVerifyAndPatch( return } startTime := time.Now() - logger = internal.LoggerWithRule(logger, *rule) + logger = internal.LoggerWithRule(logger, rule) - if err := matches(*rule, policyContext, policyContext.NewResource()); err != nil { + if err := matches(rule, policyContext, policyContext.NewResource()); err != nil { logger.V(5).Info("resource does not match rule", "reason", err.Error()) return } // check if there is a corresponding policy exception - ruleResp := e.hasPolicyExceptions(logger, engineapi.ImageVerify, policyContext, *rule) + ruleResp := e.hasPolicyExceptions(logger, engineapi.ImageVerify, policyContext, rule) if ruleResp != nil { resp.PolicyResponse.Rules = append(resp.PolicyResponse.Rules, *ruleResp) return @@ -94,7 +91,7 @@ func (e *engine) doVerifyAndPatch( ruleImages, imageRefs, err := engineutils.ExtractMatchingImages( policyContext.NewResource(), policyContext.JSONContext(), - *rule, + rule, e.configuration, ) if err != nil { @@ -118,7 +115,7 @@ func (e *engine) doVerifyAndPatch( return } policyContext.JSONContext().Restore() - if err := internal.LoadContext(ctx, e, policyContext, *rule); err != nil { + if err := internal.LoadContext(ctx, e, policyContext, rule); err != nil { internal.AddRuleResponse( &resp.PolicyResponse, internal.RuleError(rule, engineapi.ImageVerify, "failed to load context", err), @@ -126,7 +123,7 @@ func (e *engine) doVerifyAndPatch( ) return } - ruleCopy, err := substituteVariables(rule, policyContext.JSONContext(), logger) + ruleCopy, err := substituteVariables(&rule, policyContext.JSONContext(), logger) if err != nil { internal.AddRuleResponse( &resp.PolicyResponse, @@ -139,7 +136,7 @@ func (e *engine) doVerifyAndPatch( logger, e.rclient, policyContext, - ruleCopy, + *ruleCopy, ivm, ) for _, imageVerify := range ruleCopy.VerifyImages { diff --git a/pkg/engine/internal/imageverifier.go b/pkg/engine/internal/imageverifier.go index 3a3697a8a3..136f204e1f 100644 --- a/pkg/engine/internal/imageverifier.go +++ b/pkg/engine/internal/imageverifier.go @@ -29,7 +29,7 @@ type ImageVerifier struct { logger logr.Logger rclient registryclient.Client policyContext engineapi.PolicyContext - rule *kyvernov1.Rule + rule kyvernov1.Rule ivm *engineapi.ImageVerificationMetadata } @@ -37,7 +37,7 @@ func NewImageVerifier( logger logr.Logger, rclient registryclient.Client, policyContext engineapi.PolicyContext, - rule *kyvernov1.Rule, + rule kyvernov1.Rule, ivm *engineapi.ImageVerificationMetadata, ) *ImageVerifier { return &ImageVerifier{ @@ -197,7 +197,7 @@ func (iv *ImageVerifier) Verify( if HasImageVerifiedAnnotationChanged(iv.policyContext, iv.logger) { msg := engineapi.ImageVerifyAnnotationKey + " annotation cannot be changed" iv.logger.Info("image verification error", "reason", msg) - responses = append(responses, RuleResponse(*iv.rule, engineapi.ImageVerify, msg, engineapi.RuleStatusFail)) + responses = append(responses, RuleResponse(iv.rule, engineapi.ImageVerify, msg, engineapi.RuleStatusFail)) continue } @@ -312,7 +312,7 @@ func (iv *ImageVerifier) handleRegistryErrors(image string, err error) *engineap if errors.As(err, &netErr) { return RuleError(iv.rule, engineapi.ImageVerify, fmt.Sprintf("failed to verify image %s", image), err) } - return RuleResponse(*iv.rule, engineapi.ImageVerify, msg, engineapi.RuleStatusFail) + return RuleResponse(iv.rule, engineapi.ImageVerify, msg, engineapi.RuleStatusFail) } func (iv *ImageVerifier) verifyAttestations( @@ -326,7 +326,7 @@ func (iv *ImageVerifier) verifyAttestations( path := fmt.Sprintf(".attestations[%d]", i) if attestation.PredicateType == "" { - return RuleResponse(*iv.rule, engineapi.ImageVerify, path+": missing predicateType", engineapi.RuleStatusFail), "" + return RuleResponse(iv.rule, engineapi.ImageVerify, path+": missing predicateType", engineapi.RuleStatusFail), "" } if len(attestation.Attestors) == 0 { @@ -356,7 +356,7 @@ func (iv *ImageVerifier) verifyAttestations( attestationError = iv.verifyAttestation(cosignResp.Statements, attestation, imageInfo) if attestationError != nil { attestationError = fmt.Errorf("%s: %w", entryPath+subPath, attestationError) - return RuleResponse(*iv.rule, engineapi.ImageVerify, attestationError.Error(), engineapi.RuleStatusFail), "" + return RuleResponse(iv.rule, engineapi.ImageVerify, attestationError.Error(), engineapi.RuleStatusFail), "" } verifiedCount++ @@ -368,7 +368,7 @@ func (iv *ImageVerifier) verifyAttestations( if verifiedCount < requiredCount { msg := fmt.Sprintf("image attestations verification failed, verifiedCount: %v, requiredCount: %v", verifiedCount, requiredCount) - return RuleResponse(*iv.rule, engineapi.ImageVerify, msg, engineapi.RuleStatusFail), "" + return RuleResponse(iv.rule, engineapi.ImageVerify, msg, engineapi.RuleStatusFail), "" } } diff --git a/pkg/engine/internal/response.go b/pkg/engine/internal/response.go index 7c9a9d8828..1f02e7491a 100644 --- a/pkg/engine/internal/response.go +++ b/pkg/engine/internal/response.go @@ -8,16 +8,16 @@ import ( engineapi "github.com/kyverno/kyverno/pkg/engine/api" ) -func RuleError(rule *kyvernov1.Rule, ruleType engineapi.RuleType, msg string, err error) *engineapi.RuleResponse { - return RuleResponse(*rule, ruleType, fmt.Sprintf("%s: %s", msg, err.Error()), engineapi.RuleStatusError) +func RuleError(rule kyvernov1.Rule, ruleType engineapi.RuleType, msg string, err error) *engineapi.RuleResponse { + return RuleResponse(rule, ruleType, fmt.Sprintf("%s: %s", msg, err.Error()), engineapi.RuleStatusError) } -func RuleSkip(rule *kyvernov1.Rule, ruleType engineapi.RuleType, msg string) *engineapi.RuleResponse { - return RuleResponse(*rule, ruleType, msg, engineapi.RuleStatusSkip) +func RuleSkip(rule kyvernov1.Rule, ruleType engineapi.RuleType, msg string) *engineapi.RuleResponse { + return RuleResponse(rule, ruleType, msg, engineapi.RuleStatusSkip) } -func RulePass(rule *kyvernov1.Rule, ruleType engineapi.RuleType, msg string) *engineapi.RuleResponse { - return RuleResponse(*rule, ruleType, msg, engineapi.RuleStatusPass) +func RulePass(rule kyvernov1.Rule, ruleType engineapi.RuleType, msg string) *engineapi.RuleResponse { + return RuleResponse(rule, ruleType, msg, engineapi.RuleStatusPass) } func RuleResponse(rule kyvernov1.Rule, ruleType engineapi.RuleType, msg string, status engineapi.RuleStatus) *engineapi.RuleResponse {