|
| 1 | +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. |
| 2 | +// SPDX-License-Identifier: Apache-2.0 |
| 3 | + |
| 4 | +package controllerutil |
| 5 | + |
| 6 | +import ( |
| 7 | + "context" |
| 8 | + "fmt" |
| 9 | + "reflect" |
| 10 | + |
| 11 | + "sigs.k8s.io/controller-runtime/pkg/client" |
| 12 | +) |
| 13 | + |
| 14 | +// MutateAndPatchSpec captures the current state of obj, applies mutate, and |
| 15 | +// patches the object using a JSON merge patch with optimistic concurrency. |
| 16 | +// A concurrent writer that advances resourceVersion between our read and our |
| 17 | +// Patch triggers a 409 Conflict; controller-runtime then re-Gets, recomputes |
| 18 | +// the diff, and writes on a fresh view — preserving cross-writer coexistence |
| 19 | +// on the same resource. |
| 20 | +// |
| 21 | +// This is the canonical idiom for every spec or metadata write on a CR that |
| 22 | +// another controller may also write (see #4767). A full PUT (r.Update) is a |
| 23 | +// bug trap: any field the operator's local copy does not track — most |
| 24 | +// importantly spec.authzConfig on MCPServer, which a separate authorization |
| 25 | +// controller will own — is zeroed on every reconcile. A merge-patch body |
| 26 | +// only carries fields the caller actually changed, so untouched fields never |
| 27 | +// hit the wire and cannot be clobbered. MergeFromWithOptimisticLock sends |
| 28 | +// resourceVersion as a precondition, giving 409-on-collision semantics for |
| 29 | +// concurrent writers and defending metadata.finalizers (which has no |
| 30 | +// array-merge semantics under RFC 7396 merge-patch) against wholesale |
| 31 | +// replacement when another controller is mid-flight adding its own entry. |
| 32 | +// |
| 33 | +// Unlike MutateAndPatchStatus, this helper does NOT short-circuit on an |
| 34 | +// empty diff. MergeFromWithOptimisticLock always emits metadata.resourceVersion |
| 35 | +// into the patch body, so the status helper's "body == {}" check never fires; |
| 36 | +// and every current call site carries a real mutation (finalizer add/remove, |
| 37 | +// annotation stamp), so there is no no-op caller to optimize for. |
| 38 | +// |
| 39 | +// Do NOT use for status writes. Status-subresource writes are scoped to the |
| 40 | +// status stanza, and forcing a 409 on every disjoint-field overlap would |
| 41 | +// produce permanent churn with nothing gained — use MutateAndPatchStatus. |
| 42 | +// |
| 43 | +// If Patch returns an error, obj has already been mutated; callers must |
| 44 | +// re-fetch obj before retrying rather than reusing the modified in-memory |
| 45 | +// copy. The standard reconciler pattern — returning the error so |
| 46 | +// controller-runtime requeues with a fresh Get — is the correct retry path. |
| 47 | +// |
| 48 | +// Typical usage: |
| 49 | +// |
| 50 | +// err := ctrlutil.MutateAndPatchSpec(ctx, r.Client, mcpServer, |
| 51 | +// func(m *mcpv1beta1.MCPServer) { |
| 52 | +// controllerutil.AddFinalizer(m, MCPServerFinalizerName) |
| 53 | +// }) |
| 54 | +// if err != nil { |
| 55 | +// return ctrl.Result{}, err |
| 56 | +// } |
| 57 | +// |
| 58 | +// Expect 409s as routine log noise once external writers land — the guard |
| 59 | +// doing its job, not a bug. |
| 60 | +func MutateAndPatchSpec[T client.Object]( |
| 61 | + ctx context.Context, c client.Client, obj T, mutate func(T), |
| 62 | +) error { |
| 63 | + // Reject both a true-nil interface and a typed-nil pointer. T is |
| 64 | + // constrained to client.Object; every real implementer is a pointer |
| 65 | + // to a struct, so a nil obj is always a programmer error. Returning |
| 66 | + // an explicit error is nicer than the raw panic that the subsequent |
| 67 | + // .(T) type assertion would produce. |
| 68 | + v := reflect.ValueOf(obj) |
| 69 | + if !v.IsValid() || (v.Kind() == reflect.Pointer && v.IsNil()) { |
| 70 | + return fmt.Errorf("MutateAndPatchSpec: obj must be non-nil") |
| 71 | + } |
| 72 | + original := obj.DeepCopyObject().(T) |
| 73 | + mutate(obj) |
| 74 | + return c.Patch(ctx, obj, client.MergeFromWithOptions( |
| 75 | + original, client.MergeFromWithOptimisticLock{})) |
| 76 | +} |
0 commit comments