COO-1690: fix: add finalizers with Patch (#1029)

* fix: add finalizers with Patch

This avoid the chances of trying to updates a resource that had been
removed concurrently. E2E test runs have had log messages suggesting
this was happening. Moving to Patch should avoid this as we no longer
call Update with a UUID that is no longer present in etcd.

Signed-off-by: Jan Fajerski <jan@fajerski.name>

* fix: add patch permissions for monitoringstacks

---------

Signed-off-by: Jan Fajerski <jan@fajerski.name>
Co-authored-by: Jan Fajerski <jan@fajerski.name>
Co-authored-by: PeterYurkovich <pyurkovi@redhat.com>

Author: 3 people

bundle/manifests/observability-operator.clusterserviceversion.yaml

@@ -528,6 +528,7 @@ spec:
           - create
           - get
           - list
+          - patch
           - update
           - watch
         - apiGroups:

deploy/operator/observability-operator-cluster-role.yaml

@@ -211,6 +211,7 @@ rules:
   - create
   - get
   - list
+  - patch
   - update
   - watch
 - apiGroups:

pkg/controllers/monitoring/monitoring-stack/controller.go

@@ -18,7 +18,6 @@ package monitoringstack
 
 import (
 	"context"
-	"slices"
 	"time"
 
 	"github.com/go-logr/logr"
@@ -32,6 +31,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/builder"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/predicate"
 
 	stack "github.com/rhobs/observability-operator/pkg/apis/monitoring/v1alpha1"
@@ -69,7 +69,7 @@ type Options struct {
 const finalizerName = "monitoring.observability.openshift.io/finalizer"
 
 // RBAC for managing monitoring stacks
-//+kubebuilder:rbac:groups=monitoring.rhobs,resources=monitoringstacks,verbs=list;watch;create;update
+//+kubebuilder:rbac:groups=monitoring.rhobs,resources=monitoringstacks,verbs=list;watch;create;update;patch
 //+kubebuilder:rbac:groups=monitoring.rhobs,resources=monitoringstacks/status,verbs=get;update
 
 // RBAC for managing Prometheus Operator CRs
@@ -149,11 +149,13 @@ func (rm resourceManager) Reconcile(ctx context.Context, req ctrl.Request) (ctrl
 		}
 
 		// Remove finalizer if present
-		if slices.Contains(ms.ObjectMeta.Finalizers, finalizerName) {
-			ms.ObjectMeta.Finalizers = slices.DeleteFunc(ms.ObjectMeta.Finalizers, func(currentFinalizerName string) bool {
-				return currentFinalizerName == finalizerName
-			})
-			if err := rm.k8sClient.Update(ctx, ms); err != nil {
+		if controllerutil.ContainsFinalizer(ms, finalizerName) {
+			patch := client.MergeFrom(ms.DeepCopy())
+			controllerutil.RemoveFinalizer(ms, finalizerName)
+			if err := rm.k8sClient.Patch(ctx, ms, patch); err != nil {
+				if errors.IsNotFound(err) {
+					return ctrl.Result{}, nil
+				}
 				return ctrl.Result{}, err
 			}
 		}
@@ -163,9 +165,13 @@ func (rm resourceManager) Reconcile(ctx context.Context, req ctrl.Request) (ctrl
 	}
 
 	// Add finalizer if not present
-	if !slices.Contains(ms.ObjectMeta.Finalizers, finalizerName) {
-		ms.ObjectMeta.Finalizers = append(ms.ObjectMeta.Finalizers, finalizerName)
-		if err := rm.k8sClient.Update(ctx, ms); err != nil {
+	if !controllerutil.ContainsFinalizer(ms, finalizerName) {
+		patch := client.MergeFrom(ms.DeepCopy())
+		controllerutil.AddFinalizer(ms, finalizerName)
+		if err := rm.k8sClient.Patch(ctx, ms, patch); err != nil {
+			if errors.IsNotFound(err) {
+				return ctrl.Result{}, nil
+			}
 			return ctrl.Result{}, err
 		}
 	}

pkg/controllers/observability/observability_controller.go

@@ -88,11 +88,13 @@ func (o observabilityInstallerController) Reconcile(ctx context.Context, request
 
 	// Add finalizer for this CR
 	if !controllerutil.ContainsFinalizer(instance, finalizerName) {
-		if controllerutil.AddFinalizer(instance, finalizerName) {
-			err := o.client.Update(ctx, instance)
-			if err != nil {
-				return ctrl.Result{}, err
+		patch := client.MergeFrom(instance.DeepCopy())
+		controllerutil.AddFinalizer(instance, finalizerName)
+		if err := o.client.Patch(ctx, instance, patch); err != nil {
+			if apierrors.IsNotFound(err) {
+				return ctrl.Result{}, nil
 			}
+			return ctrl.Result{}, err
 		}
 	}
 
@@ -153,11 +155,13 @@ func (o observabilityInstallerController) Reconcile(ctx context.Context, request
 		if controllerutil.ContainsFinalizer(instance, finalizerName) {
 			// Once all finalizers have been
 			// removed, the object will be deleted.
-			if controllerutil.RemoveFinalizer(instance, finalizerName) {
-				err := o.client.Update(ctx, instance)
-				if err != nil {
-					return ctrl.Result{}, err
+			patch := client.MergeFrom(instance.DeepCopy())
+			controllerutil.RemoveFinalizer(instance, finalizerName)
+			if err := o.client.Patch(ctx, instance, patch); err != nil {
+				if apierrors.IsNotFound(err) {
+					return ctrl.Result{}, nil
 				}
+				return ctrl.Result{}, err
 			}
 		}
 	}

pkg/controllers/uiplugin/controller.go

@@ -25,6 +25,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/builder"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/predicate"
 
 	uiv1alpha1 "github.com/rhobs/observability-operator/pkg/apis/uiplugin/v1alpha1"
@@ -222,11 +223,13 @@ func (rm resourceManager) Reconcile(ctx context.Context, req ctrl.Request) (ctrl
 		}
 
 		// Remove finalizer if present
-		if slices.Contains(plugin.ObjectMeta.Finalizers, finalizerName) {
-			plugin.ObjectMeta.Finalizers = slices.DeleteFunc(plugin.ObjectMeta.Finalizers, func(currentFinalizerName string) bool {
-				return currentFinalizerName == finalizerName
-			})
-			if err := rm.k8sClient.Update(ctx, plugin); err != nil {
+		if controllerutil.ContainsFinalizer(plugin, finalizerName) {
+			patch := client.MergeFrom(plugin.DeepCopy())
+			controllerutil.RemoveFinalizer(plugin, finalizerName)
+			if err := rm.k8sClient.Patch(ctx, plugin, patch); err != nil {
+				if apierrors.IsNotFound(err) {
+					return ctrl.Result{}, nil
+				}
 				return ctrl.Result{}, err
 			}
 		}
@@ -236,9 +239,13 @@ func (rm resourceManager) Reconcile(ctx context.Context, req ctrl.Request) (ctrl
 	}
 
 	// Add finalizer if not present
-	if !slices.Contains(plugin.ObjectMeta.Finalizers, finalizerName) {
-		plugin.ObjectMeta.Finalizers = append(plugin.ObjectMeta.Finalizers, finalizerName)
-		if err := rm.k8sClient.Update(ctx, plugin); err != nil {
+	if !controllerutil.ContainsFinalizer(plugin, finalizerName) {
+		patch := client.MergeFrom(plugin.DeepCopy())
+		controllerutil.AddFinalizer(plugin, finalizerName)
+		if err := rm.k8sClient.Patch(ctx, plugin, patch); err != nil {
+			if apierrors.IsNotFound(err) {
+				return ctrl.Result{}, nil
+			}
 			return ctrl.Result{}, err
 		}
 	}