feat: distinguish resources based on desired state (#2252)

Signed-off-by: Attila Mészáros <[email protected]>

Author: csviri

Diff for: docs/documentation/dependent-resources.md

@@ -301,20 +301,31 @@ tests [here](https://github.com/java-operator-sdk/java-operator-sdk/blob/main/op
 
 When dealing with multiple dependent resources of same type, the dependent resource implementation
 needs to know which specific resource should be targeted when reconciling a given dependent
-resource, since there will be multiple instances of that type which could possibly be used, each
-associated with the same primary resource. In order to do this, JOSDK relies on the
-[resource discriminator](https://github.com/java-operator-sdk/java-operator-sdk/blob/main/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ResourceDiscriminator.java)
-concept. Resource discriminators uniquely identify the target resource of a dependent resource.
-In the managed Kubernetes dependent resources case, the discriminator can be declaratively set
-using the `@KubernetesDependent` annotation:
-
-```java
-
-@KubernetesDependent(resourceDiscriminator = ConfigMap1Discriminator.class)
-public class MultipleManagedDependentResourceConfigMap1 {
-//...
-}
-```
+resource, since there could be multiple instances of that type which could possibly be used, each
+associated with the same primary resource. In this situation, JOSDK automatically selects the appropriate secondary
+resource matching the desired state associated with the primary resource. This makes sense because the desired
+state computation already needs to be able to discriminate among multiple related secondary resources to tell JOSDK how
+they should be reconciled.
+
+There might be casees, though, where it might be problematic to call the `desired` method several times (for example, because it is costly to do so), it is always possible to override this automated discrimination using several means:
+
+- Implement your own `getSecondaryResource` method on your `DependentResource` implementation from scratch.
+- Override the `selectManagedSecondaryResource` method, if your `DependentResource` extends `AbstractDependentResource`.
+  This should be relatively simple to override this method to optimize the matching to your needs. You can see an
+  example of such an implementation in
+  the [`ExternalWithStateDependentResource`](https://github.com/operator-framework/java-operator-sdk/blob/6cd0f884a7c9b60c81bd2d52da54adbd64d6e118/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/externalstate/ExternalWithStateDependentResource.java#L43-L49)
+  class.
+- Override the `managedSecondaryResourceID` method, if your `DependentResource` extends `KubernetesDependentResource`,
+  where it's very often possible to easily determine the `ResourceID` of the secondary resource. This would probably be
+  the easiest solution if you're working with Kubernetes resources.
+- Configure
+  a [`ResourceDiscriminator`](https://github.com/java-operator-sdk/java-operator-sdk/blob/main/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ResourceDiscriminator.java)
+  implementation for your `DependentResource`. This was the approach that was used before JOSDK v5 but should not be
+  needed anymore as it is simpler and more efficient to override one the methods above instead of creating a separate
+  class. Discriminators can be declaratively set when using managed Kubernetes dependent resources via
+  the `resourceDiscriminator` field of the `@KubernetesDependent` annotation.
+
+### Sharing an Event Source Between Dependent Resources
 
 Dependent resources usually also provide event sources. When dealing with multiple dependents of
 the same type, one needs to decide whether these dependent resources should track the same
@@ -330,10 +341,10 @@ would look as follows:
    useEventSourceWithName = "configMapSource")
 ```
 
-A sample is provided as an integration test both
-for [managed](https://github.com/java-operator-sdk/java-operator-sdk/blob/main/operator-framework/src/test/java/io/javaoperatorsdk/operator/MultipleManagedDependentSameTypeIT.java)
-and
-for [standalone](https://github.com/java-operator-sdk/java-operator-sdk/blob/main/operator-framework/src/test/java/io/javaoperatorsdk/operator/MultipleDependentResourceIT.java)
+A sample is provided as an integration test both:
+for [managed](https://github.com/operator-framework/java-operator-sdk/blob/main/operator-framework/src/test/java/io/javaoperatorsdk/operator/MultipleManagedDependentNoDiscriminatorIT.java)
+
+For [standalone](https://github.com/operator-framework/java-operator-sdk/blob/main/operator-framework/src/test/java/io/javaoperatorsdk/operator/MultipleDependentResourceIT.java)
 cases.
 
 ## Bulk Dependent Resources

Diff for: operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/DependentResource.java

@@ -49,6 +49,16 @@ default Optional<? extends ResourceEventSource<R, P>> eventSource(
     return Optional.empty();
   }
 
+  /**
+   * Retrieves the secondary resource (if it exists) associated with the specified primary resource
+   * for this DependentResource.
+   *
+   * @param primary the primary resource for which we want to retrieve the secondary resource
+   *        associated with this DependentResource
+   * @param context the current {@link Context} in which the operation is called
+   * @return the secondary resource or {@link Optional#empty()} if it doesn't exist
+   * @throws IllegalStateException if more than one secondary is found to match the primary resource
+   */
   default Optional<R> getSecondaryResource(P primary, Context<P> context) {
     return Optional.empty();
   }

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

@@ -1,6 +1,7 @@
 package io.javaoperatorsdk.operator.processing.dependent;
 
 import java.util.Optional;
+import java.util.Set;
 
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -97,8 +98,39 @@ protected ReconcileResult<R> reconcile(P primary, R actualResource, Context<P> c
 
   @Override
   public Optional<R> getSecondaryResource(P primary, Context<P> context) {
-    return resourceDiscriminator == null ? context.getSecondaryResource(resourceType())
-        : resourceDiscriminator.distinguish(resourceType(), primary, context);
+    if (resourceDiscriminator != null) {
+      return resourceDiscriminator.distinguish(resourceType(), primary, context);
+    } else {
+      var secondaryResources = context.getSecondaryResources(resourceType());
+      if (secondaryResources.isEmpty()) {
+        return Optional.empty();
+      } else {
+        return selectManagedSecondaryResource(secondaryResources, primary, context);
+      }
+    }
+  }
+
+  /**
+   * Selects the actual secondary resource matching the desired state derived from the primary
+   * resource when several resources of the same type are found in the context. This method allows
+   * for optimized implementations in subclasses since this default implementation will check each
+   * secondary candidates for equality with the specified desired state, which might end up costly.
+   *
+   * @param secondaryResources to select the target resource from
+   *
+   * @return the matching secondary resource or {@link Optional#empty()} if none matches
+   * @throws IllegalStateException if more than one candidate is found, in which case some other
+   *         mechanism might be necessary to distinguish between candidate secondary resources
+   */
+  protected Optional<R> selectManagedSecondaryResource(Set<R> secondaryResources, P primary,
+      Context<P> context) {
+    R desired = desired(primary, context);
+    var targetResources = secondaryResources.stream().filter(r -> r.equals(desired)).toList();
+    if (targetResources.size() > 1) {
+      throw new IllegalStateException(
+          "More than one secondary resource related to primary: " + targetResources);
+    }
+    return targetResources.isEmpty() ? Optional.empty() : Optional.of(targetResources.get(0));
   }
 
   private void throwIfNull(R desired, P primary, String descriptor) {
@@ -166,8 +198,7 @@ protected void handleDelete(P primary, R secondary, Context<P> context) {
         "handleDelete method must be implemented if Deleter trait is supported");
   }
 
-  public void setResourceDiscriminator(
-      ResourceDiscriminator<R, P> resourceDiscriminator) {
+  public void setResourceDiscriminator(ResourceDiscriminator<R, P> resourceDiscriminator) {
     this.resourceDiscriminator = resourceDiscriminator;
   }
 

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

@@ -1,6 +1,7 @@
 package io.javaoperatorsdk.operator.processing.dependent.kubernetes;
 
 import java.util.Map;
+import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
 
@@ -294,6 +295,29 @@ protected void addSecondaryToPrimaryMapperAnnotations(R desired, P primary, Stri
     }
   }
 
+  @Override
+  protected Optional<R> selectManagedSecondaryResource(Set<R> secondaryResources, P primary,
+      Context<P> context) {
+    ResourceID managedResourceID = managedSecondaryResourceID(primary, context);
+    return secondaryResources.stream()
+        .filter(r -> r.getMetadata().getName().equals(managedResourceID.getName()) &&
+            Objects.equals(r.getMetadata().getNamespace(),
+                managedResourceID.getNamespace().orElse(null)))
+        .findFirst();
+  }
+
+  /**
+   * Override this method in order to optimize and not compute the desired when selecting the target
+   * secondary resource. Simply, a static ResourceID can be returned.
+   *
+   * @param primary resource
+   * @param context of current reconciliation
+   * @return id of the target managed resource
+   */
+  protected ResourceID managedSecondaryResourceID(P primary, Context<P> context) {
+    return ResourceID.fromResource(desired(primary, context));
+  }
+
   protected boolean addOwnerReference() {
     return garbageCollected;
   }

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

@@ -34,7 +34,7 @@ class ExternalStateBulkIT {
           .build();
 
   @Test
-  void reconcilesResourceWithPersistentState() throws InterruptedException {
+  void reconcilesResourceWithPersistentState() {
     var resource = operator.create(testResource());
     assertResources(resource, INITIAL_TEST_DATA, INITIAL_BULK_SIZE);
 

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

@@ -1,62 +1,80 @@
 package io.javaoperatorsdk.operator;
 
 import java.time.Duration;
-import java.util.stream.IntStream;
 
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.RegisterExtension;
 
 import io.fabric8.kubernetes.api.model.ConfigMap;
 import io.fabric8.kubernetes.api.model.ObjectMetaBuilder;
 import io.javaoperatorsdk.operator.junit.LocallyRunOperatorExtension;
-import io.javaoperatorsdk.operator.sample.multipledependentresource.MultipleDependentResourceConfigMap;
 import io.javaoperatorsdk.operator.sample.multipledependentresource.MultipleDependentResourceCustomResource;
 import io.javaoperatorsdk.operator.sample.multipledependentresource.MultipleDependentResourceReconciler;
+import io.javaoperatorsdk.operator.sample.multipledependentresource.MultipleDependentResourceSpec;
+import io.javaoperatorsdk.operator.sample.multipledrsametypenodiscriminator.*;
 
+import static io.javaoperatorsdk.operator.sample.multipledependentresource.MultipleDependentResourceConfigMap.DATA_KEY;
+import static io.javaoperatorsdk.operator.sample.multipledependentresource.MultipleDependentResourceConfigMap.getConfigMapName;
+import static io.javaoperatorsdk.operator.sample.multipledependentresource.MultipleDependentResourceReconciler.FIRST_CONFIG_MAP_ID;
+import static io.javaoperatorsdk.operator.sample.multipledependentresource.MultipleDependentResourceReconciler.SECOND_CONFIG_MAP_ID;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.awaitility.Awaitility.await;
 
-class MultipleDependentResourceIT {
+public class MultipleDependentResourceIT {
+
+  public static final String CHANGED_VALUE = "changed value";
+  public static final String INITIAL_VALUE = "initial value";
 
-  public static final String TEST_RESOURCE_NAME = "multipledependentresource-testresource";
   @RegisterExtension
-  LocallyRunOperatorExtension operator =
+  LocallyRunOperatorExtension extension =
       LocallyRunOperatorExtension.builder()
-          .withReconciler(MultipleDependentResourceReconciler.class)
-          .waitForNamespaceDeletion(true)
+          .withReconciler(new MultipleDependentResourceReconciler())
           .build();
 
   @Test
-  void twoConfigMapsHaveBeenCreated() {
-    MultipleDependentResourceCustomResource customResource = createTestCustomResource();
-    operator.create(customResource);
-
-    var reconciler = operator.getReconcilerOfType(MultipleDependentResourceReconciler.class);
-
-    await().pollDelay(Duration.ofMillis(300))
-        .until(() -> reconciler.getNumberOfExecutions() <= 1);
-
-    IntStream.of(MultipleDependentResourceReconciler.FIRST_CONFIG_MAP_ID,
-        MultipleDependentResourceReconciler.SECOND_CONFIG_MAP_ID).forEach(configMapId -> {
-          ConfigMap configMap =
-              operator.get(ConfigMap.class, customResource.getConfigMapName(configMapId));
-          assertThat(configMap).isNotNull();
-          assertThat(configMap.getMetadata().getName())
-              .isEqualTo(customResource.getConfigMapName(configMapId));
-          assertThat(configMap.getData().get(MultipleDependentResourceConfigMap.DATA_KEY))
-              .isEqualTo(String.valueOf(configMapId));
-        });
-  }
+  void handlesCRUDOperations() {
+    var res = extension.create(testResource());
+
+    await().untilAsserted(() -> {
+      var cm1 = extension.get(ConfigMap.class, getConfigMapName(FIRST_CONFIG_MAP_ID));
+      var cm2 = extension.get(ConfigMap.class, getConfigMapName(SECOND_CONFIG_MAP_ID));
+
+      assertThat(cm1).isNotNull();
+      assertThat(cm2).isNotNull();
+      assertThat(cm1.getData()).containsEntry(DATA_KEY, INITIAL_VALUE);
+      assertThat(cm2.getData()).containsEntry(DATA_KEY, INITIAL_VALUE);
+    });
+
+    res.getSpec().setValue(CHANGED_VALUE);
+    res = extension.replace(res);
+
+    await().untilAsserted(() -> {
+      var cm1 = extension.get(ConfigMap.class, getConfigMapName(FIRST_CONFIG_MAP_ID));
+      var cm2 = extension.get(ConfigMap.class, getConfigMapName(SECOND_CONFIG_MAP_ID));
 
-  public MultipleDependentResourceCustomResource createTestCustomResource() {
-    MultipleDependentResourceCustomResource resource =
-        new MultipleDependentResourceCustomResource();
-    resource.setMetadata(
-        new ObjectMetaBuilder()
-            .withName(TEST_RESOURCE_NAME)
-            .withNamespace(operator.getNamespace())
-            .build());
-    return resource;
+      assertThat(cm1.getData()).containsEntry(DATA_KEY, CHANGED_VALUE);
+      assertThat(cm2.getData()).containsEntry(DATA_KEY, CHANGED_VALUE);
+    });
+
+    extension.delete(res);
+
+    await().timeout(Duration.ofSeconds(120)).untilAsserted(() -> {
+      var cm1 = extension.get(ConfigMap.class, getConfigMapName(FIRST_CONFIG_MAP_ID));
+      var cm2 = extension.get(ConfigMap.class, getConfigMapName(SECOND_CONFIG_MAP_ID));
+
+      assertThat(cm1).isNull();
+      assertThat(cm2).isNull();
+    });
   }
 
+  MultipleDependentResourceCustomResource testResource() {
+    var res = new MultipleDependentResourceCustomResource();
+    res.setMetadata(new ObjectMetaBuilder()
+        .withName("test1")
+        .build());
+    res.setSpec(new MultipleDependentResourceSpec());
+    res.getSpec().setValue(INITIAL_VALUE);
+
+    return res;
+  }
 }

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

@@ -0,0 +1,65 @@
+package io.javaoperatorsdk.operator;
+
+import java.time.Duration;
+import java.util.stream.IntStream;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
+
+import io.fabric8.kubernetes.api.model.ConfigMap;
+import io.fabric8.kubernetes.api.model.ObjectMetaBuilder;
+import io.javaoperatorsdk.operator.junit.LocallyRunOperatorExtension;
+import io.javaoperatorsdk.operator.sample.multipledependentresourcewithdiscriminator.MultipleDependentResourceConfigMap;
+import io.javaoperatorsdk.operator.sample.multipledependentresourcewithdiscriminator.MultipleDependentResourceCustomResourceWithDiscriminator;
+import io.javaoperatorsdk.operator.sample.multipledependentresourcewithdiscriminator.MultipleDependentResourceWithDiscriminatorReconciler;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.awaitility.Awaitility.await;
+
+class MultipleDependentResourceWithNoDiscriminatorIT {
+
+  public static final String TEST_RESOURCE_NAME = "multipledependentresource-testresource";
+  @RegisterExtension
+  LocallyRunOperatorExtension operator =
+      LocallyRunOperatorExtension.builder()
+          .withReconciler(MultipleDependentResourceWithDiscriminatorReconciler.class)
+          .waitForNamespaceDeletion(true)
+          .build();
+
+  @Test
+  void twoConfigMapsHaveBeenCreated() {
+    MultipleDependentResourceCustomResourceWithDiscriminator customResource =
+        createTestCustomResource();
+    operator.create(customResource);
+
+    var reconciler =
+        operator.getReconcilerOfType(MultipleDependentResourceWithDiscriminatorReconciler.class);
+
+    await().pollDelay(Duration.ofMillis(300))
+        .until(() -> reconciler.getNumberOfExecutions() <= 1);
+
+    IntStream.of(MultipleDependentResourceWithDiscriminatorReconciler.FIRST_CONFIG_MAP_ID,
+        MultipleDependentResourceWithDiscriminatorReconciler.SECOND_CONFIG_MAP_ID)
+        .forEach(configMapId -> {
+          ConfigMap configMap =
+              operator.get(ConfigMap.class, customResource.getConfigMapName(configMapId));
+          assertThat(configMap).isNotNull();
+          assertThat(configMap.getMetadata().getName())
+              .isEqualTo(customResource.getConfigMapName(configMapId));
+          assertThat(configMap.getData().get(MultipleDependentResourceConfigMap.DATA_KEY))
+              .isEqualTo(String.valueOf(configMapId));
+        });
+  }
+
+  public MultipleDependentResourceCustomResourceWithDiscriminator createTestCustomResource() {
+    MultipleDependentResourceCustomResourceWithDiscriminator resource =
+        new MultipleDependentResourceCustomResourceWithDiscriminator();
+    resource.setMetadata(
+        new ObjectMetaBuilder()
+            .withName(TEST_RESOURCE_NAME)
+            .withNamespace(operator.getNamespace())
+            .build());
+    return resource;
+  }
+
+}