refactor: updateMatcher removal (#2431)

Author: csviri

Diff for: operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericKubernetesResourceMatcher.java

@@ -4,17 +4,14 @@
 import java.util.Collections;
 import java.util.List;
 
-import io.fabric8.kubernetes.api.model.ConfigMap;
 import io.fabric8.kubernetes.api.model.HasMetadata;
-import io.fabric8.kubernetes.api.model.Secret;
 import io.fabric8.zjsonpatch.JsonDiff;
 import io.javaoperatorsdk.operator.api.reconciler.Context;
 import io.javaoperatorsdk.operator.processing.dependent.Matcher;
 
 import com.fasterxml.jackson.databind.JsonNode;
 
-public class GenericKubernetesResourceMatcher<R extends HasMetadata, P extends HasMetadata>
-    implements Matcher<R, P> {
+public class GenericKubernetesResourceMatcher<R extends HasMetadata, P extends HasMetadata> {
 
   private static final String SPEC = "/spec";
   private static final String METADATA = "/metadata";
@@ -26,40 +23,7 @@ public class GenericKubernetesResourceMatcher<R extends HasMetadata, P extends H
 
   private static final String PATH = "path";
   private static final String[] EMPTY_ARRAY = {};
-  private final KubernetesDependentResource<R, P> dependentResource;
 
-  private GenericKubernetesResourceMatcher(KubernetesDependentResource<R, P> dependentResource) {
-    this.dependentResource = dependentResource;
-  }
-
-  @SuppressWarnings({"unchecked", "rawtypes", "unused"})
-  static <R extends HasMetadata, P extends HasMetadata> Matcher<R, P> matcherFor(
-      KubernetesDependentResource<R, P> dependentResource) {
-    return new GenericKubernetesResourceMatcher(dependentResource);
-  }
-
-  /**
-   * {@inheritDoc}
-   * <p>
-   * This implementation attempts to cover most common cases out of the box by considering
-   * non-additive changes to the resource's spec (if the resource in question has a {@code spec}
-   * field), making special provisions for {@link ConfigMap} and {@link Secret} resources. Additive
-   * changes (i.e. a field is added that previously didn't exist) are not considered as triggering a
-   * mismatch by default to account for validating webhooks that might add default values
-   * automatically when not present or some other controller adding labels and/or annotations.
-   * </p>
-   * <p>
-   * It should be noted that this implementation is potentially intensive because it generically
-   * attempts to cover common use cases by performing diffs on the JSON representation of objects.
-   * If performance is a concern, it might be easier / simpler to provide a {@link Matcher}
-   * implementation optimized for your use case.
-   * </p>
-   */
-  @Override
-  public Result<R> match(R actualResource, P primary, Context<P> context) {
-    var desired = dependentResource.desired(primary, context);
-    return match(desired, actualResource, false, false, context);
-  }
 
   /**
    * Determines whether the specified actual resource matches the specified desired resource,
@@ -84,14 +48,20 @@ public Result<R> match(R actualResource, P primary, Context<P> context) {
    * @param <R> resource
    * @return results of matching
    */
-  public static <R extends HasMetadata, P extends HasMetadata> Result<R> match(R desired,
+  public static <R extends HasMetadata, P extends HasMetadata> Matcher.Result<R> match(R desired,
       R actualResource,
       boolean labelsAndAnnotationsEquality,
       boolean valuesEquality, Context<P> context) {
     return match(desired, actualResource,
         labelsAndAnnotationsEquality, valuesEquality, context, EMPTY_ARRAY);
   }
 
+  public static <R extends HasMetadata, P extends HasMetadata> Matcher.Result<R> match(R desired,
+      R actualResource, Context<P> context) {
+    return match(desired, actualResource,
+        false, false, context, EMPTY_ARRAY);
+  }
+
   /**
    * Determines whether the specified actual resource matches the specified desired resource,
    * possibly considering metadata and deeper equality checks.
@@ -108,7 +78,7 @@ public static <R extends HasMetadata, P extends HasMetadata> Result<R> match(R d
    * @param <R> resource
    * @return results of matching
    */
-  public static <R extends HasMetadata, P extends HasMetadata> Result<R> match(R desired,
+  public static <R extends HasMetadata, P extends HasMetadata> Matcher.Result<R> match(R desired,
       R actualResource,
       boolean labelsAndAnnotationsEquality,
       Context<P> context, String... ignorePaths) {
@@ -139,7 +109,7 @@ public static <R extends HasMetadata, P extends HasMetadata> Result<R> match(R d
    *        match fails.
    * @return a {@link io.javaoperatorsdk.operator.processing.dependent.Matcher.Result} object
    */
-  public static <R extends HasMetadata, P extends HasMetadata> Result<R> match(
+  public static <R extends HasMetadata, P extends HasMetadata> Matcher.Result<R> match(
       KubernetesDependentResource<R, P> dependentResource, R actualResource, P primary,
       Context<P> context,
       boolean labelsAndAnnotationsEquality,
@@ -150,7 +120,7 @@ public static <R extends HasMetadata, P extends HasMetadata> Result<R> match(
         ignorePaths);
   }
 
-  public static <R extends HasMetadata, P extends HasMetadata> Result<R> match(
+  public static <R extends HasMetadata, P extends HasMetadata> Matcher.Result<R> match(
       KubernetesDependentResource<R, P> dependentResource, R actualResource, P primary,
       Context<P> context,
       boolean specEquality,
@@ -161,7 +131,7 @@ public static <R extends HasMetadata, P extends HasMetadata> Result<R> match(
         labelsAndAnnotationsEquality, specEquality, context, ignorePaths);
   }
 
-  public static <R extends HasMetadata, P extends HasMetadata> Result<R> match(R desired,
+  public static <R extends HasMetadata, P extends HasMetadata> Matcher.Result<R> match(R desired,
       R actualResource, boolean labelsAndAnnotationsEquality, boolean valuesEquality,
       Context<P> context,
       String... ignoredPaths) {
@@ -194,7 +164,7 @@ public static <R extends HasMetadata, P extends HasMetadata> Result<R> match(R d
       }
     }
 
-    return Result.computed(matched, desired);
+    return Matcher.Result.computed(matched, desired);
   }
 
   private static boolean match(boolean equality, JsonNode diff,

Diff for: operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/updatermatcher/GenericResourceUpdaterMatcher.java renamed to operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericResourceUpdater.java

@@ -1,29 +1,17 @@
-package io.javaoperatorsdk.operator.processing.dependent.kubernetes.updatermatcher;
+package io.javaoperatorsdk.operator.processing.dependent.kubernetes;
 
 import java.util.Map;
 
 import io.fabric8.kubernetes.api.model.HasMetadata;
 import io.fabric8.kubernetes.client.utils.KubernetesSerialization;
 import io.javaoperatorsdk.operator.api.reconciler.Context;
-import io.javaoperatorsdk.operator.processing.dependent.kubernetes.GenericKubernetesResourceMatcher;
-import io.javaoperatorsdk.operator.processing.dependent.kubernetes.ResourceUpdaterMatcher;
 
-public class GenericResourceUpdaterMatcher<R extends HasMetadata> implements
-    ResourceUpdaterMatcher<R> {
+public class GenericResourceUpdater {
 
   private static final String METADATA = "metadata";
-  private static final ResourceUpdaterMatcher<?> INSTANCE = new GenericResourceUpdaterMatcher<>();
-
-  protected GenericResourceUpdaterMatcher() {}
-
-  @SuppressWarnings("unchecked")
-  public static <R extends HasMetadata> ResourceUpdaterMatcher<R> updaterMatcherFor() {
-    return (ResourceUpdaterMatcher<R>) INSTANCE;
-  }
 
   @SuppressWarnings("unchecked")
-  @Override
-  public R updateResource(R actual, R desired, Context<?> context) {
+  public static <R extends HasMetadata> R updateResource(R actual, R desired, Context<?> context) {
     KubernetesSerialization kubernetesSerialization =
         context.getClient().getKubernetesSerialization();
     Map<String, Object> actualMap = kubernetesSerialization.convertValue(actual, Map.class);
@@ -40,12 +28,6 @@ public R updateResource(R actual, R desired, Context<?> context) {
     return clonedActual;
   }
 
-  @Override
-  public boolean matches(R actual, R desired, Context<?> context) {
-    return GenericKubernetesResourceMatcher.match(desired, actual,
-        false, false, context).matched();
-  }
-
   public static <K extends HasMetadata> void updateLabelsAndAnnotation(K actual, K desired) {
     actual.getMetadata().getLabels().putAll(desired.getMetadata().getLabels());
     actual.getMetadata().getAnnotations().putAll(desired.getMetadata().getAnnotations());

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

@@ -21,7 +21,6 @@
 import io.javaoperatorsdk.operator.api.reconciler.dependent.managed.ConfiguredDependentResource;
 import io.javaoperatorsdk.operator.processing.dependent.AbstractEventSourceHolderDependentResource;
 import io.javaoperatorsdk.operator.processing.dependent.Matcher.Result;
-import io.javaoperatorsdk.operator.processing.dependent.kubernetes.updatermatcher.GenericResourceUpdaterMatcher;
 import io.javaoperatorsdk.operator.processing.event.ResourceID;
 import io.javaoperatorsdk.operator.processing.event.source.SecondaryToPrimaryMapper;
 import io.javaoperatorsdk.operator.processing.event.source.informer.InformerEventSource;
@@ -38,10 +37,7 @@ public abstract class KubernetesDependentResource<R extends HasMetadata, P exten
   private static final Logger log = LoggerFactory.getLogger(KubernetesDependentResource.class);
   private final boolean garbageCollected = this instanceof GarbageCollected;
   private final boolean usingCustomResourceUpdateMatcher = this instanceof ResourceUpdaterMatcher;
-  @SuppressWarnings("unchecked")
-  private final ResourceUpdaterMatcher<R> updaterMatcher = usingCustomResourceUpdateMatcher
-      ? (ResourceUpdaterMatcher<R>) this
-      : GenericResourceUpdaterMatcher.updaterMatcherFor();
+
   private KubernetesDependentResourceConfig<R> kubernetesDependentResourceConfig;
 
   public KubernetesDependentResource(Class<R> resourceType) {
@@ -57,7 +53,6 @@ public void configureWith(KubernetesDependentResourceConfig<R> config) {
     this.kubernetesDependentResourceConfig = config;
   }
 
-
   @SuppressWarnings("unused")
   public R create(R desired, P primary, Context<P> context) {
     if (useSSA(context)) {
@@ -91,7 +86,7 @@ public R update(R actual, R desired, P primary, Context<P> context) {
           .fieldManager(context.getControllerConfiguration().fieldManager())
           .forceConflicts().serverSideApply();
     } else {
-      var updatedActual = updaterMatcher.updateResource(actual, desired, context);
+      var updatedActual = GenericResourceUpdater.updateResource(actual, desired, context);
       updatedResource = prepare(context, updatedActual, primary, "Updating").update();
     }
     log.debug("Resource version after update: {}",
@@ -102,25 +97,19 @@ public R update(R actual, R desired, P primary, Context<P> context) {
   @Override
   public Result<R> match(R actualResource, P primary, Context<P> context) {
     final var desired = desired(primary, context);
-    return match(actualResource, desired, primary, updaterMatcher, context);
-  }
-
-  @SuppressWarnings({"unused"})
-  public Result<R> match(R actualResource, R desired, P primary, Context<P> context) {
-    return match(actualResource, desired, primary,
-        GenericResourceUpdaterMatcher.updaterMatcherFor(),
-        context);
+    return match(actualResource, desired, primary, context);
   }
 
-  public Result<R> match(R actualResource, R desired, P primary, ResourceUpdaterMatcher<R> matcher,
+  public Result<R> match(R actualResource, R desired, P primary,
       Context<P> context) {
     final boolean matches;
     addMetadata(true, actualResource, desired, primary, context);
     if (useSSA(context)) {
       matches = SSABasedGenericKubernetesResourceMatcher.getInstance()
           .matches(actualResource, desired, context);
     } else {
-      matches = matcher.matches(actualResource, desired, context);
+      matches = GenericKubernetesResourceMatcher.match(desired, actualResource,
+          false, false, context).matched();
     }
     return Result.computed(matches, desired);
   }

Diff for: operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericKubernetesResourceMatcherTest.java

@@ -13,23 +13,21 @@
 import io.javaoperatorsdk.operator.MockKubernetesClient;
 import io.javaoperatorsdk.operator.ReconcilerUtils;
 import io.javaoperatorsdk.operator.api.reconciler.Context;
-import io.javaoperatorsdk.operator.processing.dependent.Matcher;
-import io.javaoperatorsdk.operator.processing.dependent.kubernetes.updatermatcher.GenericResourceUpdaterMatcher;
 
 import static io.javaoperatorsdk.operator.processing.dependent.kubernetes.GenericKubernetesResourceMatcher.match;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
-@SuppressWarnings({"unchecked", "rawtypes"})
+@SuppressWarnings({"unchecked"})
 class GenericKubernetesResourceMatcherTest {
 
   private static final Context context = mock(Context.class);
 
   Deployment actual = createDeployment();
   Deployment desired = createDeployment();
   TestDependentResource dependentResource = new TestDependentResource(desired);
-  Matcher matcher = GenericKubernetesResourceMatcher.matcherFor(dependentResource);
+
 
   @BeforeAll
   static void setUp() {
@@ -39,15 +37,17 @@ static void setUp() {
 
   @Test
   void matchesTrivialCases() {
-    assertThat(matcher.match(actual, null, context).matched()).isTrue();
-    assertThat(matcher.match(actual, null, context).computedDesired()).isPresent();
-    assertThat(matcher.match(actual, null, context).computedDesired()).contains(desired);
+    assertThat(GenericKubernetesResourceMatcher.match(desired, actual, context).matched()).isTrue();
+    assertThat(GenericKubernetesResourceMatcher.match(desired, actual, context).computedDesired())
+        .isPresent();
+    assertThat(GenericKubernetesResourceMatcher.match(desired, actual, context).computedDesired())
+        .contains(desired);
   }
 
   @Test
   void matchesAdditiveOnlyChanges() {
     actual.getSpec().getTemplate().getMetadata().getLabels().put("new-key", "val");
-    assertThat(matcher.match(actual, null, context).matched())
+    assertThat(GenericKubernetesResourceMatcher.match(desired, actual, context).matched())
         .withFailMessage("Additive changes should not cause a mismatch by default")
         .isTrue();
   }
@@ -64,7 +64,9 @@ void matchesWithStrongSpecEquality() {
   @Test
   void doesNotMatchRemovedValues() {
     actual = createDeployment();
-    assertThat(matcher.match(actual, createPrimary("removed"), context).matched())
+    assertThat(GenericKubernetesResourceMatcher
+        .match(dependentResource.desired(createPrimary("removed"), null), actual, context)
+        .matched())
         .withFailMessage("Removing values in metadata should lead to a mismatch")
         .isFalse();
   }
@@ -73,7 +75,7 @@ void doesNotMatchRemovedValues() {
   void doesNotMatchChangedValues() {
     actual = createDeployment();
     actual.getSpec().setReplicas(2);
-    assertThat(matcher.match(actual, null, context).matched())
+    assertThat(GenericKubernetesResourceMatcher.match(desired, actual, context).matched())
         .withFailMessage("Should not have matched because values have changed")
         .isFalse();
   }
@@ -82,7 +84,7 @@ void doesNotMatchChangedValues() {
   void ignoreStatus() {
     actual = createDeployment();
     actual.setStatus(new DeploymentStatusBuilder().withReadyReplicas(1).build());
-    assertThat(matcher.match(actual, null, context).matched())
+    assertThat(GenericKubernetesResourceMatcher.match(desired, actual, context).matched())
         .withFailMessage("Should ignore status in actual")
         .isTrue();
   }
@@ -146,8 +148,8 @@ void checkServiceAccount() {
         .addNewImagePullSecret("imagePullSecret3")
         .build();
 
-    final var matcher = GenericResourceUpdaterMatcher.<ServiceAccount>updaterMatcherFor();
-    assertThat(matcher.matches(actual, desired, context)).isTrue();
+    assertThat(GenericKubernetesResourceMatcher.match(desired, actual, false, false, context)
+        .matched()).isTrue();
   }
 
   @Test