Skip to content

fix: nil guard SandboxConditionPaused in calculateStatus#572

Open
ashnaaseth2325-oss wants to merge 1 commit into
openkruise:masterfrom
ashnaaseth2325-oss:fix/sandbox-paused-nil-cond
Open

fix: nil guard SandboxConditionPaused in calculateStatus#572
ashnaaseth2325-oss wants to merge 1 commit into
openkruise:masterfrom
ashnaaseth2325-oss:fix/sandbox-paused-nil-cond

Conversation

@ashnaaseth2325-oss

Copy link
Copy Markdown

SUMMARY

This PR adds nil checks before accessing SandboxConditionPaused in the Sandbox controller's status calculation logic. It prevents a potential nil pointer dereference when a Sandbox is in the Paused phase but the corresponding paused condition is absent.

The change is limited to pkg/controller/sandbox/sandbox_controller.go, specifically the calculateStatus() logic for the SandboxPaused state.

FIX

// Before
if cond.Status == metav1.ConditionTrue && !box.Spec.Paused {
    ...
} else if !box.Spec.Paused && cond.Status == metav1.ConditionFalse {
    ...
}

// After
if cond != nil && cond.Status == metav1.ConditionTrue && !box.Spec.Paused {
    ...
} else if cond != nil && !box.Spec.Paused && cond.Status == metav1.ConditionFalse {
    ...
}

Signed-off-by: ashnaaseth2325-oss <ashnaaseth2325@gmail.com>
@kruise-bot

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign zmberg for approval by writing /assign @zmberg in a comment. For more information see:The Kubernetes 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:

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

@codecov

codecov Bot commented Jun 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.78%. Comparing base (901921b) to head (fa0d394).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #572   +/-   ##
=======================================
  Coverage   79.78%   79.78%           
=======================================
  Files         202      202           
  Lines       14688    14688           
=======================================
  Hits        11719    11719           
  Misses       2544     2544           
  Partials      425      425           
Flag Coverage Δ
unittests 79.78% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@zmberg zmberg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Code Review

Thanks for the fix! The nil guard correctly prevents the panic. A couple of suggestions below:

  • Issue: 1
  • Suggestion: 1

cond := utils.GetSandboxCondition(newStatus, string(agentsv1alpha1.SandboxConditionPaused))
// sandbox will only enter the resuming state after successful paused
if cond.Status == metav1.ConditionTrue && !box.Spec.Paused {
if cond != nil && cond.Status == metav1.ConditionTrue && !box.Spec.Paused {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[Issue] The PR correctly adds cond != nil defensive checks to prevent nil pointer panics. However, when cond == nil, neither branch is entered and calculateStatus silently skips processing. This edge case lacks observability.

Suggest adding a cond == nil branch with a log message (e.g., klog.InfoS recording that the sandbox is in Paused phase but the paused condition is absent) to make this scenario traceable in production. The existing EnsureSandboxPaused mechanism will naturally handle condition initialization, so no control flow change is needed.

} else if cond != nil && !box.Spec.Paused && cond.Status == metav1.ConditionFalse {
klog.InfoS("sandbox pause not completed, cannot enter resume state temporarily", "sandbox", klog.KObj(box))
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[Suggestion] Suggest adding test cases to cover the cond == nil scenario — verifying the controller's behavior when the SandboxPaused phase lacks the paused condition — to prevent regressions. Existing Paused-related test cases in TestCalculateStatus can serve as reference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants