From a05407675511a470b5e5070891158311c7ae397e Mon Sep 17 00:00:00 2001 From: Yi Rae Kim Date: Wed, 29 Mar 2023 14:56:27 -0400 Subject: [PATCH] remove log and add comment ConfigurationPolicy message for enforce omits objects when multiple namespaces are specified When set to inform, the full list of objects is shown. But when set to enforce (the first two messages displayed), only the last object message is shown. In multiple namespaces, the message should be merged. Should be fixed like this example: pod [pod1] in namespace test1 found; [pod1] in namespace test2 found as specified, therefore this Object template is compliant Ref: https://issues.redhat.com/browse/ACM-2604 Signed-off-by: Yi Rae Kim --- controllers/configurationpolicy_controller.go | 143 ++++++++++-------- .../configurationpolicy_controller_test.go | 12 +- test/e2e/case5_multi_test.go | 83 ++++++++++ test/e2e/case8_status_check_test.go | 8 +- .../case5_multi_namespace_enforce.yaml | 24 +++ .../case5_multi_namespace_inform.yaml | 24 +++ .../case5_multi_obj_template_enforce.yaml | 49 ++++++ test/utils/utils.go | 48 ++++++ 8 files changed, 319 insertions(+), 72 deletions(-) create mode 100644 test/resources/case5_multi/case5_multi_namespace_enforce.yaml create mode 100644 test/resources/case5_multi/case5_multi_namespace_inform.yaml create mode 100644 test/resources/case5_multi/case5_multi_obj_template_enforce.yaml diff --git a/controllers/configurationpolicy_controller.go b/controllers/configurationpolicy_controller.go index 9fd9b8de..599ab83b 100644 --- a/controllers/configurationpolicy_controller.go +++ b/controllers/configurationpolicy_controller.go @@ -1041,9 +1041,9 @@ func (r *ConfigurationPolicyReconciler) handleObjectTemplates(plc policyv1.Confi nonCompliantObjects := map[string]map[string]interface{}{} compliantObjects := map[string]map[string]interface{}{} enforce := strings.EqualFold(string(plc.Spec.RemediationAction), string(policyv1.Enforce)) - kind := "" + kind := templateObjs[indx].kind objShouldExist := !strings.EqualFold(string(objectT.ComplianceType), string(policyv1.MustNotHave)) - + mergeMessageEnforce := false // If the object does not have a namespace specified, use the previously retrieved namespaces // from the NamespaceSelector. If no namespaces are found/specified, use the value from the // object so that the objectTemplate is processed: @@ -1057,19 +1057,15 @@ func (r *ConfigurationPolicyReconciler) handleObjectTemplates(plc policyv1.Confi relevantNamespaces = []string{templateObjs[indx].namespace} } + if enforce && len(relevantNamespaces) > 1 { + mergeMessageEnforce = true + } + numCompliant := 0 numNonCompliant := 0 handled := false - // iterate through all namespaces the configurationpolicy is set on for _, ns := range relevantNamespaces { - log.Info( - "Handling the object template for the relevant namespace", - "namespace", ns, - "desiredName", templateObjs[indx].name, - "index", indx, - ) - names, compliant, reason, objKind, related, statusUpdateNeeded := r.handleObjects( objectT, ns, templateObjs[indx], indx, &plc, ) @@ -1079,20 +1075,32 @@ func (r *ConfigurationPolicyReconciler) handleObjectTemplates(plc policyv1.Confi } if objKind != "" { + // object template enforced, objKind is empty kind = objKind } + if mergeMessageEnforce { + names = append([]string{}, templateObjs[indx].name) + } + + // object template enforced, already handled in handleObjects if names == nil { - // object template enforced, already handled in handleObjects handled = true } else { enforce = false + } + + // violations for enforce configurationpolicies are already handled in handleObjects, + // so we only need to generate a violation if the remediationAction is set to inform + // Or multiple namespaces and enforce use this + if mergeMessageEnforce || (!handled && !enforce) { if !compliant { if len(names) == 0 { numNonCompliant++ } else { numNonCompliant += len(names) } + nonCompliantObjects[ns] = map[string]interface{}{ "names": names, "reason": reason, @@ -1110,28 +1118,79 @@ func (r *ConfigurationPolicyReconciler) handleObjectTemplates(plc policyv1.Confi relatedObjects = updateRelatedObjectsStatus(relatedObjects, object) } } - // violations for enforce configurationpolicies are already handled in handleObjects, - // so we only need to generate a violation if the remediationAction is set to inform - if !handled && !enforce { - objData := map[string]interface{}{ - "indx": indx, - "kind": kind, - "desiredName": templateObjs[indx].name, - "namespaced": templateObjs[indx].isNamespaced, - } - statusUpdateNeeded := createInformStatus( + objData := map[string]interface{}{ + "indx": indx, + "kind": kind, + "desiredName": templateObjs[indx].name, + "namespaced": templateObjs[indx].isNamespaced, + } + if !handled && !enforce { + statusUpdateNeeded := createMergedStatus( objShouldExist, numCompliant, numNonCompliant, compliantObjects, nonCompliantObjects, &plc, objData, ) if statusUpdateNeeded { parentStatusUpdateNeeded = true } } + + // When enforce and multiple namespaces, it creates integrated messages, + if mergeMessageEnforce { + parentStatusUpdateNeeded = createMergedStatus( + objShouldExist, numCompliant, numNonCompliant, compliantObjects, nonCompliantObjects, &plc, objData, + ) + + r.Recorder.Event(&plc, eventNormal, fmt.Sprintf(plcFmtStr, plc.GetName()), + convertPolicyStatusToString(&plc)) + } } r.checkRelatedAndUpdate(plc, relatedObjects, oldRelated, parentStatusUpdateNeeded) } +// createMergedStatus updates the status field for a configurationpolicy with remediationAction=inform +// based on how many compliant/noncompliant objects are found when processing the templates in the configurationpolicy +// Or multiple namespaces and enforce use this +func createMergedStatus( + objShouldExist bool, + numCompliant, + numNonCompliant int, + compliantObjects, + nonCompliantObjects map[string]map[string]interface{}, + plc *policyv1.ConfigurationPolicy, + objData map[string]interface{}, +) bool { + //nolint:forcetypeassert + desiredName := objData["desiredName"].(string) + //nolint:forcetypeassert + indx := objData["indx"].(int) + //nolint:forcetypeassert + kind := objData["kind"].(string) + //nolint:forcetypeassert + namespaced := objData["namespaced"].(bool) + + if kind == "" { + return false + } + + var compObjs map[string]map[string]interface{} + var compliant bool + + if numNonCompliant > 0 { + compliant = false + compObjs = nonCompliantObjects + } else if objShouldExist && numCompliant == 0 { + // Special case: No resources found is NonCompliant + compliant = false + compObjs = nonCompliantObjects + } else { + compliant = true + compObjs = compliantObjects + } + + return createStatus(desiredName, kind, compObjs, namespaced, plc, indx, compliant, objShouldExist) +} + // checkRelatedAndUpdate checks the related objects field and triggers an update on the ConfigurationPolicy func (r *ConfigurationPolicyReconciler) checkRelatedAndUpdate( plc policyv1.ConfigurationPolicy, related, oldRelated []policyv1.RelatedObject, sendEvent bool, @@ -1294,48 +1353,6 @@ func addConditionToStatus( return updateNeeded } -// createInformStatus updates the status field for a configurationpolicy with remediationAction=inform -// based on how many compliant/noncompliant objects are found when processing the templates in the configurationpolicy -func createInformStatus( - objShouldExist bool, - numCompliant, - numNonCompliant int, - compliantObjects, - nonCompliantObjects map[string]map[string]interface{}, - plc *policyv1.ConfigurationPolicy, - objData map[string]interface{}, -) bool { - //nolint:forcetypeassert - desiredName := objData["desiredName"].(string) - //nolint:forcetypeassert - indx := objData["indx"].(int) - //nolint:forcetypeassert - kind := objData["kind"].(string) - //nolint:forcetypeassert - namespaced := objData["namespaced"].(bool) - - if kind == "" { - return false - } - - var compObjs map[string]map[string]interface{} - var compliant bool - - if numNonCompliant > 0 { - compliant = false - compObjs = nonCompliantObjects - } else if objShouldExist && numCompliant == 0 { - // Special case: No resources found is NonCompliant - compliant = false - compObjs = nonCompliantObjects - } else { - compliant = true - compObjs = compliantObjects - } - - return createStatus(desiredName, kind, compObjs, namespaced, plc, indx, compliant, objShouldExist) -} - // handleObjects controls the processing of each individual object template within a configurationpolicy func (r *ConfigurationPolicyReconciler) handleObjects( objectT *policyv1.ObjectTemplate, diff --git a/controllers/configurationpolicy_controller_test.go b/controllers/configurationpolicy_controller_test.go index d58f5bf6..8fcb4585 100644 --- a/controllers/configurationpolicy_controller_test.go +++ b/controllers/configurationpolicy_controller_test.go @@ -435,7 +435,7 @@ func TestSortRelatedObjectsAndUpdate(t *testing.T) { assert.True(t, relatedList[0].Object.Metadata.Name == "bar") } -func TestCreateInformStatus(t *testing.T) { +func TestCreateMergedStatus(t *testing.T) { policy := &policyv1.ConfigurationPolicy{ ObjectMeta: metav1.ObjectMeta{ Name: "foo", @@ -473,7 +473,7 @@ func TestCreateInformStatus(t *testing.T) { } // Test 1 NonCompliant resource - createInformStatus(!mustNotHave, numCompliant, numNonCompliant, + createMergedStatus(!mustNotHave, numCompliant, numNonCompliant, compliantObjects, nonCompliantObjects, policy, objData) assert.True(t, policy.Status.CompliancyDetails[0].ComplianceState == policyv1.NonCompliant) @@ -484,7 +484,7 @@ func TestCreateInformStatus(t *testing.T) { numNonCompliant = 2 // Test 2 NonCompliant resources - createInformStatus(!mustNotHave, numCompliant, numNonCompliant, + createMergedStatus(!mustNotHave, numCompliant, numNonCompliant, compliantObjects, nonCompliantObjects, policy, objData) assert.True(t, policy.Status.CompliancyDetails[0].ComplianceState == policyv1.NonCompliant) @@ -493,7 +493,7 @@ func TestCreateInformStatus(t *testing.T) { // Test 0 resources numNonCompliant = 0 - createInformStatus(!mustNotHave, numCompliant, numNonCompliant, + createMergedStatus(!mustNotHave, numCompliant, numNonCompliant, compliantObjects, nonCompliantObjects, policy, objData) assert.True(t, policy.Status.CompliancyDetails[0].ComplianceState == policyv1.NonCompliant) @@ -509,7 +509,7 @@ func TestCreateInformStatus(t *testing.T) { numNonCompliant = 1 // Test 1 compliant and 1 noncompliant resource NOTE: This use case is the new behavior change! - createInformStatus(!mustNotHave, numCompliant, numNonCompliant, + createMergedStatus(!mustNotHave, numCompliant, numNonCompliant, compliantObjects, nonCompliantObjects, policy, objData) assert.True(t, policy.Status.CompliancyDetails[0].ComplianceState == policyv1.NonCompliant) @@ -523,7 +523,7 @@ func TestCreateInformStatus(t *testing.T) { delete(nonCompliantObjects, "test2") // Test 2 compliant resources - createInformStatus(!mustNotHave, numCompliant, numNonCompliant, + createMergedStatus(!mustNotHave, numCompliant, numNonCompliant, compliantObjects, nonCompliantObjects, policy, objData) assert.True(t, policy.Status.CompliancyDetails[0].ComplianceState == policyv1.Compliant) } diff --git a/test/e2e/case5_multi_test.go b/test/e2e/case5_multi_test.go index 7fcaa9e6..0aa7e58f 100644 --- a/test/e2e/case5_multi_test.go +++ b/test/e2e/case5_multi_test.go @@ -87,4 +87,87 @@ var _ = Describe("Test multiple obj template handling", func() { deleteConfigPolicies(policies) }) }) + + Describe("Check messages when it is multiple namesapces and multiple obj-templates", Ordered, func() { + const ( + case5MultiNamespace1 string = "n1" + case5MultiNamespace2 string = "n2" + case5MultiNamespace3 string = "n3" + case5MultiNSConfigPolicyName string = "policy-multi-namespace-enforce" + case5MultiNSInformConfigPolicyName string = "policy-multi-namespace-inform" + case5MultiObjNSConfigPolicyName string = "policy-pod-multi-obj-temp-enforce" + case5InformYaml string = "../resources/case5_multi/case5_multi_namespace_inform.yaml" + case5EnforceYaml string = "../resources/case5_multi/case5_multi_namespace_enforce.yaml" + case5MultiObjTmpYaml string = "../resources/case5_multi/case5_multi_obj_template_enforce.yaml" + ) + BeforeAll(func() { + nss := []string{ + case5MultiNamespace1, + case5MultiNamespace2, + case5MultiNamespace3, + } + + for _, ns := range nss { + utils.Kubectl("create", "ns", ns) + } + }) + It("Should show merged Noncompliant messages when it is multiple namespaces and inform", func() { + expectedMsg := "pods not found: [case5-multi-namespace-inform-pod] " + + "in namespace n1 missing; [case5-multi-namespace-inform-pod] " + + "in namespace n2 missing; [case5-multi-namespace-inform-pod] " + + "in namespace n3 missing" + utils.Kubectl("apply", "-f", case5InformYaml) + utils.DoConfigPolicyMessageTest(clientManagedDynamic, gvrConfigPolicy, testNamespace, + case5MultiNSInformConfigPolicyName, 0, defaultTimeoutSeconds, expectedMsg) + }) + It("Should show merged messages when it is multiple namespaces", func() { + expectedMsg := "Pod [case5-multi-namespace-enforce-pod] in namespace n1 found; " + + "[case5-multi-namespace-enforce-pod] in namespace n2 found; " + + "[case5-multi-namespace-enforce-pod] in namespace n3 found " + + "as specified, therefore this Object template is compliant" + utils.Kubectl("apply", "-f", case5EnforceYaml) + utils.DoConfigPolicyMessageTest(clientManagedDynamic, gvrConfigPolicy, testNamespace, + case5MultiNSConfigPolicyName, 0, defaultTimeoutSeconds, expectedMsg) + }) + It("Should show 3 merged messages when it is multiple namespaces and multiple obj-template", func() { + firstMsg := "Pod [case5-multi-obj-temp-pod-11] in namespace n1 found; " + + "[case5-multi-obj-temp-pod-11] in namespace n2 found; " + + "[case5-multi-obj-temp-pod-11] in namespace n3 found " + + "as specified, therefore this Object template is compliant" + secondMsg := "Pod [case5-multi-obj-temp-pod-22] in namespace n1 found; " + + "[case5-multi-obj-temp-pod-22] in namespace n2 found; " + + "[case5-multi-obj-temp-pod-22] in namespace n3 found " + + "as specified, therefore this Object template is compliant" + thirdMsg := "Pod [case5-multi-obj-temp-pod-33] in namespace n1 found; " + + "[case5-multi-obj-temp-pod-33] in namespace n2 found; " + + "[case5-multi-obj-temp-pod-33] in namespace n3 found " + + "as specified, therefore this Object template is compliant" + utils.Kubectl("apply", "-f", case5MultiObjTmpYaml) + utils.DoConfigPolicyMessageTest(clientManagedDynamic, gvrConfigPolicy, testNamespace, + case5MultiObjNSConfigPolicyName, 0, defaultTimeoutSeconds, firstMsg) + utils.DoConfigPolicyMessageTest(clientManagedDynamic, gvrConfigPolicy, testNamespace, + case5MultiObjNSConfigPolicyName, 1, defaultTimeoutSeconds, secondMsg) + utils.DoConfigPolicyMessageTest(clientManagedDynamic, gvrConfigPolicy, testNamespace, + case5MultiObjNSConfigPolicyName, 2, defaultTimeoutSeconds, thirdMsg) + }) + cleanup := func() { + policies := []string{ + case5MultiNSConfigPolicyName, + case5MultiNSInformConfigPolicyName, + case5MultiObjNSConfigPolicyName, + } + + deleteConfigPolicies(policies) + nss := []string{ + case5MultiNamespace1, + case5MultiNamespace2, + case5MultiNamespace3, + } + + for _, ns := range nss { + utils.Kubectl("delete", "ns", ns, "--ignore-not-found") + } + } + AfterAll(cleanup) + }) }) diff --git a/test/e2e/case8_status_check_test.go b/test/e2e/case8_status_check_test.go index 1e41fd99..50f95d54 100644 --- a/test/e2e/case8_status_check_test.go +++ b/test/e2e/case8_status_check_test.go @@ -25,7 +25,7 @@ const ( ) var _ = Describe("Test pod obj template handling", func() { - Describe("Create a policy on managed cluster in ns:"+testNamespace, func() { + Describe("Create a policy on managed cluster in ns:"+testNamespace, Ordered, func() { It("should create a policy properly on the managed cluster", func() { By("Creating " + case8ConfigPolicyNamePod + " on managed") utils.Kubectl("apply", "-f", case8PolicyYamlPod, "-n", testNamespace) @@ -89,7 +89,7 @@ var _ = Describe("Test pod obj template handling", func() { deleteConfigPolicies(policies) }) }) - Describe("Create a policy with status on managed cluster in ns:"+testNamespace, func() { + Describe("Create a policy with status on managed cluster in ns:"+testNamespace, Ordered, func() { It("should create a policy properly on the managed cluster", func() { By("Creating " + case8ConfigPolicyStatusPod + " on managed") utils.Kubectl("apply", "-f", case8PolicyYamlBadPod, "-n", testNamespace) @@ -140,11 +140,13 @@ var _ = Describe("Test pod obj template handling", func() { return utils.GetComplianceState(managedPlc) }, defaultTimeoutSeconds, 1).Should(Equal("Compliant")) }) - It("Cleans up", func() { + AfterEach(func() { policies := []string{ case8ConfigPolicyStatusPod, } deleteConfigPolicies(policies) + + utils.Kubectl("delete", "pod", "nginx-badpod-e2e-8", "-n", "default", "--ignore-not-found") }) }) }) diff --git a/test/resources/case5_multi/case5_multi_namespace_enforce.yaml b/test/resources/case5_multi/case5_multi_namespace_enforce.yaml new file mode 100644 index 00000000..ccc35be9 --- /dev/null +++ b/test/resources/case5_multi/case5_multi_namespace_enforce.yaml @@ -0,0 +1,24 @@ +apiVersion: policy.open-cluster-management.io/v1 +kind: ConfigurationPolicy +metadata: + name: policy-multi-namespace-enforce + namespace: managed +spec: + remediationAction: enforce + pruneObjectBehavior: DeleteAll + namespaceSelector: + exclude: ["kube-*"] + include: ["n1","n2","n3"] + object-templates: + - complianceType: musthave + objectDefinition: + apiVersion: v1 + kind: Pod + metadata: + name: case5-multi-namespace-enforce-pod + spec: + containers: + - image: nginx:1.7.9 + name: nginx + ports: + - containerPort: 80 diff --git a/test/resources/case5_multi/case5_multi_namespace_inform.yaml b/test/resources/case5_multi/case5_multi_namespace_inform.yaml new file mode 100644 index 00000000..c538a76f --- /dev/null +++ b/test/resources/case5_multi/case5_multi_namespace_inform.yaml @@ -0,0 +1,24 @@ +apiVersion: policy.open-cluster-management.io/v1 +kind: ConfigurationPolicy +metadata: + name: policy-multi-namespace-inform + namespace: managed +spec: + remediationAction: inform + pruneObjectBehavior: DeleteAll + namespaceSelector: + exclude: ["kube-*"] + include: ["n1","n2","n3"] + object-templates: + - complianceType: musthave + objectDefinition: + apiVersion: v1 + kind: Pod + metadata: + name: case5-multi-namespace-inform-pod + spec: + containers: + - image: nginx:1.7.9 + name: nginx + ports: + - containerPort: 80 diff --git a/test/resources/case5_multi/case5_multi_obj_template_enforce.yaml b/test/resources/case5_multi/case5_multi_obj_template_enforce.yaml new file mode 100644 index 00000000..0fa1a9d2 --- /dev/null +++ b/test/resources/case5_multi/case5_multi_obj_template_enforce.yaml @@ -0,0 +1,49 @@ +apiVersion: policy.open-cluster-management.io/v1 +kind: ConfigurationPolicy +metadata: + name: policy-pod-multi-obj-temp-enforce + namespace: managed +spec: + remediationAction: enforce + pruneObjectBehavior: DeleteAll + namespaceSelector: + exclude: ["kube-*"] + include: ["n1","n2","n3"] + object-templates: + - complianceType: musthave + objectDefinition: + apiVersion: v1 + kind: Pod + metadata: + name: case5-multi-obj-temp-pod-11 + spec: + containers: + - image: nginx:1.7.9 + name: nginx + ports: + - containerPort: 80 + - complianceType: musthave + objectDefinition: + apiVersion: v1 + kind: Pod + metadata: + name: case5-multi-obj-temp-pod-22 + spec: + containers: + - image: nginx:1.7.9 + name: nginx + ports: + - containerPort: 80 + - complianceType: musthave + objectDefinition: + apiVersion: v1 + kind: Pod + metadata: + name: case5-multi-obj-temp-pod-33 + spec: + containers: + - image: nginx:1.7.9 + name: nginx + ports: + - containerPort: 80 + diff --git a/test/utils/utils.go b/test/utils/utils.go index 1e4de09d..2663c8da 100644 --- a/test/utils/utils.go +++ b/test/utils/utils.go @@ -292,3 +292,51 @@ func GetMetrics(metricPatterns ...string) []string { return values } + +func getConfigPolicyStatusMessages(clientHubDynamic dynamic.Interface, + gvrConfigPolicy schema.GroupVersionResource, namespace string, policyName string, templateIdx int, +) (string, bool, error) { + empty := "" + policyInterface := clientHubDynamic.Resource(gvrConfigPolicy).Namespace(namespace) + + configPolicy, err := policyInterface.Get(context.TODO(), policyName, metav1.GetOptions{}) + if err != nil { + return empty, false, fmt.Errorf("error in getting policy") + } + + details, found, err := unstructured.NestedSlice(configPolicy.Object, "status", "compliancyDetails") + if !found || err != nil || len(details) <= templateIdx { + return empty, false, fmt.Errorf("error in getting status") + } + + templateDetails, ok := details[templateIdx].(map[string]interface{}) + if !ok { + return empty, false, fmt.Errorf("error in getting detail") + } + + conditions, ok, _ := unstructured.NestedSlice(templateDetails, "conditions") + + if !ok { + return empty, false, fmt.Errorf("error conditions not found") + } + + condition := conditions[0].(map[string]interface{}) + + message, ok, err := unstructured.NestedString(condition, "message") + + return message, ok, err +} + +func DoConfigPolicyMessageTest(clientHubDynamic dynamic.Interface, + gvrConfigPolicy schema.GroupVersionResource, namespace string, policyName string, + templateIdx int, timeout int, expectedMsg string, +) { + Eventually(func(g Gomega) string { + message, _, err := getConfigPolicyStatusMessages(clientHubDynamic, + gvrConfigPolicy, namespace, policyName, templateIdx) + + g.Expect(err).Should(BeNil()) + + return message + }, timeout, 1).Should(Equal(expectedMsg)) +}