Skip to content
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.

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

cri: ensure NRI API never has nil CRI #10401

Merged
merged 1 commit into from
Jul 1, 2024

Conversation

samuelkarp
Copy link
Member

A nil CRIImplementation field can cause a nil pointer dereference and panic during startup recovery.

Prior to this change, the nri.API struct would have a nil cri (CRIImplementation) field after nri.NewAPI until nri.Register was called. Register is called mid-way through initialization of the CRI plugin, but recovery for containers occurs prior to that. Container recovery includes establishing new exit monitors for existing containers that were discovered. When a container exits, NRI plugins are given the opportunity to be notified about the lifecycle event, and this is done by accessing that CRIImplementation field inside the nri.API. If a container exits prior to nri.Register being called, access to the CRIImplementation field can cause a panic.

Here's the call-path:

  • The CRI plugin starts running here
  • It then calls into recover() to recover state from previous runs of containerd
  • recover() then attempts to recover all containers through loadContainer()
  • When loadContainer() finds a container that is still running, it waits for the task (internal containerd object) to exit and sets up exit monitoring
  • Any exit that then happens must be handled
  • Handling an exit includes deleting the Task and specifying nri.WithContainerExit to notify any subscribed NRI plugins
  • NRI plugins need to know information about the pod (not just the sandbox), so before a plugin is notified the NRI API package queries the Sandbox Store through the CRI implementation
  • The cri implementation member field in the nri.API struct is set as part of the Register() method
  • The nri.Register() method is only called much further down in the CRI Run() method

A nil CRIImplementation field can cause a nil pointer dereference and
panic during startup recovery.

Prior to this change, the nri.API struct would have a nil cri
(CRIImplementation) field after nri.NewAPI until nri.Register was
called.  Register is called mid-way through initialization of the CRI
plugin, but recovery for containers occurs prior to that.  Container
recovery includes establishing new exit monitors for existing containers
that were discovered.  When a container exits, NRI plugins are given the
opportunity to be notified about the lifecycle event, and this is done
by accessing that CRIImplementation field inside the nri.API.  If a
container exits prior to nri.Register being called, access to the
CRIImplementation field can cause a panic.

Here's the call-path:

* The CRI plugin starts running
  [here](https://github.com/containerd/containerd/blob/ae71819c4f5e67bb4d5ae76a6b735f29cc25774e/pkg/cri/server/service.go#L222)
* It then [calls into](https://github.com/containerd/containerd/blob/ae71819c4f5e67bb4d5ae76a6b735f29cc25774e/pkg/cri/server/service.go#L227)
  `recover()` to recover state from previous runs of containerd
* `recover()` then attempts to recover all containers through
  [`loadContainer()`](https://github.com/containerd/containerd/blob/ae7d74b9e21bd08260586db104a1fe04af754545/internal/cri/server/restart.go#L175)
* When `loadContainer()` finds a container that is still running, it waits
  for the task (internal containerd object) to exit and sets up
  [exit monitoring](https://github.com/containerd/containerd/blob/ae7d74b9e21bd08260586db104a1fe04af754545/internal/cri/server/restart.go#L391)
* Any exit that then happens must be
  [handled](https://github.com/containerd/containerd/blob/ae7d74b9e21bd08260586db104a1fe04af754545/internal/cri/server/events.go#L145)
* Handling an exit includes
  [deleting the Task](https://github.com/containerd/containerd/blob/ae7d74b9e21bd08260586db104a1fe04af754545/internal/cri/server/events.go#L188)
  and specifying [`nri.WithContainerExit`](https://github.com/containerd/containerd/blob/ae7d74b9e21bd08260586db104a1fe04af754545/internal/cri/nri/nri_api_linux.go#L348)
  to [notify](https://github.com/containerd/containerd/blob/ae7d74b9e21bd08260586db104a1fe04af754545/internal/cri/nri/nri_api_linux.go#L356)
  any subscribed NRI plugins
* NRI plugins need to know information about the pod (not just the sandbox),
  so before a plugin is notified the NRI API package
  [queries the Sandbox Store](https://github.com/containerd/containerd/blob/ae7d74b9e21bd08260586db104a1fe04af754545/internal/cri/nri/nri_api_linux.go#L232)
  through the CRI implementation
* The `cri` implementation member field in the `nri.API` struct is set as part of the
  [`Register()`](https://github.com/containerd/containerd/blob/ae7d74b9e21bd08260586db104a1fe04af754545/internal/cri/nri/nri_api_linux.go#L66) method
* The `nri.Register()` method is only called
  [much further down in the CRI `Run()` method](https://github.com/containerd/containerd/blob/ae71819c4f5e67bb4d5ae76a6b735f29cc25774e/pkg/cri/server/service.go#L279)

Signed-off-by: Samuel Karp <samuelkarp@google.com>
Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

LGTM

@samuelkarp samuelkarp added the cherry-pick/1.7.x Change to be cherry picked to release/1.7 branch label Jul 1, 2024
@samuelkarp samuelkarp added this pull request to the merge queue Jul 1, 2024
@samuelkarp samuelkarp added the area/cri Container Runtime Interface (CRI) label Jul 1, 2024
Merged via the queue into containerd:main with commit ebcbbe5 Jul 1, 2024
47 checks passed
@samuelkarp samuelkarp added cherry-picked/1.7.x PR commits are cherry-picked into release/1.7 branch cherry-picked/sbserver Changes are backported to sbserver and removed cherry-pick/1.7.x Change to be cherry picked to release/1.7 branch labels Jul 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cri Container Runtime Interface (CRI) cherry-picked/sbserver Changes are backported to sbserver cherry-picked/1.7.x PR commits are cherry-picked into release/1.7 branch size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants