Skip to content

fix: decouple DependentResource creation from configuration #883

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jan 31, 2022
Merged

fix: decouple DependentResource creation from configuration #883

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
23fdf20
fix: do not call default resource class resolution logic by default
metacosm Jan 28, 2022
04cc4dd
fix: do not create DependentResources directly from configuration
metacosm Jan 28, 2022
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
2 changes: 1 addition & 1 deletion ...rk-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ default ResourceEventFilter<R> getEventFilter() {
return ResourceEventFilters.passthrough();
}

default List<DependentResource> getDependentResources() {
default List<DependentResourceConfiguration> getDependentResources() {
return Collections.emptyList();
}

Expand Down
6 changes: 3 additions & 3 deletions .../src/main/java/io/javaoperatorsdk/operator/api/config/DefaultControllerConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ public class DefaultControllerConfiguration<R extends HasMetadata>
private final boolean generationAware;
private final RetryConfiguration retryConfiguration;
private final ResourceEventFilter<R> resourceEventFilter;
private final List<DependentResource> dependents;
private final List<DependentResourceConfiguration> dependents;

// NOSONAR constructor is meant to provide all information
public DefaultControllerConfiguration(
Expand All @@ -33,7 +33,7 @@ public DefaultControllerConfiguration(
ResourceEventFilter<R> resourceEventFilter,
Class<R> resourceClass,
ConfigurationService service,
List<DependentResource> dependents) {
List<DependentResourceConfiguration> dependents) {
super(labelSelector, resourceClass, namespaces);
this.associatedControllerClassName = associatedControllerClassName;
this.name = name;
Expand Down Expand Up @@ -96,7 +96,7 @@ public ResourceEventFilter<R> getEventFilter() {
}

@Override
public List<DependentResource> getDependentResources() {
public List<DependentResourceConfiguration> getDependentResources() {
return dependents;
}
}
3 changes: 1 addition & 2 deletions ...re/src/main/java/io/javaoperatorsdk/operator/api/config/DefaultResourceConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,7 @@ public DefaultResourceConfiguration(String labelSelector, Class<R> resourceClass
public DefaultResourceConfiguration(String labelSelector, Class<R> resourceClass,
Set<String> namespaces) {
this.labelSelector = labelSelector;
this.resourceClass = resourceClass == null ? ResourceConfiguration.super.getResourceClass()
: resourceClass;
this.resourceClass = resourceClass;
this.namespaces = namespaces != null ? namespaces : Collections.emptySet();
this.watchAllNamespaces = this.namespaces.isEmpty();
}
Expand Down
10 changes: 10 additions & 0 deletions .../src/main/java/io/javaoperatorsdk/operator/api/config/DependentResourceConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package io.javaoperatorsdk.operator.api.config;

import io.fabric8.kubernetes.api.model.HasMetadata;

public interface DependentResourceConfiguration<R, P extends HasMetadata> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are these classes configurable? or what is the point of this configuration?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The goal is to decouple the configuration of the dependent resource from its creation. Previously, this was done by the same class and the AnnotationConfiguration was doing the instantiating of the DependentResource implementation which led to it being too complex but also needed to know some details about how the implementation worked (in particular how to deal with Kubernetes dependent resources).
This also forced the quarkus extension to have to instantiate the dependent resource implementation class at build time because it was right there on the ControllerConfiguration.getDependentResources() method. Well, it's not exactly true because it's possible to delay the instantiation until the method is called but that wasn't clean enough and led to more complications.


Class<? extends DependentResource<R, P>> getDependentResourceClass();

Class<R> getResourceClass();
Copy link
Collaborator

@csviri csviri Jan 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This should not be getPrimaryResourceClass() ? or? it's not clear for me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, that's actually not the primary resource class but the dependent resource one. The naming is tricky and should probably improved indeed: getDependentResourceClass returns the DependentResource implementation class (e.g. DeploymentDependentResource), while getResourceClass returns the actual associated resource class (e.g. Deployment).
Note that I've been trying to be consistent in the parameter types naming for these new classes: R represents a resource class, while P represents the primary resource types when it's needed. If we wanted to be more explicit we could use more explicit parameter types (e.g. use Primary instead of simply P) but that's not a common pattern in Java code…

}
11 changes: 9 additions & 2 deletions ...ava/io/javaoperatorsdk/operator/api/reconciler/dependent/DependentResourceController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,12 @@

import io.fabric8.kubernetes.api.model.HasMetadata;
import io.javaoperatorsdk.operator.api.config.DependentResource;
import io.javaoperatorsdk.operator.api.config.DependentResourceConfiguration;
import io.javaoperatorsdk.operator.api.reconciler.Context;
import io.javaoperatorsdk.operator.api.reconciler.EventSourceContext;
import io.javaoperatorsdk.operator.processing.event.source.EventSource;

public class DependentResourceController<R, P extends HasMetadata>
public class DependentResourceController<R, P extends HasMetadata, C extends DependentResourceConfiguration<R, P>>
implements DependentResource<R, P>, Builder<R, P>, Updater<R, P>, Persister<R, P>,
Cleaner<R, P> {

Expand All @@ -15,14 +16,16 @@ public class DependentResourceController<R, P extends HasMetadata>
private final Cleaner<R, P> cleaner;
private final Persister<R, P> persister;
private final DependentResource<R, P> delegate;
private final C configuration;

@SuppressWarnings("unchecked")
public DependentResourceController(DependentResource<R, P> delegate) {
public DependentResourceController(DependentResource<R, P> delegate, C configuration) {
this.delegate = delegate;
builder = (delegate instanceof Builder) ? (Builder<R, P>) delegate : null;
updater = (delegate instanceof Updater) ? (Updater<R, P>) delegate : null;
cleaner = (delegate instanceof Cleaner) ? (Cleaner<R, P>) delegate : null;
persister = initPersister(delegate);
this.configuration = configuration;
}

@SuppressWarnings("unchecked")
Expand Down Expand Up @@ -84,4 +87,8 @@ public void createOrReplace(R dependentResource, Context context) {
public R getFor(P primary, Context context) {
return persister.getFor(primary, context);
}

public C getConfiguration() {
return configuration;
}
}
24 changes: 18 additions & 6 deletions ...javaoperatorsdk/operator/api/reconciler/dependent/DependentResourceControllerFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,26 @@
package io.javaoperatorsdk.operator.api.reconciler.dependent;

import java.lang.reflect.InvocationTargetException;

import io.fabric8.kubernetes.api.model.HasMetadata;
import io.javaoperatorsdk.operator.api.config.DependentResource;
import io.javaoperatorsdk.operator.api.config.DependentResourceConfiguration;

public interface DependentResourceControllerFactory<P extends HasMetadata> {

default <R> DependentResourceController<R, P> from(DependentResource<R, P> dependent) {
// todo: this needs to be cleaned-up / redesigned
return dependent instanceof DependentResourceController
? (DependentResourceController<R, P>) dependent
: new DependentResourceController<>(dependent);
default <T extends DependentResourceController<R, P, C>, R, C extends DependentResourceConfiguration<R, P>> T from(
C config) {
try {
final var dependentResource = config.getDependentResourceClass().getConstructor()
.newInstance();
if (config instanceof KubernetesDependentResourceConfiguration) {
return (T) new KubernetesDependentResourceController(dependentResource,
(KubernetesDependentResourceConfiguration) config);
} else {
return (T) new DependentResourceController(dependentResource, config);
}
} catch (NoSuchMethodException | InvocationTargetException | InstantiationException
| IllegalAccessException e) {
throw new IllegalArgumentException(e);
}
}
}
65 changes: 47 additions & 18 deletions ...eratorsdk/operator/api/reconciler/dependent/KubernetesDependentResourceConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,35 +4,64 @@

import io.fabric8.kubernetes.api.model.HasMetadata;
import io.javaoperatorsdk.operator.api.config.ConfigurationService;
import io.javaoperatorsdk.operator.api.config.DependentResource;
import io.javaoperatorsdk.operator.api.config.DependentResourceConfiguration;
import io.javaoperatorsdk.operator.processing.event.source.AssociatedSecondaryResourceIdentifier;
import io.javaoperatorsdk.operator.processing.event.source.PrimaryResourcesRetriever;
import io.javaoperatorsdk.operator.processing.event.source.informer.InformerConfiguration;

public class KubernetesDependentResourceConfiguration<R extends HasMetadata, P extends HasMetadata>
extends InformerConfiguration<R, P> {
public interface KubernetesDependentResourceConfiguration<R extends HasMetadata, P extends HasMetadata>
extends InformerConfiguration<R, P>, DependentResourceConfiguration<R, P> {

private final boolean owned;
class DefaultKubernetesDependentResourceConfiguration<R extends HasMetadata, P extends HasMetadata>
extends DefaultInformerConfiguration<R, P>
implements KubernetesDependentResourceConfiguration<R, P> {

protected KubernetesDependentResourceConfiguration(
ConfigurationService service,
String labelSelector, Class<R> resourceClass,
PrimaryResourcesRetriever<R> secondaryToPrimaryResourcesIdSet,
AssociatedSecondaryResourceIdentifier<P> associatedWith,
boolean skipUpdateEventPropagationIfNoChange, Set<String> namespaces, boolean owned) {
super(service, labelSelector, resourceClass, secondaryToPrimaryResourcesIdSet, associatedWith,
skipUpdateEventPropagationIfNoChange, namespaces);
this.owned = owned;
private final boolean owned;
private final Class<DependentResource<R, P>> dependentResourceClass;

protected DefaultKubernetesDependentResourceConfiguration(
ConfigurationService service,
String labelSelector, Class<R> resourceClass,
PrimaryResourcesRetriever<R> secondaryToPrimaryResourcesIdSet,
AssociatedSecondaryResourceIdentifier<P> associatedWith,
boolean skipUpdateEventPropagationIfNoChange, Set<String> namespaces, boolean owned,
Class<DependentResource<R, P>> dependentResourceClass) {
super(service, labelSelector, resourceClass, secondaryToPrimaryResourcesIdSet, associatedWith,
skipUpdateEventPropagationIfNoChange, namespaces);
this.owned = owned;
this.dependentResourceClass = dependentResourceClass;
}

public boolean isOwned() {
return owned;
}

@Override
public Class<? extends DependentResource<R, P>> getDependentResourceClass() {
return dependentResourceClass;
}

@Override
public Class<R> getResourceClass() {
return super.getResourceClass();
}
}

public static <R extends HasMetadata, P extends HasMetadata> KubernetesDependentResourceConfiguration<R, P> from(
InformerConfiguration<R, P> cfg, boolean owned) {
return new KubernetesDependentResourceConfiguration<>(cfg.getConfigurationService(),
static <R extends HasMetadata, P extends HasMetadata> KubernetesDependentResourceConfiguration<R, P> from(
InformerConfiguration<R, P> cfg, boolean owned,
Class<? extends DependentResource> dependentResourceClass) {
return new DefaultKubernetesDependentResourceConfiguration<R, P>(cfg.getConfigurationService(),
cfg.getLabelSelector(), cfg.getResourceClass(), cfg.getPrimaryResourcesRetriever(),
cfg.getAssociatedResourceIdentifier(), cfg.isSkipUpdateEventPropagationIfNoChange(),
cfg.getNamespaces(), owned);
cfg.getNamespaces(), owned,
(Class<DependentResource<R, P>>) dependentResourceClass);
}

public boolean isOwned() {
return owned;
boolean isOwned();

@Override
default Class<R> getResourceClass() {
return InformerConfiguration.super.getResourceClass();
}
}
10 changes: 6 additions & 4 deletions ...aoperatorsdk/operator/api/reconciler/dependent/KubernetesDependentResourceController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,16 @@
import io.javaoperatorsdk.operator.processing.event.source.informer.InformerEventSource;

public class KubernetesDependentResourceController<R extends HasMetadata, P extends HasMetadata>
extends DependentResourceController<R, P> {
extends DependentResourceController<R, P, KubernetesDependentResourceConfiguration<R, P>> {

private final KubernetesDependentResourceConfiguration<R, P> configuration;
private KubernetesClient client;
private InformerEventSource<R, P> informer;


public KubernetesDependentResourceController(DependentResource<R, P> delegate,
KubernetesDependentResourceConfiguration<R, P> configuration) {
super(delegate);
super(delegate, configuration);
// todo: check if we can validate that types actually match properly
final var associatedPrimaries =
(delegate instanceof PrimaryResourcesRetriever)
Expand All @@ -36,7 +37,8 @@ public KubernetesDependentResourceController(DependentResource<R, P> delegate,
.withAssociatedSecondaryResourceIdentifier(associatedSecondary)
.build();
this.configuration =
KubernetesDependentResourceConfiguration.from(augmented, configuration.isOwned());
KubernetesDependentResourceConfiguration.from(augmented, configuration.isOwned(),
configuration.getDependentResourceClass());
}

@Override
Expand Down Expand Up @@ -69,6 +71,6 @@ public R getFor(P primary, Context context) {
}

public boolean owned() {
return configuration.isOwned();
return getConfiguration().isOwned();
}
}
9 changes: 5 additions & 4 deletions .../main/java/io/javaoperatorsdk/operator/processing/dependent/DependentResourceManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

import io.fabric8.kubernetes.api.model.HasMetadata;
import io.javaoperatorsdk.operator.api.config.ControllerConfiguration;
import io.javaoperatorsdk.operator.api.config.DependentResource;
import io.javaoperatorsdk.operator.api.config.DependentResourceConfiguration;
import io.javaoperatorsdk.operator.api.reconciler.Context;
import io.javaoperatorsdk.operator.api.reconciler.ContextInitializer;
import io.javaoperatorsdk.operator.api.reconciler.DeleteControl;
Expand Down Expand Up @@ -42,13 +42,14 @@ public DependentResourceManager(Controller<R> controller) {

@Override
public List<EventSource> prepareEventSources(EventSourceContext<R> context) {
final List<DependentResource> configured = configuration.getDependentResources();
final List<DependentResourceConfiguration> configured = configuration.getDependentResources();
dependents = new ArrayList<>(configured.size());

List<EventSource> sources = new ArrayList<>(configured.size() + 5);
configured.forEach(dependent -> {
dependents.add(configuration.dependentFactory().from(dependent));
sources.add(dependent.initEventSource(context));
final var dependentResourceController = configuration.dependentFactory().from(dependent);
dependents.add(dependentResourceController);
sources.add(dependentResourceController.initEventSource(context));
});

return sources;
Expand Down
Loading