Fix the conditions history in compliancyDetails

The status.compliancyDetails[].conditions array was always limited to
just a single condition due to a bug. This sets the limit to 10 instead.

Relates:
https://issues.redhat.com/browse/ACM-2708

Signed-off-by: mprahl <mprahl@users.noreply.github.com>

controllers/configurationpolicy_controller.go

@@ -48,8 +48,9 @@ import (
 )
 
 const (
-	ControllerName string = "configuration-policy-controller"
-	CRDName        string = "configurationpolicies.policy.open-cluster-management.io"
+	ControllerName                 string = "configuration-policy-controller"
+	CRDName                        string = "configurationpolicies.policy.open-cluster-management.io"
+	complianceStatusConditionLimit        = 10
 )
 
 var log = ctrl.Log.WithName(ControllerName)
@@ -1155,7 +1156,7 @@ func addConditionToStatus(
 
 	// do not add condition unless it does not already appear in the status
 	if !checkMessageSimilarity(plc.Status.CompliancyDetails[index].Conditions, cond) {
-		conditions := AppendCondition(plc.Status.CompliancyDetails[index].Conditions, cond, "", false)
+		conditions := AppendCondition(plc.Status.CompliancyDetails[index].Conditions, cond)
 		plc.Status.CompliancyDetails[index].Conditions = conditions
 		updateNeeded = true
 	}
@@ -1757,8 +1758,7 @@ func (r *ConfigurationPolicyReconciler) getMapping(
 			policy.Status.CompliancyDetails[index].ComplianceState = policyv1.NonCompliant
 
 			if !checkMessageSimilarity(policy.Status.CompliancyDetails[index].Conditions, cond) {
-				conditions := AppendCondition(policy.Status.CompliancyDetails[index].Conditions,
-					cond, gvk.GroupKind().Kind, false)
+				conditions := AppendCondition(policy.Status.CompliancyDetails[index].Conditions, cond)
 				policy.Status.CompliancyDetails[index].Conditions = conditions
 				updateNeeded = true
 			}
@@ -2470,26 +2470,19 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
 
 // AppendCondition check and appends conditions to the policy status
 func AppendCondition(
-	conditions []policyv1.Condition, newCond *policyv1.Condition, resourceType string, resolved ...bool,
+	conditions []policyv1.Condition, newCond *policyv1.Condition,
 ) (conditionsRes []policyv1.Condition) {
-	defer recoverFlow()
-
-	lastIndex := len(conditions)
-	if lastIndex > 0 {
-		oldCond := conditions[lastIndex-1]
-		if IsSimilarToLastCondition(oldCond, *newCond) {
-			conditions[lastIndex-1] = *newCond
-
-			return conditions
-		}
-	} else {
-		// first condition => trigger event
-		conditions = append(conditions, *newCond)
+	if len(conditions) != 0 && IsSimilarToLastCondition(conditions[len(conditions)-1], *newCond) {
+		conditions[len(conditions)-1] = *newCond
 
 		return conditions
 	}
 
-	conditions[lastIndex-1] = *newCond
+	conditions = append(conditions, *newCond)
+
+	if len(conditions) > complianceStatusConditionLimit {
+		conditions = conditions[1:]
+	}
 
 	return conditions
 }
@@ -2679,10 +2672,3 @@ func convertPolicyStatusToString(plc *policyv1.ConfigurationPolicy) (results str
 
 	return result
 }
-
-func recoverFlow() {
-	if r := recover(); r != nil {
-		// V(-2) is the error level
-		log.V(-2).Info("ALERT!!!! -> recovered from ", "recover", r)
-	}
-}

controllers/configurationpolicy_controller_test.go

@@ -701,3 +701,69 @@ func TestShouldEvaluatePolicy(t *testing.T) {
 		)
 	}
 }
+
+func TestAppendCondition(t *testing.T) {
+	conditions := []policyv1.Condition{}
+	newCond1 := &policyv1.Condition{
+		Type:               "violation",
+		LastTransitionTime: metav1.Time{Time: metav1.Now().Add(5 * time.Minute)},
+		Reason:             "K8s `must have` object already exists",
+		Message:            "some message",
+	}
+
+	conditions = AppendCondition(conditions, newCond1)
+	if len(conditions) != 1 {
+		t.Fatalf("Expected a single condition to be appended but the length was %d", len(conditions))
+	}
+
+	lastTransition := metav1.Time{Time: metav1.Now().Add(5 * time.Minute)}
+	newCond2 := &policyv1.Condition{
+		Type:               "violation",
+		LastTransitionTime: lastTransition,
+		Reason:             "K8s `must have` object already exists",
+		Message:            "some message",
+	}
+	conditions = AppendCondition(conditions, newCond2)
+
+	if len(conditions) != 1 {
+		t.Fatalf("Expected the single condition to be updated and not appended but the length was %d", len(conditions))
+	}
+
+	if conditions[0].LastTransitionTime != lastTransition {
+		t.Fatal("The single condition was not updated with the last transition time")
+	}
+
+	newCond3 := &policyv1.Condition{
+		Type:               "violation",
+		LastTransitionTime: lastTransition,
+		Reason:             "K8s creation success",
+		Message:            "some other message",
+	}
+	conditions = AppendCondition(conditions, newCond3)
+
+	if len(conditions) != 2 {
+		t.Fatalf("Expected the new condition to be appended but the length was %d", len(conditions))
+	}
+
+	if conditions[0].Reason != newCond2.Reason || conditions[1].Reason != newCond3.Reason {
+		t.Fatalf("Expected the new condition to be appended the order is incorrect")
+	}
+
+	for i := 0; i < complianceStatusConditionLimit+2; i++ {
+		newCond := &policyv1.Condition{
+			Type:               "violation",
+			LastTransitionTime: lastTransition,
+			Reason:             "K8s creation success",
+			Message:            fmt.Sprintf("some other message%d", i),
+		}
+		conditions = AppendCondition(conditions, newCond)
+	}
+
+	if len(conditions) != complianceStatusConditionLimit {
+		t.Fatalf(
+			"Expected the conditions array to be limited to %d but the length was %d",
+			complianceStatusConditionLimit,
+			len(conditions),
+		)
+	}
+}

test/e2e/case1_pod_handling_test.go

@@ -6,6 +6,7 @@ package e2e
 import (
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 
 	"open-cluster-management.io/config-policy-controller/test/utils"
 )
@@ -47,12 +48,35 @@ var _ = Describe("Test pod obj template handling", func() {
 			plc := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy,
 				case1ConfigPolicyNameEnforce, testNamespace, true, defaultTimeoutSeconds)
 			Expect(plc).NotTo(BeNil())
+
 			Eventually(func() interface{} {
 				managedPlc := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy,
 					case1ConfigPolicyNameEnforce, testNamespace, true, defaultTimeoutSeconds)
 
 				return utils.GetComplianceState(managedPlc)
 			}, defaultTimeoutSeconds, 1).Should(Equal("Compliant"))
+
+			By("verifying status.compliancyDetails[].conditions")
+			Eventually(func(g Gomega) {
+				managedPlc := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy,
+					case1ConfigPolicyNameEnforce, testNamespace, true, defaultTimeoutSeconds)
+
+				compliancyDetails, _, _ := unstructured.NestedSlice(managedPlc.Object, "status", "compliancyDetails")
+				g.Expect(compliancyDetails).To(HaveLen(1))
+
+				conditions, ok := compliancyDetails[0].(map[string]interface{})["conditions"].([]interface{})
+				g.Expect(ok).To(BeTrue())
+				g.Expect(conditions).To(HaveLen(3))
+				g.Expect(conditions[0].(map[string]interface{})["reason"]).To(
+					Equal("K8s does not have a `must have` object"),
+				)
+				g.Expect(conditions[1].(map[string]interface{})["reason"]).To(Equal("K8s creation success"))
+				g.Expect(conditions[2].(map[string]interface{})["reason"]).To(
+					Equal("K8s `must have` object already exists"),
+				)
+			}, defaultTimeoutSeconds, 1).Should(Succeed())
+
+			By("verifying that " + case1ConfigPolicyNameInform + " is compliant")
 			Eventually(func() interface{} {
 				informPlc := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy,
 					case1ConfigPolicyNameInform, testNamespace, true, defaultTimeoutSeconds)

test/utils/utils.go

@@ -207,8 +207,9 @@ func GetComplianceState(managedPlc *unstructured.Unstructured) (result interface
 func GetStatusMessage(managedPlc *unstructured.Unstructured) (result interface{}) {
 	if managedPlc.Object["status"] != nil {
 		detail := managedPlc.Object["status"].(map[string]interface{})["compliancyDetails"].([]interface{})[0]
+		conditions := detail.(map[string]interface{})["conditions"].([]interface{})
 
-		return detail.(map[string]interface{})["conditions"].([]interface{})[0].(map[string]interface{})["message"]
+		return conditions[len(conditions)-1].(map[string]interface{})["message"]
 	}
 
 	return nil