Test related object properties present for any remediation action

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

Tests scenarios where the policy reports a mismatched object,
but the dry run validation reports a compliant object due to
the apiserver adding or modifying fields in the request body.

This commit also prevents timeouts in case40 by wrapping assertions with
Eventually.

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

Author: jan-law

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
@@ -1544,3 +1613,155 @@ 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 compliant=true", func(t *testing.T) {
+		// Call setEvaluatedObject with compliant=true
+		r.setEvaluatedObject(policy, testObj, true, "test message")
+
+		// Verify the object was cached correctly
+		evaluated, compliant, msg := 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")
+	})
+
+	t.Run("Test caching compliant=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 compliant=false
+		r.setEvaluatedObject(policy2, testObj2, false, "error message")
+
+		// Verify the object was cached correctly
+		evaluated, compliant, msg := 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")
+	})
+
+	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 := 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")
+	})
+
+	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 := 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")
+	})
+
+	t.Run("Test nil inputs", func(t *testing.T) {
+		// Test with nil policy
+		evaluated, compliant, msg := r.alreadyEvaluated(nil, testObj)
+		assert.False(t, evaluated)
+		assert.False(t, compliant)
+		assert.Empty(t, msg)
+
+		// Test with nil object
+		evaluated, compliant, msg = r.alreadyEvaluated(policy, nil)
+		assert.False(t, evaluated)
+		assert.False(t, compliant)
+		assert.Empty(t, msg)
+
+		// Test with both nil
+		evaluated, compliant, msg = r.alreadyEvaluated(nil, nil)
+		assert.False(t, evaluated)
+		assert.False(t, compliant)
+		assert.Empty(t, msg)
+	})
+}

test/e2e/case20_delete_objects_test.go

@@ -130,26 +130,40 @@ 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())
+
+			matchesAfterDryRun, found, _ := unstructured.NestedBool(relatedObj, "properties", "matchesAfterDryRun")
+			Expect(found).To(BeTrue())
+			Expect(matchesAfterDryRun).To(BeTrue())
 		})
 		AfterAll(func() {
 			policies := []string{

test/e2e/case40_recreate_option_test.go

@@ -117,7 +117,7 @@ var _ = Describe("Recreate options", Ordered, func() {
 
 		By("Verifying the ConfigurationPolicy is Compliant")
 		var managedPlc *unstructured.Unstructured
-
+		var propsUID string
 		Eventually(func(g Gomega) {
 			managedPlc = utils.GetWithTimeout(
 				clientManagedDynamic,
@@ -129,37 +129,39 @@ var _ = Describe("Recreate options", Ordered, func() {
 			)
 
 			utils.CheckComplianceStatus(g, managedPlc, "Compliant")
-		}, defaultTimeoutSeconds, 1).Should(Succeed())
 
-		By("Verifying the diff is not set")
-		relatedObjects, _, err := unstructured.NestedSlice(managedPlc.Object, "status", "relatedObjects")
-		Expect(err).ToNot(HaveOccurred())
-		Expect(relatedObjects).To(HaveLen(1))
+			relatedObjects, _, err := unstructured.NestedSlice(managedPlc.Object, "status", "relatedObjects")
+			g.Expect(err).ToNot(HaveOccurred())
+			g.Expect(relatedObjects).To(HaveLen(1))
 
-		relatedObject := relatedObjects[0].(map[string]interface{})
+			relatedObject := relatedObjects[0].(map[string]interface{})
 
-		diff, _, _ := unstructured.NestedString(relatedObject, "properties", "diff")
-		Expect(diff).To(BeEmpty())
+			diff, _, _ := unstructured.NestedString(relatedObject, "properties", "diff")
+			g.Expect(diff).To(BeEmpty())
 
-		propsUID, _, _ := unstructured.NestedString(relatedObject, "properties", "uid")
-		Expect(propsUID).ToNot(BeEmpty())
+			propsUID, _, _ = unstructured.NestedString(relatedObject, "properties", "uid")
+			g.Expect(propsUID).ToNot(BeEmpty())
 
-		createdByPolicy, _, _ := unstructured.NestedBool(relatedObject, "properties", "createdByPolicy")
-		Expect(createdByPolicy).To(BeTrue())
+			createdByPolicy, _, _ := unstructured.NestedBool(relatedObject, "properties", "createdByPolicy")
+			g.Expect(createdByPolicy).To(BeTrue())
 
-		deployment, err = clientManagedDynamic.Resource(gvrDeployment).Namespace("default").Get(
-			ctx, "case40", metav1.GetOptions{},
-		)
-		Expect(err).ToNot(HaveOccurred())
+			deployment, err = clientManagedDynamic.Resource(gvrDeployment).Namespace("default").Get(
+				ctx, "case40", metav1.GetOptions{},
+			)
+			g.Expect(err).ToNot(HaveOccurred())
+		}, defaultTimeoutSeconds, 1).Should(Succeed())
 
 		By("Verifying the Deployment was recreated")
-		Expect(deployment.GetUID()).ToNot(Equal(uid), "Expected a new UID on the Deployment after it got recreated")
-		Expect(propsUID).To(
-			BeEquivalentTo(deployment.GetUID()), "Expect the object properties UID to match the new Deployment",
-		)
+		Eventually(func(g Gomega) {
+			g.Expect(deployment.GetUID()).ToNot(
+				Equal(uid), "Expected a new UID on the Deployment after it got recreated")
+			g.Expect(propsUID).To(
+				BeEquivalentTo(deployment.GetUID()), "Expect the object properties UID to match the new Deployment",
+			)
 
-		selector, _, _ := unstructured.NestedString(deployment.Object, "spec", "selector", "matchLabels", "app")
-		Expect(selector).To(Equal("case40-2"))
+			selector, _, _ := unstructured.NestedString(deployment.Object, "spec", "selector", "matchLabels", "app")
+			g.Expect(selector).To(Equal("case40-2"))
+		}, defaultTimeoutSeconds, 1).Should(Succeed())
 
 		deleteConfigPolicies([]string{"case40"})
 	})