Skip to content

Commit

Permalink
Bug: message when enforce and multiple namespaces
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 Apr 4, 2023
1 parent b70748b commit aed9a71
Show file tree
Hide file tree
Showing 10 changed files with 452 additions and 66 deletions.
132 changes: 76 additions & 56 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,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(
Expand All @@ -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,
Expand All @@ -1110,28 +1125,75 @@ 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 mergeMessageEnforce || (!handled && !enforce) {
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))
}
}
}

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 +1356,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
11 changes: 10 additions & 1 deletion controllers/configurationpolicy_utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.Info("metadata was nil, add minimum to avoid panic")

metadata = map[string]interface{}{
"name": "",
}
}

return formatMetadata(metadata)
}
Expand Down
Loading

0 comments on commit aed9a71

Please sign in to comment.