refactor: isolate bulk dependent resource handling more (#1530)

Author: metacosm

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java

@@ -11,6 +11,7 @@
 import io.javaoperatorsdk.operator.api.reconciler.ResourceDiscriminator;
 import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource;
 import io.javaoperatorsdk.operator.api.reconciler.dependent.ReconcileResult;
+import io.javaoperatorsdk.operator.processing.dependent.Matcher.Result;
 import io.javaoperatorsdk.operator.processing.event.ResourceID;
 
 @Ignore
@@ -20,78 +21,50 @@ public abstract class AbstractDependentResource<R, P extends HasMetadata>
 
   private final boolean creatable = this instanceof Creator;
   private final boolean updatable = this instanceof Updater;
-  private final boolean bulk = this instanceof BulkDependentResource;
 
   protected Creator<R, P> creator;
   protected Updater<R, P> updater;
-  protected BulkDependentResource<R, P> bulkDependentResource;
   private ResourceDiscriminator<R, P> resourceDiscriminator;
+  private final DependentResourceReconciler<R, P> dependentResourceReconciler;
 
-  @SuppressWarnings({"unchecked", "rawtypes"})
+  @SuppressWarnings({"unchecked"})
   protected AbstractDependentResource() {
     creator = creatable ? (Creator<R, P>) this : null;
     updater = updatable ? (Updater<R, P>) this : null;
 
-    bulkDependentResource = bulk ? (BulkDependentResource) this : null;
+    dependentResourceReconciler = this instanceof BulkDependentResource
+        ? new BulkDependentResourceReconciler<>((BulkDependentResource<R, P>) this)
+        : new SingleDependentResourceReconciler<>(this);
   }
 
-
+  /**
+   * Overriding classes are strongly encouraged to call this implementation as part of their
+   * implementation, as they otherwise risk breaking functionality.
+   *
+   * @param primary the primary resource for which we want to reconcile the dependent state
+   * @param context {@link Context} providing useful contextual information
+   * @return the reconciliation result
+   */
   @Override
   public ReconcileResult<R> reconcile(P primary, Context<P> context) {
-    if (bulk) {
-      final var targetResources = bulkDependentResource.desiredResources(primary, context);
-
-      Map<String, R> actualResources =
-          bulkDependentResource.getSecondaryResources(primary, context);
-
-      deleteBulkResourcesIfRequired(targetResources.keySet(), actualResources, primary, context);
-      final List<ReconcileResult<R>> results = new ArrayList<>(targetResources.size());
-
-      targetResources.forEach((key, resource) -> {
-        results.add(reconcileIndexAware(primary, actualResources.get(key), resource, key, context));
-      });
-
-      return ReconcileResult.aggregatedResult(results);
-    } else {
-      var actualResource = getSecondaryResource(primary, context);
-      return reconcileIndexAware(primary, actualResource.orElse(null), null, null, context);
-    }
+    return dependentResourceReconciler.reconcile(primary, context);
   }
 
-  @SuppressWarnings({"rawtypes"})
-  protected void deleteBulkResourcesIfRequired(Set targetKeys, Map<String, R> actualResources,
-      P primary, Context<P> context) {
-    actualResources.forEach((key, value) -> {
-      if (!targetKeys.contains(key)) {
-        bulkDependentResource.deleteBulkResource(primary, value, key, context);
-      }
-    });
-  }
-
-  protected ReconcileResult<R> reconcileIndexAware(P primary, R actualResource, R desiredResource,
-      String key,
-      Context<P> context) {
+  protected ReconcileResult<R> reconcile(P primary, R actualResource, Context<P> context) {
     if (creatable || updatable) {
       if (actualResource == null) {
         if (creatable) {
-          var desired = bulkAwareDesired(primary, desiredResource, context);
+          var desired = desired(primary, context);
           throwIfNull(desired, primary, "Desired");
           logForOperation("Creating", primary, desired);
           var createdResource = handleCreate(desired, primary, context);
           return ReconcileResult.resourceCreated(createdResource);
         }
       } else {
         if (updatable) {
-          final Matcher.Result<R> match;
-          if (bulk) {
-            match =
-                bulkDependentResource.match(actualResource, desiredResource, primary, key, context);
-          } else {
-            match = updater.match(actualResource, primary, context);
-          }
+          final Matcher.Result<R> match = match(actualResource, primary, context);
           if (!match.matched()) {
-            final var desired =
-                match.computedDesired().orElse(bulkAwareDesired(primary, desiredResource, context));
+            final var desired = match.computedDesired().orElse(desired(primary, context));
             throwIfNull(desired, primary, "Desired");
             logForOperation("Updating", primary, desired);
             var updatedResource = handleUpdate(actualResource, desired, primary, context);
@@ -110,9 +83,8 @@ protected ReconcileResult<R> reconcileIndexAware(P primary, R actualResource, R
     return ReconcileResult.noOperation(actualResource);
   }
 
-  private R bulkAwareDesired(P primary, R alreadyComputedDesire, Context<P> context) {
-    return bulk ? alreadyComputedDesire
-        : desired(primary, context);
+  public Result<R> match(R resource, P primary, Context<P> context) {
+    return updater.match(resource, primary, context);
   }
 
   @Override
@@ -179,12 +151,7 @@ protected R desired(P primary, Context<P> context) {
   }
 
   public void delete(P primary, Context<P> context) {
-    if (bulk) {
-      var actualResources = bulkDependentResource.getSecondaryResources(primary, context);
-      deleteBulkResourcesIfRequired(Collections.emptySet(), actualResources, primary, context);
-    } else {
-      handleDelete(primary, context);
-    }
+    dependentResourceReconciler.delete(primary, context);
   }
 
   protected void handleDelete(P primary, Context<P> context) {
@@ -200,11 +167,8 @@ protected boolean isCreatable() {
     return creatable;
   }
 
+  @SuppressWarnings("unused")
   protected boolean isUpdatable() {
     return updatable;
   }
-
-  protected boolean isBulk() {
-    return bulk;
-  }
 }

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/BulkDependentResource.java

@@ -17,10 +17,27 @@ public interface BulkDependentResource<R, P extends HasMetadata>
     extends Creator<R, P>, Deleter<P> {
 
   /**
-   * @return number of resources to create
+   * Retrieves a map of desired secondary resources associated with the specified primary resource,
+   * identified by an arbitrary key.
+   *
+   * @param primary the primary resource with which we want to identify which secondary resources
+   *        are associated
+   * @param context the {@link Context} associated with the current reconciliation
+   * @return a Map associating desired secondary resources with the specified primary via arbitrary
+   *         identifiers
    */
   Map<String, R> desiredResources(P primary, Context<P> context);
 
+  /**
+   * Retrieves the actual secondary resources currently existing on the server and associated with
+   * the specified primary resource.
+   *
+   * @param primary the primary resource for which we want to retrieve the associated secondary
+   *        resources
+   * @param context the {@link Context} associated with the current reconciliation
+   * @return a Map associating actual secondary resources with the specified primary via arbitrary
+   *         identifiers
+   */
   Map<String, R> getSecondaryResources(P primary, Context<P> context);
 
   /**
@@ -39,14 +56,15 @@ public interface BulkDependentResource<R, P extends HasMetadata>
    * {@link Context}.
    *
    * @param actualResource the resource we want to determine whether it's matching the desired state
+   * @param desired the resource's desired state
    * @param primary the primary resource from which the desired state is inferred
-   * @param key key of the resource
    * @param context the context in which the resource is being matched
    * @return a {@link Result} encapsulating whether the resource matched its desired state and this
    *         associated state if it was computed as part of the matching process. Use the static
    *         convenience methods ({@link Result#nonComputed(boolean)} and
    *         {@link Result#computed(boolean, Object)})
    */
-  Result<R> match(R actualResource, R desired, P primary, String key, Context<P> context);
-
+  default Result<R> match(R actualResource, R desired, P primary, Context<P> context) {
+    return Matcher.Result.computed(desired.equals(actualResource), desired);
+  }
 }

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/BulkDependentResourceReconciler.java

@@ -0,0 +1,135 @@
+package io.javaoperatorsdk.operator.processing.dependent;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import io.fabric8.kubernetes.api.model.HasMetadata;
+import io.javaoperatorsdk.operator.api.reconciler.Context;
+import io.javaoperatorsdk.operator.api.reconciler.dependent.Deleter;
+import io.javaoperatorsdk.operator.api.reconciler.dependent.ReconcileResult;
+import io.javaoperatorsdk.operator.processing.dependent.Matcher.Result;
+import io.javaoperatorsdk.operator.processing.event.ResourceID;
+
+class BulkDependentResourceReconciler<R, P extends HasMetadata>
+    implements DependentResourceReconciler<R, P> {
+
+  private final BulkDependentResource<R, P> bulkDependentResource;
+
+  BulkDependentResourceReconciler(BulkDependentResource<R, P> bulkDependentResource) {
+    this.bulkDependentResource = bulkDependentResource;
+  }
+
+  @Override
+  public ReconcileResult<R> reconcile(P primary, Context<P> context) {
+    final var desiredResources = bulkDependentResource.desiredResources(primary, context);
+    Map<String, R> actualResources = bulkDependentResource.getSecondaryResources(primary, context);
+
+    // remove existing resources that are not needed anymore according to the primary state
+    deleteBulkResourcesIfRequired(desiredResources.keySet(), actualResources, primary, context);
+
+    final List<ReconcileResult<R>> results = new ArrayList<>(desiredResources.size());
+    final var updatable = bulkDependentResource instanceof Updater;
+    desiredResources.forEach((key, value) -> {
+      final var instance =
+          updatable ? new UpdatableBulkDependentResourceInstance<>(bulkDependentResource, value)
+              : new BulkDependentResourceInstance<>(bulkDependentResource, value);
+      results.add(instance.reconcile(primary, actualResources.get(key), context));
+    });
+
+    return ReconcileResult.aggregatedResult(results);
+  }
+
+  @Override
+  public void delete(P primary, Context<P> context) {
+    var actualResources = bulkDependentResource.getSecondaryResources(primary, context);
+    deleteBulkResourcesIfRequired(Collections.emptySet(), actualResources, primary, context);
+  }
+
+  private void deleteBulkResourcesIfRequired(Set<String> expectedKeys,
+      Map<String, R> actualResources, P primary, Context<P> context) {
+    actualResources.forEach((key, value) -> {
+      if (!expectedKeys.contains(key)) {
+        bulkDependentResource.deleteBulkResource(primary, value, key, context);
+      }
+    });
+  }
+
+  /**
+   * Exposes a dynamically-created instance of the bulk dependent resource precursor as an
+   * AbstractDependentResource so that we can reuse its reconciliation logic.
+   *
+   * @param <R>
+   * @param <P>
+   */
+  private static class BulkDependentResourceInstance<R, P extends HasMetadata>
+      extends AbstractDependentResource<R, P>
+      implements Creator<R, P>, Deleter<P> {
+    private final BulkDependentResource<R, P> bulkDependentResource;
+    private final R desired;
+
+    private BulkDependentResourceInstance(BulkDependentResource<R, P> bulkDependentResource,
+        R desired) {
+      this.bulkDependentResource = bulkDependentResource;
+      this.desired = desired;
+    }
+
+    @SuppressWarnings("unchecked")
+    private AbstractDependentResource<R, P> asAbstractDependentResource() {
+      return (AbstractDependentResource<R, P>) bulkDependentResource;
+    }
+
+    @Override
+    protected R desired(P primary, Context<P> context) {
+      return desired;
+    }
+
+    @SuppressWarnings("unchecked")
+    public R update(R actual, R desired, P primary, Context<P> context) {
+      return ((Updater<R, P>) bulkDependentResource).update(actual, desired, primary, context);
+    }
+
+    @Override
+    public Result<R> match(R resource, P primary, Context<P> context) {
+      return bulkDependentResource.match(resource, desired, primary, context);
+    }
+
+    @Override
+    protected void onCreated(ResourceID primaryResourceId, R created) {
+      asAbstractDependentResource().onCreated(primaryResourceId, created);
+    }
+
+    @Override
+    protected void onUpdated(ResourceID primaryResourceId, R updated, R actual) {
+      asAbstractDependentResource().onUpdated(primaryResourceId, updated, actual);
+    }
+
+    @Override
+    public Class<R> resourceType() {
+      return asAbstractDependentResource().resourceType();
+    }
+
+    @Override
+    public R create(R desired, P primary, Context<P> context) {
+      return bulkDependentResource.create(desired, primary, context);
+    }
+  }
+
+  /**
+   * Makes sure that the instance implements Updater if its precursor does as well.
+   *
+   * @param <R>
+   * @param <P>
+   */
+  private static class UpdatableBulkDependentResourceInstance<R, P extends HasMetadata>
+      extends BulkDependentResourceInstance<R, P> implements Updater<R, P> {
+
+    private UpdatableBulkDependentResourceInstance(
+        BulkDependentResource<R, P> bulkDependentResource,
+        R desired) {
+      super(bulkDependentResource, desired);
+    }
+  }
+}

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/DependentResourceReconciler.java

@@ -0,0 +1,20 @@
+package io.javaoperatorsdk.operator.processing.dependent;
+
+import io.fabric8.kubernetes.api.model.HasMetadata;
+import io.javaoperatorsdk.operator.api.reconciler.Context;
+import io.javaoperatorsdk.operator.api.reconciler.dependent.ReconcileResult;
+
+/**
+ * An internal interface that abstracts away how to reconcile dependent resources, in particular
+ * when they can be dynamically created based on the state provided by the primary resource (e.g.
+ * the primary resource dictates which/how many secondary resources need to be created).
+ *
+ * @param <R> the type of the secondary resource to be reconciled
+ * @param <P> the primary resource type
+ */
+interface DependentResourceReconciler<R, P extends HasMetadata> {
+
+  ReconcileResult<R> reconcile(P primary, Context<P> context);
+
+  void delete(P primary, Context<P> context);
+}

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/SingleDependentResourceReconciler.java

@@ -0,0 +1,26 @@
+package io.javaoperatorsdk.operator.processing.dependent;
+
+import io.fabric8.kubernetes.api.model.HasMetadata;
+import io.javaoperatorsdk.operator.api.reconciler.Context;
+import io.javaoperatorsdk.operator.api.reconciler.dependent.ReconcileResult;
+
+class SingleDependentResourceReconciler<R, P extends HasMetadata>
+    implements DependentResourceReconciler<R, P> {
+
+  private final AbstractDependentResource<R, P> instance;
+
+  SingleDependentResourceReconciler(AbstractDependentResource<R, P> dependentResource) {
+    this.instance = dependentResource;
+  }
+
+  @Override
+  public ReconcileResult<R> reconcile(P primary, Context<P> context) {
+    final var maybeActual = instance.getSecondaryResource(primary, context);
+    return instance.reconcile(primary, maybeActual.orElse(null), context);
+  }
+
+  @Override
+  public void delete(P primary, Context<P> context) {
+    instance.handleDelete(primary, context);
+  }
+}

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java

@@ -139,7 +139,8 @@ public Result<R> match(R actualResource, P primary, Context<P> context) {
     return matcher.match(actualResource, primary, context);
   }
 
-  public Result<R> match(R actualResource, R desired, P primary, String key, Context<P> context) {
+  @SuppressWarnings("unused")
+  public Result<R> match(R actualResource, R desired, P primary, Context<P> context) {
     return GenericKubernetesResourceMatcher.match(desired, actualResource, false);
   }
 
@@ -148,6 +149,7 @@ protected void handleDelete(P primary, Context<P> context) {
     resource.ifPresent(r -> client.resource(r).delete());
   }
 
+  @SuppressWarnings("unused")
   public void deleteBulkResource(P primary, R resource, String key, Context<P> context) {
     client.resource(resource).delete();
   }