Repository navigation
fix: SSA finalizer add no longer re-creates a deleted primary resource #3660
+290
−25
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -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 AddFinalizerWithSSAIntegrationTest { | ||||||
|
There was a problem hiding this comment. 🔎 Supported by static analysis
Name this integration test
Suggested fix
Committable suggestion
 |
||||||
|
|
||||||
| 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(); | ||||||
| } | ||||||
| } | ||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🔎 Supported by static analysis
🏁 Script executed:
Repository: operator-framework/java-operator-sdk
Length of output: 10911
Do not re-fetch and adopt a replacement resource with a different UID during the finalizer retry.
On a 409,
conflictRetryingreplaces itsresourcevariable with a name-based GET.finalizerApplyForthen copies that object's UID into the SSA request. The UID guard is evaluated only against the original cached resource. After one re-fetch, the same SSA request can be applied to a new object with the same namespace and name, which this reconciliation never observed. The controller's finalizer is then added to that replacement.The correction belongs in the retry routine, not in the finalizer payload. After a re-fetch, the UID of the returned resource must match the UID of the cached primary (
context.getPrimaryResource()). On a mismatch, the routine must apply nothing and returnnull, as theaddFinalizerWithSSAJavadoc states. It must not return the re-fetched object. A precondition-false result currently returnsresource, so a UID check that only fails the precondition would still hand the replacement back toReconciliationDispatcher.🤖 Prompt for AI Agents