fix(event): handle typed nil in NotifyError to prevent build controller panic#6589
Closed
Diesel93 wants to merge 1 commit into
Closed
fix(event): handle typed nil in NotifyError to prevent build controller panic#6589Diesel93 wants to merge 1 commit into
Diesel93 wants to merge 1 commit into
Conversation
…er panic When an action's Handle() returns (nil, err), newTarget is a typed nil *v1.Build. When passed as a runtime.Object interface, the nil check newResource != nil evaluates to true because the interface has type information even though the underlying pointer is nil. This causes a nil pointer dereference when recorder.Eventf calls GetReference which calls GetObjectKind() on the nil pointer. Fix by using reflect.ValueOf(newResource).IsNil() to properly detect typed nils before dereferencing. Fixes apache#6587
Contributor
|
Thanks for the work and, indeed, the reflection would fix this problem likely. However it is not a cheap operation and we try to avoid as much as we can for that reason. I think I was the one that introduced the bug by mistake with this work #6556 - while refactoring I've abstracted too much and lost the original type reference. I will revert this and apply a patch soon. |
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 #6587
Problem
When an action's
Handle()returns(nil, err),newTargetis a typed nil*v1.Build. When passed as aruntime.Objectinterface parameter toNotifyError, the checknewResource != nilevaluates totruebecause the interface has type information even though the underlying pointer is nil.This causes
recorder.Eventf→reference.GetReference→obj.GetObjectKind()to panic on the nil pointer, crashing the build controller on every reconcile. The build pod never spawns.This was introduced in #6492 when migrating from
record.EventRecordertoevents.EventRecorder— the new recorder goes throughGetReferencewhereas the old one did not.Fix
Use
reflect.ValueOf(newResource).IsNil()to properly detect typed nils before using the value.Testing
Reproduced the panic consistently on 2.10.0 (Helm install, EKS). Built a custom operator image from this branch and deployed it to a live EKS cluster. Build controller completed a full build lifecycle without panicking — kit reached
Readystate with a valid image reference.Note: the e2e test suite won't cover this path as it only exercises the happy path where
Handle()doesn't return(nil, err).