fix: improve duplicated controller detection, add tests

It is now possible to add different controllers for the same CR with
different versions.
Fixes #637.

Author: metacosm

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

@@ -4,13 +4,15 @@
 import java.io.IOException;
 import java.net.ConnectException;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import io.fabric8.kubernetes.api.model.HasMetadata;
 import io.fabric8.kubernetes.client.CustomResource;
 import io.fabric8.kubernetes.client.DefaultKubernetesClient;
 import io.fabric8.kubernetes.client.KubernetesClient;
@@ -159,10 +161,13 @@ public <R extends CustomResource> void register(
     }
   }
 
-  private static class ControllerManager implements Closeable {
+  static class ControllerManager implements Closeable {
     private final Map<String, ConfiguredController> controllers = new HashMap<>();
     private boolean started = false;
 
+    synchronized Map<String, ConfiguredController> getControllers() {
+      return Collections.unmodifiableMap(controllers);
+    }
 
     public synchronized void shouldStart() {
       if (started) {
@@ -199,13 +204,15 @@ public synchronized void close() {
     public synchronized void add(ConfiguredController configuredController) {
       final var configuration = configuredController.getConfiguration();
       final var crdName = configuration.getCRDName();
-      final var existing = controllers.get(crdName);
+      final var version = configuration.getCustomResourceVersion();
+      final var key = crdName + "/" + version;
+      final var existing = controllers.get(key);
       if (existing != null) {
-        throw new OperatorException("Cannot register controller " + configuration.getName()
-            + ": another controller (" + existing.getConfiguration().getName()
-            + ") is already registered for CRD " + crdName);
+        throw new OperatorException("Cannot register controller '" + configuration.getName()
+            + "': another controller (" + existing.getConfiguration().getName()
+            + ") is already registered for CRD '" + crdName + "' (version: " + version + ")");
       }
-      this.controllers.put(crdName, configuredController);
+      this.controllers.put(key, configuredController);
       if (started) {
         configuredController.start();
       }

Diff for: operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java

@@ -1,5 +1,6 @@
 package io.javaoperatorsdk.operator.api.config;
 
+import io.fabric8.kubernetes.api.model.HasMetadata;
 import java.lang.reflect.ParameterizedType;
 import java.util.Collections;
 import java.util.Set;
@@ -20,6 +21,10 @@ default String getCRDName() {
     return CustomResource.getCRDName(getCustomResourceClass());
   }
 
+  default String getCustomResourceVersion() {
+    return HasMetadata.getVersion(getCustomResourceClass());
+  }
+
   default String getFinalizer() {
     return ControllerUtils.getDefaultFinalizerName(getCRDName());
   }

Diff for: operator-framework-core/src/test/java/io/javaoperatorsdk/operator/ControllerManagerTest.java

@@ -0,0 +1,47 @@
+package io.javaoperatorsdk.operator;
+
+import io.fabric8.kubernetes.api.model.HasMetadata;
+import io.fabric8.kubernetes.client.CustomResource;
+import io.javaoperatorsdk.operator.api.config.ControllerConfiguration;
+import io.javaoperatorsdk.operator.api.config.DefaultControllerConfiguration;
+import io.javaoperatorsdk.operator.sample.simple.TestCustomResource;
+import io.javaoperatorsdk.operator.sample.simple.TestCustomResourceV2;
+import org.junit.Test;
+
+import io.javaoperatorsdk.operator.Operator.ControllerManager;
+import io.javaoperatorsdk.operator.processing.ConfiguredController;
+import io.javaoperatorsdk.operator.sample.simple.DuplicateCRController;
+import io.javaoperatorsdk.operator.sample.simple.TestCustomResourceController;
+import io.javaoperatorsdk.operator.sample.simple.TestCustomResourceControllerV2;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+public class ControllerManagerTest {
+
+  @Test(expected = OperatorException.class)
+  public void shouldNotAddMultipleControllersForSameCustomResource() {
+    final var controllerManager = new ControllerManager();
+    controllerManager
+        .add(new ConfiguredController<>(new TestCustomResourceController(null), config(
+            TestCustomResource.class), null));
+    controllerManager.add(new ConfiguredController<>(new DuplicateCRController(), config(TestCustomResource.class), null));
+  }
+
+  @Test
+  public void addingMultipleControllersForCustomResourcesWithDifferentVersionsShouldWork() {
+    final var controllerManager = new ControllerManager();
+    controllerManager
+        .add(new ConfiguredController<>(new TestCustomResourceController(null), config(TestCustomResource.class), null));
+    controllerManager
+        .add(new ConfiguredController<>(new TestCustomResourceControllerV2(), config(
+            TestCustomResourceV2.class), null));
+    final var controllers = controllerManager.getControllers();
+    assertEquals(2, controllers.size());
+  }
+
+  private <R extends CustomResource<?,?>> ControllerConfiguration<R> config(Class<R> crClass) {
+    return new DefaultControllerConfiguration<>(null, crClass.getSimpleName() + "Controller",
+        CustomResource.getCRDName(crClass), null, false, null, null, null, null, crClass, null);
+  }
+}

Diff for: operator-framework-core/src/test/java/io/javaoperatorsdk/operator/sample/simple/DuplicateCRController.java

@@ -0,0 +1,16 @@
+package io.javaoperatorsdk.operator.sample.simple;
+
+import io.javaoperatorsdk.operator.api.Context;
+import io.javaoperatorsdk.operator.api.Controller;
+import io.javaoperatorsdk.operator.api.ResourceController;
+import io.javaoperatorsdk.operator.api.UpdateControl;
+
+@Controller
+public class DuplicateCRController implements ResourceController<TestCustomResource> {
+
+  @Override
+  public UpdateControl<TestCustomResource> createOrUpdateResource(TestCustomResource resource,
+      Context<TestCustomResource> context) {
+    return UpdateControl.noUpdate();
+  }
+}

Diff for: operator-framework-core/src/test/java/io/javaoperatorsdk/operator/sample/simple/TestCustomResourceControllerV2.java

@@ -0,0 +1,16 @@
+package io.javaoperatorsdk.operator.sample.simple;
+
+import io.javaoperatorsdk.operator.api.Controller;
+import io.javaoperatorsdk.operator.api.Context;
+import io.javaoperatorsdk.operator.api.ResourceController;
+import io.javaoperatorsdk.operator.api.UpdateControl;
+
+@Controller
+public class TestCustomResourceControllerV2 implements ResourceController<TestCustomResourceV2> {
+
+  @Override
+  public UpdateControl<TestCustomResourceV2> createOrUpdateResource(TestCustomResourceV2 resource,
+      Context<TestCustomResourceV2> context) {
+    return UpdateControl.noUpdate();
+  }
+}

Diff for: operator-framework-core/src/test/java/io/javaoperatorsdk/operator/sample/simple/TestCustomResourceV2.java

@@ -0,0 +1,12 @@
+package io.javaoperatorsdk.operator.sample.simple;
+
+import io.fabric8.kubernetes.client.CustomResource;
+import io.fabric8.kubernetes.model.annotation.Group;
+import io.fabric8.kubernetes.model.annotation.Version;
+
+@Group("sample.javaoperatorsdk.io")
+@Version("v2")
+public class TestCustomResourceV2
+    extends CustomResource<TestCustomResourceSpec, TestCustomResourceStatus> {
+
+}