Bug: message when enforce and multiple namespaces (#116)

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 <yikim@redhat.com>

controllers/configurationpolicy_controller.go

@@ -1068,9 +1068,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:
@@ -1084,10 +1084,13 @@ 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(
@@ -1106,20 +1109,33 @@ func (r *ConfigurationPolicyReconciler) handleObjectTemplates(plc policyv1.Confi
 			}
 
 			if objKind != "" {
+				// object template enforced, objKind is empty
 				kind = objKind
 			}
 
+			if mergeMessageEnforce {
+				names = []string{templateObjs[indx].name}
+			}
+
+			// object template enforced, already handled in handleObjects
 			if names == nil {
-				// object template enforced, already handled in handleObjects
 				handled = true
 			} else {
+				// when multiple messages and enforce mode, it doesn't mean remediactionAction is changed
 				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 !handled && !enforce {
 				if !compliant {
 					if len(names) == 0 {
 						numNonCompliant++
 					} else {
 						numNonCompliant += len(names)
 					}
+
 					nonCompliantObjects[ns] = map[string]interface{}{
 						"names":  names,
 						"reason": reason,
@@ -1137,8 +1153,7 @@ 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 := templateIdentifier{
 				index:       indx,
@@ -1147,9 +1162,10 @@ func (r *ConfigurationPolicyReconciler) handleObjectTemplates(plc policyv1.Confi
 				namespaced:  templateObjs[indx].isNamespaced,
 			}
 
-			statusUpdateNeeded := createInformStatus(
+			statusUpdateNeeded := createMergedStatus(
 				objShouldExist, numCompliant, numNonCompliant, compliantObjects, nonCompliantObjects, &plc, objData,
 			)
+
 			if statusUpdateNeeded {
 				parentStatusUpdateNeeded = true
 			}
@@ -1342,9 +1358,10 @@ type templateIdentifier struct {
 	namespaced  bool
 }
 
-// createInformStatus updates the status field for a configurationpolicy with remediationAction=inform
+// 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
-func createInformStatus(
+// Or multiple namespaces and enforce use this
+func createMergedStatus(
 	objShouldExist bool,
 	numCompliant,
 	numNonCompliant int,

controllers/configurationpolicy_controller_test.go

@@ -437,7 +437,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",
@@ -475,7 +475,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)
 
@@ -486,7 +486,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)
 
@@ -495,7 +495,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)
 
@@ -511,7 +511,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)
 
@@ -525,7 +525,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)
 }

test/e2e/case5_multi_test.go

@@ -87,4 +87,134 @@ 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"
+			case5MultiEnforceErrYaml           string = "../resources/case5_multi/" +
+				"case5_multi_namespace_enforce_error.yaml"
+			case5KindMissPlcName     string = "policy-multi-namespace-enforce-kind-missing"
+			case5KindNameMissPlcName string = "policy-multi-namespace-enforce-both-missing"
+			case5NameMissPlcName     string = "policy-multi-namespace-enforce-name-missing"
+		)
+
+		BeforeAll(func() {
+			nss := []string{
+				case5MultiNamespace1,
+				case5MultiNamespace2,
+				case5MultiNamespace3,
+			}
+
+			for _, ns := range nss {
+				utils.Kubectl("create", "ns", ns)
+			}
+		})
+		It("Should handle when kind or name missing without any panic", func() {
+			utils.Kubectl("apply", "-f", case5MultiEnforceErrYaml)
+
+			By("Kind missing")
+			kindErrMsg := "Decoding error, please check your policy file! Aborting handling the " +
+				"object template at index [0] in policy `policy-multi-namespace-enforce-kind-missing` with " +
+				"error = `Object 'Kind' is missing in '{\"apiVersion\":\"v1\",\"metadata\":" +
+				"{\"name\":\"case5-multi-namespace-enforce-kind-missing-pod\"}," +
+				"\"spec\":{\"containers\":[{\"image\":\"nginx:1.7.9\",\"imagePullPolicy\":\"Never\",\"name\":" +
+				"\"nginx\",\"ports\":[{\"containerPort\":80}]}]}}'`"
+			utils.DoConfigPolicyMessageTest(clientManagedDynamic, gvrConfigPolicy, testNamespace,
+				case5KindMissPlcName, 0, defaultTimeoutSeconds, kindErrMsg)
+
+			By("Kind and Name missing")
+			kindNameErrMsg := "Decoding error, please check your policy file! Aborting handling the " +
+				"object template at index [0] in policy `policy-multi-namespace-enforce-both-missing` " +
+				"with error = `Object 'Kind' is missing in " +
+				"'{\"apiVersion\":\"v1\",\"metadata\":{\"name\":\"\"},\"spec\":{\"containers\":" +
+				"[{\"image\":\"nginx:1.7.9\",\"imagePullPolicy\":\"Never\"," +
+				"\"name\":\"nginx\",\"ports\":[{\"containerPort\":80}]}]}}'`"
+			utils.DoConfigPolicyMessageTest(clientManagedDynamic, gvrConfigPolicy, testNamespace,
+				case5KindNameMissPlcName, 0, defaultTimeoutSeconds, kindNameErrMsg)
+
+			By("Name missing")
+			nameErrMsg := "No instances of `pods` found as specified in namespaces: n1, n2, n3"
+			utils.DoConfigPolicyMessageTest(clientManagedDynamic, gvrConfigPolicy, testNamespace,
+				case5NameMissPlcName, 0, defaultTimeoutSeconds, nameErrMsg)
+		})
+		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)
+
+			By("Configuration policy name " + case5NameMissPlcName +
+				" should be compliant after apply " + case5MultiObjTmpYaml)
+
+			Eventually(func() interface{} {
+				plc := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy,
+					case5NameMissPlcName, testNamespace, true, defaultTimeoutSeconds)
+
+				return utils.GetComplianceState(plc)
+			}, defaultTimeoutSeconds, 1).Should(Equal("Compliant"))
+		})
+		cleanup := func() {
+			policies := []string{
+				case5MultiNSConfigPolicyName,
+				case5MultiNSInformConfigPolicyName,
+				case5MultiObjNSConfigPolicyName,
+				case5NameMissPlcName,
+				case5KindNameMissPlcName,
+				case5KindMissPlcName,
+			}
+
+			deleteConfigPolicies(policies)
+			nss := []string{
+				case5MultiNamespace1,
+				case5MultiNamespace2,
+				case5MultiNamespace3,
+			}
+
+			for _, ns := range nss {
+				utils.Kubectl("delete", "ns", ns, "--ignore-not-found")
+			}
+		}
+		AfterAll(cleanup)
+	})
 })

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() {
+		AfterAll(func() {
 			policies := []string{
 				case8ConfigPolicyStatusPod,
 			}
 			deleteConfigPolicies(policies)
+
+			utils.Kubectl("delete", "pod", "nginx-badpod-e2e-8", "-n", "default", "--ignore-not-found")
 		})
 	})
 })

test/resources/case5_multi/case5_multi_namespace_enforce.yaml

@@ -0,0 +1,25 @@
+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
+              imagePullPolicy: Never
+              name: nginx
+              ports:
+                - containerPort: 80