Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improve: dependent configuration improvements - context independent #2389

Merged
merged 6 commits into from
Jun 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ public List<EventSource> prepareEventSources(
boundedItemStore(new KubernetesClientBuilder().build(),
ConfigMap.class, Duration.ofMinutes(1), 1); // setting max size for testing purposes

var es = new InformerEventSource<>(InformerConfiguration.from(ConfigMap.class, context)
var es = new InformerEventSource<>(InformerConfiguration.from(ConfigMap.class, primaryClass())
.withItemStore(boundedItemStore)
.withSecondaryToPrimaryMapper(
Mappers.fromOwnerReferences(context.getPrimaryResourceClass(),
Expand All @@ -104,4 +104,7 @@ public static <R extends HasMetadata> BoundedItemStore<R> boundedItemStore(
.build();
return CaffeineBoundedItemStores.boundedItemStore(client, rClass, cache);
}

protected abstract Class<P> primaryClass();

}
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,8 @@
public class BoundedCacheClusterScopeTestReconciler extends
AbstractTestReconciler<BoundedCacheClusterScopeTestCustomResource> {

@Override
protected Class<BoundedCacheClusterScopeTestCustomResource> primaryClass() {
return BoundedCacheClusterScopeTestCustomResource.class;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,8 @@
public class BoundedCacheTestReconciler
extends AbstractTestReconciler<BoundedCacheTestCustomResource> {

@Override
protected Class<BoundedCacheTestCustomResource> primaryClass() {
return BoundedCacheTestCustomResource.class;
}
}
65 changes: 0 additions & 65 deletions docs/documentation/v5-0-migration.md

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import io.javaoperatorsdk.operator.OperatorException;
import io.javaoperatorsdk.operator.ReconcilerUtils;
import io.javaoperatorsdk.operator.api.config.Utils.Configurator;
import io.javaoperatorsdk.operator.api.config.dependent.DependentResourceConfigurationResolver;
import io.javaoperatorsdk.operator.api.config.dependent.DependentResourceSpec;
import io.javaoperatorsdk.operator.api.config.workflow.WorkflowSpec;
import io.javaoperatorsdk.operator.api.reconciler.Constants;
Expand Down Expand Up @@ -194,7 +195,7 @@ public boolean handleExceptionsInReconciler() {
@SuppressWarnings({"unchecked", "rawtypes"})
private static List<DependentResourceSpec> dependentResources(
Workflow annotation,
ControllerConfiguration<?> parent) {
ControllerConfiguration<?> controllerConfiguration) {
final var dependents = annotation.dependents();


Expand All @@ -213,7 +214,7 @@ private static List<DependentResourceSpec> dependentResources(
"A DependentResource named '" + dependentName + "' already exists: " + spec);
}

final var name = parent.getName();
final var name = controllerConfiguration.getName();

var eventSourceName = dependent.useEventSourceWithName();
eventSourceName = Constants.NO_VALUE_SET.equals(eventSourceName) ? null : eventSourceName;
Expand All @@ -225,6 +226,12 @@ private static List<DependentResourceSpec> dependentResources(
Utils.instantiate(dependent.deletePostcondition(), Condition.class, context),
Utils.instantiate(dependent.activationCondition(), Condition.class, context),
eventSourceName);

// extract potential configuration
DependentResourceConfigurationResolver.configureSpecFromConfigured(spec,
controllerConfiguration,
dependentType);

specsMap.put(dependentName, spec);
}
return specsMap.values().stream().toList();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

import io.fabric8.kubernetes.api.model.HasMetadata;
import io.javaoperatorsdk.operator.ReconcilerUtils;
import io.javaoperatorsdk.operator.api.config.dependent.DependentResourceSpec;
import io.javaoperatorsdk.operator.api.config.workflow.WorkflowSpec;
import io.javaoperatorsdk.operator.api.reconciler.MaxReconciliationInterval;
import io.javaoperatorsdk.operator.processing.event.rate.LinearRateLimiter;
Expand Down Expand Up @@ -71,7 +72,6 @@ default Optional<Duration> maxReconciliationInterval() {
return Optional.of(Duration.ofHours(MaxReconciliationInterval.DEFAULT_INTERVAL));
}

@SuppressWarnings("unused")
ConfigurationService getConfigurationService();

@SuppressWarnings("unchecked")
Expand All @@ -86,7 +86,7 @@ default Class<P> getResourceClass() {

@SuppressWarnings("unused")
default Set<String> getEffectiveNamespaces() {
return ResourceConfiguration.super.getEffectiveNamespaces(getConfigurationService());
return ResourceConfiguration.super.getEffectiveNamespaces(this);
}

/**
Expand All @@ -100,4 +100,5 @@ default String fieldManager() {
return getName();
}

<C> C getConfigurationFor(DependentResourceSpec<?, P, C> spec);
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
import static io.javaoperatorsdk.operator.api.reconciler.Constants.DEFAULT_NAMESPACES_SET;
import static io.javaoperatorsdk.operator.api.reconciler.Constants.WATCH_CURRENT_NAMESPACE_SET;

@SuppressWarnings({"rawtypes", "unused"})
@SuppressWarnings({"rawtypes", "unused", "UnusedReturnValue"})
public class ControllerConfigurationOverrider<R extends HasMetadata> {

private String finalizer;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@

import io.fabric8.kubernetes.api.model.HasMetadata;
import io.fabric8.kubernetes.client.informers.cache.ItemStore;
import io.javaoperatorsdk.operator.api.config.dependent.DependentResourceConfigurationProvider;
import io.javaoperatorsdk.operator.api.config.dependent.DependentResourceSpec;
import io.javaoperatorsdk.operator.api.config.workflow.WorkflowSpec;
import io.javaoperatorsdk.operator.api.reconciler.Reconciler;
Expand All @@ -22,8 +21,7 @@
@SuppressWarnings("rawtypes")
public class ResolvedControllerConfiguration<P extends HasMetadata>
extends DefaultResourceConfiguration<P>
implements io.javaoperatorsdk.operator.api.config.ControllerConfiguration<P>,
DependentResourceConfigurationProvider {
implements io.javaoperatorsdk.operator.api.config.ControllerConfiguration<P> {

private final String name;
private final boolean generationAware;
Expand Down Expand Up @@ -168,8 +166,15 @@ public ConfigurationService getConfigurationService() {
}

@Override
public Object getConfigurationFor(DependentResourceSpec spec) {
return configurations.get(spec);
@SuppressWarnings("unchecked")
public <C> C getConfigurationFor(DependentResourceSpec<?, P, C> spec) {
// first check if there's an overridden configuration at the controller level
var config = configurations.get(spec);
if (config == null) {
// if not, check the spec for configuration
config = spec.getConfiguration().orElse(null);
}
return (C) config;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,11 +111,12 @@ static Set<String> ensureValidNamespaces(Collection<String> namespaces) {
*
* @return a Set of namespace names the associated controller will watch
*/
default Set<String> getEffectiveNamespaces(ConfigurationService configurationService) {
default Set<String> getEffectiveNamespaces(ControllerConfiguration<?> controllerConfiguration) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This really isn't ideal: an interface should not reference one of its children in its API.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but this is just an utility method effectively. I think the problem is more about that ControllerConfiguraiton extends ResourceConfiguration, since Controller is not really a resource resource. It should rather reference a resource configuration (the primary resource), but not sure if we want to change that at this point.

Maybe in a different PR we can take a look on this.

var targetNamespaces = getNamespaces();
if (watchCurrentNamespace()) {
final String namespace =
configurationService.getKubernetesClient().getConfiguration().getNamespace();
controllerConfiguration.getConfigurationService().getKubernetesClient().getConfiguration()
.getNamespace();
if (namespace == null) {
throw new OperatorException(
"Couldn't retrieve the currently connected namespace. Make sure it's correctly set in your ~/.kube/config file, using, e.g. 'kubectl config set-context <your context> --namespace=<your namespace>'");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,9 @@
import java.lang.annotation.Annotation;

import io.javaoperatorsdk.operator.api.config.ControllerConfiguration;
import io.javaoperatorsdk.operator.api.reconciler.dependent.managed.DependentResourceConfigurator;

public interface ConfigurationConverter<A extends Annotation, C, D extends DependentResourceConfigurator<? extends C>> {
public interface ConfigurationConverter<A extends Annotation, C> {

C configFrom(A configAnnotation, ControllerConfiguration<?> parentConfiguration,
Class<D> originatingClass);
C configFrom(A configAnnotation, DependentResourceSpec<?, ?, C> spec,
ControllerConfiguration<?> parentConfiguration);
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
import io.javaoperatorsdk.operator.api.config.ControllerConfiguration;
import io.javaoperatorsdk.operator.api.config.Utils;
import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource;
import io.javaoperatorsdk.operator.api.reconciler.dependent.managed.DependentResourceConfigurator;

@SuppressWarnings({"rawtypes", "unchecked"})
public class DependentResourceConfigurationResolver {
Expand All @@ -20,39 +19,18 @@ private DependentResourceConfigurationResolver() {}
private static final Map<Class<? extends ConfigurationConverter>, ConfigurationConverter> knownConverters =
new HashMap<>();

public static <C extends ControllerConfiguration<? extends HasMetadata>> void configure(
DependentResource dependentResource, DependentResourceSpec spec, C parentConfiguration) {
if (dependentResource instanceof DependentResourceConfigurator configurator) {
final var config = configurationFor(spec, parentConfiguration);
configurator.configureWith(config);
}
}

public static <C extends ControllerConfiguration<? extends HasMetadata>> Object configurationFor(
DependentResourceSpec spec, C parentConfiguration) {

// first check if the parent configuration has potentially already resolved the configuration
if (parentConfiguration instanceof DependentResourceConfigurationProvider provider) {
final var configuration = provider.getConfigurationFor(spec);
if (configuration != null) {
return configuration;
}
}

// find Configured-annotated class if it exists
return extractConfigurationFromConfigured(spec.getDependentResourceClass(),
parentConfiguration);
}

public static <C extends ControllerConfiguration<? extends HasMetadata>> Object extractConfigurationFromConfigured(
Class<? extends DependentResource> dependentResourceClass, C parentConfiguration) {
public static <C extends ControllerConfiguration<?>> void configureSpecFromConfigured(
DependentResourceSpec spec,
C parentConfiguration,
Class<? extends DependentResource> dependentResourceClass) {
var converterAnnotationPair = converters.get(dependentResourceClass);

Annotation configAnnotation;
if (converterAnnotationPair == null) {
var configuredClassPair = getConfigured(dependentResourceClass);
if (configuredClassPair == null) {
return null;
return;
}

// check if we already have a converter registered for the found Configured annotated class
Expand All @@ -76,7 +54,8 @@ public static <C extends ControllerConfiguration<? extends HasMetadata>> Object

// always called even if the annotation is null so that implementations can provide default
// values
return converter.configFrom(configAnnotation, parentConfiguration, dependentResourceClass);
final var config = converter.configFrom(configAnnotation, spec, parentConfiguration);
spec.setNullableConfiguration(config);
}

private static ConfiguredClassPair getConfigured(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,23 +8,17 @@
import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource;
import io.javaoperatorsdk.operator.processing.dependent.workflow.Condition;

public class DependentResourceSpec<R, P extends HasMetadata> {
public class DependentResourceSpec<R, P extends HasMetadata, C> {

private final Class<? extends DependentResource<R, P>> dependentResourceClass;

private final String name;

private final Set<String> dependsOn;

private final Condition<?, ?> readyCondition;

private final Condition<?, ?> reconcileCondition;

private final Condition<?, ?> deletePostCondition;

private final Condition<?, ?> activationCondition;

private final String useEventSourceWithName;
private C nullableConfiguration;

public DependentResourceSpec(Class<? extends DependentResource<R, P>> dependentResourceClass,
String name, Set<String> dependsOn, Condition<?, ?> readyCondition,
Expand Down Expand Up @@ -62,7 +56,7 @@ public boolean equals(Object o) {
if (o == null || getClass() != o.getClass()) {
return false;
}
DependentResourceSpec<?, ?> that = (DependentResourceSpec<?, ?>) o;
DependentResourceSpec<?, ?, ?> that = (DependentResourceSpec<?, ?, ?>) o;
return name.equals(that.name);
}

Expand Down Expand Up @@ -98,4 +92,13 @@ public Condition getActivationCondition() {
public Optional<String> getUseEventSourceWithName() {
return Optional.ofNullable(useEventSourceWithName);
}

public Optional<C> getConfiguration() {
return Optional.ofNullable(nullableConfiguration);
}

protected void setNullableConfiguration(C configuration) {
this.nullableConfiguration = configuration;
}

}
Loading
Loading