From d10ca901ce334a92d378b2786440e33b16e53c33 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Thu, 1 Sep 2022 17:18:51 +0200 Subject: [PATCH 1/2] refactor: improve configuration utilities --- .../AnnotationControllerConfiguration.java | 99 ++++++----------- .../operator/api/config/Utils.java | 105 ++++++++++++++++-- .../operator/api/config/UtilsTest.java | 11 ++ 3 files changed, 137 insertions(+), 78 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/AnnotationControllerConfiguration.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/AnnotationControllerConfiguration.java index b5e3fffcf0..a3f1effaf4 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/AnnotationControllerConfiguration.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/AnnotationControllerConfiguration.java @@ -1,8 +1,6 @@ package io.javaoperatorsdk.operator.api.config; import java.lang.annotation.Annotation; -import java.lang.reflect.Constructor; -import java.lang.reflect.InvocationTargetException; import java.time.Duration; import java.util.Arrays; import java.util.Collections; @@ -41,10 +39,6 @@ public class AnnotationControllerConfiguration

implements io.javaoperatorsdk.operator.api.config.ControllerConfiguration

{ - private static final String CONTROLLER_CONFIG_ANNOTATION = - ControllerConfiguration.class.getSimpleName(); - private static final String KUBE_DEPENDENT_NAME = KubernetesDependent.class.getSimpleName(); - protected final Reconciler

reconciler; private final ControllerConfiguration annotation; private List specs; @@ -152,71 +146,54 @@ public Optional maxReconciliationInterval() { @Override public RateLimiter getRateLimiter() { final Class rateLimiterClass = annotation.rateLimiter(); - return instantiateAndConfigureIfNeeded(rateLimiterClass, RateLimiter.class, - CONTROLLER_CONFIG_ANNOTATION); + return Utils.instantiateAndConfigureIfNeeded(rateLimiterClass, RateLimiter.class, + Utils.contextFor(this, null, null), this::configureFromAnnotatedReconciler); } @Override public Retry getRetry() { final Class retryClass = annotation.retry(); - return instantiateAndConfigureIfNeeded(retryClass, Retry.class, CONTROLLER_CONFIG_ANNOTATION); + return Utils.instantiateAndConfigureIfNeeded(retryClass, Retry.class, + Utils.contextFor(this, null, null), this::configureFromAnnotatedReconciler); } + @SuppressWarnings("unchecked") - protected T instantiateAndConfigureIfNeeded(Class targetClass, - Class expectedType, String context) { - try { - final Constructor constructor = targetClass.getDeclaredConstructor(); - constructor.setAccessible(true); - final var instance = constructor.newInstance(); - if (instance instanceof AnnotationConfigurable) { - AnnotationConfigurable configurable = (AnnotationConfigurable) instance; - final Class configurationClass = - (Class) Utils.getFirstTypeArgumentFromSuperClassOrInterface( - targetClass, AnnotationConfigurable.class); - final var configAnnotation = reconciler.getClass().getAnnotation(configurationClass); - if (configAnnotation != null) { - configurable.initFrom(configAnnotation); - } + private void configureFromAnnotatedReconciler(T instance) { + if (instance instanceof AnnotationConfigurable) { + AnnotationConfigurable configurable = (AnnotationConfigurable) instance; + final Class configurationClass = + (Class) Utils.getFirstTypeArgumentFromSuperClassOrInterface( + instance.getClass(), AnnotationConfigurable.class); + final var configAnnotation = reconciler.getClass().getAnnotation(configurationClass); + if (configAnnotation != null) { + configurable.initFrom(configAnnotation); } - return instance; - } catch (InstantiationException | IllegalAccessException | InvocationTargetException - | NoSuchMethodException e) { - throw new OperatorException("Couldn't instantiate " + expectedType.getSimpleName() + " '" - + targetClass.getName() + "' for '" + getName() - + "' reconciler in " + context - + ". You need to provide an accessible no-arg constructor.", e); } } @Override @SuppressWarnings("unchecked") public Optional> onAddFilter() { - return (Optional>) createFilter(annotation.onAddFilter(), OnAddFilter.class, - CONTROLLER_CONFIG_ANNOTATION); - } - - protected Optional createFilter(Class filter, Class defaultValue, - String origin) { - if (defaultValue.equals(filter)) { - return Optional.empty(); - } else { - return Optional.of(instantiateAndConfigureIfNeeded(filter, defaultValue, origin)); - } + return Optional.ofNullable( + Utils.instantiate(annotation.onAddFilter(), OnAddFilter.class, + Utils.contextFor(this, null, null))); } @SuppressWarnings("unchecked") @Override public Optional> onUpdateFilter() { - return (Optional>) createFilter(annotation.onUpdateFilter(), - OnUpdateFilter.class, CONTROLLER_CONFIG_ANNOTATION); + return Optional.ofNullable( + Utils.instantiate(annotation.onUpdateFilter(), OnUpdateFilter.class, + Utils.contextFor(this, null, null))); } @SuppressWarnings("unchecked") @Override public Optional> genericFilter() { - return (Optional>) createFilter(annotation.genericFilter(), - GenericFilter.class, CONTROLLER_CONFIG_ANNOTATION); + return Optional.ofNullable( + Utils.instantiate(annotation.genericFilter(), GenericFilter.class, + Utils.contextFor(this, null, null))); } @SuppressWarnings({"rawtypes", "unchecked"}) @@ -244,12 +221,12 @@ public List getDependentResources() { throw new IllegalArgumentException( "A DependentResource named '" + name + "' already exists: " + spec); } - final var context = "DependentResource of type '" + dependentType.getName() + "'"; + final var context = Utils.contextFor(this, dependentType, null); spec = new DependentResourceSpec(dependentType, config, name, Set.of(dependent.dependsOn()), - instantiateConditionIfNotDefault(dependent.readyPostcondition(), context), - instantiateConditionIfNotDefault(dependent.reconcilePrecondition(), context), - instantiateConditionIfNotDefault(dependent.deletePostcondition(), context)); + Utils.instantiate(dependent.readyPostcondition(), Condition.class, context), + Utils.instantiate(dependent.reconcilePrecondition(), Condition.class, context), + Utils.instantiate(dependent.deletePostcondition(), Condition.class, context)); specsMap.put(name, spec); } @@ -258,14 +235,6 @@ public List getDependentResources() { return specs; } - protected Condition instantiateConditionIfNotDefault(Class condition, - String context) { - if (condition != Condition.class) { - return instantiateAndConfigureIfNeeded(condition, Condition.class, context); - } - return null; - } - private String getName(Dependent dependent, Class dependentType) { var name = dependent.name(); if (name.isBlank()) { @@ -299,18 +268,14 @@ private Object createKubernetesResourceConfig(Class final var context = - KUBE_DEPENDENT_NAME + " annotation on " + dependentType.getName() + " DependentResource"; - onAddFilter = createFilter(kubeDependent.onAddFilter(), OnAddFilter.class, context) - .orElse(null); + Utils.contextFor(this, dependentType, null); + onAddFilter = Utils.instantiate(kubeDependent.onAddFilter(), OnAddFilter.class, context); onUpdateFilter = - createFilter(kubeDependent.onUpdateFilter(), OnUpdateFilter.class, context) - .orElse(null); + Utils.instantiate(kubeDependent.onUpdateFilter(), OnUpdateFilter.class, context); onDeleteFilter = - createFilter(kubeDependent.onDeleteFilter(), OnDeleteFilter.class, context) - .orElse(null); + Utils.instantiate(kubeDependent.onDeleteFilter(), OnDeleteFilter.class, context); genericFilter = - createFilter(kubeDependent.genericFilter(), GenericFilter.class, context) - .orElse(null); + Utils.instantiate(kubeDependent.genericFilter(), GenericFilter.class, context); } config = diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/Utils.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/Utils.java index c9f636101a..efcf9d6e3b 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/Utils.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/Utils.java @@ -1,18 +1,23 @@ package io.javaoperatorsdk.operator.api.config; import java.io.IOException; +import java.lang.annotation.Annotation; +import java.lang.reflect.Constructor; +import java.lang.reflect.InvocationTargetException; import java.lang.reflect.ParameterizedType; import java.lang.reflect.Type; import java.text.SimpleDateFormat; import java.time.Instant; import java.util.Arrays; import java.util.Date; +import java.util.Optional; import java.util.Properties; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import io.javaoperatorsdk.operator.OperatorException; +import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; public class Utils { @@ -106,17 +111,41 @@ public static Class getFirstTypeArgumentFromExtendedClass(Class clazz) { public static Class getFirstTypeArgumentFromInterface(Class clazz, Class expectedImplementedInterface) { - return Arrays.stream(clazz.getGenericInterfaces()) - .filter(type -> type.getTypeName().startsWith(expectedImplementedInterface.getName()) - && type instanceof ParameterizedType) - .map(ParameterizedType.class::cast) - .findFirst() - .map(t -> (Class) t.getActualTypeArguments()[0]) - .orElseThrow(() -> new RuntimeException( - "Couldn't retrieve generic parameter type from " + clazz.getSimpleName() - + " because it doesn't implement " - + expectedImplementedInterface.getSimpleName() - + " directly")); + if (expectedImplementedInterface.isAssignableFrom(clazz)) { + final var genericInterfaces = clazz.getGenericInterfaces(); + Optional> target = Optional.empty(); + if (genericInterfaces.length > 0) { + // try to find the target interface among them + target = Arrays.stream(genericInterfaces) + .filter(type -> type.getTypeName().startsWith(expectedImplementedInterface.getName()) + && type instanceof ParameterizedType) + .map(ParameterizedType.class::cast) + .findFirst() + .map(t -> { + final Type argument = t.getActualTypeArguments()[0]; + if (argument instanceof Class) { + return (Class) argument; + } + throw new IllegalArgumentException(clazz.getSimpleName() + " implements " + + expectedImplementedInterface.getSimpleName() + + " but indirectly. Java type erasure doesn't allow to retrieve the generic type from it. Retrieved type was: " + + argument); + }); + } + + if (target.isPresent()) { + return target.get(); + } + + // try the parent + var parent = clazz.getSuperclass(); + if (!Object.class.equals(parent)) { + return getFirstTypeArgumentFromInterface(parent, expectedImplementedInterface); + } + } + throw new IllegalArgumentException("Couldn't retrieve generic parameter type from " + + clazz.getSimpleName() + " because it or its superclasses don't implement " + + expectedImplementedInterface.getSimpleName()); } public static Class getFirstTypeArgumentFromSuperClassOrInterface(Class clazz, @@ -144,4 +173,58 @@ public static Class getFirstTypeArgumentFromSuperClassOrInterface(Class cl "Couldn't retrieve generic parameter type from " + clazz.getSimpleName(), e); } } + + public static T instantiateAndConfigureIfNeeded(Class targetClass, + Class expectedType, String context, Configurator configurator) { + // if class to instantiate equals the expected interface, we cannot instantiate it so just + // return null as it means we passed on void-type default value + if (expectedType.equals(targetClass)) { + return null; + } + + try { + final Constructor constructor = targetClass.getDeclaredConstructor(); + constructor.setAccessible(true); + final var instance = constructor.newInstance(); + + if (configurator != null) { + configurator.configure(instance); + } + + return instance; + } catch (InstantiationException | IllegalAccessException | InvocationTargetException + | NoSuchMethodException e) { + throw new OperatorException("Couldn't instantiate " + expectedType.getSimpleName() + " '" + + targetClass.getName() + "': you need to provide an accessible no-arg constructor." + + (context != null ? " Context: " + context : ""), e); + } + } + + public static T instantiate(Class toInstantiate, Class expectedType, + String context) { + return instantiateAndConfigureIfNeeded(toInstantiate, expectedType, context, null); + } + + @FunctionalInterface + public interface Configurator { + void configure(T instance); + } + + @SuppressWarnings("rawtypes") + public static String contextFor(ControllerConfiguration controllerConfiguration, + Class dependentType, + Class configurationAnnotation) { + final var annotationName = + configurationAnnotation != null ? configurationAnnotation.getSimpleName() + : io.javaoperatorsdk.operator.api.reconciler.ControllerConfiguration.class + .getSimpleName(); + var context = "annotation: " + annotationName + ", "; + if (dependentType != null) { + context += "DependentResource: " + dependentType.getName() + ", "; + } + context += "reconciler: " + controllerConfiguration.getName(); + + + return context; + } } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/UtilsTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/UtilsTest.java index 87e60b8aa6..cc4fe9bc48 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/UtilsTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/UtilsTest.java @@ -9,11 +9,14 @@ import io.javaoperatorsdk.operator.api.reconciler.Reconciler; import io.javaoperatorsdk.operator.api.reconciler.UpdateControl; import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; +import io.javaoperatorsdk.operator.api.reconciler.dependent.managed.DependentResourceConfigurator; import io.javaoperatorsdk.operator.processing.dependent.EmptyTestDependentResource; import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependentResource; +import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependentResourceConfig; import io.javaoperatorsdk.operator.sample.simple.TestCustomResource; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; @@ -89,6 +92,14 @@ void getsFirstTypeArgumentFromInterface() { assertThat(Utils.getFirstTypeArgumentFromInterface(EmptyTestDependentResource.class, DependentResource.class)) .isEqualTo(Deployment.class); + + assertThatIllegalArgumentException().isThrownBy( + () -> Utils.getFirstTypeArgumentFromInterface(TestKubernetesDependentResource.class, + DependentResource.class)); + + assertThat(Utils.getFirstTypeArgumentFromInterface(TestKubernetesDependentResource.class, + DependentResourceConfigurator.class)) + .isEqualTo(KubernetesDependentResourceConfig.class); } @Test From 06e3226bf9e7c95316be81c4b0716fe65bab1490 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Wed, 5 Oct 2022 11:41:32 +0200 Subject: [PATCH 2/2] fix: account for parameterized argument --- .../io/javaoperatorsdk/operator/api/config/Utils.java | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/Utils.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/Utils.java index efcf9d6e3b..98d835af58 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/Utils.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/Utils.java @@ -126,6 +126,14 @@ public static Class getFirstTypeArgumentFromInterface(Class clazz, if (argument instanceof Class) { return (Class) argument; } + // account for the case where the argument itself has parameters, which we will ignore + // and just return the raw type + if (argument instanceof ParameterizedType) { + final var rawType = ((ParameterizedType) argument).getRawType(); + if (rawType instanceof Class) { + return (Class) rawType; + } + } throw new IllegalArgumentException(clazz.getSimpleName() + " implements " + expectedImplementedInterface.getSimpleName() + " but indirectly. Java type erasure doesn't allow to retrieve the generic type from it. Retrieved type was: "