feat: use ssa for status update by default from UpdateControl (#2278)

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

Author: csviri

Diff for: docs/documentation/v5-0-migration.md

@@ -17,5 +17,12 @@ permalink: /docs/v5-0-migration
    [`EventSourceUtils`](https://github.com/operator-framework/java-operator-sdk/blob/main/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/EventSourceUtils.java#L11-L11)
    now contains all the utility methods used for event sources naming that were previously defined in
    the `EventSourceInitializer` interface.
-3. `ManagedDependentResourceContext` has been renamed to `ManagedWorkflowAndDependentResourceContext` and is accessed
+3. Patching status through `UpdateControl` like the `patchStatus` method now by default
+   uses Server Side Apply instead of simple patch. To use the former approach, use the feature flag 
+   in [`ConfigurationService`](https://github.com/operator-framework/java-operator-sdk/blob/main/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java#L400-L400)
+   !!! IMPORTANT !!!
+   Migration from a non-SSA based controller to SSA based controller can cause problems, due to known issues. 
+   See the following [integration test](https://github.com/operator-framework/java-operator-sdk/blob/main/operator-framework/src/test/java/io/javaoperatorsdk/operator/StatusPatchSSAMigrationIT.java#L71-L82) where it is demonstrated.
+   Also, the related part of a [workaround](https://github.com/operator-framework/java-operator-sdk/blob/main/operator-framework/src/test/java/io/javaoperatorsdk/operator/StatusPatchSSAMigrationIT.java#L110-L116).
+4. `ManagedDependentResourceContext` has been renamed to `ManagedWorkflowAndDependentResourceContext` and is accessed
    via the accordingly renamed `managedWorkflowAndDependentResourceContext` method.

Diff for: operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java

@@ -401,4 +401,14 @@ default boolean parseResourceVersionsForEventFilteringAndCaching() {
     return false;
   }
 
+  /**
+   * {@link io.javaoperatorsdk.operator.api.reconciler.UpdateControl} patchStatus can either use
+   * simple update or SSA for status subresource patching.
+   *
+   * @return true by default
+   */
+  default boolean useSSAForResourceStatusPatch() {
+    return true;
+  }
+
 }

Diff for: operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java

@@ -40,6 +40,7 @@ public class ConfigurationServiceOverrider {
   private Set<Class<? extends HasMetadata>> defaultNonSSAResource;
   private Boolean previousAnnotationForDependentResources;
   private Boolean parseResourceVersions;
+  private Boolean useSSAForResourceStatusPatch;
   private DependentResourceFactory dependentResourceFactory;
 
   ConfigurationServiceOverrider(ConfigurationService original) {
@@ -174,12 +175,17 @@ public ConfigurationServiceOverrider withPreviousAnnotationForDependentResources
     return this;
   }
 
-  public ConfigurationServiceOverrider wihtParseResourceVersions(
+  public ConfigurationServiceOverrider withParseResourceVersions(
       boolean value) {
     this.parseResourceVersions = value;
     return this;
   }
 
+  public ConfigurationServiceOverrider withUseSSAForResourceStatusPatch(boolean value) {
+    this.useSSAForResourceStatusPatch = value;
+    return this;
+  }
+
   public ConfigurationService build() {
     return new BaseConfigurationService(original.getVersion(), cloner, client) {
       @Override
@@ -320,6 +326,14 @@ public boolean parseResourceVersionsForEventFilteringAndCaching() {
             ? parseResourceVersions
             : super.parseResourceVersionsForEventFilteringAndCaching();
       }
+
+      @Override
+      public boolean useSSAForResourceStatusPatch() {
+        return useSSAForResourceStatusPatch != null
+            ? useSSAForResourceStatusPatch
+            : super.useSSAForResourceStatusPatch();
+
+      }
     };
   }
 

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

@@ -12,6 +12,8 @@
 import io.fabric8.kubernetes.client.KubernetesClientException;
 import io.fabric8.kubernetes.client.dsl.MixedOperation;
 import io.fabric8.kubernetes.client.dsl.Resource;
+import io.fabric8.kubernetes.client.dsl.base.PatchContext;
+import io.fabric8.kubernetes.client.dsl.base.PatchType;
 import io.javaoperatorsdk.operator.OperatorException;
 import io.javaoperatorsdk.operator.api.ObservedGenerationAware;
 import io.javaoperatorsdk.operator.api.config.Cloner;
@@ -25,9 +27,7 @@
 import io.javaoperatorsdk.operator.api.reconciler.UpdateControl;
 import io.javaoperatorsdk.operator.processing.Controller;
 
-import static io.javaoperatorsdk.operator.processing.KubernetesResourceUtils.getName;
-import static io.javaoperatorsdk.operator.processing.KubernetesResourceUtils.getUID;
-import static io.javaoperatorsdk.operator.processing.KubernetesResourceUtils.getVersion;
+import static io.javaoperatorsdk.operator.processing.KubernetesResourceUtils.*;
 
 /**
  * Handles calls and results of a Reconciler and finalizer related logic
@@ -45,8 +45,7 @@ class ReconciliationDispatcher<P extends HasMetadata> {
   private final boolean retryConfigurationHasZeroAttempts;
   private final Cloner cloner;
 
-  ReconciliationDispatcher(Controller<P> controller,
-      CustomResourceFacade<P> customResourceFacade) {
+  ReconciliationDispatcher(Controller<P> controller, CustomResourceFacade<P> customResourceFacade) {
     this.controller = controller;
     this.customResourceFacade = customResourceFacade;
     this.cloner = controller.getConfiguration().getConfigurationService().getResourceCloner();
@@ -56,7 +55,8 @@ class ReconciliationDispatcher<P extends HasMetadata> {
   }
 
   public ReconciliationDispatcher(Controller<P> controller) {
-    this(controller, new CustomResourceFacade<>(controller.getCRClient()));
+    this(controller,
+        new CustomResourceFacade<>(controller.getCRClient(), controller.getConfiguration()));
   }
 
   public PostExecutionControl<P> handleExecution(ExecutionScope<P> executionScope) {
@@ -139,33 +139,25 @@ private PostExecutionControl<P> reconcileExecution(ExecutionScope<P> executionSc
 
     UpdateControl<P> updateControl = controller.reconcile(resourceForExecution, context);
     P updatedCustomResource = null;
+    final var toUpdate =
+        updateControl.isNoUpdate() ? originalResource : updateControl.getResource();
     if (updateControl.isUpdateResourceAndStatus()) {
-      updatedCustomResource =
-          updateCustomResource(updateControl.getResource());
-      updateControl
-          .getResource()
+      updatedCustomResource = updateCustomResource(toUpdate);
+      toUpdate
           .getMetadata()
           .setResourceVersion(updatedCustomResource.getMetadata().getResourceVersion());
-      updatedCustomResource =
-          updateStatusGenerationAware(updateControl.getResource(), originalResource,
-              updateControl.isPatchStatus());
-    } else if (updateControl.isUpdateStatus()) {
-      updatedCustomResource =
-          updateStatusGenerationAware(updateControl.getResource(), originalResource,
-              updateControl.isPatchStatus());
     } else if (updateControl.isUpdateResource()) {
+      updatedCustomResource = updateCustomResource(toUpdate);
+    }
+
+    // check if status also needs to be updated
+    final var updateObservedGeneration = updateControl.isNoUpdate()
+        ? shouldUpdateObservedGenerationAutomatically(resourceForExecution)
+        : shouldUpdateObservedGenerationAutomatically(updatedCustomResource);
+    if (updateControl.isUpdateResourceAndStatus() || updateControl.isUpdateStatus()
+        || updateObservedGeneration) {
       updatedCustomResource =
-          updateCustomResource(updateControl.getResource());
-      if (shouldUpdateObservedGenerationAutomatically(updatedCustomResource)) {
-        updatedCustomResource =
-            updateStatusGenerationAware(updateControl.getResource(), originalResource,
-                updateControl.isPatchStatus());
-      }
-    } else if (updateControl.isNoUpdate()
-        && shouldUpdateObservedGenerationAutomatically(resourceForExecution)) {
-      updatedCustomResource =
-          updateStatusGenerationAware(originalResource, originalResource,
-              updateControl.isPatchStatus());
+          updateStatusGenerationAware(toUpdate, originalResource, updateControl.isPatchStatus());
     }
     return createPostExecutionControl(updatedCustomResource, updateControl);
   }
@@ -380,10 +372,16 @@ public P conflictRetryingUpdate(P resource, Function<P, Boolean> modificationFun
   static class CustomResourceFacade<R extends HasMetadata> {
 
     private final MixedOperation<R, KubernetesResourceList<R>, Resource<R>> resourceOperation;
+    private final boolean useSSAToUpdateStatus;
+    private final String fieldManager;
 
     public CustomResourceFacade(
-        MixedOperation<R, KubernetesResourceList<R>, Resource<R>> resourceOperation) {
+        MixedOperation<R, KubernetesResourceList<R>, Resource<R>> resourceOperation,
+        ControllerConfiguration<R> configuration) {
       this.resourceOperation = resourceOperation;
+      this.useSSAToUpdateStatus =
+          configuration.getConfigurationService().useSSAForResourceStatusPatch();
+      this.fieldManager = configuration.fieldManager();
     }
 
     public R getResource(String namespace, String name) {
@@ -413,14 +411,30 @@ public R updateStatus(R resource) {
     }
 
     public R patchStatus(R resource, R originalResource) {
-      log.trace("Updating status for resource: {}", resource);
+      log.trace("Patching status for resource: {} with ssa: {}", resource, useSSAToUpdateStatus);
       String resourceVersion = resource.getMetadata().getResourceVersion();
       // don't do optimistic locking on patch
       originalResource.getMetadata().setResourceVersion(null);
       resource.getMetadata().setResourceVersion(null);
       try {
-        return resource(originalResource)
-            .editStatus(r -> resource);
+        if (useSSAToUpdateStatus) {
+          var managedFields = resource.getMetadata().getManagedFields();
+          try {
+
+            resource.getMetadata().setManagedFields(null);
+            var res = resource(resource);
+            return res.subresource("status").patch(new PatchContext.Builder()
+                .withFieldManager(fieldManager)
+                .withForce(true)
+                .withPatchType(PatchType.SERVER_SIDE_APPLY)
+                .build());
+          } finally {
+            resource.getMetadata().setManagedFields(managedFields);
+          }
+        } else {
+          var res = resource(originalResource);
+          return res.editStatus(r -> resource);
+        }
       } finally {
         // restore initial resource version
         originalResource.getMetadata().setResourceVersion(resourceVersion);

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

@@ -43,19 +43,9 @@
 import static io.javaoperatorsdk.operator.TestUtils.markForDeletion;
 import static io.javaoperatorsdk.operator.processing.event.ReconciliationDispatcher.MAX_UPDATE_RETRY;
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertFalse;
-import static org.junit.jupiter.api.Assertions.assertTrue;
-import static org.junit.jupiter.api.Assertions.fail;
+import static org.junit.jupiter.api.Assertions.*;
 import static org.mockito.ArgumentMatchers.argThat;
-import static org.mockito.Mockito.any;
-import static org.mockito.Mockito.eq;
-import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.never;
-import static org.mockito.Mockito.spy;
-import static org.mockito.Mockito.times;
-import static org.mockito.Mockito.verify;
-import static org.mockito.Mockito.when;
+import static org.mockito.Mockito.*;
 
 @SuppressWarnings({"unchecked", "rawtypes"})
 class ReconciliationDispatcherTest {

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

@@ -58,10 +58,12 @@ void valuesAreDeletedIfSetToNull() {
       assertThat(actual.getStatus().getMessage()).isEqualTo(MESSAGE);
     });
 
+    // resource needs to be read again to we don't replace the with wrong managed fields
+    resource = operator.get(StatusPatchLockingCustomResource.class, TEST_RESOURCE_NAME);
     resource.getSpec().setMessageInStatus(false);
     operator.replace(resource);
 
-    await().untilAsserted(() -> {
+    await().timeout(Duration.ofMinutes(3)).untilAsserted(() -> {
       var actual = operator.get(StatusPatchLockingCustomResource.class,
           TEST_RESOURCE_NAME);
       assertThat(actual.getStatus()).isNotNull();