From 10aec359a04367895ff2a0ca634a6eb6771db615 Mon Sep 17 00:00:00 2001 From: Samuel Karp Date: Fri, 28 Jun 2024 00:30:37 -0700 Subject: [PATCH] cri: ensure NRI API never has nil CRI 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 --- internal/cri/nri/nri_api_linux.go | 6 +++--- internal/cri/nri/nri_api_other.go | 4 ++-- internal/cri/server/service.go | 8 +++++--- internal/nri/nri.go | 2 +- plugins/cri/cri.go | 6 ++---- 5 files changed, 13 insertions(+), 13 deletions(-) diff --git a/internal/cri/nri/nri_api_linux.go b/internal/cri/nri/nri_api_linux.go index c57e67fd5..0adcd9d11 100644 --- a/internal/cri/nri/nri_api_linux.go +++ b/internal/cri/nri/nri_api_linux.go @@ -46,9 +46,10 @@ type API struct { nri nri.API } -func NewAPI(nri nri.API) *API { +func NewAPI(nri nri.API, cri CRIImplementation) *API { return &API{ nri: nri, + cri: cri, } } @@ -58,12 +59,11 @@ func (a *API) IsDisabled() bool { func (a *API) IsEnabled() bool { return !a.IsDisabled() } -func (a *API) Register(cri CRIImplementation) error { +func (a *API) Register() error { if a.IsDisabled() { return nil } - a.cri = cri nri.RegisterDomain(a) return a.nri.Start() diff --git a/internal/cri/nri/nri_api_other.go b/internal/cri/nri/nri_api_other.go index a8b3dfedb..14bd7ddd5 100644 --- a/internal/cri/nri/nri_api_other.go +++ b/internal/cri/nri/nri_api_other.go @@ -37,11 +37,11 @@ import ( type API struct { } -func NewAPI(nri.API) *API { +func NewAPI(nri.API, CRIImplementation) *API { return nil } -func (a *API) Register(CRIImplementation) error { +func (a *API) Register() error { return nil } diff --git a/internal/cri/server/service.go b/internal/cri/server/service.go index fd2b21acc..4c215c2a6 100644 --- a/internal/cri/server/service.go +++ b/internal/cri/server/service.go @@ -35,6 +35,7 @@ import ( "k8s.io/kubelet/pkg/cri/streaming" apitypes "github.com/containerd/containerd/api/types" + containerd "github.com/containerd/containerd/v2/client" "github.com/containerd/containerd/v2/core/introspection" _ "github.com/containerd/containerd/v2/core/runtime" // for typeurl init @@ -50,6 +51,7 @@ import ( snapshotstore "github.com/containerd/containerd/v2/internal/cri/store/snapshot" ctrdutil "github.com/containerd/containerd/v2/internal/cri/util" "github.com/containerd/containerd/v2/internal/eventq" + nriservice "github.com/containerd/containerd/v2/internal/nri" "github.com/containerd/containerd/v2/internal/registrar" "github.com/containerd/containerd/v2/pkg/oci" osinterface "github.com/containerd/containerd/v2/pkg/os" @@ -164,7 +166,7 @@ type CRIServiceOptions struct { StreamingConfig streaming.Config - NRI *nri.API + NRI nriservice.API // SandboxControllers is a map of all the loaded sandbox controllers SandboxControllers map[string]sandbox.Controller @@ -236,7 +238,7 @@ func NewCRIService(options *CRIServiceOptions) (CRIService, runtime.RuntimeServi } } - c.nri = options.NRI + c.nri = nri.NewAPI(options.NRI, &criImplementation{c}) c.runtimeHandlers, err = c.introspectRuntimeHandlers(ctx) if err != nil { @@ -297,7 +299,7 @@ func (c *criService) Run(ready func()) error { }() // register CRI domain with NRI - if err := c.nri.Register(&criImplementation{c}); err != nil { + if err := c.nri.Register(); err != nil { return fmt.Errorf("failed to set up NRI for CRI service: %w", err) } diff --git a/internal/nri/nri.go b/internal/nri/nri.go index 21aa1a450..a3922bcf3 100644 --- a/internal/nri/nri.go +++ b/internal/nri/nri.go @@ -38,7 +38,7 @@ type API interface { // IsEnabled returns true if the NRI interface is enabled and initialized. IsEnabled() bool - // Start start the NRI interface, allowing external NRI plugins to + // Start starts the NRI interface, allowing external NRI plugins to // connect, register, and hook themselves into the lifecycle events // of pods and containers. Start() error diff --git a/plugins/cri/cri.go b/plugins/cri/cri.go index 07f174a95..950187648 100644 --- a/plugins/cri/cri.go +++ b/plugins/cri/cri.go @@ -32,7 +32,6 @@ import ( criconfig "github.com/containerd/containerd/v2/internal/cri/config" "github.com/containerd/containerd/v2/internal/cri/constants" "github.com/containerd/containerd/v2/internal/cri/instrument" - "github.com/containerd/containerd/v2/internal/cri/nri" "github.com/containerd/containerd/v2/internal/cri/server" nriservice "github.com/containerd/containerd/v2/internal/nri" "github.com/containerd/containerd/v2/plugins" @@ -212,7 +211,7 @@ func (c criGRPCServerWithTCP) RegisterTCP(s *grpc.Server) error { } // Get the NRI plugin, and set up our NRI API for it. -func getNRIAPI(ic *plugin.InitContext) *nri.API { +func getNRIAPI(ic *plugin.InitContext) nriservice.API { const ( pluginType = plugins.NRIApiPlugin pluginName = "nri" @@ -234,8 +233,7 @@ func getNRIAPI(ic *plugin.InitContext) *nri.API { } log.G(ctx).Info("using experimental NRI integration - disable nri plugin to prevent this") - - return nri.NewAPI(api) + return api } func getSandboxControllers(ic *plugin.InitContext) (map[string]sandbox.Controller, error) {