sandbox_controller: get pod name from sandbox annotations on deletion

[Iceber]

agent-sandbox/controllers/sandbox_controller.go

Lines 314 to 320 in 7ed7d38

	podName := sandbox.Name
	var trackedPodName string
	var podNameAnnotationExists bool
	if trackedPodName, podNameAnnotationExists = sandbox.Annotations[SandboxPodNameAnnotation]; podNameAnnotationExists && trackedPodName != "" {
	podName = trackedPodName
	log.Info("Using tracked pod name from sandbox annotation", "podName", podName)
	}

consistent with pod assignment

ref #265

[netlify]

✅ Deploy Preview for agent-sandbox canceled.

Name	Link
🔨 Latest commit	13aa72c
🔍 Latest deploy log	https://app.netlify.com/projects/agent-sandbox/deploys/694e5be34f4da70008b62e41

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Iceber
Once this PR has been reviewed and has the lgtm label, please assign janetkuo for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[k8s-ci-robot]

Hi @Iceber. Thanks for your PR.

I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[barney-s]

One is the logging name.
The other is the deletion bug.
Wondering if there were no unittests for this case ?
or e2e ?

@sdowell FYI

[barney-s]

Could we add a unit-test that fails without this fix ?

[barney-s]

/ok-to-test

[Iceber]

Could we add a unit-test that fails without this fix ?

@barney-s I added a simple test, PTAL, Thanks

[janetkuo]

If a malicious user has permissions to edit Sandbox annotations but not to delete arbitrary Pods in the namespace, they could set annotations on their Sandbox to indirectly delete any target Pod they want (in the same namespace).

Before deleting the Pod, Get it and verify its ControllerRef (OwnerReferences with controller=true) point to this Sandbox.

[janetkuo]

This variable declaration shadows the imported log package. It's safer and cleaner to rename it to avoid confusion and potential scope issues.

[janetkuo]

What if the pod annotation is empty? Should we fall back to use the sandbox name, and/or produce a warning?

FYI: I found an existing bug (#228) while reviewing this change. Sending a fix here #229

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 30d of inactivity, lifecycle/stale is applied
• After 15d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle stale
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to us at Agent Sandbox.

/lifecycle stale

[k8s-ci-robot]

@Iceber: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
presubmit-agent-sandbox-lint-api	13aa72c	link	true	/test presubmit-agent-sandbox-lint-api

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

[barney-s]

please rebase @Iceber

[barney-s]

and fix the linter error @Iceber

[codebot-robot]

Overall, the PR effectively aligns the deletion logic with the pod assignment changes, correctly addressing the deletion of adopted pods upon sandbox expiry by reading the tracked pod name from annotations. However, I've left several comments focused on maintainability, safety, and test coverage:

• Extract duplicated logic for determining the Pod name into a helper function to keep the codebase DRY.
• Enhance r.Delete safety by using Preconditions to prevent race conditions during Pod deletion.
• Standardize structured logging keys and error messages to include the dynamic pod name.
• Expand test coverage to include edge cases like ShutdownPolicyDelete.
• Improve test rigor by asserting boolean return values and status updates, verifying that the "un-annotated" pod is not inadvertently deleted (using negative assertions), and enhancing test failure visibility (e.g., avoiding require.True for error checking and improving failure context messages).

(This review was generated by Overseer)