From 4e3f297da20bd23062bb81dabd55e486d2d03cec Mon Sep 17 00:00:00 2001 From: Vishal Choudhary Date: Mon, 21 Oct 2024 19:40:06 +0530 Subject: [PATCH] fix: update match logic for old object validation (#11427) * fix: update match logic for old object validation Signed-off-by: Vishal Choudhary * fix: linter Signed-off-by: Vishal Choudhary * fix: failing test due to user info Signed-off-by: Vishal Choudhary * fix: debug logs Signed-off-by: Vishal Choudhary --------- Signed-off-by: Vishal Choudhary Co-authored-by: shuting --- pkg/engine/handlers/validation/utils.go | 61 ++++++++----------- .../handlers/validation/validate_assert.go | 2 +- .../handlers/validation/validate_pss.go | 2 +- .../handlers/validation/validate_resource.go | 10 ++- 4 files changed, 30 insertions(+), 45 deletions(-) diff --git a/pkg/engine/handlers/validation/utils.go b/pkg/engine/handlers/validation/utils.go index 2984e2b051..1f6c9e1a94 100644 --- a/pkg/engine/handlers/validation/utils.go +++ b/pkg/engine/handlers/validation/utils.go @@ -3,45 +3,32 @@ package validation import ( kyvernov1 "github.com/kyverno/kyverno/api/kyverno/v1" kyvernov2 "github.com/kyverno/kyverno/api/kyverno/v2" - kyvernov2beta1 "github.com/kyverno/kyverno/api/kyverno/v2beta1" - "github.com/kyverno/kyverno/pkg/utils/match" + engineutils "github.com/kyverno/kyverno/pkg/engine/utils" + authenticationv1 "k8s.io/api/authentication/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ) -func matchResource(resource unstructured.Unstructured, rule kyvernov1.Rule) bool { - if rule.MatchResources.All != nil || rule.MatchResources.Any != nil { - matched := match.CheckMatchesResources( - resource, - kyvernov2beta1.MatchResources{ - Any: rule.MatchResources.Any, - All: rule.MatchResources.All, - }, - make(map[string]string), - kyvernov2.RequestInfo{}, - resource.GroupVersionKind(), - "", - ) - if matched != nil { - return false - } +func matchResource(resource unstructured.Unstructured, rule kyvernov1.Rule, namespaceLabels map[string]string, policyNamespace string, operation kyvernov1.AdmissionOperation) bool { + // cannot use admission info from the current request as the user can be different, if the rule matches on old request user info, it should skip + admissionInfo := kyvernov2.RequestInfo{ + Roles: []string{"kyverno:invalidrole"}, + ClusterRoles: []string{"kyverno:invalidrole"}, + AdmissionUserInfo: authenticationv1.UserInfo{ + Username: "kyverno:kyverno-invalid-controller", + UID: "kyverno:invaliduid", + Groups: []string{"kyverno:invalidgroup"}, + }, } - if rule.ExcludeResources != nil { - if rule.ExcludeResources.All != nil || rule.ExcludeResources.Any != nil { - excluded := match.CheckMatchesResources( - resource, - kyvernov2beta1.MatchResources{ - Any: rule.ExcludeResources.Any, - All: rule.ExcludeResources.All, - }, - make(map[string]string), - kyvernov2.RequestInfo{}, - resource.GroupVersionKind(), - "", - ) - if excluded == nil { - return false - } - } - } - return true + + err := engineutils.MatchesResourceDescription( + resource, + rule, + admissionInfo, + namespaceLabels, + policyNamespace, + resource.GroupVersionKind(), + "", + operation, + ) + return err == nil } diff --git a/pkg/engine/handlers/validation/validate_assert.go b/pkg/engine/handlers/validation/validate_assert.go index 511fef8645..50d518028c 100644 --- a/pkg/engine/handlers/validation/validate_assert.go +++ b/pkg/engine/handlers/validation/validate_assert.go @@ -156,7 +156,7 @@ func validateOldObject(ctx context.Context, policyContext engineapi.PolicyContex oldResource := policyContext.OldResource() - if ok := matchResource(oldResource, rule); !ok { + if ok := matchResource(oldResource, rule, policyContext.NamespaceLabels(), policyContext.Policy().GetNamespace(), kyvernov1.Create); !ok { return nil, nil } diff --git a/pkg/engine/handlers/validation/validate_pss.go b/pkg/engine/handlers/validation/validate_pss.go index 4d3f2cee0a..39507d7efe 100644 --- a/pkg/engine/handlers/validation/validate_pss.go +++ b/pkg/engine/handlers/validation/validate_pss.go @@ -181,7 +181,7 @@ func (h validatePssHandler) validateOldObject( oldResource := policyContext.OldResource() emptyResource := unstructured.Unstructured{} - if ok := matchResource(oldResource, rule); !ok { + if ok := matchResource(oldResource, rule, policyContext.NamespaceLabels(), policyContext.Policy().GetNamespace(), kyvernov1.Create); !ok { return nil, nil } if err := policyContext.SetResources(emptyResource, oldResource); err != nil { diff --git a/pkg/engine/handlers/validation/validate_resource.go b/pkg/engine/handlers/validation/validate_resource.go index acffdc2457..037ad53b4a 100644 --- a/pkg/engine/handlers/validation/validate_resource.go +++ b/pkg/engine/handlers/validation/validate_resource.go @@ -163,11 +163,9 @@ func (v *validator) validate(ctx context.Context) *engineapi.RuleResponse { return engineapi.RuleSkip(v.rule.Name, engineapi.Validation, "failed to validate old object", ruleResponse.Properties()) } - if engineutils.IsSameRuleResponse(ruleResponse, priorResp) { - v.log.V(2).Info("warning: skipping the rule evaluation as pre-existing violations are allowed") - if ruleResponse.Status() == engineapi.RuleStatusPass { - return ruleResponse - } + // when an existing resource violates, and the updated resource also violates, then skip + if ruleResponse.Status() == engineapi.RuleStatusFail && priorResp.Status() == engineapi.RuleStatusFail { // + v.log.V(2).Info("warning: skipping the rule evaluation as pre-existing violations are allowed", "ruleResponse", ruleResponse, "priorResp", priorResp) return engineapi.RuleSkip(v.rule.Name, engineapi.Validation, "skipping the rule evaluation as pre-existing violations are allowed", v.rule.ReportProperties) } } @@ -185,7 +183,7 @@ func (v *validator) validateOldObject(ctx context.Context) (*engineapi.RuleRespo oldResource := v.policyContext.OldResource() emptyResource := unstructured.Unstructured{} - if ok := matchResource(oldResource, v.rule); !ok { + if ok := matchResource(oldResource, v.rule, v.policyContext.NamespaceLabels(), v.policyContext.Policy().GetNamespace(), kyvernov1.Create); !ok { return engineapi.RuleSkip(v.rule.Name, engineapi.Validation, "resource not matched", v.rule.ReportProperties), nil }