improve: extend SecondaryToPrimaryMapper so it also gets the old resource

docs/content/en/docs/documentation/eventing.md

@@ -139,6 +139,25 @@ rare corner cases. Returning an empty set means that the mapper considered the s
 resource event as irrelevant and the SDK will thus not trigger a reconciliation of the primary
 resource in that situation.
 
+`SecondaryToPrimaryMapper` exposes two methods:
+
+- `toPrimaryResourceIDs(R resource)` — the original mapper. Implementing it is sufficient for
+  the vast majority of use cases.
+- `toPrimaryResourceIDs(R newResource, R oldResource)` — a variant that is the one actually
+  invoked by the SDK on every secondary event. Its default implementation delegates to the
+  single-argument method, so existing mappers keep working unchanged.
+
+Override the two-argument variant only in edge cases where the set of primary resources to
+reconcile depends on what changed between the previous and the new version of the secondary
+resource (e.g. a reference that moved from one primary to another, where both primaries need
+to be reconciled). **Use it with caution:** `oldResource` is sourced from the informer cache and
+is only populated for genuine update events observed while the controller is already running.
+On controller startup the cache is empty, so the initial events received for resources that
+already exist in the cluster are delivered as adds with `oldResource == null` — even if those
+resources had been updated before the operator came up. `oldResource` is also `null` for delete
+events and for events triggered through the primary-to-secondary index. Implementations must
+therefore handle a `null` `oldResource` gracefully.
+
 Adding a `SecondaryToPrimaryMapper` is typically sufficient when there is a one-to-many relationship
 between primary and secondary resources. The secondary resources can be mapped to its primary
 owner, and this is enough information to also get these secondary resources from the `Context`

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/SecondaryToPrimaryMapper.java

@@ -26,9 +26,49 @@
  */
 @FunctionalInterface
 public interface SecondaryToPrimaryMapper<R> {
+
   /**
-   * @param resource - secondary
-   * @return set of primary resource IDs
+   * Maps a secondary resource to the set of primary resources that should be reconciled in
+   * response. Implementing this single-argument form is sufficient for the vast majority of use
+   * cases — prefer it unless you specifically need access to the previous version of the secondary
+   * resource (see {@link #toPrimaryResourceIDs(Object, Object)}).
+   *
+   * @param resource the secondary resource for which an event was received
+   * @return set of primary resource IDs to enqueue for reconciliation; an empty set means the event
+   *     is irrelevant and no reconciliation is triggered
    */
   Set<ResourceID> toPrimaryResourceIDs(R resource);
+
+  /**
+   * Variant invoked by the framework for every secondary resource event, providing both the new and
+   * the previous version of the resource (when available). The default implementation simply
+   * delegates to {@link #toPrimaryResourceIDs(Object)} and ignores {@code oldResource}, so existing
+   * mappers keep working unchanged.
+   *
+   * <p>Override this method only for edge cases where the set of primary resources to reconcile
+   * depends on what changed between the old and the new version of the secondary resource (for
+   * example, when a reference held by the secondary resource has moved from one primary to another
+   * and both primaries need to be reconciled).
+   *
+   * <p><strong>Use with caution.</strong> {@code oldResource} is sourced from the informer cache
+   * and is therefore only populated for genuine update events observed while the controller is
+   * already running. In particular, when the controller starts up, the cache is empty and the
+   * initial events received for resources that already existed in the cluster are delivered as adds
+   * with {@code oldResource == null} (even if those resources had been updated previously). {@code
+   * oldResource} is also {@code null} for delete events and for events triggered through the
+   * primary-to-secondary index.
+   *
+   * <p>Implementations must therefore handle a {@code null} {@code oldResource} gracefully and not
+   * rely on it being present for correctness — overriding this method is intended for edge cases
+   * only. Genericly speaking controller should also handle such change checking during
+   * reconciliation, so when controller starts and event is missed it is properly reconiled.
+   *
+   * @param newResource the current version of the secondary resource
+   * @param oldResource the previous version of the secondary resource, or {@code null} if not
+   *     available (see above)
+   * @return set of primary resource IDs to enqueue for reconciliation
+   */
+  default Set<ResourceID> toPrimaryResourceIDs(R newResource, R oldResource) {
+    return toPrimaryResourceIDs(newResource);
+  }
 }

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/DefaultPrimaryToSecondaryIndex.java

@@ -33,7 +33,8 @@ public DefaultPrimaryToSecondaryIndex(SecondaryToPrimaryMapper<R> secondaryToPri
 
   @Override
   public synchronized void onAddOrUpdate(R resource) {
-    Set<ResourceID> primaryResources = secondaryToPrimaryMapper.toPrimaryResourceIDs(resource);
+    Set<ResourceID> primaryResources =
+        secondaryToPrimaryMapper.toPrimaryResourceIDs(resource, null);
     primaryResources.forEach(
         primaryResource -> {
           var resourceSet =
@@ -44,7 +45,8 @@ public synchronized void onAddOrUpdate(R resource) {
 
   @Override
   public synchronized void onDelete(R resource) {
-    Set<ResourceID> primaryResources = secondaryToPrimaryMapper.toPrimaryResourceIDs(resource);
+    Set<ResourceID> primaryResources =
+        secondaryToPrimaryMapper.toPrimaryResourceIDs(resource, null);
     primaryResources.forEach(
         primaryResource -> {
           var secondaryResources = index.get(primaryResource);

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSource.java

@@ -130,7 +130,7 @@ public synchronized void onDelete(R resource, boolean deletedFinalStateUnknown)
           primaryToSecondaryIndex.onDelete(resource);
           if (eventAcceptedByFilter(
               ResourceAction.DELETED, resource, null, deletedFinalStateUnknown)) {
-            propagateEvent(resource);
+            propagateEvent(resource, null);
           }
         });
   }
@@ -166,7 +166,7 @@ protected void handleEvent(
           action,
           resource.getMetadata().getResourceVersion());
     }
-    propagateEvent(resource);
+    propagateEvent(resource, oldResource);
   }
 
   @Override
@@ -194,15 +194,15 @@ private synchronized void onAddOrUpdate(ResourceAction action, R newObject, R ol
           "Propagating event for {}, resource with same version not result of a our update.",
           action);
       var event = resultEvent.get();
-      propagateEvent((R) event.getResource().orElseThrow());
+      propagateEvent((R) event.getResource().orElseThrow(), oldObject);
     } else {
       log.debug("Event filtered out for operation: {}, resourceID: {}", action, resourceID);
     }
   }
 
-  protected void propagateEvent(R object) {
+  void propagateEvent(R resource, R oldResource) {
     var primaryResourceIdSet =
-        configuration().getSecondaryToPrimaryMapper().toPrimaryResourceIDs(object);
+        configuration().getSecondaryToPrimaryMapper().toPrimaryResourceIDs(resource, oldResource);
     if (primaryResourceIdSet.isEmpty()) {
       return;
     }

operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java

@@ -85,7 +85,7 @@ void setup() {
     SecondaryToPrimaryMapper secondaryToPrimaryMapper = mock(SecondaryToPrimaryMapper.class);
     when(informerEventSourceConfiguration.getSecondaryToPrimaryMapper())
         .thenReturn(secondaryToPrimaryMapper);
-    when(secondaryToPrimaryMapper.toPrimaryResourceIDs(any()))
+    when(secondaryToPrimaryMapper.toPrimaryResourceIDs(any(), any()))
         .thenReturn(Set.of(ResourceID.fromResource(testDeployment())));
     when(informerEventSourceConfiguration.getInformerConfig()).thenReturn(informerConfig);
 
@@ -745,14 +745,16 @@ private void assertNoEventProduced() {
     await()
         .pollDelay(Duration.ofMillis(70))
         .timeout(Duration.ofMillis(150))
-        .untilAsserted(() -> verify(informerEventSource, never()).propagateEvent(any()));
+        .untilAsserted(() -> verify(informerEventSource, never()).propagateEvent(any(), any()));
   }
 
   private void expectPropagateEvent(Deployment newResourceVersion) {
     await()
         .atMost(Duration.ofSeconds(1))
         .untilAsserted(
-            () -> verify(informerEventSource, times(1)).propagateEvent(newResourceVersion));
+            () ->
+                verify(informerEventSource, times(1))
+                    .propagateEvent(eq(newResourceVersion), any()));
   }
 
   private void expectHandleUpdateEvent(int newResourceVersion, int oldResourceVersion) {

operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/MappersTest.java

@@ -40,7 +40,8 @@ void secondaryToPrimaryMapperFromOwnerReference() {
     var secondary = getConfigMap(primary);
     secondary.addOwnerReference(primary);
 
-    var res = Mappers.fromOwnerReferences(TestCustomResource.class).toPrimaryResourceIDs(secondary);
+    var res =
+        Mappers.fromOwnerReferences(TestCustomResource.class).toPrimaryResourceIDs(secondary, null);
 
     assertThat(res).contains(ResourceID.fromResource(primary));
   }
@@ -65,7 +66,7 @@ void secondaryToPrimaryMapperFromOwnerReferenceWhereGroupIdIsEmpty() {
             .build();
     secondary.addOwnerReference(primary);
 
-    var res = Mappers.fromOwnerReferences(ConfigMap.class).toPrimaryResourceIDs(secondary);
+    var res = Mappers.fromOwnerReferences(ConfigMap.class).toPrimaryResourceIDs(secondary, null);
 
     assertThat(res).contains(ResourceID.fromResource(primary));
   }
@@ -79,7 +80,7 @@ void secondaryToPrimaryMapperFromOwnerReferenceFiltersByType() {
 
     var res =
         Mappers.fromOwnerReferences(TestCustomResourceOtherV1.class)
-            .toPrimaryResourceIDs(secondary);
+            .toPrimaryResourceIDs(secondary, null);
 
     assertThat(res).isEmpty();
   }
@@ -103,7 +104,7 @@ void fromOwnerReferenceIgnoresVersionFromApiVersion() {
                 HasMetadata.getGroup(TestCustomResource.class) + "/v2",
                 HasMetadata.getKind(TestCustomResource.class),
                 false)
-            .toPrimaryResourceIDs(secondary);
+            .toPrimaryResourceIDs(secondary, null);
 
     assertThat(res).contains(ResourceID.fromResource(primary));
   }

operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/PrimaryToSecondaryIndexTest.java

@@ -46,7 +46,7 @@ class PrimaryToSecondaryIndexTest {
 
   @BeforeEach
   void setup() {
-    when(secondaryToPrimaryMapperMock.toPrimaryResourceIDs(any()))
+    when(secondaryToPrimaryMapperMock.toPrimaryResourceIDs(any(), any()))
         .thenReturn(Set.of(primaryID1, primaryID2));
   }
 

operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/secondarytoprimaryreferencechange/ConfigCustomResource.java

@@ -0,0 +1,33 @@
+/*
+ * Copyright Java Operator SDK Authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *         http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package io.javaoperatorsdk.operator.baseapi.secondarytoprimaryreferencechange;
+
+import io.fabric8.kubernetes.api.model.Namespaced;
+import io.fabric8.kubernetes.client.CustomResource;
+import io.fabric8.kubernetes.model.annotation.Group;
+import io.fabric8.kubernetes.model.annotation.Kind;
+import io.fabric8.kubernetes.model.annotation.ShortNames;
+import io.fabric8.kubernetes.model.annotation.Version;
+
+/**
+ * Secondary resource that references a {@link TargetCustomResource} via {@code spec.targetName} and
+ * serves as input for it.
+ */
+@Group("sample.javaoperatorsdk")
+@Version("v1")
+@Kind("SecondaryToPrimaryRefConfig")
+@ShortNames("s2pconfig")
+public class ConfigCustomResource extends CustomResource<ConfigSpec, Void> implements Namespaced {}

operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/secondarytoprimaryreferencechange/ConfigSpec.java

@@ -0,0 +1,45 @@
+/*
+ * Copyright Java Operator SDK Authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *         http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package io.javaoperatorsdk.operator.baseapi.secondarytoprimaryreferencechange;
+
+public class ConfigSpec {
+
+  /**
+   * Name of the {@link TargetCustomResource} (in the same namespace) this config provides input.
+   */
+  private String targetName;
+
+  /** Value to be applied to the referenced target's status. */
+  private String value;
+
+  public String getTargetName() {
+    return targetName;
+  }
+
+  public ConfigSpec setTargetName(String targetName) {
+    this.targetName = targetName;
+    return this;
+  }
+
+  public String getValue() {
+    return value;
+  }
+
+  public ConfigSpec setValue(String value) {
+    this.value = value;
+    return this;
+  }
+}

operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/secondarytoprimaryreferencechange/ConfigToTargetMapper.java

@@ -0,0 +1,57 @@
+/*
+ * Copyright Java Operator SDK Authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *         http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package io.javaoperatorsdk.operator.baseapi.secondarytoprimaryreferencechange;
+
+import java.util.HashSet;
+import java.util.Set;
+
+import io.javaoperatorsdk.operator.processing.event.ResourceID;
+import io.javaoperatorsdk.operator.processing.event.source.SecondaryToPrimaryMapper;
+
+/**
+ * Maps a {@link ConfigCustomResource} (secondary) to the {@link TargetCustomResource} (primary) it
+ * references via {@code spec.targetName}.
+ *
+ * <p>The interesting case is handling a <em>reference change</em>: when {@code spec.targetName} is
+ * edited to point from one target to another, both targets must be reconciled — the previously
+ * referenced one so it can fall back to its default value, and the newly referenced one so it can
+ * pick up the config's value. The single-argument mapper only knows about the new reference, so it
+ * would only enqueue the new target, leaving the old target with a stale value. By overriding the
+ * two-argument variant we additionally enqueue the old target whenever the reference moved.
+ */
+public class ConfigToTargetMapper implements SecondaryToPrimaryMapper<ConfigCustomResource> {
+
+  @Override
+  public Set<ResourceID> toPrimaryResourceIDs(ConfigCustomResource config) {
+    var targetName = config.getSpec().getTargetName();
+    if (targetName == null || targetName.isBlank()) {
+      return Set.of();
+    }
+    return Set.of(new ResourceID(targetName, config.getMetadata().getNamespace()));
+  }
+
+  @Override
+  public Set<ResourceID> toPrimaryResourceIDs(
+      ConfigCustomResource newConfig, ConfigCustomResource oldConfig) {
+    var result = new HashSet<>(toPrimaryResourceIDs(newConfig));
+    // oldConfig is only populated for genuine update events while the controller is running; for
+    // adds, deletes and startup it is null and there is no previous reference to reconcile.
+    if (oldConfig != null) {
+      result.addAll(toPrimaryResourceIDs(oldConfig));
+    }
+    return result;
+  }
+}