generalization of the update matcher (#2014)

* feat: create resource only if not exists (#2001)

* feat: leader election callbacks (#2015)

* discriminator improvements (#2013)

* generalization of the update matcher

* consolidating the diffs

also adding a test that removes a field not present in the desired

---------

Co-authored-by: Attila Mészáros <[email protected]>
Signed-off-by: csviri <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>

Author: shawkins

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

@@ -1,11 +1,8 @@
 package io.javaoperatorsdk.operator.processing.dependent.kubernetes;
 
-import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
-import java.util.Objects;
-import java.util.Optional;
 
 import io.fabric8.kubernetes.api.model.ConfigMap;
 import io.fabric8.kubernetes.api.model.HasMetadata;
@@ -20,8 +17,10 @@ public class GenericKubernetesResourceMatcher<R extends HasMetadata, P extends H
     implements Matcher<R, P> {
 
   private static final String SPEC = "/spec";
+  private static final String METADATA = "/metadata";
   private static final String ADD = "add";
   private static final String OP = "op";
+  private static final List<String> IGNORED_FIELDS = List.of("/apiVersion", "/kind", "/status");
   public static final String METADATA_LABELS = "/metadata/labels";
   public static final String METADATA_ANNOTATIONS = "/metadata/annotations";
 
@@ -184,76 +183,39 @@ public static <R extends HasMetadata, P extends HasMetadata> Result<R> match(R d
           "Equality should be false in case of ignore list provided");
     }
 
-    if (considerMetadata) {
-      Optional<Result<R>> res =
-          matchMetadata(desired, actualResource, labelsAndAnnotationsEquality, context);
-      if (res.isPresent()) {
-        return res.orElseThrow();
-      }
-    }
-
-    final var matched = matchSpec(actualResource, desired, specEquality, context, ignoreList);
-    return Result.computed(matched, desired);
-  }
-
-  private static <R extends HasMetadata> boolean matchSpec(R actual, R desired, boolean equality,
-      Context<?> context, List<String> ignoreList) {
     final var kubernetesSerialization = context.getClient().getKubernetesSerialization();
     var desiredNode = kubernetesSerialization.convertValue(desired, JsonNode.class);
-    var actualNode = kubernetesSerialization.convertValue(actual, JsonNode.class);
+    var actualNode = kubernetesSerialization.convertValue(actualResource, JsonNode.class);
     var wholeDiffJsonPatch = JsonDiff.asJson(desiredNode, actualNode);
 
-    // 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()) {
-      return allDiffsOnIgnoreList(specDiffJsonPatch, ignoreList);
-    } else {
-      return allDiffsAreAddOps(specDiffJsonPatch);
+    boolean matched = true;
+    for (int i = 0; i < wholeDiffJsonPatch.size() && matched; i++) {
+      var node = wholeDiffJsonPatch.get(i);
+      if (nodeIsChildOf(node, List.of(SPEC))) {
+        matched = match(specEquality, node, ignoreList);
+      } else if (nodeIsChildOf(node, List.of(METADATA))) {
+        // conditionally consider labels and annotations
+        if (considerMetadata
+            && nodeIsChildOf(node, List.of(METADATA_LABELS, METADATA_ANNOTATIONS))) {
+          matched = match(labelsAndAnnotationsEquality, node, Collections.emptyList());
+        }
+      } else if (!nodeIsChildOf(node, IGNORED_FIELDS)) {
+        matched = match(true, node, ignoreList);
+      }
     }
+
+    return Result.computed(matched, desired);
   }
 
-  private static boolean allDiffsOnIgnoreList(List<JsonNode> metadataJSonDiffs,
-      List<String> ignoreList) {
-    if (metadataJSonDiffs.isEmpty()) {
+  private static boolean match(boolean equality, JsonNode diff,
+      final List<String> ignoreList) {
+    if (equality) {
       return false;
     }
-    return metadataJSonDiffs.stream().allMatch(n -> nodeIsChildOf(n, ignoreList));
-  }
-
-  private static <R extends HasMetadata, P extends HasMetadata> Optional<Result<R>> matchMetadata(
-      R desired,
-      R actualResource,
-      boolean labelsAndAnnotationsEquality, Context<P> context) {
-    if (labelsAndAnnotationsEquality) {
-      final var desiredMetadata = desired.getMetadata();
-      final var actualMetadata = actualResource.getMetadata();
-
-      final var matched =
-          Objects.equals(desiredMetadata.getAnnotations(), actualMetadata.getAnnotations()) &&
-              Objects.equals(desiredMetadata.getLabels(), actualMetadata.getLabels());
-      if (!matched) {
-        return Optional.of(Result.computed(false, desired));
-      }
-    } else {
-      final var objectMapper = context.getClient().getKubernetesSerialization();
-      var desiredNode = objectMapper.convertValue(desired, JsonNode.class);
-      var actualNode = objectMapper.convertValue(actualResource, JsonNode.class);
-      var wholeDiffJsonPatch = JsonDiff.asJson(desiredNode, actualNode);
-      var metadataJSonDiffs = getDiffsImpactingPathsWithPrefixes(wholeDiffJsonPatch,
-          METADATA_LABELS,
-          METADATA_ANNOTATIONS);
-      if (!allDiffsAreAddOps(metadataJSonDiffs)) {
-        return Optional.of(Result.computed(false, desired));
-      }
+    if (!ignoreList.isEmpty()) {
+      return nodeIsChildOf(diff, ignoreList);
     }
-    return Optional.empty();
+    return ADD.equals(diff.get(OP).asText());
   }
 
   static boolean nodeIsChildOf(JsonNode n, List<String> prefixes) {
@@ -265,29 +227,6 @@ static String getPath(JsonNode n) {
     return n.get(PATH).asText();
   }
 
-  static boolean allDiffsAreAddOps(List<JsonNode> metadataJSonDiffs) {
-    if (metadataJSonDiffs.isEmpty()) {
-      return true;
-    }
-    return metadataJSonDiffs.stream().allMatch(n -> ADD.equals(n.get(OP).asText()));
-  }
-
-  public static List<JsonNode> getDiffsImpactingPathsWithPrefixes(JsonNode diffJsonPatch,
-      String... prefixes) {
-    if (prefixes != null && prefixes.length > 0) {
-      var res = new ArrayList<JsonNode>();
-      var prefixList = Arrays.asList(prefixes);
-      for (int i = 0; i < diffJsonPatch.size(); i++) {
-        var node = diffJsonPatch.get(i);
-        if (nodeIsChildOf(node, prefixList)) {
-          res.add(node);
-        }
-      }
-      return res;
-    }
-    return Collections.emptyList();
-  }
-
   @Deprecated(forRemoval = true)
   public static <R extends HasMetadata, P extends HasMetadata> Result<R> match(
       KubernetesDependentResource<R, P> dependentResource, R actualResource, P primary,

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

@@ -2,48 +2,40 @@
 
 import java.util.Map;
 
-import io.fabric8.kubernetes.api.model.*;
-import io.fabric8.kubernetes.api.model.discovery.v1.EndpointSlice;
-import io.fabric8.kubernetes.api.model.rbac.ClusterRole;
-import io.fabric8.kubernetes.api.model.rbac.ClusterRoleBinding;
-import io.fabric8.kubernetes.api.model.rbac.Role;
-import io.fabric8.kubernetes.api.model.rbac.RoleBinding;
-import io.javaoperatorsdk.operator.ReconcilerUtils;
+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> {
 
+  private static final String METADATA = "metadata";
   private static final ResourceUpdaterMatcher<?> INSTANCE = new GenericResourceUpdaterMatcher<>();
 
-  @SuppressWarnings("rawtypes")
-  private static final Map<Class, ResourceUpdaterMatcher> processors = Map.of(
-      Secret.class, new SecretResourceUpdaterMatcher(),
-      ConfigMap.class, new ConfigMapResourceUpdaterMatcher(),
-      ServiceAccount.class, new ServiceAccountResourceUpdaterMatcher(),
-      Role.class, new RoleResourceUpdaterMatcher(),
-      ClusterRole.class, new ClusterRoleResourceUpdaterMatcher(),
-      RoleBinding.class, new RoleBindingResourceUpdaterMatcher(),
-      ClusterRoleBinding.class, new ClusterRoleBindingResourceUpdaterMatcher(),
-      Endpoints.class, new EndpointsResourceUpdaterMatcher(),
-      EndpointSlice.class, new EndpointSliceResourceUpdateMatcher());
-
   protected GenericResourceUpdaterMatcher() {}
 
   @SuppressWarnings("unchecked")
   public static <R extends HasMetadata> ResourceUpdaterMatcher<R> updaterMatcherFor(
       Class<R> resourceType) {
-    final var processor = processors.get(resourceType);
-    return processor != null ? processor : (ResourceUpdaterMatcher<R>) INSTANCE;
+    return (ResourceUpdaterMatcher<R>) INSTANCE;
   }
 
+  @SuppressWarnings("unchecked")
+  @Override
   public R updateResource(R actual, R desired, Context<?> context) {
-    var clonedActual = context.getControllerConfiguration().getConfigurationService()
-        .getResourceCloner().clone(actual);
+    KubernetesSerialization kubernetesSerialization =
+        context.getClient().getKubernetesSerialization();
+    Map<String, Object> actualMap = kubernetesSerialization.convertValue(actual, Map.class);
+    Map<String, Object> desiredMap = kubernetesSerialization.convertValue(desired, Map.class);
+    // replace all top level fields from actual with desired, but merge metadata separately
+    var metadata = actualMap.remove(METADATA);
+    actualMap.replaceAll((k, v) -> desiredMap.get(k));
+    actualMap.putAll(desiredMap);
+    actualMap.put(METADATA, metadata);
+    var clonedActual = (R) kubernetesSerialization.convertValue(actualMap, desired.getClass());
     updateLabelsAndAnnotation(clonedActual, desired);
-    updateClonedActual(clonedActual, desired);
     return clonedActual;
   }
 
@@ -53,15 +45,6 @@ public boolean matches(R actual, R desired, Context<?> context) {
         false, false, context).matched();
   }
 
-  protected void updateClonedActual(R actual, R desired) {
-    updateSpec(actual, desired);
-  }
-
-  public static <K extends HasMetadata> void updateSpec(K actual, K desired) {
-    var desiredSpec = ReconcilerUtils.getSpec(desired);
-    ReconcilerUtils.setSpec(actual, desiredSpec);
-  }
-
   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());