various refinements to ssa logic

- localizes sanitation to ssa matcher
- further ensures values won't be logged
- will detect if a non-meaningful revision has been created

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

Author: shawkins

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

@@ -11,6 +11,7 @@
 import io.fabric8.kubernetes.client.KubernetesClient;
 import io.fabric8.kubernetes.client.dsl.Resource;
 import io.javaoperatorsdk.operator.OperatorException;
+import io.javaoperatorsdk.operator.api.config.ControllerConfiguration;
 import io.javaoperatorsdk.operator.api.config.dependent.Configured;
 import io.javaoperatorsdk.operator.api.config.informer.InformerConfiguration;
 import io.javaoperatorsdk.operator.api.reconciler.Constants;
@@ -114,7 +115,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 +131,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 +147,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,19 +190,22 @@ 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) {
+    return useSSA(context.getControllerConfiguration());
   }
 
-  protected boolean useSSA(Context<P> context) {
+  protected boolean useSSA(ControllerConfiguration<P> controllerConfiguration) {
+    if (this instanceof ResourceUpdaterMatcher) {
+      return false;
+    }
     Optional<Boolean> useSSAConfig =
         configuration().flatMap(KubernetesDependentResourceConfig::useSSA);
-    var configService = context.getControllerConfiguration().getConfigurationService();
+    var configService = controllerConfiguration.getConfigurationService();
     // don't use SSA for certain resources by default, only if explicitly overriden
     if (useSSAConfig.isEmpty() && configService.defaultNonSSAResource().contains(resourceType())) {
       return false;
     }
-    return useSSAConfig.orElse(context.getControllerConfiguration().getConfigurationService()
+    return useSSAConfig.orElse(controllerConfiguration.getConfigurationService()
         .ssaBasedCreateUpdateMatchForDependentResources());
   }
 
@@ -261,6 +262,10 @@ protected InformerEventSource<R, P> createEventSource(EventSourceContext<P> cont
           "Using default configuration for {} KubernetesDependentResource, call configureWith to provide configuration",
           resourceType().getSimpleName());
     }
+    if (useSSA(context.getControllerConfiguration())) {
+      onUpdateFilter = new MetadataAwareOnUpdateFilter<>(onUpdateFilter,
+          context.getClient().getKubernetesSerialization());
+    }
     return eventSource().orElseThrow();
   }
 

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

@@ -0,0 +1,85 @@
+/*
+ * Copyright 2021 Red Hat, Inc. and/or its affiliates and other contributors as indicated by
+ * the @author tags.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
+ * in compliance with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package io.javaoperatorsdk.operator.processing.dependent.kubernetes;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import io.fabric8.kubernetes.api.model.HasMetadata;
+import io.fabric8.kubernetes.api.model.ObjectMeta;
+import io.fabric8.kubernetes.api.model.ObjectMetaBuilder;
+import io.fabric8.kubernetes.client.informers.cache.Cache;
+import io.fabric8.kubernetes.client.utils.KubernetesSerialization;
+import io.javaoperatorsdk.operator.processing.event.source.filter.OnUpdateFilter;
+
+import com.fasterxml.jackson.databind.node.ObjectNode;
+
+/**
+ * addresses the additional events noted on
+ * https://github.com/fabric8io/kubernetes-client/issues/5215
+ * <p>
+ * Several of these circumstances seem to have been addressed by ~1.25+
+ */
+public class MetadataAwareOnUpdateFilter<T extends HasMetadata> implements OnUpdateFilter<T> {
+
+  private static final Logger log = LoggerFactory.getLogger(MetadataAwareOnUpdateFilter.class);
+
+  private final OnUpdateFilter<T> underlyingFilter;
+  private final KubernetesSerialization kubernetesSerialization;
+
+  public MetadataAwareOnUpdateFilter(OnUpdateFilter<T> underlyingFilter,
+      KubernetesSerialization kubernetesSerialization) {
+    this.underlyingFilter = underlyingFilter;
+    this.kubernetesSerialization = kubernetesSerialization;
+  }
+
+  @Override
+  public boolean accept(T newResource, T oldResource) {
+    if (underlyingFilter != null && !underlyingFilter.accept(newResource, oldResource)) {
+      return false;
+    }
+
+    ObjectMeta newMetadata = newResource.getMetadata();
+    ObjectMeta oldMetadata = oldResource.getMetadata();
+    // quick check if anything meta has changed
+    if (newMetadata != null && oldMetadata != null
+        && !minimizeObjectMeta(newMetadata).equals(minimizeObjectMeta(oldMetadata))) {
+      return true;
+    }
+    // check everything else besides the metadata
+    // since the hierarchy of model may not implement hashCode/equals, we'll convert to a generic
+    // form
+    // that should be less expensive than full serialization
+    var newNode = kubernetesSerialization.convertValue(newResource, ObjectNode.class);
+    newNode.remove("metadata");
+    var oldNode = kubernetesSerialization.convertValue(oldResource, ObjectNode.class);
+    oldNode.remove("metadata");
+    if (newNode.equals(oldNode)) {
+      // could check the previous annotation to see if it's our update
+      log.warn("Ignoring revision with no actual changes for resource type {}, {}. "
+          + "If the operator sdk was responsible for this update, this is a situation not handled well by SSA.",
+          newResource.getKind(), Cache.metaNamespaceKeyFunc(newResource));
+      return false;
+    }
+    return true;
+  }
+
+  private ObjectMeta minimizeObjectMeta(ObjectMeta newMetadata) {
+    return new ObjectMetaBuilder(newMetadata).withManagedFields().withResourceVersion(null)
+        .withUid(null).build();
+  }
+
+}

Diff for: operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/MetadataAwareOnUpdateFilter.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,33 @@ 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"));
+          }
+          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 +185,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 +313,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 +336,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));
   }