From 4456400508a041a0bbe90085d3e7298c08aba87b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Wed, 17 Aug 2022 13:22:51 +0200 Subject: [PATCH 1/3] feat: use fabric8 client json mapper by default (#1382) Also remove constants to avoid mismatched configurations Co-authored-by: Chris Laprun --- .../config/AbstractConfigurationService.java | 14 ++++++++ .../api/config/BaseConfigurationService.java | 4 +++ .../api/config/ConfigurationService.java | 36 +++++++++---------- .../config/ConfigurationServiceOverrider.java | 7 +--- .../StandaloneDependentResourceIT.java | 4 +-- 5 files changed, 39 insertions(+), 26 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/AbstractConfigurationService.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/AbstractConfigurationService.java index 2c9e01551e..54451aa0b3 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/AbstractConfigurationService.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/AbstractConfigurationService.java @@ -13,9 +13,16 @@ public class AbstractConfigurationService implements ConfigurationService { private final Map configurations = new ConcurrentHashMap<>(); private final Version version; + private final Cloner cloner; public AbstractConfigurationService(Version version) { this.version = version; + this.cloner = ConfigurationService.super.getResourceCloner(); + } + + public AbstractConfigurationService(Version version, Cloner cloner) { + this.version = version; + this.cloner = cloner; } protected void register(ControllerConfiguration config) { @@ -77,10 +84,12 @@ protected String keyFor(Reconciler reconciler) { return ReconcilerUtils.getNameFor(reconciler); } + @SuppressWarnings("unused") protected ControllerConfiguration getFor(String reconcilerName) { return configurations.get(reconcilerName); } + @SuppressWarnings("unused") protected Stream controllerConfigurations() { return configurations.values().stream(); } @@ -94,4 +103,9 @@ public Set getKnownReconcilerNames() { public Version getVersion() { return version; } + + @Override + public Cloner getResourceCloner() { + return cloner; + } } 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 bfc4c6c588..19899a818f 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 @@ -15,6 +15,10 @@ public BaseConfigurationService(Version version) { super(version); } + public BaseConfigurationService(Version version, Cloner cloner) { + super(version, cloner); + } + public BaseConfigurationService() { this(Utils.loadFromProperties()); } 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 27f8d194d1..a425bda5bd 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 @@ -7,6 +7,7 @@ import io.fabric8.kubernetes.api.model.HasMetadata; import io.fabric8.kubernetes.client.Config; import io.fabric8.kubernetes.client.CustomResource; +import io.fabric8.kubernetes.client.utils.Serialization; import io.javaoperatorsdk.operator.api.monitoring.Metrics; import io.javaoperatorsdk.operator.api.reconciler.Reconciler; import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResourceFactory; @@ -17,20 +18,6 @@ /** An interface from which to retrieve configuration information. */ public interface ConfigurationService { - ObjectMapper OBJECT_MAPPER = new ObjectMapper(); - - Cloner DEFAULT_CLONER = new Cloner() { - @SuppressWarnings("unchecked") - @Override - public HasMetadata clone(HasMetadata object) { - try { - return OBJECT_MAPPER.readValue(OBJECT_MAPPER.writeValueAsString(object), object.getClass()); - } catch (JsonProcessingException e) { - throw new IllegalStateException(e); - } - } - }; - /** * Retrieves the configuration associated with the specified reconciler * @@ -93,12 +80,25 @@ default int concurrentReconciliationThreads() { } /** - * Used to clone custom resources. + * Used to clone custom resources. It is strongly suggested that implementors override this method + * since the default implementation creates a new {@link Cloner} instance each time this method is + * called. * - * @return the ObjectMapper to use + * @return the configured {@link Cloner} */ default Cloner getResourceCloner() { - return DEFAULT_CLONER; + return new Cloner() { + @SuppressWarnings("unchecked") + @Override + public HasMetadata clone(HasMetadata object) { + try { + final var mapper = getObjectMapper(); + return mapper.readValue(mapper.writeValueAsString(object), object.getClass()); + } catch (JsonProcessingException e) { + throw new IllegalStateException(e); + } + } + }; } int DEFAULT_TERMINATION_TIMEOUT_SECONDS = 10; @@ -126,7 +126,7 @@ default boolean closeClientOnStop() { } default ObjectMapper getObjectMapper() { - return OBJECT_MAPPER; + return Serialization.jsonMapper(); } default DependentResourceFactory dependentResourceFactory() { 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 44db8e9999..f8833231c2 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 @@ -81,7 +81,7 @@ public ConfigurationServiceOverrider withObjectMapper(ObjectMapper objectMapper) } public ConfigurationService build() { - return new BaseConfigurationService(original.getVersion()) { + return new BaseConfigurationService(original.getVersion(), cloner) { @Override public Set getKnownReconcilerNames() { return original.getKnownReconcilerNames(); @@ -102,11 +102,6 @@ public int concurrentReconciliationThreads() { return threadNumber; } - @Override - public Cloner getResourceCloner() { - return cloner; - } - @Override public int getTerminationTimeoutSeconds() { return timeoutSeconds; diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/StandaloneDependentResourceIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/StandaloneDependentResourceIT.java index f1e38ce6eb..f4e6624c4a 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/StandaloneDependentResourceIT.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/StandaloneDependentResourceIT.java @@ -7,7 +7,7 @@ import io.fabric8.kubernetes.api.model.ObjectMeta; import io.fabric8.kubernetes.api.model.apps.Deployment; -import io.javaoperatorsdk.operator.api.config.ConfigurationService; +import io.javaoperatorsdk.operator.api.config.ConfigurationServiceProvider; import io.javaoperatorsdk.operator.junit.LocallyRunOperatorExtension; import io.javaoperatorsdk.operator.sample.standalonedependent.StandaloneDependentTestCustomResource; import io.javaoperatorsdk.operator.sample.standalonedependent.StandaloneDependentTestCustomResourceSpec; @@ -52,7 +52,7 @@ void executeUpdateForTestingCacheUpdateForGetResource() { awaitForDeploymentReadyReplicas(1); - var clonedCr = ConfigurationService.DEFAULT_CLONER.clone(createdCR); + var clonedCr = ConfigurationServiceProvider.instance().getResourceCloner().clone(createdCR); clonedCr.getSpec().setReplicaCount(2); operator.replace(StandaloneDependentTestCustomResource.class, clonedCr); From 6861635c8f9f35a2a37d4dc90ebd5344cf151544 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Thu, 18 Aug 2022 09:16:32 +0200 Subject: [PATCH 2/3] refactor: make it easier to initialize mapper & cloner in subclasses --- .../config/AbstractConfigurationService.java | 33 ++++++++++++++++--- .../api/config/BaseConfigurationService.java | 6 ++-- .../config/ConfigurationServiceOverrider.java | 7 +--- 3 files changed, 34 insertions(+), 12 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/AbstractConfigurationService.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/AbstractConfigurationService.java index 54451aa0b3..f2a7c94bec 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/AbstractConfigurationService.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/AbstractConfigurationService.java @@ -9,20 +9,40 @@ import io.javaoperatorsdk.operator.ReconcilerUtils; import io.javaoperatorsdk.operator.api.reconciler.Reconciler; +import com.fasterxml.jackson.databind.ObjectMapper; + @SuppressWarnings("rawtypes") public class AbstractConfigurationService implements ConfigurationService { private final Map configurations = new ConcurrentHashMap<>(); private final Version version; - private final Cloner cloner; + private Cloner cloner; + private ObjectMapper mapper; public AbstractConfigurationService(Version version) { - this.version = version; - this.cloner = ConfigurationService.super.getResourceCloner(); + this(version, null, null); } public AbstractConfigurationService(Version version, Cloner cloner) { + this(version, cloner, null); + } + + public AbstractConfigurationService(Version version, Cloner cloner, ObjectMapper mapper) { this.version = version; - this.cloner = cloner; + init(cloner, mapper); + } + + /** + * Subclasses can call this method to more easily initialize the {@link Cloner} and + * {@link ObjectMapper} associated with this ConfigurationService implementation. This is useful + * in situations where the cloner depends on a mapper that might require additional configuration + * steps before it's ready to be used. + * + * @param cloner the {@link Cloner} instance to be used + * @param mapper the {@link ObjectMapper} instance to be used + */ + protected void init(Cloner cloner, ObjectMapper mapper) { + this.cloner = cloner != null ? cloner : ConfigurationService.super.getResourceCloner(); + this.mapper = mapper != null ? mapper : ConfigurationService.super.getObjectMapper(); } protected void register(ControllerConfiguration config) { @@ -108,4 +128,9 @@ public Version getVersion() { public Cloner getResourceCloner() { return cloner; } + + @Override + public ObjectMapper getObjectMapper() { + return mapper; + } } 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 19899a818f..7dfc52e50a 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 @@ -6,6 +6,8 @@ import io.fabric8.kubernetes.api.model.HasMetadata; import io.javaoperatorsdk.operator.api.reconciler.Reconciler; +import com.fasterxml.jackson.databind.ObjectMapper; + public class BaseConfigurationService extends AbstractConfigurationService { private static final String LOGGER_NAME = "Default ConfigurationService implementation"; @@ -15,8 +17,8 @@ public BaseConfigurationService(Version version) { super(version); } - public BaseConfigurationService(Version version, Cloner cloner) { - super(version, cloner); + public BaseConfigurationService(Version version, Cloner cloner, ObjectMapper mapper) { + super(version, cloner, mapper); } public BaseConfigurationService() { 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 f8833231c2..6233b59fd5 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 @@ -81,7 +81,7 @@ public ConfigurationServiceOverrider withObjectMapper(ObjectMapper objectMapper) } public ConfigurationService build() { - return new BaseConfigurationService(original.getVersion(), cloner) { + return new BaseConfigurationService(original.getVersion(), cloner, objectMapper) { @Override public Set getKnownReconcilerNames() { return original.getKnownReconcilerNames(); @@ -125,11 +125,6 @@ public ExecutorService getExecutorService() { return super.getExecutorService(); } } - - @Override - public ObjectMapper getObjectMapper() { - return objectMapper; - } }; } From c18cf49c11428823b05f2aafbd6cd2a1035a96c7 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Thu, 18 Aug 2022 09:39:22 +0200 Subject: [PATCH 3/3] fix: format --- .../operator/api/config/AbstractConfigurationService.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/AbstractConfigurationService.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/AbstractConfigurationService.java index f2a7c94bec..2895eb4c93 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/AbstractConfigurationService.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/AbstractConfigurationService.java @@ -36,7 +36,7 @@ public AbstractConfigurationService(Version version, Cloner cloner, ObjectMapper * {@link ObjectMapper} associated with this ConfigurationService implementation. This is useful * in situations where the cloner depends on a mapper that might require additional configuration * steps before it's ready to be used. - * + * * @param cloner the {@link Cloner} instance to be used * @param mapper the {@link ObjectMapper} instance to be used */