From da43c5b27bad1f2d4cbe4f117fe317af27f8d89a Mon Sep 17 00:00:00 2001 From: Olaf Klischat Date: Tue, 31 Mar 2026 23:45:26 +0200 Subject: [PATCH] change ProviderData.Update() semantics to only modify the in-memory representation on successful updates Previously, if the modifier did not change anything, the function would still as a side effect fill the passed-in machine struct with the latest version from the controller-runtime cache, which might be arbitrarily outdated. So e.g. if you called this function twice in a row, the first call might've performed an update and filled the struct with the latest state of the machine from the cluster, but then the next call might've performed no update and filled the struct with an outdated state, potentially behind even what it was before the two calls. This could lead to hard to find bugs because callers generally don't expect these semantics and proceed working with the returned outdated struct. This changes the function so the machine struct that the caller passes in is only modified if the update succeded, in which case the structure is filled with the latest state from the cluster, post-update. If the modifier didn't change anything or the update failed, the struct is left untouched. Signed-off-by: Olaf Klischat --- pkg/cloudprovider/types/types.go | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/pkg/cloudprovider/types/types.go b/pkg/cloudprovider/types/types.go index e9e3c241e..9c237de2e 100644 --- a/pkg/cloudprovider/types/types.go +++ b/pkg/cloudprovider/types/types.go @@ -99,20 +99,27 @@ func GetMachineUpdater(ctx context.Context, client ctrlruntimeclient.Client) Mac // Store name here, because the machine can be nil if an update failed. namespacedName := types.NamespacedName{Namespace: machine.Namespace, Name: machine.Name} return retry.RetryOnConflict(retry.DefaultBackoff, func() error { - if err := client.Get(ctx, namespacedName, machine); err != nil { + cachedMachine := &clusterv1alpha1.Machine{} + if err := client.Get(ctx, namespacedName, cachedMachine); err != nil { return fmt.Errorf("failed to get machine: %w", err) } // Check if we actually change something and only update if that is the case. - unmodifiedMachine := machine.DeepCopy() + unmodifiedMachine := cachedMachine.DeepCopy() for _, modify := range modifiers { - modify(machine) + modify(cachedMachine) } - if equality.Semantic.DeepEqual(unmodifiedMachine, machine) { + if equality.Semantic.DeepEqual(unmodifiedMachine, cachedMachine) { return nil } - return client.Update(ctx, machine) + if err := client.Update(ctx, cachedMachine); err != nil { + return err + } + + cachedMachine.DeepCopyInto(machine) + + return nil }) } }