skip other checks if operations do not match (#8324)

* skip other checks if operations do not match

Signed-off-by: Jim Bugwadia <jim@nirmata.com>

* copy resource/rule as match seems to mutate for wildcard checks

Signed-off-by: Jim Bugwadia <jim@nirmata.com>

* fix deepcopy

Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>

---------

Signed-off-by: Jim Bugwadia <jim@nirmata.com>
Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
Co-authored-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>

pkg/engine/utils/match.go

@@ -59,8 +59,15 @@ func doesResourceMatchConditionBlock(
 	subresource string,
 	operation kyvernov1.AdmissionOperation,
 ) []error {
-	var errs []error
+	if len(conditionBlock.Operations) > 0 {
+		if !slices.Contains(conditionBlock.Operations, operation) {
+			// if operation does not match, return immediately
+			err := fmt.Errorf("operation does not match")
+			return []error{err}
+		}
+	}
 
+	var errs []error
 	if len(conditionBlock.Kinds) > 0 {
 		// Matching on ephemeralcontainers even when they are not explicitly specified for backward compatibility.
 		if !matchutils.CheckKind(conditionBlock.Kinds, gvk, subresource, true) {
@@ -130,12 +137,6 @@ func doesResourceMatchConditionBlock(
 		}
 	}
 
-	if len(conditionBlock.Operations) > 0 {
-		if !slices.Contains(conditionBlock.Operations, operation) {
-			errs = append(errs, fmt.Errorf("operation does not match"))
-		}
-	}
-
 	var userInfoErrors []error
 	if len(userInfo.Roles) > 0 {
 		if !datautils.SliceContains(userInfo.Roles, admissionInfo.Roles...) {
@@ -165,24 +166,21 @@ func matchSubjects(ruleSubjects []rbacv1.Subject, userInfo authenticationv1.User
 
 // matchesResourceDescription checks if the resource matches resource description of the rule or not
 func MatchesResourceDescription(
-	resourceRef unstructured.Unstructured,
-	ruleRef kyvernov1.Rule,
-	admissionInfoRef kyvernov1beta1.RequestInfo,
+	resource unstructured.Unstructured,
+	rule kyvernov1.Rule,
+	admissionInfo kyvernov1beta1.RequestInfo,
 	namespaceLabels map[string]string,
 	policyNamespace string,
 	gvk schema.GroupVersionKind,
 	subresource string,
 	operation kyvernov1.AdmissionOperation,
 ) error {
-	if resourceRef.Object == nil {
+	if resource.Object == nil {
 		return fmt.Errorf("resource is empty")
 	}
-	rule := ruleRef.DeepCopy()
-	resource := *resourceRef.DeepCopy()
-	admissionInfo := *admissionInfoRef.DeepCopy()
 
 	var reasonsForFailure []error
-	if policyNamespace != "" && policyNamespace != resourceRef.GetNamespace() {
+	if policyNamespace != "" && policyNamespace != resource.GetNamespace() {
 		return fmt.Errorf("policy and resource namespaces mismatch")
 	}
 
@@ -210,32 +208,35 @@ func MatchesResourceDescription(
 		reasonsForFailure = append(reasonsForFailure, matchesResourceDescriptionMatchHelper(rmr, admissionInfo, resource, namespaceLabels, gvk, subresource, operation)...)
 	}
 
-	if len(rule.ExcludeResources.Any) > 0 {
-		// exclude the object if ANY of the criteria match
-		for _, rer := range rule.ExcludeResources.Any {
+	// check exlude conditions only if match succeeds
+	if len(reasonsForFailure) == 0 {
+		if len(rule.ExcludeResources.Any) > 0 {
+			// exclude the object if ANY of the criteria match
+			for _, rer := range rule.ExcludeResources.Any {
+				reasonsForFailure = append(reasonsForFailure, matchesResourceDescriptionExcludeHelper(rer, admissionInfo, resource, namespaceLabels, gvk, subresource, operation)...)
+			}
+		} else if len(rule.ExcludeResources.All) > 0 {
+			// exclude the object if ALL the criteria match
+			excludedByAll := true
+			for _, rer := range rule.ExcludeResources.All {
+				// we got no errors inplying a resource did NOT exclude it
+				// "matchesResourceDescriptionExcludeHelper" returns errors if resource is excluded by a filter
+				if len(matchesResourceDescriptionExcludeHelper(rer, admissionInfo, resource, namespaceLabels, gvk, subresource, operation)) == 0 {
+					excludedByAll = false
+					break
+				}
+			}
+			if excludedByAll {
+				reasonsForFailure = append(reasonsForFailure, fmt.Errorf("resource excluded since the combination of all criteria exclude it"))
+			}
+		} else {
+			rer := kyvernov1.ResourceFilter{UserInfo: rule.ExcludeResources.UserInfo, ResourceDescription: rule.ExcludeResources.ResourceDescription}
 			reasonsForFailure = append(reasonsForFailure, matchesResourceDescriptionExcludeHelper(rer, admissionInfo, resource, namespaceLabels, gvk, subresource, operation)...)
 		}
-	} else if len(rule.ExcludeResources.All) > 0 {
-		// exclude the object if ALL the criteria match
-		excludedByAll := true
-		for _, rer := range rule.ExcludeResources.All {
-			// we got no errors inplying a resource did NOT exclude it
-			// "matchesResourceDescriptionExcludeHelper" returns errors if resource is excluded by a filter
-			if len(matchesResourceDescriptionExcludeHelper(rer, admissionInfo, resource, namespaceLabels, gvk, subresource, operation)) == 0 {
-				excludedByAll = false
-				break
-			}
-		}
-		if excludedByAll {
-			reasonsForFailure = append(reasonsForFailure, fmt.Errorf("resource excluded since the combination of all criteria exclude it"))
-		}
-	} else {
-		rer := kyvernov1.ResourceFilter{UserInfo: rule.ExcludeResources.UserInfo, ResourceDescription: rule.ExcludeResources.ResourceDescription}
-		reasonsForFailure = append(reasonsForFailure, matchesResourceDescriptionExcludeHelper(rer, admissionInfo, resource, namespaceLabels, gvk, subresource, operation)...)
 	}
 
 	// creating final error
-	errorMessage := fmt.Sprintf("rule %s not matched:", ruleRef.Name)
+	errorMessage := fmt.Sprintf("rule %s not matched:", rule.Name)
 	for i, reasonForFailure := range reasonsForFailure {
 		if reasonForFailure != nil {
 			errorMessage += "\n " + fmt.Sprint(i+1) + ". " + reasonForFailure.Error()

pkg/engine/wildcards/wildcards.go

@@ -10,9 +10,11 @@ import (
 
 // ReplaceInSelector replaces label selector keys and values containing
 // wildcard characters with matching keys and values from the resource labels.
-func ReplaceInSelector(labelSelector *metav1.LabelSelector, resourceLabels map[string]string) {
+func ReplaceInSelector(labelSelector *metav1.LabelSelector, resourceLabels map[string]string) *metav1.LabelSelector {
+	labelSelector = labelSelector.DeepCopy()
 	result := replaceWildcardsInMapKeyValues(labelSelector.MatchLabels, resourceLabels)
 	labelSelector.MatchLabels = result
+	return labelSelector
 }
 
 // replaceWildcardsInMap will expand  the "key" and "value" and will replace wildcard characters

pkg/utils/match/labels.go

@@ -11,8 +11,7 @@ func CheckSelector(expected *metav1.LabelSelector, actual map[string]string) (bo
 	if expected == nil {
 		return false, nil
 	}
-
-	wildcards.ReplaceInSelector(expected, actual)
+	expected = wildcards.ReplaceInSelector(expected, actual)
 	selector, err := metav1.LabelSelectorAsSelector(expected)
 	if err != nil {
 		logging.Error(err, "failed to build label selector")