fix(workloadmanager): check CodeInterpreter child ownership - #450
fix(workloadmanager): check CodeInterpreter child ownership#450avinxshKD wants to merge 4 commits into
Conversation
Signed-off-by: Avinash Kumar Deepak <avinash8655279@gmail.com>
Signed-off-by: Avinash Kumar Deepak <avinash8655279@gmail.com>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@ranxi2001 @acsoto ptal when u get chance, this prevents the CodeInterpreter controller from modifying or deleting same-named resources it doesn’t own. |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #450 +/- ##
==========================================
+ Coverage 58.41% 61.25% +2.83%
==========================================
Files 36 36
Lines 3463 3613 +150
==========================================
+ Hits 2023 2213 +190
+ Misses 1231 1171 -60
- Partials 209 229 +20
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| return err | ||
| } | ||
|
|
||
| if err := r.Delete(ctx, sandboxTemplate); err != nil { |
There was a problem hiding this comment.
[Medium] Bind the ownership check to the object being deleted
Both delete helpers first GET the child and confirm metav1.IsControlledBy, but r.Delete(ctx, child) sends no UID or resourceVersion preconditions. Passing the fetched object to Delete does not automatically add those fields to DeleteOptions, so with another authorized writer the checked object and the deleted object can differ:
sequenceDiagram
participant WM as CodeInterpreter controller
participant API as Kubernetes API
participant Other as Concurrent authorized writer
WM->>API: GET child "test"
API-->>WM: A (UID 111, RV 10, owned by this CodeInterpreter)
Note over WM: IsControlledBy(A) is true
Other->>API: Delete A
Other->>API: Create B with the same name (UID 222, not owned)
alt Current: DELETE has no preconditions
WM->>API: DELETE "test" by namespace/name
API-->>WM: Delete the current object B
else Requested: bind DELETE to observed identity/version
WM->>API: DELETE "test" with UID 111 and RV 10
API-->>WM: Reject the stale request, then reconcile re-reads
end
This is a source-proven latent race, not an observed failure: it requires a concurrent authorized writer to replace or mutate the child after the GET. Even so, the final DELETE can violate this PR's ownership invariant.
Could both delete helpers pass the observed UID and resourceVersion through client.Preconditions and return a precondition conflict so reconciliation re-reads the child? Please also add focused tests for both SandboxTemplate and SandboxWarmPool that replace or mutate the object between GET and DELETE.
There was a problem hiding this comment.
good catch, both delete paths now use UID and resourceVersion preconditions, with replacement-race tests for SandboxTemplate and SandboxWarmPool. PTAL. @ranxi2001
Signed-off-by: Avinash Kumar Deepak <avinash8655279@gmail.com>
Signed-off-by: Avinash Kumar Deepak <avinash8655279@gmail.com>
*What type of PR is this?
/kind bug
What this PR does / why we need it:
Prevents the CodeInterpreter controller from updating or deleting same-named
SandboxTemplateandSandboxWarmPoolresources that it does not ownOwnership conflicts now fail reconciliation and set the CodeInterpreter Ready condition to
Falsewith reasonOwnershipConflict.Which issue(s) this PR fixes:
Fixes #449
Special notes for your reviewer:
AlreadyExistsfrom child creation is now returned so a concurrent name collision is retried and checked instead of being treated as success.Does this PR introduce a user-facing change?: