feat: using SSA for finalizer and primary patch (#2305)

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

Author: csviri

docs/documentation/v5-0-migration.md

@@ -17,15 +17,21 @@ 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. 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)
+3. Updates through `UpdateControl` now use [Server Side Apply (SSA)](https://kubernetes.io/docs/reference/using-api/server-side-apply/) by default to add the finalizer and for all
+   the patch operations in `UpdateControl`. The update operations were removed. If you do not wish to use SSA, you can deactivate the feature using `ConfigurationService.useSSAToPatchPrimaryResource` and related `ConfigurationServiceOverrider.withUseSSAToPatchPrimaryResource`.
+
    !!! 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)
+
+   See known issues with migration from non-SSA to SSA based status updates here:
+   [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).
+
+   Related automatic observed generation handling changes: 
+   Automated Observed Generation (see features in docs), is automatically handled for non-SSA, even if
+   the status sub-resource is not instructed to be updated. This is not true for SSA, observed generation is updated 
+   only when patch status is instructed by `UpdateControl`.
+
 4. `ManagedDependentResourceContext` has been renamed to `ManagedWorkflowAndDependentResourceContext` and is accessed
    via the accordingly renamed `managedWorkflowAndDependentResourceContext` method.
 5. `ResourceDiscriminator` was removed. In most of the cases you can just delete the discriminator, everything should

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

@@ -386,12 +386,15 @@ default boolean parseResourceVersionsForEventFilteringAndCaching() {
   }
 
   /**
-   * {@link io.javaoperatorsdk.operator.api.reconciler.UpdateControl} patchStatus can either use
-   * simple update or SSA for status subresource patching.
+   * {@link io.javaoperatorsdk.operator.api.reconciler.UpdateControl} patch resource or status can
+   * either use simple patches or SSA. Setting this to {@code true}, controllers will use SSA for
+   * adding finalizers, managing observed generation, patching resources and status.
    *
-   * @return true by default
+   * @return {@code true} by default
+   * @since 5.0.0
+   * @see ConfigurationServiceOverrider#withUseSSAToPatchPrimaryResource(boolean)
    */
-  default boolean useSSAForResourceStatusPatch() {
+  default boolean useSSAToPatchPrimaryResource() {
     return true;
   }
 

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

@@ -39,7 +39,7 @@ public class ConfigurationServiceOverrider {
   private Set<Class<? extends HasMetadata>> defaultNonSSAResource;
   private Boolean previousAnnotationForDependentResources;
   private Boolean parseResourceVersions;
-  private Boolean useSSAForResourceStatusPatch;
+  private Boolean useSSAToPatchPrimaryResource;
   private DependentResourceFactory dependentResourceFactory;
 
   ConfigurationServiceOverrider(ConfigurationService original) {
@@ -175,8 +175,8 @@ public ConfigurationServiceOverrider withParseResourceVersions(
     return this;
   }
 
-  public ConfigurationServiceOverrider withUseSSAForResourceStatusPatch(boolean value) {
-    this.useSSAForResourceStatusPatch = value;
+  public ConfigurationServiceOverrider withUseSSAToPatchPrimaryResource(boolean value) {
+    this.useSSAToPatchPrimaryResource = value;
     return this;
   }
 
@@ -317,10 +317,10 @@ public boolean parseResourceVersionsForEventFilteringAndCaching() {
       }
 
       @Override
-      public boolean useSSAForResourceStatusPatch() {
-        return useSSAForResourceStatusPatch != null
-            ? useSSAForResourceStatusPatch
-            : super.useSSAForResourceStatusPatch();
+      public boolean useSSAToPatchPrimaryResource() {
+        return useSSAToPatchPrimaryResource != null
+            ? useSSAToPatchPrimaryResource
+            : super.useSSAToPatchPrimaryResource();
 
       }
     };

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

@@ -9,24 +9,18 @@ public class ErrorStatusUpdateControl<P extends HasMetadata>
     extends BaseControl<ErrorStatusUpdateControl<P>> {
 
   private final P resource;
-  private final boolean patch;
   private boolean noRetry = false;
 
   public static <T extends HasMetadata> ErrorStatusUpdateControl<T> patchStatus(T resource) {
-    return new ErrorStatusUpdateControl<>(resource, true);
-  }
-
-  public static <T extends HasMetadata> ErrorStatusUpdateControl<T> updateStatus(T resource) {
-    return new ErrorStatusUpdateControl<>(resource, false);
+    return new ErrorStatusUpdateControl<>(resource);
   }
 
   public static <T extends HasMetadata> ErrorStatusUpdateControl<T> noStatusUpdate() {
-    return new ErrorStatusUpdateControl<>(null, true);
+    return new ErrorStatusUpdateControl<>(null);
   }
 
-  private ErrorStatusUpdateControl(P resource, boolean patch) {
+  private ErrorStatusUpdateControl(P resource) {
     this.resource = resource;
-    this.patch = patch;
   }
 
   /**
@@ -47,10 +41,6 @@ public boolean isNoRetry() {
     return noRetry;
   }
 
-  public boolean isPatch() {
-    return patch;
-  }
-
   /**
    * If re-scheduled using this method, it is not considered as retry, it effectively cancels retry.
    *

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

@@ -1,150 +1,80 @@
 package io.javaoperatorsdk.operator.api.reconciler;
 
+import java.util.Optional;
+
 import io.fabric8.kubernetes.api.model.HasMetadata;
 import io.fabric8.kubernetes.client.CustomResource;
 
 public class UpdateControl<P extends HasMetadata> extends BaseControl<UpdateControl<P>> {
 
   private final P resource;
-  private final boolean updateStatus;
-  private final boolean updateResource;
+  private final boolean patchResource;
   private final boolean patchStatus;
 
   private UpdateControl(
-      P resource, boolean updateStatus, boolean updateResource, boolean patchStatus) {
-    if ((updateResource || updateStatus) && resource == null) {
+      P resource, boolean patchResource, boolean patchStatus) {
+    if ((patchResource || patchStatus) && resource == null) {
       throw new IllegalArgumentException("CustomResource cannot be null in case of update");
     }
     this.resource = resource;
-    this.updateStatus = updateStatus;
-    this.updateResource = updateResource;
+    this.patchResource = patchResource;
     this.patchStatus = patchStatus;
   }
 
-  /**
-   * Creates an update control instance that instructs the framework to do an update on resource
-   * itself, not on the status. Note that usually as a results of a reconciliation should be a
-   * status update not an update to the resource itself.
-   *
-   * Using this update makes sure that the resource in the next reconciliation is the updated one -
-   * this is not guaranteed by default if you do an update on a resource by the Kubernetes client.
-   *
-   * @param <T> custom resource type
-   * @param customResource customResource to use for update
-   * @return initialized update control
-   */
-  public static <T extends HasMetadata> UpdateControl<T> updateResource(T customResource) {
-    return new UpdateControl<>(customResource, false, true, false);
-  }
-
   /**
    * 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.
+   * Note that this does not work, if the {@link CustomResource#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=
+   * There is also an issue with setting value to {@code 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
    * @return UpdateControl instance
    */
   public static <T extends HasMetadata> UpdateControl<T> patchStatus(T customResource) {
-    return new UpdateControl<>(customResource, true, false, true);
-  }
-
-  /**
-   * Note that usually "patchStatus" is advised to be used instead of this method.
-   * <p>
-   * Updates the status with optimistic locking regarding current resource version reconciled. Note
-   * that this also ensures that on next reconciliation is the most up-to-date custom resource is
-   * used.
-   * </p>
-   *
-   * @param <T> resource type
-   * @param customResource the custom resource with target status
-   * @return UpdateControl instance
-   */
-  public static <T extends HasMetadata> UpdateControl<T> updateStatus(T customResource) {
-    return new UpdateControl<>(customResource, true, false, false);
-  }
-
-  /**
-   * As a results of this there will be two call to K8S API. First the custom resource will be
-   * updates then the status sub-resource.
-   *
-   * Using this update makes sure that the resource in the next reconciliation is the updated one -
-   * this is not guaranteed by default if you do an update on a resource by the Kubernetes client.
-   *
-   * @param <T> resource type
-   * @param customResource - custom resource to use in both API calls
-   * @return UpdateControl instance
-   */
-  public static <T extends HasMetadata> UpdateControl<T> updateResourceAndStatus(
-      T customResource) {
-    return new UpdateControl<>(customResource, true, true, false);
+    return new UpdateControl<>(customResource, false, true);
   }
 
-  /**
-   * Updates the resource - with optimistic locking - and patches the status without optimistic
-   * locking in place.
-   *
-   * Note that using this method, it is not guaranteed that the most recent updated resource will be
-   * in case for next reconciliation.
-   *
-   * @param customResource to update
-   * @return UpdateControl instance
-   * @param <T> resource type
-   */
-  public static <T extends HasMetadata> UpdateControl<T> updateResourceAndPatchStatus(
-      T customResource) {
-    return new UpdateControl<>(customResource, true, true, true);
+  public static <T extends HasMetadata> UpdateControl<T> patchResource(T customResource) {
+    return new UpdateControl<>(customResource, true, false);
   }
 
   /**
-   * Marked for removal, because of confusing name. It does not patch the resource but rather
-   * updates it.
-   *
-   * @deprecated use {@link UpdateControl#updateResourceAndPatchStatus(HasMetadata)}
-   *
    * @param customResource to update
    * @return UpdateControl instance
    * @param <T> resource type
    */
-  @Deprecated(forRemoval = true)
   public static <T extends HasMetadata> UpdateControl<T> patchResourceAndStatus(T customResource) {
-    return updateResourceAndStatus(customResource);
+    return new UpdateControl<>(customResource, true, true);
   }
 
   public static <T extends HasMetadata> UpdateControl<T> noUpdate() {
-    return new UpdateControl<>(null, false, false, false);
-  }
-
-  public P getResource() {
-    return resource;
+    return new UpdateControl<>(null, false, false);
   }
 
-  public boolean isUpdateStatus() {
-    return updateStatus;
+  public Optional<P> getResource() {
+    return Optional.ofNullable(resource);
   }
 
-  public boolean isUpdateResource() {
-    return updateResource;
+  public boolean isPatchResource() {
+    return patchResource;
   }
 
   public boolean isPatchStatus() {
     return patchStatus;
   }
 
   public boolean isNoUpdate() {
-    return !updateResource && !updateStatus;
+    return !patchResource && !patchStatus;
   }
 
-  public boolean isUpdateResourceAndStatus() {
-    return updateResource && updateStatus;
+  public boolean isPatchResourceAndStatus() {
+    return patchResource && patchStatus;
   }
 
 }

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

@@ -122,10 +122,10 @@ public String controllerName() {
           @Override
           public String successTypeName(UpdateControl<P> result) {
             String successType = RESOURCE;
-            if (result.isUpdateStatus()) {
+            if (result.isPatchStatus()) {
               successType = STATUS;
             }
-            if (result.isUpdateResourceAndStatus()) {
+            if (result.isPatchResourceAndStatus()) {
               successType = BOTH;
             }
             return successType;

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

@@ -245,17 +245,6 @@ synchronized void eventProcessingFinished(
       state.markProcessedMarkForDeletion();
       metrics.cleanupDoneFor(resourceID, metricsMetadata);
     } else {
-      postExecutionControl
-          .getUpdatedCustomResource()
-          .ifPresent(
-              p -> {
-                if (!postExecutionControl.updateIsStatusPatch()) {
-                  eventSourceManager
-                      .getControllerResourceEventSource()
-                      .handleRecentResourceUpdate(
-                          ResourceID.fromResource(p), p, executionScope.getResource());
-                }
-              });
       if (state.eventPresent()) {
         submitReconciliationExecution(state);
       } else {

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

@@ -37,7 +37,7 @@ public static <R extends HasMetadata> PostExecutionControl<R> customResourceStat
     return new PostExecutionControl<>(false, updatedCustomResource, true, null);
   }
 
-  public static <R extends HasMetadata> PostExecutionControl<R> customResourceUpdated(
+  public static <R extends HasMetadata> PostExecutionControl<R> customResourcePatched(
       R updatedCustomResource) {
     return new PostExecutionControl<>(false, updatedCustomResource, false, null);
   }