fix: null values deleted - using edit (#1219)

Author: csviri

docs/documentation/features.md

@@ -152,10 +152,8 @@ In order to have this feature working:
   interface. See also
   the [`ObservedGenerationAwareStatus`](https://github.com/java-operator-sdk/java-operator-sdk/blob/main/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/ObservedGenerationAwareStatus.java)
   which can also be extended.
-- The other condition is that the `CustomResource.getStatus()` method should not return `null`
-  , but an instance of the class representing `status`. The best way to achieve this is to
-  override [`CustomResource.initStatus()`](https://github.com/fabric8io/kubernetes-client/blob/865e0ddf67b99f954aa55ab14e5806d53ae149ec/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/CustomResource.java#L139)
-  .
+- The other condition is that the `CustomResource.getStatus()` should not return `null`. So the status should be instantiated
+  when the object is returned using the `UpdateControl`.
 
 If these conditions are fulfilled and generation awareness not turned off, the observed generation is automatically set
 by the framework after the `reconcile` method is called. Note that the observed generation is updated also

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

@@ -1,6 +1,7 @@
 package io.javaoperatorsdk.operator.api.reconciler;
 
 import io.fabric8.kubernetes.api.model.HasMetadata;
+import io.fabric8.kubernetes.client.CustomResource;
 
 public class UpdateControl<T extends HasMetadata> extends BaseControl<UpdateControl<T>> {
 
@@ -34,7 +35,15 @@ public static <T extends HasMetadata> UpdateControl<T> updateResource(T customRe
   }
 
   /**
-   * Preferred way to update the status. It does not do optimistic locking.
+   * Preferred way to update the status. It does not do optimistic locking. Uses JSON Patch to patch
+   * the resource.
+   * <p>
+   * Note that this does not work, if the {@link CustomResource#initStatus() initStatus} is
+   * implemented, since it breaks the diffing process. Don't implement it if using this method.
+   * </p>
+   * There is also an issue with setting value to null with older Kubernetes versions (1.19 and
+   * below). See: <a href=
+   * "https://github.com/fabric8io/kubernetes-client/issues/4158">https://github.com/fabric8io/kubernetes-client/issues/4158</a>
    *
    * @param <T> resource type
    * @param customResource the custom resource with target status

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

@@ -1,5 +1,9 @@
 package io.javaoperatorsdk.operator.processing.event;
 
+import java.io.ByteArrayInputStream;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -9,6 +13,7 @@
 import io.fabric8.kubernetes.client.dsl.MixedOperation;
 import io.fabric8.kubernetes.client.dsl.Resource;
 import io.fabric8.kubernetes.client.dsl.internal.HasMetadataOperationsImpl;
+import io.fabric8.kubernetes.client.utils.Serialization;
 import io.javaoperatorsdk.operator.api.ObservedGenerationAware;
 import io.javaoperatorsdk.operator.api.config.ConfigurationServiceProvider;
 import io.javaoperatorsdk.operator.api.config.ControllerConfiguration;
@@ -96,31 +101,19 @@ private PostExecutionControl<R> handleReconcile(
       updateCustomResourceWithFinalizer(originalResource);
       return PostExecutionControl.onlyFinalizerAdded();
     } else {
+      var resourceForExecution =
+          cloneResource(originalResource);
       try {
-        var resourceForExecution =
-            cloneResourceForErrorStatusHandlerIfNeeded(originalResource);
         return reconcileExecution(executionScope, resourceForExecution, originalResource, context);
       } catch (Exception e) {
-        return handleErrorStatusHandler(originalResource, context, e);
+        return handleErrorStatusHandler(resourceForExecution, originalResource, context, e);
       }
     }
   }
 
-  /**
-   * Resource make sense only to clone for the ErrorStatusHandler or if the observed generation in
-   * status is handled automatically. Otherwise, this operation can be skipped since it can be
-   * memory and time-consuming. However, it needs to be cloned since it's common that the custom
-   * resource is changed during an execution, and it's much cleaner to have to original resource in
-   * place for status update.
-   */
-  private R cloneResourceForErrorStatusHandlerIfNeeded(R resource) {
-    if (isErrorStatusHandlerPresent() ||
-        shouldUpdateObservedGenerationAutomatically(resource)) {
-      final var cloner = ConfigurationServiceProvider.instance().getResourceCloner();
-      return cloner.clone(resource);
-    } else {
-      return resource;
-    }
+  private R cloneResource(R resource) {
+    final var cloner = ConfigurationServiceProvider.instance().getResourceCloner();
+    return cloner.clone(resource);
   }
 
   private PostExecutionControl<R> reconcileExecution(ExecutionScope<R> executionScope,
@@ -141,27 +134,31 @@ private PostExecutionControl<R> reconcileExecution(ExecutionScope<R> executionSc
           .getMetadata()
           .setResourceVersion(updatedCustomResource.getMetadata().getResourceVersion());
       updatedCustomResource =
-          updateStatusGenerationAware(updateControl.getResource(), updateControl.isPatch());
+          updateStatusGenerationAware(updateControl.getResource(), originalResource,
+              updateControl.isPatch());
     } else if (updateControl.isUpdateStatus()) {
       updatedCustomResource =
-          updateStatusGenerationAware(updateControl.getResource(), updateControl.isPatch());
+          updateStatusGenerationAware(updateControl.getResource(), originalResource,
+              updateControl.isPatch());
     } else if (updateControl.isUpdateResource()) {
       updatedCustomResource =
           updateCustomResource(updateControl.getResource());
       if (shouldUpdateObservedGenerationAutomatically(updatedCustomResource)) {
         updatedCustomResource =
-            updateStatusGenerationAware(originalResource, updateControl.isPatch());
+            updateStatusGenerationAware(updateControl.getResource(), originalResource,
+                updateControl.isPatch());
       }
     } else if (updateControl.isNoUpdate()
         && shouldUpdateObservedGenerationAutomatically(resourceForExecution)) {
       updatedCustomResource =
-          updateStatusGenerationAware(originalResource, updateControl.isPatch());
+          updateStatusGenerationAware(originalResource, originalResource, updateControl.isPatch());
     }
     return createPostExecutionControl(updatedCustomResource, updateControl);
   }
 
   @SuppressWarnings("unchecked")
-  private PostExecutionControl<R> handleErrorStatusHandler(R resource, Context<R> context,
+  private PostExecutionControl<R> handleErrorStatusHandler(R resource, R originalResource,
+      Context<R> context,
       Exception e) throws Exception {
     if (isErrorStatusHandlerPresent()) {
       try {
@@ -183,7 +180,7 @@ public boolean isLastAttempt() {
         R updatedResource = null;
         if (errorStatusUpdateControl.getResource().isPresent()) {
           updatedResource = errorStatusUpdateControl.isPatch() ? customResourceFacade
-              .patchStatus(errorStatusUpdateControl.getResource().orElseThrow())
+              .patchStatus(errorStatusUpdateControl.getResource().orElseThrow(), originalResource)
               : customResourceFacade
                   .updateStatus(errorStatusUpdateControl.getResource().orElseThrow());
         }
@@ -207,10 +204,10 @@ private boolean isErrorStatusHandlerPresent() {
     return controller.getReconciler() instanceof ErrorStatusHandler;
   }
 
-  private R updateStatusGenerationAware(R resource, boolean patch) {
+  private R updateStatusGenerationAware(R resource, R originalResource, boolean patch) {
     updateStatusObservedGenerationIfRequired(resource);
     if (patch) {
-      return customResourceFacade.patchStatus(resource);
+      return customResourceFacade.patchStatus(resource, originalResource);
     } else {
       return customResourceFacade.updateStatus(resource);
     }
@@ -357,18 +354,24 @@ public R updateStatus(R resource) {
       return (R) hasMetadataOperation.replaceStatus(resource);
     }
 
-    public R patchStatus(R resource) {
+    public R patchStatus(R resource, R originalResource) {
       log.trace("Updating status for resource: {}", resource);
       String resourceVersion = resource.getMetadata().getResourceVersion();
-      try {
-        // don't do optimistic locking on patch
-        resource.getMetadata().setResourceVersion(null);
+      // don't do optimistic locking on patch
+      originalResource.getMetadata().setResourceVersion(null);
+      resource.getMetadata().setResourceVersion(null);
+      try (var bis = new ByteArrayInputStream(
+          Serialization.asJson(originalResource).getBytes(StandardCharsets.UTF_8))) {
         return resourceOperation
             .inNamespace(resource.getMetadata().getNamespace())
-            .withName(getName(resource))
-            .patchStatus(resource);
+            // will be simplified in fabric8 v6
+            .load(bis)
+            .editStatus(r -> resource);
+      } catch (IOException e) {
+        throw new IllegalStateException(e);
       } finally {
         // restore initial resource version
+        originalResource.getMetadata().setResourceVersion(resourceVersion);
         resource.getMetadata().setResourceVersion(resourceVersion);
       }
     }

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

@@ -142,7 +142,7 @@ void updatesOnlyStatusSubResourceIfFinalizerSet() {
 
     reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource));
 
-    verify(customResourceFacade, times(1)).patchStatus(testCustomResource);
+    verify(customResourceFacade, times(1)).patchStatus(eq(testCustomResource), any());
     verify(customResourceFacade, never()).replaceResourceWithLock(any());
   }
 
@@ -168,7 +168,7 @@ void patchesStatus() {
 
     reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource));
 
-    verify(customResourceFacade, times(1)).patchStatus(testCustomResource);
+    verify(customResourceFacade, times(1)).patchStatus(eq(testCustomResource), any());
     verify(customResourceFacade, never()).updateStatus(any());
     verify(customResourceFacade, never()).replaceResourceWithLock(any());
   }
@@ -362,7 +362,7 @@ void setObservedGenerationForStatusIfNeeded() throws Exception {
 
     when(reconciler.reconcile(any(), any()))
         .thenReturn(UpdateControl.patchStatus(observedGenResource));
-    when(facade.patchStatus(observedGenResource)).thenReturn(observedGenResource);
+    when(facade.patchStatus(eq(observedGenResource), any())).thenReturn(observedGenResource);
 
     PostExecutionControl<ObservedGenCustomResource> control = dispatcher.handleExecution(
         executionScopeWithCREvent(observedGenResource));
@@ -521,7 +521,7 @@ void errorStatusHandlerCanPatchResource() {
         new ExecutionScope(
             testCustomResource, null));
 
-    verify(customResourceFacade, times(1)).patchStatus(testCustomResource);
+    verify(customResourceFacade, times(1)).patchStatus(eq(testCustomResource), any());
     verify(((ErrorStatusHandler) reconciler), times(1)).updateErrorStatus(eq(testCustomResource),
         any(), any());
   }

operator-framework/src/test/java/io/javaoperatorsdk/operator/MultiVersionCRDIT.java

@@ -65,10 +65,8 @@ void invalidEventsDoesNotBreakEventHandling() {
     assertThat(
         operator
             .get(MultiVersionCRDTestCustomResource2.class, CR_V2_NAME)
-            .getStatus()
-            .getReconciledBy()
-            .size())
-                .isZero();
+            .getStatus())
+                .isNull();
   }
 
 

operator-framework/src/test/java/io/javaoperatorsdk/operator/StatusPatchNotLockingIT.java

@@ -3,14 +3,17 @@
 import java.time.Duration;
 import java.util.Map;
 
+import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.RegisterExtension;
 
 import io.fabric8.kubernetes.api.model.ObjectMetaBuilder;
 import io.javaoperatorsdk.operator.junit.LocalOperatorExtension;
 import io.javaoperatorsdk.operator.sample.statuspatchnonlocking.StatusPatchLockingCustomResource;
+import io.javaoperatorsdk.operator.sample.statuspatchnonlocking.StatusPatchLockingCustomResourceSpec;
 import io.javaoperatorsdk.operator.sample.statuspatchnonlocking.StatusPatchLockingReconciler;
 
+import static io.javaoperatorsdk.operator.sample.statuspatchnonlocking.StatusPatchLockingReconciler.MESSAGE;
 import static io.javaoperatorsdk.operator.sample.statusupdatelocking.StatusUpdateLockingReconciler.WAIT_TIME;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.awaitility.Awaitility.await;
@@ -44,8 +47,34 @@ void noOptimisticLockingDoneOnStatusUpdate() throws InterruptedException {
     });
   }
 
+  // see https://github.com/fabric8io/kubernetes-client/issues/4158
+  @Disabled("Patch generated supported by K8S version below 1.20.x")
+  @Test
+  void valuesAreDeletedIfSetToNull() {
+    var resource = operator.create(StatusPatchLockingCustomResource.class, createResource());
+
+    await().untilAsserted(() -> {
+      var actual = operator.get(StatusPatchLockingCustomResource.class,
+          TEST_RESOURCE_NAME);
+      assertThat(actual.getStatus()).isNotNull();
+      assertThat(actual.getStatus().getMessage()).isEqualTo(MESSAGE);
+    });
+
+    resource.getSpec().setMessageInStatus(false);
+    operator.replace(StatusPatchLockingCustomResource.class, resource);
+
+    await().untilAsserted(() -> {
+      var actual = operator.get(StatusPatchLockingCustomResource.class,
+          TEST_RESOURCE_NAME);
+      assertThat(actual.getStatus()).isNotNull();
+      assertThat(actual.getStatus().getMessage()).isNull();
+    });
+  }
+
+
   StatusPatchLockingCustomResource createResource() {
     StatusPatchLockingCustomResource res = new StatusPatchLockingCustomResource();
+    res.setSpec(new StatusPatchLockingCustomResourceSpec());
     res.setMetadata(new ObjectMetaBuilder().withName(TEST_RESOURCE_NAME).build());
     return res;
   }

operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/changenamespace/ChangeNamespaceTestCustomResource.java

@@ -11,8 +11,4 @@ public class ChangeNamespaceTestCustomResource
     extends CustomResource<Void, ChangeNamespaceTestCustomResourceStatus>
     implements Namespaced {
 
-  @Override
-  protected ChangeNamespaceTestCustomResourceStatus initStatus() {
-    return new ChangeNamespaceTestCustomResourceStatus();
-  }
 }

operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/changenamespace/ChangeNamespaceTestReconciler.java

@@ -45,7 +45,9 @@ public UpdateControl<ChangeNamespaceTestCustomResource> reconcile(
     }
 
     increaseNumberOfResourceExecutions(primary);
-
+    if (primary.getStatus() == null) {
+      primary.setStatus(new ChangeNamespaceTestCustomResourceStatus());
+    }
     var statusUpdates = primary.getStatus().getNumberOfStatusUpdates();
     primary.getStatus().setNumberOfStatusUpdates(statusUpdates + 1);
     return UpdateControl.patchStatus(primary);

operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/multiversioncrd/MultiVersionCRDTestCustomResource1.java

@@ -16,9 +16,5 @@ public class MultiVersionCRDTestCustomResource1
     CustomResource<MultiVersionCRDTestCustomResourceSpec1, MultiVersionCRDTestCustomResourceStatus1>
     implements Namespaced {
 
-  @Override
-  protected MultiVersionCRDTestCustomResourceStatus1 initStatus() {
-    return new MultiVersionCRDTestCustomResourceStatus1();
-  }
 
 }

operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/multiversioncrd/MultiVersionCRDTestCustomResource2.java

@@ -16,9 +16,4 @@ public class MultiVersionCRDTestCustomResource2
     CustomResource<MultiVersionCRDTestCustomResourceSpec2, MultiVersionCRDTestCustomResourceStatus2>
     implements Namespaced {
 
-  @Override
-  protected MultiVersionCRDTestCustomResourceStatus2 initStatus() {
-    return new MultiVersionCRDTestCustomResourceStatus2();
-  }
-
 }