feat: feature flag to not clone secondary resource on access (#2364)

csviri · csviri · commit 486c7502f50d · 2024-05-21T21:02:49.000+02:00
Signed-off-by: Attila Mészáros &lt;csviri@gmail.com&gt;
diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java
@@ -20,6 +20,7 @@
 import io.fabric8.kubernetes.client.KubernetesClientBuilder;
 import io.fabric8.kubernetes.client.utils.KubernetesSerialization;
 import io.javaoperatorsdk.operator.api.monitoring.Metrics;
+import io.javaoperatorsdk.operator.api.reconciler.Context;
 import io.javaoperatorsdk.operator.api.reconciler.Reconciler;
 import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResourceFactory;
 import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependent;
@@ -396,12 +397,36 @@ default boolean parseResourceVersionsForEventFilteringAndCaching() {
    * either use simple patches or SSA. Setting this to {@code true}, controllers will use SSA for
    * adding finalizers, managing observed generation, patching resources and status.
    *
-   * @return {@code true} by default
+   * @return {@code true} if Server-Side Apply (SSA) should be used when patching the primary
+   *         resources, {@code false} otherwise
    * @since 5.0.0
    * @see ConfigurationServiceOverrider#withUseSSAToPatchPrimaryResource(boolean)
    */
   default boolean useSSAToPatchPrimaryResource() {
     return true;
   }
 
+  /**
+   * <p>
+   * Determines whether resources retrieved from caches such as via calls to
+   * {@link Context#getSecondaryResource(Class)} should be defensively cloned first.
+   * </p>
+   * 
+   * <p>
+   * Defensive cloning to prevent problematic cache modifications (modifying the resource would
+   * otherwise modify the stored copy in the cache) was transparently done in previous JOSDK
+   * versions. This might have performance consequences and, with the more prevalent use of
+   * Server-Side Apply, where you should create a new copy of your resource with only modified
+   * fields, such modifications of these resources are less likely to occur.
+   * </p>
+   * 
+   * @return {@code true} if resources should be defensively cloned before returning them from
+   *         caches, {@code false} otherwise
+   * 
+   * @since 5.0.0
+   */
+  default boolean cloneSecondaryResourcesWhenGettingFromCache() {
+    return false;
+  }
+
 }
diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java
@@ -5,6 +5,7 @@
 import java.util.Set;
 import java.util.concurrent.ExecutorService;
 import java.util.function.Consumer;
+import java.util.function.Function;
 
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -40,6 +41,7 @@ public class ConfigurationServiceOverrider {
   private Boolean previousAnnotationForDependentResources;
   private Boolean parseResourceVersions;
   private Boolean useSSAToPatchPrimaryResource;
+  private Boolean cloneSecondaryResourcesWhenGettingFromCache;
   private DependentResourceFactory dependentResourceFactory;
 
   ConfigurationServiceOverrider(ConfigurationService original) {
@@ -180,29 +182,42 @@ public ConfigurationServiceOverrider withUseSSAToPatchPrimaryResource(boolean va
     return this;
   }
 
+  public ConfigurationServiceOverrider withCloneSecondaryResourcesWhenGettingFromCache(
+      boolean value) {
+    this.cloneSecondaryResourcesWhenGettingFromCache = value;
+    return this;
+  }
+
   public ConfigurationService build() {
     return new BaseConfigurationService(original.getVersion(), cloner, client) {
       @Override
       public Set<String> getKnownReconcilerNames() {
         return original.getKnownReconcilerNames();
       }
 
+      private <T> T overriddenValueOrDefault(T value,
+          Function<ConfigurationService, T> defaultValue) {
+        return value != null ? value : defaultValue.apply(original);
+      }
+
       @Override
       public boolean checkCRDAndValidateLocalModel() {
-        return checkCR != null ? checkCR : original.checkCRDAndValidateLocalModel();
+        return overriddenValueOrDefault(checkCR,
+            ConfigurationService::checkCRDAndValidateLocalModel);
       }
 
       @Override
+      @SuppressWarnings("rawtypes")
       public DependentResourceFactory dependentResourceFactory() {
-        return dependentResourceFactory != null ? dependentResourceFactory
-            : DependentResourceFactory.DEFAULT;
+        return overriddenValueOrDefault(dependentResourceFactory,
+            ConfigurationService::dependentResourceFactory);
       }
 
       @Override
       public int concurrentReconciliationThreads() {
         return Utils.ensureValid(
-            concurrentReconciliationThreads != null ? concurrentReconciliationThreads
-                : original.concurrentReconciliationThreads(),
+            overriddenValueOrDefault(concurrentReconciliationThreads,
+                ConfigurationService::concurrentReconciliationThreads),
             "maximum reconciliation threads",
             minimumMaxValueFor(minConcurrentReconciliationThreads),
             original.concurrentReconciliationThreads());
@@ -211,8 +226,8 @@ public int concurrentReconciliationThreads() {
       @Override
       public int concurrentWorkflowExecutorThreads() {
         return Utils.ensureValid(
-            concurrentWorkflowExecutorThreads != null ? concurrentWorkflowExecutorThreads
-                : original.concurrentWorkflowExecutorThreads(),
+            overriddenValueOrDefault(concurrentWorkflowExecutorThreads,
+                ConfigurationService::concurrentWorkflowExecutorThreads),
             "maximum workflow execution threads",
             minimumMaxValueFor(minConcurrentWorkflowExecutorThreads),
             original.concurrentWorkflowExecutorThreads());
@@ -224,8 +239,8 @@ public int concurrentWorkflowExecutorThreads() {
       @Deprecated(forRemoval = true)
       @Override
       public int minConcurrentReconciliationThreads() {
-        return minConcurrentReconciliationThreads != null ? minConcurrentReconciliationThreads
-            : original.minConcurrentReconciliationThreads();
+        return overriddenValueOrDefault(minConcurrentReconciliationThreads,
+            ConfigurationService::minConcurrentReconciliationThreads);
       }
 
       /**
@@ -234,30 +249,29 @@ public int minConcurrentReconciliationThreads() {
       @Override
       @Deprecated(forRemoval = true)
       public int minConcurrentWorkflowExecutorThreads() {
-        return minConcurrentWorkflowExecutorThreads != null ? minConcurrentWorkflowExecutorThreads
-            : original.minConcurrentWorkflowExecutorThreads();
+        return overriddenValueOrDefault(minConcurrentWorkflowExecutorThreads,
+            ConfigurationService::minConcurrentWorkflowExecutorThreads);
       }
 
       @Override
       public Metrics getMetrics() {
-        return metrics != null ? metrics : original.getMetrics();
+        return overriddenValueOrDefault(metrics, ConfigurationService::getMetrics);
       }
 
       @Override
       public boolean closeClientOnStop() {
-        return closeClientOnStop != null ? closeClientOnStop : original.closeClientOnStop();
+        return overriddenValueOrDefault(closeClientOnStop, ConfigurationService::closeClientOnStop);
       }
 
       @Override
       public ExecutorService getExecutorService() {
-        return executorService != null ? executorService
-            : super.getExecutorService();
+        return overriddenValueOrDefault(executorService, ConfigurationService::getExecutorService);
       }
 
       @Override
       public ExecutorService getWorkflowExecutorService() {
-        return workflowExecutorService != null ? workflowExecutorService
-            : super.getWorkflowExecutorService();
+        return overriddenValueOrDefault(workflowExecutorService,
+            ConfigurationService::getWorkflowExecutorService);
       }
 
       @Override
@@ -274,54 +288,55 @@ public Optional<InformerStoppedHandler> getInformerStoppedHandler() {
 
       @Override
       public boolean stopOnInformerErrorDuringStartup() {
-        return stopOnInformerErrorDuringStartup != null ? stopOnInformerErrorDuringStartup
-            : super.stopOnInformerErrorDuringStartup();
+        return overriddenValueOrDefault(stopOnInformerErrorDuringStartup,
+            ConfigurationService::stopOnInformerErrorDuringStartup);
       }
 
       @Override
       public Duration cacheSyncTimeout() {
-        return cacheSyncTimeout != null ? cacheSyncTimeout : super.cacheSyncTimeout();
+        return overriddenValueOrDefault(cacheSyncTimeout, ConfigurationService::cacheSyncTimeout);
       }
 
       @Override
       public ResourceClassResolver getResourceClassResolver() {
-        return resourceClassResolver != null ? resourceClassResolver
-            : super.getResourceClassResolver();
+        return overriddenValueOrDefault(resourceClassResolver,
+            ConfigurationService::getResourceClassResolver);
       }
 
       @Override
       public boolean ssaBasedCreateUpdateMatchForDependentResources() {
-        return ssaBasedCreateUpdateMatchForDependentResources != null
-            ? ssaBasedCreateUpdateMatchForDependentResources
-            : super.ssaBasedCreateUpdateMatchForDependentResources();
+        return overriddenValueOrDefault(ssaBasedCreateUpdateMatchForDependentResources,
+            ConfigurationService::ssaBasedCreateUpdateMatchForDependentResources);
       }
 
       @Override
       public Set<Class<? extends HasMetadata>> defaultNonSSAResource() {
-        return defaultNonSSAResource != null ? defaultNonSSAResource
-            : super.defaultNonSSAResource();
+        return overriddenValueOrDefault(defaultNonSSAResource,
+            ConfigurationService::defaultNonSSAResource);
       }
 
       @Override
       public boolean previousAnnotationForDependentResourcesEventFiltering() {
-        return previousAnnotationForDependentResources != null
-            ? previousAnnotationForDependentResources
-            : super.previousAnnotationForDependentResourcesEventFiltering();
+        return overriddenValueOrDefault(previousAnnotationForDependentResources,
+            ConfigurationService::previousAnnotationForDependentResourcesEventFiltering);
       }
 
       @Override
       public boolean parseResourceVersionsForEventFilteringAndCaching() {
-        return parseResourceVersions != null
-            ? parseResourceVersions
-            : super.parseResourceVersionsForEventFilteringAndCaching();
+        return overriddenValueOrDefault(parseResourceVersions,
+            ConfigurationService::parseResourceVersionsForEventFilteringAndCaching);
       }
 
       @Override
       public boolean useSSAToPatchPrimaryResource() {
-        return useSSAToPatchPrimaryResource != null
-            ? useSSAToPatchPrimaryResource
-            : super.useSSAToPatchPrimaryResource();
+        return overriddenValueOrDefault(useSSAToPatchPrimaryResource,
+            ConfigurationService::useSSAToPatchPrimaryResource);
+      }
 
+      @Override
+      public boolean cloneSecondaryResourcesWhenGettingFromCache() {
+        return overriddenValueOrDefault(cloneSecondaryResourcesWhenGettingFromCache,
+            ConfigurationService::cloneSecondaryResourcesWhenGettingFromCache);
       }
     };
   }
diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerManager.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerManager.java
@@ -170,7 +170,9 @@ public Stream<T> list(String namespace, Predicate<T> predicate) {
   public Optional<T> get(ResourceID resourceID) {
     return getSource(resourceID.getNamespace().orElse(WATCH_ALL_NAMESPACES))
         .flatMap(source -> source.get(resourceID))
-        .map(r -> configurationService.getResourceCloner().clone(r));
+        .map(r -> configurationService.cloneSecondaryResourcesWhenGettingFromCache()
+            ? configurationService.getResourceCloner().clone(r)
+            : r);
   }
 
   @Override
diff --git a/sample-operators/webpage/src/main/java/io/javaoperatorsdk/operator/sample/WebPageReconciler.java b/sample-operators/webpage/src/main/java/io/javaoperatorsdk/operator/sample/WebPageReconciler.java
@@ -13,7 +13,6 @@
 import io.fabric8.kubernetes.api.model.apps.Deployment;
 import io.fabric8.kubernetes.api.model.networking.v1.Ingress;
 import io.fabric8.kubernetes.client.KubernetesClient;
-import io.fabric8.kubernetes.client.dsl.Replaceable;
 import io.javaoperatorsdk.operator.ReconcilerUtils;
 import io.javaoperatorsdk.operator.api.config.informer.InformerConfiguration;
 import io.javaoperatorsdk.operator.api.reconciler.*;
@@ -91,7 +90,7 @@ public UpdateControl<WebPage> reconcile(WebPage webPage, Context<WebPage> contex
           desiredHtmlConfigMap.getMetadata().getName(),
           ns);
       kubernetesClient.configMaps().inNamespace(ns).resource(desiredHtmlConfigMap)
-          .createOr(Replaceable::update);
+          .serverSideApply();
     }
 
     var existingDeployment = context.getSecondaryResource(Deployment.class).orElse(null);
@@ -101,7 +100,7 @@ public UpdateControl<WebPage> reconcile(WebPage webPage, Context<WebPage> contex
           desiredDeployment.getMetadata().getName(),
           ns);
       kubernetesClient.apps().deployments().inNamespace(ns).resource(desiredDeployment)
-          .createOr(Replaceable::update);
+          .serverSideApply();
     }
 
     var existingService = context.getSecondaryResource(Service.class).orElse(null);
@@ -111,14 +110,14 @@ public UpdateControl<WebPage> reconcile(WebPage webPage, Context<WebPage> contex
           desiredDeployment.getMetadata().getName(),
           ns);
       kubernetesClient.services().inNamespace(ns).resource(desiredService)
-          .createOr(Replaceable::update);
+          .serverSideApply();
     }
 
     var existingIngress = context.getSecondaryResource(Ingress.class);
     if (Boolean.TRUE.equals(webPage.getSpec().getExposed())) {
       var desiredIngress = makeDesiredIngress(webPage);
       if (existingIngress.isEmpty() || !match(desiredIngress, existingIngress.get())) {
-        kubernetesClient.resource(desiredIngress).inNamespace(ns).createOr(Replaceable::update);
+        kubernetesClient.resource(desiredIngress).inNamespace(ns).serverSideApply();
       }
     } else
       existingIngress.ifPresent(
