From 634aeb0ec2cb57a6c34ff2b5be88bf6a0f2f62ce Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Wed, 21 Jun 2023 13:00:57 +0200 Subject: [PATCH] fix: default implementations shouldn't always recreate a client (#1959) --- .../config/AbstractConfigurationService.java | 48 ++++++++++++++++--- .../api/config/BaseConfigurationService.java | 9 +++- .../config/ConfigurationServiceOverrider.java | 7 +-- 3 files changed, 49 insertions(+), 15 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/AbstractConfigurationService.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/AbstractConfigurationService.java index 21039b2215..1a54dbafc7 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/AbstractConfigurationService.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/AbstractConfigurationService.java @@ -6,6 +6,7 @@ import java.util.stream.Stream; import io.fabric8.kubernetes.api.model.HasMetadata; +import io.fabric8.kubernetes.client.KubernetesClient; import io.javaoperatorsdk.operator.ReconcilerUtils; import io.javaoperatorsdk.operator.api.reconciler.Reconciler; @@ -13,21 +14,36 @@ public class AbstractConfigurationService implements ConfigurationService { private final Map configurations = new ConcurrentHashMap<>(); private final Version version; + private KubernetesClient client; private Cloner cloner; private ExecutorServiceManager executorServiceManager; public AbstractConfigurationService(Version version) { - this(version, null, null); + this(version, null); } public AbstractConfigurationService(Version version, Cloner cloner) { - this(version, cloner, null); + this(version, cloner, null, null); } + /** + * Creates a new {@link AbstractConfigurationService} with the specified parameters. + * + * @param client the {@link KubernetesClient} instance to use to connect to the cluster, if let + * {@code null}, the client will be lazily instantiated with the default configuration + * provided by {@link ConfigurationService#getKubernetesClient()} the first time + * {@link #getKubernetesClient()} is called + * @param version the version information + * @param cloner the {@link Cloner} to use, if {@code null} the default provided by + * {@link ConfigurationService#getResourceCloner()} will be used + * @param executorServiceManager the {@link ExecutorServiceManager} instance to be used, can be + * {@code null} to lazily initialize one by default when + * {@link #getExecutorServiceManager()} is called + */ public AbstractConfigurationService(Version version, Cloner cloner, - ExecutorServiceManager executorServiceManager) { + ExecutorServiceManager executorServiceManager, KubernetesClient client) { this.version = version; - init(cloner, executorServiceManager); + init(cloner, executorServiceManager, client); } /** @@ -36,10 +52,19 @@ public AbstractConfigurationService(Version version, Cloner cloner, * is useful in situations where the cloner depends on a mapper that might require additional * configuration steps before it's ready to be used. * - * @param cloner the {@link Cloner} instance to be used - * @param executorServiceManager the {@link ExecutorServiceManager} instance to be used + * @param cloner the {@link Cloner} instance to be used, if {@code null}, the default provided by + * {@link ConfigurationService#getResourceCloner()} will be used + * @param executorServiceManager the {@link ExecutorServiceManager} instance to be used, can be + * {@code null} to lazily initialize one by default when + * {@link #getExecutorServiceManager()} is called + * @param client the {@link KubernetesClient} instance to use to connect to the cluster, if let + * {@code null}, the client will be lazily instantiated with the default configuration + * provided by {@link ConfigurationService#getKubernetesClient()} the first time + * {@link #getKubernetesClient()} is called */ - protected void init(Cloner cloner, ExecutorServiceManager executorServiceManager) { + protected void init(Cloner cloner, ExecutorServiceManager executorServiceManager, + KubernetesClient client) { + this.client = client; this.cloner = cloner != null ? cloner : ConfigurationService.super.getResourceCloner(); this.executorServiceManager = executorServiceManager; } @@ -127,6 +152,15 @@ public Cloner getResourceCloner() { return cloner; } + @Override + public KubernetesClient getKubernetesClient() { + // lazy init to avoid needing initializing a client when not needed (in tests, in particular) + if (client == null) { + client = ConfigurationService.super.getKubernetesClient(); + } + return client; + } + @Override public ExecutorServiceManager getExecutorServiceManager() { // lazy init to avoid initializing thread pools for nothing in an overriding scenario diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/BaseConfigurationService.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/BaseConfigurationService.java index b7d4c2bda2..88f6b41197 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/BaseConfigurationService.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/BaseConfigurationService.java @@ -13,6 +13,7 @@ import org.slf4j.LoggerFactory; import io.fabric8.kubernetes.api.model.HasMetadata; +import io.fabric8.kubernetes.client.KubernetesClient; import io.fabric8.kubernetes.client.informers.cache.ItemStore; import io.javaoperatorsdk.operator.OperatorException; import io.javaoperatorsdk.operator.ReconcilerUtils; @@ -40,11 +41,15 @@ public class BaseConfigurationService extends AbstractConfigurationService { private static final Logger logger = LoggerFactory.getLogger(LOGGER_NAME); public BaseConfigurationService(Version version) { - super(version); + this(version, null); } public BaseConfigurationService(Version version, Cloner cloner) { - super(version, cloner); + this(version, cloner, null); + } + + public BaseConfigurationService(Version version, Cloner cloner, KubernetesClient client) { + super(version, cloner, null, client); } public BaseConfigurationService() { 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 index f66f648a1f..3006665297 100644 --- 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 @@ -158,7 +158,7 @@ public ConfigurationServiceOverrider withSSABasedDefaultMatchingForDependentReso } public ConfigurationService build() { - return new BaseConfigurationService(original.getVersion(), cloner) { + return new BaseConfigurationService(original.getVersion(), cloner, client) { @Override public Set getKnownReconcilerNames() { return original.getKnownReconcilerNames(); @@ -228,11 +228,6 @@ public ExecutorService getWorkflowExecutorService() { : super.getWorkflowExecutorService(); } - @Override - public KubernetesClient getKubernetesClient() { - return client != null ? client : original.getKubernetesClient(); - } - @Override public Optional getLeaderElectionConfiguration() { return leaderElectionConfiguration != null ? Optional.of(leaderElectionConfiguration)