various refinements to ssa logic (#2028)

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

Signed-off-by: Steven 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 - leading to infinite reconciliations. 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 on an older Kubernetes version, consider changing your desired state, turning off SSA for that resource, or even upgrading your Kubernetes version.  If you encounter it on a newer Kubernetes version, please log an issue with the JOSDK and with upstream Kubernetes.
+
 ## 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

@@ -43,11 +43,14 @@ public abstract class KubernetesDependentResource<R extends HasMetadata, P exten
   private final boolean garbageCollected = this instanceof GarbageCollected;
   private KubernetesDependentResourceConfig<R> kubernetesDependentResourceConfig;
 
+  private boolean usingCustomResourceUpdateMatcher;
+
   @SuppressWarnings("unchecked")
   public KubernetesDependentResource(Class<R> resourceType) {
     super(resourceType);
 
-    updaterMatcher = this instanceof ResourceUpdaterMatcher
+    usingCustomResourceUpdateMatcher = this instanceof ResourceUpdaterMatcher;
+    updaterMatcher = usingCustomResourceUpdateMatcher
         ? (ResourceUpdaterMatcher<R>) this
         : GenericResourceUpdaterMatcher.updaterMatcherFor(resourceType);
   }
@@ -114,7 +117,6 @@ public R create(R desired, P primary, Context<P> context) {
       }
     }
     addMetadata(false, null, desired, primary, context);
-    sanitizeDesired(desired, null, primary, context);
     final var resource = prepare(desired, primary, "Creating");
     return useSSA(context)
         ? resource
@@ -131,7 +133,6 @@ public R update(R actual, R desired, P primary, Context<P> context) {
     }
     R updatedResource;
     addMetadata(false, actual, desired, primary, context);
-    sanitizeDesired(desired, actual, primary, context);
     if (useSSA(context)) {
       updatedResource = prepare(desired, primary, "Updating")
           .fieldManager(context.getControllerConfiguration().fieldManager())
@@ -148,7 +149,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);
   }
 
@@ -193,11 +193,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 (usingCustomResourceUpdateMatcher) {
+      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.trace("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));
   }