fix: cleaner issue for pod caused by missing generation in metadata (#1600)

Author: csviri

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/controller/InternalEventFilters.java

@@ -18,6 +18,11 @@ static <T extends HasMetadata> OnUpdateFilter<T> onUpdateGenerationAware(
       if (!generationAware) {
         return true;
       }
+      // for example pods don't have generation
+      if (oldResource.getMetadata().getGeneration() == null) {
+        return true;
+      }
+
       return oldResource.getMetadata().getGeneration() < newResource
           .getMetadata().getGeneration();
     };

operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/controller/InternalEventFiltersTest.java

@@ -4,6 +4,8 @@
 
 import org.junit.jupiter.api.Test;
 
+import io.fabric8.kubernetes.api.model.ObjectMetaBuilder;
+import io.fabric8.kubernetes.api.model.Service;
 import io.javaoperatorsdk.operator.TestUtils;
 
 import static io.javaoperatorsdk.operator.TestUtils.markForDeletion;
@@ -31,6 +33,12 @@ void generationAware() {
     assertThat(InternalEventFilters.onUpdateGenerationAware(false).accept(res, res)).isTrue();
   }
 
+  @Test
+  void acceptsEventIfNoGenerationOnResource() {
+    assertThat(InternalEventFilters.onUpdateGenerationAware(true)
+        .accept(testService(), testService())).isTrue();
+  }
+
   @Test
   void finalizerCheckedIfConfigured() {
     assertThat(InternalEventFilters.onUpdateFinalizerNeededAndApplied(true, FINALIZER)
@@ -57,4 +65,10 @@ void dontAcceptIfFinalizerNotUsed() {
     assertThat(InternalEventFilters.onUpdateFinalizerNeededAndApplied(false, FINALIZER)
         .accept(TestUtils.testCustomResource1(), TestUtils.testCustomResource1())).isFalse();
   }
+
+  Service testService() {
+    var service = new Service();
+    service.setMetadata(new ObjectMetaBuilder().withName("test").withNamespace("default").build());
+    return service;
+  }
 }

operator-framework-junit5/src/main/java/io/javaoperatorsdk/operator/junit/AbstractOperatorExtension.java

@@ -34,13 +34,15 @@ public abstract class AbstractOperatorExtension implements HasKubernetesClient,
 
   private static final Logger LOGGER = LoggerFactory.getLogger(AbstractOperatorExtension.class);
   public static final int CRD_READY_WAIT = 2000;
+  public static final int DEFAULT_NAMESPACE_DELETE_TIMEOUT = 90;
 
   private final KubernetesClient kubernetesClient;
   protected final List<HasMetadata> infrastructure;
   protected Duration infrastructureTimeout;
   protected final boolean oneNamespacePerClass;
   protected final boolean preserveNamespaceOnError;
   protected final boolean waitForNamespaceDeletion;
+  protected final int namespaceDeleteTimeout = DEFAULT_NAMESPACE_DELETE_TIMEOUT;
 
   protected String namespace;
 
@@ -198,7 +200,7 @@ protected void after(ExtensionContext context) {
           LOGGER.info("Waiting for namespace {} to be deleted", namespace);
           Awaitility.await("namespace deleted")
               .pollInterval(50, TimeUnit.MILLISECONDS)
-              .atMost(90, TimeUnit.SECONDS)
+              .atMost(namespaceDeleteTimeout, TimeUnit.SECONDS)
               .until(() -> kubernetesClient.namespaces().withName(namespace).get() == null);
         }
       }
@@ -216,6 +218,7 @@ public static abstract class AbstractBuilder<T extends AbstractBuilder<T>> {
     protected boolean preserveNamespaceOnError;
     protected boolean waitForNamespaceDeletion;
     protected boolean oneNamespacePerClass;
+    protected int namespaceDeleteTimeout;
 
     protected AbstractBuilder() {
       this.infrastructure = new ArrayList<>();
@@ -232,6 +235,10 @@ protected AbstractBuilder() {
       this.oneNamespacePerClass = Utils.getSystemPropertyOrEnvVar(
           "josdk.it.oneNamespacePerClass",
           false);
+
+      this.namespaceDeleteTimeout = Utils.getSystemPropertyOrEnvVar(
+          "josdk.it.namespaceDeleteTimeout",
+          DEFAULT_NAMESPACE_DELETE_TIMEOUT);
     }
 
     public T preserveNamespaceOnError(boolean value) {
@@ -269,5 +276,9 @@ public T withInfrastructure(HasMetadata... hms) {
       return (T) this;
     }
 
+    public T withNamespaceDeleteTimeout(int timeout) {
+      this.namespaceDeleteTimeout = timeout;
+      return (T) this;
+    }
   }
 }

operator-framework/src/test/java/io/javaoperatorsdk/operator/BuiltInResourceCleanerIT.java

@@ -0,0 +1,57 @@
+package io.javaoperatorsdk.operator;
+
+import java.util.Map;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import io.fabric8.kubernetes.api.model.Service;
+import io.javaoperatorsdk.operator.junit.LocallyRunOperatorExtension;
+import io.javaoperatorsdk.operator.sample.builtinresourcecleaner.ObservedGenerationTestReconciler;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.awaitility.Awaitility.await;
+
+class BuiltInResourceCleanerIT {
+
+  private static final Logger log = LoggerFactory.getLogger(BuiltInResourceCleanerIT.class);
+
+  @RegisterExtension
+  LocallyRunOperatorExtension operator =
+      LocallyRunOperatorExtension.builder()
+          .withReconciler(new ObservedGenerationTestReconciler())
+          .build();
+
+  /**
+   * Issue is with generation, some built in resources like Pod, Service does not seem to use
+   * generation.
+   */
+  @Test
+  void cleanerIsCalledOnBuiltInResource() {
+    var service = operator.create(testService());
+
+    await().untilAsserted(() -> {
+      assertThat(operator.getReconcilerOfType(ObservedGenerationTestReconciler.class)
+          .getReconcileCount()).isPositive();
+      var actualService = operator.get(Service.class, service.getMetadata().getName());
+      assertThat(actualService.getMetadata().getFinalizers()).isNotEmpty();
+    });
+
+    operator.delete(service);
+
+    await().untilAsserted(() -> {
+      assertThat(operator.getReconcilerOfType(ObservedGenerationTestReconciler.class)
+          .getCleanCount()).isPositive();
+    });
+  }
+
+  Service testService() {
+    Service service = ReconcilerUtils.loadYaml(Service.class, StandaloneDependentResourceIT.class,
+        "service-template.yaml");
+    service.getMetadata().setLabels(Map.of("builtintest", "true"));
+    return service;
+  }
+
+}

operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/builtinresourcecleaner/ObservedGenerationTestReconciler.java

@@ -0,0 +1,41 @@
+package io.javaoperatorsdk.operator.sample.builtinresourcecleaner;
+
+import java.util.concurrent.atomic.AtomicInteger;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import io.fabric8.kubernetes.api.model.Service;
+import io.javaoperatorsdk.operator.api.reconciler.*;
+
+@ControllerConfiguration(labelSelector = "builtintest=true")
+public class ObservedGenerationTestReconciler
+    implements Reconciler<Service>, Cleaner<Service> {
+
+  private static final Logger log = LoggerFactory.getLogger(ObservedGenerationTestReconciler.class);
+
+  private AtomicInteger reconciled = new AtomicInteger(0);
+  private AtomicInteger cleaned = new AtomicInteger(0);
+
+  @Override
+  public UpdateControl<Service> reconcile(
+      Service resource,
+      Context<Service> context) {
+    reconciled.addAndGet(1);
+    return UpdateControl.noUpdate();
+  }
+
+  @Override
+  public DeleteControl cleanup(Service resource, Context<Service> context) {
+    cleaned.addAndGet(1);
+    return DeleteControl.defaultDelete();
+  }
+
+  public int getReconcileCount() {
+    return reconciled.get();
+  }
+
+  public int getCleanCount() {
+    return cleaned.get();
+  }
+}

operator-framework/src/test/resources/io/javaoperatorsdk/operator/service-template.yaml

@@ -0,0 +1,11 @@
+apiVersion: v1
+kind: Service
+metadata:
+  name: my-service
+spec:
+  selector:
+    app.kubernetes.io/name: MyApp
+  ports:
+    - protocol: TCP
+      port: 80
+      targetPort: 9376

pod.yaml

@@ -0,0 +1,10 @@
+apiVersion: v1
+kind: Pod
+metadata:
+  name: nginx
+spec:
+  containers:
+    - name: nginx
+      image: nginx:1.14.2
+      ports:
+        - containerPort: 80