Reject CreateContainer on a non-ready sandbox#13614
Open
yugstar wants to merge 1 commit into
Open
Conversation
CreateContainer loaded the sandbox but never verified it was in the ready state, so a container could be created against a stopped sandbox, which violates the CRI spec (CreateContainer must fail if the sandbox is not running). Add the same guard that StartContainer already applies, returning "sandbox container %q is not running". The sandbox store package was imported as `sandbox`, which is shadowed by the local `sandbox` variable inside CreateContainer; alias it to `sandboxstore` (matching container_start.go) so the state constant can be referenced. Fixes containerd#13599 Signed-off-by: Aman Raj <[email protected]>
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a CRI-spec guard to prevent creating containers in non-ready sandboxes, and introduces a unit test to verify the behavior.
Changes:
- Reject
CreateContainerrequests when the target sandbox is not inStateReady. - Align sandbox store import usage via
sandboxstorealias and update related types. - Add a unit test ensuring non-ready sandboxes cause
CreateContainerto fail.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/cri/server/container_create.go | Adds a readiness-state check for the sandbox before allowing container creation; updates sandbox store import/type references. |
| internal/cri/server/container_create_test.go | Adds a regression test that verifies CreateContainer is rejected for a non-ready sandbox. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // The CRI spec requires that containers are only created in a running | ||
| // sandbox. Reject the request otherwise, matching StartContainer. | ||
| if sandbox.Status.Get().State != sandboxstore.StateReady { | ||
| return nil, fmt.Errorf("sandbox container %q is not running", sandbox.ID) |
Comment on lines
+70
to
+74
| // The CRI spec requires that containers are only created in a running | ||
| // sandbox. Reject the request otherwise, matching StartContainer. | ||
| if sandbox.Status.Get().State != sandboxstore.StateReady { | ||
| return nil, fmt.Errorf("sandbox container %q is not running", sandbox.ID) | ||
| } |
| SandboxConfig: sandboxConfig, | ||
| }) | ||
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), "is not running") |
Contributor
Author
|
Thanks for the review. All three points come down to staying consistent with the existing sandbox-not-ready guard in container_start.go, which this mirrors:
Happy to switch to a typed error with a FailedPrecondition status if preferred, but it would be better done for both create and start together in a separate change so the two stay in sync. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #13599
Description
CreateContainerloads the sandbox (c.sandboxStore.Get) but never checks that the sandbox is in the ready state before proceeding. As a result a container can be created against a stopped sandbox, which violates the CRI spec —RunPodSandbox/CreateContainerrequire the sandbox to be running — and is what blocks the cri-tools conformance test referenced in #13599.StartContaineralready guards against this:This applies the same guard in
CreateContainer, right after the sandbox is loaded.Note:
container_create.goimported the sandbox store package assandbox, which is shadowed by the localsandboxvariable insideCreateContainer. I aliased the import tosandboxstore(matchingcontainer_start.go) so theStateReadyconstant can be referenced; the only other use of the package alias in the file (a struct field type) was updated accordingly.Test
Added
TestCreateContainerNotReadySandbox, which adds aStateNotReadysandbox to the store and assertsCreateContainerfails withis not running.go vetand the existinginternal/cri/servercontainer/sandbox tests pass.