resourceChangesOperator, Predicate
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
preCondition, UnaryOperator
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,28 +1261,51 @@ 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. + * + *
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
@@ -1273,7 +1313,7 @@ public P addFinalizerWithSSA(String finalizerName) {
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);
}
}
diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/reconciler/AddFinalizerWithSSAIntegrationIT.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/reconciler/AddFinalizerWithSSAIntegrationIT.java
new file mode 100644
index 0000000000..09db57fe45
--- /dev/null
+++ b/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