Skip to content

Commit

Permalink
remove log and add comment
Browse files Browse the repository at this point in the history 
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>
  • Loading branch information
@yiraeChristineKim
yiraeChristineKim committed Mar 31, 2023
1 parent b70748b commit a054076
Show file tree
Hide file tree
Showing 8 changed files with 319 additions and 72 deletions.
143 changes: 80 additions & 63 deletions controllers/configurationpolicy_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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,
)
Expand All @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
12 changes: 6 additions & 6 deletions controllers/configurationpolicy_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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)

Expand All @@ -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)

Expand All @@ -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)

Expand All @@ -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)

Expand All @@ -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)
}
Expand Down
83 changes: 83 additions & 0 deletions test/e2e/case5_multi_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
})
})
8 changes: 5 additions & 3 deletions test/e2e/case8_status_check_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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")
})
})
})
Loading

0 comments on commit a054076

Please sign in to comment.