From e6d4c56ebb349d0de3c52b79743a5f1cd89f86e1 Mon Sep 17 00:00:00 2001 From: Will Kutler Date: Fri, 6 Jan 2023 11:09:59 -0500 Subject: [PATCH] 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 --- controllers/configurationpolicy_controller.go | 75 +++++++++++++++---- test/e2e/case27_showupdateinstatus_test.go | 51 +++++++++++++ .../case27-create-cfgmap-policy.yaml | 18 +++++ .../case27-update-cfgmap-policy.yaml | 18 +++++ 4 files changed, 148 insertions(+), 14 deletions(-) create mode 100644 test/e2e/case27_showupdateinstatus_test.go create mode 100644 test/resources/case27_showupdateinstatus/case27-create-cfgmap-policy.yaml create mode 100644 test/resources/case27_showupdateinstatus/case27-update-cfgmap-policy.yaml diff --git a/controllers/configurationpolicy_controller.go b/controllers/configurationpolicy_controller.go index 5cfd1ee1..4bb3e12c 100644 --- 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,13 +2363,15 @@ 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, ) @@ -2333,7 +2379,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource( 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 new file mode 100644 index 00000000..4d94802e --- /dev/null +++ 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 new file mode 100644 index 00000000..0868ce52 --- /dev/null +++ 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 new file mode 100644 index 00000000..6ed25c06 --- /dev/null +++ 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"