fix: SSA finalizer add no longer re-creates a deleted primary resource

docs/content/en/docs/documentation/reconciler.md

@@ -157,6 +157,8 @@ You can specify the name of the finalizer to use for your `Reconciler` using the
 annotation. If you do not specify a finalizer name, one will be automatically generated for you.
 
 From v5, by default, the finalizer is added using Server Side Apply. See also `UpdateControl` in docs.
+The apply carries the resource's UID, so a resource deleted before the finalizer was added is not
+re-created: the API server rejects the apply, and the framework skips adding the finalizer.
 
 ### Read-cache-after-write consistency and event filtering
 

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

@@ -16,7 +16,9 @@
 package io.javaoperatorsdk.operator.api.reconciler;
 
 import java.lang.reflect.InvocationTargetException;
+import java.net.HttpURLConnection;
 import java.util.Optional;
+import java.util.function.IntPredicate;
 import java.util.function.Predicate;
 import java.util.function.UnaryOperator;
 
@@ -1170,9 +1172,34 @@ public P conflictRetryingPatchPrimary(
    * @return updated resource from the server or unchanged if the precondition does not hold
    * @throws OperatorException if the maximum number of retry attempts is exceeded
    */
-  @SuppressWarnings("unchecked")
   public P conflictRetryingPatchPrimary(
       UnaryOperator<P> resourceChangesOperator, Predicate<P> preCondition, boolean cacheOnly) {
+    return conflictRetrying(
+        preCondition,
+        r ->
+            jsonPatchPrimary(
+                r,
+                resourceChangesOperator,
+                cacheOnly
+                    ? Options.cacheOnly()
+                    : Options.matchAndFilterWithDefaultMatcher(UpdateType.JSON_PATCH)),
+        // also retry on unprocessable content (422) which can happen if JSON Patch is not a valid
+        // request since there was a concurrent request which already removed another finalizer:
+        // List element removal from a list is by index in JSON Patch so if addressing a second
+        // finalizer but first is meanwhile removed it is a wrong request.
+        code -> code == HttpURLConnection.HTTP_CONFLICT || code == 422);
+  }
+
+  /**
+   * Runs the given write against the primary resource, re-fetching the resource and retrying when
+   * the server responds with one of the retried HTTP codes, up to {@link
+   * ResourceOperations#DEFAULT_MAX_RETRY} times. The pre-condition is evaluated against the
+   * (possibly re-fetched) resource before each attempt; a re-fetched resource is {@code null} if it
+   * no longer exists.
+   */
+  @SuppressWarnings("unchecked")
+  private P conflictRetrying(
+      Predicate<P> preCondition, UnaryOperator<P> write, IntPredicate retriedCode) {
     var resource = context.getPrimaryResource();
     var client = context.getClient();
     if (log.isDebugEnabled()) {
@@ -1184,22 +1211,11 @@ public P conflictRetryingPatchPrimary(
         if (!preCondition.test(resource)) {
           return resource;
         }
-        return jsonPatchPrimary(
-            resource,
-            resourceChangesOperator,
-            cacheOnly
-                ? Options.cacheOnly()
-                : Options.matchAndFilterWithDefaultMatcher(UpdateType.JSON_PATCH));
+        return write.apply(resource);
       } catch (KubernetesClientException e) {
         log.trace("Exception during patch for resource: {}", resource);
         retryIndex++;
-        // only retry on conflict (409) and unprocessable content (422) which
-        // can happen if JSON Patch is not a valid request since there was
-        // a concurrent request which already removed another finalizer:
-        // List element removal from a list is by index in JSON Patch
-        // so if addressing a second finalizer but first is meanwhile removed
-        // it is a wrong request.
-        if (e.getCode() != 409 && e.getCode() != 422) {
+        if (!retriedCode.test(e.getCode())) {
           throw e;
         }
         if (retryIndex >= DEFAULT_MAX_RETRY) {
@@ -1235,7 +1251,8 @@ public P conflictRetryingPatchPrimary(
    * String)} with the configured finalizer name. Note that explicitly adding finalizer is required
    * only if "Trigger reconciliation on all event" mode is on.
    *
-   * @return the patched resource from the server response
+   * @return the patched resource from the server response, the resource unchanged if the finalizer
+   *     does not need to be added, or {@code null} if the resource no longer exists
    * @see #addFinalizerWithSSA(String)
    */
   public P addFinalizerWithSSA() {
@@ -1244,36 +1261,59 @@ public P addFinalizerWithSSA() {
 
   /**
    * Adds finalizer using Server-Side Apply. In the background this method creates a fresh copy of
-   * the target resource, setting only name, namespace and finalizer. Does not use optimistic
-   * locking for the patch. Note that explicitly adding finalizer is required only if "Trigger
-   * reconciliation on all event" mode is on.
+   * the target resource, setting only name, namespace, UID and finalizer. It does not try to add
+   * the finalizer if there is already a finalizer or the resource is marked for deletion. Note that
+   * explicitly adding finalizer is required only if "Trigger reconciliation on all event" mode is
+   * on.
+   *
+   * <p>A Server-Side Apply creates the resource if it does not exist, so without further
+   * precautions the finalizer add would re-create a resource deleted after it was cached. The UID
+   * makes the API server reject the apply instead: with a conflict (HTTP 409) if the resource no
+   * longer exists, and with unprocessable content (HTTP 422) if it was re-created under the same
+   * name. In both cases the resource is re-fetched and the apply retried, up to {@link
+   * ResourceOperations#DEFAULT_MAX_RETRY} times; if the resource no longer exists, nothing is
+   * applied and {@code null} is returned. No optimistic locking is used.
    *
    * @param finalizerName name of the finalizer to add
-   * @return the patched resource from the server response
+   * @return the patched resource from the server response, the resource unchanged if the finalizer
+   *     does not need to be added, or {@code null} if the resource no longer exists
    */
   public P addFinalizerWithSSA(String finalizerName) {
-    var originalResource = context.getPrimaryResource();
     if (log.isDebugEnabled()) {
+      var originalResource = context.getPrimaryResource();
       log.debug(
           "Adding finalizer (using SSA) for resource: {} version: {}",
           getUID(originalResource),
           getVersion(originalResource));
     }
+    return conflictRetrying(
+        r -> {
+          if (r == null) {
+            log.warn("Cannot add finalizer since resource no longer exists.");
+            return false;
+          }
+          return !r.isMarkedForDeletion() && !r.hasFinalizer(finalizerName);
+        },
+        r -> serverSideApplyPrimary(finalizerApplyFor(r, finalizerName), Options.cacheOnly()),
+        code -> code == HttpURLConnection.HTTP_CONFLICT || code == 422);
+  }
+
+  private P finalizerApplyFor(P actualResource, String finalizerName) {
     try {
       @SuppressWarnings("unchecked")
-      P resource = (P) originalResource.getClass().getConstructor().newInstance();
-      resource.initNameAndNamespaceFrom(originalResource);
+      P resource = (P) actualResource.getClass().getConstructor().newInstance();
+      resource.initNameAndNamespaceFrom(actualResource);
+      resource.getMetadata().setUid(actualResource.getMetadata().getUid());
       resource.addFinalizer(finalizerName);
-
-      return serverSideApplyPrimary(resource, Options.cacheOnly());
+      return resource;
     } catch (InstantiationException
         | IllegalAccessException
         | InvocationTargetException
         | NoSuchMethodException e) {
       throw new RuntimeException(
           "Issue with creating custom resource instance with reflection."
               + " Custom Resources must provide a no-arg constructor. Class: "
-              + originalResource.getClass().getName(),
+              + actualResource.getClass().getName(),
           e);
     }
   }

operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/reconciler/AddFinalizerWithSSAIntegrationIT.java

@@ -0,0 +1,151 @@
+/*
+ * Copyright Java Operator SDK Authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *         http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package io.javaoperatorsdk.operator.api.reconciler;
+
+import java.util.Map;
+import java.util.function.UnaryOperator;
+
+import org.junit.jupiter.api.Test;
+
+import io.fabric8.kubeapitest.junit.EnableKubeAPIServer;
+import io.fabric8.kubernetes.api.model.ConfigMap;
+import io.fabric8.kubernetes.api.model.ConfigMapBuilder;
+import io.fabric8.kubernetes.client.KubernetesClient;
+import io.javaoperatorsdk.operator.api.config.ControllerConfiguration;
+import io.javaoperatorsdk.operator.processing.event.EventSourceRetriever;
+import io.javaoperatorsdk.operator.processing.event.source.controller.ControllerEventSource;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+/**
+ * Adds the finalizer with Server-Side Apply against a real API server, which is what decides
+ * whether the apply creates a missing resource or rejects it.
+ */
+@EnableKubeAPIServer
+class AddFinalizerWithSSAIntegrationIT {
+
+  private static final String NAMESPACE = "default";
+  private static final String FINALIZER = "int.test/finalizer";
+
+  static KubernetesClient client;
+
+  @Test
+  void addsFinalizerToExistingResource() {
+    var cached = createConfigMap("existing");
+
+    var result = resourceOperationsFor(cached).addFinalizerWithSSA(FINALIZER);
+
+    var actual = get("existing");
+    assertThat(result.getMetadata().getFinalizers()).containsExactly(FINALIZER);
+    assertThat(actual.getMetadata().getFinalizers()).containsExactly(FINALIZER);
+    assertThat(actual.getData()).containsEntry("key", "value");
+    removeFinalizerAndDelete(actual);
+  }
+
+  @Test
+  void doesNotRecreateDeletedResource() {
+    var cached = createConfigMap("deleted");
+    client.resource(cached).delete();
+
+    var result = resourceOperationsFor(cached).addFinalizerWithSSA(FINALIZER);
+
+    assertThat(result).isNull();
+    assertThat(get("deleted")).isNull();
+  }
+
+  @Test
+  void addsFinalizerToResourceRecreatedWithSameName() {
+    var cached = createConfigMap("recreated");
+    client.resource(cached).delete();
+    var recreated = createConfigMap("recreated");
+
+    var result = resourceOperationsFor(cached).addFinalizerWithSSA(FINALIZER);
+
+    var actual = get("recreated");
+    assertThat(result.getMetadata().getUid()).isEqualTo(recreated.getMetadata().getUid());
+    assertThat(actual.getMetadata().getUid()).isEqualTo(recreated.getMetadata().getUid());
+    assertThat(actual.getMetadata().getFinalizers()).containsExactly(FINALIZER);
+    removeFinalizerAndDelete(actual);
+  }
+
+  @Test
+  void addsFinalizerUsingStaleCachedResource() {
+    var cached = createConfigMap("stale");
+    var updated = get("stale");
+    updated.setData(Map.of("key", "updated"));
+    client.resource(updated).update();
+
+    resourceOperationsFor(cached).addFinalizerWithSSA(FINALIZER);
+
+    var actual = get("stale");
+    assertThat(actual.getMetadata().getFinalizers()).containsExactly(FINALIZER);
+    assertThat(actual.getData()).containsEntry("key", "updated");
+    removeFinalizerAndDelete(actual);
+  }
+
+  /**
+   * Only the framework plumbing around the API call is mocked: the controller event source just
+   * runs the write it is given, so the apply and the re-fetch on retry hit the real API server.
+   */
+  @SuppressWarnings("unchecked")
+  private static ResourceOperations<ConfigMap> resourceOperationsFor(ConfigMap primary) {
+    Context<ConfigMap> context = mock(Context.class);
+    ControllerConfiguration<ConfigMap> controllerConfiguration =
+        mock(ControllerConfiguration.class);
+    var eventSourceRetriever = mock(EventSourceRetriever.class);
+    ControllerEventSource<ConfigMap> controllerEventSource = mock(ControllerEventSource.class);
+
+    when(context.getClient()).thenReturn(client);
+    when(context.getPrimaryResource()).thenReturn(primary);
+    when(context.getControllerConfiguration()).thenReturn(controllerConfiguration);
+    when(controllerConfiguration.fieldManager()).thenReturn("finalizer-test");
+    when(context.eventSourceRetriever()).thenReturn(eventSourceRetriever);
+    when(eventSourceRetriever.getControllerEventSource()).thenReturn(controllerEventSource);
+    when(controllerEventSource.updateAndCacheResource(any(), any(UnaryOperator.class)))
+        .thenAnswer(
+            invocation -> {
+              UnaryOperator<ConfigMap> write = invocation.getArgument(1);
+              return write.apply(invocation.getArgument(0));
+            });
+    return new ResourceOperations<>(context);
+  }
+
+  private static ConfigMap createConfigMap(String name) {
+    return client
+        .resource(
+            new ConfigMapBuilder()
+                .withNewMetadata()
+                .withName(name)
+                .withNamespace(NAMESPACE)
+                .endMetadata()
+                .withData(Map.of("key", "value"))
+                .build())
+        .create();
+  }
+
+  private static ConfigMap get(String name) {
+    return client.configMaps().inNamespace(NAMESPACE).withName(name).get();
+  }
+
+  private static void removeFinalizerAndDelete(ConfigMap configMap) {
+    configMap.getMetadata().getFinalizers().clear();
+    client.resource(configMap).update();
+    client.resource(configMap).delete();
+  }
+}

operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/reconciler/ResourceOperationsTest.java

@@ -156,6 +156,78 @@ void addsFinalizerWithSSA() {
     verify(controllerEventSource, times(1)).updateAndCacheResource(any(), any(UnaryOperator.class));
   }
 
+  @Test
+  void ssaFinalizerAddSendsUidWithoutResourceVersion() {
+    var resource = TestUtils.testCustomResource1();
+    resource.getMetadata().setUid("uid-1");
+    resource.getMetadata().setResourceVersion("1");
+    when(context.getPrimaryResource()).thenReturn(resource);
+    var applied = ArgumentCaptor.forClass(TestCustomResource.class);
+    when(controllerEventSource.updateAndCacheResource(applied.capture(), any(UnaryOperator.class)))
+        .thenAnswer(invocation -> invocation.getArgument(0));
+
+    resourceOperations.addFinalizerWithSSA(FINALIZER_NAME);
+
+    var metadata = applied.getValue().getMetadata();
+    assertThat(metadata.getName()).isEqualTo(resource.getMetadata().getName());
+    assertThat(metadata.getUid()).isEqualTo("uid-1");
+    assertThat(metadata.getResourceVersion()).isNull();
+    assertThat(metadata.getFinalizers()).containsExactly(FINALIZER_NAME);
+  }
+
+  @Test
+  void ssaFinalizerAddDoesNotRecreateDeletedResource() {
+    var resource = TestUtils.testCustomResource1();
+    resource.getMetadata().setUid("uid-1");
+    when(context.getPrimaryResource()).thenReturn(resource);
+    // with the UID set the API server rejects the apply of a missing resource with a conflict
+    when(controllerEventSource.updateAndCacheResource(any(), any(UnaryOperator.class)))
+        .thenThrow(new KubernetesClientException("Conflict", 409, null));
+    when(resourceOp.get()).thenReturn(null);
+
+    var result = resourceOperations.addFinalizerWithSSA(FINALIZER_NAME);
+
+    assertThat(result).isNull();
+    verify(controllerEventSource, times(1)).updateAndCacheResource(any(), any(UnaryOperator.class));
+    verify(resourceOp, times(1)).get();
+  }
+
+  @Test
+  void ssaFinalizerAddRetriesWithUidOfRecreatedResource() {
+    var resource = TestUtils.testCustomResource1();
+    resource.getMetadata().setUid("uid-1");
+    when(context.getPrimaryResource()).thenReturn(resource);
+    var recreatedResource = TestUtils.testCustomResource1();
+    recreatedResource.getMetadata().setUid("uid-2");
+    when(resourceOp.get()).thenReturn(recreatedResource);
+    var applied = ArgumentCaptor.forClass(TestCustomResource.class);
+    // a UID differing from the existing resource's is rejected as an immutable field change
+    when(controllerEventSource.updateAndCacheResource(applied.capture(), any(UnaryOperator.class)))
+        .thenThrow(new KubernetesClientException("Unprocessable", 422, null))
+        .thenAnswer(invocation -> invocation.getArgument(0));
+
+    var result = resourceOperations.addFinalizerWithSSA(FINALIZER_NAME);
+
+    assertThat(result.hasFinalizer(FINALIZER_NAME)).isTrue();
+    assertThat(applied.getAllValues())
+        .extracting(r -> r.getMetadata().getUid())
+        .containsExactly("uid-1", "uid-2");
+    verify(resourceOp, times(1)).get();
+  }
+
+  @Test
+  void ssaFinalizerAddSkipsResourceThatAlreadyHasFinalizer() {
+    var resource = TestUtils.testCustomResource1();
+    resource.getMetadata().setResourceVersion("1");
+    resource.addFinalizer(FINALIZER_NAME);
+    when(context.getPrimaryResource()).thenReturn(resource);
+
+    var result = resourceOperations.addFinalizerWithSSA(FINALIZER_NAME);
+
+    assertThat(result).isSameAs(resource);
+    verify(controllerEventSource, never()).updateAndCacheResource(any(), any(UnaryOperator.class));
+  }
+
   @Test
   void removesFinalizer() {
     var resource = TestUtils.testCustomResource1();