feat: improve diff logging (#2544)

* imporve diff logging

Signed-off-by: bachmanity1 <[email protected]>

* compute diff only when actual doesn't match desired

Signed-off-by: bachmanity1 <[email protected]>

* slight improvements

Signed-off-by: bachmanity1 <[email protected]>

* increase context size

Signed-off-by: bachmanity1 <[email protected]>

* fix style

Signed-off-by: bachmanity1 <[email protected]>

* calculate diff only if debug is enabled

Signed-off-by: bachmanity1 <[email protected]>

* print actual resources when trace is enabled

Signed-off-by: bachmanity1 <[email protected]>

* use java-diff-utils

Signed-off-by: bachmanity1 <[email protected]>

* add unit tests

---------

Signed-off-by: bachmanity1 <[email protected]>

Author: bachmanity1

Diff for: operator-framework-core/pom.xml

@@ -14,6 +14,10 @@
   <description>Core framework for implementing Kubernetes operators</description>
 
   <dependencies>
+    <dependency>
+      <groupId>io.github.java-diff-utils</groupId>
+      <artifactId>java-diff-utils</artifactId>
+    </dependency>
     <dependency>
       <groupId>io.fabric8</groupId>
       <artifactId>kubernetes-client</artifactId>

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

@@ -4,6 +4,7 @@
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.HashMap;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
@@ -33,6 +34,9 @@
 
 import static io.javaoperatorsdk.operator.processing.dependent.kubernetes.ResourceRequirementsSanitizer.sanitizeResourceRequirements;
 
+import com.github.difflib.DiffUtils;
+import com.github.difflib.UnifiedDiffUtils;
+
 /**
  * Matches the actual state on the server vs the desired state. Based on the managedFields of SSA.
  * <p>
@@ -107,20 +111,66 @@ public boolean matches(R actual, R desired, Context<?> context) {
 
     removeIrrelevantValues(desiredMap);
 
-    if (LoggingUtils.isNotSensitiveResource(desired)) {
-      logDiff(prunedActual, desiredMap, objectMapper);
+    var matches = prunedActual.equals(desiredMap);
+
+    if (!matches && log.isDebugEnabled() && LoggingUtils.isNotSensitiveResource(desired)) {
+      var diff = getDiff(prunedActual, desiredMap, objectMapper);
+      log.debug(
+          "Diff between actual and desired state for resource: {} with name: {} in namespace: {} is: \n{}",
+          actual.getKind(), actual.getMetadata().getName(), actual.getMetadata().getNamespace(),
+          diff);
     }
 
-    return prunedActual.equals(desiredMap);
+    return matches;
   }
 
-  private void logDiff(Map<String, Object> prunedActualMap, Map<String, Object> desiredMap,
+  private String getDiff(Map<String, Object> prunedActualMap, Map<String, Object> desiredMap,
       KubernetesSerialization serialization) {
-    if (log.isDebugEnabled()) {
-      var actualYaml = serialization.asYaml(prunedActualMap);
-      var desiredYaml = serialization.asYaml(desiredMap);
-      log.debug("Pruned actual yaml: \n {} \n desired yaml: \n {} ", actualYaml, desiredYaml);
+    var actualYaml = serialization.asYaml(sortMap(prunedActualMap));
+    var desiredYaml = serialization.asYaml(sortMap(desiredMap));
+    if (log.isTraceEnabled()) {
+      log.trace("Pruned actual resource: \n {} \ndesired resource: \n {} ", actualYaml,
+          desiredYaml);
+    }
+
+    var patch = DiffUtils.diff(actualYaml.lines().toList(), desiredYaml.lines().toList());
+    List<String> unifiedDiff =
+        UnifiedDiffUtils.generateUnifiedDiff("", "", actualYaml.lines().toList(), patch, 1);
+    return String.join("\n", unifiedDiff);
+  }
+
+  @SuppressWarnings("unchecked")
+  Map<String, Object> sortMap(Map<String, Object> map) {
+    List<String> sortedKeys = new ArrayList<>(map.keySet());
+    Collections.sort(sortedKeys);
+
+    Map<String, Object> sortedMap = new LinkedHashMap<>();
+    for (String key : sortedKeys) {
+      Object value = map.get(key);
+      if (value instanceof Map) {
+        sortedMap.put(key, sortMap((Map<String, Object>) value));
+      } else if (value instanceof List) {
+        sortedMap.put(key, sortListItems((List<Object>) value));
+      } else {
+        sortedMap.put(key, value);
+      }
+    }
+    return sortedMap;
+  }
+
+  @SuppressWarnings("unchecked")
+  List<Object> sortListItems(List<Object> list) {
+    List<Object> sortedList = new ArrayList<>();
+    for (Object item : list) {
+      if (item instanceof Map) {
+        sortedList.add(sortMap((Map<String, Object>) item));
+      } else if (item instanceof List) {
+        sortedList.add(sortListItems((List<Object>) item));
+      } else {
+        sortedList.add(item);
+      }
     }
+    return sortedList;
   }
 
   /**

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

@@ -1,5 +1,8 @@
 package io.javaoperatorsdk.operator.processing.dependent.kubernetes;
 
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
 
 import org.junit.jupiter.api.BeforeEach;
@@ -206,4 +209,48 @@ private static <R> R loadResource(String fileName, Class<R> clazz) {
     return ReconcilerUtils.loadYaml(clazz, SSABasedGenericKubernetesResourceMatcherTest.class,
         fileName);
   }
+
+  @Test
+  @SuppressWarnings("unchecked")
+  void sortListItemsTest() {
+    Map<String, Object> nestedMap1 = new HashMap<>();
+    nestedMap1.put("z", 26);
+    nestedMap1.put("y", 25);
+
+    Map<String, Object> nestedMap2 = new HashMap<>();
+    nestedMap2.put("b", 26);
+    nestedMap2.put("c", 25);
+    nestedMap2.put("a", 24);
+
+    List<Object> unsortedListItems = Arrays.asList(1, nestedMap1, nestedMap2);
+    List<Object> sortedListItems = matcher.sortListItems(unsortedListItems);
+
+    assertThat(sortedListItems).element(0).isEqualTo(1);
+
+    Map<String, Object> sortedNestedMap1 = (Map<String, Object>) sortedListItems.get(1);
+    assertThat(sortedNestedMap1.keySet()).containsExactly("y", "z");
+
+    Map<String, Object> sortedNestedMap2 = (Map<String, Object>) sortedListItems.get(2);
+    assertThat(sortedNestedMap2.keySet()).containsExactly("a", "b", "c");
+  }
+
+  @Test
+  @SuppressWarnings("unchecked")
+  void testSortMapWithNestedMap() {
+    Map<String, Object> nestedMap = new HashMap<>();
+    nestedMap.put("z", 26);
+    nestedMap.put("y", 25);
+
+    Map<String, Object> unsortedMap = new HashMap<>();
+    unsortedMap.put("b", nestedMap);
+    unsortedMap.put("a", 1);
+    unsortedMap.put("c", 2);
+
+    Map<String, Object> sortedMap = matcher.sortMap(unsortedMap);
+
+    assertThat(sortedMap.keySet()).containsExactly("a", "b", "c");
+
+    Map<String, Object> sortedNestedMap = (Map<String, Object>) sortedMap.get("b");
+    assertThat(sortedNestedMap.keySet()).containsExactly("y", "z");
+  }
 }

Diff for: pom.xml

@@ -75,6 +75,7 @@
     <caffeine.version>3.1.8</caffeine.version>
     <mustache.version>0.9.11</mustache.version>
     <commons.io.version>2.16.1</commons.io.version>
+    <java.diff.version>4.12</java.diff.version>
 
     <fmt-maven-plugin.version>2.11</fmt-maven-plugin.version>
     <maven-compiler-plugin.version>3.12.1</maven-compiler-plugin.version>
@@ -161,6 +162,11 @@
         <version>${mokito.version}</version>
       </dependency>
 
+      <dependency>
+        <groupId>io.github.java-diff-utils</groupId>
+        <artifactId>java-diff-utils</artifactId>
+        <version>${java.diff.version}</version>
+      </dependency>
       <dependency>
         <groupId>org.slf4j</groupId>
         <artifactId>slf4j-api</artifactId>