- skip delete if no finalizer

- fix tests for EventDispatcher

Author: attila.meszaros

operator-framework/src/main/java/com/github/containersolutions/operator/processing/EventDispatcher.java

@@ -42,7 +42,9 @@ public void handleEvent(CustomResourceEvent event) {
         /* Its interesting problem if we should call delete if received event after object is marked for deletion
            but finalizer is not on the object. Since it can happen that there are multiple finalizers, also other events after
            we called delete and remove finalizers already. Delete should be also idempotent, we call it now. */
-        // todo should we check if it has also finalizer?
+        if (markedForDeletion(resource) && !ControllerUtils.hasDefaultFinalizer(resource, resourceDefaultFinalizer)) {
+            return;
+        }
         if (markedForDeletion(resource)) {
             boolean removeFinalizer = controller.deleteResource(resource, context);
             if (removeFinalizer && ControllerUtils.hasDefaultFinalizer(resource, resourceDefaultFinalizer)) {

operator-framework/src/test/java/com/github/containersolutions/operator/EventDispatcherTest.java

@@ -94,7 +94,6 @@ void callDeleteOnControllerIfMarkedForDeletionButThereIsNoDefaultFinalizer() {
     @Test
     void removesDefaultFinalizerOnDelete() {
         markForDeletion(testCustomResource);
-        testCustomResource.getMetadata().getFinalizers().add(DEFAULT_FINALIZER);
 
         eventDispatcher.handleEvent(customResourceEvent(Watcher.Action.MODIFIED, testCustomResource));
 
@@ -106,7 +105,6 @@ void removesDefaultFinalizerOnDelete() {
     void doesNotRemovesTheFinalizerIfTheDeleteMethodRemovesFalse() {
         when(resourceController.deleteResource(eq(testCustomResource), any())).thenReturn(false);
         markForDeletion(testCustomResource);
-        testCustomResource.getMetadata().getFinalizers().add(DEFAULT_FINALIZER);
 
         eventDispatcher.handleEvent(customResourceEvent(Watcher.Action.MODIFIED, testCustomResource));
 
@@ -116,7 +114,6 @@ void doesNotRemovesTheFinalizerIfTheDeleteMethodRemovesFalse() {
 
     @Test
     void doesNotUpdateTheResourceIfNoUpdateUpdateControl() {
-        testCustomResource.getMetadata().getFinalizers().add(DEFAULT_FINALIZER);
         when(resourceController.createOrUpdateResource(eq(testCustomResource), any())).thenReturn(UpdateControl.noUpdate());
 
         eventDispatcher.handleEvent(customResourceEvent(Watcher.Action.MODIFIED, testCustomResource));
@@ -126,6 +123,7 @@ void doesNotUpdateTheResourceIfNoUpdateUpdateControl() {
 
     @Test
     void addsFinalizerIfNotMarkedForDeletionAndEmptyCustomResourceReturned() {
+        removeFinalizers(testCustomResource);
         when(resourceController.createOrUpdateResource(eq(testCustomResource), any())).thenReturn(UpdateControl.noUpdate());
 
         eventDispatcher.handleEvent(customResourceEvent(Watcher.Action.MODIFIED, testCustomResource));
@@ -135,20 +133,24 @@ void addsFinalizerIfNotMarkedForDeletionAndEmptyCustomResourceReturned() {
     }
 
     @Test
-    void doesNotAddFinalizerIfNoUpdateIsReturnedButMarkedForDeletion() {
+    void doesNotCallDeleteIfMarkedForDeletionButNotOurFinalizer() {
+        removeFinalizers(testCustomResource);
         markForDeletion(testCustomResource);
-        when(resourceController.createOrUpdateResource(eq(testCustomResource), any())).thenReturn(UpdateControl.noUpdate());
 
         eventDispatcher.handleEvent(customResourceEvent(Watcher.Action.MODIFIED, testCustomResource));
 
-        assertEquals(0, testCustomResource.getMetadata().getFinalizers().size());
         verify(customResourceFacade, never()).replaceWithLock(any());
+        verify(resourceController, never()).deleteResource(eq(testCustomResource), any());
     }
 
     private void markForDeletion(CustomResource customResource) {
         customResource.getMetadata().setDeletionTimestamp("2019-8-10");
     }
 
+    private void removeFinalizers(CustomResource customResource) {
+        customResource.getMetadata().getFinalizers().clear();
+    }
+
     CustomResource getResource() {
         TestCustomResource resource = new TestCustomResource();
         resource.setMetadata(new ObjectMetaBuilder()
@@ -157,6 +159,7 @@ CustomResource getResource() {
                 .withDeletionGracePeriodSeconds(10L)
                 .withGeneration(10L)
                 .withName("name")
+                .withFinalizers(DEFAULT_FINALIZER)
                 .withNamespace("namespace")
                 .withResourceVersion("resourceVersion")
                 .withSelfLink("selfLink")

operator-framework/src/test/java/com/github/containersolutions/operator/SubResourceUpdateIT.java

@@ -44,6 +44,21 @@ public void updatesSubResourceStatus() {
         });
     }
 
+    @Test
+    public void ifNoFinalizerPresentFirstAddsTheFinalizerThenExecutesControllerAgain() {
+        initAndCleanup();
+        integrationTestSupport.teardownIfSuccess(() -> {
+            SubResourceTestCustomResource resource = createTestCustomResource("1");
+            resource.getMetadata().getFinalizers().clear();
+            integrationTestSupport.getCrOperations().inNamespace(TEST_NAMESPACE).create(resource);
+
+            awaitStatusUpdated(resource.getMetadata().getName());
+            // wait for sure, there are no more events
+            waitXms(200);
+            // there is no event on status update processed
+            assertThat(integrationTestSupport.numberOfControllerExecutions()).isEqualTo(2);
+        });
+    }
 
     void awaitStatusUpdated(String name) {
         await("cr status updated").atMost(5, TimeUnit.SECONDS)