consolidating the diffs

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

Author: shawkins

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

@@ -4,8 +4,6 @@
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
-import java.util.Objects;
-import java.util.Optional;
 import java.util.Set;
 
 import io.fabric8.kubernetes.api.model.ConfigMap;
@@ -188,33 +186,18 @@ 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 = match(actualResource, desired, specEquality, context, ignoredPaths);
-    return Result.computed(matched, desired);
-  }
-
-  private static <R extends HasMetadata> boolean match(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 actualNode = kubernetesSerialization.convertValue(actualResource, JsonNode.class);
     var wholeDiffJsonPatch = JsonDiff.asJson(desiredNode, actualNode);
 
-    final List<String> ignoreList =
-        ignoredPaths != null && ignoredPaths.length > 0 ? Arrays.asList(ignoredPaths)
-            : Collections.emptyList();
-    boolean specMatch = match(equality, wholeDiffJsonPatch, ignoreList, SPEC);
+    boolean specMatch = match(specEquality, wholeDiffJsonPatch, ignoreList, SPEC);
     if (!specMatch) {
-      return false;
+      return Result.computed(false, desired);
+    }
+    if (considerMetadata && !match(labelsAndAnnotationsEquality, wholeDiffJsonPatch, ignoreList,
+        METADATA_LABELS, METADATA_ANNOTATIONS)) {
+      return Result.computed(false, desired);
     }
     // expect everything else to be equal
     var names = desiredNode.fieldNames();
@@ -225,7 +208,8 @@ private static <R extends HasMetadata> boolean match(R actual, R desired, boolea
         prefixes.add(prefix);
       }
     }
-    return match(true, wholeDiffJsonPatch, ignoreList, prefixes.toArray(String[]::new));
+    boolean matched = match(true, wholeDiffJsonPatch, ignoreList, prefixes.toArray(String[]::new));
+    return Result.computed(matched, desired);
   }
 
   private static boolean match(boolean equality, JsonNode wholeDiffJsonPatch,
@@ -241,49 +225,11 @@ private static boolean match(boolean equality, JsonNode wholeDiffJsonPatch,
       return false;
     }
     if (!ignoreList.isEmpty()) {
-      return allDiffsOnIgnoreList(diffJsonPatch, ignoreList);
+      return diffJsonPatch.stream().allMatch(n -> nodeIsChildOf(n, ignoreList));
     }
     return allDiffsAreAddOps(diffJsonPatch);
   }
 
-  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,
-      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));
-      }
-    }
-    return Optional.empty();
-  }
-
   static boolean nodeIsChildOf(JsonNode n, List<String> prefixes) {
     var path = getPath(n);
     return prefixes.stream().anyMatch(path::startsWith);

operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericResourceUpdaterMatcherTest.java

@@ -2,6 +2,7 @@
 
 import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
 
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.Test;
@@ -99,6 +100,23 @@ void checkSecret() {
     assertThat(secret.getData()).containsOnlyKeys("foo");
   }
 
+  @Test
+  void checkSeviceAccount() {
+    var processor = GenericResourceUpdaterMatcher.updaterMatcherFor(ServiceAccount.class);
+    var desired = new ServiceAccountBuilder()
+        .withMetadata(new ObjectMetaBuilder().addToLabels("new", "label").build())
+        .build();
+    var actual = new ServiceAccountBuilder()
+        .withMetadata(new ObjectMetaBuilder().addToLabels("a", "label").build())
+        .withImagePullSecrets(new LocalObjectReferenceBuilder().withName("secret").build())
+        .build();
+
+    final var serviceAccount = processor.updateResource(actual, desired, context);
+    assertThat(serviceAccount.getMetadata().getLabels())
+        .isEqualTo(Map.of("a", "label", "new", "label"));
+    assertThat(serviceAccount.getImagePullSecrets()).isNullOrEmpty();
+  }
+
   Deployment createDeployment() {
     return ReconcilerUtils.loadYaml(
         Deployment.class, GenericResourceUpdaterMatcherTest.class, "nginx-deployment.yaml");