Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -751,7 +751,10 @@ public <R extends HasMetadata> R jsonMergePatch(R desired) {
* @return the patched resource as returned by the API server
*/
public <R extends HasMetadata> 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);
}

/**
Expand All @@ -767,7 +770,10 @@ public <R extends HasMetadata> R jsonMergePatch(R desired, Options options) {
public <R extends HasMetadata> R jsonMergePatch(
R desired, InformerEventSource<R, P> 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);
}

/**
Expand Down Expand Up @@ -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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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;

Expand Down Expand Up @@ -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<Object> 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<Object> runUpdateOperation() {
return invocation ->
((UnaryOperator) invocation.getArgument(1)).apply(invocation.getArgument(0));
}
Comment on lines +484 to +489

// ---- update -------------------------------------------------------------

@Test
Expand Down Expand Up @@ -610,7 +618,7 @@ void createWithNullEventSourceFallsBackToForceFilter() {
// ---- patch / status verbs ----------------------------------------------

@Test
void jsonMergePatchCacheOnlyCallsClientPatch() {
void jsonMergePatchCacheOnlyCallsClientMergePatch() {
var resource = TestUtils.testCustomResource1();
var updated = wireVerbMocks();

Expand All @@ -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<TestCustomResource, TestCustomResource> 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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -40,6 +41,8 @@ public class ResourceOperationsReconciler implements Reconciler<ResourceOperatio

public static final String STATUS_VALUE = "reconciled";
public static final String SPEC_MARKER = "applied";
public static final String LABEL_KEY = "resolved";
public static final String LABEL_VALUE = "true";
Comment on lines +44 to +45

public enum Operation {
SSA,
Expand All @@ -49,6 +52,7 @@ public enum Operation {
JSON_PATCH,
JSON_PATCH_STATUS,
JSON_MERGE_PATCH,
JSON_MERGE_PATCH_PARTIAL_METADATA,
JSON_MERGE_PATCH_STATUS
}

Expand Down Expand Up @@ -102,6 +106,18 @@ public UpdateControl<ResourceOperationsCustomResource> 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);
Expand Down
Loading