fix #4861: various changes to refine resourceVersion and replace

Author: shawkins

CHANGELOG.md

@@ -49,6 +49,11 @@
 * Fix #4659: The SupportTestingClient interface has been deprecated.  Please use one of the supports methods or getApiGroup to determine what is available on the api server.
 * Fix #4825: removed or deprecated/moved methods that are unrelated to the rolling timeout from ImageEditReplacePatchable.  Deprecated rollout methods for timeout and edit - future versions will not support
 * Fix #4826: removed RequestConfig upload connection and rolling timeouts.  Both were no longer used with no plans to re-introduce their usage.
+* Fix #4861: several breaking changes related to resourceVersion handling and the replace operation:
+  - replace is deprecated, you should use update instead.  If you set the resourceVersion to null it will not be optimistically locked
+  - json patch methods using an item for the diff generation such as edit or patch will no longer omit the resourceVersion in the patch.  If you want the patch to be unlocked, then set the resourceVersion to null on the item to be patched.
+  - createOrReplace is deprecated, you should use server side apply instead.
+  - internal logic to mimic an apply that modify an item prior to a json patch is deprecated - you should instead build the item to be patched off of base version, such as with the edit method.
 
 ### 6.4.1 (2023-01-31)
 

junit/kubernetes-server-mock/src/test/java/io/fabric8/kubernetes/client/server/mock/crud/KubernetesCrudDispatcherPatchTest.java

@@ -24,6 +24,7 @@
 import io.fabric8.kubernetes.api.model.rbac.ClusterRoleBuilder;
 import io.fabric8.kubernetes.client.KubernetesClient;
 import io.fabric8.kubernetes.client.KubernetesClientException;
+import io.fabric8.kubernetes.client.dsl.NamespaceableResource;
 import io.fabric8.kubernetes.client.dsl.Resource;
 import io.fabric8.kubernetes.client.dsl.base.PatchContext;
 import io.fabric8.kubernetes.client.dsl.base.PatchType;
@@ -325,27 +326,24 @@ class ResourceVersion {
     @Test
     @DisplayName("JSON patch, with different resource version, should patch the resource ---->" +
         "Invalid Client behavior which removes the resource version from the list of operations")
-    // TODO: This test should be converted to differentResourceVersionConflict if we address
-    //       the inconsistent behavior of the client
-    void differentResourceVersionOk() {
+    void differentResourceVersionConflictEdit() {
       // Given
-      final ConfigMap initialCm = client.resource(new ConfigMapBuilder()
+      client.resource(new ConfigMapBuilder()
           .withNewMetadata().withName("json-different-resource-version").endMetadata()
           .addToData("key", "value")
           .build())
           .create();
       // When
-      final ConfigMap patchedCm = client.resource(new ConfigMapBuilder()
+      final NamespaceableResource<ConfigMap> patchedCmOp = client.resource(new ConfigMapBuilder()
           .withNewMetadata().withName("json-different-resource-version").withResourceVersion("different").endMetadata()
           .addToData("key", "changed")
-          .build())
-          .patch();
+          .build());
       // Then
-      assertThat(patchedCm)
-          .hasFieldOrPropertyWithValue("metadata.name", "json-different-resource-version")
-          .extracting("metadata.resourceVersion")
-          .isNotNull()
-          .isNotEqualTo(initialCm.getMetadata().getResourceVersion());
+      assertThatThrownBy(() -> patchedCmOp.patch())
+          .asInstanceOf(InstanceOfAssertFactories.type(KubernetesClientException.class))
+          .hasFieldOrPropertyWithValue("code", 409)
+          .extracting(KubernetesClientException::getMessage).asString()
+          .contains("the object has been modified;");
     }
 
     @Test

kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/dsl/CreateOrReplaceable.java

@@ -22,7 +22,10 @@ public interface CreateOrReplaceable<T> extends Replaceable<T> {
    * fails with a HTTP_CONFLICT, it tries to replace resource.
    *
    * @return created item returned in kubernetes api response
+   *
+   * @deprecated please user {@link ServerSideApplicable#serverSideApply()} or attempt a create and edit/patch operation.
    */
+  @Deprecated
   T createOrReplace();
 
   /**

kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/dsl/EditReplacePatchable.java

@@ -72,6 +72,13 @@ public interface EditReplacePatchable<T>
    * concurrent changes (between when you obtained your item and the current state) in an unexpected way.
    * <p>
    * Consider using edit, which allows for a known base, and a builder instead.
+   * <p>
+   * WARNING: For some resource types there is an attempt to make this operation more like
+   * an apply by considering implicit server side state as not being part of the patch. This behavior will be
+   * removed in future versions, you should instead construct the resource to be patched from a resource obtained
+   * from the api server or use patch method that is tolerant to missing state, or
+   * {@link ServerSideApplicable#serverSideApply()}
+   * <p>
    *
    * @param item to be patched with patched values
    * @return returns deserialized version of api server response
@@ -93,6 +100,13 @@ default T patch(T item) {
    * You may explicitly set the {@link PatchContext#getFieldManager()} as well to override the default.
    * </ul>
    *
+   * WARNING: For a JSON patch and some resource types there is an attempt to make this operation more like
+   * an apply by considering implicit server side state as not being part of the patch. This behavior will be
+   * removed in future versions, you should instead construct the resource to be patched from a resource obtained
+   * from the api server or use patch method that is tolerant to missing state, or
+   * {@link ServerSideApplicable#serverSideApply()}
+   * <p>
+   *
    * @param item to be patched with patched values
    * @param patchContext {@link PatchContext} for patch request
    * @return returns deserialized version of api server response
@@ -149,6 +163,13 @@ default T patch(String patch) {
    * <p>
    * Consider using edit instead.
    *
+   * WARNING: For some resource types there is an attempt to make this operation more like
+   * an apply by considering implicit server side state as not being part of the patch. This behavior will be
+   * removed in future versions, you should instead construct the resource to be patched from a resource obtained
+   * from the api server or use patch method that is tolerant to missing state, or
+   * {@link ServerSideApplicable#serverSideApply()}
+   * <p>
+   *
    * @return returns deserialized version of api server response
    */
   T patch();
@@ -169,6 +190,13 @@ default T patch(String patch) {
    * You may explicitly set the {@link PatchContext#getFieldManager()} as well to override the default.
    * </ul>
    *
+   * WARNING: For a JSON patch and some resource types there is an attempt to make this operation more like
+   * an apply by considering implicit server side state as not being part of the patch. This behavior will be
+   * removed in future versions, you should instead construct the resource to be patched from a resource obtained
+   * from the api server or use patch method that is tolerant to missing state, or
+   * {@link ServerSideApplicable#serverSideApply()}
+   * <p>
+   *
    * @param patchContext {@link PatchContext} for patch request
    * @return returns deserialized version of api server response
    */

kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/dsl/ItemWritableOperation.java

@@ -62,8 +62,7 @@ public interface ItemWritableOperation<T> extends DeletableWithOptions, ItemRepl
    *
    * @param item kubernetes object
    * @return updated object
-   * @deprecated please use one of patchStatus, editStatus, or replaceStatus, or a locked replace
-   *             {@link Resource#lockResourceVersion(String)}
+   * @deprecated please use resource(item).updateStatus();
    */
   @Deprecated
   T updateStatus(T item);

kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/dsl/Replaceable.java

@@ -30,14 +30,38 @@ public interface Replaceable<T> {
    * the latest resourceVersion from the server.
    *
    * @return returns deserialized version of api server response
+   *
+   * @deprecated use {@link #update()} instead
    */
+  @Deprecated
   T replace();
 
   /**
    * Similar to {@link Replaceable#replace()}, but only affects the status subresource
    *
    * @return returns deserialized version of api server response
+   *
+   * @deprecated use {@link #updateStatus()} instead
    */
+  @Deprecated
   T replaceStatus();
 
+  /**
+   * Similar to {@link Replaceable#update()}, but only affects the status subresource
+   *
+   * @return returns deserialized version of api server response
+   */
+  T updateStatus();
+
+  /**
+   * Update the server's state with the given item (PUT).
+   * <p>
+   * If the resourceVersion is on the resource, the update will be performed with optimistic locking, and may
+   * result in a conflict (409 error). If no resourceVersion is on the resource, the latest resourceVersion will
+   * be obtained from the server prior to the update call - which may still be a conflict in a rare circumstance.
+   *
+   * @return returns deserialized version of api server response
+   */
+  T update();
+
 }

kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/extension/ResourceAdapter.java

@@ -330,6 +330,7 @@ public T item() {
     return resource.item();
   }
 
+  @Override
   public DeletableWithOptions withTimeout(long timeout, TimeUnit unit) {
     return resource.withTimeout(timeout, unit);
   }
@@ -339,4 +340,14 @@ public DeletableWithOptions withTimeoutInMillis(long timeoutInMillis) {
     return withTimeout(timeoutInMillis, TimeUnit.MILLISECONDS);
   }
 
+  @Override
+  public T update() {
+    return resource.update();
+  }
+
+  @Override
+  public T updateStatus() {
+    return resource.updateStatus();
+  }
+
 }

kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/BaseOperation.java

@@ -1103,7 +1103,7 @@ public ExtensibleResource<T> lockResourceVersion() {
 
   @Override
   public T updateStatus(T item) {
-    return resource(item).lockResourceVersion().replaceStatus();
+    return resource(item).updateStatus();
   }
 
   @Override
@@ -1140,4 +1140,14 @@ public ExtensibleResource<T> withTimeout(long timeout, TimeUnit unit) {
     return newInstance(context.withTimeout(timeout, unit));
   }
 
+  @Override
+  public T updateStatus() {
+    throw new KubernetesClientException(READ_ONLY_UPDATE_EXCEPTION_MESSAGE);
+  }
+
+  @Override
+  public T update() {
+    throw new KubernetesClientException(READ_ONLY_UPDATE_EXCEPTION_MESSAGE);
+  }
+
 }

kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/HasMetadataOperation.java

@@ -42,6 +42,7 @@ public class HasMetadataOperation<T extends HasMetadata, L extends KubernetesRes
   public static final long DEFAULT_GRACE_PERIOD_IN_SECONDS = -1L;
   private static final String PATCH_OPERATION = "patch";
   private static final String REPLACE_OPERATION = "replace";
+  private static final String UPDATE_OPERATION = "update";
 
   public HasMetadataOperation(OperationContext ctx, Class<T> type, Class<L> listType) {
     super(ctx);
@@ -103,6 +104,36 @@ protected T modifyItemForReplaceOrPatch(Supplier<T> current, T item) {
     return item;
   }
 
+  @Override
+  public T update() {
+    return this.update(getItem(), false);
+  }
+
+  @Override
+  public T updateStatus() {
+    return this.update(getItem(), true);
+  }
+
+  protected T update(T item, boolean status) {
+    String existingResourceVersion = KubernetesResourceUtil.getResourceVersion(item);
+    try {
+      if (existingResourceVersion == null) {
+        T got = requireFromServer();
+        String resourceVersion = KubernetesResourceUtil.getResourceVersion(got);
+        item = clone(item);
+        item.getMetadata().setResourceVersion(resourceVersion);
+      }
+      return handleUpdate(item, status);
+    } catch (KubernetesClientException e) {
+      throw KubernetesClientException.launderThrowable(forOperationType(UPDATE_OPERATION), e);
+    } catch (InterruptedException e) {
+      Thread.currentThread().interrupt();
+      throw KubernetesClientException.launderThrowable(forOperationType(UPDATE_OPERATION), e);
+    } catch (IOException e) {
+      throw KubernetesClientException.launderThrowable(forOperationType(UPDATE_OPERATION), e);
+    }
+  }
+
   /**
    * base replace operation, which is effectively a forced update with retries
    */
@@ -171,17 +202,10 @@ protected T replace(T item, boolean status) {
    * be fetched from the server.
    */
   protected T patch(PatchContext context, T base, T item, boolean status) {
-    if (context == null || context.getPatchType() == PatchType.JSON) {
+    if ((context == null || context.getPatchType() == PatchType.JSON) && base == null) {
       if (base == null) {
         base = requireFromServer();
       }
-      if (base.getMetadata() != null) {
-        // prevent the resourceVersion from being modified in the patch
-        if (item.getMetadata() == null) {
-          item.setMetadata(new ObjectMeta());
-        }
-        item.getMetadata().setResourceVersion(base.getMetadata().getResourceVersion());
-      }
       final T current = base;
       try {
         item = modifyItemForReplaceOrPatch(() -> current, item);

kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableListImpl.java

@@ -322,4 +322,14 @@ public DeletableWithOptions withTimeoutInMillis(long timeoutInMillis) {
     return this.withTimeout(timeoutInMillis, TimeUnit.MILLISECONDS);
   }
 
+  @Override
+  public List<HasMetadata> updateStatus() {
+    return performOperation(Resource::updateStatus);
+  }
+
+  @Override
+  public List<HasMetadata> update() {
+    return performOperation(Resource::update);
+  }
+
 }