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]>
operator-framework · Oct 10, 2024 · 68832a0 · 68832a0
1 parent 0be0f42
commit 68832a0
Show file tree

Hide file tree

Showing 4 changed files with 114 additions and 8 deletions.
diff --git a/operator-framework-core/pom.xml b/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 --git a/...dk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java b/...dk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java
@@ -5,6 +5,7 @@
 import java.util.Arrays;
 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;
@@ -25,6 +26,9 @@
 import io.javaoperatorsdk.operator.api.reconciler.Context;
 import io.javaoperatorsdk.operator.processing.LoggingUtils;
 
+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.
  *
@@ -103,20 +107,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 --git a/...perator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcherTest.java b/...perator/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;
@@ -121,4 +124,47 @@ private <R> R loadResource(String fileName, Class<R> clazz) {
         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 --git a/pom.xml b/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>