fix(reports-controller): Allow UPDATE operation in validation

Pass operations list instead of one to check for resources that match policy's condition block. Also, decouple operation list for resource validation from the policy context's operation. Fixes kyverno#9562 Signed-off-by: Bhargav Ravuri <[email protected]>
infracloudio · Mar 20, 2024 · 1affc43 · 1affc43
1 parent ad62014
commit 1affc43
Show file tree

Hide file tree

Showing 9 changed files with 844 additions and 103 deletions.
diff --git a/pkg/controllers/report/utils/scanner.go b/pkg/controllers/report/utils/scanner.go
@@ -105,7 +105,9 @@ func (s *scanner) validateResource(ctx context.Context, resource unstructured.Un
 		WithNewResource(resource).
 		WithPolicy(policy).
 		WithNamespaceLabels(nsLabels)
-	response := s.engine.Validate(ctx, policyCtx)
+	response := s.engine.Validate(ctx, policyCtx,
+		[]kyvernov1.AdmissionOperation{kyvernov1.Create, kyvernov1.Update}..., // Pass operations that are allowed
+	)
 	return &response, nil
 }
 

diff --git a/pkg/engine/api/engine.go b/pkg/engine/api/engine.go
@@ -19,6 +19,7 @@ type Engine interface {
 	Validate(
 		ctx context.Context,
 		policyContext PolicyContext,
+		allowedOperations ...kyvernov1.AdmissionOperation,
 	) EngineResponse
 
 	// Mutate performs mutation. Overlay first and then mutation patches

diff --git a/pkg/engine/background.go b/pkg/engine/background.go
@@ -87,10 +87,10 @@ func (e *engine) filterRule(
 	policy := policyContext.Policy()
 	gvk, subresource := policyContext.ResourceKind()
 
-	if err := engineutils.MatchesResourceDescription(newResource, rule, admissionInfo, namespaceLabels, policy.GetNamespace(), gvk, subresource, policyContext.Operation()); err != nil {
+	if err := engineutils.MatchesResourceDescription(newResource, rule, admissionInfo, namespaceLabels, policy.GetNamespace(), gvk, subresource, []kyvernov1.AdmissionOperation{policyContext.Operation()}); err != nil {
 		if ruleType == engineapi.Generation {
 			// if the oldResource matched, return "false" to delete GR for it
-			if err = engineutils.MatchesResourceDescription(oldResource, rule, admissionInfo, namespaceLabels, policy.GetNamespace(), gvk, subresource, policyContext.Operation()); err == nil {
+			if err = engineutils.MatchesResourceDescription(oldResource, rule, admissionInfo, namespaceLabels, policy.GetNamespace(), gvk, subresource, []kyvernov1.AdmissionOperation{policyContext.Operation()}); err == nil {
 				return engineapi.RuleFail(rule.Name, ruleType, "")
 			}
 		}

diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go
@@ -84,12 +84,13 @@ func NewEngine(
 func (e *engine) Validate(
 	ctx context.Context,
 	policyContext engineapi.PolicyContext,
+	allowedOperations ...kyvernov1.AdmissionOperation,
 ) engineapi.EngineResponse {
 	startTime := time.Now()
 	response := engineapi.NewEngineResponseFromPolicyContext(policyContext)
 	logger := internal.LoggerWithPolicyContext(logging.WithName("engine.validate"), policyContext)
 	if internal.MatchPolicyContext(logger, e.client, policyContext, e.configuration) {
-		policyResponse := e.validate(ctx, logger, policyContext)
+		policyResponse := e.validate(ctx, logger, policyContext, allowedOperations)
 		response = response.WithPolicyResponse(policyResponse)
 	}
 	response = response.WithStats(engineapi.NewExecutionStats(startTime, time.Now()))
@@ -188,6 +189,7 @@ func (e *engine) matches(
 	rule kyvernov1.Rule,
 	policyContext engineapi.PolicyContext,
 	resource unstructured.Unstructured,
+	allowedOperations []kyvernov1.AdmissionOperation,
 ) error {
 	if policyContext.AdmissionOperation() {
 		request := policyContext.AdmissionInfo()
@@ -204,7 +206,7 @@ func (e *engine) matches(
 		policyContext.Policy().GetNamespace(),
 		gvk,
 		subresource,
-		policyContext.Operation(),
+		allowedOperations,
 	)
 	if err == nil {
 		return nil
@@ -219,7 +221,7 @@ func (e *engine) matches(
 			policyContext.Policy().GetNamespace(),
 			gvk,
 			subresource,
-			policyContext.Operation(),
+			allowedOperations,
 		)
 		if err == nil {
 			return nil
@@ -236,14 +238,15 @@ func (e *engine) invokeRuleHandler(
 	resource unstructured.Unstructured,
 	rule kyvernov1.Rule,
 	ruleType engineapi.RuleType,
+	allowedOperations ...kyvernov1.AdmissionOperation,
 ) (unstructured.Unstructured, []engineapi.RuleResponse) {
 	return tracing.ChildSpan2(
 		ctx,
 		"pkg/engine",
 		fmt.Sprintf("RULE %s", rule.Name),
 		func(ctx context.Context, span trace.Span) (patchedResource unstructured.Unstructured, results []engineapi.RuleResponse) {
 			// check if resource and rule match
-			if err := e.matches(rule, policyContext, resource); err != nil {
+			if err := e.matches(rule, policyContext, resource, allowedOperations); err != nil {
 				logger.V(4).Info("rule not matched", "reason", err.Error())
 				return resource, nil
 			}

diff --git a/pkg/engine/utils/match.go b/pkg/engine/utils/match.go
@@ -57,13 +57,18 @@ func doesResourceMatchConditionBlock(
 	namespaceLabels map[string]string,
 	gvk schema.GroupVersionKind,
 	subresource string,
-	operation kyvernov1.AdmissionOperation,
+	allowedOperations []kyvernov1.AdmissionOperation,
 ) []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}
+		// Check if all operations in condition block are in the allowed operations list
+		if !slices.ContainsFunc(allowedOperations, func(op kyvernov1.AdmissionOperation) bool {
+			return slices.Contains(conditionBlock.Operations, op)
+		}) {
+			// If none of the input operations match the operations from condition block, return immediately
+			return []error{
+				fmt.Errorf("one or more operations from the %v don't match any from the allowed operations list %v",
+					allowedOperations, conditionBlock.Operations),
+			}
 		}
 	}
 
@@ -173,7 +178,7 @@ func MatchesResourceDescription(
 	policyNamespace string,
 	gvk schema.GroupVersionKind,
 	subresource string,
-	operation kyvernov1.AdmissionOperation,
+	allowedOperations []kyvernov1.AdmissionOperation,
 ) error {
 	if resource.Object == nil {
 		return fmt.Errorf("resource is empty")
@@ -190,7 +195,7 @@ func MatchesResourceDescription(
 		oneMatched := false
 		for _, rmr := range rule.MatchResources.Any {
 			// if there are no errors it means it was a match
-			if len(matchesResourceDescriptionMatchHelper(rmr, admissionInfo, resource, namespaceLabels, gvk, subresource, operation)) == 0 {
+			if len(matchesResourceDescriptionMatchHelper(rmr, admissionInfo, resource, namespaceLabels, gvk, subresource, allowedOperations)) == 0 {
 				oneMatched = true
 				break
 			}
@@ -201,27 +206,27 @@ func MatchesResourceDescription(
 	} else if len(rule.MatchResources.All) > 0 {
 		// include object if ALL of the criteria match
 		for _, rmr := range rule.MatchResources.All {
-			reasonsForFailure = append(reasonsForFailure, matchesResourceDescriptionMatchHelper(rmr, admissionInfo, resource, namespaceLabels, gvk, subresource, operation)...)
+			reasonsForFailure = append(reasonsForFailure, matchesResourceDescriptionMatchHelper(rmr, admissionInfo, resource, namespaceLabels, gvk, subresource, allowedOperations)...)
 		}
 	} else {
 		rmr := kyvernov1.ResourceFilter{UserInfo: rule.MatchResources.UserInfo, ResourceDescription: rule.MatchResources.ResourceDescription}
-		reasonsForFailure = append(reasonsForFailure, matchesResourceDescriptionMatchHelper(rmr, admissionInfo, resource, namespaceLabels, gvk, subresource, operation)...)
+		reasonsForFailure = append(reasonsForFailure, matchesResourceDescriptionMatchHelper(rmr, admissionInfo, resource, namespaceLabels, gvk, subresource, allowedOperations)...)
 	}
 
 	// 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)...)
+				reasonsForFailure = append(reasonsForFailure, matchesResourceDescriptionExcludeHelper(rer, admissionInfo, resource, namespaceLabels, gvk, subresource, allowedOperations)...)
 			}
 		} 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 {
+				if len(matchesResourceDescriptionExcludeHelper(rer, admissionInfo, resource, namespaceLabels, gvk, subresource, allowedOperations)) == 0 {
 					excludedByAll = false
 					break
 				}
@@ -231,7 +236,7 @@ func MatchesResourceDescription(
 			}
 		} else {
 			rer := kyvernov1.ResourceFilter{UserInfo: rule.ExcludeResources.UserInfo, ResourceDescription: rule.ExcludeResources.ResourceDescription}
-			reasonsForFailure = append(reasonsForFailure, matchesResourceDescriptionExcludeHelper(rer, admissionInfo, resource, namespaceLabels, gvk, subresource, operation)...)
+			reasonsForFailure = append(reasonsForFailure, matchesResourceDescriptionExcludeHelper(rer, admissionInfo, resource, namespaceLabels, gvk, subresource, allowedOperations)...)
 		}
 	}
 
@@ -257,7 +262,7 @@ func matchesResourceDescriptionMatchHelper(
 	namespaceLabels map[string]string,
 	gvk schema.GroupVersionKind,
 	subresource string,
-	operation kyvernov1.AdmissionOperation,
+	allowedOperations []kyvernov1.AdmissionOperation,
 ) []error {
 	var errs []error
 	if datautils.DeepEqual(admissionInfo, kyvernov1beta1.RequestInfo{}) {
@@ -267,7 +272,7 @@ func matchesResourceDescriptionMatchHelper(
 	// checking if resource matches the rule
 	if !datautils.DeepEqual(rmr.ResourceDescription, kyvernov1.ResourceDescription{}) ||
 		!datautils.DeepEqual(rmr.UserInfo, kyvernov1.UserInfo{}) {
-		matchErrs := doesResourceMatchConditionBlock(rmr.ResourceDescription, rmr.UserInfo, admissionInfo, resource, namespaceLabels, gvk, subresource, operation)
+		matchErrs := doesResourceMatchConditionBlock(rmr.ResourceDescription, rmr.UserInfo, admissionInfo, resource, namespaceLabels, gvk, subresource, allowedOperations)
 		errs = append(errs, matchErrs...)
 	} else {
 		errs = append(errs, fmt.Errorf("match cannot be empty"))
@@ -282,13 +287,13 @@ func matchesResourceDescriptionExcludeHelper(
 	namespaceLabels map[string]string,
 	gvk schema.GroupVersionKind,
 	subresource string,
-	operation kyvernov1.AdmissionOperation,
+	allowedOperations []kyvernov1.AdmissionOperation,
 ) []error {
 	var errs []error
 	// checking if resource matches the rule
 	if !datautils.DeepEqual(rer.ResourceDescription, kyvernov1.ResourceDescription{}) ||
 		!datautils.DeepEqual(rer.UserInfo, kyvernov1.UserInfo{}) {
-		excludeErrs := doesResourceMatchConditionBlock(rer.ResourceDescription, rer.UserInfo, admissionInfo, resource, namespaceLabels, gvk, subresource, operation)
+		excludeErrs := doesResourceMatchConditionBlock(rer.ResourceDescription, rer.UserInfo, admissionInfo, resource, namespaceLabels, gvk, subresource, allowedOperations)
 		// it was a match so we want to exclude it
 		if len(excludeErrs) == 0 {
 			errs = append(errs, fmt.Errorf("resource excluded since one of the criteria excluded it"))