Skip to content

Fix NPE in ManagedInformerEventSource#3332

Open
Donnerbart wants to merge 3 commits intooperator-framework:mainfrom
Donnerbart:bugfix/fix-npe-in-managed-informerer-event-source
Open

Fix NPE in ManagedInformerEventSource#3332
Donnerbart wants to merge 3 commits intooperator-framework:mainfrom
Donnerbart:bugfix/fix-npe-in-managed-informerer-event-source

Conversation

@Donnerbart
Copy link
Copy Markdown
Contributor

Fixes #3331

Copilot AI review requested due to automatic review settings May 4, 2026 12:48
@openshift-ci openshift-ci Bot requested review from csviri and metacosm May 4, 2026 12:48
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a null-handling gap in the informer-managed update path where Fabric8 edit() can return null (notably on HTTP 404), which previously could lead to an NPE when updating the temporary resource cache. This aligns behavior with the reported issue #3331 (“patch target already deleted”).

Changes:

  • Add a null guard in ManagedInformerEventSource.eventFilteringUpdateAndCacheResource to skip cache update when the update operation returns null.
  • Add regression tests covering null returns from the update method at both the informer event source layer and via ResourceOperations.removeFinalizer.
  • Minor test refactors/formatting adjustments.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/ManagedInformerEventSource.java Adds null-guard around cache update after updateMethod.apply(...); also changes manager() visibility.
operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java Adds regression test ensuring eventFilteringUpdateAndCacheResource cleanly propagates null.
operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/reconciler/ResourceOperationsTest.java Adds integration-style regression test for removeFinalizer when underlying patch/edit returns null.

@Donnerbart Donnerbart force-pushed the bugfix/fix-npe-in-managed-informerer-event-source branch from 396094f to d103e18 Compare May 4, 2026 14:01
@csviri csviri requested a review from shawkins May 5, 2026 07:50
Copy link
Copy Markdown
Collaborator

@csviri csviri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you @Donnerbart

@shawkins if you have the bandwidth please take a look

@shawkins
Copy link
Copy Markdown
Collaborator

shawkins commented May 5, 2026

When that edit() returns null (fabric8's behavior on HTTP 404 when the target is gone)

@csviri @Donnerbart this assumption from the issue is not correct. The edit function throws an exception on 404.

Donnerbart added 3 commits May 5, 2026 18:25
…emoval

Signed-off-by: David Sondermann <david.sondermann@hivemq.com>
…atch target is deleted

Signed-off-by: David Sondermann <david.sondermann@hivemq.com>
…of the returned class

Signed-off-by: David Sondermann <david.sondermann@hivemq.com>
@Donnerbart Donnerbart force-pushed the bugfix/fix-npe-in-managed-informerer-event-source branch from d103e18 to 339b6c3 Compare May 5, 2026 16:25
@Donnerbart
Copy link
Copy Markdown
Contributor Author

@shawkins You are correct, it looks like with a real K8s server the fabric8 client always throws on a 404. The null we hit comes from a different path: KubernetesMockServer's PatchHandler returns HTTP 202 with an empty body when a PATCH removes the last finalizer on a deletion-pending resource (exactly what removeFinalizer does on cleanup), and fabric8's unmarshal returns null on empty bodies via parseYaml. The bug is real but the cause I described originally was off. I've updated the issue with the full trace and more info.

The IT result indicates that the scope is just the mock server, so one option is to just close this PR and fix it upstream instead, in kubernetes-server-mock (and arguably in fabric8's empty-body unmarshal behavior). Do you think it makes sense to file that issue for the mock server or the client?

I fixed the reasoning in the PR, in case we still want a JOSDK-side guard. I can also refactor this PR to the other options I mention in the issue: fail loud with Objects.requireNonNull or translate null to the input resource at the ResourceOperations layer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NPE in ManagedInformerEventSource.eventFilteringUpdateAndCacheResource when patch target was already deleted on the API server

4 participants