stacklok / toolhive / 24777693100

push

github

Patch MCPServer spec instead of Update (#4914)

Fixes #4767.

The controller writes finalizers, finalizer removal, and the
restart-processed annotation via r.Update. Update is a full PUT, so
any spec field the operator does not track — most importantly
spec.authzConfig, which a separate authorization controller will soon
own — is zeroed on every reconcile.

Replace the three Update call sites with an optimistic-lock merge
patch. The merge-patch body carries only fields the caller changed,
so untouched fields never hit the wire and cannot be clobbered.
MergeFromWithOptimisticLock sends resourceVersion as a precondition,
giving 409-on-collision semantics for concurrent writers and
defending metadata.finalizers (which has no array-merge semantics
under merge-patch) against wholesale replacement when another
controller is mid-flight adding its own entry.

Tests:

- Envtest suite writes spec.authzConfig out-of-band and asserts it
  survives both the finalizer-add reconcile and the
  restart-annotation reconcile.
- Unit suite uses a patch-recording client to assert each migrated
  call site emits a body carrying the resourceVersion precondition
  — a deterministic wire-level signal that
  MergeFromWithOptimisticLock is in effect. A regression to plain
  MergeFrom would drop the precondition and fail the assertion
  independent of the higher-level survival test.

Also:

- .claude/rules/operator.md: new "Spec / metadata patching" section
  documenting the pattern for future CR writes. Status patching is
  a separate follow-up (#4633).
- Rename the mock-client flag failOnMCPServerUpdate →
  failOnMCPServerWrite; it now intercepts both Update and Patch on
  MCPServer, so the name matches reality.

31 of 33 new or added lines in 3 files covered. (93.94%)

58675 of 88521 relevant lines covered (66.28%)

62.79 hits per line