fix: update match logic for old object validation (kyverno#11427) (ky…

…verno#11443) * fix: update match logic for old object validation * fix: linter * fix: failing test due to user info * fix: debug logs --------- Signed-off-by: Vishal Choudhary <[email protected]> Co-authored-by: Vishal Choudhary <[email protected]> Co-authored-by: shuting <[email protected]>
realshuting · Oct 22, 2024 · 7de05ec · 7de05ec
1 parent 5defef6
commit 7de05ec
Show file tree

Hide file tree

Showing 4 changed files with 30 additions and 45 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
 	}