Bug: message when enforce and multiple namespaces

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

@@ -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,10 +1057,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(
@@ -1079,20 +1082,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,22 +1125,27 @@ 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 {
+
+ if mergeMessageEnforce || (!handled && !enforce) {
  objData := map[string]interface{}{
  "indx": indx,
  "kind": kind,
  "desiredName": templateObjs[indx].name,
  "namespaced": templateObjs[indx].isNamespaced,
  }
 
- statusUpdateNeeded := createInformStatus(
+ statusUpdateNeeded := createMergedStatus(
  objShouldExist, numCompliant, numNonCompliant, compliantObjects, nonCompliantObjects, &plc, objData,
  )
+
  if statusUpdateNeeded {
  parentStatusUpdateNeeded = true
  }
+
+ if mergeMessageEnforce {
+ r.Recorder.Event(&plc, eventNormal, fmt.Sprintf(plcFmtStr, plc.GetName()),
+ convertPolicyStatusToString(&plc))
+ }
  }
  }
 
@@ -1294,9 +1314,10 @@ func addConditionToStatus(
  return updateNeeded
 }
 
-// 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

@@ -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)
 }

controllers/configurationpolicy_utils.go

@@ -346,7 +346,16 @@ func isAutogenerated(key string) (result bool) {
 func formatTemplate(unstruct unstructured.Unstructured, key string) (obj interface{}) {
  if key == "metadata" {
  //nolint:forcetypeassert
- metadata := unstruct.Object[key].(map[string]interface{})
+ var metadata map[string]interface{}
+ if unstruct.Object[key] != nil {
+ metadata = unstruct.Object[key].(map[string]interface{})
+ } else {
+ log.V(2).Info("metadata was nil, add minimum to avoid panic")
+
+ metadata = map[string]interface{}{
+ "name": "",
+ }
+ }
 
  return formatMetadata(metadata)
  }

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