Verify controller ownership before GetForRole/Scale/legacy-slice0 mutate an LWS (#983)
* Verify controller ownership before GetForRole/Scale/legacy-slice0/Create mutate an LWS GetForRole and Scale looked up an LWS by its generated name only, with no check that the found object is actually controller-owned by the DisaggregatedSet calling them. During rapid same-name recreation (a DS named "foo" is deleted and a new one also named "foo" is created before garbage collection removes the old one's LWS objects), the new controller's reconcileRoleSimple could find and then Scale the old "foo"'s still-present LWS - mutating an object it doesn't own. This is the same class of bug already fixed for List via metav1.IsControlledBy; GetForRole/Scale are a separate code path that fix didn't cover. recreateLegacySlice0 had the identical gap: it fetched a legacy-named LWS by raw Get with no ownership check before deleting it to make way for the slice-aware migration. - Added LeaderWorkerSetManager.getOwned, an ownership-filtered wrapper around Get (mirrors List's existing filtering). GetForRole now uses it for both the slice-aware and legacy-name lookups; a foreign-owned LWS at either name is treated as absent rather than returned. - Scale now takes the DisaggregatedSet (instead of a bare namespace) and refuses to patch a same-named LWS that isn't controller-owned by it, returning an error instead of silently mutating a foreign object. Updated all three call sites (reconcileRoleSimple, scaleUpNew, and the scale-down path in executor.go). - recreateLegacySlice0 uses getOwned instead of a raw Get before deleting a legacy-named LWS. - Create's AlreadyExists handling now checks ownership too: if the taken name is owned by this DisaggregatedSet, it's a benign concurrent-reconcile race (no-op, as before); if foreign-owned, it now errors instead of silently returning success, so the reconcile requeues. A silent no-op here would otherwise be permanent: this DS's owned-object watches never fire for a foreign object it doesn't own, so nothing would ever prompt another attempt once GC clears the name. Test fixtures across the package that built LWS objects without a matching controller OwnerReference are updated to carry one (matching what the real Create() path always sets), since the new ownership checks depend on it. Added regression tests covering each fixed path: a same-named LWS owned by a different DisaggregatedSet is ignored or refused as appropriate, and normal operation is unaffected when ownership is intact. * Fix unparam lint findings in lws_manager_test.go buildManagerTestLWS's name and replicas parameters had become constant across all remaining callers after the ownership-check test updates, tripping golangci-lint's unparam check. Dropped both parameters and hardcoded the values instead of threading through arguments nothing varies. * Fold ownership filtering into Get itself; fixes a fourth unguarded call site Per @ahg-g's review: List already filters by controller ownership, so Get should behave the same way instead of requiring callers to reach for a separate getOwned helper. Get now takes the DisaggregatedSet and checks metav1.IsControlledBy directly; getOwned is removed and GetForRole/recreateLegacySlice0 now just call Get. Making Get itself ownership-aware also caught a fourth, previously unnoticed instance of the same bug class this issue is about: executor.go's ensureNewLWSExists did the identical unguarded check-then-create against a raw Get, so a foreign-owned LWS at the generated name would have made it wrongly conclude "already exists, nothing to do" - permanently skipping creation for that role, the same failure mode GetForRole/Scale had. Fixed for free by the signature change; updated its one call site. Updated all Get callers (test and production) to the new signature. One test fixture in TestEnsureNewLWSExists used a UID that didn't match the file's shared owner-reference convention, which the ownership check now surfaces - aligned it. Existing tests otherwise cover the behavior; no new tests needed since Get's contract itself didn't change, only where the check lives.
B
Ben Ayed Mohamed Ala committed
382435997e4eb436e27a2d5c43d4679b44a064a7
Parent: c29562d
Committed by GitHub <noreply@github.com>
on 8/18/2026, 6:09:11 PM