From 2073d16c6ac48839ba7cf13f775502e42f6eaa2f Mon Sep 17 00:00:00 2001 From: Dale Haiducek <19750917+dhaiducek@users.noreply.github.com> Date: Tue, 16 May 2023 17:08:17 -0400 Subject: [PATCH] Adjust CompliancyDetails array properly - Clear compliancyDetails on policy-wide errors - Reduce the length of the array if the number of object-templates is lower ref: https://issues.redhat.com/browse/ACM-5512 Signed-off-by: Dale Haiducek <19750917+dhaiducek@users.noreply.github.com> (cherry picked from commit 9b8a98f2a3fd3088576885f825a5e4a5251f6597) --- controllers/configurationpolicy_controller.go | 31 ++++++++++--- .../case30_multiline_templatization_test.go | 44 ++++++++++++++++++- .../case30_configmaps.yaml | 4 ++ .../case30_policy.yaml | 2 +- .../case30_unterminated.yaml | 27 ++++++------ 5 files changed, 86 insertions(+), 22 deletions(-) diff --git a/controllers/configurationpolicy_controller.go b/controllers/configurationpolicy_controller.go index d6bf5288..32b2a628 100644 --- a/controllers/configurationpolicy_controller.go +++ b/controllers/configurationpolicy_controller.go @@ -466,7 +466,7 @@ func (r *ConfigurationPolicyReconciler) getObjectTemplateDetails( reason := "namespaceSelector error" msg := fmt.Sprintf( "%s: %s", errMsg, err.Error()) - statusChanged := addConditionToStatus(&plc, 0, false, reason, msg) + statusChanged := addConditionToStatus(&plc, -1, false, reason, msg) if statusChanged { r.Recorder.Event( &plc, @@ -682,7 +682,7 @@ func (r *ConfigurationPolicyReconciler) handleObjectTemplates(plc policyv1.Confi if validationErr != "" { message := validationErr log.Info(message) - statusChanged := addConditionToStatus(&plc, 0, false, "Invalid spec", message) + statusChanged := addConditionToStatus(&plc, -1, false, "Invalid spec", message) if statusChanged { r.Recorder.Event(&plc, eventWarning, @@ -768,7 +768,7 @@ func (r *ConfigurationPolicyReconciler) handleObjectTemplates(plc policyv1.Confi statusChanged := addConditionToStatus( &plc, - 0, + -1, false, reasonCleanupError, "Failed to delete objects: "+strings.Join(failures, ", ")) @@ -808,7 +808,7 @@ func (r *ConfigurationPolicyReconciler) handleObjectTemplates(plc policyv1.Confi reason = "Error processing template" } - statusChanged := addConditionToStatus(&plc, 0, false, reason, msg) + statusChanged := addConditionToStatus(&plc, -1, false, reason, msg) if statusChanged { parentStatusUpdateNeeded = true @@ -1033,6 +1033,12 @@ func (r *ConfigurationPolicyReconciler) handleObjectTemplates(plc policyv1.Confi templateObjs, selectedNamespaces, objTmplStatusChangeNeeded, err = r.getObjectTemplateDetails(plc) + // Set the CompliancyDetails array length accordingly in case the number of + // object-templates was reduced (the status update will handle if it's longer) + if len(templateObjs) < len(plc.Status.CompliancyDetails) { + plc.Status.CompliancyDetails = plc.Status.CompliancyDetails[:len(templateObjs)] + } + if objTmplStatusChangeNeeded { parentStatusUpdateNeeded = true } @@ -1050,7 +1056,7 @@ func (r *ConfigurationPolicyReconciler) handleObjectTemplates(plc policyv1.Confi msg := fmt.Sprintf("%v contains no object templates to check, and thus has no violations", plc.GetName()) - statusUpdateNeeded := addConditionToStatus(&plc, 0, true, reason, msg) + statusUpdateNeeded := addConditionToStatus(&plc, -1, true, reason, msg) if statusUpdateNeeded { eventType := eventNormal @@ -1288,6 +1294,7 @@ func (r *ConfigurationPolicyReconciler) deleteDetachedObj(plc policyv1.Configura } // helper function that appends a condition (violation or compliant) to the status of a configurationpolicy +// Set the index to -1 to signal that the status should be cleared. func addConditionToStatus( plc *policyv1.ConfigurationPolicy, index int, compliant bool, reason string, message string, ) (updateNeeded bool) { @@ -1324,6 +1331,15 @@ func addConditionToStatus( cond.Message += fmt.Sprintf(". %s.", msg) } + // Set a boolean to clear the details array if the index is -1, but set + // the index to zero to determine whether a status update is required + clearStatus := false + + if index == -1 { + clearStatus = true + index = 0 + } + if len(plc.Status.CompliancyDetails) <= index { plc.Status.CompliancyDetails = append(plc.Status.CompliancyDetails, policyv1.TemplateStatus{ ComplianceState: complianceState, @@ -1344,6 +1360,11 @@ func addConditionToStatus( updateNeeded = true } + // Clear the details array if the index provided was -1 + if clearStatus { + plc.Status.CompliancyDetails = plc.Status.CompliancyDetails[0:1] + } + if updateNeeded { log.Info("Will update the policy status") } diff --git a/test/e2e/case30_multiline_templatization_test.go b/test/e2e/case30_multiline_templatization_test.go index b15cf6c6..12095433 100644 --- a/test/e2e/case30_multiline_templatization_test.go +++ b/test/e2e/case30_multiline_templatization_test.go @@ -10,6 +10,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "open-cluster-management.io/config-policy-controller/test/utils" ) @@ -23,7 +24,7 @@ const ( ) const ( - case30Unterminated string = "policy-pod-create-unterminated" + case30Unterminated string = "case30-configpolicy" case30UnterminatedYaml string = "../resources/case30_multiline_templatization/case30_unterminated.yaml" case30WrongArgs string = "policy-pod-create-wrong-args" case30WrongArgsYaml string = "../resources/case30_multiline_templatization/case30_wrong_args.yaml" @@ -110,6 +111,41 @@ var _ = Describe("Test multiline templatization", Ordered, func() { }) }) Describe("Test invalid multiline templates", func() { + It("configmap should be created properly on the managed cluster", func() { + By("Creating config maps on managed") + utils.Kubectl("apply", "-f", case30ConfigMapsYaml, "-n", "default") + for _, cfgMapName := range []string{"30config1", "30config2"} { + cfgmap := utils.GetWithTimeout(clientManagedDynamic, gvrConfigMap, + cfgMapName, "default", true, defaultTimeoutSeconds) + Expect(cfgmap).NotTo(BeNil()) + } + }) + It("should create compliant policy", func() { + By("Creating policy with range template on managed") + utils.Kubectl("apply", "-f", case30RangePolicyYaml, "-n", testNamespace) + plc := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy, + case30RangePolicyName, testNamespace, true, defaultTimeoutSeconds) + Expect(plc).NotTo(BeNil()) + + By("Verifying that the " + case30RangePolicyName + " policy is compliant") + Eventually(func(g Gomega) interface{} { + managedPlc := utils.GetWithTimeout( + clientManagedDynamic, + gvrConfigPolicy, + case30RangePolicyName, + testNamespace, + true, + defaultTimeoutSeconds, + ) + + details, _, err := unstructured.NestedSlice(managedPlc.Object, "status", "compliancyDetails") + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(details).To(HaveLen(2)) + + return utils.GetComplianceState(managedPlc) + }, defaultTimeoutSeconds, 1).Should(Equal("Compliant")) + }) + It("should generate noncompliant for invalid template strings", func() { By("Creating policies on managed") // create policy with unterminated template @@ -117,10 +153,14 @@ var _ = Describe("Test multiline templatization", Ordered, func() { plc := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy, case30Unterminated, testNamespace, true, defaultTimeoutSeconds) Expect(plc).NotTo(BeNil()) - Eventually(func() interface{} { + Eventually(func(g Gomega) interface{} { managedPlc := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy, case30Unterminated, testNamespace, true, defaultTimeoutSeconds) + details, _, err := unstructured.NestedSlice(managedPlc.Object, "status", "compliancyDetails") + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(details).To(HaveLen(1)) + return utils.GetComplianceState(managedPlc) }, defaultTimeoutSeconds, 1).Should(Equal("NonCompliant")) Eventually(func() interface{} { diff --git a/test/resources/case30_multiline_templatization/case30_configmaps.yaml b/test/resources/case30_multiline_templatization/case30_configmaps.yaml index 6e1be6bd..bc7d9deb 100644 --- a/test/resources/case30_multiline_templatization/case30_configmaps.yaml +++ b/test/resources/case30_multiline_templatization/case30_configmaps.yaml @@ -2,6 +2,8 @@ apiVersion: v1 kind: ConfigMap metadata: name: 30config1 + labels: + testcase: "30" data: name: testvalue1 --- @@ -9,5 +11,7 @@ apiVersion: v1 kind: ConfigMap metadata: name: 30config2 + labels: + testcase: "30" data: name: testvalue2 \ No newline at end of file diff --git a/test/resources/case30_multiline_templatization/case30_policy.yaml b/test/resources/case30_multiline_templatization/case30_policy.yaml index be10769d..29eee834 100644 --- a/test/resources/case30_multiline_templatization/case30_policy.yaml +++ b/test/resources/case30_multiline_templatization/case30_policy.yaml @@ -6,7 +6,7 @@ spec: remediationAction: enforce severity: low object-templates-raw: | - {{ range (lookup "v1" "ConfigMap" "default" "").items }} + {{ range (lookup "v1" "ConfigMap" "default" "" "testcase=30").items }} - complianceType: musthave objectDefinition: apiVersion: v1 diff --git a/test/resources/case30_multiline_templatization/case30_unterminated.yaml b/test/resources/case30_multiline_templatization/case30_unterminated.yaml index 113cffe4..09debfec 100644 --- a/test/resources/case30_multiline_templatization/case30_unterminated.yaml +++ b/test/resources/case30_multiline_templatization/case30_unterminated.yaml @@ -1,22 +1,21 @@ apiVersion: policy.open-cluster-management.io/v1 kind: ConfigurationPolicy metadata: - name: policy-pod-create-unterminated + name: case30-configpolicy spec: remediationAction: enforce namespaceSelector: exclude: ["kube-*"] include: ["default"] - object-templates-raw: | - - complianceType: musthave - objectDefinition: - apiVersion: v1 - kind: Pod - metadata: - name: '{{' - spec: - containers: - - image: nginx:1.7.9 - name: nginx - ports: - - containerPort: 80 + object-templates-raw: | + {{ range (lookup "v1" "ConfigMap" "default" "" "testcase=30").items }} + - complianceType: musthave + objectDefinition: + apiVersion: v1 + kind: ConfigMap + metadata: + name: '{{' + namespace: default + data: + extraData: exists! + {{ end }}