improve: non-SSA resource matching and updating (#1963)

Author: csviri

Diff for: docs/documentation/v4-4-migration.md

@@ -62,19 +62,23 @@ default [Dependent Resources](https://javaoperatorsdk.io/docs/dependent-resource
 [Server Side Apply (SSA)](https://kubernetes.io/docs/reference/using-api/server-side-apply/) to
 create and
 update Kubernetes resources. A
-new [default matching](https://github.com/java-operator-sdk/java-operator-sdk/blob/e95f9c8a8b8a8561c9a735e60fc5d82b7758df8e/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java#L163-L163)
+new [default matching](https://github.com/java-operator-sdk/java-operator-sdk/blob/2cc3bb7710adb8fca14767fbff8d93533dd05ef0/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java#L157-L157)
 algorithm is provided for `KubernetesDependentResource` that is based on `managedFields` of SSA. For
 details
-see [SSABasedGenericKubernetesResourceMatcher](https://github.com/java-operator-sdk/java-operator-sdk/blob/e95f9c8a8b8a8561c9a735e60fc5d82b7758df8e/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java)
+see [SSABasedGenericKubernetesResourceMatcher](https://github.com/java-operator-sdk/java-operator-sdk/blob/main/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java)
 
 Since those features are hard to completely test, we provided feature flags to revert to the
 legacy behavior if needed,
-see those
-in [ConfigurationService](https://github.com/java-operator-sdk/java-operator-sdk/blob/e95f9c8a8b8a8561c9a735e60fc5d82b7758df8e/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java#L268-L289)
+see 
+in [ConfigurationService](https://github.com/java-operator-sdk/java-operator-sdk/blob/2cc3bb7710adb8fca14767fbff8d93533dd05ef0/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java#L332-L347)
 
 Note that it is possible to override the related methods/behavior on class level when extending
 the `KubernetesDependentResource`. 
 
+The SSA based create/update can be combined with the legacy matcher, simply override the `match` method 
+and use the [GenericKubernetesResourceMatcher](https://github.com/java-operator-sdk/java-operator-sdk/blob/main/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericKubernetesResourceMatcher.java#L19-L19)
+directly.
+
 ### Migration from plain Update/Create to SSA Based Patch
 
 Migration to SSA might not be trivial based on the uses cases and the type of managed resources. 

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

@@ -330,27 +330,18 @@ default ExecutorServiceManager getExecutorServiceManager() {
   }
 
   /**
-   * Allows to revert to the 4.3 behavior when it comes to creating or updating Kubernetes Dependent
-   * Resources when set to {@code false}. The default approach how these resources are
-   * created/updated was change to use
+   * Allows to revert to the 4.3 behavior when it comes to creating, updating and matching
+   * Kubernetes Dependent Resources when set to {@code false}. The default approach how these
+   * resources are created/updated and match was change to use
    * <a href="https://kubernetes.io/docs/reference/using-api/server-side-apply/">Server-Side
    * Apply</a> (SSA) by default.
    *
-   * @since 4.4.0
-   */
-  default boolean ssaBasedCreateUpdateForDependentResources() {
-    return true;
-  }
-
-  /**
-   * Allows to revert to the 4.3 generic matching algorithm for Kubernetes Dependent Resources when
-   * set to {@code false}. Version 4.4 introduced a new generic matching algorithm for Kubernetes
-   * Dependent Resources which is quite complex. As a consequence, we introduced this setting to
-   * allow folks to revert to the previous matching algorithm if needed.
+   * SSA based create/update can be still used with the legacy matching, just overriding the match
+   * method of Kubernetes Dependent Resource.
    *
    * @since 4.4.0
    */
-  default boolean ssaBasedDefaultMatchingForDependentResources() {
+  default boolean ssaBasedCreateUpdateMatchForDependentResources() {
     return true;
   }
 

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

@@ -34,8 +34,7 @@ public class ConfigurationServiceOverrider {
   private Boolean stopOnInformerErrorDuringStartup;
   private Duration cacheSyncTimeout;
   private ResourceClassResolver resourceClassResolver;
-  private Boolean ssaBasedCreateUpdateForDependentResources;
-  private Boolean ssaBasedDefaultMatchingForDependentResources;
+  private Boolean ssaBasedCreateUpdateMatchForDependentResources;
 
   ConfigurationServiceOverrider(ConfigurationService original) {
     this.original = original;
@@ -145,15 +144,9 @@ public ConfigurationServiceOverrider withResourceClassResolver(
     return this;
   }
 
-  public ConfigurationServiceOverrider withSSABasedCreateUpdateForDependentResources(
+  public ConfigurationServiceOverrider withSSABasedCreateUpdateMatchForDependentResources(
       boolean value) {
-    this.ssaBasedCreateUpdateForDependentResources = value;
-    return this;
-  }
-
-  public ConfigurationServiceOverrider withSSABasedDefaultMatchingForDependentResources(
-      boolean value) {
-    this.ssaBasedDefaultMatchingForDependentResources = value;
+    this.ssaBasedCreateUpdateMatchForDependentResources = value;
     return this;
   }
 
@@ -258,17 +251,10 @@ public ResourceClassResolver getResourceClassResolver() {
       }
 
       @Override
-      public boolean ssaBasedCreateUpdateForDependentResources() {
-        return ssaBasedCreateUpdateForDependentResources != null
-            ? ssaBasedCreateUpdateForDependentResources
-            : super.ssaBasedCreateUpdateForDependentResources();
-      }
-
-      @Override
-      public boolean ssaBasedDefaultMatchingForDependentResources() {
-        return ssaBasedDefaultMatchingForDependentResources != null
-            ? ssaBasedDefaultMatchingForDependentResources
-            : super.ssaBasedDefaultMatchingForDependentResources();
+      public boolean ssaBasedCreateUpdateMatchForDependentResources() {
+        return ssaBasedCreateUpdateMatchForDependentResources != null
+            ? ssaBasedCreateUpdateMatchForDependentResources
+            : super.ssaBasedCreateUpdateMatchForDependentResources();
       }
     };
   }

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

@@ -13,14 +13,13 @@
 import io.fabric8.zjsonpatch.JsonDiff;
 import io.javaoperatorsdk.operator.api.reconciler.Context;
 import io.javaoperatorsdk.operator.processing.dependent.Matcher;
-import io.javaoperatorsdk.operator.processing.dependent.kubernetes.processors.GenericResourceUpdatePreProcessor;
 
 import com.fasterxml.jackson.databind.JsonNode;
 
 public class GenericKubernetesResourceMatcher<R extends HasMetadata, P extends HasMetadata>
     implements Matcher<R, P> {
 
-
+  private static String SPEC = "/spec";
   private static final String ADD = "add";
   private static final String OP = "op";
   public static final String METADATA_LABELS = "/metadata/labels";
@@ -171,7 +170,6 @@ public static <R extends HasMetadata, P extends HasMetadata> Result<R> match(
         labelsAndAnnotationsEquality, specEquality, context);
   }
 
-  @SuppressWarnings("unchecked")
   public static <R extends HasMetadata, P extends HasMetadata> Result<R> match(R desired,
       R actualResource,
       boolean considerMetadata, boolean labelsAndAnnotationsEquality, boolean specEquality,
@@ -194,13 +192,50 @@ public static <R extends HasMetadata, P extends HasMetadata> Result<R> match(R d
       }
     }
 
-    final ResourceUpdatePreProcessor<R> processor =
-        GenericResourceUpdatePreProcessor.processorFor((Class<R>) desired.getClass());
-    final var matched =
-        processor.matches(actualResource, desired, specEquality, context, ignoredPaths);
+    final var matched = matchSpec(actualResource, desired, specEquality, context, ignoredPaths);
     return Result.computed(matched, desired);
   }
 
+  private static <R extends HasMetadata> boolean matchSpec(R actual, R desired, boolean equality,
+      Context<?> context,
+      String[] ignoredPaths) {
+
+    final var kubernetesSerialization = context.getClient().getKubernetesSerialization();
+    var desiredNode = kubernetesSerialization.convertValue(desired, JsonNode.class);
+    var actualNode = kubernetesSerialization.convertValue(actual, JsonNode.class);
+    var wholeDiffJsonPatch = JsonDiff.asJson(desiredNode, actualNode);
+
+    final List<String> ignoreList =
+        ignoredPaths != null && ignoredPaths.length > 0 ? Arrays.asList(ignoredPaths)
+            : Collections.emptyList();
+    // reflection will be replaced by this:
+    // https://github.com/fabric8io/kubernetes-client/issues/3816
+    var specDiffJsonPatch = getDiffsImpactingPathsWithPrefixes(wholeDiffJsonPatch, SPEC);
+    // In case of equality is set to true, no diffs are allowed, so we return early if diffs exist
+    // On contrary (if equality is false), "add" is allowed for cases when for some
+    // resources Kubernetes fills-in values into spec.
+    if (equality && !specDiffJsonPatch.isEmpty()) {
+      return false;
+    }
+    if (!equality && !ignoreList.isEmpty()) {
+      if (!allDiffsOnIgnoreList(specDiffJsonPatch, ignoreList)) {
+        return false;
+      }
+    } else {
+      if (!allDiffsAreAddOps(specDiffJsonPatch)) {
+        return false;
+      }
+    }
+    return true;
+  }
+
+  private static boolean allDiffsOnIgnoreList(List<JsonNode> metadataJSonDiffs,
+      List<String> ignoreList) {
+    if (metadataJSonDiffs.isEmpty()) {
+      return false;
+    }
+    return metadataJSonDiffs.stream().allMatch(n -> nodeIsChildOf(n, ignoreList));
+  }
 
   private static <R extends HasMetadata, P extends HasMetadata> Optional<Result<R>> matchMetadata(
       R desired,

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

@@ -23,7 +23,7 @@
 import io.javaoperatorsdk.operator.api.reconciler.dependent.managed.KubernetesClientAware;
 import io.javaoperatorsdk.operator.processing.dependent.AbstractEventSourceHolderDependentResource;
 import io.javaoperatorsdk.operator.processing.dependent.Matcher.Result;
-import io.javaoperatorsdk.operator.processing.dependent.kubernetes.processors.GenericResourceUpdatePreProcessor;
+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;
@@ -40,18 +40,17 @@ public abstract class KubernetesDependentResource<R extends HasMetadata, P exten
   private static final Logger log = LoggerFactory.getLogger(KubernetesDependentResource.class);
 
   protected KubernetesClient client;
-  private final ResourceUpdatePreProcessor<R> processor;
+  private final ResourceUpdaterMatcher<R> updaterMatcher;
   private final boolean garbageCollected = this instanceof GarbageCollected;
   private KubernetesDependentResourceConfig<R> kubernetesDependentResourceConfig;
 
-
   @SuppressWarnings("unchecked")
   public KubernetesDependentResource(Class<R> resourceType) {
     super(resourceType);
 
-    processor = this instanceof ResourceUpdatePreProcessor
-        ? (ResourceUpdatePreProcessor<R>) this
-        : GenericResourceUpdatePreProcessor.processorFor(resourceType);
+    updaterMatcher = this instanceof ResourceUpdaterMatcher
+        ? (ResourceUpdaterMatcher<R>) this
+        : GenericResourceUpdaterMatcher.updaterMatcherFor(resourceType);
   }
 
   @SuppressWarnings("unchecked")
@@ -129,57 +128,59 @@ protected R handleUpdate(R actual, R desired, P primary, Context<P> context) {
 
   @SuppressWarnings("unused")
   public R create(R target, P primary, Context<P> context) {
-    if (!context.getControllerConfiguration().getConfigurationService()
-        .ssaBasedCreateUpdateForDependentResources()) {
-      return prepare(target, primary, "Creating").create();
-    } else {
-      return prepare(target, primary, "Creating")
-          .fieldManager(context.getControllerConfiguration().fieldManager())
-          .forceConflicts()
-          .serverSideApply();
-    }
+    final var resource = prepare(target, primary, "Creating");
+    return useSSA(context)
+        ? resource
+            .fieldManager(context.getControllerConfiguration().fieldManager())
+            .forceConflicts()
+            .serverSideApply()
+        : resource.create();
   }
 
   public R update(R actual, R target, P primary, Context<P> context) {
-    if (!context.getControllerConfiguration().getConfigurationService()
-        .ssaBasedCreateUpdateForDependentResources()) {
-      var updatedActual = processor.replaceSpecOnActual(actual, target, context);
-      return prepare(updatedActual, primary, "Updating").replace();
-    } else {
+    if (useSSA(context)) {
       target.getMetadata().setResourceVersion(actual.getMetadata().getResourceVersion());
       return prepare(target, primary, "Updating")
           .fieldManager(context.getControllerConfiguration().fieldManager())
           .forceConflicts().serverSideApply();
+    } else {
+      var updatedActual = updaterMatcher.updateResource(actual, target, context);
+      return prepare(updatedActual, primary, "Updating").replace();
     }
   }
 
   public Result<R> match(R actualResource, P primary, Context<P> context) {
-    if (!context.getControllerConfiguration().getConfigurationService()
-        .ssaBasedDefaultMatchingForDependentResources()) {
-      return GenericKubernetesResourceMatcher.match(this, actualResource, primary, context, false);
-    } else {
-      final var desired = desired(primary, context);
+    final var desired = desired(primary, context);
+    final boolean matches;
+    if (useSSA(context)) {
       addReferenceHandlingMetadata(desired, primary);
-      var matches = SSABasedGenericKubernetesResourceMatcher.getInstance().matches(actualResource,
-          desired, context);
-      return Result.computed(matches, desired);
+      matches = SSABasedGenericKubernetesResourceMatcher.getInstance()
+          .matches(actualResource, desired, context);
+    } else {
+      matches = updaterMatcher.matches(actualResource, desired, context);
     }
+    return Result.computed(matches, desired);
   }
 
   @SuppressWarnings("unused")
   public Result<R> match(R actualResource, R desired, P primary, Context<P> context) {
-    if (!context.getControllerConfiguration().getConfigurationService()
-        .ssaBasedDefaultMatchingForDependentResources()) {
-      return GenericKubernetesResourceMatcher.match(desired, actualResource, false,
-          false, false, context);
-    } else {
+    if (useSSA(context)) {
       addReferenceHandlingMetadata(desired, primary);
-      var matches = SSABasedGenericKubernetesResourceMatcher.getInstance().matches(actualResource,
-          desired, context);
+      var matches = SSABasedGenericKubernetesResourceMatcher.getInstance()
+          .matches(actualResource, desired, context);
       return Result.computed(matches, desired);
+    } else {
+      return GenericKubernetesResourceMatcher
+          .match(desired, actualResource, true,
+              false, false, context);
     }
   }
 
+  private boolean useSSA(Context<P> context) {
+    return context.getControllerConfiguration().getConfigurationService()
+        .ssaBasedCreateUpdateMatchForDependentResources();
+  }
+
   protected void handleDelete(P primary, R secondary, Context<P> context) {
     if (secondary != null) {
       client.resource(secondary).delete();