The function shouldRecordEvent will panic when the value of input obj… #95662
Conversation
k8s-ci-robot
added
release-note-none
|
@SergeyKanzhelev: This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
the
needs-priority
k8s-ci-robot
added
sig/node
| if ref, ok := object.(*v1.ObjectReference); ok { | ||
| // this check is needed AFTER the cast. See https://github.com/kubernetes/kubernetes/issues/95552 | ||
| if ref == nil { | ||
| return nil, false |
There was a problem hiding this comment.
I THINK it should NOT record the event if the object is nil.
There was a problem hiding this comment.
it is similar to the behavior that seems to be expected before. But worth paying attention to when reviewing this change
|
I think I like this a little better than #95634, but since this is such a strange condition, maybe we log it. Although if it's a bunch of nil, I'm not sure there's much information to log. |
|
@MHBauer can you LGTM? |
|
mark |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dchen1107, SergeyKanzhelev The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
What type of PR is this?
/kind bug
What this PR does / why we need it:
Makes a proper nil check.
Which issue(s) this PR fixes:
Related to #95552.
Special notes for your reviewer:
This issue is a clean up based on the report from #95552. There might be a need for further analysis on whether any of k8s versions are affected to cherry pick this fix back. Also some analysis may be needed if we can prevent this situation all together. It is not a good practice that
nilinterfaces are even allowed to get to be passed to this method.Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: