Skip to content

Fix NPE in ManagedInformerEventSource #3332

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Donnerbart wants to merge 3 commits into
base: main
Choose a base branch
from
+99 −21
Open

Fix NPE in ManagedInformerEventSource #3332

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6c119d7
test: add tests for graceful handling of null updates and finalizer r…
Donnerbart May 4, 2026
41c9a46
fix: guard against NPE in eventFilteringUpdateAndCacheResource when p…
Donnerbart May 4, 2026
339b6c3
refactor: fix visibility of manager() method to match the visibility …
Donnerbart May 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions ...javaoperatorsdk/operator/processing/event/source/informer/ManagedInformerEventSource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ protected ManagedInformerEventSource(String name, MixedOperation client, C confi
this.configuration = configuration;
}

protected InformerManager<R, C> manager() {
InformerManager<R, C> manager() {
Comment thread
Donnerbart marked this conversation as resolved.
return cache;
}

Expand All @@ -99,8 +99,12 @@ public R eventFilteringUpdateAndCacheResource(R resourceToUpdate, UnaryOperator<
try {
temporaryResourceCache.startEventFilteringModify(id);
updatedResource = updateMethod.apply(resourceToUpdate);
log.debug("Resource update successful");
handleRecentResourceUpdate(id, updatedResource, resourceToUpdate);
if (updatedResource != null) {
log.debug("Resource update successful");
handleRecentResourceUpdate(id, updatedResource, resourceToUpdate);
} else {
log.debug("Update operation returned null");
}
return updatedResource;
} finally {
var res =
Expand Down
62 changes: 62 additions & 0 deletions ...core/src/test/java/io/javaoperatorsdk/operator/api/reconciler/ResourceOperationsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,15 @@
import io.fabric8.kubernetes.client.KubernetesClient;
import io.fabric8.kubernetes.client.KubernetesClientException;
import io.fabric8.kubernetes.client.dsl.MixedOperation;
import io.fabric8.kubernetes.client.dsl.NamespaceableResource;
import io.fabric8.kubernetes.client.dsl.Resource;
import io.javaoperatorsdk.operator.MockKubernetesClient;
import io.javaoperatorsdk.operator.TestUtils;
import io.javaoperatorsdk.operator.api.config.BaseConfigurationService;
import io.javaoperatorsdk.operator.api.config.ControllerConfiguration;
import io.javaoperatorsdk.operator.api.config.ResolvedControllerConfiguration;
import io.javaoperatorsdk.operator.api.config.informer.InformerConfiguration;
import io.javaoperatorsdk.operator.processing.Controller;
import io.javaoperatorsdk.operator.processing.event.EventSourceRetriever;
import io.javaoperatorsdk.operator.processing.event.source.EventSource;
import io.javaoperatorsdk.operator.processing.event.source.controller.ControllerEventSource;
Expand Down Expand Up @@ -247,6 +253,38 @@ void retriesFinalizerRemovalWithFreshResource() {
verify(resourceOp, times(1)).get();
}

@Test
void removeFinalizerHandlesAlreadyDeletedTargetGracefully() {
// When the underlying patch returns null, removeFinalizer must return cleanly rather
// than throw
var resource = TestUtils.testCustomResource1();
resource.getMetadata().setResourceVersion("1");
resource.addFinalizer(FINALIZER_NAME);
when(context.getPrimaryResource()).thenReturn(resource);

// Real ControllerEventSource so eventFilteringUpdateAndCacheResource and the
// TemporaryResourceCache it talks to are wired up the way they would be in production
NamespaceableResource<TestCustomResource> single = mock();
when(single.edit(any(UnaryOperator.class))).thenReturn(null);
var client = MockKubernetesClient.client(TestCustomResource.class);
when(client.resource(any(TestCustomResource.class))).thenReturn(single);

var eventSource = new ControllerEventSource<>(testController(client));
eventSource.start();
try {
when(context.getClient()).thenReturn(client);
var eventSourceRetriever = mock(EventSourceRetriever.class);
when(eventSourceRetriever.getControllerEventSource()).thenReturn(eventSource);
when(context.eventSourceRetriever()).thenReturn(eventSourceRetriever);

var result = resourceOperations.removeFinalizer(FINALIZER_NAME);

assertThat(result).isNull();
} finally {
eventSource.stop();
}
}

@Test
void resourcePatchWithSingleEventSource() {
var resource = TestUtils.testCustomResource1();
Expand Down Expand Up @@ -324,4 +362,28 @@ void resourcePatchThrowsWhenEventSourceIsNotManagedInformer() {
assertThat(exception.getMessage()).contains("Target event source must be a subclass off");
assertThat(exception.getMessage()).contains("ManagedInformerEventSource");
}

private Controller<TestCustomResource> testController(KubernetesClient client) {
var informerConfig =
InformerConfiguration.builder(TestCustomResource.class)
.withComparableResourceVersions(true)
.withGhostResourceCacheCheckInterval(Constants.DEFAULT_GHOST_RESOURCE_CHECK_INTERVAL)
.buildForController();
var configuration =
new ResolvedControllerConfiguration<>(
"test",
true,
null,
null,
null,
null,
FINALIZER_NAME,
null,
null,
new BaseConfigurationService(),
informerConfig,
false,
null);
return new Controller<>((r, ctx) -> UpdateControl.noUpdate(), configuration, client);
}
}
48 changes: 30 additions & 18 deletions ...io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,6 +412,18 @@ void ghostCheckRunsConcurrentlyWithPutResource() {
ghostCheckExecutor.shutdownNow();
}

@Test
void eventFilteringUpdateHandlesNullFromUpdateMethodGracefully() {
// When the supplied update operation returns null, null must propagate cleanly out of
// eventFilteringUpdateAndCacheResource rather than throw
withRealTemporaryResourceCache();

var result =
informerEventSource.eventFilteringUpdateAndCacheResource(testDeployment(), r -> null);

assertThat(result).isNull();
}

@Test
void filteringUpdateAndGhostCheckWithNamespaceChange() {
var mes = mock(ManagedInformerEventSource.class);
Expand Down Expand Up @@ -461,26 +473,26 @@ private void assertNoEventProduced() {
private void expectHandleEvent(int newResourceVersion, int oldResourceVersion) {
await()
.untilAsserted(
() -> {
verify(informerEventSource, times(1))
.handleEvent(
eq(ResourceAction.UPDATED),
argThat(
newResource -> {
assertThat(newResource.getMetadata().getResourceVersion())
.isEqualTo("" + newResourceVersion);
return true;
}),
argThat(
newResource -> {
assertThat(newResource.getMetadata().getResourceVersion())
.isEqualTo("" + oldResourceVersion);
return true;
}),
isNull());
});
() ->
verify(informerEventSource, times(1))
.handleEvent(
eq(ResourceAction.UPDATED),
argThat(
newResource -> {
assertThat(newResource.getMetadata().getResourceVersion())
.isEqualTo("" + newResourceVersion);
return true;
}),
argThat(
newResource -> {
assertThat(newResource.getMetadata().getResourceVersion())
.isEqualTo("" + oldResourceVersion);
return true;
}),
isNull()));
}

@SuppressWarnings("SameParameterValue")
private CountDownLatch sendForEventFilteringUpdate(int resourceVersion) {
return sendForEventFilteringUpdate(testDeployment(), resourceVersion);
}
Expand Down
Loading