Add property when compliant dry run overrides policy mismatch

ref: https://issues.redhat.com/browse/ACM-14577

Adds DryRunNoOpOverride boolean in the relatedObject properties. Indicates when configuration policies
detect desired vs actual object mismatches, but dry run updates produce no
changes due to webhooks or mutating admission controllers modifying apiserver requests.

Signed-off-by: Janelle Law <[email protected]>

Author: jan-law

api/v1/configurationpolicy_types.go

@@ -417,6 +417,11 @@ type ObjectProperties struct {
 	// Diff stores the difference between the `objectDefinition` in the policy and the object on the
 	// cluster.
 	Diff string `json:"diff,omitempty"`
+
+	// DryRunNoOpOverride indicates that a difference was detected between the policy and the
+	// object, but a dry run update reported no changes. The dry run result overrides the policy
+	// assessment, meaning the object is considered compliant despite the initial mismatch.
+	DryRunNoOpOverride *bool `json:"dryRunNoOpOverride,omitempty"`
 }
 
 // RelatedObject contains the details of an object matched by the policy.

api/v1/zz_generated.deepcopy.go

@@ -2393,8 +2393,9 @@ func (r *ConfigurationPolicyReconciler) handleSingleObj(
 
 		created := false
 		uid := string(obj.existingObj.GetUID())
+		dryRunNoOpOverride := false
 
-		if evaluated, compliant, cachedMsg := r.alreadyEvaluated(obj.policy, obj.existingObj); evaluated {
+		if evaluated, compliant, cachedMsg, dryRunNoOp := r.alreadyEvaluated(obj.policy, obj.existingObj); evaluated {
 			log.V(1).Info("Skipping object comparison since the resourceVersion hasn't changed")
 
 			for _, relatedObj := range obj.policy.Status.RelatedObjects {
@@ -2408,6 +2409,7 @@ func (r *ConfigurationPolicyReconciler) handleSingleObj(
 
 			throwSpecViolation = !compliant
 			msg = cachedMsg
+			dryRunNoOpOverride = dryRunNoOp
 		} else {
 			throwSpecViolation, msg, diff, triedUpdate, updatedObj = r.checkAndUpdateResource(
 				obj, objectT, remediation,
@@ -2417,6 +2419,10 @@ func (r *ConfigurationPolicyReconciler) handleSingleObj(
 				uid = string(updatedObj.GetUID())
 				created = true
 			}
+
+			if triedUpdate && diff == "" {
+				dryRunNoOpOverride = true
+			}
 		}
 
 		if triedUpdate && !strings.Contains(msg, "Error validating the object") {
@@ -2435,9 +2441,10 @@ func (r *ConfigurationPolicyReconciler) handleSingleObj(
 			}
 
 			objectProperties = &policyv1.ObjectProperties{
-				CreatedByPolicy: &created,
-				UID:             uid,
-				Diff:            diff,
+				CreatedByPolicy:          &created,
+				UID:                      uid,
+				Diff:                     diff,
+				DryRunNoOpOverrodePolicy: &dryRunNoOpOverrodePolicy,
 			}
 
 			result.events = append(result.events, objectTmplEvalEvent{false, resultReason, resultMsg})
@@ -2451,9 +2458,10 @@ func (r *ConfigurationPolicyReconciler) handleSingleObj(
 				}
 
 				objectProperties = &policyv1.ObjectProperties{
-					CreatedByPolicy: &created,
-					UID:             uid,
-					Diff:            diff,
+					CreatedByPolicy:          &created,
+					UID:                      uid,
+					Diff:                     diff,
+					DryRunNoOpOverrodePolicy: &dryRunNoOpOverrodePolicy,
 				}
 			} else {
 				result.events = append(result.events, objectTmplEvalEvent{true, reasonWantFoundExists, ""})
@@ -3149,9 +3157,10 @@ func handleSingleKey(
 }
 
 type cachedEvaluationResult struct {
-	resourceVersion string
-	compliant       bool
-	msg             string
+	resourceVersion    string
+	compliant          bool
+	msg                string
+	dryRunNoOpOverride bool
 }
 
 // checkAndUpdateResource checks each individual key of a resource and passes it to handleKeys to see if it
@@ -3227,7 +3236,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
 			diff = handleDiff(log, recordDiff, existingObjectCopy, mergedObjCopy, r.FullDiffs)
 		}
 
-		r.setEvaluatedObject(obj.policy, obj.existingObj, !throwSpecViolation, "")
+		r.setEvaluatedObject(obj.policy, obj.existingObj, !throwSpecViolation, "", false)
 
 		return throwSpecViolation, "", diff, updateNeeded, updatedObj
 	}
@@ -3270,7 +3279,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
 
 			// If the user specifies an unknown or invalid field, it comes back as a bad request.
 			if k8serrors.IsBadRequest(err) {
-				r.setEvaluatedObject(obj.policy, obj.existingObj, false, message)
+				r.setEvaluatedObject(obj.policy, obj.existingObj, false, message, false)
 			}
 
 			return true, message, "", updateNeeded, nil
@@ -3297,7 +3306,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
 					`you may set spec["object-templates"][].recreateOption to recreate the object`
 			}
 
-			r.setEvaluatedObject(obj.policy, obj.existingObj, false, message)
+			r.setEvaluatedObject(obj.policy, obj.existingObj, false, message, false)
 
 			return true, message, diff, false, nil
 		}
@@ -3322,7 +3331,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
 				diff = handleDiff(log, recordDiff, existingObjectCopy, mergedObjCopy, r.FullDiffs)
 			}
 
-			r.setEvaluatedObject(obj.policy, obj.existingObj, !throwSpecViolation, "")
+			r.setEvaluatedObject(obj.policy, obj.existingObj, !throwSpecViolation, "", true)
 
 			return throwSpecViolation, "", diff, updateNeeded, updatedObj
 		}
@@ -3332,7 +3341,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
 
 	// The object would have been updated, so if it's inform, return as noncompliant.
 	if isInform {
-		r.setEvaluatedObject(obj.policy, obj.existingObj, false, "")
+		r.setEvaluatedObject(obj.policy, obj.existingObj, false, "", false)
 
 		return true, "", diff, false, nil
 	}
@@ -3412,7 +3421,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
 	}
 
 	if !statusMismatch {
-		r.setEvaluatedObject(obj.policy, updatedObj, true, message)
+		r.setEvaluatedObject(obj.policy, updatedObj, true, message, false)
 	}
 
 	return throwSpecViolation, "", diff, updateNeeded, updatedObj
@@ -3575,7 +3584,11 @@ func removeFieldsForComparison(obj *unstructured.Unstructured) {
 // setEvaluatedObject updates the cache to indicate that the ConfigurationPolicy has evaluated this
 // object at its current resourceVersion.
 func (r *ConfigurationPolicyReconciler) setEvaluatedObject(
-	policy *policyv1.ConfigurationPolicy, currentObject *unstructured.Unstructured, compliant bool, msg string,
+	policy *policyv1.ConfigurationPolicy,
+	currentObject *unstructured.Unstructured,
+	compliant bool,
+	msg string,
+	dryRunOverride bool,
 ) {
 	policyMap := &sync.Map{}
 
@@ -3587,9 +3600,10 @@ func (r *ConfigurationPolicyReconciler) setEvaluatedObject(
 	policyMap.Store(
 		currentObject.GetUID(),
 		cachedEvaluationResult{
-			resourceVersion: currentObject.GetResourceVersion(),
-			compliant:       compliant,
-			msg:             msg,
+			resourceVersion:    currentObject.GetResourceVersion(),
+			compliant:          compliant,
+			msg:                msg,
+			dryRunNoOpOverride: dryRunOverride,
 		},
 	)
 }
@@ -3598,29 +3612,29 @@ func (r *ConfigurationPolicyReconciler) setEvaluatedObject(
 // resourceVersion.
 func (r *ConfigurationPolicyReconciler) alreadyEvaluated(
 	policy *policyv1.ConfigurationPolicy, currentObject *unstructured.Unstructured,
-) (evaluated bool, compliant bool, msg string) {
+) (evaluated bool, compliant bool, msg string, dryRunNoOpOverrode bool) {
 	if policy == nil || currentObject == nil {
-		return false, false, ""
+		return false, false, "", false
 	}
 
 	loadedPolicyMap, loaded := r.processedPolicyCache.Load(policy.GetUID())
 	if !loaded {
-		return false, false, ""
+		return false, false, "", false
 	}
 
 	policyMap := loadedPolicyMap.(*sync.Map)
 
 	result, loaded := policyMap.Load(currentObject.GetUID())
 	if !loaded {
-		return false, false, ""
+		return false, false, "", false
 	}
 
 	resultTyped := result.(cachedEvaluationResult)
 
 	alreadyEvaluated := resultTyped.resourceVersion != "" &&
 		resultTyped.resourceVersion == currentObject.GetResourceVersion()
 
-	return alreadyEvaluated, resultTyped.compliant, resultTyped.msg
+	return alreadyEvaluated, resultTyped.compliant, resultTyped.msg, resultTyped.dryRunNoOpOverride
 }
 
 func getUpdateErrorMsg(err error, kind string, name string) string {

controllers/configurationpolicy_controller.go

@@ -1566,3 +1566,162 @@ func TestShouldHandleSingleKeyFalse(t *testing.T) {
 		assert.False(t, skip)
 	}
 }
+
+// TestAlreadyEvaluated tests the core caching functionality for evaluated objects
+func TestAlreadyEvaluated(t *testing.T) {
+	// Create a test ConfigurationPolicy
+	policy := &policyv1.ConfigurationPolicy{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-policy",
+			Namespace: "default",
+			UID:       types.UID("test-policy-uid"),
+		},
+	}
+
+	// Create a test unstructured object
+	testObj := &unstructured.Unstructured{
+		Object: map[string]interface{}{
+			"apiVersion": "v1",
+			"kind":       "ConfigMap",
+			"metadata": map[string]interface{}{
+				"name":            "test-configmap",
+				"namespace":       "default",
+				"uid":             "test-object-uid",
+				"resourceVersion": "123",
+			},
+		},
+	}
+
+	// Create a reconciler with an empty cache
+	r := &ConfigurationPolicyReconciler{
+		processedPolicyCache: sync.Map{},
+	}
+
+	t.Run("Test caching dryRunNoOpOverride=true", func(t *testing.T) {
+		// Call setEvaluatedObject with dryRunNoOpOverride=true
+		r.setEvaluatedObject(policy, testObj, true, "test message", true)
+
+		// Verify the object was cached correctly
+		evaluated, compliant, msg, dryRunNoOpOverride := r.alreadyEvaluated(policy, testObj)
+
+		assert.True(t, evaluated, "Object should be marked as evaluated")
+		assert.True(t, compliant, "Object should be marked as compliant")
+		assert.Equal(t, "test message", msg, "Message should match")
+		assert.True(t, dryRunNoOpOverride, "dryRunNoOpOverride should be true")
+	})
+
+	t.Run("Test caching dryRunNoOpOverride=false", func(t *testing.T) {
+		// Create a new policy and object to avoid cache interference
+		policy2 := &policyv1.ConfigurationPolicy{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "test-policy-2",
+				Namespace: "default",
+				UID:       types.UID("test-policy-uid-2"),
+			},
+		}
+
+		testObj2 := &unstructured.Unstructured{
+			Object: map[string]interface{}{
+				"apiVersion": "v1",
+				"kind":       "ConfigMap",
+				"metadata": map[string]interface{}{
+					"name":            "test-configmap-2",
+					"namespace":       "default",
+					"uid":             "test-object-uid-2",
+					"resourceVersion": "124",
+				},
+			},
+		}
+
+		// Call setEvaluatedObject with dryRunNoOpOverride=false
+		r.setEvaluatedObject(policy2, testObj2, false, "error message", false)
+
+		// Verify the object was cached correctly
+		evaluated, compliant, msg, dryRunNoOpOverride := r.alreadyEvaluated(policy2, testObj2)
+
+		assert.True(t, evaluated, "Object should be marked as evaluated")
+		assert.False(t, compliant, "Object should be marked as non-compliant")
+		assert.Equal(t, "error message", msg, "Message should match")
+		assert.False(t, dryRunNoOpOverride, "dryRunNoOpOverride should be false")
+	})
+
+	t.Run("Test not cached object", func(t *testing.T) {
+		// Create a completely new policy and object
+		policy3 := &policyv1.ConfigurationPolicy{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "test-policy-3",
+				Namespace: "default",
+				UID:       types.UID("test-policy-uid-3"),
+			},
+		}
+
+		testObj3 := &unstructured.Unstructured{
+			Object: map[string]interface{}{
+				"apiVersion": "v1",
+				"kind":       "ConfigMap",
+				"metadata": map[string]interface{}{
+					"name":            "test-configmap-3",
+					"namespace":       "default",
+					"uid":             "test-object-uid-3",
+					"resourceVersion": "125",
+				},
+			},
+		}
+
+		// Call alreadyEvaluated without caching first
+		evaluated, compliant, msg, dryRunNoOpOverride := r.alreadyEvaluated(policy3, testObj3)
+
+		assert.False(t, evaluated, "Object should not be marked as evaluated")
+		assert.False(t, compliant, "Object should be marked as non-compliant")
+		assert.Empty(t, msg, "Message should be empty")
+		assert.False(t, dryRunNoOpOverride, "dryRunNoOpOverride should be false")
+	})
+
+	t.Run("Test resource version change invalidates cache", func(t *testing.T) {
+		// Use the same policy but change the resource version
+		testObjNewVersion := &unstructured.Unstructured{
+			Object: map[string]interface{}{
+				"apiVersion": "v1",
+				"kind":       "ConfigMap",
+				"metadata": map[string]interface{}{
+					"name":            "test-configmap",
+					"namespace":       "default",
+					"uid":             "test-object-uid",
+					"resourceVersion": "456", // Different resource version
+				},
+			},
+		}
+
+		// Call alreadyEvaluated - should return false due to different resource version
+		// but the cached values from the previous evaluation should still be returned
+		evaluated, compliant, msg, dryRunNoOpOverride := r.alreadyEvaluated(policy, testObjNewVersion)
+
+		assert.False(t, evaluated, "Object should not be marked as evaluated due to different resource version")
+		assert.True(t, compliant, "Should return the cached compliance state from previous evaluation")
+		assert.Equal(t, "test message", msg, "Should return the cached message from previous evaluation")
+		assert.True(t, dryRunNoOpOverride, "Should return the cached dryRunNoOpOverride from previous evaluation")
+	})
+
+	t.Run("Test nil inputs", func(t *testing.T) {
+		// Test with nil policy
+		evaluated, compliant, msg, dryRunNoOpOverride := r.alreadyEvaluated(nil, testObj)
+		assert.False(t, evaluated)
+		assert.False(t, compliant)
+		assert.Empty(t, msg)
+		assert.False(t, dryRunNoOpOverride)
+
+		// Test with nil object
+		evaluated, compliant, msg, dryRunNoOpOverride = r.alreadyEvaluated(policy, nil)
+		assert.False(t, evaluated)
+		assert.False(t, compliant)
+		assert.Empty(t, msg)
+		assert.False(t, dryRunNoOpOverride)
+
+		// Test with both nil
+		evaluated, compliant, msg, dryRunNoOpOverride = r.alreadyEvaluated(nil, nil)
+		assert.False(t, evaluated)
+		assert.False(t, compliant)
+		assert.Empty(t, msg)
+		assert.False(t, dryRunNoOpOverride)
+	})
+}

controllers/configurationpolicy_controller_test.go

@@ -462,6 +462,12 @@ spec:
                             Diff stores the difference between the `objectDefinition` in the policy and the object on the
                             cluster.
                           type: string
+                        dryRunNoOpOverride:
+                          description: |-
+                            DryRunNoOpOverride indicates that a difference was detected between the policy and the
+                            object, but a dry run update reported no changes. The dry run result overrides the policy
+                            assessment, meaning the object is considered compliant despite the initial mismatch.
+                          type: boolean
                         uid:
                           description: |-
                             UID stores the object UID to help track object ownership for deletion when pruning is