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") +}