fix: decouple DependentResource creation from configuration (#883)

metacosm · web-flow · commit 45b187dc302c · 2022-01-31T22:21:49.000+01:00
The idea is to delay instantiation until needed and rely on
DependentResourceControllerFactory for this purpose instead. Not sure if
we actually need to use an interface there or not.
diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java
@@ -48,7 +48,7 @@ default ResourceEventFilter<R> getEventFilter() {
     return ResourceEventFilters.passthrough();
   }
 
-  default List<DependentResource> getDependentResources() {
+  default List<DependentResourceConfiguration> getDependentResources() {
     return Collections.emptyList();
   }
 
diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/DefaultControllerConfiguration.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/DefaultControllerConfiguration.java
@@ -18,7 +18,7 @@ public class DefaultControllerConfiguration<R extends HasMetadata>
   private final boolean generationAware;
   private final RetryConfiguration retryConfiguration;
   private final ResourceEventFilter<R> resourceEventFilter;
-  private final List<DependentResource> dependents;
+  private final List<DependentResourceConfiguration> dependents;
 
   // NOSONAR constructor is meant to provide all information
   public DefaultControllerConfiguration(
@@ -33,7 +33,7 @@ public DefaultControllerConfiguration(
       ResourceEventFilter<R> resourceEventFilter,
       Class<R> resourceClass,
       ConfigurationService service,
-      List<DependentResource> dependents) {
+      List<DependentResourceConfiguration> dependents) {
     super(labelSelector, resourceClass, namespaces);
     this.associatedControllerClassName = associatedControllerClassName;
     this.name = name;
@@ -96,7 +96,7 @@ public ResourceEventFilter<R> getEventFilter() {
   }
 
   @Override
-  public List<DependentResource> getDependentResources() {
+  public List<DependentResourceConfiguration> getDependentResources() {
     return dependents;
   }
 }
diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/DefaultResourceConfiguration.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/DefaultResourceConfiguration.java
@@ -23,8 +23,7 @@ public DefaultResourceConfiguration(String labelSelector, Class<R> resourceClass
   public DefaultResourceConfiguration(String labelSelector, Class<R> resourceClass,
       Set<String> namespaces) {
     this.labelSelector = labelSelector;
-    this.resourceClass = resourceClass == null ? ResourceConfiguration.super.getResourceClass()
-        : resourceClass;
+    this.resourceClass = resourceClass;
     this.namespaces = namespaces != null ? namespaces : Collections.emptySet();
     this.watchAllNamespaces = this.namespaces.isEmpty();
   }
diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/DependentResourceConfiguration.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/DependentResourceConfiguration.java
@@ -0,0 +1,10 @@
+package io.javaoperatorsdk.operator.api.config;
+
+import io.fabric8.kubernetes.api.model.HasMetadata;
+
+public interface DependentResourceConfiguration<R, P extends HasMetadata> {
+
+  Class<? extends DependentResource<R, P>> getDependentResourceClass();
+
+  Class<R> getResourceClass();
+}
diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/DependentResourceController.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/DependentResourceController.java
@@ -2,11 +2,12 @@
 
 import io.fabric8.kubernetes.api.model.HasMetadata;
 import io.javaoperatorsdk.operator.api.config.DependentResource;
+import io.javaoperatorsdk.operator.api.config.DependentResourceConfiguration;
 import io.javaoperatorsdk.operator.api.reconciler.Context;
 import io.javaoperatorsdk.operator.api.reconciler.EventSourceContext;
 import io.javaoperatorsdk.operator.processing.event.source.EventSource;
 
-public class DependentResourceController<R, P extends HasMetadata>
+public class DependentResourceController<R, P extends HasMetadata, C extends DependentResourceConfiguration<R, P>>
     implements DependentResource<R, P>, Builder<R, P>, Updater<R, P>, Persister<R, P>,
     Cleaner<R, P> {
 
@@ -15,14 +16,16 @@ public class DependentResourceController<R, P extends HasMetadata>
   private final Cleaner<R, P> cleaner;
   private final Persister<R, P> persister;
   private final DependentResource<R, P> delegate;
+  private final C configuration;
 
   @SuppressWarnings("unchecked")
-  public DependentResourceController(DependentResource<R, P> delegate) {
+  public DependentResourceController(DependentResource<R, P> delegate, C configuration) {
     this.delegate = delegate;
     builder = (delegate instanceof Builder) ? (Builder<R, P>) delegate : null;
     updater = (delegate instanceof Updater) ? (Updater<R, P>) delegate : null;
     cleaner = (delegate instanceof Cleaner) ? (Cleaner<R, P>) delegate : null;
     persister = initPersister(delegate);
+    this.configuration = configuration;
   }
 
   @SuppressWarnings("unchecked")
@@ -84,4 +87,8 @@ public void createOrReplace(R dependentResource, Context context) {
   public R getFor(P primary, Context context) {
     return persister.getFor(primary, context);
   }
+
+  public C getConfiguration() {
+    return configuration;
+  }
 }
diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/DependentResourceControllerFactory.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/DependentResourceControllerFactory.java
@@ -1,14 +1,26 @@
 package io.javaoperatorsdk.operator.api.reconciler.dependent;
 
+import java.lang.reflect.InvocationTargetException;
+
 import io.fabric8.kubernetes.api.model.HasMetadata;
-import io.javaoperatorsdk.operator.api.config.DependentResource;
+import io.javaoperatorsdk.operator.api.config.DependentResourceConfiguration;
 
 public interface DependentResourceControllerFactory<P extends HasMetadata> {
 
-  default <R> DependentResourceController<R, P> from(DependentResource<R, P> dependent) {
-    // todo: this needs to be cleaned-up / redesigned
-    return dependent instanceof DependentResourceController
-        ? (DependentResourceController<R, P>) dependent
-        : new DependentResourceController<>(dependent);
+  default <T extends DependentResourceController<R, P, C>, R, C extends DependentResourceConfiguration<R, P>> T from(
+      C config) {
+    try {
+      final var dependentResource = config.getDependentResourceClass().getConstructor()
+          .newInstance();
+      if (config instanceof KubernetesDependentResourceConfiguration) {
+        return (T) new KubernetesDependentResourceController(dependentResource,
+            (KubernetesDependentResourceConfiguration) config);
+      } else {
+        return (T) new DependentResourceController(dependentResource, config);
+      }
+    } catch (NoSuchMethodException | InvocationTargetException | InstantiationException
+        | IllegalAccessException e) {
+      throw new IllegalArgumentException(e);
+    }
   }
 }
diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/KubernetesDependentResourceConfiguration.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/KubernetesDependentResourceConfiguration.java
@@ -4,35 +4,64 @@
 
 import io.fabric8.kubernetes.api.model.HasMetadata;
 import io.javaoperatorsdk.operator.api.config.ConfigurationService;
+import io.javaoperatorsdk.operator.api.config.DependentResource;
+import io.javaoperatorsdk.operator.api.config.DependentResourceConfiguration;
 import io.javaoperatorsdk.operator.processing.event.source.AssociatedSecondaryResourceIdentifier;
 import io.javaoperatorsdk.operator.processing.event.source.PrimaryResourcesRetriever;
 import io.javaoperatorsdk.operator.processing.event.source.informer.InformerConfiguration;
 
-public class KubernetesDependentResourceConfiguration<R extends HasMetadata, P extends HasMetadata>
-    extends InformerConfiguration<R, P> {
+public interface KubernetesDependentResourceConfiguration<R extends HasMetadata, P extends HasMetadata>
+    extends InformerConfiguration<R, P>, DependentResourceConfiguration<R, P> {
 
-  private final boolean owned;
+  class DefaultKubernetesDependentResourceConfiguration<R extends HasMetadata, P extends HasMetadata>
+      extends DefaultInformerConfiguration<R, P>
+      implements KubernetesDependentResourceConfiguration<R, P> {
 
-  protected KubernetesDependentResourceConfiguration(
-      ConfigurationService service,
-      String labelSelector, Class<R> resourceClass,
-      PrimaryResourcesRetriever<R> secondaryToPrimaryResourcesIdSet,
-      AssociatedSecondaryResourceIdentifier<P> associatedWith,
-      boolean skipUpdateEventPropagationIfNoChange, Set<String> namespaces, boolean owned) {
-    super(service, labelSelector, resourceClass, secondaryToPrimaryResourcesIdSet, associatedWith,
-        skipUpdateEventPropagationIfNoChange, namespaces);
-    this.owned = owned;
+    private final boolean owned;
+    private final Class<DependentResource<R, P>> dependentResourceClass;
+
+    protected DefaultKubernetesDependentResourceConfiguration(
+        ConfigurationService service,
+        String labelSelector, Class<R> resourceClass,
+        PrimaryResourcesRetriever<R> secondaryToPrimaryResourcesIdSet,
+        AssociatedSecondaryResourceIdentifier<P> associatedWith,
+        boolean skipUpdateEventPropagationIfNoChange, Set<String> namespaces, boolean owned,
+        Class<DependentResource<R, P>> dependentResourceClass) {
+      super(service, labelSelector, resourceClass, secondaryToPrimaryResourcesIdSet, associatedWith,
+          skipUpdateEventPropagationIfNoChange, namespaces);
+      this.owned = owned;
+      this.dependentResourceClass = dependentResourceClass;
+    }
+
+    public boolean isOwned() {
+      return owned;
+    }
+
+    @Override
+    public Class<? extends DependentResource<R, P>> getDependentResourceClass() {
+      return dependentResourceClass;
+    }
+
+    @Override
+    public Class<R> getResourceClass() {
+      return super.getResourceClass();
+    }
   }
 
-  public static <R extends HasMetadata, P extends HasMetadata> KubernetesDependentResourceConfiguration<R, P> from(
-      InformerConfiguration<R, P> cfg, boolean owned) {
-    return new KubernetesDependentResourceConfiguration<>(cfg.getConfigurationService(),
+  static <R extends HasMetadata, P extends HasMetadata> KubernetesDependentResourceConfiguration<R, P> from(
+      InformerConfiguration<R, P> cfg, boolean owned,
+      Class<? extends DependentResource> dependentResourceClass) {
+    return new DefaultKubernetesDependentResourceConfiguration<R, P>(cfg.getConfigurationService(),
         cfg.getLabelSelector(), cfg.getResourceClass(), cfg.getPrimaryResourcesRetriever(),
         cfg.getAssociatedResourceIdentifier(), cfg.isSkipUpdateEventPropagationIfNoChange(),
-        cfg.getNamespaces(), owned);
+        cfg.getNamespaces(), owned,
+        (Class<DependentResource<R, P>>) dependentResourceClass);
   }
 
-  public boolean isOwned() {
-    return owned;
+  boolean isOwned();
+
+  @Override
+  default Class<R> getResourceClass() {
+    return InformerConfiguration.super.getResourceClass();
   }
 }
diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/KubernetesDependentResourceController.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/KubernetesDependentResourceController.java
@@ -12,15 +12,16 @@
 import io.javaoperatorsdk.operator.processing.event.source.informer.InformerEventSource;
 
 public class KubernetesDependentResourceController<R extends HasMetadata, P extends HasMetadata>
-    extends DependentResourceController<R, P> {
+    extends DependentResourceController<R, P, KubernetesDependentResourceConfiguration<R, P>> {
+
   private final KubernetesDependentResourceConfiguration<R, P> configuration;
   private KubernetesClient client;
   private InformerEventSource<R, P> informer;
 
 
   public KubernetesDependentResourceController(DependentResource<R, P> delegate,
       KubernetesDependentResourceConfiguration<R, P> configuration) {
-    super(delegate);
+    super(delegate, configuration);
     // todo: check if we can validate that types actually match properly
     final var associatedPrimaries =
         (delegate instanceof PrimaryResourcesRetriever)
@@ -36,7 +37,8 @@ public KubernetesDependentResourceController(DependentResource<R, P> delegate,
         .withAssociatedSecondaryResourceIdentifier(associatedSecondary)
         .build();
     this.configuration =
-        KubernetesDependentResourceConfiguration.from(augmented, configuration.isOwned());
+        KubernetesDependentResourceConfiguration.from(augmented, configuration.isOwned(),
+            configuration.getDependentResourceClass());
   }
 
   @Override
@@ -69,6 +71,6 @@ public R getFor(P primary, Context context) {
   }
 
   public boolean owned() {
-    return configuration.isOwned();
+    return getConfiguration().isOwned();
   }
 }
diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/DependentResourceManager.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/DependentResourceManager.java
@@ -8,7 +8,7 @@
 
 import io.fabric8.kubernetes.api.model.HasMetadata;
 import io.javaoperatorsdk.operator.api.config.ControllerConfiguration;
-import io.javaoperatorsdk.operator.api.config.DependentResource;
+import io.javaoperatorsdk.operator.api.config.DependentResourceConfiguration;
 import io.javaoperatorsdk.operator.api.reconciler.Context;
 import io.javaoperatorsdk.operator.api.reconciler.ContextInitializer;
 import io.javaoperatorsdk.operator.api.reconciler.DeleteControl;
@@ -42,13 +42,14 @@ public DependentResourceManager(Controller<R> controller) {
 
   @Override
   public List<EventSource> prepareEventSources(EventSourceContext<R> context) {
-    final List<DependentResource> configured = configuration.getDependentResources();
+    final List<DependentResourceConfiguration> configured = configuration.getDependentResources();
     dependents = new ArrayList<>(configured.size());
 
     List<EventSource> sources = new ArrayList<>(configured.size() + 5);
     configured.forEach(dependent -> {
-      dependents.add(configuration.dependentFactory().from(dependent));
-      sources.add(dependent.initEventSource(context));
+      final var dependentResourceController = configuration.dependentFactory().from(dependent);
+      dependents.add(dependentResourceController);
+      sources.add(dependentResourceController.initEventSource(context));
     });
 
     return sources;
diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerConfiguration.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerConfiguration.java
diff --git a/operator-framework/src/main/java/io/javaoperatorsdk/operator/config/runtime/AnnotationConfiguration.java b/operator-framework/src/main/java/io/javaoperatorsdk/operator/config/runtime/AnnotationConfiguration.java

Original file line number	Diff line number	Diff line change
`@@ -48,7 +48,7 @@ default ResourceEventFilter<R> getEventFilter() {`
`48`	`48`	`return ResourceEventFilters.passthrough();`
`49`	`49`	`}`
`50`	`50`
`51`		`- default List<DependentResource> getDependentResources() {`
	`51`	`+ default List<DependentResourceConfiguration> getDependentResources() {`
`52`	`52`	`return Collections.emptyList();`
`53`	`53`	`}`
`54`	`54`