Improve patch helper error handling

Signed-off-by: Stefan Büringer buringerst@vmware.com

bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller.go

@@ -226,7 +226,7 @@ func (r *KubeadmConfigReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 			patchOpts = append(patchOpts, patch.WithStatusObservedGeneration{})
 		}
 		if err := patchHelper.Patch(ctx, config, patchOpts...); err != nil {
-			rerr = kerrors.NewAggregate([]error{rerr, errors.Wrapf(err, "failed to patch %s", klog.KObj(config))})
+			rerr = kerrors.NewAggregate([]error{rerr, err})
 		}
 	}()
 

cmd/clusterctl/client/cluster/mover.go

@@ -741,7 +741,7 @@ func pauseClusterClass(ctx context.Context, proxy Proxy, n *node, pause bool, mu
 
 	patchHelper, err := patch.NewHelper(clusterClass, cFrom)
 	if err != nil {
-		return errors.Wrapf(err, "error creating patcher for ClusterClass %s/%s", n.identity.Namespace, n.identity.Name)
+		return err
 	}
 
 	// Update the annotation to the desired state
@@ -759,11 +759,8 @@ func pauseClusterClass(ctx context.Context, proxy Proxy, n *node, pause bool, mu
 
 	// Update the ClusterClass with the new annotations.
 	clusterClass.SetAnnotations(ccAnnotations)
-	if err := patchHelper.Patch(ctx, clusterClass); err != nil {
-		return errors.Wrapf(err, "error patching ClusterClass %s/%s", n.identity.Namespace, n.identity.Name)
-	}
 
-	return nil
+	return patchHelper.Patch(ctx, clusterClass)
 }
 
 // ensureNamespaces ensures all the expected target namespaces are in place before creating objects.

controlplane/kubeadm/internal/control_plane.go

@@ -70,7 +70,7 @@ func NewControlPlane(ctx context.Context, managementCluster ManagementCluster, c
 	for _, machine := range ownedMachines {
 		patchHelper, err := patch.NewHelper(machine, client)
 		if err != nil {
-			return nil, errors.Wrapf(err, "failed to create patch helper for machine %s", machine.Name)
+			return nil, err
 		}
 		patchHelpers[machine.Name] = patchHelper
 	}
@@ -271,7 +271,7 @@ func (c *ControlPlane) PatchMachines(ctx context.Context) error {
 				controlplanev1.MachineEtcdPodHealthyCondition,
 				controlplanev1.MachineEtcdMemberHealthyCondition,
 			}}); err != nil {
-				errList = append(errList, errors.Wrapf(err, "failed to patch machine %s", machine.Name))
+				errList = append(errList, err)
 			}
 			continue
 		}

controlplane/kubeadm/internal/controllers/controller.go

@@ -179,8 +179,7 @@ func (r *KubeadmControlPlaneReconciler) Reconcile(ctx context.Context, req ctrl.
 		// Patch ObservedGeneration only if the reconciliation completed successfully
 		patchOpts := []patch.Option{patch.WithStatusObservedGeneration{}}
 		if err := patchHelper.Patch(ctx, kcp, patchOpts...); err != nil {
-			log.Error(err, "Failed to patch KubeadmControlPlane to add finalizer")
-			return ctrl.Result{}, err
+			return ctrl.Result{}, errors.Wrapf(err, "failed to add finalizer")
 		}
 
 		return ctrl.Result{}, nil
@@ -627,7 +626,7 @@ func (r *KubeadmControlPlaneReconciler) syncMachines(ctx context.Context, contro
 		// TODO: This should be cleaned-up to have a more streamline way of constructing and using patchHelpers.
 		patchHelper, err := patch.NewHelper(updatedMachine, r.Client)
 		if err != nil {
-			return errors.Wrapf(err, "failed to create patch helper for Machine %s", klog.KObj(updatedMachine))
+			return err
 		}
 		patchHelpers[machineName] = patchHelper
 
@@ -812,7 +811,7 @@ func (r *KubeadmControlPlaneReconciler) reconcileCertificateExpiries(ctx context
 		log.V(2).Info(fmt.Sprintf("Setting certificate expiry to %s", expiry))
 		patchHelper, err := patch.NewHelper(kubeadmConfig, r.Client)
 		if err != nil {
-			return errors.Wrapf(err, "failed to reconcile certificate expiry for Machine/%s: failed to create PatchHelper for KubeadmConfig/%s", m.Name, kubeadmConfig.Name)
+			return errors.Wrapf(err, "failed to reconcile certificate expiry for Machine/%s", m.Name)
 		}
 
 		if annotations == nil {
@@ -822,7 +821,7 @@ func (r *KubeadmControlPlaneReconciler) reconcileCertificateExpiries(ctx context
 		kubeadmConfig.SetAnnotations(annotations)
 
 		if err := patchHelper.Patch(ctx, kubeadmConfig); err != nil {
-			return errors.Wrapf(err, "failed to reconcile certificate expiry for Machine/%s: failed to patch KubeadmConfig/%s", m.Name, kubeadmConfig.Name)
+			return errors.Wrapf(err, "failed to reconcile certificate expiry for Machine/%s", m.Name)
 		}
 	}
 
@@ -950,7 +949,7 @@ func (r *KubeadmControlPlaneReconciler) ensureCertificatesOwnerRef(ctx context.C
 
 		patchHelper, err := patch.NewHelper(c.Secret, r.Client)
 		if err != nil {
-			return errors.Wrapf(err, "failed to create patchHelper for Secret %s", klog.KObj(c.Secret))
+			return err
 		}
 
 		controller := metav1.GetControllerOf(c.Secret)
@@ -971,7 +970,7 @@ func (r *KubeadmControlPlaneReconciler) ensureCertificatesOwnerRef(ctx context.C
 			c.Secret.SetOwnerReferences(util.EnsureOwnerRef(c.Secret.GetOwnerReferences(), owner))
 		}
 		if err := patchHelper.Patch(ctx, c.Secret); err != nil {
-			return errors.Wrapf(err, "failed to patch Secret %s with ownerReference %s", klog.KObj(c.Secret), owner.String())
+			return errors.Wrapf(err, "failed to set ownerReference")
 		}
 	}
 	return nil

controlplane/kubeadm/internal/controllers/helpers.go

@@ -104,11 +104,11 @@ func (r *KubeadmControlPlaneReconciler) reconcileKubeconfig(ctx context.Context,
 func (r *KubeadmControlPlaneReconciler) adoptKubeconfigSecret(ctx context.Context, configSecret *corev1.Secret, kcp *controlplanev1.KubeadmControlPlane) (reterr error) {
 	patchHelper, err := patch.NewHelper(configSecret, r.Client)
 	if err != nil {
-		return errors.Wrap(err, "failed to create patch helper for the kubeconfig secret")
+		return err
 	}
 	defer func() {
 		if err := patchHelper.Patch(ctx, configSecret); err != nil {
-			reterr = errors.Wrap(err, "failed to patch the kubeconfig secret")
+			reterr = kerrors.NewAggregate([]error{reterr, err})
 		}
 	}()
 	controller := metav1.GetControllerOf(configSecret)

controlplane/kubeadm/internal/controllers/remediation.go

@@ -55,7 +55,7 @@ func (r *KubeadmControlPlaneReconciler) reconcileUnhealthyMachines(ctx context.C
 			m.DeletionTimestamp.IsZero() {
 			patchHelper, err := patch.NewHelper(m, r.Client)
 			if err != nil {
-				errList = append(errList, errors.Wrapf(err, "failed to get PatchHelper for machine %s", m.Name))
+				errList = append(errList, err)
 				continue
 			}
 
@@ -64,7 +64,7 @@ func (r *KubeadmControlPlaneReconciler) reconcileUnhealthyMachines(ctx context.C
 			if err := patchHelper.Patch(ctx, m, patch.WithOwnedConditions{Conditions: []clusterv1.ConditionType{
 				clusterv1.MachineOwnerRemediatedCondition,
 			}}); err != nil {
-				errList = append(errList, errors.Wrapf(err, "failed to patch machine %s", m.Name))
+				errList = append(errList, err)
 			}
 		}
 	}

docs/book/src/developer/providers/migrations/v1.6-to-v1.7.md

@@ -24,4 +24,6 @@ maintainers of providers and consumers of our Go API.
 
 ### Other
 
+* Patch helper now return error with enough error context (https://github.com/kubernetes-sigs/cluster-api/pull/9946). It is recommended to remove redundant error context on call sites if applicable.
+
 ### Suggested changes for providers

exp/addons/internal/controllers/clusterresourceset_controller.go

@@ -212,7 +212,6 @@ func (r *ClusterResourceSetReconciler) reconcileDelete(ctx context.Context, clus
 				log.Error(err, "failed to delete empty ClusterResourceSetBinding")
 			}
 		} else if err := patchHelper.Patch(ctx, clusterResourceSetBinding); err != nil {
-			log.Error(err, "failed to patch ClusterResourceSetBinding")
 			return err
 		}
 	}

exp/addons/internal/controllers/clusterresourcesetbinding_controller.go

@@ -123,7 +123,7 @@ func (r *ClusterResourceSetBindingReconciler) clusterToClusterResourceSetBinding
 func (r *ClusterResourceSetBindingReconciler) updateClusterReference(ctx context.Context, binding *addonsv1.ClusterResourceSetBinding) error {
 	patchHelper, err := patch.NewHelper(binding, r.Client)
 	if err != nil {
-		return errors.Wrap(err, "failed to configure the patch helper")
+		return err
 	}
 
 	// If the `.spec.clusterName` is not set, take the value from the ownerReference.

exp/internal/controllers/machinepool_controller_noderef.go

@@ -228,7 +228,7 @@ func (r *MachinePoolReconciler) patchNodes(ctx context.Context, c client.Client,
 		// Patch the node if needed.
 		if hasAnnotationChanges || hasTaintChanges {
 			if err := patchHelper.Patch(ctx, node); err != nil {
-				log.V(2).Info("Failed patch node to set annotations and drop taints", "err", err, "node name", node.Name)
+				log.V(2).Info("Failed patch Node to set annotations and drop taints", "err", err, "node name", node.Name)
 				return err
 			}
 		}

exp/runtime/internal/controllers/extensionconfig_controller.go

@@ -159,17 +159,13 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 func patchExtensionConfig(ctx context.Context, client client.Client, original, modified *runtimev1.ExtensionConfig, options ...patch.Option) error {
 	patchHelper, err := patch.NewHelper(original, client)
 	if err != nil {
-		return errors.Wrapf(err, "failed to create patch helper for %s", tlog.KObj{Obj: modified})
+		return err
 	}
 
 	options = append(options, patch.WithOwnedConditions{Conditions: []clusterv1.ConditionType{
 		runtimev1.RuntimeExtensionDiscoveredCondition,
 	}})
-	err = patchHelper.Patch(ctx, modified, options...)
-	if err != nil {
-		return errors.Wrapf(err, "failed to patch %s", tlog.KObj{Obj: modified})
-	}
-	return nil
+	return patchHelper.Patch(ctx, modified, options...)
 }
 
 // reconcileDelete will remove the ExtensionConfig from the registry on deletion of the object. Note this is a best

internal/controllers/clusterclass/clusterclass_controller.go

@@ -114,7 +114,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
 
 	patchHelper, err := patch.NewHelper(clusterClass, r.Client)
 	if err != nil {
-		return ctrl.Result{}, errors.Wrapf(err, "failed to create patch helper for %s", tlog.KObj{Obj: clusterClass})
+		return ctrl.Result{}, err
 	}
 
 	defer func() {
@@ -124,7 +124,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
 			patchOpts = append(patchOpts, patch.WithStatusObservedGeneration{})
 		}
 		if err := patchHelper.Patch(ctx, clusterClass, patchOpts...); err != nil {
-			reterr = kerrors.NewAggregate([]error{reterr, errors.Wrapf(err, "failed to patch %s", tlog.KObj{Obj: clusterClass})})
+			reterr = kerrors.NewAggregate([]error{reterr, err})
 			return
 		}
 	}()
@@ -374,7 +374,7 @@ func (r *Reconciler) reconcileExternal(ctx context.Context, clusterClass *cluste
 	// Initialize the patch helper.
 	patchHelper, err := patch.NewHelper(obj, r.Client)
 	if err != nil {
-		return errors.Wrapf(err, "failed to create patch helper for %s", tlog.KObj{Obj: obj})
+		return err
 	}
 
 	// Set external object ControllerReference to the ClusterClass.
@@ -383,11 +383,7 @@ func (r *Reconciler) reconcileExternal(ctx context.Context, clusterClass *cluste
 	}
 
 	// Patch the external object.
-	if err := patchHelper.Patch(ctx, obj); err != nil {
-		return errors.Wrapf(err, "failed to patch object %s", tlog.KObj{Obj: obj})
-	}
-
-	return nil
+	return patchHelper.Patch(ctx, obj)
 }
 
 func uniqueObjectRefKey(ref *corev1.ObjectReference) string {

internal/controllers/machinehealthcheck/machinehealthcheck_controller.go

@@ -148,7 +148,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
 	// Initialize the patch helper
 	patchHelper, err := patch.NewHelper(m, r.Client)
 	if err != nil {
-		log.Error(err, "Failed to build patch helper")
 		return ctrl.Result{}, err
 	}
 

internal/controllers/machinehealthcheck/machinehealthcheck_controller_test.go

@@ -2607,7 +2607,8 @@ func TestPatchTargets(t *testing.T) {
 	// To make the patch fail, create patchHelper with a different client.
 	fakeMachine := machine1.DeepCopy()
 	fakeMachine.Name = "fake"
-	patchHelper, _ := patch.NewHelper(fakeMachine, fake.NewClientBuilder().WithObjects(fakeMachine).Build())
+	patchHelper, err := patch.NewHelper(fakeMachine, fake.NewClientBuilder().WithObjects(fakeMachine).Build())
+	g.Expect(err).ToNot(HaveOccurred())
 	// healthCheckTarget with fake patchHelper, patch should fail on this target.
 	target1 := healthCheckTarget{
 		MHC:         mhc,
@@ -2617,7 +2618,8 @@ func TestPatchTargets(t *testing.T) {
 	}
 
 	// healthCheckTarget with correct patchHelper.
-	patchHelper2, _ := patch.NewHelper(machine2, cl)
+	patchHelper2, err := patch.NewHelper(machine2, cl)
+	g.Expect(err).ToNot(HaveOccurred())
 	target3 := healthCheckTarget{
 		MHC:         mhc,
 		Machine:     machine2,

internal/controllers/machinehealthcheck/machinehealthcheck_targets.go

@@ -212,7 +212,7 @@ func (r *Reconciler) getTargetsFromMHC(ctx context.Context, logger logr.Logger,
 
 		patchHelper, err := patch.NewHelper(&machines[k], r.Client)
 		if err != nil {
-			return nil, errors.Wrap(err, "unable to initialize patch helper")
+			return nil, err
 		}
 		target := healthCheckTarget{
 			Cluster:     cluster,

internal/controllers/machineset/machineset_controller.go

@@ -995,12 +995,12 @@ func (r *Reconciler) reconcileUnhealthyMachines(ctx context.Context, cluster *cl
 		for _, m := range machinesToRemediate {
 			patchHelper, err := patch.NewHelper(m, r.Client)
 			if err != nil {
-				errs = append(errs, errors.Wrapf(err, "failed to create patch helper for Machine %s", klog.KObj(m)))
+				errs = append(errs, err)
 				continue
 			}
 			conditions.MarkFalse(m, clusterv1.MachineOwnerRemediatedCondition, clusterv1.WaitingForRemediationReason, clusterv1.ConditionSeverityWarning, preflightCheckErrMessage)
 			if err := patchHelper.Patch(ctx, m); err != nil {
-				errs = append(errs, errors.Wrapf(err, "failed to patch Machine %s", klog.KObj(m)))
+				errs = append(errs, err)
 			}
 		}
 

internal/controllers/topology/cluster/cluster_controller.go

@@ -191,7 +191,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
 			patch.WithForceOverwriteConditions{},
 		}
 		if err := patchHelper.Patch(ctx, cluster, options...); err != nil {
-			reterr = kerrors.NewAggregate([]error{reterr, errors.Wrap(err, "failed to patch cluster")})
+			reterr = kerrors.NewAggregate([]error{reterr, err})
 			return
 		}
 	}()

internal/controllers/topology/machinedeployment/machinedeployment_controller.go

@@ -116,11 +116,11 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
 	// Create a patch helper to add or remove the finalizer from the MachineDeployment.
 	patchHelper, err := patch.NewHelper(md, r.Client)
 	if err != nil {
-		return ctrl.Result{}, errors.Wrapf(err, "failed to create patch helper for %s", tlog.KObj{Obj: md})
+		return ctrl.Result{}, err
 	}
 	defer func() {
 		if err := patchHelper.Patch(ctx, md); err != nil {
-			reterr = kerrors.NewAggregate([]error{reterr, errors.Wrapf(err, "failed to patch %s", tlog.KObj{Obj: md})})
+			reterr = kerrors.NewAggregate([]error{reterr, err})
 		}
 	}()
 

internal/controllers/topology/machineset/machineset_controller.go

@@ -123,11 +123,11 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
 	// Create a patch helper to add or remove the finalizer from the MachineSet.
 	patchHelper, err := patch.NewHelper(ms, r.Client)
 	if err != nil {
-		return ctrl.Result{}, errors.Wrapf(err, "failed to create patch helper for %s", tlog.KObj{Obj: ms})
+		return ctrl.Result{}, err
 	}
 	defer func() {
 		if err := patchHelper.Patch(ctx, ms); err != nil {
-			reterr = kerrors.NewAggregate([]error{reterr, errors.Wrapf(err, "failed to patch %s", tlog.KObj{Obj: ms})})
+			reterr = kerrors.NewAggregate([]error{reterr, err})
 		}
 	}()
 

internal/hooks/tracking.go

@@ -41,7 +41,7 @@ func MarkAsPending(ctx context.Context, c client.Client, obj client.Object, hook
 
 	patchHelper, err := patch.NewHelper(obj, c)
 	if err != nil {
-		return errors.Wrapf(err, "failed to mark %q hook(s) as pending: failed to create patch helper for %s", strings.Join(hookNames, ","), tlog.KObj{Obj: obj})
+		return errors.Wrapf(err, "failed to mark %q hook(s) as pending", strings.Join(hookNames, ","))
 	}
 
 	// Read the annotation of the objects and add the hook to the comma separated list
@@ -53,7 +53,7 @@ func MarkAsPending(ctx context.Context, c client.Client, obj client.Object, hook
 	obj.SetAnnotations(annotations)
 
 	if err := patchHelper.Patch(ctx, obj); err != nil {
-		return errors.Wrapf(err, "failed to mark %q hook(s) as pending: failed to patch %s", strings.Join(hookNames, ","), tlog.KObj{Obj: obj})
+		return errors.Wrapf(err, "failed to mark %q hook(s) as pending", strings.Join(hookNames, ","))
 	}
 
 	return nil
@@ -80,7 +80,7 @@ func MarkAsDone(ctx context.Context, c client.Client, obj client.Object, hooks .
 
 	patchHelper, err := patch.NewHelper(obj, c)
 	if err != nil {
-		return errors.Wrapf(err, "failed to mark %q hook(s) as done: failed to create patch helper for %s", strings.Join(hookNames, ","), tlog.KObj{Obj: obj})
+		return errors.Wrapf(err, "failed to mark %q hook(s) as done", strings.Join(hookNames, ","))
 	}
 
 	// Read the annotation of the objects and add the hook to the comma separated list
@@ -95,7 +95,7 @@ func MarkAsDone(ctx context.Context, c client.Client, obj client.Object, hooks .
 	obj.SetAnnotations(annotations)
 
 	if err := patchHelper.Patch(ctx, obj); err != nil {
-		return errors.Wrapf(err, "failed to mark %q hook(s) as done: failed to patch %s", strings.Join(hookNames, ","), tlog.KObj{Obj: obj})
+		return errors.Wrapf(err, "failed to mark %q hook(s) as done", strings.Join(hookNames, ","))
 	}
 
 	return nil
@@ -117,7 +117,7 @@ func IsOkToDelete(obj client.Object) bool {
 func MarkAsOkToDelete(ctx context.Context, c client.Client, obj client.Object) error {
 	patchHelper, err := patch.NewHelper(obj, c)
 	if err != nil {
-		return errors.Wrapf(err, "failed to mark %s as ok to delete: failed to create patch helper", tlog.KObj{Obj: obj})
+		return errors.Wrapf(err, "failed to mark %s as ok to delete", tlog.KObj{Obj: obj})
 	}
 
 	annotations := obj.GetAnnotations()
@@ -128,7 +128,7 @@ func MarkAsOkToDelete(ctx context.Context, c client.Client, obj client.Object) e
 	obj.SetAnnotations(annotations)
 
 	if err := patchHelper.Patch(ctx, obj); err != nil {
-		return errors.Wrapf(err, "failed to mark %s as ok to delete: failed to patch", tlog.KObj{Obj: obj})
+		return errors.Wrapf(err, "failed to mark %s as ok to delete", tlog.KObj{Obj: obj})
 	}
 
 	return nil

test/framework/autoscaler_helpers.go

@@ -277,7 +277,7 @@ func DisableAutoscalerForMachineDeploymentTopologyAndWait(ctx context.Context, i
 
 	log.Logf("Dropping the %s and %s annotations from the MachineDeployments in ClusterTopology", clusterv1.AutoscalerMinSizeAnnotation, clusterv1.AutoscalerMaxSizeAnnotation)
 	patchHelper, err := patch.NewHelper(input.Cluster, mgmtClient)
-	Expect(err).ToNot(HaveOccurred(), fmt.Sprintf("failed to create patch helper for Cluster %s", klog.KObj(input.Cluster)))
+	Expect(err).ToNot(HaveOccurred())
 	for i := range input.Cluster.Spec.Topology.Workers.MachineDeployments {
 		md := input.Cluster.Spec.Topology.Workers.MachineDeployments[i]
 		delete(md.Metadata.Annotations, clusterv1.AutoscalerMinSizeAnnotation)
@@ -321,7 +321,7 @@ func EnableAutoscalerForMachineDeploymentTopologyAndWait(ctx context.Context, in
 
 	log.Logf("Add the %s and %s annotations to the MachineDeployments in ClusterTopology", clusterv1.AutoscalerMinSizeAnnotation, clusterv1.AutoscalerMaxSizeAnnotation)
 	patchHelper, err := patch.NewHelper(input.Cluster, mgmtClient)
-	Expect(err).ToNot(HaveOccurred(), fmt.Sprintf("failed to create patch helper for Cluster %s", klog.KObj(input.Cluster)))
+	Expect(err).ToNot(HaveOccurred())
 	for i := range input.Cluster.Spec.Topology.Workers.MachineDeployments {
 		md := input.Cluster.Spec.Topology.Workers.MachineDeployments[i]
 		if md.Metadata.Annotations == nil {