fix: requeue instead of error when finalizer removal yields a conflict (

#454)

* fix: requeue instead of error when finalizer removal yields a conflict

* Update controller/gateway/controller_cleanup.go

* chore: reword comments

CHANGELOG.md

@@ -48,6 +48,8 @@
 - Fixed `ControlPlane` cluster wide resources not migrating to new ownership labels
   (introduced in 1.3.0) when upgrading the operator form 1.2 (or older) to 1.3.0.
   [#369](https://github.com/Kong/gateway-operator/pull/369)
+- Requeue instead of reporting an error when a finalizer removal yields a conflict.
+  [#454](https://github.com/Kong/gateway-operator/pull/454)
 
 ## [v1.3.0]
 

controller/gateway/controller.go

@@ -116,9 +116,10 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 	}
 
 	log.Trace(logger, "managing cleanup for gateway resource", gateway)
-	cleanupUnderway, result, err := r.cleanup(ctx, logger, &gateway)
-	if cleanupUnderway {
+	if shouldReturnEarly, result, err := r.cleanup(ctx, logger, &gateway); err != nil || result.Requeue {
 		return result, err
+	} else if shouldReturnEarly {
+		return ctrl.Result{}, nil
 	}
 
 	log.Trace(logger, "managing the gateway resource finalizers", gateway)

controller/gateway/controller_cleanup.go

@@ -23,15 +23,25 @@ import (
 const requeueWithoutBackoff = 200 * time.Millisecond
 
 // cleanup determines whether cleanup is needed/underway for a Gateway and
-// performs all necessary cleanup steps. Namely, it cleans up resources
-// managed on behalf of the Gateway and removes the finalizers once all
-// cleanup is finished so that the garbage collector can remove the resource.
+// performs all necessary cleanup steps.
+// Namely, it cleans up resources managed on behalf of the Gateway and removes
+// the finalizers one by one, after each cleanup step is finished.
+// If the Gateway is marked for deletion, it will wait for all owned resources
+// to be deleted before removing the finalizers.
+//
+// It returns a boolean indicating whether the caller should return early
+// from the reconciliation loop, a ctrl.Result to requeue the request, and an error.
+// The caller should return early if
+//   - the requeue is set explicitly, then the ctrl.Result should be returned as is.
+//   - the error is not nil, then the error should be returned as is.
+//   - the boolean is true, then the reconciliation loop should return early without
+//     requeuing the request.
 func (r *Reconciler) cleanup(
 	ctx context.Context,
 	logger logr.Logger,
 	gateway *gatewayv1.Gateway,
 ) (
-	bool, // whether or not cleanup is being performed
+	bool, // Whether the caller should return early from the reconciliation loop.
 	ctrl.Result,
 	error,
 ) {
@@ -42,80 +52,87 @@ func (r *Reconciler) cleanup(
 
 	if gateway.DeletionTimestamp.After(time.Now()) {
 		log.Debug(logger, "gateway deletion still under grace period", gateway)
-		return true, ctrl.Result{
+		return false, ctrl.Result{
 			Requeue:      true,
 			RequeueAfter: time.Until(gateway.DeletionTimestamp.Time),
 		}, nil
 	}
-	log.Trace(logger, "gateway is marked delete, waiting for owned resources deleted", gateway)
+	log.Trace(logger, "gateway is marked for deletion, waiting for owned resources to be deleted", gateway)
 
 	// Delete owned controlplanes.
 	// Because controlplanes have finalizers, so we only remove the finalizer
 	// for cleaning up owned controlplanes when they disappeared.
 	controlplanes, err := gatewayutils.ListControlPlanesForGateway(ctx, r.Client, gateway)
 	if err != nil {
-		return true, ctrl.Result{}, err
+		return false, ctrl.Result{}, err
 	}
 	if len(controlplanes) > 0 {
 		deletions, err := r.ensureOwnedControlPlanesDeleted(ctx, gateway)
 		if err != nil {
-			return true, ctrl.Result{}, err
+			return false, ctrl.Result{}, err
 		}
 		if deletions {
 			log.Debug(logger, "deleted owned controlplanes", gateway)
-			return true, ctrl.Result{}, err
+			// Return early from reconciliation, deletion will trigger a new reconcile.
+			return true, ctrl.Result{}, nil
 		}
 	} else {
 		oldGateway := gateway.DeepCopy()
 		if controllerutil.RemoveFinalizer(gateway, string(GatewayFinalizerCleanupControlPlanes)) {
 			if err := r.Client.Patch(ctx, gateway, client.MergeFrom(oldGateway)); err != nil {
 				res, err := handleGatewayFinalizerPatchOrUpdateError(err, gateway, logger)
-				return true, res, err
+				return false, res, err
 			}
 			log.Debug(logger, "finalizer for cleaning up controlplanes removed", gateway)
-			return true, ctrl.Result{}, nil
+			// Requeue to ensure that we continue reconciliation in case the patch
+			// was empty and didn't trigger a new reconcile.
+			return false, ctrl.Result{Requeue: true}, nil
 		}
 	}
 
 	// Delete owned dataplanes.
 	dataplanes, err := gatewayutils.ListDataPlanesForGateway(ctx, r.Client, gateway)
 	if err != nil {
-		return true, ctrl.Result{}, err
+		return false, ctrl.Result{}, err
 	}
 
 	if len(dataplanes) > 0 {
 		deletions, err := r.ensureOwnedDataPlanesDeleted(ctx, gateway)
 		if err != nil {
-			return true, ctrl.Result{}, err
+			return false, ctrl.Result{}, err
 		}
 		if deletions {
 			log.Debug(logger, "deleted owned dataplanes", gateway)
+			// Return early from reconciliation, deletion will trigger a new reconcile.
 			return true, ctrl.Result{}, err
 		}
 	} else {
 		oldGateway := gateway.DeepCopy()
 		if controllerutil.RemoveFinalizer(gateway, string(GatewayFinalizerCleanupDataPlanes)) {
 			if err := r.Client.Patch(ctx, gateway, client.MergeFrom(oldGateway)); err != nil {
 				res, err := handleGatewayFinalizerPatchOrUpdateError(err, gateway, logger)
-				return true, res, err
+				return false, res, err
 			}
 			log.Debug(logger, "finalizer for cleaning up dataplanes removed", gateway)
-			return true, ctrl.Result{}, nil
+			// Requeue to ensure that we continue reconciliation in case the patch
+			// was empty and didn't trigger a new reconcile.
+			return false, ctrl.Result{Requeue: true}, nil
 		}
 	}
 
 	// Delete owned network policies
 	networkPolicies, err := gatewayutils.ListNetworkPoliciesForGateway(ctx, r.Client, gateway)
 	if err != nil {
-		return true, ctrl.Result{}, err
+		return false, ctrl.Result{}, err
 	}
 	if len(networkPolicies) > 0 {
 		deletions, err := r.ensureOwnedNetworkPoliciesDeleted(ctx, gateway)
 		if err != nil {
-			return true, ctrl.Result{}, err
+			return false, ctrl.Result{}, err
 		}
 		if deletions {
 			log.Debug(logger, "deleted owned network policies", gateway)
+			// Return early from reconciliation, deletion will trigger a new reconcile.
 			return true, ctrl.Result{}, err
 		}
 	} else {
@@ -126,12 +143,14 @@ func (r *Reconciler) cleanup(
 				return true, res, err
 			}
 			log.Debug(logger, "finalizer for cleaning up network policies removed", gateway)
-			return true, ctrl.Result{}, nil
+			// Requeue to ensure that we continue reconciliation in case the patch
+			// was empty and didn't trigger a new reconcile.
+			return false, ctrl.Result{Requeue: true}, nil
 		}
 	}
 
 	log.Debug(logger, "owned resources cleanup completed", gateway)
-	return true, ctrl.Result{}, nil
+	return false, ctrl.Result{}, nil
 }
 
 func handleGatewayFinalizerPatchOrUpdateError(err error, gateway *gatewayv1.Gateway, logger logr.Logger) (ctrl.Result, error) {
@@ -140,13 +159,14 @@ func handleGatewayFinalizerPatchOrUpdateError(err error, gateway *gatewayv1.Gate
 		return ctrl.Result{}, nil
 	}
 
-	// If the Gateway is not found, then requeue without an error.
-	if k8serrors.IsNotFound(err) {
+	// If the Gateway is not found or there's a conflict patching, then requeue without an error.
+	if k8serrors.IsNotFound(err) || k8serrors.IsConflict(err) {
 		return ctrl.Result{
 			Requeue:      true,
 			RequeueAfter: requeueWithoutBackoff,
 		}, nil
 	}
+
 	// Since controllers use cached clients, it's possible that the Gateway is out of sync with what
 	// is in the API server and this causes:
 	// Forbidden: no new finalizers can be added if the object is being deleted, found new finalizers []string{...}