From 06071908965ec811e1e97a3c87514315cd182fae Mon Sep 17 00:00:00 2001 From: redhat-chai-bot Date: Fri, 7 Aug 2026 15:56:09 +0000 Subject: [PATCH] Bug 105432: fix nil pointer panic in IRI controller informer race The InternalReleaseImage controller's New() constructor registered informer event handlers before assigning the listers. When an informer is already started, AddEventHandler replays synthetic Add events on a separate goroutine, which could invoke isControlPlaneNode/isNodeReady before ctrl.nodeLister was assigned, causing a nil pointer panic. Move all lister and HasSynced assignments ahead of the AddEventHandler registrations (pure reordering, no logic change), and add defensive nil guards in isControlPlaneNode and isNodeReady that log a warning and return false when the nodeLister is not yet initialized. The guard warnings intentionally omit the node name to avoid logging potentially sensitive infrastructure identifiers. Add TestNewWithAlreadyStartedInformers, a regression test that starts and preloads the informers before calling New() to exercise the replayed-event path and assert construction completes without panicking. Co-Authored-By: Claude Opus 4.8 --- .../internalreleaseimage_controller.go | 74 ++++++++++++------ .../internalreleaseimage_controller_test.go | 78 +++++++++++++++++++ 2 files changed, 128 insertions(+), 24 deletions(-) diff --git a/pkg/controller/internalreleaseimage/internalreleaseimage_controller.go b/pkg/controller/internalreleaseimage/internalreleaseimage_controller.go index a58872db27..4ca6a1bdb4 100644 --- a/pkg/controller/internalreleaseimage/internalreleaseimage_controller.go +++ b/pkg/controller/internalreleaseimage/internalreleaseimage_controller.go @@ -111,6 +111,36 @@ func New( ctrl.syncHandler = ctrl.syncInternalReleaseImage + // Assign the listers and their HasSynced functions BEFORE registering the + // event handlers below. AddEventHandler on an already-started informer + // replays synthetic Add events from a separate goroutine, which can invoke + // handlers (e.g. isControlPlaneNode/isNodeReady, which dereference + // ctrl.nodeLister) before these fields are assigned. Assigning the listers + // first eliminates that nil-pointer race. + ctrl.iriLister = iriInformer.Lister() + ctrl.iriListerSynced = iriInformer.Informer().HasSynced + + ctrl.ccLister = ccInformer.Lister() + ctrl.ccListerSynced = ccInformer.Informer().HasSynced + + ctrl.mcLister = mcInformer.Lister() + ctrl.mcListerSynced = mcInformer.Informer().HasSynced + + ctrl.clusterVersionLister = clusterVersionInformer.Lister() + ctrl.clusterVersionListerSynced = clusterVersionInformer.Informer().HasSynced + + ctrl.secretLister = secretInformer.Lister() + ctrl.secretListerSynced = secretInformer.Informer().HasSynced + + ctrl.mcnLister = mcnInformer.Lister() + ctrl.mcnListerSynced = mcnInformer.Informer().HasSynced + + ctrl.nodeLister = nodeInformer.Lister() + ctrl.nodeListerSynced = nodeInformer.Informer().HasSynced + + ctrl.infraLister = infraInformer.Lister() + ctrl.infraListerSynced = infraInformer.Informer().HasSynced + iriInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{ AddFunc: ctrl.addInternalReleaseImage, UpdateFunc: ctrl.updateInternalReleaseImage, @@ -144,30 +174,6 @@ func New( UpdateFunc: ctrl.updateNode, }) - ctrl.iriLister = iriInformer.Lister() - ctrl.iriListerSynced = iriInformer.Informer().HasSynced - - ctrl.ccLister = ccInformer.Lister() - ctrl.ccListerSynced = ccInformer.Informer().HasSynced - - ctrl.mcLister = mcInformer.Lister() - ctrl.mcListerSynced = mcInformer.Informer().HasSynced - - ctrl.clusterVersionLister = clusterVersionInformer.Lister() - ctrl.clusterVersionListerSynced = clusterVersionInformer.Informer().HasSynced - - ctrl.secretLister = secretInformer.Lister() - ctrl.secretListerSynced = secretInformer.Informer().HasSynced - - ctrl.mcnLister = mcnInformer.Lister() - ctrl.mcnListerSynced = mcnInformer.Informer().HasSynced - - ctrl.nodeLister = nodeInformer.Lister() - ctrl.nodeListerSynced = nodeInformer.Informer().HasSynced - - ctrl.infraLister = infraInformer.Lister() - ctrl.infraListerSynced = infraInformer.Informer().HasSynced - return ctrl } @@ -384,6 +390,16 @@ func (ctrl *Controller) updateNode(_, cur interface{}) { // isControlPlaneNode checks if a node is a control plane node by checking its labels. // Returns true if the node has the master or control-plane role label. func (ctrl *Controller) isControlPlaneNode(nodeName string) bool { + // Defensive guard: the nodeLister may not be assigned yet if an event + // handler fires during controller construction. Treat the node as + // non-control-plane rather than panicking on a nil lister. + if ctrl.nodeLister == nil { + // Do not log the node name: node names can contain internal hostnames + // or other infrastructure identifiers. Log only the lister state. + klog.Warning("nodeLister not initialized yet; treating node as non-control-plane") + return false + } + node, err := ctrl.nodeLister.Get(nodeName) if err != nil { klog.V(4).Infof("Failed to get node %s: %v", nodeName, err) @@ -403,6 +419,16 @@ func (ctrl *Controller) isControlPlaneNode(nodeName string) bool { // isNodeReady checks if a node is ready by examining its Ready condition. func (ctrl *Controller) isNodeReady(nodeName string) bool { + // Defensive guard: the nodeLister may not be assigned yet if an event + // handler fires during controller construction. Treat the node as not + // ready rather than panicking on a nil lister. + if ctrl.nodeLister == nil { + // Do not log the node name: node names can contain internal hostnames + // or other infrastructure identifiers. Log only the lister state. + klog.Warning("nodeLister not initialized yet; treating node as not ready") + return false + } + node, err := ctrl.nodeLister.Get(nodeName) if err != nil { klog.V(4).Infof("Failed to get node %s: %v", nodeName, err) diff --git a/pkg/controller/internalreleaseimage/internalreleaseimage_controller_test.go b/pkg/controller/internalreleaseimage/internalreleaseimage_controller_test.go index 6430437a43..cc0c1c2e29 100644 --- a/pkg/controller/internalreleaseimage/internalreleaseimage_controller_test.go +++ b/pkg/controller/internalreleaseimage/internalreleaseimage_controller_test.go @@ -501,3 +501,81 @@ func TestTransformToAPIIntURL(t *testing.T) { }) } } + +// TestNewWithAlreadyStartedInformers is a regression test for the nil-pointer +// panic caused by an informer race in New(). When an informer is already +// started, AddEventHandler replays the current cache contents as synthetic Add +// events on a separate goroutine. If the listers were assigned after the event +// handlers were registered, the replayed MachineConfigNode Add would invoke +// isControlPlaneNode -> ctrl.nodeLister.Get on a nil nodeLister and panic. +// +// This test preloads and starts the informers (in particular mcnInformer and +// nodeInformer) BEFORE calling New, exercising that ordering and asserting that +// construction completes without panicking and that the replayed event is +// handled. +func TestNewWithAlreadyStartedInformers(t *testing.T) { + // A healthy MachineConfigNode plus its control-plane Node so that the + // replayed Add event drives addMachineConfigNode -> isControlPlaneNode, + // which dereferences the node lister. + mcfgClient := fake.NewSimpleClientset(mcn("master-0").build()) + k8sClient := k8sfake.NewSimpleClientset(node("master-0").build()) + configClient := fakeconfigv1client.NewSimpleClientset() + + i := mcfginformers.NewSharedInformerFactory(mcfgClient, 0) + k := informers.NewSharedInformerFactory(k8sClient, 0) + ci := configinformers.NewSharedInformerFactory(configClient, 0) + + iriInformer := i.Machineconfiguration().V1().InternalReleaseImages() + ccInformer := i.Machineconfiguration().V1().ControllerConfigs() + mcInformer := i.Machineconfiguration().V1().MachineConfigs() + cvInformer := ci.Config().V1().ClusterVersions() + secretInformer := k.Core().V1().Secrets() + mcnInformer := i.Machineconfiguration().V1().MachineConfigNodes() + nodeInformer := k.Core().V1().Nodes() + infraInformer := ci.Config().V1().Infrastructures() + + // Instantiate each informer so the factories start and sync them below. + iriInformer.Informer() + ccInformer.Informer() + mcInformer.Informer() + cvInformer.Informer() + secretInformer.Informer() + mcnInformer.Informer() + nodeInformer.Informer() + infraInformer.Informer() + + stopCh := make(chan struct{}) + defer close(stopCh) + + // Start and sync the informers BEFORE constructing the controller. This is + // the ordering that previously triggered the nil-pointer race. + i.Start(stopCh) + k.Start(stopCh) + ci.Start(stopCh) + i.WaitForCacheSync(stopCh) + k.WaitForCacheSync(stopCh) + ci.WaitForCacheSync(stopCh) + + c := New( + iriInformer, + ccInformer, + mcInformer, + cvInformer, + secretInformer, + mcnInformer, + nodeInformer, + infraInformer, + k8sClient, + mcfgClient, + ) + assert.NotNil(t, c) + + // The replayed Add event for the control-plane MachineConfigNode must be + // handled by addMachineConfigNode without panicking, which enqueues the IRI + // singleton. Waiting for the enqueue proves the handler ran to completion + // against a non-nil node lister. + assert.Eventually(t, func() bool { + return c.queue.Len() > 0 + }, 5*time.Second, 10*time.Millisecond, + "expected replayed MachineConfigNode Add to be handled and enqueue the IRI without panicking") +}