Add RelatedObject property when compliant dry run overrides policy mismatch

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"`
+
+	// MatchesAfterDryRun indicates whether the object matches the policy after the dry run update. If true,
+	// there was an initial mismatch between the policy and object, but the dry run update produced
+	// a compliant result.
+	MatchesAfterDryRun bool `json:"matchesAfterDryRun,omitempty"`
 }
 
 // RelatedObject contains the details of an object matched by the policy.

controllers/configurationpolicy_controller.go

@@ -2388,7 +2388,7 @@ func (r *ConfigurationPolicyReconciler) handleSingleObj(
 	if exists && obj.shouldExist {
 		log.V(2).Info("The object already exists. Verifying the object fields match what is desired.")
 
-		var throwSpecViolation, triedUpdate bool
+		var throwSpecViolation, triedUpdate, matchesAfterDryRun bool
 		var msg, diff string
 		var updatedObj *unstructured.Unstructured
 
@@ -2400,8 +2400,9 @@ func (r *ConfigurationPolicyReconciler) handleSingleObj(
 
 			for _, relatedObj := range obj.policy.Status.RelatedObjects {
 				if relatedObj.Properties != nil && relatedObj.Properties.UID == uid {
-					// Retain the diff from the previous evaluation
+					// Retain the properties from the previous evaluation
 					diff = relatedObj.Properties.Diff
+					matchesAfterDryRun = relatedObj.Properties.MatchesAfterDryRun
 
 					break
 				}
@@ -2410,7 +2411,7 @@ func (r *ConfigurationPolicyReconciler) handleSingleObj(
 			throwSpecViolation = !compliant
 			msg = cachedMsg
 		} else {
-			throwSpecViolation, msg, diff, triedUpdate, updatedObj = r.checkAndUpdateResource(
+			throwSpecViolation, msg, diff, triedUpdate, updatedObj, matchesAfterDryRun = r.checkAndUpdateResource(
 				obj, objectT, remediation,
 			)
 
@@ -2435,12 +2436,6 @@ func (r *ConfigurationPolicyReconciler) handleSingleObj(
 				resultReason = reasonWantFoundNoMatch
 			}
 
-			objectProperties = &policyv1.ObjectProperties{
-				CreatedByPolicy: &created,
-				UID:             uid,
-				Diff:            diff,
-			}
-
 			result.events = append(result.events, objectTmplEvalEvent{false, resultReason, resultMsg})
 		} else {
 			// it is a must have and it does exist, so it is compliant
@@ -2450,16 +2445,17 @@ func (r *ConfigurationPolicyReconciler) handleSingleObj(
 				} else {
 					result.events = append(result.events, objectTmplEvalEvent{true, reasonWantFoundExists, ""})
 				}
-
-				objectProperties = &policyv1.ObjectProperties{
-					CreatedByPolicy: &created,
-					UID:             uid,
-					Diff:            diff,
-				}
 			} else {
 				result.events = append(result.events, objectTmplEvalEvent{true, reasonWantFoundExists, ""})
 			}
 		}
+
+		objectProperties = &policyv1.ObjectProperties{
+			CreatedByPolicy:    &created,
+			UID:                uid,
+			Diff:               diff,
+			MatchesAfterDryRun: matchesAfterDryRun,
+		}
 	}
 
 	return
@@ -3162,7 +3158,12 @@ type cachedEvaluationResult struct {
 func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
 	obj singleObject, objectT *policyv1.ObjectTemplate, remediation policyv1.RemediationAction,
 ) (
-	throwSpecViolation bool, message string, diff string, updateNeeded bool, updatedObj *unstructured.Unstructured,
+	throwSpecViolation bool,
+	message string,
+	diff string,
+	updateNeeded bool,
+	updatedObj *unstructured.Unstructured,
+	matchesAfterDryRun bool,
 ) {
 	log := log.WithValues(
 		"policy", obj.policy.Name, "name", obj.name, "namespace", obj.namespace, "resource", obj.scopedGVR.Resource,
@@ -3188,7 +3189,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
 	if obj.existingObj == nil {
 		log.Info("Skipping update: Previous object retrieval from the API server failed")
 
-		return false, "", "", false, nil
+		return false, "", "", false, nil, false
 	}
 
 	var res dynamic.ResourceInterface
@@ -3210,7 +3211,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
 		objectT.MetadataComplianceType,
 	)
 	if errMsg != "" {
-		return true, errMsg, "", true, nil
+		return true, errMsg, "", true, nil, false
 	}
 
 	recordDiff := objectT.RecordDiffWithDefault()
@@ -3230,7 +3231,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
 
 		r.setEvaluatedObject(obj.policy, obj.existingObj, !throwSpecViolation, "")
 
-		return throwSpecViolation, "", diff, updateNeeded, updatedObj
+		return throwSpecViolation, "", diff, updateNeeded, updatedObj, false
 	}
 
 	if updateNeeded {
@@ -3274,7 +3275,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
 				r.setEvaluatedObject(obj.policy, obj.existingObj, false, message)
 			}
 
-			return true, message, "", updateNeeded, nil
+			return true, message, "", updateNeeded, nil, false
 		}
 
 		// If an update is invalid (i.e. modifying Pod spec fields), then return noncompliant since that
@@ -3300,7 +3301,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
 
 			r.setEvaluatedObject(obj.policy, obj.existingObj, false, message)
 
-			return true, message, diff, false, nil
+			return true, message, diff, false, nil, false
 		}
 
 		mergedObjCopy := obj.existingObj.DeepCopy()
@@ -3323,9 +3324,12 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
 				diff = handleDiff(log, recordDiff, existingObjectCopy, mergedObjCopy, r.FullDiffs)
 			}
 
+			// Assume object is compliant by inverting the value of throwSpecViolation
 			r.setEvaluatedObject(obj.policy, obj.existingObj, !throwSpecViolation, "")
 
-			return throwSpecViolation, "", diff, updateNeeded, updatedObj
+			matchesAfterDryRun = true
+
+			return throwSpecViolation, "", diff, updateNeeded, updatedObj, matchesAfterDryRun
 		}
 
 		diff = handleDiff(log, recordDiff, existingObjectCopy, dryRunUpdatedObj, r.FullDiffs)
@@ -3335,7 +3339,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
 	if isInform {
 		r.setEvaluatedObject(obj.policy, obj.existingObj, false, "")
 
-		return true, "", diff, false, nil
+		return true, "", diff, false, nil, matchesAfterDryRun
 	}
 
 	// If it's not inform (i.e. enforce), update the object
@@ -3355,7 +3359,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
 		if err != nil && !k8serrors.IsNotFound(err) {
 			message = fmt.Sprintf(`%s failed to delete when recreating with the error %v`, getMsgPrefix(&obj), err)
 
-			return true, message, "", updateNeeded, nil
+			return true, message, "", updateNeeded, nil, matchesAfterDryRun
 		}
 
 		attempts := 0
@@ -3373,7 +3377,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
 				message = getMsgPrefix(&obj) + " timed out waiting for the object to delete during recreate, " +
 					"will retry on the next policy evaluation"
 
-				return true, message, "", updateNeeded, nil
+				return true, message, "", updateNeeded, nil, matchesAfterDryRun
 			}
 
 			time.Sleep(time.Second)
@@ -3409,14 +3413,14 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
 			message = fmt.Sprintf("%s failed to %s with the error `%v`", getMsgPrefix(&obj), action, err)
 		}
 
-		return true, message, diff, updateNeeded, nil
+		return true, message, diff, updateNeeded, nil, matchesAfterDryRun
 	}
 
 	if !statusMismatch {
 		r.setEvaluatedObject(obj.policy, updatedObj, true, message)
 	}
 
-	return throwSpecViolation, "", diff, updateNeeded, updatedObj
+	return throwSpecViolation, "", diff, updateNeeded, updatedObj, matchesAfterDryRun
 }
 
 func getMsgPrefix(obj *singleObject) string {
@@ -3571,6 +3575,10 @@ func removeFieldsForComparison(obj *unstructured.Unstructured) {
 	)
 	// The generation might actually bump but the API output might be the same.
 	unstructured.RemoveNestedField(obj.Object, "metadata", "generation")
+
+	if len(obj.GetAnnotations()) == 0 {
+		unstructured.RemoveNestedField(obj.Object, "metadata", "annotations")
+	}
 }
 
 // setEvaluatedObject updates the cache to indicate that the ConfigurationPolicy has evaluated this

controllers/configurationpolicy_controller_test.go

@@ -705,6 +705,75 @@ func TestUpdatedRelatedObjects(t *testing.T) {
 	assert.Equal(t, "bar", relatedList[0].Object.Metadata.Name)
 }
 
+func TestAddRelatedObjectProperties(t *testing.T) {
+	compliant := true
+	scopedGVR := depclient.ScopedGVR{
+		GroupVersionResource: policyv1.SchemeBuilder.GroupVersion.WithResource("ConfigurationPolicy"),
+		Namespaced:           true,
+	}
+	namespace := "default"
+	name := "foo"
+	reason := "reason"
+
+	// Create test data for ObjectProperties
+	createdByPolicy := true
+	testUID := "test-uid-12345"
+	testDiff := "test diff content"
+	dryRunMatches := true
+
+	creationInfo := &policyv1.ObjectProperties{
+		CreatedByPolicy:    &createdByPolicy,
+		UID:                testUID,
+		Diff:               testDiff,
+		MatchesAfterDryRun: dryRunMatches,
+	}
+
+	relatedList := addRelatedObjects(
+		compliant, scopedGVR, "ConfigurationPolicy", namespace, []string{name}, reason, creationInfo,
+	)
+	related := relatedList[0]
+
+	// Validate that the properties are correctly set
+	assert.NotNil(t, related.Properties, "Properties should not be nil when creationInfo is provided")
+	assert.NotNil(t, related.Properties.CreatedByPolicy, "CreatedByPolicy should not be nil")
+	assert.Equal(t, createdByPolicy, *related.Properties.CreatedByPolicy)
+	assert.Equal(t, testUID, related.Properties.UID)
+	assert.Equal(t, testDiff, related.Properties.Diff)
+	assert.Equal(t, dryRunMatches, related.Properties.MatchesAfterDryRun)
+
+	// add the same object and make sure the existing one is overwritten
+	reason = "new"
+	compliant = false
+
+	// Create new test data for ObjectProperties with different values
+	newCreatedByPolicy := false
+	newTestDiff := "updated diff content"
+	newDryRunMatches := false
+
+	newCreationInfo := &policyv1.ObjectProperties{
+		CreatedByPolicy:    &newCreatedByPolicy,
+		UID:                testUID,
+		Diff:               newTestDiff,
+		MatchesAfterDryRun: newDryRunMatches,
+	}
+
+	relatedList = addRelatedObjects(
+		compliant, scopedGVR, "ConfigurationPolicy", namespace, []string{name}, reason, newCreationInfo)
+	related = relatedList[0]
+
+	assert.Len(t, relatedList, 1)
+	assert.Equal(t, string(policyv1.NonCompliant), related.Compliant)
+	assert.Equal(t, "new", related.Reason)
+
+	// Validate that the properties were updated with new values
+	assert.NotNil(t, related.Properties, "Properties should not be nil when newCreationInfo is provided")
+	assert.NotNil(t, related.Properties.CreatedByPolicy, "CreatedByPolicy should not be nil")
+	assert.Equal(t, newCreatedByPolicy, *related.Properties.CreatedByPolicy)
+	assert.Equal(t, testUID, related.Properties.UID)
+	assert.Equal(t, newTestDiff, related.Properties.Diff)
+	assert.Equal(t, newDryRunMatches, related.Properties.MatchesAfterDryRun)
+}
+
 func TestCreateStatus(t *testing.T) {
 	testcases := []struct {
 		testName          string

deploy/crds/kustomize_configurationpolicy/policy.open-cluster-management.io_configurationpolicies.yaml

@@ -477,6 +477,12 @@ spec:
                             Diff stores the difference between the `objectDefinition` in the policy and the object on the
                             cluster.
                           type: string
+                        matchesAfterDryRun:
+                          description: |-
+                            MatchesAfterDryRun indicates whether the object matches the policy after the dry run update. If true,
+                            there was an initial mismatch between the policy and object, but the dry run update produced
+                            a compliant result.
+                          type: boolean
                         uid:
                           description: |-
                             UID stores the object UID to help track object ownership for deletion when pruning is

deploy/crds/kustomize_operatorpolicy/kustomization.yaml

@@ -14,3 +14,9 @@ patches:
     version: v1
     kind: CustomResourceDefinition
     name: operatorpolicies.policy.open-cluster-management.io
+- path: remove-matches-after-dry-run.json
+  target:
+    group: apiextensions.k8s.io
+    version: v1
+    kind: CustomResourceDefinition
+    name: operatorpolicies.policy.open-cluster-management.io

deploy/crds/kustomize_operatorpolicy/policy.open-cluster-management.io_operatorpolicies.yaml

@@ -384,6 +384,12 @@ spec:
                             Diff stores the difference between the `objectDefinition` in the policy and the object on the
                             cluster.
                           type: string
+                        matchesAfterDryRun:
+                          description: |-
+                            MatchesAfterDryRun indicates whether the object matches the policy after the dry run update. If true,
+                            there was an initial mismatch between the policy and object, but the dry run update produced
+                            a compliant result.
+                          type: boolean
                         uid:
                           description: |-
                             UID stores the object UID to help track object ownership for deletion when pruning is

deploy/crds/kustomize_operatorpolicy/remove-matches-after-dry-run.json

@@ -0,0 +1,6 @@
+[
+    {
+        "op": "remove",
+        "path": "/spec/versions/0/schema/openAPIV3Schema/properties/status/properties/relatedObjects/items/properties/properties/properties/matchesAfterDryRun"
+    }
+]

deploy/crds/policy.open-cluster-management.io_configurationpolicies.yaml

@@ -484,6 +484,12 @@ spec:
                             Diff stores the difference between the `objectDefinition` in the policy and the object on the
                             cluster.
                           type: string
+                        matchesAfterDryRun:
+                          description: |-
+                            MatchesAfterDryRun indicates whether the object matches the policy after the dry run update. If true,
+                            there was an initial mismatch between the policy and object, but the dry run update produced
+                            a compliant result.
+                          type: boolean
                         uid:
                           description: |-
                             UID stores the object UID to help track object ownership for deletion when pruning is

pkg/dryrun/policy.open-cluster-management.io_configurationpolicies.yaml

@@ -484,6 +484,12 @@ spec:
                             Diff stores the difference between the `objectDefinition` in the policy and the object on the
                             cluster.
                           type: string
+                        matchesAfterDryRun:
+                          description: |-
+                            MatchesAfterDryRun indicates whether the object matches the policy after the dry run update. If true,
+                            there was an initial mismatch between the policy and object, but the dry run update produced
+                            a compliant result.
+                          type: boolean
                         uid:
                           description: |-
                             UID stores the object UID to help track object ownership for deletion when pruning is

test/e2e/case20_delete_objects_test.go

@@ -130,26 +130,36 @@ var _ = Describe("Test Object deletion", Ordered, func() {
 				return properties["uid"]
 			}, defaultTimeoutSeconds, 1).ShouldNot(BeEmpty())
 		})
-		It("should not update status field for inform policies", func() {
+		It("should update status field for inform policies", func() {
 			By("Creating " + case20ConfigPolicyNameInform + " on managed")
 			utils.Kubectl("apply", "-f", case20PolicyYamlInform, "-n", testNamespace)
 			plc := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy,
 				case20ConfigPolicyNameInform, testNamespace, true, defaultTimeoutSeconds)
 			Expect(plc).NotTo(BeNil())
+
+			var managedPlc *unstructured.Unstructured
+
 			Eventually(func(g Gomega) {
-				managedPlc := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy,
+				managedPlc = utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy,
 					case20ConfigPolicyNameInform, testNamespace, true, defaultTimeoutSeconds)
 
 				utils.CheckComplianceStatus(g, managedPlc, "Compliant")
 			}, defaultTimeoutSeconds, 1).Should(Succeed())
-			Eventually(func() interface{} {
-				managedPlc := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy,
-					case20ConfigPolicyNameInform, testNamespace, true, defaultTimeoutSeconds)
-				relatedObj := managedPlc.Object["status"].(map[string]interface{})["relatedObjects"].([]interface{})[0]
-				properties := relatedObj.(map[string]interface{})["properties"]
 
-				return properties
-			}, defaultTimeoutSeconds, 1).Should(BeNil())
+			By("Verifying the related object properties")
+			relatedObjects, _, err := unstructured.NestedSlice(managedPlc.Object, "status", "relatedObjects")
+			Expect(err).ToNot(HaveOccurred())
+			Expect(relatedObjects).To(HaveLen(1))
+
+			relatedObj := relatedObjects[0].(map[string]interface{})
+
+			createdByPolicy, found, _ := unstructured.NestedBool(relatedObj, "properties", "createdByPolicy")
+			Expect(found).To(BeTrue())
+			Expect(createdByPolicy).To(BeFalse())
+
+			uid, found, _ := unstructured.NestedString(relatedObj, "properties", "uid")
+			Expect(found).To(BeTrue())
+			Expect(uid).ToNot(BeEmpty())
 		})
 		AfterAll(func() {
 			policies := []string{