Revert "Fix the conditions history in compliancyDetails"

This reverts commit 0234403 since conditions aren't meant to contain a history. They are supposed to contain just the current state. See: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#typical-status-properties The ConfigurationPolicy status events combined all conditions and led to the wrong status message being stored in the Policy.
open-cluster-management-io · Jan 23, 2023 · 20d473c · 20d473c
1 parent 03101f6
commit 20d473c
Show file tree

Hide file tree

Showing 4 changed files with 29 additions and 106 deletions.
diff --git a/controllers/configurationpolicy_controller.go b/controllers/configurationpolicy_controller.go
@@ -49,10 +49,9 @@ import (
 )
 
 const (
-	ControllerName                 string = "configuration-policy-controller"
-	CRDName                        string = "configurationpolicies.policy.open-cluster-management.io"
-	complianceStatusConditionLimit        = 10
-	pruneObjectFinalizer           string = "policy.open-cluster-management.io/delete-related-objects"
+	ControllerName       string = "configuration-policy-controller"
+	CRDName              string = "configurationpolicies.policy.open-cluster-management.io"
+	pruneObjectFinalizer string = "policy.open-cluster-management.io/delete-related-objects"
 )
 
 var log = ctrl.Log.WithName(ControllerName)
@@ -1258,7 +1257,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)
+		conditions := AppendCondition(plc.Status.CompliancyDetails[index].Conditions, cond, "", false)
 		plc.Status.CompliancyDetails[index].Conditions = conditions
 		updateNeeded = true
 	}
@@ -1860,7 +1859,8 @@ 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)
+				conditions := AppendCondition(policy.Status.CompliancyDetails[index].Conditions,
+					cond, gvk.GroupKind().Kind, false)
 				policy.Status.CompliancyDetails[index].Conditions = conditions
 				updateNeeded = true
 			}
@@ -2572,20 +2572,27 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
 
 // AppendCondition check and appends conditions to the policy status
 func AppendCondition(
-	conditions []policyv1.Condition, newCond *policyv1.Condition,
+	conditions []policyv1.Condition, newCond *policyv1.Condition, resourceType string, resolved ...bool,
 ) (conditionsRes []policyv1.Condition) {
-	if len(conditions) != 0 && IsSimilarToLastCondition(conditions[len(conditions)-1], *newCond) {
-		conditions[len(conditions)-1] = *newCond
+	defer recoverFlow()
 
-		return conditions
-	}
+	lastIndex := len(conditions)
+	if lastIndex > 0 {
+		oldCond := conditions[lastIndex-1]
+		if IsSimilarToLastCondition(oldCond, *newCond) {
+			conditions[lastIndex-1] = *newCond
 
-	conditions = append(conditions, *newCond)
+			return conditions
+		}
+	} else {
+		// first condition => trigger event
+		conditions = append(conditions, *newCond)
 
-	if len(conditions) > complianceStatusConditionLimit {
-		conditions = conditions[1:]
+		return conditions
 	}
 
+	conditions[lastIndex-1] = *newCond
+
 	return conditions
 }
 
@@ -2800,3 +2807,10 @@ func (r *ConfigurationPolicyReconciler) manageDeploymentFinalizer(shouldBeSet bo
 
 	return r.Update(context.TODO(), &deployment)
 }
+
+func recoverFlow() {
+	if r := recover(); r != nil {
+		// V(-2) is the error level
+		log.V(-2).Info("ALERT!!!! -> recovered from ", "recover", r)
+	}
+}
diff --git a/controllers/configurationpolicy_controller_test.go b/controllers/configurationpolicy_controller_test.go
@@ -701,69 +701,3 @@ 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),
-		)
-	}
-}
diff --git a/test/e2e/case1_pod_handling_test.go b/test/e2e/case1_pod_handling_test.go
@@ -6,7 +6,6 @@ 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"
 )
@@ -48,35 +47,12 @@ 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)

diff --git a/test/utils/utils.go b/test/utils/utils.go
@@ -207,9 +207,8 @@ 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 conditions[len(conditions)-1].(map[string]interface{})["message"]
+		return detail.(map[string]interface{})["conditions"].([]interface{})[0].(map[string]interface{})["message"]
 	}
 
 	return nil