From adf1b27f69c66ad90c9320a7bc352e0a0b98b5c9 Mon Sep 17 00:00:00 2001 From: mprahl Date: Thu, 6 Apr 2023 12:31:13 -0400 Subject: [PATCH] Use patches for adding and removing finalizers Using update can have unintended consequences such as setting empty values of fields that were nil on the server. This happens for the evaluationInterval field where if it wasn't set, the update adds an empty map, which triggers the spec-sync to remove the nil map. This unnecessary spec-sync update causes the status update to fail later on in the ConfigurationPolicy processing. Additionally, when a finalizer can't be added or removed, the evaluation stops and would be retried on the next run. This is a safer approach. Signed-off-by: mprahl --- controllers/configurationpolicy_controller.go | 34 ++++++++++++------- controllers/configurationpolicy_utils.go | 21 ++++-------- 2 files changed, 28 insertions(+), 27 deletions(-) diff --git a/controllers/configurationpolicy_controller.go b/controllers/configurationpolicy_controller.go index b6b382e1..906e8219 100644 --- a/controllers/configurationpolicy_controller.go +++ b/controllers/configurationpolicy_controller.go @@ -704,11 +704,13 @@ func (r *ConfigurationPolicyReconciler) handleObjectTemplates(plc policyv1.Confi if cleanupNow { if objHasFinalizer(&plc, pruneObjectFinalizer) { - plc.SetFinalizers(removeObjFinalizer(&plc, pruneObjectFinalizer)) + patch := removeObjFinalizerPatch(&plc, pruneObjectFinalizer) - err := r.Update(context.TODO(), &plc) + err = r.Patch(context.TODO(), &plc, client.RawPatch(types.JSONPatchType, patch)) if err != nil { - log.V(1).Error(err, "Error removing finalizer for configuration policy", plc) + log.V(1).Error(err, "Error removing finalizer for configuration policy") + + return } } @@ -717,11 +719,13 @@ func (r *ConfigurationPolicyReconciler) handleObjectTemplates(plc policyv1.Confi // set finalizer if it hasn't been set if !objHasFinalizer(&plc, pruneObjectFinalizer) { - plc.SetFinalizers(addObjFinalizer(&plc, pruneObjectFinalizer)) + mergePatch := []byte(`{"metadata": {"finalizers": ["` + pruneObjectFinalizer + `"]}}`) - err := r.Update(context.TODO(), &plc) + err := r.Patch(context.TODO(), &plc, client.RawPatch(types.MergePatchType, mergePatch)) if err != nil { - log.V(1).Error(err, "Error setting finalizer for configuration policy", plc) + log.V(1).Error(err, "Error setting finalizer for configuration policy") + + return } } @@ -733,11 +737,14 @@ func (r *ConfigurationPolicyReconciler) handleObjectTemplates(plc policyv1.Confi if len(failures) == 0 { log.V(1).Info("Objects have been successfully cleaned up, removing finalizer") - plc.SetFinalizers(removeObjFinalizer(&plc, pruneObjectFinalizer)) - err := r.Update(context.TODO(), &plc) + patch := removeObjFinalizerPatch(&plc, pruneObjectFinalizer) + + err = r.Patch(context.TODO(), &plc, client.RawPatch(types.JSONPatchType, patch)) if err != nil { - log.V(1).Error(err, "Error unsetting finalizer for configuration policy", plc) + log.V(1).Error(err, "Error removing finalizer for configuration policy") + + return } } else { log.V(1).Info("Object cleanup failed, some objects have not been deleted from the cluster") @@ -767,10 +774,13 @@ func (r *ConfigurationPolicyReconciler) handleObjectTemplates(plc policyv1.Confi } } else if objHasFinalizer(&plc, pruneObjectFinalizer) { // if pruneObjectBehavior is none, no finalizer is needed - plc.SetFinalizers(removeObjFinalizer(&plc, pruneObjectFinalizer)) - err := r.Update(context.TODO(), &plc) + patch := removeObjFinalizerPatch(&plc, pruneObjectFinalizer) + + err := r.Patch(context.TODO(), &plc, client.RawPatch(types.JSONPatchType, patch)) if err != nil { - log.V(1).Error(err, "Error unsetting finalizer for configuration policy", plc) + log.V(1).Error(err, "Error removing finalizer for configuration policy") + + return } } diff --git a/controllers/configurationpolicy_utils.go b/controllers/configurationpolicy_utils.go index 89e2cc3e..3756fd83 100644 --- a/controllers/configurationpolicy_utils.go +++ b/controllers/configurationpolicy_utils.go @@ -7,6 +7,7 @@ import ( "fmt" "reflect" "sort" + "strconv" "strings" gocmp "github.com/google/go-cmp/cmp" @@ -562,24 +563,14 @@ func objHasFinalizer(obj metav1.Object, finalizer string) bool { return false } -func addObjFinalizer(obj metav1.Object, finalizer string) []string { - if objHasFinalizer(obj, finalizer) { - return obj.GetFinalizers() - } - - return append(obj.GetFinalizers(), finalizer) -} - -func removeObjFinalizer(obj metav1.Object, finalizer string) []string { - result := []string{} - - for _, existingFinalizer := range obj.GetFinalizers() { - if existingFinalizer != finalizer { - result = append(result, existingFinalizer) +func removeObjFinalizerPatch(obj metav1.Object, finalizer string) []byte { + for i, existingFinalizer := range obj.GetFinalizers() { + if existingFinalizer == finalizer { + return []byte(`[{"op":"remove","path":"/metadata/finalizers/` + strconv.FormatInt(int64(i), 10) + `"}]`) } } - return result + return nil } func containRelated(arr []policyv1.RelatedObject, input policyv1.RelatedObject) bool {