From 6c119d796a189f165aaf567e06ed9d704d4e4d80 Mon Sep 17 00:00:00 2001 From: David Sondermann Date: Mon, 4 May 2026 13:50:35 +0200 Subject: [PATCH 1/3] test: add tests for graceful handling of null updates and finalizer removal Signed-off-by: David Sondermann --- .../reconciler/ResourceOperationsTest.java | 62 +++++++++++++++++++ .../informer/InformerEventSourceTest.java | 48 ++++++++------ 2 files changed, 92 insertions(+), 18 deletions(-) diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/reconciler/ResourceOperationsTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/reconciler/ResourceOperationsTest.java index 8d0176cd4a..dbc2106cb0 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/reconciler/ResourceOperationsTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/reconciler/ResourceOperationsTest.java @@ -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; @@ -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 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(); @@ -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 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); + } } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java index e60ac02280..92a2772ffc 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java @@ -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); @@ -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); } From 41c9a46d875e6e7ae7d827270339c8ccf2548d78 Mon Sep 17 00:00:00 2001 From: David Sondermann Date: Mon, 4 May 2026 13:52:27 +0200 Subject: [PATCH 2/3] fix: guard against NPE in eventFilteringUpdateAndCacheResource when patch target is deleted Signed-off-by: David Sondermann --- .../event/source/informer/ManagedInformerEventSource.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/ManagedInformerEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/ManagedInformerEventSource.java index 26543e8322..5fc2361d80 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/ManagedInformerEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/ManagedInformerEventSource.java @@ -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 = From 339b6c37eedfc4359f708d0e241a37d203fac65f Mon Sep 17 00:00:00 2001 From: David Sondermann Date: Mon, 4 May 2026 14:23:26 +0200 Subject: [PATCH 3/3] refactor: fix visibility of manager() method to match the visibility of the returned class Signed-off-by: David Sondermann --- .../event/source/informer/ManagedInformerEventSource.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/ManagedInformerEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/ManagedInformerEventSource.java index 5fc2361d80..ee65b3ee9d 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/ManagedInformerEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/ManagedInformerEventSource.java @@ -75,7 +75,7 @@ protected ManagedInformerEventSource(String name, MixedOperation client, C confi this.configuration = configuration; } - protected InformerManager manager() { + InformerManager manager() { return cache; }