fix: explicit workflow invocation uses the same resource intance that…

… reconcile api (operator-framework#2686)



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

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

@@ -73,7 +73,7 @@ public <T> Optional<T> getSecondaryResource(Class<T> expectedType, String eventS
        * If a workflow has an activation condition there can be event sources which are only
        * registered if the activation condition holds, but to provide a consistent API we return an
        * Optional instead of throwing an exception.
-       * 
+       *
        * Note that not only the resource which has an activation condition might not be registered
        * but dependents which depend on it.
        */
@@ -116,4 +116,8 @@ public DefaultContext<P> setRetryInfo(RetryInfo retryInfo) {
     this.retryInfo = retryInfo;
     return this;
   }
+
+    public P getPrimaryResource() {
+        return primaryResource;
+    }
 }

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

@@ -86,7 +86,7 @@ private PostExecutionControl<P> handleDispatch(ExecutionScope<P> executionScope)
     }
 
     Context<P> context =
-        new DefaultContext<>(executionScope.getRetryInfo(), controller, originalResource);
+        new DefaultContext<>(executionScope.getRetryInfo(), controller, resourceForExecution);
     if (markedForDeletion) {
       return handleCleanup(resourceForExecution, originalResource, context);
     } else {
@@ -234,29 +234,29 @@ private void updatePostExecutionControlWithReschedule(
     baseControl.getScheduleDelay().ifPresent(postExecutionControl::withReSchedule);
   }
 
-  private PostExecutionControl<P> handleCleanup(P resource,
+  private PostExecutionControl<P> handleCleanup(P resourceForExecution,
       P originalResource, Context<P> context) {
     if (log.isDebugEnabled()) {
       log.debug(
           "Executing delete for resource: {} with version: {}",
-          ResourceID.fromResource(resource),
-          getVersion(resource));
+          ResourceID.fromResource(resourceForExecution),
+          getVersion(resourceForExecution));
     }
-    DeleteControl deleteControl = controller.cleanup(resource, context);
+    DeleteControl deleteControl = controller.cleanup(resourceForExecution, context);
     final var useFinalizer = controller.useFinalizer();
     if (useFinalizer) {
       // note that we don't reschedule here even if instructed. Removing finalizer means that
-      // cleanup is finished, nothing left to done
+      // cleanup is finished, nothing left to be done
       final var finalizerName = configuration().getFinalizerName();
-      if (deleteControl.isRemoveFinalizer() && resource.hasFinalizer(finalizerName)) {
-        P customResource = conflictRetryingPatch(resource, originalResource, r -> {
+      if (deleteControl.isRemoveFinalizer() && resourceForExecution.hasFinalizer(finalizerName)) {
+        P customResource = conflictRetryingPatch(resourceForExecution, originalResource, r -> {
           // the operator might not be allowed to retrieve the resource on a retry, e.g. when its
           // permissions are removed by deleting the namespace concurrently
           if (r == null) {
             log.warn(
                 "Could not remove finalizer on null resource: {} with version: {}",
-                getUID(resource),
-                getVersion(resource));
+                getUID(resourceForExecution),
+                getVersion(resourceForExecution));
             return false;
           }
           return r.removeFinalizer(finalizerName);
@@ -266,8 +266,8 @@ private PostExecutionControl<P> handleCleanup(P resource,
     }
     log.debug(
         "Skipping finalizer remove for resource: {} with version: {}. delete control: {}, uses finalizer: {}",
-        getUID(resource),
-        getVersion(resource),
+        getUID(resourceForExecution),
+        getVersion(resourceForExecution),
         deleteControl,
         useFinalizer);
     PostExecutionControl<P> postExecutionControl = PostExecutionControl.defaultDispatch();

operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java

@@ -7,6 +7,8 @@
 import java.util.function.BiFunction;
 import java.util.function.Supplier;
 
+import io.fabric8.kubernetes.client.utils.KubernetesSerialization;
+import io.javaoperatorsdk.operator.api.reconciler.DefaultContext;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.mockito.ArgumentCaptor;
@@ -68,6 +70,9 @@ void setup() {
   }
 
   static void initConfigService(boolean useSSA) {
+      initConfigService(useSSA,true);
+  }
+  static void initConfigService(boolean useSSA, boolean noCloning) {
     /*
      * We need this for mock reconcilers to properly generate the expected UpdateControl: without
      * this, calls such as `when(reconciler.reconcile(eq(testCustomResource),
@@ -77,14 +82,18 @@ static void initConfigService(boolean useSSA) {
      */
     configurationService =
         ConfigurationService.newOverriddenConfigurationService(new BaseConfigurationService(),
-            overrider -> overrider.checkingCRDAndValidateLocalModel(false)
+            overrider ->  overrider.checkingCRDAndValidateLocalModel(false)
+
                 .withResourceCloner(new Cloner() {
                   @Override
                   public <R extends HasMetadata> R clone(R object) {
+                  if (noCloning) {
                     return object;
+                  }else {
+                    return new KubernetesSerialization().clone(object);
                   }
-                })
-                .withUseSSAToPatchPrimaryResource(useSSA));
+                }})
+                    .withUseSSAToPatchPrimaryResource(useSSA));
   }
 
   private <R extends HasMetadata> ReconciliationDispatcher<R> init(R customResource,
@@ -659,10 +668,24 @@ void reSchedulesFromErrorHandler() {
   }
 
   @Test
-  void addsFinalizerToPatchWithSSA() {
+  void reconcilerContextUsesTheSameInstanceOfResourceAsParam() {
+    initConfigService(false,false);
 
-  }
+    final ReconciliationDispatcher<TestCustomResource> dispatcher =
+            init(testCustomResource, reconciler, null, customResourceFacade, true);
 
+    testCustomResource.addFinalizer(DEFAULT_FINALIZER);
+    ArgumentCaptor<DefaultContext> contextArgumentCaptor = ArgumentCaptor.forClass(DefaultContext.class);
+    ArgumentCaptor<TestCustomResource> customResourceCaptor = ArgumentCaptor.forClass(TestCustomResource.class);
+
+    dispatcher.handleExecution(executionScopeWithCREvent(testCustomResource));
+      verify(reconciler, times(1))
+              .reconcile(customResourceCaptor.capture(), contextArgumentCaptor.capture());
+
+      assertThat(contextArgumentCaptor.getValue().getPrimaryResource())
+              .isSameAs(customResourceCaptor.getValue())
+              .isNotSameAs(testCustomResource);
+  }
 
   private ObservedGenCustomResource createObservedGenCustomResource() {
     ObservedGenCustomResource observedGenCustomResource = new ObservedGenCustomResource();