wip

Signed-off-by: Attila Mészáros <a_meszaros@apple.com>

Author: csviri

docs/content/en/blog/releases/v5-5-release.md

@@ -43,8 +43,8 @@ operation type; when one does not fit, supply your own via `Options.matchAndFilt
 
 - `matchAndFilter(...)` / `matchAndFilterWithDefaultMatcher(...)` — match, then write-and-filter only
   if needed (the default).
-- `filterIfOptimisticLocking()` — filter the own event only when the write uses optimistic locking,
-  otherwise just cache the response.
+- `filterWithOptimisticLocking()` — filter the own event; the write must use optimistic locking,
+  otherwise an `IllegalArgumentException` is thrown.
 - `cacheOnly()` — only cache the response (read-cache-after-write consistency), no filtering.
 - `forceFilterEvents()` — always filter (mostly internal usage).
 

docs/content/en/docs/documentation/reconciler.md

@@ -212,8 +212,10 @@ third-party write. This requires **either**:
   avoids a request to the API server when nothing changed. Default matchers are provided for every
   operation type, but they are heuristics — a workflow relying on them should be tested against the
   concrete resources it manages, or a custom matcher supplied.
-- `Options.filterIfOptimisticLocking()` — filter the own event only when the write uses optimistic
-  locking, otherwise just cache the response.
+- `Options.filterWithOptimisticLocking()` — filter the own event; the write must use optimistic
+  locking (a resource version set on the written resource), otherwise an `IllegalArgumentException`
+  is thrown. Requiring optimistic locking guarantees a concurrent third-party change is rejected by
+  the API server rather than being silently filtered out.
 - `Options.cacheOnly()` — only cache the response (read-cache-after-write consistency), no own-event
   filtering.
 - `Options.forceFilterEvents()` — always filter, regardless of optimistic locking. Only safe when

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ResourceOperations.java

@@ -50,10 +50,11 @@
  * selected through {@link Mode}:
  *
  * <ul>
- *   <li>{@link Options#filterIfOptimisticLocking()} ({@link Mode#FILTER_IF_OPTIMISTIC_LOCKING}) -
- *       the own event is filtered only when the write uses optimistic locking (i.e. the resource
- *       version is set on the resource being written); otherwise the response is only cached. This
- *       is the safe default for plain update/patch operations.
+ *   <li>{@link Options#filterWithOptimisticLocking()} ({@link Mode#FILTER_WITH_OPTIMISTIC_LOCKING})
+ *       - the own event is filtered; the write must use optimistic locking (i.e. a resource version
+ *       is set on the resource being written), otherwise an {@link IllegalArgumentException} is
+ *       thrown. Requiring optimistic locking guarantees that a concurrent third-party change is
+ *       rejected by the API server rather than silently filtered out.
  *   <li>{@link Options#matchAndFilter(Matcher)} / {@link
  *       Options#matchAndFilterWithDefaultMatcher(io.javaoperatorsdk.operator.processing.matcher.UpdateType)}
  *       ({@link Mode#FILTER_IF_NOT_MATCHING}) - before writing, the desired state is compared to
@@ -860,7 +861,7 @@ public P jsonMergePatchPrimaryStatus(P resource, Options options) {
    * Low-level building block behind all the update/patch operations above: runs the given {@code
    * updateOperation} against the API server, then caches the response and (depending on the {@link
    * Options}) filters the resulting own event. The target event source is resolved automatically
-   * from the desired resource's type. Uses {@link Options#filterIfOptimisticLocking()}.
+   * from the desired resource's type. Uses {@link Options#filterWithOptimisticLocking()}.
    *
    * @param resource the desired resource being written
    * @param updateOperation the actual write to perform (update, patch, edit, ...) returning the
@@ -871,7 +872,7 @@ public P jsonMergePatchPrimaryStatus(P resource, Options options) {
    *     type
    */
   public <R extends HasMetadata> R resourcePatch(R resource, UnaryOperator<R> updateOperation) {
-    return resourcePatch(resource, updateOperation, Options.filterIfOptimisticLocking());
+    return resourcePatch(resource, updateOperation, Options.filterWithOptimisticLocking());
   }
 
   @SuppressWarnings({"rawtypes", "unchecked"})
@@ -917,7 +918,7 @@ private <R extends HasMetadata> R resourcePatch(
   @Deprecated(forRemoval = true)
   public <R extends HasMetadata> R resourcePatch(
       R desired, UnaryOperator<R> updateOperation, ManagedInformerEventSource<R, P, ?> ies) {
-    return resourcePatch(desired, updateOperation, ies, Options.filterIfOptimisticLocking());
+    return resourcePatch(desired, updateOperation, ies, Options.filterWithOptimisticLocking());
   }
 
   private <R extends HasMetadata> R resourcePatch(
@@ -947,7 +948,9 @@ private <R extends HasMetadata> R resourcePatch(
    * @param <R> the resource type
    * @return the resource as returned by {@code updateOperation}, or {@code actualResource} when the
    *     desired state already matches
-   * @throws IllegalArgumentException if the mode requires a matcher but none is provided
+   * @throws IllegalArgumentException if the mode requires a matcher but none is provided, or if the
+   *     mode is {@link Mode#FILTER_WITH_OPTIMISTIC_LOCKING} but the resource being written has no
+   *     resource version set
    */
   // visible for testing
   <R extends HasMetadata> R resourcePatch(
@@ -982,10 +985,16 @@ <R extends HasMetadata> R resourcePatch(
       return actualResource;
     }
 
-    boolean optimisticLocking = desiredResource.getMetadata().getResourceVersion() != null;
+    if (options.getMode() == Mode.FILTER_WITH_OPTIMISTIC_LOCKING
+        && desiredResource.getMetadata().getResourceVersion() == null) {
+      throw new IllegalArgumentException(
+          "Mode "
+              + Mode.FILTER_WITH_OPTIMISTIC_LOCKING
+              + " requires optimistic locking, but no resource version is set on the resource: "
+              + ResourceID.fromResource(desiredResource));
+    }
 
-    if (options.getMode() == Mode.CACHE_ONLY
-        || (options.getMode() == Mode.FILTER_IF_OPTIMISTIC_LOCKING && !optimisticLocking)) {
+    if (options.getMode() == Mode.CACHE_ONLY) {
       return ies.updateAndCacheResource(desiredResource, updateOperation);
     } else {
       return ies.eventFilteringUpdateAndCacheResource(desiredResource, updateOperation);
@@ -1250,8 +1259,8 @@ public P addFinalizerWithSSA(String finalizerName) {
    * is needed for safe own-event filtering), and the trade-offs of the default matchers.
    *
    * <ul>
-   *   <li>{@link #filterIfOptimisticLocking()} - filter the own event only when the write uses
-   *       optimistic locking, otherwise only cache the response.
+   *   <li>{@link #filterWithOptimisticLocking()} - filter the own event; requires the write to use
+   *       optimistic locking (a resource version set), throwing if none is set.
    *   <li>{@link #matchAndFilter(Matcher)} / {@link #matchAndFilterWithDefaultMatcher(UpdateType)}
    *       - skip the write when the desired state already matches the actual state, otherwise write
    *       and filter the own event.
@@ -1265,8 +1274,8 @@ public static class Options {
 
     private static final Options ALWAYS_FILTER = new Options(Mode.FORCE_FILTER, null);
     private static final Options ONLY_CACHE = new Options(Mode.CACHE_ONLY, null);
-    private static final Options FILTER_IF_OPTIMISTIC_LOCKING =
-        new Options(Mode.FILTER_IF_OPTIMISTIC_LOCKING, null);
+    private static final Options FILTER_WITH_OPTIMISTIC_LOCKING =
+        new Options(Mode.FILTER_WITH_OPTIMISTIC_LOCKING, null);
 
     private final Mode mode;
     private final Matcher matcher;
@@ -1310,14 +1319,31 @@ public static Options cacheOnly(Matcher matcher) {
     }
 
     /**
-     * Filters the own event only when the write uses optimistic locking (a resource version is set
-     * on the written resource); otherwise only caches the response. This is the safe default for
-     * plain update/patch operations.
+     * Filters the own event resulting from the write, requiring the write to use optimistic locking
+     * (a resource version set on the written resource). If no resource version is set an {@link
+     * IllegalArgumentException} is thrown when the operation is performed. Requiring optimistic
+     * locking guarantees that a concurrent third-party change is rejected by the API server rather
+     * than being silently filtered out.
      *
-     * @return options that filter the own event only when optimistic locking is used
+     * @return options that filter the own event, requiring optimistic locking
+     */
+    public static Options filterWithOptimisticLocking() {
+      return FILTER_WITH_OPTIMISTIC_LOCKING;
+    }
+
+    /**
+     * Like {@link #filterWithOptimisticLocking()} but additionally skips the write when the desired
+     * state already matches the actual (cached) state according to the given {@link Matcher}. When
+     * a write is needed it still requires optimistic locking (see {@link
+     * #filterWithOptimisticLocking()}).
+     *
+     * @param matcher the matcher used to decide whether the actual state already matches the
+     *     desired
+     * @return options that match using the given matcher and, when a write is needed, filter the
+     *     own event requiring optimistic locking
      */
-    public static Options filterIfOptimisticLocking() {
-      return FILTER_IF_OPTIMISTIC_LOCKING;
+    public static Options filterWithOptimisticLocking(Matcher matcher) {
+      return new Options(Mode.FILTER_WITH_OPTIMISTIC_LOCKING, matcher);
     }
 
     /**
@@ -1362,14 +1388,14 @@ public boolean requiresMatcher() {
   /**
    * The strategy used to decide how the own event resulting from a write is handled. This is a
    * low-level enum; users should not reference it directly but select the desired behavior through
-   * the {@link Options} factory methods (e.g. {@link Options#filterIfOptimisticLocking()}, {@link
+   * the {@link Options} factory methods (e.g. {@link Options#filterWithOptimisticLocking()}, {@link
    * Options#matchAndFilter(Matcher)}, {@link Options#cacheOnly()}, {@link
    * Options#forceFilterEvents()}), which is why each constant links to its corresponding factory.
    */
   @Experimental(API_MIGHT_CHANGE)
   enum Mode {
-    /** See {@link Options#filterIfOptimisticLocking()}. */
-    FILTER_IF_OPTIMISTIC_LOCKING,
+    /** See {@link Options#filterWithOptimisticLocking()}. */
+    FILTER_WITH_OPTIMISTIC_LOCKING,
     /** See {@link Options#matchAndFilter(Matcher)}. */
     FILTER_IF_NOT_MATCHING,
     /** See {@link Options#cacheOnly()}. */

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

@@ -21,10 +21,8 @@
 
 import io.fabric8.kubernetes.api.model.HasMetadata;
 import io.javaoperatorsdk.operator.api.reconciler.Context;
-import io.javaoperatorsdk.operator.api.reconciler.dependent.RecentOperationCacheFiller;
 import io.javaoperatorsdk.operator.processing.ResourceIDMapper;
 import io.javaoperatorsdk.operator.processing.event.EventSourceRetriever;
-import io.javaoperatorsdk.operator.processing.event.ResourceID;
 import io.javaoperatorsdk.operator.processing.event.source.EventSource;
 import io.javaoperatorsdk.operator.processing.event.source.informer.InformerEventSource;
 
@@ -96,11 +94,7 @@ private void handleExplicitStateDelete(P primary, R secondary, Context<P> contex
   @SuppressWarnings({"rawtypes", "unchecked", "unused"})
   protected void handleExplicitStateCreation(P primary, R created, Context<P> context) {
     var resource = dependentResourceWithExplicitState.stateResource(primary, created);
-    var stateResource = context.getClient().resource(resource).create();
-    if (externalStateEventSource != null) {
-      ((RecentOperationCacheFiller) externalStateEventSource)
-          .handleRecentResourceCreate(ResourceID.fromResource(primary), stateResource);
-    }
+    context.resourceOperations().create(resource);
   }
 
   @Override

operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/reconciler/ResourceOperationsTest.java

@@ -408,25 +408,7 @@ void onlyCacheModeSkipsEventFiltering() {
   }
 
   @Test
-  void filterIfOptimisticLockingCachesWithoutFilteringWhenNoResourceVersion() {
-    var desired = TestUtils.testCustomResource1();
-    desired.getMetadata().setResourceVersion(null);
-    var ies = mock(ManagedInformerEventSource.class);
-    when(ies.updateAndCacheResource(any(), any(UnaryOperator.class))).thenReturn(desired);
-
-    resourceOperations.resourcePatch(
-        desired,
-        null,
-        UnaryOperator.identity(),
-        ies,
-        ResourceOperations.Options.filterIfOptimisticLocking());
-
-    verify(ies, times(1)).updateAndCacheResource(eq(desired), any(UnaryOperator.class));
-    verify(ies, never()).eventFilteringUpdateAndCacheResource(any(), any(UnaryOperator.class));
-  }
-
-  @Test
-  void filterIfOptimisticLockingFiltersWhenResourceVersionPresent() {
+  void filterWithOptimisticLockingFiltersWhenResourceVersionPresent() {
     var desired = TestUtils.testCustomResource1();
     desired.getMetadata().setResourceVersion("1");
     var ies = mock(ManagedInformerEventSource.class);
@@ -438,7 +420,7 @@ void filterIfOptimisticLockingFiltersWhenResourceVersionPresent() {
         null,
         UnaryOperator.identity(),
         ies,
-        ResourceOperations.Options.filterIfOptimisticLocking());
+        ResourceOperations.Options.filterWithOptimisticLocking());
 
     verify(ies, times(1))
         .eventFilteringUpdateAndCacheResource(eq(desired), any(UnaryOperator.class));
@@ -532,32 +514,18 @@ void updateForceFilterFiltersAndCallsClientUpdate() {
   }
 
   @Test
-  void updateFilterIfOptimisticLockingFiltersWhenResourceVersionPresent() {
+  void updateFilterWithOptimisticLockingFiltersWhenResourceVersionPresent() {
     var resource = TestUtils.testCustomResource1();
     resource.getMetadata().setResourceVersion("1");
     wireVerbMocks();
 
-    resourceOperations.update(resource, ResourceOperations.Options.filterIfOptimisticLocking());
+    resourceOperations.update(resource, ResourceOperations.Options.filterWithOptimisticLocking());
 
     verify(verbEventSource, times(1))
         .eventFilteringUpdateAndCacheResource(eq(resource), any(UnaryOperator.class));
     verify(verbEventSource, never()).updateAndCacheResource(any(), any(UnaryOperator.class));
   }
 
-  @Test
-  void updateFilterIfOptimisticLockingCachesWhenNoResourceVersion() {
-    var resource = TestUtils.testCustomResource1();
-    resource.getMetadata().setResourceVersion(null);
-    wireVerbMocks();
-
-    resourceOperations.update(resource, ResourceOperations.Options.filterIfOptimisticLocking());
-
-    verify(verbEventSource, times(1))
-        .updateAndCacheResource(eq(resource), any(UnaryOperator.class));
-    verify(verbEventSource, never())
-        .eventFilteringUpdateAndCacheResource(any(), any(UnaryOperator.class));
-  }
-
   @Test
   void updateWithMatcherMatchingSkipsWrite() {
     var desired = TestUtils.testCustomResource1();

operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/resourceoperations/SecondaryResourceOperationsReconciler.java

@@ -101,7 +101,7 @@ public UpdateControl<SecondaryResourceOperationsCustomResource> reconcile(
       }
       case UPDATE -> {
         actual.getData().put(DATA_KEY, APPLIED_VALUE);
-        ops.update(actual, configMapEventSource, Options.filterIfOptimisticLocking());
+        ops.update(actual, configMapEventSource, Options.filterWithOptimisticLocking());
       }
       case JSON_PATCH ->
           ops.jsonPatch(
@@ -111,10 +111,10 @@ public UpdateControl<SecondaryResourceOperationsCustomResource> reconcile(
                 return cm;
               },
               configMapEventSource,
-              Options.filterIfOptimisticLocking());
+              Options.filterWithOptimisticLocking());
       case JSON_MERGE_PATCH -> {
         var desired = desiredConfigMap(resource, APPLIED_VALUE);
-        ops.jsonMergePatch(desired, configMapEventSource, Options.filterIfOptimisticLocking());
+        ops.jsonMergePatch(desired, configMapEventSource, Options.filterWithOptimisticLocking());
       }
       default -> throw new IllegalStateException("Unexpected operation: " + operation);
     }