From c61be3317e799916e0a66ec9f35ba013f7b59464 Mon Sep 17 00:00:00 2001 From: csviri Date: Thu, 8 Jun 2023 15:37:39 +0200 Subject: [PATCH 01/25] feat: SSA based dependent resource matching and create/update --- cm.yaml | 14 + cm2.yaml | 9 + .../api/config/BaseConfigurationService.java | 6 + .../api/config/ConfigurationService.java | 6 + .../config/ConfigurationServiceOverrider.java | 14 + .../api/config/ControllerConfiguration.java | 8 + .../ControllerConfigurationOverrider.java | 10 +- .../ResolvedControllerConfiguration.java | 17 +- .../operator/api/reconciler/Context.java | 1 + .../reconciler/ControllerConfiguration.java | 5 + .../GenericKubernetesResourceMatcher.java | 4 +- .../KubernetesDependentResource.java | 56 ++- ...BasedGenericKubernetesResourceMatcher.java | 321 ++++++++++++++++++ ...dGenericKubernetesResourceMatcherTest.java | 85 +++++ .../event/source/ResourceEventFilterTest.java | 2 +- .../ControllerResourceEventSourceTest.java | 2 +- ...nfigmap.empty-owner-reference-desired.yaml | 14 + .../configmap.empty-owner-reference.yaml | 27 ++ ...-managed-fields-additional-controller.yaml | 105 ++++++ .../deployment-with-managed-fields.yaml | 52 +++ .../multi-container-pod-desired.yaml | 21 ++ .../kubernetes/multi-container-pod.yaml | 214 ++++++++++++ ...le-whole-complex-part-managed-desired.yaml | 24 ++ .../sample-whole-complex-part-managed.yaml | 44 +++ .../operator/DependentSSAMatchingIT.java | 99 ++++++ .../operator/DependentSSAMigrationIT.java | 167 +++++++++ .../dependentssa/DependentSSAReconciler.java | 27 ++ .../sample/dependentssa/DependentSSASpec.java | 15 + .../DependnetSSACustomResource.java | 15 + .../dependentssa/SSAConfigMapDependent.java | 42 +++ .../leader-elector-stop-role-noaccess.yaml | 2 +- .../operator/rback-test-full-access-role.yaml | 2 +- .../rback-test-no-configmap-access.yaml | 2 +- .../operator/rback-test-no-cr-access.yaml | 2 +- .../rback-test-only-main-ns-access.yaml | 2 +- testi-multi-container.yaml | 21 ++ 36 files changed, 1435 insertions(+), 22 deletions(-) create mode 100644 cm.yaml create mode 100644 cm2.yaml create mode 100644 operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java create mode 100644 operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcherTest.java create mode 100644 operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/configmap.empty-owner-reference-desired.yaml create mode 100644 operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/configmap.empty-owner-reference.yaml create mode 100644 operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/deployment-with-managed-fields-additional-controller.yaml create mode 100644 operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/deployment-with-managed-fields.yaml create mode 100644 operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/multi-container-pod-desired.yaml create mode 100644 operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/multi-container-pod.yaml create mode 100644 operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/sample-whole-complex-part-managed-desired.yaml create mode 100644 operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/sample-whole-complex-part-managed.yaml create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/DependentSSAMatchingIT.java create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/DependentSSAMigrationIT.java create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/dependentssa/DependentSSAReconciler.java create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/dependentssa/DependentSSASpec.java create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/dependentssa/DependnetSSACustomResource.java create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/dependentssa/SSAConfigMapDependent.java create mode 100644 testi-multi-container.yaml diff --git a/cm.yaml b/cm.yaml new file mode 100644 index 0000000000..9e9bab6716 --- /dev/null +++ b/cm.yaml @@ -0,0 +1,14 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: test1 + namespace: default + ownerReferences: + - apiVersion: v1 + kind: ConfigMap + name: kube-root-ca.crt + uid: 1ef74cb4-dbbd-45ef-9caf-aa76186594ea +data: + key1: "val1" +# key2: "val2" + diff --git a/cm2.yaml b/cm2.yaml new file mode 100644 index 0000000000..3c7c14c111 --- /dev/null +++ b/cm2.yaml @@ -0,0 +1,9 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: test1 + namespace: default +data: + key3: "val3" + + diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/BaseConfigurationService.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/BaseConfigurationService.java index bd939d4b32..d6bac0f5c4 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/BaseConfigurationService.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/BaseConfigurationService.java @@ -33,6 +33,7 @@ import com.fasterxml.jackson.databind.ObjectMapper; +import static io.javaoperatorsdk.operator.api.config.ControllerConfiguration.DEFAULT_FIELD_MANAGER; import static io.javaoperatorsdk.operator.api.reconciler.Constants.DEFAULT_NAMESPACES_SET; public class BaseConfigurationService extends AbstractConfigurationService { @@ -135,6 +136,11 @@ protected

ControllerConfiguration

configFor(Reconcile timeUnit = reconciliationInterval.timeUnit(); } + final var dependentFieldManager = valueOrDefault( + annotation, + io.javaoperatorsdk.operator.api.reconciler.ControllerConfiguration::fieldManager, + DEFAULT_FIELD_MANAGER); + final var config = new ResolvedControllerConfiguration

( resourceClass, name, generationAware, associatedReconcilerClass, retry, rateLimiter, diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java index 52d78fb1e9..263eee2f03 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java @@ -264,4 +264,10 @@ static ConfigurationService newOverriddenConfigurationService( default ExecutorServiceManager getExecutorServiceManager() { return new ExecutorServiceManager(this); } + + // todo test transition + // todo javadoc + default boolean legacyCreateUpdateAndMatchingForDependentResources() { + return false; + } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java index 8d9f4200bc..02e3b82359 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java @@ -32,6 +32,7 @@ public class ConfigurationServiceOverrider { private Boolean stopOnInformerErrorDuringStartup; private Duration cacheSyncTimeout; private ResourceClassResolver resourceClassResolver; + private Boolean legacyCreateUpdateAndMatchingForDependentResources; ConfigurationServiceOverrider(ConfigurationService original) { this.original = original; @@ -139,6 +140,12 @@ public ConfigurationServiceOverrider withResourceClassResolver( return this; } + public ConfigurationServiceOverrider withLegacyCreateUpdateAndMatchingForDependentResources( + boolean value) { + this.legacyCreateUpdateAndMatchingForDependentResources = value; + return this; + } + public ConfigurationService build() { return new BaseConfigurationService(original.getVersion(), cloner, objectMapper) { @Override @@ -248,6 +255,13 @@ public ResourceClassResolver getResourceClassResolver() { return resourceClassResolver != null ? resourceClassResolver : super.getResourceClassResolver(); } + + @Override + public boolean legacyCreateUpdateAndMatchingForDependentResources() { + return legacyCreateUpdateAndMatchingForDependentResources != null + ? legacyCreateUpdateAndMatchingForDependentResources + : super.legacyCreateUpdateAndMatchingForDependentResources(); + } }; } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java index 60a053d0a8..37c2e39503 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java @@ -22,6 +22,8 @@ public interface ControllerConfiguration

extends Resource @SuppressWarnings("rawtypes") RateLimiter DEFAULT_RATE_LIMITER = LinearRateLimiter.deactivatedRateLimiter(); + String DEFAULT_FIELD_MANAGER = "controller"; + String FABRIC8_CLIENT_DEFAULT_FIELD_MANAGER = "fabric8-kubernetes-client"; default String getName() { return ensureValidName(null, getAssociatedReconcilerClassName()); @@ -124,4 +126,10 @@ default Class

getResourceClass() { default Set getEffectiveNamespaces() { return ResourceConfiguration.super.getEffectiveNamespaces(getConfigurationService()); } + + // todo javadoc + default String fieldManager() { + return DEFAULT_FIELD_MANAGER; + } + } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfigurationOverrider.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfigurationOverrider.java index 5d96c2abf6..3b2dcd8289 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfigurationOverrider.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfigurationOverrider.java @@ -39,6 +39,7 @@ public class ControllerConfigurationOverrider { private Map configurations; private ItemStore itemStore; private String name; + private String fieldManager; private ControllerConfigurationOverrider(ControllerConfiguration original) { this.finalizer = original.getFinalizerName(); @@ -54,6 +55,7 @@ private ControllerConfigurationOverrider(ControllerConfiguration original) { this.original = original; this.rateLimiter = original.getRateLimiter(); this.name = original.getName(); + this.fieldManager = original.fieldManager(); } public ControllerConfigurationOverrider withFinalizer(String finalizer) { @@ -168,6 +170,12 @@ public ControllerConfigurationOverrider withName(String name) { return this; } + public ControllerConfigurationOverrider withFieldManager( + String dependentFieldManager) { + this.fieldManager = dependentFieldManager; + return this; + } + public ControllerConfigurationOverrider replacingNamedDependentResourceConfig(String name, Object dependentResourceConfig) { @@ -190,7 +198,7 @@ public ControllerConfiguration build() { generationAware, original.getAssociatedReconcilerClassName(), retry, rateLimiter, reconciliationMaxInterval, onAddFilter, onUpdateFilter, genericFilter, original.getDependentResources(), - namespaces, finalizer, labelSelector, configurations, itemStore, + namespaces, finalizer, labelSelector, configurations, itemStore, fieldManager, original.getConfigurationService()); overridden.setEventFilter(customResourcePredicate); return overridden; diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ResolvedControllerConfiguration.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ResolvedControllerConfiguration.java index f91e9c506a..0b8b7306f8 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ResolvedControllerConfiguration.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ResolvedControllerConfiguration.java @@ -32,6 +32,7 @@ public class ResolvedControllerConfiguration

private final Map configurations; private final ItemStore

itemStore; private final ConfigurationService configurationService; + private final String fieldManager; private ResourceEventFilter

eventFilter; private List dependentResources; @@ -44,7 +45,8 @@ public ResolvedControllerConfiguration(Class

resourceClass, ControllerConfigu other.genericFilter().orElse(null), other.getDependentResources(), other.getNamespaces(), other.getFinalizerName(), other.getLabelSelector(), Collections.emptyMap(), - other.getItemStore().orElse(null), other.getConfigurationService()); + other.getItemStore().orElse(null), other.fieldManager(), + other.getConfigurationService()); } public static Duration getMaxReconciliationInterval(long interval, TimeUnit timeUnit) { @@ -72,10 +74,12 @@ public ResolvedControllerConfiguration(Class

resourceClass, String name, List dependentResources, Set namespaces, String finalizer, String labelSelector, Map configurations, ItemStore

itemStore, + String fieldManager, ConfigurationService configurationService) { this(resourceClass, name, generationAware, associatedReconcilerClassName, retry, rateLimiter, maxReconciliationInterval, onAddFilter, onUpdateFilter, genericFilter, - namespaces, finalizer, labelSelector, configurations, itemStore, configurationService); + namespaces, finalizer, labelSelector, configurations, itemStore, fieldManager, + configurationService); setDependentResources(dependentResources); } @@ -85,6 +89,7 @@ protected ResolvedControllerConfiguration(Class

resourceClass, String name, OnAddFilter

onAddFilter, OnUpdateFilter

onUpdateFilter, GenericFilter

genericFilter, Set namespaces, String finalizer, String labelSelector, Map configurations, ItemStore

itemStore, + String fieldManager, ConfigurationService configurationService) { super(resourceClass, namespaces, labelSelector, onAddFilter, onUpdateFilter, genericFilter, itemStore); @@ -99,13 +104,14 @@ protected ResolvedControllerConfiguration(Class

resourceClass, String name, this.itemStore = itemStore; this.finalizer = ControllerConfiguration.ensureValidFinalizerName(finalizer, getResourceTypeName()); + this.fieldManager = fieldManager; } protected ResolvedControllerConfiguration(Class

resourceClass, String name, Class reconcilerClas, ConfigurationService configurationService) { this(resourceClass, name, false, getAssociatedReconcilerClassName(reconcilerClas), null, null, null, null, null, null, null, - null, null, null, null, configurationService); + null, null, null, null, null, configurationService); } @Override @@ -182,4 +188,9 @@ public Object getConfigurationFor(DependentResourceSpec spec) { public Optional> getItemStore() { return Optional.ofNullable(itemStore); } + + @Override + public String fieldManager() { + return fieldManager; + } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/Context.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/Context.java index 5043ed675c..a6482119c3 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/Context.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/Context.java @@ -3,6 +3,7 @@ import java.util.Optional; import java.util.Set; import java.util.concurrent.ExecutorService; +import java.util.concurrent.ExecutorService; import java.util.stream.Stream; import io.fabric8.kubernetes.api.model.HasMetadata; diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ControllerConfiguration.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ControllerConfiguration.java index e2c0de896b..0fd731dd78 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ControllerConfiguration.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ControllerConfiguration.java @@ -17,6 +17,8 @@ import io.javaoperatorsdk.operator.processing.retry.GenericRetry; import io.javaoperatorsdk.operator.processing.retry.Retry; +import static io.javaoperatorsdk.operator.api.config.ControllerConfiguration.DEFAULT_FIELD_MANAGER; + @Inherited @Retention(RetentionPolicy.RUNTIME) @Target({ElementType.TYPE}) @@ -129,4 +131,7 @@ MaxReconciliationInterval maxReconciliationInterval() default @MaxReconciliation Class rateLimiter() default LinearRateLimiter.class; Class itemStore() default ItemStore.class; + + // todo javadoc + String fieldManager() default DEFAULT_FIELD_MANAGER; } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericKubernetesResourceMatcher.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericKubernetesResourceMatcher.java index 0cee7c2413..f0ca32bd98 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericKubernetesResourceMatcher.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericKubernetesResourceMatcher.java @@ -19,8 +19,8 @@ public class GenericKubernetesResourceMatcher> { private static final Logger log = LoggerFactory.getLogger(KubernetesDependentResource.class); + private static final PatchContext SSA_PATCH_CONTEXT = new PatchContext.Builder() + .withPatchType(PatchType.SERVER_SIDE_APPLY) + .withForce(true) + .build(); protected KubernetesClient client; private final ResourceUpdatePreProcessor processor; private final boolean garbageCollected = this instanceof GarbageCollected; private KubernetesDependentResourceConfig kubernetesDependentResourceConfig; + @SuppressWarnings("unchecked") public KubernetesDependentResource(Class resourceType) { super(resourceType); @@ -128,16 +135,35 @@ protected R handleUpdate(R actual, R desired, P primary, Context

context) { @SuppressWarnings("unused") public R create(R target, P primary, Context

context) { - return prepare(target, primary, "Creating").create(); + if (context.getControllerConfiguration().getConfigurationService() + .legacyCreateUpdateAndMatchingForDependentResources()) { + return prepare(target, primary, "Creating").create(); + } else { + return prepare(target, primary, "Creating").patch(getSSAPatchContext(context)); + } } public R update(R actual, R target, P primary, Context

context) { - var updatedActual = processor.replaceSpecOnActual(actual, target, context); - return prepare(updatedActual, primary, "Updating").replace(); + if (context.getControllerConfiguration().getConfigurationService() + .legacyCreateUpdateAndMatchingForDependentResources()) { + var updatedActual = processor.replaceSpecOnActual(actual, target, context); + return prepare(updatedActual, primary, "Updating").replace(); + } else { + return prepare(target, primary, "Updating").patch(getSSAPatchContext(context)); + } } public Result match(R actualResource, P primary, Context

context) { - return GenericKubernetesResourceMatcher.match(this, actualResource, primary, context, false); + if (context.getControllerConfiguration().getConfigurationService() + .legacyCreateUpdateAndMatchingForDependentResources()) { + return GenericKubernetesResourceMatcher.match(this, actualResource, primary, context, false); + } else { + final var desired = desired(primary, context); + addReferenceHandlingMetadata(desired, primary); + var matches = SSABasedGenericKubernetesResourceMatcher.getInstance().matches(actualResource, + desired, context); + return Result.computed(matches, desired); + } } @SuppressWarnings("unused") @@ -164,11 +190,7 @@ protected Resource prepare(R desired, P primary, String actionName) { desired.getClass(), ResourceID.fromResource(desired)); - if (addOwnerReference()) { - desired.addOwnerReference(primary); - } else if (useDefaultAnnotationsToIdentifyPrimary()) { - addDefaultSecondaryToPrimaryMapperAnnotations(desired, primary); - } + addReferenceHandlingMetadata(desired, primary); if (desired instanceof Namespaced) { return client.resource(desired).inNamespace(desired.getMetadata().getNamespace()); @@ -177,6 +199,14 @@ protected Resource prepare(R desired, P primary, String actionName) { } } + protected void addReferenceHandlingMetadata(R desired, P primary) { + if (addOwnerReference()) { + desired.addOwnerReference(primary); + } else if (useDefaultAnnotationsToIdentifyPrimary()) { + addDefaultSecondaryToPrimaryMapperAnnotations(desired, primary); + } + } + @Override @SuppressWarnings("unchecked") protected InformerEventSource createEventSource(EventSourceContext

context) { @@ -260,4 +290,12 @@ public boolean isDeletable() { return super.isDeletable() && !garbageCollected; } + private PatchContext getSSAPatchContext(Context

context) { + return new PatchContext.Builder() + .withPatchType(PatchType.SERVER_SIDE_APPLY) + .withForce(true) + .withFieldManager(context.getControllerConfiguration().fieldManager()) + .build(); + } + } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java new file mode 100644 index 0000000000..369bc0cd26 --- /dev/null +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java @@ -0,0 +1,321 @@ +package io.javaoperatorsdk.operator.processing.dependent.kubernetes; + +import java.util.*; +import java.util.stream.Collectors; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import io.fabric8.kubernetes.api.model.HasMetadata; +import io.fabric8.kubernetes.api.model.ManagedFieldsEntry; +import io.javaoperatorsdk.operator.api.reconciler.Context; + +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.core.type.TypeReference; +import com.fasterxml.jackson.databind.ObjectMapper; + +/** + * Matches the actual state on the server vs the desired state. Based on the managedFields of SSA. + * + * The ide of algorithm is basically trivial, we convert resources to Map/List composition. + * The actual resource (from the server) is pruned, all the fields which are not mentioed in managedFields + * of the target manager is removed. Some irrelevant fields are also removed from desired. And the + * two resulted Maps are compared for equality. The implementation is a bit nasty since have to deal with + * some specific cases of managedFields format. + * + * @param matched resource type + */ +// https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.27/#fieldsv1-v1-meta +// https://github.com/kubernetes-sigs/structured-merge-diff +// https://docs.aws.amazon.com/eks/latest/userguide/kubernetes-field-management.html +// see also: https://kubernetes.slack.com/archives/C0123CNN8F3/p1686141087220719 +public class SSABasedGenericKubernetesResourceMatcher { + + @SuppressWarnings("rawtypes") + private static final SSABasedGenericKubernetesResourceMatcher INSTANCE = + new SSABasedGenericKubernetesResourceMatcher<>(); + public static final String APPLY_OPERATION = "Apply"; + public static final String DOT_KEY = "."; + + @SuppressWarnings("unchecked") + public static SSABasedGenericKubernetesResourceMatcher getInstance() { + return INSTANCE; + } + + private static final TypeReference> typeRef = new TypeReference<>() {}; + + private static final String F_PREFIX = "f:"; + private static final String K_PREFIX = "k:"; + private static final String V_PREFIX = "v:"; + public static final String METADATA_KEY = "metadata"; + + private static final Logger log = + LoggerFactory.getLogger(SSABasedGenericKubernetesResourceMatcher.class); + + + public boolean matches(R actual, R desired, Context context) { + try { + var optionalManagedFieldsEntry = + checkIfFieldManagerExists(actual, + context.getControllerConfiguration().fieldManager()); + // the results of this is that it will add the field manager; it's important from migration + // aspect + if (optionalManagedFieldsEntry.isEmpty()) { + return false; + } + + var managedFieldsEntry = optionalManagedFieldsEntry.orElseThrow(); + + var objectMapper = + context.getControllerConfiguration().getConfigurationService().getObjectMapper(); + + var actualMap = objectMapper.convertValue(actual, typeRef); + var desiredMap = objectMapper.convertValue(desired, typeRef); + + log.trace("Original actual: \n {} \n original desired: \n {} ", actual, desiredMap); + + var prunedActual = new HashMap(); + pruneActualAccordingManagedFields(prunedActual, actualMap, + managedFieldsEntry.getFieldsV1().getAdditionalProperties(), objectMapper); + + removeIrrelevantValues(desiredMap); + + log.debug("Pruned actual: \n {} \n desired: \n {} ", prunedActual, desiredMap); + + return prunedActual.equals(desiredMap); + } catch (JsonProcessingException e) { + throw new IllegalStateException(e); + } + } + + private void removeIrrelevantValues(HashMap desiredMap) { + var metadata = (Map) desiredMap.get(METADATA_KEY); + metadata.remove("name"); + metadata.remove("namespace"); + if (metadata.isEmpty()) { + desiredMap.remove(METADATA_KEY); + } + desiredMap.remove("kind"); + desiredMap.remove("apiVersion"); + } + + private void pruneActualAccordingManagedFields(Map result, + Map actualMap, + Map managedFields, ObjectMapper objectMapper) throws JsonProcessingException { + + if (managedFields.isEmpty()) { + result.putAll(actualMap); + return; + } + for (Map.Entry entry : managedFields.entrySet()) { + String key = entry.getKey(); + if (key.startsWith(F_PREFIX)) { + String keyInActual = keyWithoutPrefix(key); + var managedFieldValue = entry.getValue(); + if (isNestedValue(managedFieldValue)) { + var managedEntrySet = ((Map) managedFieldValue).entrySet(); + + // two special cases "k:" and "v:" prefixes + if (isListKeyEntrySet(managedEntrySet)) { + handleListKeyEntrySet(result, actualMap, objectMapper, keyInActual, managedEntrySet); + } else if (isSetValueField(managedEntrySet)) { + handleSetValues(result, actualMap, objectMapper, keyInActual, managedEntrySet); + } else { + // basically if we should travers further + fillResultsAndTraversFurther(result, actualMap, managedFields, objectMapper, key, + keyInActual, managedFieldValue); + } + } else { + // this should handle the case when the value is complex in the actual map (not just a + // simple value). + result.put(keyInActual, actualMap.get(keyInActual)); + } + } else { + // .:{} is ignored, other should not be present + if (!DOT_KEY.equals(key)) { + throw new IllegalStateException("Key: " + key + " has no prefix: " + F_PREFIX); + } + } + } + } + + private void fillResultsAndTraversFurther(Map result, + Map actualMap, Map managedFields, ObjectMapper objectMapper, + String key, String keyInActual, Object managedFieldValue) throws JsonProcessingException { + var emptyMapValue = new HashMap(); + result.put(keyInActual, emptyMapValue); + var actualMapValue = actualMap.get(keyInActual); + log.debug("key: {} actual map value: {} managedFieldValue: {}", keyInActual, + actualMapValue, managedFieldValue); + + pruneActualAccordingManagedFields(emptyMapValue, (Map) actualMapValue, + (Map) managedFields.get(key), objectMapper); + } + + private static boolean isNestedValue(Object managedFieldValue) { + return managedFieldValue instanceof Map && (!((Map) managedFieldValue).isEmpty()); + } + + // list entries referenced by key, or when "k:" prefix is used + private void handleListKeyEntrySet(Map result, Map actualMap, + ObjectMapper objectMapper, String keyInActual, Set> managedEntrySet) { + var valueList = new ArrayList<>(); + result.put(keyInActual, valueList); + var actualValueList = (List>) actualMap.get(keyInActual); + + Map> targetValuesByIndex = new HashMap<>(); + Map> mangedEntryByIndex = new HashMap<>(); + + for (Map.Entry listEntry : managedEntrySet) { + if (DOT_KEY.equals(listEntry.getKey())) { + continue; + } + var actualListEntry = selectListEntryBasedOnKey(keyWithoutPrefix(listEntry.getKey()), + actualValueList, objectMapper); + targetValuesByIndex.put(actualListEntry.getKey(), actualListEntry.getValue()); + mangedEntryByIndex.put(actualListEntry.getKey(), (Map) listEntry.getValue()); + } + + targetValuesByIndex.entrySet() + .stream() + // list is sorted according to the value in actual + .sorted(Map.Entry.comparingByKey()) + .forEach(e -> { + var emptyResMapValue = new HashMap(); + valueList.add(emptyResMapValue); + try { + pruneActualAccordingManagedFields(emptyResMapValue, e.getValue(), + mangedEntryByIndex.get(e.getKey()), objectMapper); + } catch (JsonProcessingException ex) { + throw new IllegalStateException(ex); + } + }); + } + + // set values, the "v:" prefix + private static void handleSetValues(Map result, Map actualMap, + ObjectMapper objectMapper, String keyInActual, + Set> managedEntrySet) { + var valueList = new ArrayList<>(); + result.put(keyInActual, valueList); + for (Map.Entry valueEntry : managedEntrySet) { + // not clear if this can happen + if (DOT_KEY.equals(valueEntry.getKey())) { + continue; + } + Class targetClass = null; + List values = (List) actualMap.get(keyInActual); + if (!(values.get(0) instanceof Map)) { + targetClass = values.get(0).getClass(); + } + + var value = + parseKeyValue(keyWithoutPrefix(valueEntry.getKey()), targetClass, objectMapper); + valueList.add(value); + } + } + + public static Object parseKeyValue(String stringValue, Class targetClass, + ObjectMapper objectMapper) { + try { + stringValue = stringValue.trim(); + if (targetClass != null) { + return objectMapper.readValue(stringValue, targetClass); + } else { + return objectMapper.readValue(stringValue, typeRef); + } + } catch (JsonProcessingException e) { + throw new IllegalStateException(e); + } + } + + private boolean isSetValueField(Set> managedEntrySet) { + var iterator = managedEntrySet.iterator(); + var managedFieldEntry = iterator.next(); + if (managedFieldEntry.getKey().equals(DOT_KEY)) { + managedFieldEntry = iterator.next(); + } + return managedFieldEntry.getKey().startsWith(V_PREFIX); + } + + private boolean isListKeyEntrySet(Set> managedEntrySet) { + var iterator = managedEntrySet.iterator(); + var managedFieldEntry = iterator.next(); + if (managedFieldEntry.getKey().equals(DOT_KEY)) { + managedFieldEntry = iterator.next(); + } + return managedFieldEntry.getKey().startsWith(K_PREFIX); + } + + private java.util.Map.Entry> selectListEntryBasedOnKey(String key, + List> values, + ObjectMapper objectMapper) { + try { + Map ids = objectMapper.readValue(key, typeRef); + List> possibleTargets = new ArrayList<>(1); + int index = -1; + for (int i = 0; i < values.size(); i++) { + var v = values.get(i); + if (v.entrySet().containsAll(ids.entrySet())) { + possibleTargets.add(v); + index = i; + } + } + if (possibleTargets.isEmpty()) { + throw new IllegalStateException( + "Cannot find list element for key:" + key + ", in map: " + values); + } + if (possibleTargets.size() > 1) { + throw new IllegalStateException( + "More targets found in list element for key:" + key + ", in map: " + values); + } + final var finalIndex = index; + return new Map.Entry<>() { + @Override + public Integer getKey() { + return finalIndex; + } + + @Override + public Map getValue() { + return possibleTargets.get(0); + } + + @Override + public Map setValue(Map stringObjectMap) { + throw new IllegalStateException("should not be called"); + } + }; + } catch (JsonProcessingException e) { + throw new IllegalStateException(e); + } + } + + + private Optional checkIfFieldManagerExists(R actual, String fieldManager) { + var targetManagedFields = actual.getMetadata().getManagedFields().stream() + // Only the apply operations are interesting for us since those were created properly be SSA + // Patch. An update can be present with same fieldManager when migrating and having the same + // field manager name. + .filter( + f -> f.getManager().equals(fieldManager) && f.getOperation().equals(APPLY_OPERATION)) + .collect(Collectors.toList()); + if (targetManagedFields.isEmpty()) { + log.debug("No field manager exists for resource {} with name: {} and operation Apply ", + actual, actual.getMetadata().getName()); + return Optional.empty(); + } + // this should not happen in theory + if (targetManagedFields.size() > 1) { + log.debug("More field managers exists with name: {} in resource: {} with name: {} ", + fieldManager, + actual, actual.getMetadata().getName()); + } + return Optional.of(targetManagedFields.get(0)); + } + + private static String keyWithoutPrefix(String key) { + return key.substring(2); + } + +} diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcherTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcherTest.java new file mode 100644 index 0000000000..11a5fa1db4 --- /dev/null +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcherTest.java @@ -0,0 +1,85 @@ +package io.javaoperatorsdk.operator.processing.dependent.kubernetes; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import io.fabric8.kubernetes.api.model.ConfigMap; +import io.fabric8.kubernetes.api.model.HasMetadata; +import io.fabric8.kubernetes.api.model.apps.Deployment; +import io.javaoperatorsdk.operator.ReconcilerUtils; +import io.javaoperatorsdk.operator.api.config.ConfigurationService; +import io.javaoperatorsdk.operator.api.config.ControllerConfiguration; +import io.javaoperatorsdk.operator.api.reconciler.Context; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +class SSABasedGenericKubernetesResourceMatcherTest { + + Context mockedContext = mock(Context.class); + + SSABasedGenericKubernetesResourceMatcher matcher = + new SSABasedGenericKubernetesResourceMatcher<>(); + + @BeforeEach + @SuppressWarnings("unchecked") + void setup() { + var controllerConfiguration = mock(ControllerConfiguration.class); + when(controllerConfiguration.fieldManager()).thenCallRealMethod(); + var configurationService = mock(ConfigurationService.class); + when(configurationService.getObjectMapper()).thenCallRealMethod(); + when(controllerConfiguration.getConfigurationService()).thenReturn(configurationService); + when(mockedContext.getControllerConfiguration()).thenReturn(controllerConfiguration); + } + + @Test + void checksIfAddsNotAddedByController() { + var desired = loadResource("nginx-deployment.yaml", Deployment.class); + var actual = + loadResource("deployment-with-managed-fields-additional-controller.yaml", Deployment.class); + + assertThat(matcher.matches(actual, desired, mockedContext)).isTrue(); + } + + // In the example the owner reference in a list is referenced by "k:", while all the fields are + // managed but not listed + @Test + void emptyListElementMatchesAllFields() { + var desiredConfigMap = loadResource("configmap.empty-owner-reference-desired.yaml", + ConfigMap.class); + var actualConfigMap = loadResource("configmap.empty-owner-reference.yaml", + ConfigMap.class); + + assertThat(matcher.matches(actualConfigMap, desiredConfigMap, mockedContext)).isTrue(); + } + + + // the whole "rules:" part is just implicitly managed + @Test + void wholeComplexFieldManaged() { + var desiredConfigMap = loadResource("sample-whole-complex-part-managed-desired.yaml", + ConfigMap.class); + var actualConfigMap = loadResource("sample-whole-complex-part-managed.yaml", + ConfigMap.class); + + assertThat(matcher.matches(actualConfigMap, desiredConfigMap, mockedContext)).isTrue(); + } + + + @Test + void multiItemList() { + var desiredConfigMap = loadResource("multi-container-pod-desired.yaml", + ConfigMap.class); + var actualConfigMap = loadResource("multi-container-pod.yaml", + ConfigMap.class); + + assertThat(matcher.matches(actualConfigMap, desiredConfigMap, mockedContext)).isTrue(); + } + + private R loadResource(String fileName, Class clazz) { + return ReconcilerUtils.loadYaml(clazz, SSABasedGenericKubernetesResourceMatcherTest.class, + fileName); + } + +} diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/ResourceEventFilterTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/ResourceEventFilterTest.java index 9e58a93ddf..003b623b5d 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/ResourceEventFilterTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/ResourceEventFilterTest.java @@ -145,7 +145,7 @@ public ControllerConfig(String finalizer, boolean generationAware, null, null, null, - null, null, null, finalizer, null, null, new BaseConfigurationService()); + null, null, null, finalizer, null, null, null, new BaseConfigurationService()); setEventFilter(eventFilter); } } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/controller/ControllerResourceEventSourceTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/controller/ControllerResourceEventSourceTest.java index a22d2f85b6..6ee3d19d6c 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/controller/ControllerResourceEventSourceTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/controller/ControllerResourceEventSourceTest.java @@ -198,7 +198,7 @@ public TestConfiguration(boolean generationAware, OnAddFilter /data/index.html"] \ No newline at end of file diff --git a/operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/multi-container-pod.yaml b/operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/multi-container-pod.yaml new file mode 100644 index 0000000000..e1334117b6 --- /dev/null +++ b/operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/multi-container-pod.yaml @@ -0,0 +1,214 @@ +apiVersion: v1 +kind: Pod +metadata: + creationTimestamp: "2023-06-08T11:50:59Z" + managedFields: + - apiVersion: v1 + fieldsType: FieldsV1 + fieldsV1: + f:spec: + f:containers: + k:{"name":"debian-container"}: + .: {} + f:args: {} + f:command: {} + f:image: {} + f:name: {} + f:volumeMounts: + k:{"mountPath":"/data"}: + .: {} + f:mountPath: {} + f:name: {} + k:{"name":"nginx-container"}: + .: {} + f:image: {} + f:name: {} + f:volumeMounts: + k:{"mountPath":"/usr/share/nginx/html"}: + .: {} + f:mountPath: {} + f:name: {} + f:volumes: + k:{"name":"shared-data"}: + .: {} + f:emptyDir: {} + f:name: {} + manager: controller + operation: Apply + time: "2023-06-08T11:50:59Z" + - apiVersion: v1 + fieldsType: FieldsV1 + fieldsV1: + f:status: + f:conditions: + k:{"type":"ContainersReady"}: + .: {} + f:lastProbeTime: {} + f:lastTransitionTime: {} + f:message: {} + f:reason: {} + f:status: {} + f:type: {} + k:{"type":"Initialized"}: + .: {} + f:lastProbeTime: {} + f:lastTransitionTime: {} + f:status: {} + f:type: {} + k:{"type":"Ready"}: + .: {} + f:lastProbeTime: {} + f:lastTransitionTime: {} + f:message: {} + f:reason: {} + f:status: {} + f:type: {} + f:containerStatuses: {} + f:hostIP: {} + f:phase: {} + f:podIP: {} + f:podIPs: + .: {} + k:{"ip":"10.244.0.3"}: + .: {} + f:ip: {} + f:startTime: {} + manager: kubelet + operation: Update + subresource: status + time: "2023-06-08T11:51:21Z" + name: shared-storage + namespace: default + resourceVersion: "1950" + uid: 0c916935-8198-4d62-980e-193f3c3ec877 +spec: + containers: + - image: nginx + imagePullPolicy: Always + name: nginx-container + resources: {} + terminationMessagePath: /dev/termination-log + terminationMessagePolicy: File + volumeMounts: + - mountPath: /usr/share/nginx/html + name: shared-data + - mountPath: /var/run/secrets/kubernetes.io/serviceaccount + name: kube-api-access-gxpbz + readOnly: true + - args: + - -c + - echo Level Up Blue Team! > /data/index.html + command: + - /bin/sh + image: debian + imagePullPolicy: Always + name: debian-container + resources: {} + terminationMessagePath: /dev/termination-log + terminationMessagePolicy: File + volumeMounts: + - mountPath: /data + name: shared-data + - mountPath: /var/run/secrets/kubernetes.io/serviceaccount + name: kube-api-access-gxpbz + readOnly: true + dnsPolicy: ClusterFirst + enableServiceLinks: true + nodeName: minikube + preemptionPolicy: PreemptLowerPriority + priority: 0 + restartPolicy: Always + schedulerName: default-scheduler + securityContext: {} + serviceAccount: default + serviceAccountName: default + terminationGracePeriodSeconds: 30 + tolerations: + - effect: NoExecute + key: node.kubernetes.io/not-ready + operator: Exists + tolerationSeconds: 300 + - effect: NoExecute + key: node.kubernetes.io/unreachable + operator: Exists + tolerationSeconds: 300 + volumes: + - emptyDir: {} + name: shared-data + - name: kube-api-access-gxpbz + projected: + defaultMode: 420 + sources: + - serviceAccountToken: + expirationSeconds: 3607 + path: token + - configMap: + items: + - key: ca.crt + path: ca.crt + name: kube-root-ca.crt + - downwardAPI: + items: + - fieldRef: + apiVersion: v1 + fieldPath: metadata.namespace + path: namespace +status: + conditions: + - lastProbeTime: null + lastTransitionTime: "2023-06-08T11:50:59Z" + status: "True" + type: Initialized + - lastProbeTime: null + lastTransitionTime: "2023-06-08T11:50:59Z" + message: 'containers with unready status: [debian-container]' + reason: ContainersNotReady + status: "False" + type: Ready + - lastProbeTime: null + lastTransitionTime: "2023-06-08T11:50:59Z" + message: 'containers with unready status: [debian-container]' + reason: ContainersNotReady + status: "False" + type: ContainersReady + - lastProbeTime: null + lastTransitionTime: "2023-06-08T11:50:59Z" + status: "True" + type: PodScheduled + containerStatuses: + - containerID: docker://ead1d3e4beaaa9176daca99e55673a2176e0da51d9953d6a11d5786b730178ee + image: debian:latest + imageID: docker-pullable://debian@sha256:432f545c6ba13b79e2681f4cc4858788b0ab099fc1cca799cc0fae4687c69070 + lastState: + terminated: + containerID: docker://ead1d3e4beaaa9176daca99e55673a2176e0da51d9953d6a11d5786b730178ee + exitCode: 0 + finishedAt: "2023-06-08T11:51:19Z" + reason: Completed + startedAt: "2023-06-08T11:51:19Z" + name: debian-container + ready: false + restartCount: 1 + started: false + state: + waiting: + message: back-off 10s restarting failed container=debian-container pod=shared-storage_default(0c916935-8198-4d62-980e-193f3c3ec877) + reason: CrashLoopBackOff + - containerID: docker://afd6260e41afa0b149ebfd904162fb2f22bb037c18904eed599eb9ac1ce4faf0 + image: nginx:latest + imageID: docker-pullable://nginx@sha256:af296b188c7b7df99ba960ca614439c99cb7cf252ed7bbc23e90cfda59092305 + lastState: {} + name: nginx-container + ready: true + restartCount: 0 + started: true + state: + running: + startedAt: "2023-06-08T11:51:09Z" + hostIP: 192.168.49.2 + phase: Running + podIP: 10.244.0.3 + podIPs: + - ip: 10.244.0.3 + qosClass: BestEffort + startTime: "2023-06-08T11:50:59Z" \ No newline at end of file diff --git a/operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/sample-whole-complex-part-managed-desired.yaml b/operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/sample-whole-complex-part-managed-desired.yaml new file mode 100644 index 0000000000..3baff88db0 --- /dev/null +++ b/operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/sample-whole-complex-part-managed-desired.yaml @@ -0,0 +1,24 @@ +kind: FlowSchema +metadata: + annotations: + apf.kubernetes.io/autoupdate-spec: "true" + name: probes +spec: + matchingPrecedence: 2 + priorityLevelConfiguration: + name: exempt + rules: + - nonResourceRules: + - nonResourceURLs: + - /healthz + - /readyz + - /livez + verbs: + - get + subjects: + - group: + name: system:unauthenticated + kind: Group + - group: + name: system:authenticated + kind: Group diff --git a/operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/sample-whole-complex-part-managed.yaml b/operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/sample-whole-complex-part-managed.yaml new file mode 100644 index 0000000000..9d9e20efbb --- /dev/null +++ b/operator-framework-core/src/test/resources/io/javaoperatorsdk/operator/processing/dependent/kubernetes/sample-whole-complex-part-managed.yaml @@ -0,0 +1,44 @@ +kind: FlowSchema +metadata: + annotations: + apf.kubernetes.io/autoupdate-spec: "true" + creationTimestamp: "2023-06-08T11:18:25Z" + generation: 1 + managedFields: + - apiVersion: flowcontrol.apiserver.k8s.io/v1beta3 + fieldsType: FieldsV1 + fieldsV1: + f:metadata: + f:annotations: + .: {} + f:apf.kubernetes.io/autoupdate-spec: {} + f:spec: + f:matchingPrecedence: {} + f:priorityLevelConfiguration: + f:name: {} + f:rules: {} + manager: controller + operation: Apply + time: "2023-06-08T11:18:25Z" + name: probes + resourceVersion: "68" + uid: 50913e35-e855-469f-bec6-3e8cd2607ab4 +spec: + matchingPrecedence: 2 + priorityLevelConfiguration: + name: exempt + rules: + - nonResourceRules: + - nonResourceURLs: + - /healthz + - /readyz + - /livez + verbs: + - get + subjects: + - group: + name: system:unauthenticated + kind: Group + - group: + name: system:authenticated + kind: Group diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/DependentSSAMatchingIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/DependentSSAMatchingIT.java new file mode 100644 index 0000000000..0249b44927 --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/DependentSSAMatchingIT.java @@ -0,0 +1,99 @@ +package io.javaoperatorsdk.operator; + +import java.time.Duration; +import java.util.Map; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; + +import io.fabric8.kubernetes.api.model.ConfigMap; +import io.fabric8.kubernetes.api.model.ConfigMapBuilder; +import io.fabric8.kubernetes.api.model.ObjectMetaBuilder; +import io.fabric8.kubernetes.client.dsl.base.PatchContext; +import io.fabric8.kubernetes.client.dsl.base.PatchType; +import io.javaoperatorsdk.operator.junit.LocallyRunOperatorExtension; +import io.javaoperatorsdk.operator.sample.dependentssa.DependentSSAReconciler; +import io.javaoperatorsdk.operator.sample.dependentssa.DependentSSASpec; +import io.javaoperatorsdk.operator.sample.dependentssa.DependnetSSACustomResource; +import io.javaoperatorsdk.operator.sample.dependentssa.SSAConfigMapDependent; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.awaitility.Awaitility.await; + +public class DependentSSAMatchingIT { + + public static final String TEST_RESOURCE_NAME = "test1"; + public static final String INITIAL_VALUE = "INITIAL_VALUE"; + public static final String CHANGED_VALUE = "CHANGED_VALUE"; + + public static final String CUSTOM_FIELD_MANAGER_NAME = "customFieldManagerName"; + public static final String OTHER_FIELD_MANAGER = "otherFieldManager"; + public static final String ADDITIONAL_KEY = "key2"; + public static final String ADDITIONAL_VALUE = "Additional Value"; + + + @RegisterExtension + LocallyRunOperatorExtension extension = + LocallyRunOperatorExtension.builder() + .withReconciler(new DependentSSAReconciler(), + o -> o.withFieldManager(CUSTOM_FIELD_MANAGER_NAME)) + .build(); + + @Test + void testMatchingAndUpdate() { + SSAConfigMapDependent.NUMBER_OF_UPDATES.set(0); + var resource = extension.create(testResource()); + + await().untilAsserted(() -> { + var cm = extension.get(ConfigMap.class, TEST_RESOURCE_NAME); + assertThat(cm).isNotNull(); + assertThat(cm.getData()).containsEntry(SSAConfigMapDependent.DATA_KEY, INITIAL_VALUE); + assertThat(cm.getMetadata().getManagedFields().stream() + .filter(fm -> fm.getManager().equals(CUSTOM_FIELD_MANAGER_NAME))).isNotEmpty(); + assertThat(SSAConfigMapDependent.NUMBER_OF_UPDATES.get()).isZero(); + }); + + ConfigMap cmPatch = new ConfigMapBuilder() + .withMetadata(new ObjectMetaBuilder() + .withName(TEST_RESOURCE_NAME) + .withNamespace(resource.getMetadata().getNamespace()) + .build()) + .withData(Map.of(ADDITIONAL_KEY, ADDITIONAL_VALUE)) + .build(); + + extension.getKubernetesClient().configMaps().resource(cmPatch).patch(new PatchContext.Builder() + .withFieldManager(OTHER_FIELD_MANAGER) + .withPatchType(PatchType.SERVER_SIDE_APPLY) + .build()); + + await().pollDelay(Duration.ofMillis(300)).untilAsserted(() -> { + var cm = extension.get(ConfigMap.class, TEST_RESOURCE_NAME); + assertThat(cm.getData()).hasSize(2); + assertThat(SSAConfigMapDependent.NUMBER_OF_UPDATES.get()).isZero(); + assertThat(cm.getMetadata().getManagedFields()).hasSize(2); + }); + + resource.getSpec().setValue(CHANGED_VALUE); + extension.replace(resource); + + await().untilAsserted(() -> { + var cm = extension.get(ConfigMap.class, TEST_RESOURCE_NAME); + assertThat(cm.getData()).hasSize(2); + assertThat(cm.getData()).containsEntry(SSAConfigMapDependent.DATA_KEY, CHANGED_VALUE); + assertThat(cm.getData()).containsEntry(ADDITIONAL_KEY, ADDITIONAL_VALUE); + assertThat(cm.getMetadata().getManagedFields()).hasSize(2); + assertThat(SSAConfigMapDependent.NUMBER_OF_UPDATES.get()).isEqualTo(1); + }); + } + + public DependnetSSACustomResource testResource() { + DependnetSSACustomResource resource = new DependnetSSACustomResource(); + resource.setMetadata(new ObjectMetaBuilder() + .withName(TEST_RESOURCE_NAME) + .build()); + resource.setSpec(new DependentSSASpec()); + resource.getSpec().setValue(INITIAL_VALUE); + return resource; + } + +} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/DependentSSAMigrationIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/DependentSSAMigrationIT.java new file mode 100644 index 0000000000..830a41dddf --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/DependentSSAMigrationIT.java @@ -0,0 +1,167 @@ +package io.javaoperatorsdk.operator; + +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestInfo; + +import io.fabric8.kubernetes.api.model.ConfigMap; +import io.fabric8.kubernetes.api.model.NamespaceBuilder; +import io.fabric8.kubernetes.api.model.ObjectMetaBuilder; +import io.fabric8.kubernetes.client.KubernetesClient; +import io.fabric8.kubernetes.client.KubernetesClientBuilder; +import io.fabric8.kubernetes.client.utils.KubernetesResourceUtil; +import io.javaoperatorsdk.operator.junit.LocallyRunOperatorExtension; +import io.javaoperatorsdk.operator.sample.dependentssa.DependentSSAReconciler; +import io.javaoperatorsdk.operator.sample.dependentssa.DependentSSASpec; +import io.javaoperatorsdk.operator.sample.dependentssa.DependnetSSACustomResource; +import io.javaoperatorsdk.operator.sample.dependentssa.SSAConfigMapDependent; + +import static io.javaoperatorsdk.operator.api.config.ControllerConfiguration.FABRIC8_CLIENT_DEFAULT_FIELD_MANAGER; +import static org.assertj.core.api.Assertions.assertThat; +import static org.awaitility.Awaitility.await; + +class DependentSSAMigrationIT { + + public static final String TEST_RESOURCE_NAME = "test1"; + public static final String INITIAL_VALUE = "INITIAL_VALUE"; + public static final String CHANGED_VALUE = "CHANGED_VALUE"; + + private String namespace; + private final KubernetesClient client = new KubernetesClientBuilder().build(); + + @BeforeEach + void setup(TestInfo testInfo) { + SSAConfigMapDependent.NUMBER_OF_UPDATES.set(0); + LocallyRunOperatorExtension.applyCrd(DependnetSSACustomResource.class, client); + testInfo.getTestMethod().ifPresent(method -> { + namespace = KubernetesResourceUtil.sanitizeName(method.getName()); + cleanup(); + client.namespaces().resource(new NamespaceBuilder().withMetadata(new ObjectMetaBuilder() + .withName(namespace) + .build()).build()).create(); + }); + } + + @AfterEach + void cleanup() { + client.namespaces().resource(new NamespaceBuilder().withMetadata(new ObjectMetaBuilder() + .withName(namespace) + .build()).build()).delete(); + } + + @Test + void migratesFromLegacyToWorksAndBack() { + var legacyOperator = createOperator(client, true, null); + DependnetSSACustomResource testResource = reconcileWithLegacyOperator(legacyOperator); + + var operator = createOperator(client, false, null); + testResource = reconcileWithNewApproach(testResource, operator); + var cm = getDependentConfigMap(); + assertThat(cm.getMetadata().getManagedFields()).hasSize(2); + + reconcileAgainWithLegacy(legacyOperator, testResource); + } + + @Test + void usingDefaultFieldManagerDoesNotCreatesANewOneWithApplyOperation() { + var legacyOperator = createOperator(client, true, null); + DependnetSSACustomResource testResource = reconcileWithLegacyOperator(legacyOperator); + + var operator = createOperator(client, false, + FABRIC8_CLIENT_DEFAULT_FIELD_MANAGER); + reconcileWithNewApproach(testResource, operator); + + var cm = getDependentConfigMap(); + + assertThat(cm.getMetadata().getManagedFields()).hasSize(2); + assertThat(cm.getMetadata().getManagedFields()) + .allMatch(fm -> fm.getManager().equals(FABRIC8_CLIENT_DEFAULT_FIELD_MANAGER)); + } + + private void reconcileAgainWithLegacy(Operator legacyOperator, + DependnetSSACustomResource testResource) { + legacyOperator.start(); + + testResource.getSpec().setValue(INITIAL_VALUE); + testResource.getMetadata().setResourceVersion(null); + client.resource(testResource).update(); + + await().untilAsserted(() -> { + var cm = getDependentConfigMap(); + assertThat(cm.getData()).containsEntry(SSAConfigMapDependent.DATA_KEY, INITIAL_VALUE); + }); + + legacyOperator.stop(); + } + + private DependnetSSACustomResource reconcileWithNewApproach( + DependnetSSACustomResource testResource, Operator operator) { + operator.start(); + + await().untilAsserted(() -> { + var cm = getDependentConfigMap(); + assertThat(cm).isNotNull(); + assertThat(cm.getData()).hasSize(1); + }); + + testResource.getSpec().setValue(CHANGED_VALUE); + testResource.getMetadata().setResourceVersion(null); + testResource = client.resource(testResource).update(); + + await().untilAsserted(() -> { + var cm = getDependentConfigMap(); + assertThat(cm.getData()).containsEntry(SSAConfigMapDependent.DATA_KEY, CHANGED_VALUE); + }); + operator.stop(); + return testResource; + } + + private ConfigMap getDependentConfigMap() { + return client.configMaps().inNamespace(namespace).withName(TEST_RESOURCE_NAME).get(); + } + + private DependnetSSACustomResource reconcileWithLegacyOperator(Operator legacyOperator) { + legacyOperator.start(); + + var testResource = client.resource(testResource()).create(); + + await().untilAsserted(() -> { + var cm = getDependentConfigMap(); + assertThat(cm).isNotNull(); + assertThat(cm.getMetadata().getManagedFields()).hasSize(1); + assertThat(cm.getData()).hasSize(1); + }); + + legacyOperator.stop(); + return testResource; + } + + + private Operator createOperator(KubernetesClient client, boolean legacyDependentHandling, + String fieldManager) { + Operator operator = new Operator(client, + o -> o.withLegacyCreateUpdateAndMatchingForDependentResources(legacyDependentHandling) + .withCloseClientOnStop(false)); + operator.register(new DependentSSAReconciler(), o -> { + o.settingNamespace(namespace); + if (fieldManager != null) { + o.withFieldManager(fieldManager); + } + }); + return operator; + } + + + public DependnetSSACustomResource testResource() { + DependnetSSACustomResource resource = new DependnetSSACustomResource(); + resource.setMetadata(new ObjectMetaBuilder() + .withNamespace(namespace) + .withName(TEST_RESOURCE_NAME) + .build()); + resource.setSpec(new DependentSSASpec()); + resource.getSpec().setValue(INITIAL_VALUE); + return resource; + } + +} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/dependentssa/DependentSSAReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/dependentssa/DependentSSAReconciler.java new file mode 100644 index 0000000000..d5c48256f7 --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/dependentssa/DependentSSAReconciler.java @@ -0,0 +1,27 @@ +package io.javaoperatorsdk.operator.sample.dependentssa; + +import java.util.concurrent.atomic.AtomicInteger; + +import io.javaoperatorsdk.operator.api.reconciler.*; +import io.javaoperatorsdk.operator.api.reconciler.dependent.Dependent; +import io.javaoperatorsdk.operator.support.TestExecutionInfoProvider; + +@ControllerConfiguration(dependents = {@Dependent(type = SSAConfigMapDependent.class)}) +public class DependentSSAReconciler + implements Reconciler, TestExecutionInfoProvider { + + private final AtomicInteger numberOfExecutions = new AtomicInteger(0); + + @Override + public UpdateControl reconcile( + DependnetSSACustomResource resource, + Context context) { + numberOfExecutions.addAndGet(1); + return UpdateControl.noUpdate(); + } + + public int getNumberOfExecutions() { + return numberOfExecutions.get(); + } + +} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/dependentssa/DependentSSASpec.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/dependentssa/DependentSSASpec.java new file mode 100644 index 0000000000..c7b4dd0053 --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/dependentssa/DependentSSASpec.java @@ -0,0 +1,15 @@ +package io.javaoperatorsdk.operator.sample.dependentssa; + +public class DependentSSASpec { + + private String value; + + public String getValue() { + return value; + } + + public DependentSSASpec setValue(String value) { + this.value = value; + return this; + } +} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/dependentssa/DependnetSSACustomResource.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/dependentssa/DependnetSSACustomResource.java new file mode 100644 index 0000000000..06834fe211 --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/dependentssa/DependnetSSACustomResource.java @@ -0,0 +1,15 @@ +package io.javaoperatorsdk.operator.sample.dependentssa; + +import io.fabric8.kubernetes.api.model.Namespaced; +import io.fabric8.kubernetes.client.CustomResource; +import io.fabric8.kubernetes.model.annotation.Group; +import io.fabric8.kubernetes.model.annotation.ShortNames; +import io.fabric8.kubernetes.model.annotation.Version; + +@Group("sample.javaoperatorsdk") +@Version("v1") +@ShortNames("dssa") +public class DependnetSSACustomResource + extends CustomResource + implements Namespaced { +} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/dependentssa/SSAConfigMapDependent.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/dependentssa/SSAConfigMapDependent.java new file mode 100644 index 0000000000..70f0ddcf15 --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/dependentssa/SSAConfigMapDependent.java @@ -0,0 +1,42 @@ +package io.javaoperatorsdk.operator.sample.dependentssa; + +import java.util.Map; +import java.util.concurrent.atomic.AtomicInteger; + +import io.fabric8.kubernetes.api.model.ConfigMap; +import io.fabric8.kubernetes.api.model.ConfigMapBuilder; +import io.fabric8.kubernetes.api.model.ObjectMetaBuilder; +import io.javaoperatorsdk.operator.api.reconciler.Context; +import io.javaoperatorsdk.operator.processing.dependent.kubernetes.CRUDKubernetesDependentResource; + +public class SSAConfigMapDependent extends + CRUDKubernetesDependentResource { + + public static AtomicInteger NUMBER_OF_UPDATES = new AtomicInteger(0); + + public static final String DATA_KEY = "key1"; + + public SSAConfigMapDependent() { + super(ConfigMap.class); + } + + @Override + protected ConfigMap desired(DependnetSSACustomResource primary, + Context context) { + return new ConfigMapBuilder() + .withMetadata(new ObjectMetaBuilder() + .withName(primary.getMetadata().getName()) + .withNamespace(primary.getMetadata().getNamespace()) + .build()) + .withData(Map.of(DATA_KEY, primary.getSpec().getValue())) + .build(); + } + + @Override + public ConfigMap update(ConfigMap actual, ConfigMap target, + DependnetSSACustomResource primary, + Context context) { + NUMBER_OF_UPDATES.incrementAndGet(); + return super.update(actual, target, primary, context); + } +} diff --git a/operator-framework/src/test/resources/io/javaoperatorsdk/operator/leader-elector-stop-role-noaccess.yaml b/operator-framework/src/test/resources/io/javaoperatorsdk/operator/leader-elector-stop-role-noaccess.yaml index b08bbb5c5a..7d0b451bc9 100644 --- a/operator-framework/src/test/resources/io/javaoperatorsdk/operator/leader-elector-stop-role-noaccess.yaml +++ b/operator-framework/src/test/resources/io/javaoperatorsdk/operator/leader-elector-stop-role-noaccess.yaml @@ -6,4 +6,4 @@ metadata: rules: - apiGroups: [ "" ] resources: [ "configmaps" ] - verbs: [ "get", "watch", "list","post", "delete", "create" ] \ No newline at end of file + verbs: [ "get", "watch", "list","post", "delete", "create","patch" ] \ No newline at end of file diff --git a/operator-framework/src/test/resources/io/javaoperatorsdk/operator/rback-test-full-access-role.yaml b/operator-framework/src/test/resources/io/javaoperatorsdk/operator/rback-test-full-access-role.yaml index 024f298462..d5fe6b5862 100644 --- a/operator-framework/src/test/resources/io/javaoperatorsdk/operator/rback-test-full-access-role.yaml +++ b/operator-framework/src/test/resources/io/javaoperatorsdk/operator/rback-test-full-access-role.yaml @@ -9,6 +9,6 @@ rules: verbs: [ "get", "watch", "list","post", "delete" ] - apiGroups: [ "" ] resources: [ "configmaps" ] - verbs: [ "get", "watch", "list","post", "delete", "create" ] + verbs: [ "get", "watch", "list","post", "delete", "create","patch" ] diff --git a/operator-framework/src/test/resources/io/javaoperatorsdk/operator/rback-test-no-configmap-access.yaml b/operator-framework/src/test/resources/io/javaoperatorsdk/operator/rback-test-no-configmap-access.yaml index bac8c1149f..2afe7e2fdc 100644 --- a/operator-framework/src/test/resources/io/javaoperatorsdk/operator/rback-test-no-configmap-access.yaml +++ b/operator-framework/src/test/resources/io/javaoperatorsdk/operator/rback-test-no-configmap-access.yaml @@ -6,6 +6,6 @@ metadata: rules: - apiGroups: [ "sample.javaoperatorsdk" ] resources: [ "informerrelatedbehaviortestcustomresources" ] - verbs: [ "get", "watch", "list","post", "delete" ] + verbs: [ "get", "watch", "list","post", "delete","patch" ] diff --git a/operator-framework/src/test/resources/io/javaoperatorsdk/operator/rback-test-no-cr-access.yaml b/operator-framework/src/test/resources/io/javaoperatorsdk/operator/rback-test-no-cr-access.yaml index 8c6ae85aac..63e9951fda 100644 --- a/operator-framework/src/test/resources/io/javaoperatorsdk/operator/rback-test-no-cr-access.yaml +++ b/operator-framework/src/test/resources/io/javaoperatorsdk/operator/rback-test-no-cr-access.yaml @@ -6,5 +6,5 @@ metadata: rules: - apiGroups: [""] resources: [ "configmaps" ] - verbs: [ "get", "watch", "list","post", "delete","create" ] + verbs: [ "get", "watch", "list","post", "delete","create","patch"] diff --git a/operator-framework/src/test/resources/io/javaoperatorsdk/operator/rback-test-only-main-ns-access.yaml b/operator-framework/src/test/resources/io/javaoperatorsdk/operator/rback-test-only-main-ns-access.yaml index a780209c2d..776acfb258 100644 --- a/operator-framework/src/test/resources/io/javaoperatorsdk/operator/rback-test-only-main-ns-access.yaml +++ b/operator-framework/src/test/resources/io/javaoperatorsdk/operator/rback-test-only-main-ns-access.yaml @@ -8,4 +8,4 @@ rules: verbs: [ "get", "watch", "list","post", "delete" ] - apiGroups: [ "" ] resources: [ "configmaps" ] - verbs: [ "get", "watch", "list","post", "delete", "create" ] \ No newline at end of file + verbs: [ "get", "watch", "list","post", "delete", "create","patch" ] \ No newline at end of file diff --git a/testi-multi-container.yaml b/testi-multi-container.yaml new file mode 100644 index 0000000000..92ece6df00 --- /dev/null +++ b/testi-multi-container.yaml @@ -0,0 +1,21 @@ +apiVersion: v1 +kind: Pod +metadata: + name: shared-storage +spec: + volumes: + - name: shared-data + emptyDir: {} + containers: + - name: nginx-container + image: nginx + volumeMounts: + - name: shared-data + mountPath: /usr/share/nginx/html + - name: debian-container + image: debian + volumeMounts: + - name: shared-data + mountPath: /data + command: ["/bin/sh"] + args: ["-c", "echo Level Up Blue Team! > /data/index.html"] \ No newline at end of file From f605a6b089b6589ad0b80df2c1813a1a92efc532 Mon Sep 17 00:00:00 2001 From: csviri Date: Thu, 8 Jun 2023 15:38:32 +0200 Subject: [PATCH 02/25] format --- .../SSABasedGenericKubernetesResourceMatcher.java | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java index 369bc0cd26..8b40f7162e 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java @@ -17,11 +17,11 @@ /** * Matches the actual state on the server vs the desired state. Based on the managedFields of SSA. * - * The ide of algorithm is basically trivial, we convert resources to Map/List composition. - * The actual resource (from the server) is pruned, all the fields which are not mentioed in managedFields - * of the target manager is removed. Some irrelevant fields are also removed from desired. And the - * two resulted Maps are compared for equality. The implementation is a bit nasty since have to deal with - * some specific cases of managedFields format. + * The ide of algorithm is basically trivial, we convert resources to Map/List composition. The + * actual resource (from the server) is pruned, all the fields which are not mentioed in + * managedFields of the target manager is removed. Some irrelevant fields are also removed from + * desired. And the two resulted Maps are compared for equality. The implementation is a bit nasty + * since have to deal with some specific cases of managedFields format. * * @param matched resource type */ @@ -158,7 +158,8 @@ private static boolean isNestedValue(Object managedFieldValue) { // list entries referenced by key, or when "k:" prefix is used private void handleListKeyEntrySet(Map result, Map actualMap, - ObjectMapper objectMapper, String keyInActual, Set> managedEntrySet) { + ObjectMapper objectMapper, String keyInActual, + Set> managedEntrySet) { var valueList = new ArrayList<>(); result.put(keyInActual, valueList); var actualValueList = (List>) actualMap.get(keyInActual); From 1ae332d2945bbb1e7a0247857eaefcb70b743f68 Mon Sep 17 00:00:00 2001 From: csviri Date: Thu, 8 Jun 2023 15:48:01 +0200 Subject: [PATCH 03/25] fixes after rebase --- .../operator/api/config/BaseConfigurationService.java | 3 ++- .../io/javaoperatorsdk/operator/api/reconciler/Context.java | 1 - 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/BaseConfigurationService.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/BaseConfigurationService.java index d6bac0f5c4..a5167af747 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/BaseConfigurationService.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/BaseConfigurationService.java @@ -158,7 +158,8 @@ protected

ControllerConfiguration

configFor(Reconcile io.javaoperatorsdk.operator.api.reconciler.ControllerConfiguration::labelSelector, Constants.NO_VALUE_SET), null, - Utils.instantiate(annotation.itemStore(), ItemStore.class, context), this); + Utils.instantiate(annotation.itemStore(), ItemStore.class, context), dependentFieldManager, + this); ResourceEventFilter

answer = deprecatedEventFilter(annotation); config.setEventFilter(answer != null ? answer : ResourceEventFilters.passthrough()); diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/Context.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/Context.java index a6482119c3..5043ed675c 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/Context.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/Context.java @@ -3,7 +3,6 @@ import java.util.Optional; import java.util.Set; import java.util.concurrent.ExecutorService; -import java.util.concurrent.ExecutorService; import java.util.stream.Stream; import io.fabric8.kubernetes.api.model.HasMetadata; From 4ee4607ac1a71fc216179c38fa27dae1075bb2ad Mon Sep 17 00:00:00 2001 From: csviri Date: Thu, 8 Jun 2023 16:17:41 +0200 Subject: [PATCH 04/25] docs --- .../operator/api/config/ControllerConfiguration.java | 6 +++++- .../operator/api/reconciler/ControllerConfiguration.java | 6 +++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java index 37c2e39503..f3c18493da 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java @@ -127,7 +127,11 @@ default Set getEffectiveNamespaces() { return ResourceConfiguration.super.getEffectiveNamespaces(getConfigurationService()); } - // todo javadoc + /** + * fieldManager name used for the dependent resource to do create/update (in practice SSA Patch). + * In future this might be used also for other purposes like to make status updates of the primary + * resource with SSA. + */ default String fieldManager() { return DEFAULT_FIELD_MANAGER; } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ControllerConfiguration.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ControllerConfiguration.java index 0fd731dd78..8b195e9bea 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ControllerConfiguration.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ControllerConfiguration.java @@ -132,6 +132,10 @@ MaxReconciliationInterval maxReconciliationInterval() default @MaxReconciliation Class itemStore() default ItemStore.class; - // todo javadoc + /** + * fieldManager name used for the dependent resource to do create/update (in practice SSA Patch). + * In future this might be used also for other purposes like to make status updates of the primary + * resource with SSA. + */ String fieldManager() default DEFAULT_FIELD_MANAGER; } From f687134c34bdb452725bd98c0b9055dd566d75a9 Mon Sep 17 00:00:00 2001 From: csviri Date: Fri, 9 Jun 2023 09:07:22 +0200 Subject: [PATCH 05/25] tests and docs --- .../api/config/ConfigurationService.java | 8 ++++-- ...dGenericKubernetesResourceMatcherTest.java | 27 +++++++++++++++++-- 2 files changed, 31 insertions(+), 4 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java index 263eee2f03..593c77cf47 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java @@ -265,8 +265,12 @@ default ExecutorServiceManager getExecutorServiceManager() { return new ExecutorServiceManager(this); } - // todo test transition - // todo javadoc + /** + * The default approach how the Kubernetes Dependent Resources are created/updated but also + * matched was changed, by default it uses SSA based approach. This is a feature flag to use the + * old approach. Note that the legacy approach might be removed in the future (especially for + * create and update) along with this flag. + */ default boolean legacyCreateUpdateAndMatchingForDependentResources() { return false; } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcherTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcherTest.java index 11a5fa1db4..18f77ed410 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcherTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcherTest.java @@ -1,5 +1,7 @@ package io.javaoperatorsdk.operator.processing.dependent.kubernetes; +import java.util.Map; + import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -54,7 +56,6 @@ void emptyListElementMatchesAllFields() { assertThat(matcher.matches(actualConfigMap, desiredConfigMap, mockedContext)).isTrue(); } - // the whole "rules:" part is just implicitly managed @Test void wholeComplexFieldManaged() { @@ -66,7 +67,6 @@ void wholeComplexFieldManaged() { assertThat(matcher.matches(actualConfigMap, desiredConfigMap, mockedContext)).isTrue(); } - @Test void multiItemList() { var desiredConfigMap = loadResource("multi-container-pod-desired.yaml", @@ -77,6 +77,29 @@ void multiItemList() { assertThat(matcher.matches(actualConfigMap, desiredConfigMap, mockedContext)).isTrue(); } + @Test + void changeValueInDesiredMakesMatchFail() { + var desiredConfigMap = loadResource("configmap.empty-owner-reference-desired.yaml", + ConfigMap.class); + desiredConfigMap.getData().put("key1", "different value"); + var actualConfigMap = loadResource("configmap.empty-owner-reference.yaml", + ConfigMap.class); + + assertThat(matcher.matches(actualConfigMap, desiredConfigMap, mockedContext)).isFalse(); + } + + @Test + void addedLabelInDesiredMakesMatchFail() { + var desiredConfigMap = loadResource("configmap.empty-owner-reference-desired.yaml", + ConfigMap.class); + desiredConfigMap.getMetadata().setLabels(Map.of("newlabel", "val")); + + var actualConfigMap = loadResource("configmap.empty-owner-reference.yaml", + ConfigMap.class); + + assertThat(matcher.matches(actualConfigMap, desiredConfigMap, mockedContext)).isFalse(); + } + private R loadResource(String fileName, Class clazz) { return ReconcilerUtils.loadYaml(clazz, SSABasedGenericKubernetesResourceMatcherTest.class, fileName); From 5acd152303b35921f2c863583f5b863a5a41de33 Mon Sep 17 00:00:00 2001 From: csviri Date: Fri, 9 Jun 2023 09:41:31 +0200 Subject: [PATCH 06/25] test fix --- .../operator/api/config/BaseConfigurationService.java | 4 ++-- .../operator/api/config/ControllerConfiguration.java | 7 +++++-- .../operator/api/reconciler/ControllerConfiguration.java | 4 ++-- .../SSABasedGenericKubernetesResourceMatcherTest.java | 2 +- .../javaoperatorsdk/operator/DependentSSAMigrationIT.java | 5 ++++- 5 files changed, 14 insertions(+), 8 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/BaseConfigurationService.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/BaseConfigurationService.java index a5167af747..7d3964e01a 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/BaseConfigurationService.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/BaseConfigurationService.java @@ -33,7 +33,7 @@ import com.fasterxml.jackson.databind.ObjectMapper; -import static io.javaoperatorsdk.operator.api.config.ControllerConfiguration.DEFAULT_FIELD_MANAGER; +import static io.javaoperatorsdk.operator.api.config.ControllerConfiguration.CONTROLLER_NAME_AS_FIELD_MANAGER; import static io.javaoperatorsdk.operator.api.reconciler.Constants.DEFAULT_NAMESPACES_SET; public class BaseConfigurationService extends AbstractConfigurationService { @@ -139,7 +139,7 @@ protected

ControllerConfiguration

configFor(Reconcile final var dependentFieldManager = valueOrDefault( annotation, io.javaoperatorsdk.operator.api.reconciler.ControllerConfiguration::fieldManager, - DEFAULT_FIELD_MANAGER); + CONTROLLER_NAME_AS_FIELD_MANAGER); final var config = new ResolvedControllerConfiguration

( resourceClass, name, generationAware, diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java index f3c18493da..87c73d00fd 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java @@ -22,7 +22,10 @@ public interface ControllerConfiguration

extends Resource @SuppressWarnings("rawtypes") RateLimiter DEFAULT_RATE_LIMITER = LinearRateLimiter.deactivatedRateLimiter(); - String DEFAULT_FIELD_MANAGER = "controller"; + /** + * Will use the controller name as fieldManager if set. + */ + String CONTROLLER_NAME_AS_FIELD_MANAGER = "default"; String FABRIC8_CLIENT_DEFAULT_FIELD_MANAGER = "fabric8-kubernetes-client"; default String getName() { @@ -133,7 +136,7 @@ default Set getEffectiveNamespaces() { * resource with SSA. */ default String fieldManager() { - return DEFAULT_FIELD_MANAGER; + return getName(); } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ControllerConfiguration.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ControllerConfiguration.java index 8b195e9bea..0bb4d4c399 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ControllerConfiguration.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ControllerConfiguration.java @@ -17,7 +17,7 @@ import io.javaoperatorsdk.operator.processing.retry.GenericRetry; import io.javaoperatorsdk.operator.processing.retry.Retry; -import static io.javaoperatorsdk.operator.api.config.ControllerConfiguration.DEFAULT_FIELD_MANAGER; +import static io.javaoperatorsdk.operator.api.config.ControllerConfiguration.CONTROLLER_NAME_AS_FIELD_MANAGER; @Inherited @Retention(RetentionPolicy.RUNTIME) @@ -137,5 +137,5 @@ MaxReconciliationInterval maxReconciliationInterval() default @MaxReconciliation * In future this might be used also for other purposes like to make status updates of the primary * resource with SSA. */ - String fieldManager() default DEFAULT_FIELD_MANAGER; + String fieldManager() default CONTROLLER_NAME_AS_FIELD_MANAGER; } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcherTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcherTest.java index 18f77ed410..9513b8aabe 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcherTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcherTest.java @@ -28,7 +28,7 @@ class SSABasedGenericKubernetesResourceMatcherTest { @SuppressWarnings("unchecked") void setup() { var controllerConfiguration = mock(ControllerConfiguration.class); - when(controllerConfiguration.fieldManager()).thenCallRealMethod(); + when(controllerConfiguration.fieldManager()).thenReturn("controller"); var configurationService = mock(ConfigurationService.class); when(configurationService.getObjectMapper()).thenCallRealMethod(); when(controllerConfiguration.getConfigurationService()).thenReturn(configurationService); diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/DependentSSAMigrationIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/DependentSSAMigrationIT.java index 830a41dddf..7afe247d46 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/DependentSSAMigrationIT.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/DependentSSAMigrationIT.java @@ -76,7 +76,10 @@ void usingDefaultFieldManagerDoesNotCreatesANewOneWithApplyOperation() { assertThat(cm.getMetadata().getManagedFields()).hasSize(2); assertThat(cm.getMetadata().getManagedFields()) - .allMatch(fm -> fm.getManager().equals(FABRIC8_CLIENT_DEFAULT_FIELD_MANAGER)); + // Jetty seems to be a bug in fabric8 client, it is only the default fieldManager if Jetty + // is used as http client + .allMatch(fm -> fm.getManager().equals(FABRIC8_CLIENT_DEFAULT_FIELD_MANAGER) + || fm.getManager().equals("Jetty")); } private void reconcileAgainWithLegacy(Operator legacyOperator, From 192e37eb433b680e09c352896d0f48b01c3254b1 Mon Sep 17 00:00:00 2001 From: csviri Date: Fri, 9 Jun 2023 10:16:58 +0200 Subject: [PATCH 07/25] minor improvements --- .../operator/api/config/BaseConfigurationService.java | 7 +++---- .../operator/api/config/ControllerConfiguration.java | 1 - .../javaoperatorsdk/operator/DependentSSAMigrationIT.java | 1 + 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/BaseConfigurationService.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/BaseConfigurationService.java index 7d3964e01a..d92294d048 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/BaseConfigurationService.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/BaseConfigurationService.java @@ -136,10 +136,9 @@ protected

ControllerConfiguration

configFor(Reconcile timeUnit = reconciliationInterval.timeUnit(); } - final var dependentFieldManager = valueOrDefault( - annotation, - io.javaoperatorsdk.operator.api.reconciler.ControllerConfiguration::fieldManager, - CONTROLLER_NAME_AS_FIELD_MANAGER); + final var dependentFieldManager = + annotation.fieldManager().equals(CONTROLLER_NAME_AS_FIELD_MANAGER) ? name + : annotation.fieldManager(); final var config = new ResolvedControllerConfiguration

( resourceClass, name, generationAware, diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java index 87c73d00fd..aea51f7e32 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java @@ -26,7 +26,6 @@ public interface ControllerConfiguration

extends Resource * Will use the controller name as fieldManager if set. */ String CONTROLLER_NAME_AS_FIELD_MANAGER = "default"; - String FABRIC8_CLIENT_DEFAULT_FIELD_MANAGER = "fabric8-kubernetes-client"; default String getName() { return ensureValidName(null, getAssociatedReconcilerClassName()); diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/DependentSSAMigrationIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/DependentSSAMigrationIT.java index 7afe247d46..bd4e015e9d 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/DependentSSAMigrationIT.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/DependentSSAMigrationIT.java @@ -23,6 +23,7 @@ class DependentSSAMigrationIT { + public static final String FABRIC8_CLIENT_DEFAULT_FIELD_MANAGER = "fabric8-kubernetes-client"; public static final String TEST_RESOURCE_NAME = "test1"; public static final String INITIAL_VALUE = "INITIAL_VALUE"; public static final String CHANGED_VALUE = "CHANGED_VALUE"; From 8b5c80da0b2cf5b73054e41b1ed7337eb50e29cf Mon Sep 17 00:00:00 2001 From: csviri Date: Fri, 9 Jun 2023 10:20:13 +0200 Subject: [PATCH 08/25] fix --- .../io/javaoperatorsdk/operator/DependentSSAMigrationIT.java | 1 - 1 file changed, 1 deletion(-) diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/DependentSSAMigrationIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/DependentSSAMigrationIT.java index bd4e015e9d..8bbc00ba58 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/DependentSSAMigrationIT.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/DependentSSAMigrationIT.java @@ -17,7 +17,6 @@ import io.javaoperatorsdk.operator.sample.dependentssa.DependnetSSACustomResource; import io.javaoperatorsdk.operator.sample.dependentssa.SSAConfigMapDependent; -import static io.javaoperatorsdk.operator.api.config.ControllerConfiguration.FABRIC8_CLIENT_DEFAULT_FIELD_MANAGER; import static org.assertj.core.api.Assertions.assertThat; import static org.awaitility.Awaitility.await; From 4c7b339287888bf9645ec76415471c06c5c6c09b Mon Sep 17 00:00:00 2001 From: csviri Date: Fri, 9 Jun 2023 14:52:00 +0200 Subject: [PATCH 09/25] separate feature flags --- .../api/config/ConfigurationService.java | 21 ++++++++++---- .../config/ConfigurationServiceOverrider.java | 28 ++++++++++++++----- .../KubernetesDependentResource.java | 12 ++++---- .../operator/DependentSSAMigrationIT.java | 2 +- 4 files changed, 43 insertions(+), 20 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java index 593c77cf47..75cc95468b 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java @@ -266,12 +266,21 @@ default ExecutorServiceManager getExecutorServiceManager() { } /** - * The default approach how the Kubernetes Dependent Resources are created/updated but also - * matched was changed, by default it uses SSA based approach. This is a feature flag to use the - * old approach. Note that the legacy approach might be removed in the future (especially for - * create and update) along with this flag. + * The default approach how the Kubernetes Dependent Resources are created/updated was changed, by + * default it uses SSA based approach. This is a feature flag to use the old approach. Note that + * the legacy approach might be removed in the future. */ - default boolean legacyCreateUpdateAndMatchingForDependentResources() { - return false; + default boolean ssaBasedCreateUpdateForDependentResource() { + return true; + } + + /** + * We introduced a new generic matching algorithm for Kubernetes Dependent Resources, since it + * very quite high complexity and prone to bugs, this feature flag is used to revert it to the old + * default approach. However, this flag might be reverted in the future. + */ + default boolean ssaBasedDefaultMatchingForDependentResources() { + return true; } + } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java index 02e3b82359..686e7bdcf7 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java @@ -32,7 +32,8 @@ public class ConfigurationServiceOverrider { private Boolean stopOnInformerErrorDuringStartup; private Duration cacheSyncTimeout; private ResourceClassResolver resourceClassResolver; - private Boolean legacyCreateUpdateAndMatchingForDependentResources; + private Boolean ssaBasedCreateUpdateForDependentResource; + private Boolean ssaBasedDefaultMatchingForDependentResources; ConfigurationServiceOverrider(ConfigurationService original) { this.original = original; @@ -140,9 +141,15 @@ public ConfigurationServiceOverrider withResourceClassResolver( return this; } - public ConfigurationServiceOverrider withLegacyCreateUpdateAndMatchingForDependentResources( + public ConfigurationServiceOverrider withSSABasedCreateUpdateForDependentResource( boolean value) { - this.legacyCreateUpdateAndMatchingForDependentResources = value; + this.ssaBasedCreateUpdateForDependentResource = value; + return this; + } + + public ConfigurationServiceOverrider withSSABasedDefaultMatchingForDependentResources( + boolean value) { + this.ssaBasedDefaultMatchingForDependentResources = value; return this; } @@ -257,10 +264,17 @@ public ResourceClassResolver getResourceClassResolver() { } @Override - public boolean legacyCreateUpdateAndMatchingForDependentResources() { - return legacyCreateUpdateAndMatchingForDependentResources != null - ? legacyCreateUpdateAndMatchingForDependentResources - : super.legacyCreateUpdateAndMatchingForDependentResources(); + public boolean ssaBasedCreateUpdateForDependentResource() { + return ssaBasedCreateUpdateForDependentResource != null + ? ssaBasedCreateUpdateForDependentResource + : super.ssaBasedCreateUpdateForDependentResource(); + } + + @Override + public boolean ssaBasedDefaultMatchingForDependentResources() { + return ssaBasedDefaultMatchingForDependentResources != null + ? ssaBasedDefaultMatchingForDependentResources + : super.ssaBasedDefaultMatchingForDependentResources(); } }; } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java index fbde7bd211..46be09bbad 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java @@ -135,8 +135,8 @@ protected R handleUpdate(R actual, R desired, P primary, Context

context) { @SuppressWarnings("unused") public R create(R target, P primary, Context

context) { - if (context.getControllerConfiguration().getConfigurationService() - .legacyCreateUpdateAndMatchingForDependentResources()) { + if (!context.getControllerConfiguration().getConfigurationService() + .ssaBasedCreateUpdateForDependentResource()) { return prepare(target, primary, "Creating").create(); } else { return prepare(target, primary, "Creating").patch(getSSAPatchContext(context)); @@ -144,8 +144,8 @@ public R create(R target, P primary, Context

context) { } public R update(R actual, R target, P primary, Context

context) { - if (context.getControllerConfiguration().getConfigurationService() - .legacyCreateUpdateAndMatchingForDependentResources()) { + if (!context.getControllerConfiguration().getConfigurationService() + .ssaBasedCreateUpdateForDependentResource()) { var updatedActual = processor.replaceSpecOnActual(actual, target, context); return prepare(updatedActual, primary, "Updating").replace(); } else { @@ -154,8 +154,8 @@ public R update(R actual, R target, P primary, Context

context) { } public Result match(R actualResource, P primary, Context

context) { - if (context.getControllerConfiguration().getConfigurationService() - .legacyCreateUpdateAndMatchingForDependentResources()) { + if (!context.getControllerConfiguration().getConfigurationService() + .ssaBasedDefaultMatchingForDependentResources()) { return GenericKubernetesResourceMatcher.match(this, actualResource, primary, context, false); } else { final var desired = desired(primary, context); diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/DependentSSAMigrationIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/DependentSSAMigrationIT.java index 8bbc00ba58..dc6e1e3f74 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/DependentSSAMigrationIT.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/DependentSSAMigrationIT.java @@ -144,7 +144,7 @@ private DependnetSSACustomResource reconcileWithLegacyOperator(Operator legacyOp private Operator createOperator(KubernetesClient client, boolean legacyDependentHandling, String fieldManager) { Operator operator = new Operator(client, - o -> o.withLegacyCreateUpdateAndMatchingForDependentResources(legacyDependentHandling) + o -> o.withSSABasedCreateUpdateForDependentResource(legacyDependentHandling) .withCloseClientOnStop(false)); operator.register(new DependentSSAReconciler(), o -> { o.settingNamespace(namespace); From 64f8a7fcba7b909f0e585234b72a6357613c740c Mon Sep 17 00:00:00 2001 From: csviri Date: Fri, 9 Jun 2023 15:32:33 +0200 Subject: [PATCH 10/25] fixes --- .../operator/api/config/ControllerConfiguration.java | 2 +- .../api/reconciler/ControllerConfiguration.java | 2 +- ...SSABasedGenericKubernetesResourceMatcherTest.java | 12 ++++++++++++ .../operator/DependentSSAMigrationIT.java | 3 ++- 4 files changed, 16 insertions(+), 3 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java index aea51f7e32..362c4d4546 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java @@ -132,7 +132,7 @@ default Set getEffectiveNamespaces() { /** * fieldManager name used for the dependent resource to do create/update (in practice SSA Patch). * In future this might be used also for other purposes like to make status updates of the primary - * resource with SSA. + * resource with SSA. If not set, sanitized controller name is used as field manager. */ default String fieldManager() { return getName(); diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ControllerConfiguration.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ControllerConfiguration.java index 0bb4d4c399..be680fc0d6 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ControllerConfiguration.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ControllerConfiguration.java @@ -135,7 +135,7 @@ MaxReconciliationInterval maxReconciliationInterval() default @MaxReconciliation /** * fieldManager name used for the dependent resource to do create/update (in practice SSA Patch). * In future this might be used also for other purposes like to make status updates of the primary - * resource with SSA. + * resource with SSA. If not set, sanitized controller name is used as field manager. */ String fieldManager() default CONTROLLER_NAME_AS_FIELD_MANAGER; } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcherTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcherTest.java index 9513b8aabe..3864ad1c43 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcherTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcherTest.java @@ -88,6 +88,18 @@ void changeValueInDesiredMakesMatchFail() { assertThat(matcher.matches(actualConfigMap, desiredConfigMap, mockedContext)).isFalse(); } + @Test + void changeValueActualMakesMatchFail() { + var desiredConfigMap = loadResource("configmap.empty-owner-reference-desired.yaml", + ConfigMap.class); + + var actualConfigMap = loadResource("configmap.empty-owner-reference.yaml", + ConfigMap.class); + actualConfigMap.getData().put("key1", "different value"); + + assertThat(matcher.matches(actualConfigMap, desiredConfigMap, mockedContext)).isFalse(); + } + @Test void addedLabelInDesiredMakesMatchFail() { var desiredConfigMap = loadResource("configmap.empty-owner-reference-desired.yaml", diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/DependentSSAMigrationIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/DependentSSAMigrationIT.java index dc6e1e3f74..0788bfe46a 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/DependentSSAMigrationIT.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/DependentSSAMigrationIT.java @@ -144,7 +144,8 @@ private DependnetSSACustomResource reconcileWithLegacyOperator(Operator legacyOp private Operator createOperator(KubernetesClient client, boolean legacyDependentHandling, String fieldManager) { Operator operator = new Operator(client, - o -> o.withSSABasedCreateUpdateForDependentResource(legacyDependentHandling) + o -> o.withSSABasedCreateUpdateForDependentResource(!legacyDependentHandling) + .withSSABasedDefaultMatchingForDependentResources(!legacyDependentHandling) .withCloseClientOnStop(false)); operator.register(new DependentSSAReconciler(), o -> { o.settingNamespace(namespace); From c5a54bd817884b55a42bb01cb10436935645b9f8 Mon Sep 17 00:00:00 2001 From: csviri Date: Fri, 9 Jun 2023 15:35:18 +0200 Subject: [PATCH 11/25] fix docs --- .../operator/api/config/ConfigurationService.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java index 75cc95468b..871f7089a8 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java @@ -275,9 +275,10 @@ default boolean ssaBasedCreateUpdateForDependentResource() { } /** - * We introduced a new generic matching algorithm for Kubernetes Dependent Resources, since it - * very quite high complexity and prone to bugs, this feature flag is used to revert it to the old - * default approach. However, this flag might be reverted in the future. + * A new generic matching algorithm for Kubernetes Dependent Resources was introduced, since it + * has quite high complexity, this feature flag is used to revert it to the old default approach + * in case some of the use cases something goes wrong. However, this flag might be removed in the + * future. */ default boolean ssaBasedDefaultMatchingForDependentResources() { return true; From a6a150fe8263b9e373eb11be0ec707b4ac26938c Mon Sep 17 00:00:00 2001 From: csviri Date: Tue, 13 Jun 2023 08:32:19 +0200 Subject: [PATCH 12/25] ssa api --- .../kubernetes/KubernetesDependentResource.java | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java index 46be09bbad..6ec6da8aa8 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java @@ -139,7 +139,8 @@ public R create(R target, P primary, Context

context) { .ssaBasedCreateUpdateForDependentResource()) { return prepare(target, primary, "Creating").create(); } else { - return prepare(target, primary, "Creating").patch(getSSAPatchContext(context)); + return prepare(target, primary, "Creating").forceConflicts() + .serverSideApply(); } } @@ -149,7 +150,8 @@ public R update(R actual, R target, P primary, Context

context) { var updatedActual = processor.replaceSpecOnActual(actual, target, context); return prepare(updatedActual, primary, "Updating").replace(); } else { - return prepare(target, primary, "Updating").patch(getSSAPatchContext(context)); + target.getMetadata().setResourceVersion(actual.getMetadata().getResourceVersion()); + return prepare(target, primary, "Updating").forceConflicts().serverSideApply(); } } @@ -290,12 +292,4 @@ public boolean isDeletable() { return super.isDeletable() && !garbageCollected; } - private PatchContext getSSAPatchContext(Context

context) { - return new PatchContext.Builder() - .withPatchType(PatchType.SERVER_SIDE_APPLY) - .withForce(true) - .withFieldManager(context.getControllerConfiguration().fieldManager()) - .build(); - } - } From 6cf5bffd7c89aae7f627c2bfff0a94db284f7d90 Mon Sep 17 00:00:00 2001 From: csviri Date: Tue, 13 Jun 2023 09:00:49 +0200 Subject: [PATCH 13/25] manager name --- .../kubernetes/KubernetesDependentResource.java | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java index 6ec6da8aa8..acf929b29d 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java @@ -139,8 +139,10 @@ public R create(R target, P primary, Context

context) { .ssaBasedCreateUpdateForDependentResource()) { return prepare(target, primary, "Creating").create(); } else { - return prepare(target, primary, "Creating").forceConflicts() - .serverSideApply(); + return prepare(target, primary, "Creating") + .fieldManager(context.getControllerConfiguration().fieldManager()) + .forceConflicts() + .serverSideApply(); } } @@ -151,7 +153,9 @@ public R update(R actual, R target, P primary, Context

context) { return prepare(updatedActual, primary, "Updating").replace(); } else { target.getMetadata().setResourceVersion(actual.getMetadata().getResourceVersion()); - return prepare(target, primary, "Updating").forceConflicts().serverSideApply(); + return prepare(target, primary, "Updating") + .fieldManager(context.getControllerConfiguration().fieldManager()) + .forceConflicts().serverSideApply(); } } From a9d526026c5ac677f83fb177c6713305717cd235 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Tue, 13 Jun 2023 14:57:53 +0200 Subject: [PATCH 14/25] docs: improve wording --- .../api/config/ConfigurationService.java | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java index 871f7089a8..dd0129289f 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java @@ -266,19 +266,23 @@ default ExecutorServiceManager getExecutorServiceManager() { } /** - * The default approach how the Kubernetes Dependent Resources are created/updated was changed, by - * default it uses SSA based approach. This is a feature flag to use the old approach. Note that - * the legacy approach might be removed in the future. + * Allows to revert to the 4.3 behavior when it comes to creating or updating Kubernetes Dependent + * Resources when set to {@code false}. The default approach how these resources are + * created/updated was change to use + * Server-Side + * Apply (SSA) by default. Note that the legacy approach, and this setting, might be removed + * in the future. */ default boolean ssaBasedCreateUpdateForDependentResource() { return true; } /** - * A new generic matching algorithm for Kubernetes Dependent Resources was introduced, since it - * has quite high complexity, this feature flag is used to revert it to the old default approach - * in case some of the use cases something goes wrong. However, this flag might be removed in the - * future. + * Allows to revert to the 4.3 generic matching algorithm for Kubernetes Dependent Resources when + * set to {@code false}. Version 4.4 introduced a new generic matching algorithm for Kubernetes + * Dependent Resources which is quite complex. As a consequence, we introduced this setting to + * allow folks to revert to the previous matching algorithm if needed. Note, however, that the + * legacy algorithm, and this setting, might be removed in the future. */ default boolean ssaBasedDefaultMatchingForDependentResources() { return true; From 93a9810971b7fe782effaf475e7afee92b7a1a23 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Tue, 13 Jun 2023 15:00:39 +0200 Subject: [PATCH 15/25] refactor: harmonize method spelling --- .../operator/api/config/ConfigurationService.java | 4 ++-- .../api/config/ConfigurationServiceOverrider.java | 14 +++++++------- .../kubernetes/KubernetesDependentResource.java | 4 ++-- .../operator/DependentSSAMigrationIT.java | 2 +- 4 files changed, 12 insertions(+), 12 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java index dd0129289f..1ab358155e 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java @@ -269,11 +269,11 @@ default ExecutorServiceManager getExecutorServiceManager() { * Allows to revert to the 4.3 behavior when it comes to creating or updating Kubernetes Dependent * Resources when set to {@code false}. The default approach how these resources are * created/updated was change to use - * Server-Side + * Server-Side * Apply (SSA) by default. Note that the legacy approach, and this setting, might be removed * in the future. */ - default boolean ssaBasedCreateUpdateForDependentResource() { + default boolean ssaBasedCreateUpdateForDependentResources() { return true; } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java index 686e7bdcf7..b2b4b93b12 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java @@ -32,7 +32,7 @@ public class ConfigurationServiceOverrider { private Boolean stopOnInformerErrorDuringStartup; private Duration cacheSyncTimeout; private ResourceClassResolver resourceClassResolver; - private Boolean ssaBasedCreateUpdateForDependentResource; + private Boolean ssaBasedCreateUpdateForDependentResources; private Boolean ssaBasedDefaultMatchingForDependentResources; ConfigurationServiceOverrider(ConfigurationService original) { @@ -141,9 +141,9 @@ public ConfigurationServiceOverrider withResourceClassResolver( return this; } - public ConfigurationServiceOverrider withSSABasedCreateUpdateForDependentResource( + public ConfigurationServiceOverrider withSSABasedCreateUpdateForDependentResources( boolean value) { - this.ssaBasedCreateUpdateForDependentResource = value; + this.ssaBasedCreateUpdateForDependentResources = value; return this; } @@ -264,10 +264,10 @@ public ResourceClassResolver getResourceClassResolver() { } @Override - public boolean ssaBasedCreateUpdateForDependentResource() { - return ssaBasedCreateUpdateForDependentResource != null - ? ssaBasedCreateUpdateForDependentResource - : super.ssaBasedCreateUpdateForDependentResource(); + public boolean ssaBasedCreateUpdateForDependentResources() { + return ssaBasedCreateUpdateForDependentResources != null + ? ssaBasedCreateUpdateForDependentResources + : super.ssaBasedCreateUpdateForDependentResources(); } @Override diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java index acf929b29d..ba14509de5 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java @@ -136,7 +136,7 @@ protected R handleUpdate(R actual, R desired, P primary, Context

context) { @SuppressWarnings("unused") public R create(R target, P primary, Context

context) { if (!context.getControllerConfiguration().getConfigurationService() - .ssaBasedCreateUpdateForDependentResource()) { + .ssaBasedCreateUpdateForDependentResources()) { return prepare(target, primary, "Creating").create(); } else { return prepare(target, primary, "Creating") @@ -148,7 +148,7 @@ public R create(R target, P primary, Context

context) { public R update(R actual, R target, P primary, Context

context) { if (!context.getControllerConfiguration().getConfigurationService() - .ssaBasedCreateUpdateForDependentResource()) { + .ssaBasedCreateUpdateForDependentResources()) { var updatedActual = processor.replaceSpecOnActual(actual, target, context); return prepare(updatedActual, primary, "Updating").replace(); } else { diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/DependentSSAMigrationIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/DependentSSAMigrationIT.java index 0788bfe46a..7bedde86fe 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/DependentSSAMigrationIT.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/DependentSSAMigrationIT.java @@ -144,7 +144,7 @@ private DependnetSSACustomResource reconcileWithLegacyOperator(Operator legacyOp private Operator createOperator(KubernetesClient client, boolean legacyDependentHandling, String fieldManager) { Operator operator = new Operator(client, - o -> o.withSSABasedCreateUpdateForDependentResource(!legacyDependentHandling) + o -> o.withSSABasedCreateUpdateForDependentResources(!legacyDependentHandling) .withSSABasedDefaultMatchingForDependentResources(!legacyDependentHandling) .withCloseClientOnStop(false)); operator.register(new DependentSSAReconciler(), o -> { From 49c03deb56b3f161e56b64bb17470e57e5b0d060 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Tue, 13 Jun 2023 19:33:36 +0200 Subject: [PATCH 16/25] docs: improve wording --- .../operator/api/config/ControllerConfiguration.java | 8 +++++--- .../api/reconciler/ControllerConfiguration.java | 12 +++++++----- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java index 362c4d4546..b9dcfe7828 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java @@ -130,9 +130,11 @@ default Set getEffectiveNamespaces() { } /** - * fieldManager name used for the dependent resource to do create/update (in practice SSA Patch). - * In future this might be used also for other purposes like to make status updates of the primary - * resource with SSA. If not set, sanitized controller name is used as field manager. + * Retrieves the name used to assign as field manager for + * Server-Side + * Apply (SSA) operations. If unset, the sanitized controller name will be used. + * + * @return the name used as field manager for SSA operations */ default String fieldManager() { return getName(); diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ControllerConfiguration.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ControllerConfiguration.java index be680fc0d6..dcd5484b0a 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ControllerConfiguration.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ControllerConfiguration.java @@ -49,7 +49,7 @@ * Specified which namespaces this Controller monitors for custom resources events. If no * namespace is specified then the controller will monitor all namespaces by default. * - * @return the list of namespaces this controller monitors + * @return the array of namespaces this controller monitors */ String[] namespaces() default Constants.WATCH_ALL_NAMESPACES; @@ -110,7 +110,7 @@ MaxReconciliationInterval maxReconciliationInterval() default @MaxReconciliation * Optional list of {@link Dependent} configurations which associate a resource type to a * {@link io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource} implementation * - * @return the list of {@link Dependent} configurations + * @return the array of {@link Dependent} configurations */ Dependent[] dependents() default {}; @@ -133,9 +133,11 @@ MaxReconciliationInterval maxReconciliationInterval() default @MaxReconciliation Class itemStore() default ItemStore.class; /** - * fieldManager name used for the dependent resource to do create/update (in practice SSA Patch). - * In future this might be used also for other purposes like to make status updates of the primary - * resource with SSA. If not set, sanitized controller name is used as field manager. + * Retrieves the name used to assign as field manager for + * Server-Side + * Apply (SSA) operations. If unset, the sanitized controller name will be used. + * + * @return the name used as field manager for SSA operations */ String fieldManager() default CONTROLLER_NAME_AS_FIELD_MANAGER; } From 33521278ddea3e6fe74a79e6162e3fb7169b918c Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Tue, 13 Jun 2023 19:33:57 +0200 Subject: [PATCH 17/25] refactor: change default value to something more explicit --- .../operator/api/config/ControllerConfiguration.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java index b9dcfe7828..13ddd995ad 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java @@ -25,7 +25,7 @@ public interface ControllerConfiguration

extends Resource /** * Will use the controller name as fieldManager if set. */ - String CONTROLLER_NAME_AS_FIELD_MANAGER = "default"; + String CONTROLLER_NAME_AS_FIELD_MANAGER = "use_controller_name"; default String getName() { return ensureValidName(null, getAssociatedReconcilerClassName()); From 4fe7e948f31a82d1d981e7505faca184abc53e52 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Tue, 13 Jun 2023 19:34:24 +0200 Subject: [PATCH 18/25] chore: delete unneeded field --- .../dependent/kubernetes/KubernetesDependentResource.java | 6 ------ 1 file changed, 6 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java index ba14509de5..c42ec06841 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java @@ -11,8 +11,6 @@ import io.fabric8.kubernetes.api.model.Namespaced; import io.fabric8.kubernetes.client.KubernetesClient; 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.javaoperatorsdk.operator.OperatorException; import io.javaoperatorsdk.operator.api.config.dependent.Configured; import io.javaoperatorsdk.operator.api.config.informer.InformerConfiguration; @@ -40,10 +38,6 @@ public abstract class KubernetesDependentResource> { private static final Logger log = LoggerFactory.getLogger(KubernetesDependentResource.class); - private static final PatchContext SSA_PATCH_CONTEXT = new PatchContext.Builder() - .withPatchType(PatchType.SERVER_SIDE_APPLY) - .withForce(true) - .build(); protected KubernetesClient client; private final ResourceUpdatePreProcessor processor; From 7be1f5eb3694c9982ac18aede2d1a50ecad995fc Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Tue, 13 Jun 2023 21:15:35 +0200 Subject: [PATCH 19/25] refactor: minor improvements --- ...BasedGenericKubernetesResourceMatcher.java | 96 +++++++++++-------- 1 file changed, 56 insertions(+), 40 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java index 8b40f7162e..53e25233a4 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java @@ -1,6 +1,11 @@ package io.javaoperatorsdk.operator.processing.dependent.kubernetes; -import java.util.*; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.Set; import java.util.stream.Collectors; import org.slf4j.Logger; @@ -17,11 +22,13 @@ /** * Matches the actual state on the server vs the desired state. Based on the managedFields of SSA. * - * The ide of algorithm is basically trivial, we convert resources to Map/List composition. The - * actual resource (from the server) is pruned, all the fields which are not mentioed in - * managedFields of the target manager is removed. Some irrelevant fields are also removed from - * desired. And the two resulted Maps are compared for equality. The implementation is a bit nasty - * since have to deal with some specific cases of managedFields format. + *

+ * The basis of algorithm is to extract the fields managed we convert resources to Map/List + * composition. The actual resource (from the server) is pruned, all the fields which are not + * mentioed in managedFields of the target manager is removed. Some irrelevant fields are also + * removed from desired. And the two resulted Maps are compared for equality. The implementation is + * a bit nasty since have to deal with some specific cases of managedFields format. + *

* * @param matched resource type */ @@ -47,7 +54,11 @@ public static SSABasedGenericKubernetesResourceMatcher SSABasedGenericKubernetesResourceMatcher context) { try { var optionalManagedFieldsEntry = - checkIfFieldManagerExists(actual, - context.getControllerConfiguration().fieldManager()); - // the results of this is that it will add the field manager; it's important from migration - // aspect + checkIfFieldManagerExists(actual, context.getControllerConfiguration().fieldManager()); + // If no field is managed by our controller, that means the controller hasn't touched the + // resource yet and the resource probably doesn't match the desired state. Not matching here + // means that the resource will need to be updated and since this will be done using SSA, the + // fields our controller cares about will become managed by it if (optionalManagedFieldsEntry.isEmpty()) { return false; } @@ -74,8 +86,8 @@ public boolean matches(R actual, R desired, Context context) { log.trace("Original actual: \n {} \n original desired: \n {} ", actual, desiredMap); - var prunedActual = new HashMap(); - pruneActualAccordingManagedFields(prunedActual, actualMap, + var prunedActual = new HashMap(actualMap.size()); + keepOnlyManagedFields(prunedActual, actualMap, managedFieldsEntry.getFieldsV1().getAdditionalProperties(), objectMapper); removeIrrelevantValues(desiredMap); @@ -88,18 +100,20 @@ public boolean matches(R actual, R desired, Context context) { } } - private void removeIrrelevantValues(HashMap desiredMap) { + @SuppressWarnings("unchecked") + private static void removeIrrelevantValues(Map desiredMap) { var metadata = (Map) desiredMap.get(METADATA_KEY); - metadata.remove("name"); - metadata.remove("namespace"); + metadata.remove(NAME_KEY); + metadata.remove(NAMESPACE_KEY); if (metadata.isEmpty()) { desiredMap.remove(METADATA_KEY); } - desiredMap.remove("kind"); - desiredMap.remove("apiVersion"); + desiredMap.remove(KIND_KEY); + desiredMap.remove(API_VERSION_KEY); } - private void pruneActualAccordingManagedFields(Map result, + @SuppressWarnings("unchecked") + private static void keepOnlyManagedFields(Map result, Map actualMap, Map managedFields, ObjectMapper objectMapper) throws JsonProcessingException { @@ -111,9 +125,9 @@ private void pruneActualAccordingManagedFields(Map result, String key = entry.getKey(); if (key.startsWith(F_PREFIX)) { String keyInActual = keyWithoutPrefix(key); - var managedFieldValue = entry.getValue(); + var managedFieldValue = (Map) entry.getValue(); if (isNestedValue(managedFieldValue)) { - var managedEntrySet = ((Map) managedFieldValue).entrySet(); + var managedEntrySet = managedFieldValue.entrySet(); // two special cases "k:" and "v:" prefixes if (isListKeyEntrySet(managedEntrySet)) { @@ -121,8 +135,8 @@ private void pruneActualAccordingManagedFields(Map result, } else if (isSetValueField(managedEntrySet)) { handleSetValues(result, actualMap, objectMapper, keyInActual, managedEntrySet); } else { - // basically if we should travers further - fillResultsAndTraversFurther(result, actualMap, managedFields, objectMapper, key, + // basically if we should traverse further + fillResultsAndTraverseFurther(result, actualMap, managedFields, objectMapper, key, keyInActual, managedFieldValue); } } else { @@ -139,7 +153,7 @@ private void pruneActualAccordingManagedFields(Map result, } } - private void fillResultsAndTraversFurther(Map result, + private static void fillResultsAndTraverseFurther(Map result, Map actualMap, Map managedFields, ObjectMapper objectMapper, String key, String keyInActual, Object managedFieldValue) throws JsonProcessingException { var emptyMapValue = new HashMap(); @@ -148,16 +162,17 @@ private void fillResultsAndTraversFurther(Map result, log.debug("key: {} actual map value: {} managedFieldValue: {}", keyInActual, actualMapValue, managedFieldValue); - pruneActualAccordingManagedFields(emptyMapValue, (Map) actualMapValue, + keepOnlyManagedFields(emptyMapValue, (Map) actualMapValue, (Map) managedFields.get(key), objectMapper); } - private static boolean isNestedValue(Object managedFieldValue) { - return managedFieldValue instanceof Map && (!((Map) managedFieldValue).isEmpty()); + private static boolean isNestedValue(Map managedFieldValue) { + return !managedFieldValue.isEmpty(); } // list entries referenced by key, or when "k:" prefix is used - private void handleListKeyEntrySet(Map result, Map actualMap, + private static void handleListKeyEntrySet(Map result, + Map actualMap, ObjectMapper objectMapper, String keyInActual, Set> managedEntrySet) { var valueList = new ArrayList<>(); @@ -185,7 +200,7 @@ private void handleListKeyEntrySet(Map result, Map(); valueList.add(emptyResMapValue); try { - pruneActualAccordingManagedFields(emptyResMapValue, e.getValue(), + keepOnlyManagedFields(emptyResMapValue, e.getValue(), mangedEntryByIndex.get(e.getKey()), objectMapper); } catch (JsonProcessingException ex) { throw new IllegalStateException(ex); @@ -230,25 +245,26 @@ public static Object parseKeyValue(String stringValue, Class targetClass, } } - private boolean isSetValueField(Set> managedEntrySet) { - var iterator = managedEntrySet.iterator(); - var managedFieldEntry = iterator.next(); - if (managedFieldEntry.getKey().equals(DOT_KEY)) { - managedFieldEntry = iterator.next(); - } - return managedFieldEntry.getKey().startsWith(V_PREFIX); + private static boolean isSetValueField(Set> managedEntrySet) { + return isKeyPrefixedSkippingDotKey(managedEntrySet, V_PREFIX); + } + + private static boolean isListKeyEntrySet(Set> managedEntrySet) { + return isKeyPrefixedSkippingDotKey(managedEntrySet, K_PREFIX); } - private boolean isListKeyEntrySet(Set> managedEntrySet) { + private static boolean isKeyPrefixedSkippingDotKey(Set> managedEntrySet, + String prefix) { var iterator = managedEntrySet.iterator(); var managedFieldEntry = iterator.next(); if (managedFieldEntry.getKey().equals(DOT_KEY)) { managedFieldEntry = iterator.next(); } - return managedFieldEntry.getKey().startsWith(K_PREFIX); + return managedFieldEntry.getKey().startsWith(prefix); } - private java.util.Map.Entry> selectListEntryBasedOnKey(String key, + private static java.util.Map.Entry> selectListEntryBasedOnKey( + String key, List> values, ObjectMapper objectMapper) { try { @@ -308,7 +324,7 @@ private Optional checkIfFieldManagerExists(R actual, String } // this should not happen in theory if (targetManagedFields.size() > 1) { - log.debug("More field managers exists with name: {} in resource: {} with name: {} ", + log.debug("More than one field manager exists with name: {} in resource: {} with name: {} ", fieldManager, actual, actual.getMetadata().getName()); } From 157d83b2030bb48e235ac2208b7e0fec088a55f3 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Tue, 13 Jun 2023 21:39:19 +0200 Subject: [PATCH 20/25] refactor: use SortedMap instead of sorting stream --- ...BasedGenericKubernetesResourceMatcher.java | 27 +++++++++---------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java index 53e25233a4..30016163a8 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java @@ -6,6 +6,8 @@ import java.util.Map; import java.util.Optional; import java.util.Set; +import java.util.SortedMap; +import java.util.TreeMap; import java.util.stream.Collectors; import org.slf4j.Logger; @@ -179,7 +181,7 @@ private static void handleListKeyEntrySet(Map result, result.put(keyInActual, valueList); var actualValueList = (List>) actualMap.get(keyInActual); - Map> targetValuesByIndex = new HashMap<>(); + SortedMap> targetValuesByIndex = new TreeMap<>(); Map> mangedEntryByIndex = new HashMap<>(); for (Map.Entry listEntry : managedEntrySet) { @@ -192,20 +194,15 @@ private static void handleListKeyEntrySet(Map result, mangedEntryByIndex.put(actualListEntry.getKey(), (Map) listEntry.getValue()); } - targetValuesByIndex.entrySet() - .stream() - // list is sorted according to the value in actual - .sorted(Map.Entry.comparingByKey()) - .forEach(e -> { - var emptyResMapValue = new HashMap(); - valueList.add(emptyResMapValue); - try { - keepOnlyManagedFields(emptyResMapValue, e.getValue(), - mangedEntryByIndex.get(e.getKey()), objectMapper); - } catch (JsonProcessingException ex) { - throw new IllegalStateException(ex); - } - }); + targetValuesByIndex.forEach((key, value) -> { + var emptyResMapValue = new HashMap(); + valueList.add(emptyResMapValue); + try { + keepOnlyManagedFields(emptyResMapValue, value, mangedEntryByIndex.get(key), objectMapper); + } catch (JsonProcessingException ex) { + throw new IllegalStateException(ex); + } + }); } // set values, the "v:" prefix From b68b53c8369381ca5178877efcc914a3f0656eb6 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Tue, 13 Jun 2023 23:43:55 +0200 Subject: [PATCH 21/25] refactor: typo --- .../SSABasedGenericKubernetesResourceMatcher.java | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java index 30016163a8..2fc300b010 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java @@ -155,6 +155,7 @@ private static void keepOnlyManagedFields(Map result, } } + @SuppressWarnings("unchecked") private static void fillResultsAndTraverseFurther(Map result, Map actualMap, Map managedFields, ObjectMapper objectMapper, String key, String keyInActual, Object managedFieldValue) throws JsonProcessingException { @@ -173,6 +174,7 @@ private static boolean isNestedValue(Map managedFieldValue) { } // list entries referenced by key, or when "k:" prefix is used + @SuppressWarnings("unchecked") private static void handleListKeyEntrySet(Map result, Map actualMap, ObjectMapper objectMapper, String keyInActual, @@ -182,7 +184,7 @@ private static void handleListKeyEntrySet(Map result, var actualValueList = (List>) actualMap.get(keyInActual); SortedMap> targetValuesByIndex = new TreeMap<>(); - Map> mangedEntryByIndex = new HashMap<>(); + Map> managedEntryByIndex = new HashMap<>(); for (Map.Entry listEntry : managedEntrySet) { if (DOT_KEY.equals(listEntry.getKey())) { @@ -191,14 +193,14 @@ private static void handleListKeyEntrySet(Map result, var actualListEntry = selectListEntryBasedOnKey(keyWithoutPrefix(listEntry.getKey()), actualValueList, objectMapper); targetValuesByIndex.put(actualListEntry.getKey(), actualListEntry.getValue()); - mangedEntryByIndex.put(actualListEntry.getKey(), (Map) listEntry.getValue()); + managedEntryByIndex.put(actualListEntry.getKey(), (Map) listEntry.getValue()); } targetValuesByIndex.forEach((key, value) -> { var emptyResMapValue = new HashMap(); valueList.add(emptyResMapValue); try { - keepOnlyManagedFields(emptyResMapValue, value, mangedEntryByIndex.get(key), objectMapper); + keepOnlyManagedFields(emptyResMapValue, value, managedEntryByIndex.get(key), objectMapper); } catch (JsonProcessingException ex) { throw new IllegalStateException(ex); } @@ -206,6 +208,7 @@ private static void handleListKeyEntrySet(Map result, } // set values, the "v:" prefix + @SuppressWarnings("rawtypes") private static void handleSetValues(Map result, Map actualMap, ObjectMapper objectMapper, String keyInActual, Set> managedEntrySet) { From 9109a4ad3319f4100acc88c773def49f02a98fbe Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 14 Jun 2023 11:12:32 +0200 Subject: [PATCH 22/25] added docs --- ...BasedGenericKubernetesResourceMatcher.java | 21 +++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java index 2fc300b010..c27a9b61d4 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java @@ -173,7 +173,16 @@ private static boolean isNestedValue(Map managedFieldValue) { return !managedFieldValue.isEmpty(); } - // list entries referenced by key, or when "k:" prefix is used + // List entries referenced by key, or when "k:" prefix is used. It works in a way + // that in it selects the target element based on the field(s) in "k:" for example + // when there is a list of element of owner references, the uid can server as a key + // for a list element: "k:{"uid":"1ef74cb4-dbbd-45ef-9caf-aa76186594ea"}". + // It selects the element and recursively processes it. + // Note that in these lists the order matter and seems that if there are more keys ("k:"), + // the ordering of those in the managed fields are not the same as the value order. So + // this also explicitly orders the result based on the value order in the resource not + // the key order in managed field. + @SuppressWarnings("unchecked") private static void handleListKeyEntrySet(Map result, Map actualMap, @@ -207,7 +216,11 @@ private static void handleListKeyEntrySet(Map result, }); } - // set values, the "v:" prefix + // Set values, the "v:" prefix. Form in managed fields: "f:some-set":{"v:1":{}},"v:2":{},"v:3":{}} + // Note that this should be just used in very rate cases, actually was not able to produce a + // sample. + // And developers of this in Kubernetes was not able to provide one either. Basically this method + // just adds the values from "v:" to the result. @SuppressWarnings("rawtypes") private static void handleSetValues(Map result, Map actualMap, ObjectMapper objectMapper, String keyInActual, @@ -253,6 +266,10 @@ private static boolean isListKeyEntrySet(Set> managedE return isKeyPrefixedSkippingDotKey(managedEntrySet, K_PREFIX); } + // Sometimes (not always) the first subfield of a managed field ("f:") is ".:{}", + // it looks that those are added when there are more subfields of a field referenced. + // See samples of tests. But does not seem to be have additional functionality, so can be + // just skipped. private static boolean isKeyPrefixedSkippingDotKey(Set> managedEntrySet, String prefix) { var iterator = managedEntrySet.iterator(); From 81b406a5d19562c11309bdedaf1e8605cde58b09 Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 14 Jun 2023 11:15:23 +0200 Subject: [PATCH 23/25] throw exception in unexpected case --- .../SSABasedGenericKubernetesResourceMatcher.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java index c27a9b61d4..23f2c60a4a 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java @@ -15,6 +15,7 @@ import io.fabric8.kubernetes.api.model.HasMetadata; import io.fabric8.kubernetes.api.model.ManagedFieldsEntry; +import io.javaoperatorsdk.operator.OperatorException; import io.javaoperatorsdk.operator.api.reconciler.Context; import com.fasterxml.jackson.core.JsonProcessingException; @@ -341,9 +342,9 @@ private Optional checkIfFieldManagerExists(R actual, String } // this should not happen in theory if (targetManagedFields.size() > 1) { - log.debug("More than one field manager exists with name: {} in resource: {} with name: {} ", - fieldManager, - actual, actual.getMetadata().getName()); + throw new OperatorException( + "More than one field manager exists with name: " + fieldManager + "in resource: " + + actual + " with name: " + actual.getMetadata().getName()); } return Optional.of(targetManagedFields.get(0)); } From 5ce5d67e444aa05cc3a53e869c68e8f641099435 Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 14 Jun 2023 11:16:56 +0200 Subject: [PATCH 24/25] format --- .../kubernetes/SSABasedGenericKubernetesResourceMatcher.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java index 23f2c60a4a..ce2bd4482f 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java @@ -219,9 +219,8 @@ private static void handleListKeyEntrySet(Map result, // Set values, the "v:" prefix. Form in managed fields: "f:some-set":{"v:1":{}},"v:2":{},"v:3":{}} // Note that this should be just used in very rate cases, actually was not able to produce a - // sample. - // And developers of this in Kubernetes was not able to provide one either. Basically this method - // just adds the values from "v:" to the result. + // sample. And developers of this in Kubernetes was not able to provide one either. Basically this + // method just adds the values from "v:" to the result. @SuppressWarnings("rawtypes") private static void handleSetValues(Map result, Map actualMap, ObjectMapper objectMapper, String keyInActual, From 1fabc6fc4a6bf16f365a762bc35e49ce75312482 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Wed, 14 Jun 2023 19:08:54 +0200 Subject: [PATCH 25/25] docs: improve wording, convert to javadoc to track the relevant methods --- ...BasedGenericKubernetesResourceMatcher.java | 40 ++++++++++--------- 1 file changed, 22 insertions(+), 18 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java index ce2bd4482f..50158cad18 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java @@ -174,16 +174,16 @@ private static boolean isNestedValue(Map managedFieldValue) { return !managedFieldValue.isEmpty(); } - // List entries referenced by key, or when "k:" prefix is used. It works in a way - // that in it selects the target element based on the field(s) in "k:" for example - // when there is a list of element of owner references, the uid can server as a key - // for a list element: "k:{"uid":"1ef74cb4-dbbd-45ef-9caf-aa76186594ea"}". - // It selects the element and recursively processes it. - // Note that in these lists the order matter and seems that if there are more keys ("k:"), - // the ordering of those in the managed fields are not the same as the value order. So - // this also explicitly orders the result based on the value order in the resource not - // the key order in managed field. - + /** + * List entries referenced by key, or when "k:" prefix is used. It works in a way that it selects + * the target element based on the field(s) in "k:" for example when there is a list of element of + * owner references, the uid can serve as a key for a list element: + * "k:{"uid":"1ef74cb4-dbbd-45ef-9caf-aa76186594ea"}". It selects the element and recursively + * processes it. Note that in these lists the order matters and seems that if there are more keys + * ("k:"), the ordering of those in the managed fields are not the same as the value order. So + * this also explicitly orders the result based on the value order in the resource not the key + * order in managed field. + */ @SuppressWarnings("unchecked") private static void handleListKeyEntrySet(Map result, Map actualMap, @@ -217,10 +217,13 @@ private static void handleListKeyEntrySet(Map result, }); } - // Set values, the "v:" prefix. Form in managed fields: "f:some-set":{"v:1":{}},"v:2":{},"v:3":{}} - // Note that this should be just used in very rate cases, actually was not able to produce a - // sample. And developers of this in Kubernetes was not able to provide one either. Basically this - // method just adds the values from "v:" to the result. + /** + * Set values, the "v:" prefix. Form in managed fields: "f:some-set":{"v:1":{}},"v:2":{},"v:3":{}} + * Note that this should be just used in very rare cases, actually was not able to produce a + * sample. Kubernetes developers who worked on this feature were not able to provide one either + * when prompted. Basically this method just adds the values from {@code "v:"} to the + * result. + */ @SuppressWarnings("rawtypes") private static void handleSetValues(Map result, Map actualMap, ObjectMapper objectMapper, String keyInActual, @@ -266,10 +269,11 @@ private static boolean isListKeyEntrySet(Set> managedE return isKeyPrefixedSkippingDotKey(managedEntrySet, K_PREFIX); } - // Sometimes (not always) the first subfield of a managed field ("f:") is ".:{}", - // it looks that those are added when there are more subfields of a field referenced. - // See samples of tests. But does not seem to be have additional functionality, so can be - // just skipped. + /** + * Sometimes (not always) the first subfield of a managed field ("f:") is ".:{}", it looks that + * those are added when there are more subfields of a referenced field. See test samples. Does not + * seem to provide additional functionality, so can be just skipped for now. + */ private static boolean isKeyPrefixedSkippingDotKey(Set> managedEntrySet, String prefix) { var iterator = managedEntrySet.iterator();