Skip to content

fix(workloadmanager): check CodeInterpreter child ownership #450

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
avinxshKD wants to merge 4 commits into
base: main
Choose a base branch
from
+256 −18
Open

fix(workloadmanager): check CodeInterpreter child ownership #450

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3c952a7
fix(workloadmanager): check CodeInterpreter child ownership
avinxshKD Aug 2, 2026
f722b51
fix(workloadmanager): report ownership conflicts
avinxshKD Aug 2, 2026
2c5f667
fix(workloadmanager): bind child deletes to observed objects
avinxshKD Aug 3, 2026
0b646d8
test(workloadmanager): simplify replacement race setup
avinxshKD Aug 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 41 additions & 14 deletions pkg/workloadmanager/codeinterpreter_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ func (r *CodeInterpreterReconciler) Reconcile(ctx context.Context, req ctrl.Requ
}

// Update status with ready condition
if err := r.updateStatus(ctx, codeInterpreter); err != nil {
if err := r.updateStatus(ctx, codeInterpreter, true, "Reconciled", "CodeInterpreter is ready"); err != nil {
logger.Error(err, "failed to update status")
return ctrl.Result{}, err
}
Expand All @@ -102,30 +102,49 @@ func (r *CodeInterpreterReconciler) Reconcile(ctx context.Context, req ctrl.Requ
// updateStatus updates the CodeInterpreter status. It skips the API write
// when the status is already up-to-date to avoid triggering a new watch event
// that would re-enqueue the object unnecessarily.
func (r *CodeInterpreterReconciler) updateStatus(ctx context.Context, ci *runtimev1alpha1.CodeInterpreter) error {
func (r *CodeInterpreterReconciler) updateStatus(ctx context.Context, ci *runtimev1alpha1.CodeInterpreter, ready bool, reason, message string) error {
conditionStatus := metav1.ConditionFalse
if ready {
conditionStatus = metav1.ConditionTrue
}

existing := apimeta.FindStatusCondition(ci.Status.Conditions, "Ready")
if ci.Status.Ready &&
if ci.Status.Ready == ready &&
existing != nil &&
existing.Status == metav1.ConditionTrue &&
existing.Status == conditionStatus &&
existing.Reason == reason &&
existing.Message == message &&
existing.ObservedGeneration == ci.Generation {
return nil
}

ci.Status.Ready = true
ci.Status.Ready = ready
// SetStatusCondition only updates LastTransitionTime when the condition
// Status actually changes, preventing spurious status writes that would
// trigger an infinite reconciliation loop.
apimeta.SetStatusCondition(&ci.Status.Conditions, metav1.Condition{
Type: "Ready",
Status: metav1.ConditionTrue,
Reason: "Reconciled",
Message: "CodeInterpreter is ready",
Status: conditionStatus,
Reason: reason,
Message: message,
ObservedGeneration: ci.Generation,
})

return r.Status().Update(ctx, ci)
}

func (r *CodeInterpreterReconciler) validateChildOwnership(ctx context.Context, ci *runtimev1alpha1.CodeInterpreter, child metav1.Object, kind string) error {
if metav1.IsControlledBy(child, ci) {
return nil
}

ownershipErr := fmt.Errorf("existing %s %s/%s is not controlled by CodeInterpreter %s", kind, child.GetNamespace(), child.GetName(), ci.Name)
if err := r.updateStatus(ctx, ci, false, "OwnershipConflict", ownershipErr.Error()); err != nil {
return fmt.Errorf("%v: failed to update CodeInterpreter status: %w", ownershipErr, err)
}
return ownershipErr
}

// ensureSandboxTemplate ensures that a SandboxTemplate exists for this CodeInterpreter
func (r *CodeInterpreterReconciler) ensureSandboxTemplate(ctx context.Context, ci *runtimev1alpha1.CodeInterpreter) (ctrl.Result, error) {
logger := log.FromContext(ctx)
Expand Down Expand Up @@ -168,14 +187,15 @@ func (r *CodeInterpreterReconciler) ensureSandboxTemplate(ctx context.Context, c
}

if err := r.Create(ctx, sandboxTemplate); err != nil {
if !errors.IsAlreadyExists(err) {
return ctrl.Result{}, fmt.Errorf("failed to create SandboxTemplate: %w", err)
}
return ctrl.Result{}, fmt.Errorf("failed to create SandboxTemplate: %w", err)
}
return ctrl.Result{}, nil
} else if err != nil {
return ctrl.Result{}, fmt.Errorf("failed to get SandboxTemplate: %w", err)
}
if err := r.validateChildOwnership(ctx, ci, sandboxTemplate, "SandboxTemplate"); err != nil {
return ctrl.Result{}, err
}

// Update existing SandboxTemplate if needed.
needsUpdate := false
Expand Down Expand Up @@ -228,14 +248,15 @@ func (r *CodeInterpreterReconciler) ensureSandboxWarmPool(ctx context.Context, c
}

if err := r.Create(ctx, warmPool); err != nil {
if !errors.IsAlreadyExists(err) {
return fmt.Errorf("failed to create SandboxWarmPool: %w", err)
}
return fmt.Errorf("failed to create SandboxWarmPool: %w", err)
}
return nil
} else if err != nil {
return fmt.Errorf("failed to get SandboxWarmPool: %w", err)
}
if err := r.validateChildOwnership(ctx, ci, warmPool, "SandboxWarmPool"); err != nil {
return err
}

// Update existing SandboxWarmPool if needed
needsUpdate := false
Expand Down Expand Up @@ -267,6 +288,9 @@ func (r *CodeInterpreterReconciler) deleteSandboxWarmPool(ctx context.Context, c
} else if err != nil {
return fmt.Errorf("failed to get SandboxWarmPool: %w", err)
}
if err := r.validateChildOwnership(ctx, ci, warmPool, "SandboxWarmPool"); err != nil {
return err
}

if err := r.Delete(ctx, warmPool); err != nil {
if !errors.IsNotFound(err) {
Expand All @@ -287,6 +311,9 @@ func (r *CodeInterpreterReconciler) deleteSandboxTemplate(ctx context.Context, c
} else if err != nil {
return fmt.Errorf("failed to get SandboxTemplate: %w", err)
}
if err := r.validateChildOwnership(ctx, ci, sandboxTemplate, "SandboxTemplate"); err != nil {
return err
}

if err := r.Delete(ctx, sandboxTemplate); err != nil {

@ranxi2001 ranxi2001 Aug 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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
Loading

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.

@avinxshKD avinxshKD Aug 3, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch, both delete paths now use UID and resourceVersion preconditions, with replacement-race tests for SandboxTemplate and SandboxWarmPool. PTAL. @ranxi2001

if !errors.IsNotFound(err) {
Expand Down
126 changes: 124 additions & 2 deletions pkg/workloadmanager/codeinterpreter_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (

"github.com/stretchr/testify/assert"
corev1 "k8s.io/api/core/v1"
apimeta "k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
Expand Down Expand Up @@ -53,7 +54,11 @@ func newTestReconcilerWithObjects(objects ...runtime.Object) *CodeInterpreterRec
_ = extensionsv1alpha1.AddToScheme(scheme)
_ = corev1.AddToScheme(scheme)

client := fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(objects...).Build()
client := fake.NewClientBuilder().
WithScheme(scheme).
WithStatusSubresource(&runtimev1alpha1.CodeInterpreter{}).
WithRuntimeObjects(objects...).
Build()

return &CodeInterpreterReconciler{
Client: client,
Expand All @@ -71,6 +76,7 @@ func testCodeInterpreterWithWarmPool() *runtimev1alpha1.CodeInterpreter {
ObjectMeta: metav1.ObjectMeta{
Name: "test-code-interpreter",
Namespace: "default",
UID: "test-code-interpreter",
},
Spec: runtimev1alpha1.CodeInterpreterSpec{
AuthMode: runtimev1alpha1.AuthModeNone,
Expand Down Expand Up @@ -105,6 +111,9 @@ func TestEnsureSandboxTemplateUpdatesManagedNetworkPolicyToUnmanaged(t *testing.
ObjectMeta: metav1.ObjectMeta{
Name: ci.Name,
Namespace: ci.Namespace,
OwnerReferences: []metav1.OwnerReference{
*metav1.NewControllerRef(ci, runtimev1alpha1.GroupVersion.WithKind("CodeInterpreter")),
},
},
Spec: extensionsv1alpha1.SandboxTemplateSpec{
NetworkPolicyManagement: extensionsv1alpha1.NetworkPolicyManagementManaged,
Expand All @@ -118,7 +127,7 @@ func TestEnsureSandboxTemplateUpdatesManagedNetworkPolicyToUnmanaged(t *testing.
},
},
}
reconciler := newTestReconcilerWithObjects(existing)
reconciler := newTestReconcilerWithObjects(ci, existing)

_, err := reconciler.ensureSandboxTemplate(context.Background(), ci)
assert.NoError(t, err)
Expand All @@ -132,6 +141,119 @@ func TestEnsureSandboxTemplateUpdatesManagedNetworkPolicyToUnmanaged(t *testing.
assert.Equal(t, extensionsv1alpha1.NetworkPolicyManagementUnmanaged, sandboxTemplate.Spec.NetworkPolicyManagement)
}

func TestEnsureSandboxTemplateRejectsUnownedTemplate(t *testing.T) {
ci := testCodeInterpreterWithWarmPool()
existing := &extensionsv1alpha1.SandboxTemplate{
ObjectMeta: metav1.ObjectMeta{
Name: ci.Name,
Namespace: ci.Namespace,
},
Spec: extensionsv1alpha1.SandboxTemplateSpec{
PodTemplate: sandboxv1alpha1.PodTemplate{
Spec: corev1.PodSpec{
Containers: []corev1.Container{{
Name: "existing",
Image: "existing-image",
}},
},
},
},
}
reconciler := newTestReconcilerWithObjects(ci, existing)

_, err := reconciler.ensureSandboxTemplate(context.Background(), ci)
assert.ErrorContains(t, err, "is not controlled by CodeInterpreter")

sandboxTemplate := &extensionsv1alpha1.SandboxTemplate{}
err = reconciler.Get(context.Background(), types.NamespacedName{
Name: ci.Name,
Namespace: ci.Namespace,
}, sandboxTemplate)
assert.NoError(t, err)
assert.Equal(t, "existing-image", sandboxTemplate.Spec.PodTemplate.Spec.Containers[0].Image)

storedCI := &runtimev1alpha1.CodeInterpreter{}
err = reconciler.Get(context.Background(), types.NamespacedName{
Name: ci.Name,
Namespace: ci.Namespace,
}, storedCI)
assert.NoError(t, err)
condition := apimeta.FindStatusCondition(storedCI.Status.Conditions, "Ready")
if assert.NotNil(t, condition) {
assert.Equal(t, metav1.ConditionFalse, condition.Status)
assert.Equal(t, "OwnershipConflict", condition.Reason)
}
}

func TestEnsureSandboxWarmPoolRejectsUnownedWarmPool(t *testing.T) {
ci := testCodeInterpreterWithWarmPool()
existing := &extensionsv1alpha1.SandboxWarmPool{
ObjectMeta: metav1.ObjectMeta{
Name: ci.Name,
Namespace: ci.Namespace,
},
Spec: extensionsv1alpha1.SandboxWarmPoolSpec{
Replicas: 7,
TemplateRef: extensionsv1alpha1.SandboxTemplateRef{
Name: "existing-template",
},
},
}
reconciler := newTestReconcilerWithObjects(ci, existing)

err := reconciler.ensureSandboxWarmPool(context.Background(), ci)
assert.ErrorContains(t, err, "is not controlled by CodeInterpreter")

warmPool := &extensionsv1alpha1.SandboxWarmPool{}
err = reconciler.Get(context.Background(), types.NamespacedName{
Name: ci.Name,
Namespace: ci.Namespace,
}, warmPool)
assert.NoError(t, err)
assert.Equal(t, int32(7), warmPool.Spec.Replicas)
assert.Equal(t, "existing-template", warmPool.Spec.TemplateRef.Name)
}

func TestDeleteSandboxTemplateRejectsUnownedTemplate(t *testing.T) {
ci := testCodeInterpreterWithWarmPool()
existing := &extensionsv1alpha1.SandboxTemplate{
ObjectMeta: metav1.ObjectMeta{
Name: ci.Name,
Namespace: ci.Namespace,
},
}
reconciler := newTestReconcilerWithObjects(ci, existing)

err := reconciler.deleteSandboxTemplate(context.Background(), ci)
assert.ErrorContains(t, err, "is not controlled by CodeInterpreter")

err = reconciler.Get(context.Background(), types.NamespacedName{
Name: ci.Name,
Namespace: ci.Namespace,
}, &extensionsv1alpha1.SandboxTemplate{})
assert.NoError(t, err)
}

func TestDeleteSandboxWarmPoolRejectsUnownedWarmPool(t *testing.T) {
ci := testCodeInterpreterWithWarmPool()
existing := &extensionsv1alpha1.SandboxWarmPool{
ObjectMeta: metav1.ObjectMeta{
Name: ci.Name,
Namespace: ci.Namespace,
},
}
reconciler := newTestReconcilerWithObjects(ci, existing)

err := reconciler.deleteSandboxWarmPool(context.Background(), ci)
assert.ErrorContains(t, err, "is not controlled by CodeInterpreter")

err = reconciler.Get(context.Background(), types.NamespacedName{
Name: ci.Name,
Namespace: ci.Namespace,
}, &extensionsv1alpha1.SandboxWarmPool{})
assert.NoError(t, err)
}

func TestConvertToPodTemplate_RuntimeClassName_TableDriven(t *testing.T) {
reconciler := setupTestReconciler()

Expand Down