fix: improve ConfigurationServiceOverrider's resilience (#1420)

Author: metacosm

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/Operator.java

@@ -31,23 +31,23 @@ public Operator() {
   }
 
   public Operator(KubernetesClient kubernetesClient) {
-    this(kubernetesClient, ConfigurationServiceProvider.instance(), null);
+    this(kubernetesClient, ConfigurationServiceProvider.instance());
   }
 
   /**
    * @deprecated Use {@link #Operator(Consumer)} instead
    */
-  @Deprecated
+  @Deprecated(forRemoval = true)
   public Operator(ConfigurationService configurationService) {
-    this(null, configurationService, null);
+    this(null, configurationService);
   }
 
   public Operator(Consumer<ConfigurationServiceOverrider> overrider) {
     this(null, overrider);
   }
 
   public Operator(KubernetesClient client, Consumer<ConfigurationServiceOverrider> overrider) {
-    this(client, ConfigurationServiceProvider.instance(), overrider);
+    this(client, ConfigurationServiceProvider.overrideCurrent(overrider));
   }
 
   /**
@@ -58,19 +58,11 @@ public Operator(KubernetesClient client, Consumer<ConfigurationServiceOverrider>
    * @param configurationService provides configuration
    */
   public Operator(KubernetesClient kubernetesClient, ConfigurationService configurationService) {
-    this(kubernetesClient, configurationService, null);
-  }
-
-  private Operator(KubernetesClient kubernetesClient, ConfigurationService configurationService,
-      Consumer<ConfigurationServiceOverrider> overrider) {
     this.kubernetesClient =
         kubernetesClient != null ? kubernetesClient : new KubernetesClientBuilder().build();
-    ConfigurationServiceProvider.set(configurationService);
-    if (overrider != null) {
-      ConfigurationServiceProvider.overrideCurrent(overrider);
-    }
-    ConfigurationServiceProvider.instance().getLeaderElectionConfiguration()
+    configurationService.getLeaderElectionConfiguration()
         .ifPresent(c -> leaderElectionManager.init(c, this.kubernetesClient));
+    ConfigurationServiceProvider.set(configurationService);
   }
 
   /** Adds a shutdown hook that automatically calls {@link #stop()} when the app shuts down. */

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

@@ -15,28 +15,19 @@ public class ConfigurationServiceOverrider {
   private final ConfigurationService original;
   private Metrics metrics;
   private Config clientConfig;
-  private boolean checkCR;
-  private int threadNumber;
+  private Boolean checkCR;
+  private Integer threadNumber;
   private Cloner cloner;
-  private int timeoutSeconds;
-  private boolean closeClientOnStop;
+  private Integer timeoutSeconds;
+  private Boolean closeClientOnStop;
   private ObjectMapper objectMapper;
-  private ExecutorService executorService = null;
+  private ExecutorService executorService;
   private LeaderElectionConfiguration leaderElectionConfiguration;
 
   ConfigurationServiceOverrider(ConfigurationService original) {
     this.original = original;
-    this.clientConfig = original.getClientConfiguration();
-    this.checkCR = original.checkCRDAndValidateLocalModel();
-    this.threadNumber = original.concurrentReconciliationThreads();
-    this.cloner = original.getResourceCloner();
-    this.timeoutSeconds = original.getTerminationTimeoutSeconds();
-    this.metrics = original.getMetrics();
-    this.closeClientOnStop = original.closeClientOnStop();
-    this.objectMapper = original.getObjectMapper();
   }
 
-
   public ConfigurationServiceOverrider withClientConfiguration(Config configuration) {
     this.clientConfig = configuration;
     return this;
@@ -97,51 +88,48 @@ public Set<String> getKnownReconcilerNames() {
 
       @Override
       public Config getClientConfiguration() {
-        return clientConfig;
+        return clientConfig != null ? clientConfig : original.getClientConfiguration();
       }
 
       @Override
       public boolean checkCRDAndValidateLocalModel() {
-        return checkCR;
+        return checkCR != null ? checkCR : original.checkCRDAndValidateLocalModel();
       }
 
       @Override
       public int concurrentReconciliationThreads() {
-        return threadNumber;
+        return threadNumber != null ? threadNumber : original.concurrentReconciliationThreads();
       }
 
       @Override
       public int getTerminationTimeoutSeconds() {
-        return timeoutSeconds;
+        return timeoutSeconds != null ? timeoutSeconds : original.getTerminationTimeoutSeconds();
       }
 
       @Override
       public Metrics getMetrics() {
-        return metrics;
+        return metrics != null ? metrics : original.getMetrics();
       }
 
       @Override
       public boolean closeClientOnStop() {
-        return closeClientOnStop;
+        return closeClientOnStop != null ? closeClientOnStop : original.closeClientOnStop();
       }
 
       @Override
       public ExecutorService getExecutorService() {
-        if (executorService != null) {
-          return executorService;
-        } else {
-          return super.getExecutorService();
-        }
+        return executorService != null ? executorService : original.getExecutorService();
       }
 
       @Override
       public ObjectMapper getObjectMapper() {
-        return objectMapper;
+        return objectMapper != null ? objectMapper : original.getObjectMapper();
       }
 
       @Override
       public Optional<LeaderElectionConfiguration> getLeaderElectionConfiguration() {
-        return Optional.ofNullable(leaderElectionConfiguration);
+        return leaderElectionConfiguration != null ? Optional.of(leaderElectionConfiguration)
+            : original.getLeaderElectionConfiguration();
       }
     };
   }

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

@@ -45,12 +45,14 @@ private static void set(ConfigurationService instance, boolean overriding) {
     ConfigurationServiceProvider.instance = instance;
   }
 
-  public synchronized static void overrideCurrent(
+  public synchronized static ConfigurationService overrideCurrent(
       Consumer<ConfigurationServiceOverrider> overrider) {
-    final var toOverride =
-        new ConfigurationServiceOverrider(ConfigurationServiceProvider.instance());
-    overrider.accept(toOverride);
-    ConfigurationServiceProvider.set(toOverride.build(), true);
+    if (overrider != null) {
+      final var toOverride = new ConfigurationServiceOverrider(instance());
+      overrider.accept(toOverride);
+      set(toOverride.build(), true);
+    }
+    return instance();
   }
 
   public synchronized static void setDefault(ConfigurationService defaultConfigurationService) {

operator-framework-core/src/test/java/io/javaoperatorsdk/operator/OperatorTest.java

@@ -11,6 +11,7 @@
 import io.fabric8.kubernetes.client.KubernetesClient;
 import io.javaoperatorsdk.operator.api.config.AbstractConfigurationService;
 import io.javaoperatorsdk.operator.api.config.ConfigurationServiceProvider;
+import io.javaoperatorsdk.operator.api.config.LeaderElectionConfiguration;
 import io.javaoperatorsdk.operator.api.reconciler.Context;
 import io.javaoperatorsdk.operator.api.reconciler.ControllerConfiguration;
 import io.javaoperatorsdk.operator.api.reconciler.Reconciler;
@@ -74,6 +75,15 @@ void shouldBePossibleToRetrieveRegisteredControllerByName() {
     assertEquals(maybeController.get(), registeredControllers.stream().findFirst().orElseThrow());
   }
 
+  @Test
+  void shouldBeAbleToProvideLeaderElectionConfiguration() {
+    assertTrue(ConfigurationServiceProvider.instance().getLeaderElectionConfiguration().isEmpty());
+    new Operator(kubernetesClient, c -> c.withLeaderElectionConfiguration(
+        new LeaderElectionConfiguration("leader-election-test", "namespace", "identity")));
+    assertEquals("identity", ConfigurationServiceProvider.instance()
+        .getLeaderElectionConfiguration().orElseThrow().getIdentity().orElseThrow());
+  }
+
   @ControllerConfiguration
   private static class FooReconciler implements Reconciler<ConfigMap> {
 

operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverriderTest.java

@@ -0,0 +1,132 @@
+package io.javaoperatorsdk.operator.api.config;
+
+import java.util.Optional;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+
+import org.junit.jupiter.api.Test;
+
+import io.fabric8.kubernetes.api.model.HasMetadata;
+import io.fabric8.kubernetes.client.Config;
+import io.fabric8.kubernetes.client.ConfigBuilder;
+import io.javaoperatorsdk.operator.api.config.dependent.DependentResourceSpec;
+import io.javaoperatorsdk.operator.api.monitoring.Metrics;
+import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource;
+import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResourceFactory;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+
+import static org.junit.jupiter.api.Assertions.*;
+
+class ConfigurationServiceOverriderTest {
+
+  private static final Metrics METRICS = new Metrics() {};
+  private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
+  private static final LeaderElectionConfiguration LEADER_ELECTION_CONFIGURATION =
+      new LeaderElectionConfiguration("foo", "fooNS");
+  private static final DependentResourceFactory FACTORY = new DependentResourceFactory() {
+    @Override
+    public <T extends DependentResource<?, ?>> T createFrom(DependentResourceSpec<T, ?> spec) {
+      return DependentResourceFactory.super.createFrom(spec);
+    }
+  };
+
+  private static final Cloner CLONER = new Cloner() {
+    @Override
+    public <R extends HasMetadata> R clone(R object) {
+      return null;
+    }
+  };
+
+  @Test
+  void overrideShouldWork() {
+    final var config = new BaseConfigurationService(null) {
+      @Override
+      public boolean checkCRDAndValidateLocalModel() {
+        return false;
+      }
+
+      @Override
+      public Config getClientConfiguration() {
+        return new ConfigBuilder().withNamespace("namespace").build();
+      }
+
+      @Override
+      public int concurrentReconciliationThreads() {
+        return -1;
+      }
+
+      @Override
+      public int getTerminationTimeoutSeconds() {
+        return -1;
+      }
+
+      @Override
+      public Metrics getMetrics() {
+        return METRICS;
+      }
+
+      @Override
+      public ExecutorService getExecutorService() {
+        return null;
+      }
+
+      @Override
+      public boolean closeClientOnStop() {
+        return true;
+      }
+
+      @Override
+      public ObjectMapper getObjectMapper() {
+        return OBJECT_MAPPER;
+      }
+
+      @Override
+      public Cloner getResourceCloner() {
+        return CLONER;
+      }
+
+      @Override
+      public DependentResourceFactory dependentResourceFactory() {
+        return FACTORY;
+      }
+
+      @Override
+      public Optional<LeaderElectionConfiguration> getLeaderElectionConfiguration() {
+        return Optional.of(LEADER_ELECTION_CONFIGURATION);
+      }
+    };
+    final var overridden = new ConfigurationServiceOverrider(config)
+        .withClientConfiguration(new ConfigBuilder().withNamespace("newNS").build())
+        .checkingCRDAndValidateLocalModel(true)
+        .withExecutorService(Executors.newSingleThreadExecutor())
+        .withCloseClientOnStop(false)
+        .withObjectMapper(new ObjectMapper())
+        .withResourceCloner(new Cloner() {
+          @Override
+          public <R extends HasMetadata> R clone(R object) {
+            return null;
+          }
+        })
+        .withConcurrentReconciliationThreads(25)
+        .withTerminationTimeoutSeconds(100)
+        .withMetrics(new Metrics() {})
+        .withLeaderElectionConfiguration(new LeaderElectionConfiguration("newLease", "newLeaseNS"))
+        .build();
+
+    assertNotEquals(config.closeClientOnStop(), overridden.closeClientOnStop());
+    assertNotEquals(config.checkCRDAndValidateLocalModel(),
+        overridden.checkCRDAndValidateLocalModel());
+    assertNotEquals(config.concurrentReconciliationThreads(),
+        overridden.concurrentReconciliationThreads());
+    assertNotEquals(config.getTerminationTimeoutSeconds(),
+        overridden.getTerminationTimeoutSeconds());
+    assertNotEquals(config.dependentResourceFactory(), overridden.dependentResourceFactory());
+    assertNotEquals(config.getClientConfiguration(), overridden.getClientConfiguration());
+    assertNotEquals(config.getExecutorService(), overridden.getExecutorService());
+    assertNotEquals(config.getMetrics(), overridden.getMetrics());
+    assertNotEquals(config.getObjectMapper(), overridden.getObjectMapper());
+    assertNotEquals(config.getLeaderElectionConfiguration(),
+        overridden.getLeaderElectionConfiguration());
+  }
+}

operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceProviderTest.java

@@ -34,17 +34,23 @@ void shouldProvideTheSetInstanceIfProvided() {
 
   @Test
   void shouldBePossibleToOverrideConfigOnce() {
-    final var config = new AbstractConfigurationService(null);
+    final var config = new ConfigurationServiceOverrider(new AbstractConfigurationService(null))
+        .withLeaderElectionConfiguration(new LeaderElectionConfiguration("bar", "barNS"))
+        .build();
     assertFalse(config.checkCRDAndValidateLocalModel());
+    assertEquals("bar", config.getLeaderElectionConfiguration().orElseThrow().getLeaseName());
 
     ConfigurationServiceProvider.set(config);
     var instance = ConfigurationServiceProvider.instance();
     assertEquals(config, instance);
 
-    ConfigurationServiceProvider.overrideCurrent(o -> o.checkingCRDAndValidateLocalModel(true));
+    final ConfigurationService overridden = ConfigurationServiceProvider.overrideCurrent(
+        o -> o.checkingCRDAndValidateLocalModel(true));
     instance = ConfigurationServiceProvider.instance();
+    assertEquals(overridden, instance);
     assertNotEquals(config, instance);
     assertTrue(instance.checkCRDAndValidateLocalModel());
+    assertEquals("bar", instance.getLeaderElectionConfiguration().orElseThrow().getLeaseName());
 
     assertThrows(IllegalStateException.class,
         () -> ConfigurationServiceProvider.overrideCurrent(o -> o.withCloseClientOnStop(false)));

sample-operators/leader-election/src/test/java/io/javaoperatorsdk/operator/sample/LeaderElectionE2E.java

@@ -50,10 +50,10 @@ class LeaderElectionE2E {
   // not for local mode by design
   @EnabledIfSystemProperty(named = "test.deployment", matches = "remote")
   void otherInstancesTakesOverWhenSteppingDown() {
-    log.info("Deploying operator");
-    deployOperatorsInOrder();
     log.info("Applying custom resource");
     applyCustomResource();
+    log.info("Deploying operator instances");
+    deployOperatorsInOrder();
 
     log.info("Awaiting custom resource reconciliations");
     await().pollDelay(Duration.ofSeconds(MINIMAL_SECONDS_FOR_RENEWAL))
@@ -131,12 +131,14 @@ void tearDown() {
   }
 
   private void deployOperatorsInOrder() {
+    log.info("Installing 1st instance");
     applyResources("k8s/operator.yaml");
     await().atMost(Duration.ofSeconds(POD_STARTUP_TIMEOUT)).untilAsserted(() -> {
       var pod = client.pods().inNamespace(namespace).withName(OPERATOR_1_POD_NAME).get();
       assertThat(pod.getStatus().getContainerStatuses().get(0).getReady()).isTrue();
     });
 
+    log.info("Installing 2nd instance");
     applyResources("k8s/operator-instance-2.yaml");
     await().atMost(Duration.ofSeconds(POD_STARTUP_TIMEOUT)).untilAsserted(() -> {
       var pod = client.pods().inNamespace(namespace).withName(OPERATOR_2_POD_NAME).get();