Skip to content

Commit

Permalink
fix: default implementations shouldn't always recreate a client (#1959)
Browse files Browse the repository at this point in the history
  • Loading branch information
metacosm authored Jun 21, 2023
1 parent 6230790 commit 634aeb0
Show file tree
Hide file tree
Showing 3 changed files with 49 additions and 15 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -6,28 +6,44 @@
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;

@SuppressWarnings("rawtypes")
public class AbstractConfigurationService implements ConfigurationService {
private final Map<String, ControllerConfiguration> 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);
}

/**
Expand All @@ -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;
}
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<String> getKnownReconcilerNames() {
return original.getKnownReconcilerNames();
Expand Down Expand Up @@ -228,11 +228,6 @@ public ExecutorService getWorkflowExecutorService() {
: super.getWorkflowExecutorService();
}

@Override
public KubernetesClient getKubernetesClient() {
return client != null ? client : original.getKubernetesClient();
}

@Override
public Optional<LeaderElectionConfiguration> getLeaderElectionConfiguration() {
return leaderElectionConfiguration != null ? Optional.of(leaderElectionConfiguration)
Expand Down

0 comments on commit 634aeb0

Please sign in to comment.