diff --git a/controllers/configurationpolicy_controller.go b/controllers/configurationpolicy_controller.go index ba04f3e3..f2873970 100644 --- 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 index 9b8ebaaf..202c5a34 100644 --- 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 index 13d94924..d741b86a 100644 --- 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 index 1368c9d2..1e4de09d 100644 --- 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