From 883f8f84a842044bb18ba2ac24058b56e2bbd0c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Mon, 27 Jul 2026 18:19:57 +0200 Subject: [PATCH] fix: use explicit JSON merge patch type in merge patch operations Fabric8's no-context patch() fetches the resource from the server and computes an RFC 6902 JSON Patch diff, which removes the fields omitted from a partial resource (including the whole spec). Pass an explicit JSON_MERGE PatchContext so the non-status merge patch operations really send application/merge-patch+json. --- .../api/reconciler/ResourceOperations.java | 12 +++- .../reconciler/ResourceOperationsTest.java | 64 +++++++++++++++++-- .../ResourceOperationsIT.java | 27 ++++++++ .../ResourceOperationsReconciler.java | 16 +++++ 4 files changed, 110 insertions(+), 9 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ResourceOperations.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ResourceOperations.java index c4532aa284..603548e33d 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ResourceOperations.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ResourceOperations.java @@ -751,7 +751,10 @@ public R jsonMergePatch(R desired) { * @return the patched resource as returned by the API server */ public R jsonMergePatch(R desired, Options options) { - return resourcePatch(desired, r -> context.getClient().resource(r).patch(), options); + return resourcePatch( + desired, + r -> context.getClient().resource(r).patch(PatchContext.of(PatchType.JSON_MERGE)), + options); } /** @@ -767,7 +770,10 @@ public R jsonMergePatch(R desired, Options options) { public R jsonMergePatch( R desired, InformerEventSource informerEventSource, Options options) { return resourcePatch( - desired, r -> context.getClient().resource(r).patch(), informerEventSource, options); + desired, + r -> context.getClient().resource(r).patch(PatchContext.of(PatchType.JSON_MERGE)), + informerEventSource, + options); } /** @@ -838,7 +844,7 @@ public P jsonMergePatchPrimary(P resource) { public P jsonMergePatchPrimary(P resource, Options options) { return resourcePatch( resource, - r -> context.getClient().resource(r).patch(), + r -> context.getClient().resource(r).patch(PatchContext.of(PatchType.JSON_MERGE)), context.eventSourceRetriever().getControllerEventSource(), options); } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/reconciler/ResourceOperationsTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/reconciler/ResourceOperationsTest.java index f5b010d21c..95c1b1815e 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/reconciler/ResourceOperationsTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/reconciler/ResourceOperationsTest.java @@ -22,6 +22,7 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.mockito.ArgumentCaptor; import org.mockito.stubbing.Answer; import io.fabric8.kubernetes.api.model.HasMetadata; @@ -31,6 +32,7 @@ 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; import io.fabric8.kubernetes.client.utils.KubernetesSerialization; import io.javaoperatorsdk.operator.TestUtils; import io.javaoperatorsdk.operator.api.config.Cloner; @@ -40,6 +42,7 @@ import io.javaoperatorsdk.operator.processing.event.EventSourceRetriever; import io.javaoperatorsdk.operator.processing.event.source.EventSource; import io.javaoperatorsdk.operator.processing.event.source.controller.ControllerEventSource; +import io.javaoperatorsdk.operator.processing.event.source.informer.InformerEventSource; import io.javaoperatorsdk.operator.processing.event.source.informer.ManagedInformerEventSource; import io.javaoperatorsdk.operator.sample.simple.TestCustomResource; @@ -471,15 +474,20 @@ private TestCustomResource wireVerbMocks() { when(verbClientResource.patch(any(PatchContext.class))).thenReturn(updated); // both cache paths execute the update operation so the underlying client verb runs - Answer runOperation = - invocation -> ((UnaryOperator) invocation.getArgument(1)).apply(invocation.getArgument(0)); when(verbEventSource.eventFilteringUpdateAndCacheResource(any(), any(UnaryOperator.class))) - .thenAnswer(runOperation); + .thenAnswer(runUpdateOperation()); when(verbEventSource.updateAndCacheResource(any(), any(UnaryOperator.class))) - .thenAnswer(runOperation); + .thenAnswer(runUpdateOperation()); return updated; } + /** Answer that executes the update operation passed to the caching methods of an event source. */ + @SuppressWarnings("rawtypes") + private static Answer runUpdateOperation() { + return invocation -> + ((UnaryOperator) invocation.getArgument(1)).apply(invocation.getArgument(0)); + } + // ---- update ------------------------------------------------------------- @Test @@ -610,7 +618,7 @@ void createWithNullEventSourceFallsBackToForceFilter() { // ---- patch / status verbs ---------------------------------------------- @Test - void jsonMergePatchCacheOnlyCallsClientPatch() { + void jsonMergePatchCacheOnlyCallsClientMergePatch() { var resource = TestUtils.testCustomResource1(); var updated = wireVerbMocks(); @@ -620,7 +628,51 @@ void jsonMergePatchCacheOnlyCallsClientPatch() { assertThat(result).isSameAs(updated); verify(verbEventSource, times(1)) .updateAndCacheResource(eq(resource), any(UnaryOperator.class)); - verify(verbClientResource, times(1)).patch(); + assertThat(capturedPatchType()).isEqualTo(PatchType.JSON_MERGE); + } + + @Test + void jsonMergePatchWithEventSourceCallsClientMergePatch() { + var resource = TestUtils.testCustomResource1(); + var updated = wireVerbMocks(); + InformerEventSource informerEventSource = + mock(InformerEventSource.class); + when(informerEventSource.updateAndCacheResource(any(), any(UnaryOperator.class))) + .thenAnswer(runUpdateOperation()); + + var result = + resourceOperations.jsonMergePatch( + resource, informerEventSource, ResourceOperations.Options.cacheOnly()); + + assertThat(result).isSameAs(updated); + assertThat(capturedPatchType()).isEqualTo(PatchType.JSON_MERGE); + } + + @Test + void jsonMergePatchPrimaryCallsClientMergePatch() { + var resource = TestUtils.testCustomResource1(); + var updated = wireVerbMocks(); + when(controllerEventSource.updateAndCacheResource(any(), any(UnaryOperator.class))) + .thenAnswer(runUpdateOperation()); + + var result = + resourceOperations.jsonMergePatchPrimary(resource, ResourceOperations.Options.cacheOnly()); + + assertThat(result).isSameAs(updated); + assertThat(capturedPatchType()).isEqualTo(PatchType.JSON_MERGE); + } + + /** + * Asserts that a merge patch operation went through Fabric8 with an explicit patch context and + * returns the used patch type. The no-context {@code patch()} must never be used for merge + * patches: it computes an RFC 6902 diff against the resource fetched from the server, which + * removes the fields missing from a partial resource. + */ + private PatchType capturedPatchType() { + verify(verbClientResource, never()).patch(); + var captor = ArgumentCaptor.forClass(PatchContext.class); + verify(verbClientResource, times(1)).patch(captor.capture()); + return captor.getValue().getPatchType(); } @Test diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/resourceoperations/ResourceOperationsIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/resourceoperations/ResourceOperationsIT.java index d655ef7e4f..e8875bbefd 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/resourceoperations/ResourceOperationsIT.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/resourceoperations/ResourceOperationsIT.java @@ -24,6 +24,8 @@ import io.javaoperatorsdk.operator.baseapi.resourceoperations.ResourceOperationsReconciler.Operation; import io.javaoperatorsdk.operator.junit.LocallyRunOperatorExtension; +import static io.javaoperatorsdk.operator.baseapi.resourceoperations.ResourceOperationsReconciler.LABEL_KEY; +import static io.javaoperatorsdk.operator.baseapi.resourceoperations.ResourceOperationsReconciler.LABEL_VALUE; import static io.javaoperatorsdk.operator.baseapi.resourceoperations.ResourceOperationsReconciler.SPEC_MARKER; import static io.javaoperatorsdk.operator.baseapi.resourceoperations.ResourceOperationsReconciler.STATUS_VALUE; import static org.assertj.core.api.Assertions.assertThat; @@ -86,6 +88,31 @@ void jsonMergePatch() { assertSpecOperation(Operation.JSON_MERGE_PATCH); } + /** + * A merge patch applied with a partial resource must only change what the partial resource + * contains; fields omitted from it - like the whole {@code spec} - have to survive on the server. + */ + @Test + void jsonMergePatchWithPartialResourceKeepsOmittedFields() { + var operation = Operation.JSON_MERGE_PATCH_PARTIAL_METADATA; + runOperation(operation); + + await() + .atMost(Duration.ofSeconds(30)) + .untilAsserted( + () -> + assertThat(actual().getMetadata().getLabels()) + .as("the partial metadata merge patch should be applied") + .containsEntry(LABEL_KEY, LABEL_VALUE)); + + assertThat(actual().getSpec()) + .as("the spec omitted from the partial merge patch should not be removed") + .isNotNull() + .extracting(ResourceOperationsSpec::getValue) + .isEqualTo(INITIAL_SPEC_VALUE); + assertConvergesWithoutLooping(operation); + } + @Test void jsonMergePatchStatus() { assertStatusOperation(Operation.JSON_MERGE_PATCH_STATUS); diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/resourceoperations/ResourceOperationsReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/resourceoperations/ResourceOperationsReconciler.java index bbfb7cd098..493c06826b 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/resourceoperations/ResourceOperationsReconciler.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/resourceoperations/ResourceOperationsReconciler.java @@ -15,6 +15,7 @@ */ package io.javaoperatorsdk.operator.baseapi.resourceoperations; +import java.util.Map; import java.util.concurrent.atomic.AtomicInteger; import io.fabric8.kubernetes.api.model.ObjectMetaBuilder; @@ -40,6 +41,8 @@ public class ResourceOperationsReconciler implements Reconciler reconcile( markResource(resource); ops.jsonMergePatchPrimary(resource); } + // a partial resource, holding only the metadata to change: a merge patch must not touch the + // fields omitted from it (like spec) + case JSON_MERGE_PATCH_PARTIAL_METADATA -> { + ResourceOperationsCustomResource partial = new ResourceOperationsCustomResource(); + partial.setMetadata( + new ObjectMetaBuilder() + .withName(resource.getMetadata().getName()) + .withNamespace(resource.getMetadata().getNamespace()) + .withLabels(Map.of(LABEL_KEY, LABEL_VALUE)) + .build()); + ops.jsonMergePatchPrimary(partial); + } case JSON_MERGE_PATCH_STATUS -> { resource.setStatus(new ResourceOperationsStatus().setValue(STATUS_VALUE)); ops.jsonMergePatchPrimaryStatus(resource);