Skip to content

refactor: make shouldUseSSA work with types instead of instances + tests #2538

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
Sep 24, 2024
Merged

refactor: make shouldUseSSA work with types instead of instances + tests #2538

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1849f31
refactor: make shouldUseSSA work with types instead of instances + tests
metacosm Sep 24, 2024
ef5dc6e
fix: shouldUseSSA should also work with unmanaged configuration
metacosm Sep 24, 2024
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
51 changes: 33 additions & 18 deletions ...ework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResourceFactory;
import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependent;
import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependentResource;
import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependentResourceConfig;
import io.javaoperatorsdk.operator.processing.dependent.kubernetes.ResourceUpdaterMatcher;
import io.javaoperatorsdk.operator.processing.dependent.workflow.ManagedWorkflowFactory;

Expand Down Expand Up @@ -362,29 +361,45 @@ default boolean ssaBasedCreateUpdateMatchForDependentResources() {
}

/**
* This is mostly useful as an integration point for downstream projects to be able to reuse the
* logic used to determine whether a given {@link KubernetesDependentResource} should use SSA or
* not.
*
* @param dependentResource the {@link KubernetesDependentResource} under consideration
* @return {@code true} if SSA should be used for
* @param <R> the dependent resource type
* @param <P> the primary resource type
* @since 4.9.4
Copy link
Collaborator

Choose a reason for hiding this comment

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

I personally don't think that deprecating should mean removing of old JavaDoc but I don't know what's the custom in this project.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think we have one but removing the javadoc makes it even more obvious that people should really be using the other one instead, imo… 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've removed the deprecation in the end.

* @deprecated Use {@link #shouldUseSSA(Class, Class)} instead
*/
@Deprecated(forRemoval = true)
default <R extends HasMetadata, P extends HasMetadata> boolean shouldUseSSA(
KubernetesDependentResource<R, P> dependentResource) {
if (dependentResource instanceof ResourceUpdaterMatcher) {
return shouldUseSSA(dependentResource.getClass(), dependentResource.resourceType());
}

/**
* This is mostly useful as an integration point for downstream projects to be able to reuse the
* logic used to determine whether a given {@link KubernetesDependentResource} type should use SSA
* or not.
*
* @param dependentResourceType the {@link KubernetesDependentResource} type under consideration
* @param resourceType the resource type associated with the considered dependent resource type
* @return {@code true} if SSA should be used for specified dependent resource type, {@code false}
* otherwise
* @since 4.9.5
*/
@SuppressWarnings("rawtypes")
default boolean shouldUseSSA(Class<? extends KubernetesDependentResource> dependentResourceType,
Class<? extends HasMetadata> resourceType) {
if (ResourceUpdaterMatcher.class.isAssignableFrom(dependentResourceType)) {
return false;
}
Optional<Boolean> useSSAConfig = dependentResource.configuration()
.flatMap(KubernetesDependentResourceConfig::useSSA);
// don't use SSA for certain resources by default, only if explicitly overriden
if (useSSAConfig.isEmpty()
&& defaultNonSSAResources().contains(dependentResource.resourceType())) {
return false;
Boolean useSSAConfig =
Optional.ofNullable(dependentResourceType.getAnnotation(KubernetesDependent.class))
.map(kd -> kd.useSSA().asBoolean())
.orElse(null);
// don't use SSA for certain resources by default, only if explicitly overridden
if (useSSAConfig == null) {
if (defaultNonSSAResources().contains(resourceType)) {
return false;
} else {
return ssaBasedCreateUpdateMatchForDependentResources();
}
} else {
return useSSAConfig;
}
return useSSAConfig.orElse(ssaBasedCreateUpdateMatchForDependentResources());
}

/**
Expand Down
3 changes: 2 additions & 1 deletion ...javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,8 @@ protected void addMetadata(boolean forMatch, R actualResource, final R target, P

protected boolean useSSA(Context<P> context) {
if (useSSA == null) {
useSSA = context.getControllerConfiguration().getConfigurationService().shouldUseSSA(this);
useSSA = context.getControllerConfiguration().getConfigurationService()
.shouldUseSSA(getClass(), resourceType());
}
return useSSA;
}
Expand Down
125 changes: 100 additions & 25 deletions ...mework/src/test/java/io/javaoperatorsdk/operator/config/BaseConfigurationServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

import io.fabric8.kubernetes.api.model.ConfigMap;
import io.fabric8.kubernetes.api.model.HasMetadata;
import io.fabric8.kubernetes.api.model.Service;
import io.javaoperatorsdk.operator.OperatorException;
import io.javaoperatorsdk.operator.api.config.AnnotationConfigurable;
import io.javaoperatorsdk.operator.api.config.BaseConfigurationService;
Expand All @@ -31,6 +32,7 @@
import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource;
import io.javaoperatorsdk.operator.api.reconciler.dependent.ReconcileResult;
import io.javaoperatorsdk.operator.api.reconciler.dependent.managed.DependentResourceConfigurator;
import io.javaoperatorsdk.operator.processing.dependent.kubernetes.BooleanWithUndefined;
import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependent;
import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependentResource;
import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependentResourceConfig;
Expand All @@ -40,6 +42,7 @@
import io.javaoperatorsdk.operator.processing.retry.GradualRetry;
import io.javaoperatorsdk.operator.processing.retry.Retry;
import io.javaoperatorsdk.operator.processing.retry.RetryExecution;
import io.javaoperatorsdk.operator.sample.readonly.ConfigMapReader;
import io.javaoperatorsdk.operator.sample.readonly.ReadOnlyDependent;

import static org.junit.jupiter.api.Assertions.assertEquals;
Expand Down Expand Up @@ -106,7 +109,7 @@ void getDependentResources() {
var maybeConfig =
DependentResourceConfigurationResolver.configurationFor(dependentSpec, configuration);
assertNotNull(maybeConfig);
assertTrue(maybeConfig instanceof KubernetesDependentResourceConfig);
assertInstanceOf(KubernetesDependentResourceConfig.class, maybeConfig);
final var config = (KubernetesDependentResourceConfig) maybeConfig;
// check that the DependentResource inherits the controller's configuration if applicable
assertEquals(1, config.namespaces().size());
Expand All @@ -121,7 +124,7 @@ void getDependentResources() {
maybeConfig = DependentResourceConfigurationResolver.configurationFor(dependentSpec,
configuration);
assertNotNull(maybeConfig);
assertTrue(maybeConfig instanceof KubernetesDependentResourceConfig);
assertInstanceOf(KubernetesDependentResourceConfig.class, maybeConfig);
}

@Test
Expand Down Expand Up @@ -234,6 +237,50 @@ void configuringFromCustomAnnotationsShouldWork() {
assertEquals(CustomConfigConverter.CONVERTER_PROVIDED_DEFAULT, getValue(config, 1));
}

@Test
void excludedResourceClassesShouldNotUseSSAByDefault() {
final var config = configFor(new SelectorReconciler());

// ReadOnlyDependent targets ConfigMap which is configured to not use SSA by default
final var kubernetesDependentResourceConfig =
extractDependentKubernetesResourceConfig(config, 1);
assertNotNull(kubernetesDependentResourceConfig);
assertTrue(kubernetesDependentResourceConfig.useSSA().isEmpty());
assertFalse(configurationService.shouldUseSSA(ReadOnlyDependent.class, ConfigMap.class));
}

@Test
void excludedResourceClassesShouldUseSSAIfAnnotatedToDoSo() {
final var config = configFor(new SelectorReconciler());

// WithAnnotation dependent also targets ConfigMap but overrides the default with the annotation
final var kubernetesDependentResourceConfig =
extractDependentKubernetesResourceConfig(config, 0);
assertNotNull(kubernetesDependentResourceConfig);
assertFalse(kubernetesDependentResourceConfig.useSSA().isEmpty());
assertTrue((Boolean) kubernetesDependentResourceConfig.useSSA().get());
assertTrue(configurationService.shouldUseSSA(SelectorReconciler.WithAnnotation.class,
ConfigMap.class));
}

@Test
void dependentsShouldUseSSAByDefaultIfNotExcluded() {
final var config = configFor(new DefaultSSAForDependentsReconciler());

var kubernetesDependentResourceConfig = extractDependentKubernetesResourceConfig(config, 0);
assertNotNull(kubernetesDependentResourceConfig);
assertTrue(kubernetesDependentResourceConfig.useSSA().isEmpty());
assertTrue(configurationService.shouldUseSSA(
DefaultSSAForDependentsReconciler.DefaultDependent.class, ConfigMapReader.class));

kubernetesDependentResourceConfig = extractDependentKubernetesResourceConfig(config, 1);
assertNotNull(kubernetesDependentResourceConfig);
assertTrue(kubernetesDependentResourceConfig.useSSA().isPresent());
assertFalse((Boolean) kubernetesDependentResourceConfig.useSSA().get());
assertFalse(configurationService
.shouldUseSSA(DefaultSSAForDependentsReconciler.NonSSADependent.class, Service.class));
}

private static int getValue(
io.javaoperatorsdk.operator.api.config.ControllerConfiguration<?> configuration, int index) {
return ((CustomConfig) DependentResourceConfigurationResolver
Expand All @@ -247,32 +294,33 @@ private static int getValue(
private static class MaxIntervalReconciler implements Reconciler<ConfigMap> {

@Override
public UpdateControl<ConfigMap> reconcile(ConfigMap resource, Context<ConfigMap> context)
throws Exception {
public UpdateControl<ConfigMap> reconcile(ConfigMap resource, Context<ConfigMap> context) {
return null;
}
}

@ControllerConfiguration(namespaces = OneDepReconciler.CONFIGURED_NS,
dependents = @Dependent(type = ReadOnlyDependent.class))
private static class OneDepReconciler implements Reconciler<ConfigMap> {
private static class OneDepReconciler implements Reconciler<ConfigMapReader> {

private static final String CONFIGURED_NS = "foo";

@Override
public UpdateControl<ConfigMap> reconcile(ConfigMap resource, Context<ConfigMap> context) {
public UpdateControl<ConfigMapReader> reconcile(ConfigMapReader resource,
Context<ConfigMapReader> context) {
return null;
}
}

@ControllerConfiguration(
dependents = @Dependent(type = ReadOnlyDependent.class, name = NamedDepReconciler.NAME))
private static class NamedDepReconciler implements Reconciler<ConfigMap> {
private static class NamedDepReconciler implements Reconciler<ConfigMapReader> {

private static final String NAME = "foo";

@Override
public UpdateControl<ConfigMap> reconcile(ConfigMap resource, Context<ConfigMap> context) {
public UpdateControl<ConfigMapReader> reconcile(ConfigMapReader resource,
Context<ConfigMapReader> context) {
return null;
}
}
Expand All @@ -282,10 +330,11 @@ public UpdateControl<ConfigMap> reconcile(ConfigMap resource, Context<ConfigMap>
@Dependent(type = ReadOnlyDependent.class),
@Dependent(type = ReadOnlyDependent.class)
})
private static class DuplicatedDepReconciler implements Reconciler<ConfigMap> {
private static class DuplicatedDepReconciler implements Reconciler<ConfigMapReader> {

@Override
public UpdateControl<ConfigMap> reconcile(ConfigMap resource, Context<ConfigMap> context) {
public UpdateControl<ConfigMapReader> reconcile(ConfigMapReader resource,
Context<ConfigMapReader> context) {
return null;
}
}
Expand All @@ -295,21 +344,23 @@ public UpdateControl<ConfigMap> reconcile(ConfigMap resource, Context<ConfigMap>
@Dependent(type = ReadOnlyDependent.class, name = NamedDuplicatedDepReconciler.NAME),
@Dependent(type = ReadOnlyDependent.class)
})
private static class NamedDuplicatedDepReconciler implements Reconciler<ConfigMap> {
private static class NamedDuplicatedDepReconciler implements Reconciler<ConfigMapReader> {

private static final String NAME = "duplicated";

@Override
public UpdateControl<ConfigMap> reconcile(ConfigMap resource, Context<ConfigMap> context) {
public UpdateControl<ConfigMapReader> reconcile(ConfigMapReader resource,
Context<ConfigMapReader> context) {
return null;
}
}

@ControllerConfiguration
private static class NoDepReconciler implements Reconciler<ConfigMap> {
private static class NoDepReconciler implements Reconciler<ConfigMapReader> {

@Override
public UpdateControl<ConfigMap> reconcile(ConfigMap resource, Context<ConfigMap> context) {
public UpdateControl<ConfigMapReader> reconcile(ConfigMapReader resource,
Context<ConfigMapReader> context) {
return null;
}
}
Expand All @@ -318,16 +369,17 @@ public UpdateControl<ConfigMap> reconcile(ConfigMap resource, Context<ConfigMap>
@Dependent(type = SelectorReconciler.WithAnnotation.class),
@Dependent(type = ReadOnlyDependent.class)
})
private static class SelectorReconciler implements Reconciler<ConfigMap> {
private static class SelectorReconciler implements Reconciler<ConfigMapReader> {

@Override
public UpdateControl<ConfigMap> reconcile(ConfigMap resource, Context<ConfigMap> context)
throws Exception {
public UpdateControl<ConfigMapReader> reconcile(ConfigMapReader resource,
Context<ConfigMapReader> context) {
return null;
}

@KubernetesDependent
private static class WithAnnotation extends KubernetesDependentResource<ConfigMap, ConfigMap> {
@KubernetesDependent(useSSA = BooleanWithUndefined.TRUE)
private static class WithAnnotation
extends KubernetesDependentResource<ConfigMap, ConfigMapReader> {

public WithAnnotation() {
super(ConfigMap.class);
Expand All @@ -343,6 +395,32 @@ public UpdateControl<ConfigMap> reconcile(ConfigMap resource, Context<ConfigMap>
}
}

@ControllerConfiguration(dependents = {
@Dependent(type = DefaultSSAForDependentsReconciler.DefaultDependent.class),
@Dependent(type = DefaultSSAForDependentsReconciler.NonSSADependent.class)
})
private static class DefaultSSAForDependentsReconciler implements Reconciler<ConfigMap> {

@Override
public UpdateControl<ConfigMap> reconcile(ConfigMap resource, Context<ConfigMap> context) {
return null;
}

private static class DefaultDependent
extends KubernetesDependentResource<ConfigMapReader, ConfigMap> {
public DefaultDependent() {
super(ConfigMapReader.class);
}
}

@KubernetesDependent(useSSA = BooleanWithUndefined.FALSE)
private static class NonSSADependent extends KubernetesDependentResource<Service, ConfigMap> {
public NonSSADependent() {
super(Service.class);
}
}
}

public static class TestRetry implements Retry, AnnotationConfigurable<TestRetryConfiguration> {

private int value;
Expand Down Expand Up @@ -377,8 +455,7 @@ public void initFrom(TestRetryConfiguration configuration) {
private static class ConfigurableRateLimitAndRetryReconciler implements Reconciler<ConfigMap> {

@Override
public UpdateControl<ConfigMap> reconcile(ConfigMap resource, Context<ConfigMap> context)
throws Exception {
public UpdateControl<ConfigMap> reconcile(ConfigMap resource, Context<ConfigMap> context) {
return UpdateControl.noUpdate();
}
}
Expand All @@ -397,8 +474,7 @@ private static class CheckRetryingGraduallyConfiguration implements Reconciler<C
public static final int MAX_INTERVAL = 60000;

@Override
public UpdateControl<ConfigMap> reconcile(ConfigMap resource, Context<ConfigMap> context)
throws Exception {
public UpdateControl<ConfigMap> reconcile(ConfigMap resource, Context<ConfigMap> context) {
return UpdateControl.noUpdate();
}
}
Expand Down Expand Up @@ -445,8 +521,7 @@ public UpdateControl<ConfigMap> reconcile(ConfigMap resource, Context<ConfigMap>
private static class CustomAnnotationReconciler implements Reconciler<ConfigMap> {

@Override
public UpdateControl<ConfigMap> reconcile(ConfigMap resource, Context<ConfigMap> context)
throws Exception {
public UpdateControl<ConfigMap> reconcile(ConfigMap resource, Context<ConfigMap> context) {
return null;
}
}
Expand Down
Loading