feat: sanitization of resources for matching (#2042)

Signed-off-by: Attila Mészáros <[email protected]>

Author: csviri

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java

@@ -9,7 +9,9 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import io.fabric8.kubernetes.api.model.ConfigMap;
 import io.fabric8.kubernetes.api.model.HasMetadata;
+import io.fabric8.kubernetes.api.model.Secret;
 import io.fabric8.kubernetes.client.Config;
 import io.fabric8.kubernetes.client.ConfigBuilder;
 import io.fabric8.kubernetes.client.CustomResource;
@@ -19,6 +21,7 @@
 import io.javaoperatorsdk.operator.api.monitoring.Metrics;
 import io.javaoperatorsdk.operator.api.reconciler.Reconciler;
 import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResourceFactory;
+import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependent;
 import io.javaoperatorsdk.operator.processing.dependent.workflow.ManagedWorkflowFactory;
 
 import static io.javaoperatorsdk.operator.api.config.ExecutorServiceManager.newThreadPoolExecutor;
@@ -335,7 +338,7 @@ default ExecutorServiceManager getExecutorServiceManager() {
    * resources are created/updated and match was change to use
    * <a href="https://kubernetes.io/docs/reference/using-api/server-side-apply/">Server-Side
    * Apply</a> (SSA) by default.
-   *
+   * <p>
    * SSA based create/update can be still used with the legacy matching, just overriding the match
    * method of Kubernetes Dependent Resource.
    *
@@ -345,4 +348,20 @@ default boolean ssaBasedCreateUpdateMatchForDependentResources() {
     return true;
   }
 
+  /**
+   * Returns the set of default resources for which Server-Side Apply (SSA) will not be used, even
+   * if it is the default behavior for dependent resources as specified by
+   * {@link #ssaBasedCreateUpdateMatchForDependentResources()}. The exception to this is in the case
+   * where the use of SSA is explicitly enabled on the dependent resource directly using
+   * {@link KubernetesDependent#useSSA()}.
+   * <p>
+   * By default, SSA is disabled for {@link ConfigMap} and {@link Secret} resources.
+   *
+   * @return The set of resource types for which SSA will not be used
+   * @since 4.4.0
+   */
+  default Set<Class<? extends HasMetadata>> defaultNonSSAResource() {
+    return Set.of(ConfigMap.class, Secret.class);
+  }
+
 }

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java

@@ -9,6 +9,7 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import io.fabric8.kubernetes.api.model.HasMetadata;
 import io.fabric8.kubernetes.client.KubernetesClient;
 import io.javaoperatorsdk.operator.api.monitoring.Metrics;
 
@@ -35,6 +36,7 @@ public class ConfigurationServiceOverrider {
   private Duration cacheSyncTimeout;
   private ResourceClassResolver resourceClassResolver;
   private Boolean ssaBasedCreateUpdateMatchForDependentResources;
+  private Set<Class<? extends HasMetadata>> defaultNonSSAResource;
 
   ConfigurationServiceOverrider(ConfigurationService original) {
     this.original = original;
@@ -150,6 +152,12 @@ public ConfigurationServiceOverrider withSSABasedCreateUpdateMatchForDependentRe
     return this;
   }
 
+  public ConfigurationServiceOverrider withDefaultNonSSAResource(
+      Set<Class<? extends HasMetadata>> defaultNonSSAResource) {
+    this.defaultNonSSAResource = defaultNonSSAResource;
+    return this;
+  }
+
   public ConfigurationService build() {
     return new BaseConfigurationService(original.getVersion(), cloner, client) {
       @Override
@@ -256,6 +264,12 @@ public boolean ssaBasedCreateUpdateMatchForDependentResources() {
             ? ssaBasedCreateUpdateMatchForDependentResources
             : super.ssaBasedCreateUpdateMatchForDependentResources();
       }
+
+      @Override
+      public Set<Class<? extends HasMetadata>> defaultNonSSAResource() {
+        return defaultNonSSAResource != null ? defaultNonSSAResource
+            : super.defaultNonSSAResource();
+      }
     };
   }
 

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

@@ -0,0 +1,47 @@
+package io.javaoperatorsdk.operator.processing.dependent.kubernetes;
+
+import io.fabric8.kubernetes.api.model.HasMetadata;
+import io.fabric8.kubernetes.api.model.PersistentVolumeClaimStatus;
+import io.fabric8.kubernetes.api.model.Secret;
+import io.fabric8.kubernetes.api.model.apps.StatefulSet;
+import io.javaoperatorsdk.operator.api.reconciler.Context;
+
+public class DesiredResourceSanitizer {
+
+  private DesiredResourceSanitizer() {}
+
+  public static <R, P extends HasMetadata> void sanitizeDesired(R desired, R actual, P primary,
+      Context<P> context, boolean useSSA) {
+    if (useSSA) {
+      if (desired instanceof StatefulSet) {
+        fillDefaultsOnVolumeClaimTemplate((StatefulSet) desired);
+      }
+      if (desired instanceof Secret) {
+        checkIfStringDataUsed((Secret) desired);
+      }
+    }
+  }
+
+  private static void checkIfStringDataUsed(Secret secret) {
+    if (secret.getStringData() != null && !secret.getStringData().isEmpty()) {
+      throw new IllegalStateException(
+          "There is a known issue using StringData with SSA. Use data instead.");
+    }
+  }
+
+  private static void fillDefaultsOnVolumeClaimTemplate(StatefulSet statefulSet) {
+    if (!statefulSet.getSpec().getVolumeClaimTemplates().isEmpty()) {
+      statefulSet.getSpec().getVolumeClaimTemplates().forEach(t -> {
+        if (t.getSpec().getVolumeMode() == null) {
+          t.getSpec().setVolumeMode("Filesystem");
+        }
+        if (t.getStatus() == null) {
+          t.setStatus(new PersistentVolumeClaimStatus());
+        }
+        if (t.getStatus().getPhase() == null) {
+          t.getStatus().setPhase("pending");
+        }
+      });
+    }
+  }
+}

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

@@ -103,18 +103,19 @@ public void configureWith(InformerEventSource<R, P> informerEventSource) {
   }
 
   @SuppressWarnings("unused")
-  public R create(R target, P primary, Context<P> context) {
+  public R create(R desired, P primary, Context<P> context) {
     if (useSSA(context)) {
       // setting resource version for SSA so only created if it doesn't exist already
       var createIfNotExisting = kubernetesDependentResourceConfig == null
           ? KubernetesDependentResourceConfig.DEFAULT_CREATE_RESOURCE_ONLY_IF_NOT_EXISTING_WITH_SSA
           : kubernetesDependentResourceConfig.createResourceOnlyIfNotExistingWithSSA();
       if (createIfNotExisting) {
-        target.getMetadata().setResourceVersion("1");
+        desired.getMetadata().setResourceVersion("1");
       }
     }
-    addMetadata(false, null, target, primary);
-    final var resource = prepare(target, primary, "Creating");
+    addMetadata(false, null, desired, primary);
+    sanitizeDesired(desired, null, primary, context);
+    final var resource = prepare(desired, primary, "Creating");
     return useSSA(context)
         ? resource
             .fieldManager(context.getControllerConfiguration().fieldManager())
@@ -123,19 +124,20 @@ public R create(R target, P primary, Context<P> context) {
         : resource.create();
   }
 
-  public R update(R actual, R target, P primary, Context<P> context) {
+  public R update(R actual, R desired, P primary, Context<P> context) {
     if (log.isDebugEnabled()) {
       log.debug("Updating actual resource: {} version: {}", ResourceID.fromResource(actual),
           actual.getMetadata().getResourceVersion());
     }
     R updatedResource;
-    addMetadata(false, actual, target, primary);
+    addMetadata(false, actual, desired, primary);
+    sanitizeDesired(desired, actual, primary, context);
     if (useSSA(context)) {
-      updatedResource = prepare(target, primary, "Updating")
+      updatedResource = prepare(desired, primary, "Updating")
           .fieldManager(context.getControllerConfiguration().fieldManager())
           .forceConflicts().serverSideApply();
     } else {
-      var updatedActual = updaterMatcher.updateResource(actual, target, context);
+      var updatedActual = updaterMatcher.updateResource(actual, desired, context);
       updatedResource = prepare(updatedActual, primary, "Updating").update();
     }
     log.debug("Resource version after update: {}",
@@ -146,6 +148,7 @@ public R update(R actual, R target, 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);
   }
 
@@ -189,9 +192,18 @@ protected void addMetadata(boolean forMatch, R actualResource, final R target, P
     addReferenceHandlingMetadata(target, primary);
   }
 
-  private boolean useSSA(Context<P> context) {
+  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) {
     Optional<Boolean> useSSAConfig =
         configuration().flatMap(KubernetesDependentResourceConfig::useSSA);
+    var configService = context.getControllerConfiguration().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()
         .ssaBasedCreateUpdateMatchForDependentResources());
   }

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

@@ -2,6 +2,7 @@
 
 import java.util.Set;
 
+import io.javaoperatorsdk.operator.api.reconciler.Constants;
 import io.javaoperatorsdk.operator.api.reconciler.ResourceDiscriminator;
 import io.javaoperatorsdk.operator.processing.event.source.filter.GenericFilter;
 import io.javaoperatorsdk.operator.processing.event.source.filter.OnAddFilter;
@@ -10,7 +11,7 @@
 
 public final class KubernetesDependentResourceConfigBuilder<R> {
 
-  private Set<String> namespaces;
+  private Set<String> namespaces = Constants.SAME_AS_CONTROLLER_NAMESPACES_SET;
   private String labelSelector;
   private boolean createResourceOnlyIfNotExistingWithSSA;
   private ResourceDiscriminator<R, ?> resourceDiscriminator;
@@ -77,7 +78,8 @@ public KubernetesDependentResourceConfigBuilder<R> withGenericFilter(
   }
 
   public KubernetesDependentResourceConfig<R> build() {
-    return new KubernetesDependentResourceConfig<>(namespaces, labelSelector, false,
+    return new KubernetesDependentResourceConfig<>(namespaces, labelSelector,
+        namespaces != Constants.SAME_AS_CONTROLLER_NAMESPACES_SET,
         createResourceOnlyIfNotExistingWithSSA, resourceDiscriminator, useSSA, onAddFilter,
         onUpdateFilter, onDeleteFilter, genericFilter);
   }

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

@@ -1,14 +1,7 @@
 package io.javaoperatorsdk.operator.processing.dependent.kubernetes;
 
-import java.util.ArrayList;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
+import java.util.*;
 import java.util.Map.Entry;
-import java.util.Optional;
-import java.util.Set;
-import java.util.SortedMap;
-import java.util.TreeMap;
 import java.util.stream.Collectors;
 
 import org.slf4j.Logger;
@@ -159,7 +152,7 @@ private static void fillResultsAndTraverseFurther(Map<String, Object> result,
       Object managedFieldValue) {
     var emptyMapValue = new HashMap<String, Object>();
     result.put(keyInActual, emptyMapValue);
-    var actualMapValue = actualMap.get(keyInActual);
+    var actualMapValue = actualMap.getOrDefault(keyInActual, Collections.emptyMap());
     log.debug("key: {} actual map value: {} managedFieldValue: {}", keyInActual,
         actualMapValue, managedFieldValue);
 

operator-framework/src/test/java/io/javaoperatorsdk/operator/DependentSSAMigrationIT.java

@@ -144,9 +144,10 @@ private DependnetSSACustomResource reconcileWithLegacyOperator(Operator legacyOp
   private Operator createOperator(KubernetesClient client, boolean legacyDependentHandling,
       String fieldManager) {
     Operator operator = new Operator(client,
-        o -> o.withSSABasedCreateUpdateMatchForDependentResources(!legacyDependentHandling)
-            .withCloseClientOnStop(false));
-    operator.register(new DependentSSAReconciler(), o -> {
+        o -> o.withCloseClientOnStop(false));
+    var reconciler = new DependentSSAReconciler(!legacyDependentHandling);
+    reconciler.setKubernetesClient(client);
+    operator.register(reconciler, o -> {
       o.settingNamespace(namespace);
       if (fieldManager != null) {
         o.withFieldManager(fieldManager);
@@ -155,7 +156,6 @@ private Operator createOperator(KubernetesClient client, boolean legacyDependent
     return operator;
   }
 
-
   public DependnetSSACustomResource testResource() {
     DependnetSSACustomResource resource = new DependnetSSACustomResource();
     resource.setMetadata(new ObjectMetaBuilder()

operator-framework/src/test/java/io/javaoperatorsdk/operator/StatefulSetDesiredSanitizerIT.java

@@ -0,0 +1,57 @@
+package io.javaoperatorsdk.operator;
+
+import java.time.Duration;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
+
+import io.fabric8.kubernetes.api.model.ObjectMetaBuilder;
+import io.fabric8.kubernetes.api.model.apps.StatefulSet;
+import io.javaoperatorsdk.operator.junit.LocallyRunOperatorExtension;
+import io.javaoperatorsdk.operator.sample.statefulsetdesiredsanitizer.StatefulSetDesiredSanitizerCustomResource;
+import io.javaoperatorsdk.operator.sample.statefulsetdesiredsanitizer.StatefulSetDesiredSanitizerDependentResource;
+import io.javaoperatorsdk.operator.sample.statefulsetdesiredsanitizer.StatefulSetDesiredSanitizerReconciler;
+import io.javaoperatorsdk.operator.sample.statefulsetdesiredsanitizer.StatefulSetDesiredSanitizerSpec;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.awaitility.Awaitility.await;
+
+public class StatefulSetDesiredSanitizerIT {
+
+  public static final String TEST_1 = "test1";
+
+  @RegisterExtension
+  LocallyRunOperatorExtension extension =
+      LocallyRunOperatorExtension.builder()
+          .withReconciler(new StatefulSetDesiredSanitizerReconciler())
+          .build();
+
+  @Test
+  void testSSAMatcher() {
+    var resource = extension.create(testResource());
+
+    await().pollDelay(Duration.ofMillis(200)).untilAsserted(() -> {
+      var statefulSet = extension.get(StatefulSet.class, TEST_1);
+      assertThat(statefulSet).isNotNull();
+    });
+    // make sure reconciliation happens at least once more
+    resource.getSpec().setValue("changed value");
+    extension.replace(resource);
+
+    await().untilAsserted(
+        () -> assertThat(StatefulSetDesiredSanitizerDependentResource.nonMatchedAtLeastOnce)
+            .isFalse());
+  }
+
+  StatefulSetDesiredSanitizerCustomResource testResource() {
+    var res = new StatefulSetDesiredSanitizerCustomResource();
+    res.setMetadata(new ObjectMetaBuilder()
+        .withName(TEST_1)
+        .build());
+    res.setSpec(new StatefulSetDesiredSanitizerSpec());
+    res.getSpec().setValue("initial value");
+
+    return res;
+  }
+
+}