improve: blocklist of problematic resources for previous version annotation

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java

@@ -1,6 +1,7 @@
 package io.javaoperatorsdk.operator.api.config;
 
 import java.time.Duration;
+import java.util.List;
 import java.util.Optional;
 import java.util.Set;
 import java.util.concurrent.ExecutorService;
@@ -13,6 +14,8 @@
 import io.fabric8.kubernetes.api.model.ConfigMap;
 import io.fabric8.kubernetes.api.model.HasMetadata;
 import io.fabric8.kubernetes.api.model.Secret;
+import io.fabric8.kubernetes.api.model.apps.Deployment;
+import io.fabric8.kubernetes.api.model.apps.StatefulSet;
 import io.fabric8.kubernetes.client.Config;
 import io.fabric8.kubernetes.client.ConfigBuilder;
 import io.fabric8.kubernetes.client.CustomResource;
@@ -448,6 +451,25 @@ default boolean previousAnnotationForDependentResourcesEventFiltering() {
     return true;
   }
 
+  /**
+   * For dependent resources framework can add an annotation to filter our events that are results
+   * of changes made by the framework. There are, however, few resources that do not follow the K8S
+   * API convention that changes in metadata do not increase the "metadata.generation". For these
+   * resources, the generation is increased by adding the annotation and their controller increases
+   * the observedGeneration in the status. This results in a new event, that if not handled
+   * correctly with the resource matcher yet again results in an update and a previous version
+   * annotation change, thus results in an infinite loop.
+   *
+   * <p>As a workaround, we automatically skip adding previous annotation for those well-known
+   * resources. Note that if you are sure that the matcher works (most of the cases does) for your
+   * case, you can remove the resource from the blocklist.
+   *
+   * @return blocklist of resource classes where the previous version annotation won't be used.
+   */
+  default List<Class<? extends HasMetadata>> previousAnnotationUsageBlocklist() {
+    return List.of(Deployment.class, StatefulSet.class);
+  }
+
   /**
    * If the event logic should parse the resourceVersion to determine the ordering of dependent
    * resource events. This is typically not needed.

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java

@@ -1,6 +1,7 @@
 package io.javaoperatorsdk.operator.api.config;
 
 import java.time.Duration;
+import java.util.List;
 import java.util.Optional;
 import java.util.Set;
 import java.util.concurrent.ExecutorService;
@@ -40,6 +41,7 @@ public class ConfigurationServiceOverrider {
   private Boolean parseResourceVersions;
   private Boolean useSSAToPatchPrimaryResource;
   private Boolean cloneSecondaryResourcesWhenGettingFromCache;
+  private List<Class<? extends HasMetadata>> previousAnnotationUsageBlocklist;
 
   @SuppressWarnings("rawtypes")
   private DependentResourceFactory dependentResourceFactory;
@@ -188,6 +190,12 @@ public ConfigurationServiceOverrider withCloneSecondaryResourcesWhenGettingFromC
     return this;
   }
 
+  public ConfigurationServiceOverrider previousAnnotationUsageBlocklist(
+      List<Class<? extends HasMetadata>> previousAnnotationUsageBlacklist) {
+    this.previousAnnotationUsageBlocklist = previousAnnotationUsageBlacklist;
+    return this;
+  }
+
   public ConfigurationService build() {
     return new BaseConfigurationService(original.getVersion(), cloner, client) {
       @Override
@@ -328,6 +336,13 @@ public boolean cloneSecondaryResourcesWhenGettingFromCache() {
             cloneSecondaryResourcesWhenGettingFromCache,
             ConfigurationService::cloneSecondaryResourcesWhenGettingFromCache);
       }
+
+      @Override
+      public List<Class<? extends HasMetadata>> previousAnnotationUsageBlocklist() {
+        return overriddenValueOrDefault(
+            previousAnnotationUsageBlocklist,
+            ConfigurationService::previousAnnotationUsageBlocklist);
+      }
     };
   }
 }

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java

@@ -40,6 +40,7 @@ public abstract class KubernetesDependentResource<R extends HasMetadata, P exten
   private final boolean garbageCollected = this instanceof GarbageCollected;
   private KubernetesDependentResourceConfig<R> kubernetesDependentResourceConfig;
   private volatile Boolean useSSA;
+  private volatile Boolean usePreviousAnnotationForEventFiltering;
 
   public KubernetesDependentResource(Class<R> resourceType) {
     this(resourceType, null);
@@ -160,10 +161,19 @@ protected boolean useSSA(Context<P> context) {
   }
 
   private boolean usePreviousAnnotation(Context<P> context) {
-    return context
-        .getControllerConfiguration()
-        .getConfigurationService()
-        .previousAnnotationForDependentResourcesEventFiltering();
+    if (usePreviousAnnotationForEventFiltering == null) {
+      usePreviousAnnotationForEventFiltering =
+          context
+                  .getControllerConfiguration()
+                  .getConfigurationService()
+                  .previousAnnotationForDependentResourcesEventFiltering()
+              && !context
+                  .getControllerConfiguration()
+                  .getConfigurationService()
+                  .previousAnnotationUsageBlocklist()
+                  .contains(this.resourceType());
+    }
+    return usePreviousAnnotationForEventFiltering;
   }
 
   @Override

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java

@@ -198,7 +198,7 @@ List<Object> sortListItems(List<Object> list) {
   }
 
   /** Correct for known issue with SSA */
-  private void sanitizeState(R actual, R desired, Map<String, Object> actualMap) {
+  protected void sanitizeState(R actual, R desired, Map<String, Object> actualMap) {
     if (actual instanceof StatefulSet actualStatefulSet
         && desired instanceof StatefulSet desiredStatefulSet) {
       var actualSpec = actualStatefulSet.getSpec();

operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/prevblocklist/DeploymentDependent.java

@@ -0,0 +1,95 @@
+package io.javaoperatorsdk.operator.dependent.prevblocklist;
+
+import java.util.Map;
+
+import io.fabric8.kubernetes.api.model.ContainerBuilder;
+import io.fabric8.kubernetes.api.model.HasMetadata;
+import io.fabric8.kubernetes.api.model.LabelSelectorBuilder;
+import io.fabric8.kubernetes.api.model.ObjectMetaBuilder;
+import io.fabric8.kubernetes.api.model.PodSpecBuilder;
+import io.fabric8.kubernetes.api.model.PodTemplateSpecBuilder;
+import io.fabric8.kubernetes.api.model.Quantity;
+import io.fabric8.kubernetes.api.model.ResourceRequirementsBuilder;
+import io.fabric8.kubernetes.api.model.apps.Deployment;
+import io.fabric8.kubernetes.api.model.apps.DeploymentBuilder;
+import io.fabric8.kubernetes.api.model.apps.DeploymentSpecBuilder;
+import io.javaoperatorsdk.operator.api.reconciler.Context;
+import io.javaoperatorsdk.operator.processing.dependent.kubernetes.CRUDKubernetesDependentResource;
+import io.javaoperatorsdk.operator.processing.dependent.kubernetes.GenericKubernetesResourceMatcher;
+import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependent;
+import io.javaoperatorsdk.operator.processing.dependent.kubernetes.SSABasedGenericKubernetesResourceMatcher;
+
+@KubernetesDependent
+public class DeploymentDependent
+    extends CRUDKubernetesDependentResource<Deployment, PrevAnnotationBlockCustomResource> {
+
+  public static final String RESOURCE_NAME = "test1";
+
+  public DeploymentDependent() {
+    super(Deployment.class);
+  }
+
+  @Override
+  protected Deployment desired(
+      PrevAnnotationBlockCustomResource primary,
+      Context<PrevAnnotationBlockCustomResource> context) {
+
+    return new DeploymentBuilder()
+        .withMetadata(
+            new ObjectMetaBuilder()
+                .withName(primary.getMetadata().getName())
+                .withNamespace(primary.getMetadata().getNamespace())
+                .build())
+        .withSpec(
+            new DeploymentSpecBuilder()
+                .withReplicas(1)
+                .withSelector(
+                    new LabelSelectorBuilder().withMatchLabels(Map.of("app", "nginx")).build())
+                .withTemplate(
+                    new PodTemplateSpecBuilder()
+                        .withMetadata(
+                            new ObjectMetaBuilder().withLabels(Map.of("app", "nginx")).build())
+                        .withSpec(
+                            new PodSpecBuilder()
+                                .withContainers(
+                                    new ContainerBuilder()
+                                        .withName("nginx")
+                                        .withImage("nginx:1.14.2")
+                                        .withResources(
+                                            new ResourceRequirementsBuilder()
+                                                .withLimits(Map.of("cpu", new Quantity("2000m")))
+                                                .build())
+                                        .build())
+                                .build())
+                        .build())
+                .build())
+        .build();
+  }
+
+  // for testing purposes replicating the matching logic but with the special matcher
+  @Override
+  public Result<Deployment> match(
+      Deployment actualResource,
+      Deployment desired,
+      PrevAnnotationBlockCustomResource primary,
+      Context<PrevAnnotationBlockCustomResource> context) {
+    final boolean matches;
+    addMetadata(true, actualResource, desired, primary, context);
+    if (useSSA(context)) {
+      matches = new SSAMatcherWithoutSanitization().matches(actualResource, desired, context);
+    } else {
+      matches =
+          GenericKubernetesResourceMatcher.match(desired, actualResource, false, false, context)
+              .matched();
+    }
+    return Result.computed(matches, desired);
+  }
+
+  // using this matcher, so we are able to reproduce issue with resource limits
+  static class SSAMatcherWithoutSanitization<R extends HasMetadata>
+      extends SSABasedGenericKubernetesResourceMatcher<R> {
+
+    @Override
+    protected void sanitizeState(R actual, R desired, Map<String, Object> actualMap) {}
+  }
+}

operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/prevblocklist/PrevAnnotationBlockCustomResource.java

@@ -0,0 +1,13 @@
+package io.javaoperatorsdk.operator.dependent.prevblocklist;
+
+import io.fabric8.kubernetes.api.model.Namespaced;
+import io.fabric8.kubernetes.client.CustomResource;
+import io.fabric8.kubernetes.model.annotation.Group;
+import io.fabric8.kubernetes.model.annotation.ShortNames;
+import io.fabric8.kubernetes.model.annotation.Version;
+
+@Group("sample.javaoperatorsdk")
+@Version("v1")
+@ShortNames("pabc")
+public class PrevAnnotationBlockCustomResource extends CustomResource<PrevAnnotationBlockSpec, Void>
+    implements Namespaced {}

operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/prevblocklist/PrevAnnotationBlockReconciler.java

@@ -0,0 +1,34 @@
+package io.javaoperatorsdk.operator.dependent.prevblocklist;
+
+import java.util.concurrent.atomic.AtomicInteger;
+
+import io.javaoperatorsdk.operator.api.reconciler.Context;
+import io.javaoperatorsdk.operator.api.reconciler.ControllerConfiguration;
+import io.javaoperatorsdk.operator.api.reconciler.Reconciler;
+import io.javaoperatorsdk.operator.api.reconciler.UpdateControl;
+import io.javaoperatorsdk.operator.api.reconciler.Workflow;
+import io.javaoperatorsdk.operator.api.reconciler.dependent.Dependent;
+import io.javaoperatorsdk.operator.support.TestExecutionInfoProvider;
+
+@Workflow(dependents = {@Dependent(type = DeploymentDependent.class)})
+@ControllerConfiguration()
+public class PrevAnnotationBlockReconciler
+    implements Reconciler<PrevAnnotationBlockCustomResource>, TestExecutionInfoProvider {
+
+  private final AtomicInteger numberOfExecutions = new AtomicInteger(0);
+
+  public PrevAnnotationBlockReconciler() {}
+
+  @Override
+  public UpdateControl<PrevAnnotationBlockCustomResource> reconcile(
+      PrevAnnotationBlockCustomResource resource,
+      Context<PrevAnnotationBlockCustomResource> context) {
+    numberOfExecutions.getAndIncrement();
+
+    return UpdateControl.noUpdate();
+  }
+
+  public int getNumberOfExecutions() {
+    return numberOfExecutions.get();
+  }
+}

operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/prevblocklist/PrevAnnotationBlockReconcilerIT.java

@@ -0,0 +1,50 @@
+package io.javaoperatorsdk.operator.dependent.prevblocklist;
+
+import java.time.Duration;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
+
+import io.fabric8.kubernetes.api.model.ObjectMetaBuilder;
+import io.fabric8.kubernetes.api.model.apps.Deployment;
+import io.javaoperatorsdk.operator.junit.LocallyRunOperatorExtension;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.awaitility.Awaitility.await;
+
+class PrevAnnotationBlockReconcilerIT {
+
+  public static final String TEST_1 = "test1";
+
+  @RegisterExtension
+  LocallyRunOperatorExtension extension =
+      LocallyRunOperatorExtension.builder()
+          // Removing resource from blocklist List would result in test failure
+          //          .withConfigurationService(
+          //              o -> o.previousAnnotationUsageBlocklist(Collections.emptyList()))
+          .withReconciler(PrevAnnotationBlockReconciler.class)
+          .build();
+
+  @Test
+  void doNotUsePrevAnnotationForDeploymentDependent() {
+    extension.create(testResource(TEST_1));
+
+    var reconciler = extension.getReconcilerOfType(PrevAnnotationBlockReconciler.class);
+    await()
+        .pollDelay(Duration.ofMillis(200))
+        .untilAsserted(
+            () -> {
+              var deployment = extension.get(Deployment.class, TEST_1);
+              assertThat(deployment).isNotNull();
+              assertThat(reconciler.getNumberOfExecutions()).isGreaterThan(0).isLessThan(10);
+            });
+  }
+
+  PrevAnnotationBlockCustomResource testResource(String name) {
+    var res = new PrevAnnotationBlockCustomResource();
+    res.setMetadata(new ObjectMetaBuilder().withName(name).build());
+    res.setSpec(new PrevAnnotationBlockSpec());
+    res.getSpec().setValue("value");
+    return res;
+  }
+}

operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/prevblocklist/PrevAnnotationBlockSpec.java

@@ -0,0 +1,15 @@
+package io.javaoperatorsdk.operator.dependent.prevblocklist;
+
+public class PrevAnnotationBlockSpec {
+
+  private String value;
+
+  public String getValue() {
+    return value;
+  }
+
+  public PrevAnnotationBlockSpec setValue(String value) {
+    this.value = value;
+    return this;
+  }
+}