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.

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

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

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)
+ }
+}

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),
- )
- }
-}

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)

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