test: add tests for graceful handling of null updates and finalizer removal

Signed-off-by: David Sondermann <david.sondermann@hivemq.com>

Author: Donnerbart

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,39 @@ void retriesFinalizerRemovalWithFreshResource() {
     verify(resourceOp, times(1)).get();
   }
 
+  @Test
+  void removeFinalizerHandlesAlreadyDeletedTargetGracefully() {
+    // When the underlying patch returns null because the target resource is already gone
+    // on the API server (fabric8's edit() returns null on 404), 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();
@@ -324,4 +363,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);
+  }
 }

operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java

@@ -412,6 +412,19 @@ void ghostCheckRunsConcurrentlyWithPutResource() {
     ghostCheckExecutor.shutdownNow();
   }
 
+  @Test
+  void eventFilteringUpdateHandlesNullFromUpdateMethodGracefully() {
+    // When the supplied update operation returns null (e.g. fabric8's edit() returns null
+    // because the patch target is already gone on the API server), 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 +474,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);
   }