diff --git a/controllers/configurationpolicy_controller.go b/controllers/configurationpolicy_controller.go index 5cfd1ee1..55cdd044 100644 --- a/controllers/configurationpolicy_controller.go +++ b/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 } @@ -1715,8 +1716,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 } @@ -2423,26 +2423,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 } @@ -2632,10 +2625,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) - } -} diff --git a/controllers/configurationpolicy_controller_test.go b/controllers/configurationpolicy_controller_test.go index 202c5a34..9b8ebaaf 100644 --- a/controllers/configurationpolicy_controller_test.go +++ b/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), + ) + } +} diff --git a/test/e2e/case1_pod_handling_test.go b/test/e2e/case1_pod_handling_test.go index d741b86a..8b53ed57 100644 --- a/test/e2e/case1_pod_handling_test.go +++ b/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,26 @@ var _ = Describe("Test pod obj template handling", func() { plc := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy, case1ConfigPolicyNameEnforce, testNamespace, true, defaultTimeoutSeconds) Expect(plc).NotTo(BeNil()) + + var managedPlc *unstructured.Unstructured Eventually(func() interface{} { - managedPlc := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy, + managedPlc = utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy, case1ConfigPolicyNameEnforce, testNamespace, true, defaultTimeoutSeconds) return utils.GetComplianceState(managedPlc) }, defaultTimeoutSeconds, 1).Should(Equal("Compliant")) + + By("verifying status.compliancyDetails[].conditions") + compliancyDetails, _, _ := unstructured.NestedSlice(managedPlc.Object, "status", "compliancyDetails") + Expect(compliancyDetails).To(HaveLen(1)) + + conditions, ok := compliancyDetails[0].(map[string]interface{})["conditions"].([]interface{}) + Expect(ok).To(BeTrue()) + Expect(conditions).To(HaveLen(2)) + Expect(conditions[0].(map[string]interface{})["reason"]).To(Equal("K8s creation success")) + Expect(conditions[1].(map[string]interface{})["reason"]).To(Equal("K8s `must have` object already exists")) + + By("verifying that " + case1ConfigPolicyNameInform + "is compliant") Eventually(func() interface{} { informPlc := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy, case1ConfigPolicyNameInform, testNamespace, true, defaultTimeoutSeconds)