From 34821fa76280945f263d72e2cd118821c8554f91 Mon Sep 17 00:00:00 2001 From: Joe Lanford Date: Thu, 19 Sep 2024 10:27:48 -0400 Subject: [PATCH] fix: do not silently drop finalizer updates when status is also updated (#1283) --- .../controllers/clusterextension_controller.go | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/internal/controllers/clusterextension_controller.go b/internal/controllers/clusterextension_controller.go index dc3dd7247..6e06f849d 100644 --- a/internal/controllers/clusterextension_controller.go +++ b/internal/controllers/clusterextension_controller.go @@ -116,17 +116,24 @@ func (r *ClusterExtensionReconciler) Reconcile(ctx context.Context, req ctrl.Req // Do checks before any Update()s, as Update() may modify the resource structure! updateStatus := !equality.Semantic.DeepEqual(existingExt.Status, reconciledExt.Status) updateFinalizers := !equality.Semantic.DeepEqual(existingExt.Finalizers, reconciledExt.Finalizers) + + // If any unexpected fields have changed, panic before updating the resource unexpectedFieldsChanged := checkForUnexpectedFieldChange(*existingExt, *reconciledExt) + if unexpectedFieldsChanged { + panic("spec or metadata changed by reconciler") + } + // Save the finalizers off to the side. If we update the status, the reconciledExt will be updated + // to contain the new state of the ClusterExtension, which contains the status update, but (critically) + // does not contain the finalizers. After the status update, we need to re-add the finalizers to the + // reconciledExt before updating the object. + finalizers := reconciledExt.Finalizers if updateStatus { if err := r.Client.Status().Update(ctx, reconciledExt); err != nil { updateError = errors.Join(updateError, fmt.Errorf("error updating status: %v", err)) } } - - if unexpectedFieldsChanged { - panic("spec or metadata changed by reconciler") - } + reconciledExt.Finalizers = finalizers if updateFinalizers { if err := r.Client.Update(ctx, reconciledExt); err != nil {