-
Notifications
You must be signed in to change notification settings - Fork 242
feat: detect dependent resource API version changes #3536
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: next
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,7 @@ | |
| */ | ||
| package io.javaoperatorsdk.operator.processing.dependent.kubernetes; | ||
|
|
||
| import java.util.LinkedHashMap; | ||
| import java.util.Map; | ||
| import java.util.Objects; | ||
| import java.util.Optional; | ||
|
|
@@ -52,6 +53,15 @@ | |
|
|
||
| private static final Logger log = LoggerFactory.getLogger(KubernetesDependentResource.class); | ||
|
|
||
| /** | ||
| * Annotation used to record the API version the operator applied to this resource, when {@link | ||
| * KubernetesDependentResourceConfig#detectApiVersionChange()} is enabled. | ||
| * | ||
| * @see KubernetesDependent#detectApiVersionChange() | ||
| */ | ||
| public static final String LAST_APPLIED_API_VERSION_ANNOTATION_KEY = | ||
| "javaoperatorsdk.io/last-applied-api-version"; | ||
|
|
||
| private final boolean garbageCollected = this instanceof GarbageCollected; | ||
| private KubernetesDependentResourceConfig<R> kubernetesDependentResourceConfig; | ||
| private volatile Boolean useSSA; | ||
|
|
@@ -160,6 +170,12 @@ | |
|
|
||
| protected void addMetadata( | ||
| boolean forMatch, R actualResource, final R target, P primary, Context<P> context) { | ||
| if (kubernetesDependentResourceConfig != null | ||
| && kubernetesDependentResourceConfig.detectApiVersionChange()) { | ||
| // desired resources might expose a null or immutable annotations map (e.g. Map.of(...)); | ||
| // make sure it's a mutable one before this method or its callees write to it | ||
| ensureMutableAnnotations(target); | ||
| } | ||
| if (forMatch) { // keep the current previous annotation | ||
| String actual = | ||
| actualResource | ||
|
|
@@ -173,9 +189,36 @@ | |
| annotations.remove(InformerEventSource.PREVIOUS_ANNOTATION_KEY); | ||
| } | ||
| } | ||
| addLastAppliedApiVersion(target); | ||
| addReferenceHandlingMetadata(target, primary); | ||
| } | ||
|
|
||
| private static void ensureMutableAnnotations(HasMetadata target) { | ||
| var metadata = target.getMetadata(); | ||
| metadata.setAnnotations( | ||
| new LinkedHashMap<>(Optional.ofNullable(metadata.getAnnotations()).orElseGet(Map::of))); | ||
| } | ||
|
|
||
| /** | ||
| * When {@link KubernetesDependentResourceConfig#detectApiVersionChange()} is enabled, marks the | ||
| * target resource with the API version the operator is currently applying. Comparing this marker | ||
| * with the one recorded on the actual resource lets the regular matching logic detect a mismatch, | ||
| * without ever inspecting the actual, potentially unreliable, stored API version. | ||
| */ | ||
| private void addLastAppliedApiVersion(R target) { | ||
| if (kubernetesDependentResourceConfig == null | ||
| || !kubernetesDependentResourceConfig.detectApiVersionChange()) { | ||
| return; | ||
| } | ||
| var apiVersion = target.getApiVersion(); | ||
| if (apiVersion != null) { | ||
| target | ||
| .getMetadata() | ||
| .getAnnotations() | ||
| .put(LAST_APPLIED_API_VERSION_ANNOTATION_KEY, apiVersion); | ||
| } | ||
| } | ||
|
Comment on lines
+208
to
+220
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added |
||
|
|
||
| protected boolean useSSA(Context<P> context) { | ||
| if (useSSA == null) { | ||
| useSSA = | ||
|
|
@@ -220,7 +263,7 @@ | |
| configBuilder.updateFrom(kubernetesDependentResourceConfig.informerConfig()); | ||
| } | ||
|
|
||
| var es = new InformerEventSource<>(configBuilder.build(), context); | ||
|
Check warning on line 266 in operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java
|
||
| setEventSource(es); | ||
| return eventSource().orElseThrow(); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,113 @@ | ||
| /* | ||
| * 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.processing.dependent.kubernetes; | ||
|
|
||
| import java.util.Set; | ||
|
|
||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| import io.fabric8.kubernetes.api.model.ConfigMap; | ||
| import io.fabric8.kubernetes.api.model.GenericKubernetesResource; | ||
| import io.javaoperatorsdk.operator.api.config.ConfigurationService; | ||
| import io.javaoperatorsdk.operator.api.config.ControllerConfiguration; | ||
| import io.javaoperatorsdk.operator.api.config.dependent.DependentResourceSpec; | ||
| import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResourceFactory; | ||
| import io.javaoperatorsdk.operator.api.reconciler.dependent.GarbageCollected; | ||
|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
| import static org.mockito.Mockito.mock; | ||
| import static org.mockito.Mockito.when; | ||
|
|
||
| /** | ||
| * Focused unit test for the {@code detectApiVersionChange} wiring performed by {@link | ||
| * KubernetesDependentConverter}, independent of the shared, process-wide {@link | ||
| * io.javaoperatorsdk.operator.api.config.dependent.DependentResourceConfigurationResolver} state | ||
| * that other tests in this module mutate. | ||
| */ | ||
| class KubernetesDependentConverterTest { | ||
|
|
||
| private final KubernetesDependentConverter<GenericKubernetesResource, ConfigMap> converter = | ||
| new KubernetesDependentConverter<>(); | ||
|
|
||
| @Test | ||
| void detectApiVersionChangeDefaultsToFalseWhenAnnotationAbsent() { | ||
| var config = | ||
| converter.configFrom(null, spec(PlainWidgetDependentResource.class), controllerConfig()); | ||
|
|
||
| assertThat(config.detectApiVersionChange()).isFalse(); | ||
| } | ||
|
|
||
| @Test | ||
| void detectApiVersionChangeDefaultsToFalseWhenNotSetOnAnnotation() { | ||
| var annotation = PlainWidgetDependentResource.class.getAnnotation(KubernetesDependent.class); | ||
| var config = | ||
| converter.configFrom( | ||
| annotation, spec(PlainWidgetDependentResource.class), controllerConfig()); | ||
|
|
||
| assertThat(config.detectApiVersionChange()).isFalse(); | ||
| } | ||
|
|
||
| @Test | ||
| void detectApiVersionChangeCanBeEnabledViaAnnotation() { | ||
| var annotation = | ||
| ApiVersionAwareWidgetDependentResource.class.getAnnotation(KubernetesDependent.class); | ||
| var config = | ||
| converter.configFrom( | ||
| annotation, spec(ApiVersionAwareWidgetDependentResource.class), controllerConfig()); | ||
|
|
||
| assertThat(config.detectApiVersionChange()).isTrue(); | ||
| } | ||
|
|
||
| @SuppressWarnings({"unchecked", "rawtypes"}) | ||
| private static DependentResourceSpec< | ||
| GenericKubernetesResource, | ||
| ConfigMap, | ||
| KubernetesDependentResourceConfig<GenericKubernetesResource>> | ||
| spec( | ||
| Class<? extends KubernetesDependentResource<GenericKubernetesResource, ConfigMap>> | ||
| dependentResourceClass) { | ||
| return new DependentResourceSpec( | ||
| dependentResourceClass, "test", Set.of(), null, null, null, null, null); | ||
| } | ||
|
|
||
| private static ControllerConfiguration<ConfigMap> controllerConfig() { | ||
| ControllerConfiguration<ConfigMap> controllerConfig = mock(); | ||
| when(controllerConfig.getName()).thenReturn("test-reconciler"); | ||
| ConfigurationService configurationService = mock(); | ||
| when(configurationService.dependentResourceFactory()) | ||
| .thenReturn(DependentResourceFactory.DEFAULT); | ||
| when(controllerConfig.getConfigurationService()).thenReturn(configurationService); | ||
| return controllerConfig; | ||
| } | ||
|
|
||
| @KubernetesDependent | ||
| static class PlainWidgetDependentResource | ||
| extends KubernetesDependentResource<GenericKubernetesResource, ConfigMap> | ||
| implements GarbageCollected<ConfigMap> { | ||
| public PlainWidgetDependentResource() { | ||
| super(GenericKubernetesResource.class, null); | ||
| } | ||
| } | ||
|
|
||
| @KubernetesDependent(detectApiVersionChange = true) | ||
| static class ApiVersionAwareWidgetDependentResource | ||
| extends KubernetesDependentResource<GenericKubernetesResource, ConfigMap> | ||
| implements GarbageCollected<ConfigMap> { | ||
| public ApiVersionAwareWidgetDependentResource() { | ||
| super(GenericKubernetesResource.class, null); | ||
| } | ||
| } | ||
| } |
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.
Fixed in aacbb4b: the desired resource's annotations map is now defensively copied into a mutable
LinkedHashMap(viaensureMutableAnnotations) wheneverdetectApiVersionChangeis enabled, before this method or the pre-existing previous-annotation bookkeeping write to it. This is scoped to the opt-in feature so default behavior for everyone else is unchanged. Added a regression test (nonSSA_preservesExistingAnnotationsWhenMarkingEvenIfImmutable) that sets an immutableMap.of(...)on the desired resource and asserts both the existing entry and the new marker survive.