Fire event on object update

Fires an event when an object is updated by the config policy controller, rather than just when one is created or deleted. Also changes events sent by successful enforce actions to compliant and adds a noncompliant "object not found" event before the enforce event. refs: https://issues.redhat.com/browse/ACM-2021?filter=-1 Signed-off-by: Will Kutler <wkutler@redhat.com>
open-cluster-management-io · Jan 10, 2023 · 789b9c6 · 789b9c6
1 parent 1f2a2e8
commit 789b9c6
Show file tree

Hide file tree

Showing 4 changed files with 148 additions and 14 deletions.
diff --git a/controllers/configurationpolicy_controller.go b/controllers/configurationpolicy_controller.go
@@ -1459,6 +1459,17 @@ func (r *ConfigurationPolicyReconciler) handleSingleObj(
 	if !exists && obj.shouldExist {
 		// it is a musthave and it does not exist, so it must be created
 		if strings.EqualFold(string(remediation), string(policyv1.Enforce)) {
+			// object is missing, so send noncompliant event
+			_ = createStatus("", obj.gvr.Resource, compliantObject, obj.namespaced, obj.policy,
+				obj.index, false, true)
+			obj.policy.Status.ComplianceState = policyv1.NonCompliant
+			statusStr := convertPolicyStatusToString(obj.policy)
+			objLog.Info("Sending an update policy status event", "policy", obj.policy.Name, "status", statusStr)
+			r.Recorder.Event(obj.policy, eventWarning, fmt.Sprintf(eventFmtStr, obj.policy.GetName(), obj.name),
+				statusStr)
+			// update parent policy status
+			r.addForUpdate(obj.policy, true)
+
 			var uid string
 			statusUpdateNeeded, uid, err = r.enforceByCreatingOrDeleting(obj)
 
@@ -1471,6 +1482,7 @@ func (r *ConfigurationPolicyReconciler) handleSingleObj(
 					CreatedByPolicy: &created,
 					UID:             uid,
 				}
+				compliant = true
 			}
 		} else { // inform
 			compliant = false
@@ -1514,7 +1526,21 @@ func (r *ConfigurationPolicyReconciler) handleSingleObj(
 
 		before := time.Now().UTC()
 
-		throwSpecViolation, msg, pErr := r.checkAndUpdateResource(obj, compType, mdCompType, remediation)
+		throwSpecViolation, msg, pErr, triedUpdate, updatedObj := r.checkAndUpdateResource(obj, compType, mdCompType,
+			remediation)
+
+		if triedUpdate {
+			// object has a mismatch and needs an update to be enforced, throw violation for mismatch
+			_ = createStatus("", obj.gvr.Resource, compliantObject, obj.namespaced, obj.policy, obj.index,
+				false, true)
+			obj.policy.Status.ComplianceState = policyv1.NonCompliant
+			statusStr := convertPolicyStatusToString(obj.policy)
+			objLog.Info("Sending an update policy status event", "policy", obj.policy.Name, "status", statusStr)
+			r.Recorder.Event(obj.policy, eventWarning, fmt.Sprintf(eventFmtStr, obj.policy.GetName(), obj.name),
+				statusStr)
+			// update parent policy status
+			r.addForUpdate(obj.policy, true)
+		}
 
 		duration := time.Now().UTC().Sub(before)
 		seconds := float64(duration) / float64(time.Second)
@@ -1538,8 +1564,17 @@ func (r *ConfigurationPolicyReconciler) handleSingleObj(
 			// it is a must have and it does exist, so it is compliant
 			compliant = true
 			if strings.EqualFold(string(remediation), string(policyv1.Enforce)) {
-				statusUpdateNeeded = createStatus("", obj.gvr.Resource, compliantObject, obj.namespaced, obj.policy,
-					obj.index, compliant, true)
+				if updatedObj {
+					// object updated in checkAndUpdateResource, send event
+					reason := "K8s update success"
+					idStr := identifierStr([]string{obj.name}, obj.namespace, obj.namespaced)
+					msg := fmt.Sprintf("%v %v was updated successfully", obj.gvr.Resource, idStr)
+
+					statusUpdateNeeded = addConditionToStatus(obj.policy, obj.index, true, reason, msg)
+				} else {
+					statusUpdateNeeded = createStatus("", obj.gvr.Resource, compliantObject, obj.namespaced, obj.policy,
+						obj.index, compliant, true)
+				}
 				created := false
 				creationInfo = &policyv1.ObjectProperties{
 					CreatedByPolicy: &created,
@@ -1559,7 +1594,14 @@ func (r *ConfigurationPolicyReconciler) handleSingleObj(
 			compliant = false
 		}
 
+		if compliant {
+			obj.policy.Status.ComplianceState = policyv1.Compliant
+		} else {
+			obj.policy.Status.ComplianceState = policyv1.NonCompliant
+		}
+
 		statusStr := convertPolicyStatusToString(obj.policy)
+
 		log.V(1).Info("Sending an update policy status event", "policy", obj.policy.Name, "status", statusStr)
 		r.Recorder.Event(obj.policy, eventType, fmt.Sprintf(eventFmtStr, obj.policy.GetName(), obj.name), statusStr)
 
@@ -1845,6 +1887,8 @@ func (r *ConfigurationPolicyReconciler) enforceByCreatingOrDeleting(obj singleOb
 			if !uidIsString || err != nil {
 				log.Error(err, "Tried to set UID in status but the field is not a string")
 			}
+
+			completed = true
 		}
 	} else {
 		log.Info("Enforcing the policy by deleting the object")
@@ -2319,21 +2363,23 @@ func (r *ConfigurationPolicyReconciler) validateObject(object *unstructured.Unst
 }
 
 // checkAndUpdateResource checks each individual key of a resource and passes it to handleKeys to see if it
-// matches the template and update it if the remediationAction is enforce
+// matches the template and update it if the remediationAction is enforce. UpdateNeeded indicates whether the
+// function tried to update the child object and updateSucceeded indicates whether the update was applied
+// successfully.
 func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
 	obj singleObject,
 	complianceType string,
 	mdComplianceType string,
 	remediation policyv1.RemediationAction,
-) (throwSpecViolation bool, message string, processingErr bool) {
+) (throwSpecViolation bool, message string, processingErr bool, updateNeeded bool, updateSucceeded bool) {
 	log := log.WithValues(
 		"policy", obj.policy.Name, "name", obj.name, "namespace", obj.namespace, "resource", obj.gvr.Resource,
 	)
 
 	if obj.object == nil {
 		log.Info("Skipping update: Previous object retrieval from the API server failed")
 
-		return false, "", false
+		return false, "", false, false, false
 	}
 
 	var res dynamic.ResourceInterface
@@ -2344,9 +2390,10 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
 	}
 
 	var err error
-	var updateNeeded bool
 	var statusUpdated bool
 
+	updateSucceeded = false
+
 	for key := range obj.unstruct.Object {
 		isStatus := key == "status"
 
@@ -2363,7 +2410,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
 		if errorMsg != "" {
 			log.Info(errorMsg)
 
-			return false, errorMsg, true
+			return false, errorMsg, true, false, false
 		}
 
 		if mergedObj == nil && skipped {
@@ -2382,7 +2429,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
 
 		if keyUpdateNeeded {
 			if strings.EqualFold(string(remediation), string(policyv1.Inform)) {
-				return true, "", false
+				return true, "", false, false, false
 			} else if isStatus {
 				statusUpdated = true
 				log.Info("Ignoring an update to the object status", "key", key)
@@ -2399,26 +2446,26 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
 		if err := r.validateObject(obj.object); err != nil {
 			message := fmt.Sprintf("Error validating the object %s, the error is `%v`", obj.name, err)
 
-			return false, message, true
+			return false, message, true, updateNeeded, false
 		}
 
 		_, err = res.Update(context.TODO(), obj.object, metav1.UpdateOptions{})
 		if k8serrors.IsNotFound(err) {
 			message := fmt.Sprintf("`%v` is not present and must be created", obj.object.GetKind())
 
-			return false, message, true
+			return false, message, true, updateNeeded, false
 		}
 
 		if err != nil {
 			message := fmt.Sprintf("Error updating the object `%v`, the error is `%v`", obj.name, err)
 
-			return false, message, true
+			return false, message, true, updateNeeded, false
 		}
 
-		log.Info("Updated the object based on the template definition")
+		updateSucceeded = true
 	}
 
-	return statusUpdated, "", false
+	return statusUpdated, "", false, updateNeeded, updateSucceeded
 }
 
 // AppendCondition check and appends conditions to the policy status

diff --git a/test/e2e/case27_showupdateinstatus_test.go b/test/e2e/case27_showupdateinstatus_test.go
@@ -0,0 +1,51 @@
+// Copyright (c) 2020 Red Hat, Inc.
+// Copyright Contributors to the Open Cluster Management project
+
+package e2e
+
+import (
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+
+	"open-cluster-management.io/config-policy-controller/test/utils"
+)
+
+const (
+	case27ConfigPolicyName string = "policy-cfgmap-create"
+	case27CreateYaml       string = "../resources/case27_showupdateinstatus/case27-create-cfgmap-policy.yaml"
+	case27UpdateYaml       string = "../resources/case27_showupdateinstatus/case27-update-cfgmap-policy.yaml"
+)
+
+var _ = Describe("Verify status update after updating object", Ordered, func() {
+	It("configmap should be created properly on the managed cluster", func() {
+		By("Creating " + case27ConfigPolicyName + " on managed")
+		utils.Kubectl("apply", "-f", case27CreateYaml, "-n", testNamespace)
+		plc := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy,
+			case27ConfigPolicyName, testNamespace, true, defaultTimeoutSeconds)
+		Expect(plc).NotTo(BeNil())
+		Eventually(func() interface{} {
+			managedPlc := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy,
+				case27ConfigPolicyName, testNamespace, true, defaultTimeoutSeconds)
+
+			return utils.GetStatusMessage(managedPlc)
+		}, 120, 1).Should(Equal(
+			"configmaps [case27-map] in namespace default found as specified, " +
+				"therefore this Object template is compliant"))
+	})
+	It("configmap and status should be updated properly on the managed cluster", func() {
+		By("Updating " + case27ConfigPolicyName + " on managed")
+		utils.Kubectl("apply", "-f", case27UpdateYaml, "-n", testNamespace)
+		Eventually(func() interface{} {
+			managedPlc := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy,
+				case27ConfigPolicyName, testNamespace, true, defaultTimeoutSeconds)
+
+			return utils.GetStatusMessage(managedPlc)
+		}, 30, 0.5).Should(Equal(
+			"configmaps [case27-map] in namespace default was updated successfully"))
+	})
+
+	AfterAll(func() {
+		deleteConfigPolicies([]string{case27ConfigPolicyName})
+		utils.Kubectl("delete", "configmap", "case27-map")
+	})
+})
diff --git a/test/resources/case27_showupdateinstatus/case27-create-cfgmap-policy.yaml b/test/resources/case27_showupdateinstatus/case27-create-cfgmap-policy.yaml
@@ -0,0 +1,18 @@
+apiVersion: policy.open-cluster-management.io/v1
+kind: ConfigurationPolicy
+metadata:
+  name: policy-cfgmap-create
+spec:
+  remediationAction: enforce
+  namespaceSelector:
+    exclude: ["kube-*"]
+    include: ["default"]
+  object-templates:
+    - complianceType: musthave
+      objectDefinition:
+        apiVersion: v1
+        kind: ConfigMap
+        metadata:
+          name: case27-map
+        data:
+          fieldToUpdate: "1"
diff --git a/test/resources/case27_showupdateinstatus/case27-update-cfgmap-policy.yaml b/test/resources/case27_showupdateinstatus/case27-update-cfgmap-policy.yaml
@@ -0,0 +1,18 @@
+apiVersion: policy.open-cluster-management.io/v1
+kind: ConfigurationPolicy
+metadata:
+  name: policy-cfgmap-create
+spec:
+  remediationAction: enforce
+  namespaceSelector:
+    exclude: ["kube-*"]
+    include: ["default"]
+  object-templates:
+    - complianceType: musthave
+      objectDefinition:
+        apiVersion: v1
+        kind: ConfigMap
+        metadata:
+          name: case27-map
+        data:
+          fieldToUpdate: "2"