fix: infinite resource updates due to canonical format conversion of resource requirements (#2565)

* refactor: clean up SSABasedGenericKubernetesResourceMatcher

Signed-off-by: David Sondermann <[email protected]>

* test: add missing tests for StatefulSet with VolumeClaimTemplates for SSABasedGenericKubernetesResourceMatcher

Signed-off-by: David Sondermann <[email protected]>

* fix: Fix infinite resource updates due to canonical format conversion of resource requirements

Signed-off-by: David Sondermann <[email protected]>

* test: Add test cases with init containers to ResourceRequirementsSanitizerTest

Signed-off-by: David Sondermann <[email protected]>

---------

Signed-off-by: David Sondermann <[email protected]>
Signed-off-by: Chris Laprun <[email protected]>

Author: Donnerbart

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

@@ -12,10 +12,6 @@
 import java.util.Optional;
 import java.util.Set;
 import java.util.TreeMap;
-import java.util.Optional;
-import java.util.Set;
-import java.util.SortedMap;
-import java.util.TreeMap;
 
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -32,11 +28,11 @@
 import io.javaoperatorsdk.operator.api.reconciler.Context;
 import io.javaoperatorsdk.operator.processing.LoggingUtils;
 
-import static io.javaoperatorsdk.operator.processing.dependent.kubernetes.ResourceRequirementsSanitizer.sanitizeResourceRequirements;
-
 import com.github.difflib.DiffUtils;
 import com.github.difflib.UnifiedDiffUtils;
 
+import static io.javaoperatorsdk.operator.processing.dependent.kubernetes.ResourceRequirementsSanitizer.sanitizeResourceRequirements;
+
 /**
  * Matches the actual state on the server vs the desired state. Based on the managedFields of SSA.
  * <p>
@@ -112,15 +108,13 @@ public boolean matches(R actual, R desired, Context<?> context) {
     removeIrrelevantValues(desiredMap);
 
     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{}",
+          "Diff between actual and desired state for resource: {} with name: {} in namespace: {} is:\n{}",
           actual.getKind(), actual.getMetadata().getName(), actual.getMetadata().getNamespace(),
           diff);
     }
-
     return matches;
   }
 
@@ -129,24 +123,23 @@ private String getDiff(Map<String, Object> prunedActualMap, Map<String, Object>
     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);
+      log.trace("Pruned actual resource:\n {} \ndesired resource:\n {} ", actualYaml, desiredYaml);
     }
 
     var patch = DiffUtils.diff(actualYaml.lines().toList(), desiredYaml.lines().toList());
-    List<String> unifiedDiff =
+    var 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());
+    var sortedKeys = new ArrayList<>(map.keySet());
     Collections.sort(sortedKeys);
 
-    Map<String, Object> sortedMap = new LinkedHashMap<>();
-    for (String key : sortedKeys) {
-      Object value = map.get(key);
+    var sortedMap = new LinkedHashMap<String, Object>();
+    for (var key : sortedKeys) {
+      var value = map.get(key);
       if (value instanceof Map) {
         sortedMap.put(key, sortMap((Map<String, Object>) value));
       } else if (value instanceof List) {
@@ -160,8 +153,8 @@ Map<String, Object> sortMap(Map<String, Object> map) {
 
   @SuppressWarnings("unchecked")
   List<Object> sortListItems(List<Object> list) {
-    List<Object> sortedList = new ArrayList<>();
-    for (Object item : list) {
+    var sortedList = new ArrayList<>();
+    for (var item : list) {
       if (item instanceof Map) {
         sortedList.add(sortMap((Map<String, Object>) item));
       } else if (item instanceof List) {
@@ -177,9 +170,10 @@ List<Object> sortListItems(List<Object> list) {
    * Correct for known issue with SSA
    */
   private void sanitizeState(R actual, R desired, Map<String, Object> actualMap) {
-    if (actual instanceof StatefulSet) {
-      var actualSpec = (((StatefulSet) actual)).getSpec();
-      var desiredSpec = (((StatefulSet) desired)).getSpec();
+    if (actual instanceof StatefulSet actualStatefulSet
+        && desired instanceof StatefulSet desiredStatefulSet) {
+      var actualSpec = actualStatefulSet.getSpec();
+      var desiredSpec = desiredStatefulSet.getSpec();
       int claims = desiredSpec.getVolumeClaimTemplates().size();
       if (claims == actualSpec.getVolumeClaimTemplates().size()) {
         for (int i = 0; i < claims; i++) {
@@ -197,18 +191,21 @@ private void sanitizeState(R actual, R desired, Map<String, Object> actualMap) {
         }
       }
       sanitizeResourceRequirements(actualMap, actualSpec.getTemplate(), desiredSpec.getTemplate());
-    } else if (actual instanceof Deployment) {
+    } else if (actual instanceof Deployment actualDeployment
+        && desired instanceof Deployment desiredDeployment) {
       sanitizeResourceRequirements(actualMap,
-          ((Deployment) actual).getSpec().getTemplate(),
-          ((Deployment) desired).getSpec().getTemplate());
-    } else if (actual instanceof ReplicaSet) {
+          actualDeployment.getSpec().getTemplate(),
+          desiredDeployment.getSpec().getTemplate());
+    } else if (actual instanceof ReplicaSet actualReplicaSet
+        && desired instanceof ReplicaSet desiredReplicaSet) {
       sanitizeResourceRequirements(actualMap,
-          ((ReplicaSet) actual).getSpec().getTemplate(),
-          ((ReplicaSet) desired).getSpec().getTemplate());
-    } else if (actual instanceof DaemonSet) {
+          actualReplicaSet.getSpec().getTemplate(),
+          desiredReplicaSet.getSpec().getTemplate());
+    } else if (actual instanceof DaemonSet actualDaemonSet
+        && desired instanceof DaemonSet desiredDaemonSet) {
       sanitizeResourceRequirements(actualMap,
-          ((DaemonSet) actual).getSpec().getTemplate(),
-          ((DaemonSet) desired).getSpec().getTemplate());
+          actualDaemonSet.getSpec().getTemplate(),
+          desiredDaemonSet.getSpec().getTemplate());
     }
   }
 
@@ -393,7 +390,7 @@ private static Map.Entry<Integer, Map<String, Object>> selectListEntryBasedOnKey
       }
     }
     if (possibleTargets.isEmpty()) {
-      throw new IllegalStateException("Cannot find list element for key: " + key + ", in map: "
+      throw new IllegalStateException("Cannot find list element for key: " + key + " in map: "
           + values.stream().map(Map::keySet).toList());
     }
     if (possibleTargets.size() > 1) {

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

@@ -1,6 +1,5 @@
 package io.javaoperatorsdk.operator.processing.dependent.kubernetes;
 
-import java.util.Arrays;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -123,6 +122,48 @@ void addedLabelInDesiredMakesMatchFail() {
     assertThat(matcher.matches(actualConfigMap, desiredConfigMap, mockedContext)).isFalse();
   }
 
+  @Test
+  @SuppressWarnings("unchecked")
+  void sortListItemsTest() {
+    var nestedMap1 = new HashMap<String, Object>();
+    nestedMap1.put("z", 26);
+    nestedMap1.put("y", 25);
+
+    var nestedMap2 = new HashMap<String, Object>();
+    nestedMap2.put("b", 26);
+    nestedMap2.put("c", 25);
+    nestedMap2.put("a", 24);
+
+    var unsortedListItems = List.<Object>of(1, nestedMap1, nestedMap2);
+    var sortedListItems = matcher.sortListItems(unsortedListItems);
+    assertThat(sortedListItems).element(0).isEqualTo(1);
+
+    var sortedNestedMap1 = (Map<String, Object>) sortedListItems.get(1);
+    assertThat(sortedNestedMap1.keySet()).containsExactly("y", "z");
+
+    var sortedNestedMap2 = (Map<String, Object>) sortedListItems.get(2);
+    assertThat(sortedNestedMap2.keySet()).containsExactly("a", "b", "c");
+  }
+
+  @Test
+  @SuppressWarnings("unchecked")
+  void testSortMapWithNestedMap() {
+    var nestedMap = new HashMap<String, Object>();
+    nestedMap.put("z", 26);
+    nestedMap.put("y", 25);
+
+    var unsortedMap = new HashMap<String, Object>();
+    unsortedMap.put("b", nestedMap);
+    unsortedMap.put("a", 1);
+    unsortedMap.put("c", 2);
+
+    var sortedMap = matcher.sortMap(unsortedMap);
+    assertThat(sortedMap.keySet()).containsExactly("a", "b", "c");
+
+    var sortedNestedMap = (Map<String, Object>) sortedMap.get("b");
+    assertThat(sortedNestedMap.keySet()).containsExactly("y", "z");
+  }
+
   @ParameterizedTest
   @ValueSource(strings = {"sample-sts-volumeclaimtemplates-desired.yaml",
       "sample-sts-volumeclaimtemplates-desired-with-status.yaml",
@@ -209,48 +250,4 @@ 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");
-  }
 }