From 076f7fa9f7b1423b8574d9b9eda90f26837e5986 Mon Sep 17 00:00:00 2001 From: mprahl Date: Tue, 10 Jan 2023 14:41:09 -0500 Subject: [PATCH 1/2] 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 (cherry picked from commit 0234403dd4b34b2fd410ed25ef19fb95cadb6732) --- controllers/configurationpolicy_controller.go | 40 ++++------- .../configurationpolicy_controller_test.go | 66 +++++++++++++++++++ test/e2e/case1_pod_handling_test.go | 24 +++++++ test/utils/utils.go | 3 +- 4 files changed, 105 insertions(+), 28 deletions(-) diff --git a/controllers/configurationpolicy_controller.go b/controllers/configurationpolicy_controller.go index 4bb3e12c..83d060ef 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 } @@ -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) - } -} 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..13d94924 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,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) diff --git a/test/utils/utils.go b/test/utils/utils.go index 1e4de09d..1368c9d2 100644 --- a/test/utils/utils.go +++ b/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 From c491d8346c867525f274628e9842604e349b0395 Mon Sep 17 00:00:00 2001 From: mprahl Date: Wed, 11 Jan 2023 08:17:48 -0500 Subject: [PATCH 2/2] Add an eventually in the CRD deletion case Signed-off-by: mprahl (cherry picked from commit aaf185e88d101775c2b5dcdca9a9e3a0f48b83d6) --- test/e2e/case20_delete_objects_test.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/test/e2e/case20_delete_objects_test.go b/test/e2e/case20_delete_objects_test.go index 271a9240..2115a06a 100644 --- a/test/e2e/case20_delete_objects_test.go +++ b/test/e2e/case20_delete_objects_test.go @@ -575,10 +575,12 @@ var _ = Describe("Test objects are not deleted when the CRD is removed", Ordered utils.Kubectl("delete", "-f", case20ConfigPolicyCRDPath) By("Checking that the ConfigurationPolicy is gone") - namespace := clientManagedDynamic.Resource(gvrConfigPolicy).Namespace(testNamespace) - _, err := namespace.Get(context.TODO(), case20ConfigPolicyNameMHPDA, metav1.GetOptions{}) - Expect(err).NotTo(BeNil()) - Expect(err.Error()).To(ContainSubstring("the server could not find the requested resource")) + Eventually(func(g Gomega) { + namespace := clientManagedDynamic.Resource(gvrConfigPolicy).Namespace(testNamespace) + _, err := namespace.Get(context.TODO(), case20ConfigPolicyNameMHPDA, metav1.GetOptions{}) + g.Expect(err).NotTo(BeNil()) + g.Expect(err.Error()).To(ContainSubstring("the server could not find the requested resource")) + }, defaultTimeoutSeconds, 1).Should(Succeed()) By("Recreating the CRD") utils.Kubectl("apply", "-f", case20ConfigPolicyCRDPath)