various refinements to ssa logic

- localizes sanitation to ssa matcher
- further ensures values won't be logged

Signed-off-by: Steve Hawkins <[email protected]>

Author: shawkins

Diff for: docs/documentation/dependent-resources.md

@@ -370,6 +370,8 @@ server. To bypass the matching feature completely, simply override the `match` m
 return `false`, thus telling JOSDK that the actual state never matches the desired one, making 
 it always update the resources using SSA.
 
+WARNING: Older versions of Kubernetes before 1.25 would create an additional resource version for every SSA update performed with certain resources - even though there were no actual changes in the stored resource. This behavior was seen with Secrets using stringData, Ingresses using empty string fields, and StatefulSets using volume claim templates.  The operator framework has added built-in handling for the StatefulSet issue.  If you encounter this issue, consider changing your desired state, turning off SSA for that resource, or even upgrading your Kubernetes version.
+
 ## Telling JOSDK how to find which secondary resources are associated with a given primary resource
 
 [`KubernetesDependentResource`](https://github.com/java-operator-sdk/java-operator-sdk/blob/main/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java)

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

@@ -114,7 +114,6 @@ public R create(R desired, P primary, Context<P> context) {
       }
     }
     addMetadata(false, null, desired, primary);
-    sanitizeDesired(desired, null, primary, context);
     final var resource = prepare(desired, primary, "Creating");
     return useSSA(context)
         ? resource
@@ -131,7 +130,6 @@ public R update(R actual, R desired, P primary, Context<P> context) {
     }
     R updatedResource;
     addMetadata(false, actual, desired, primary);
-    sanitizeDesired(desired, actual, primary, context);
     if (useSSA(context)) {
       updatedResource = prepare(desired, primary, "Updating")
           .fieldManager(context.getControllerConfiguration().fieldManager())
@@ -148,7 +146,6 @@ public R update(R actual, R desired, P primary, Context<P> context) {
   @Override
   public Result<R> match(R actualResource, P primary, Context<P> context) {
     final var desired = desired(primary, context);
-    sanitizeDesired(desired, actualResource, primary, context);
     return match(actualResource, desired, primary, updaterMatcher, context);
   }
 
@@ -192,11 +189,10 @@ protected void addMetadata(boolean forMatch, R actualResource, final R target, P
     addReferenceHandlingMetadata(target, primary);
   }
 
-  protected void sanitizeDesired(R desired, R actual, P primary, Context<P> context) {
-    DesiredResourceSanitizer.sanitizeDesired(desired, actual, primary, context, useSSA(context));
-  }
-
   protected boolean useSSA(Context<P> context) {
+    if (this instanceof ResourceUpdaterMatcher) {
+      return false;
+    }
     Optional<Boolean> useSSAConfig =
         configuration().flatMap(KubernetesDependentResourceConfig::useSSA);
     var configService = context.getControllerConfiguration().getConfigurationService();

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

@@ -7,8 +7,10 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import io.fabric8.kubernetes.api.model.GenericKubernetesResource;
 import io.fabric8.kubernetes.api.model.HasMetadata;
 import io.fabric8.kubernetes.api.model.ManagedFieldsEntry;
+import io.fabric8.kubernetes.api.model.apps.StatefulSet;
 import io.fabric8.kubernetes.client.utils.KubernetesSerialization;
 import io.javaoperatorsdk.operator.OperatorException;
 import io.javaoperatorsdk.operator.api.reconciler.Context;
@@ -74,6 +76,9 @@ public boolean matches(R actual, R desired, Context<?> context) {
     var objectMapper = context.getClient().getKubernetesSerialization();
 
     var actualMap = objectMapper.convertValue(actual, Map.class);
+
+    sanitizeState(actual, desired, actualMap);
+
     var desiredMap = objectMapper.convertValue(desired, Map.class);
     if (LoggingUtils.isNotSensitiveResource(desired)) {
       log.trace("Original actual: \n {} \n original desired: \n {} ", actual, desiredMap);
@@ -92,6 +97,35 @@ public boolean matches(R actual, R desired, Context<?> context) {
     return prunedActual.equals(desiredMap);
   }
 
+  /**
+   * Correct for known issue with SSA
+   */
+  @SuppressWarnings("unchecked")
+  private void sanitizeState(R actual, R desired, Map<String, Object> actualMap) {
+    if (desired instanceof StatefulSet) {
+      StatefulSet desiredStatefulSet = (StatefulSet) desired;
+      StatefulSet actualStatefulSet = (StatefulSet) actual;
+      int claims = desiredStatefulSet.getSpec().getVolumeClaimTemplates().size();
+      if (claims == actualStatefulSet.getSpec().getVolumeClaimTemplates().size()) {
+        for (int i = 0; i < claims; i++) {
+          if (desiredStatefulSet.getSpec().getVolumeClaimTemplates().get(i).getSpec()
+              .getVolumeMode() == null) {
+            Optional
+                .ofNullable(GenericKubernetesResource.get(actualMap, "spec", "volumeClaimTemplates",
+                    i, "spec"))
+                .map(Map.class::cast).ifPresent(m -> m.remove("volumeMode"));
+          }
+          if (desiredStatefulSet.getSpec().getVolumeClaimTemplates().get(i).getStatus() == null) {
+            Optional
+                .ofNullable(
+                    GenericKubernetesResource.get(actualMap, "spec", "volumeClaimTemplates", i))
+                .map(Map.class::cast).ifPresent(m -> m.remove("status"));
+          }
+        }
+      }
+    }
+  }
+
   @SuppressWarnings("unchecked")
   private static void removeIrrelevantValues(Map<String, Object> desiredMap) {
     var metadata = (Map<String, Object>) desiredMap.get(METADATA_KEY);
@@ -153,8 +187,7 @@ private static void fillResultsAndTraverseFurther(Map<String, Object> result,
     var emptyMapValue = new HashMap<String, Object>();
     result.put(keyInActual, emptyMapValue);
     var actualMapValue = actualMap.getOrDefault(keyInActual, Collections.emptyMap());
-    log.debug("key: {} actual map value: {} managedFieldValue: {}", keyInActual,
-        actualMapValue, managedFieldValue);
+    log.debug("key: {} actual map value: managedFieldValue: {}", keyInActual, managedFieldValue);
 
     keepOnlyManagedFields(emptyMapValue, (Map<String, Object>) actualMapValue,
         (Map<String, Object>) managedFields.get(key), objectMapper);
@@ -282,29 +315,16 @@ private static java.util.Map.Entry<Integer, Map<String, Object>> selectListEntry
     }
     if (possibleTargets.isEmpty()) {
       throw new IllegalStateException(
-          "Cannot find list element for key:" + key + ", in map: " + values);
+          "Cannot find list element for key:" + key + ", in map: "
+              + values.stream().map(Map::keySet).collect(Collectors.toList()));
     }
     if (possibleTargets.size() > 1) {
       throw new IllegalStateException(
-          "More targets found in list element for key:" + key + ", in map: " + values);
+          "More targets found in list element for key:" + key + ", in map: "
+              + values.stream().map(Map::keySet).collect(Collectors.toList()));
     }
     final var finalIndex = index;
-    return new Map.Entry<>() {
-      @Override
-      public Integer getKey() {
-        return finalIndex;
-      }
-
-      @Override
-      public Map<String, Object> getValue() {
-        return possibleTargets.get(0);
-      }
-
-      @Override
-      public Map<String, Object> setValue(Map<String, Object> stringObjectMap) {
-        throw new IllegalStateException("should not be called");
-      }
-    };
+    return new AbstractMap.SimpleEntry<>(finalIndex, possibleTargets.get(0));
   }
 
 
@@ -318,14 +338,14 @@ private Optional<ManagedFieldsEntry> checkIfFieldManagerExists(R actual, String
         .collect(Collectors.toList());
     if (targetManagedFields.isEmpty()) {
       log.debug("No field manager exists for resource {} with name: {} and operation Apply ",
-          actual, actual.getMetadata().getName());
+          actual.getKind(), actual.getMetadata().getName());
       return Optional.empty();
     }
     // this should not happen in theory
     if (targetManagedFields.size() > 1) {
       throw new OperatorException(
           "More than one field manager exists with name: " + fieldManager + "in resource: " +
-              actual + " with name: " + actual.getMetadata().getName());
+              actual.getKind() + " with name: " + actual.getMetadata().getName());
     }
     return Optional.of(targetManagedFields.get(0));
   }