From 71999ffad60d517f952a874a9e85dff0cca3a4a9 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Fri, 22 Apr 2022 13:19:23 +0200 Subject: [PATCH 1/2] feat: make default watched namespaces behavior explicit This uses the constant that was initially introduced for the same reason on KubernetesDependent, which is now moved to Constants. Uncovered several issues with configuration overriding in the process and added test coverage. Fixes #1176 --- .../AnnotationControllerConfiguration.java | 3 +- .../ControllerConfigurationOverrider.java | 14 +++- .../config/DefaultResourceConfiguration.java | 15 ++-- .../api/config/ResourceConfiguration.java | 35 ++++++--- .../operator/api/reconciler/Constants.java | 1 + .../reconciler/ControllerConfiguration.java | 2 +- .../kubernetes/KubernetesDependent.java | 1 - .../KubernetesDependentResourceConfig.java | 6 +- .../operator/OperatorTest.java | 8 +-- .../ControllerConfigurationOverriderTest.java | 69 ++++++++++++++++-- .../config/MockControllerConfiguration.java | 18 +++++ .../api/config/ResourceConfigurationTest.java | 71 +++++++++++++++++++ .../operator/processing/ControllerTest.java | 16 ++--- .../event/EventSourceManagerTest.java | 5 +- .../event/ReconciliationDispatcherTest.java | 63 ++++++++-------- .../informer/InformerEventSourceTest.java | 21 +++--- 16 files changed, 256 insertions(+), 92 deletions(-) create mode 100644 operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/MockControllerConfiguration.java create mode 100644 operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/ResourceConfigurationTest.java 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 19535c6d3e..ddd69dff53 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 @@ -66,7 +66,8 @@ public boolean isGenerationAware() { @Override public Set getNamespaces() { - return Set.of(valueOrDefault(annotation, ControllerConfiguration::namespaces, new String[] {})); + return Set.of(valueOrDefault(annotation, ControllerConfiguration::namespaces, + new String[] {Constants.WATCH_ALL_NAMESPACES})); } @Override diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfigurationOverrider.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfigurationOverrider.java index 57b8640a8b..c098a53884 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfigurationOverrider.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfigurationOverrider.java @@ -19,7 +19,7 @@ public class ControllerConfigurationOverrider { private String finalizer; private boolean generationAware; - private final Set namespaces; + private Set namespaces; private RetryConfiguration retry; private String labelSelector; private ResourceEventFilter customResourcePredicate; @@ -52,8 +52,8 @@ public ControllerConfigurationOverrider withGenerationAware(boolean generatio return this; } - public ControllerConfigurationOverrider withCurrentNamespace() { - namespaces.clear(); + public ControllerConfigurationOverrider watchingOnlyCurrentNamespace() { + this.namespaces = ResourceConfiguration.DEPLOYED_NAMESPACE_ONLY; return this; } @@ -64,6 +64,9 @@ public ControllerConfigurationOverrider addingNamespaces(String... namespaces public ControllerConfigurationOverrider removingNamespaces(String... namespaces) { List.of(namespaces).forEach(this.namespaces::remove); + if (this.namespaces.isEmpty()) { + this.namespaces = ResourceConfiguration.DEFAULT_NAMESPACES; + } return this; } @@ -73,6 +76,11 @@ public ControllerConfigurationOverrider settingNamespace(String namespace) { return this; } + public ControllerConfigurationOverrider watchingAllNamespaces() { + this.namespaces = ResourceConfiguration.DEFAULT_NAMESPACES; + return this; + } + public ControllerConfigurationOverrider withRetry(RetryConfiguration retry) { this.retry = retry; return this; diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/DefaultResourceConfiguration.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/DefaultResourceConfiguration.java index 4959b827ad..7ee116ac60 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/DefaultResourceConfiguration.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/DefaultResourceConfiguration.java @@ -1,6 +1,5 @@ package io.javaoperatorsdk.operator.api.config; -import java.util.Collections; import java.util.Set; import io.fabric8.kubernetes.api.model.HasMetadata; @@ -10,21 +9,22 @@ public class DefaultResourceConfiguration private final String labelSelector; private final Set namespaces; - private final boolean watchAllNamespaces; private final Class resourceClass; public DefaultResourceConfiguration(String labelSelector, Class resourceClass, String... namespaces) { this(labelSelector, resourceClass, - namespaces != null ? Set.of(namespaces) : Collections.emptySet()); + namespaces == null || namespaces.length == 0 ? ResourceConfiguration.DEFAULT_NAMESPACES + : Set.of(namespaces)); } public DefaultResourceConfiguration(String labelSelector, Class resourceClass, Set namespaces) { this.labelSelector = labelSelector; this.resourceClass = resourceClass; - this.namespaces = namespaces != null ? namespaces : Collections.emptySet(); - this.watchAllNamespaces = this.namespaces.isEmpty(); + this.namespaces = + namespaces == null || namespaces.isEmpty() ? ResourceConfiguration.DEFAULT_NAMESPACES + : namespaces; } @Override @@ -42,11 +42,6 @@ public Set getNamespaces() { return namespaces; } - @Override - public boolean watchAllNamespaces() { - return watchAllNamespaces; - } - @Override public Class getResourceClass() { return resourceClass; diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ResourceConfiguration.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ResourceConfiguration.java index 2d947050e7..50dbfeb3e1 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ResourceConfiguration.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ResourceConfiguration.java @@ -10,15 +10,18 @@ public interface ResourceConfiguration { + Set DEFAULT_NAMESPACES = Set.of(Constants.WATCH_ALL_NAMESPACES); + Set DEPLOYED_NAMESPACE_ONLY = Set.of(Constants.WATCH_CURRENT_NAMESPACE); + default String getResourceTypeName() { return ReconcilerUtils.getResourceTypeName(getResourceClass()); } /** * Retrieves the label selector that is used to filter which resources are actually watched by the - * associated event source. See - * https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/ for more details on - * syntax. + * associated event source. See the official documentation on the + * topic + * for more details on syntax. * * @return the label selector filtering watched resources */ @@ -32,7 +35,7 @@ default Class getResourceClass() { } default Set getNamespaces() { - return Collections.emptySet(); + return DEFAULT_NAMESPACES; } default boolean watchAllNamespaces() { @@ -40,7 +43,8 @@ default boolean watchAllNamespaces() { } static boolean allNamespacesWatched(Set namespaces) { - return namespaces == null || namespaces.isEmpty(); + failIfNotValid(namespaces); + return DEFAULT_NAMESPACES.equals(namespaces); } default boolean watchCurrentNamespace() { @@ -48,9 +52,24 @@ default boolean watchCurrentNamespace() { } static boolean currentNamespaceWatched(Set namespaces) { - return namespaces != null - && namespaces.size() == 1 - && namespaces.contains(Constants.WATCH_CURRENT_NAMESPACE); + failIfNotValid(namespaces); + return DEPLOYED_NAMESPACE_ONLY.equals(namespaces); + } + + static void failIfNotValid(Set namespaces) { + if (namespaces != null && !namespaces.isEmpty()) { + final var present = namespaces.contains(Constants.WATCH_CURRENT_NAMESPACE) + || namespaces.contains(Constants.WATCH_ALL_NAMESPACES); + if (!present || namespaces.size() == 1) { + return; + } + } + + throw new IllegalArgumentException( + "Must specify namespaces. To watch all namespaces, use only '" + + Constants.WATCH_ALL_NAMESPACES + + "'. To watch only the namespace in which the operator is deployed, use only '" + + Constants.WATCH_CURRENT_NAMESPACE + "'"); } /** diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/Constants.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/Constants.java index 285bcb16cb..7d7d44ec32 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/Constants.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/Constants.java @@ -4,6 +4,7 @@ public final class Constants { public static final String NO_VALUE_SET = ""; public static final String WATCH_CURRENT_NAMESPACE = "JOSDK_WATCH_CURRENT"; + public static final String WATCH_ALL_NAMESPACES = "JOSDK_ALL_NAMESPACES"; public static final long NO_RECONCILIATION_MAX_INTERVAL = -1L; private Constants() {} diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ControllerConfiguration.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ControllerConfiguration.java index 3bd5b93a61..a5e1459cb7 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ControllerConfiguration.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ControllerConfiguration.java @@ -39,7 +39,7 @@ * * @return the list of namespaces this controller monitors */ - String[] namespaces() default {}; + String[] namespaces() default Constants.WATCH_ALL_NAMESPACES; /** * Optional label selector used to identify the set of custom resources the controller will acc diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependent.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependent.java index fbebe34648..8ce04d752e 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependent.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependent.java @@ -11,7 +11,6 @@ @Target({ElementType.TYPE}) public @interface KubernetesDependent { String SAME_AS_PARENT = "JOSDK_SAME_AS_PARENT"; - String WATCH_ALL_NAMESPACES = "JOSDK_ALL_NAMESPACES"; String[] DEFAULT_NAMESPACES = {SAME_AS_PARENT}; diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResourceConfig.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResourceConfig.java index 48a93cb102..5f325bbfcc 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResourceConfig.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResourceConfig.java @@ -37,11 +37,7 @@ public KubernetesDependentResourceConfig setLabelSelector(String labelSelector) } public Set namespaces() { - if (!namespaces.contains(KubernetesDependent.WATCH_ALL_NAMESPACES)) { - return namespaces; - } else { - return Collections.emptySet(); - } + return namespaces; } public String labelSelector() { diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/OperatorTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/OperatorTest.java index f4de6b2141..2a1b48f019 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/OperatorTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/OperatorTest.java @@ -10,20 +10,17 @@ import io.fabric8.kubernetes.client.KubernetesClient; import io.javaoperatorsdk.operator.api.config.AbstractConfigurationService; import io.javaoperatorsdk.operator.api.config.ConfigurationServiceProvider; -import io.javaoperatorsdk.operator.api.config.ControllerConfiguration; +import io.javaoperatorsdk.operator.api.config.MockControllerConfiguration; import io.javaoperatorsdk.operator.api.reconciler.Context; import io.javaoperatorsdk.operator.api.reconciler.Reconciler; import io.javaoperatorsdk.operator.api.reconciler.UpdateControl; import static org.assertj.core.api.AssertionsForClassTypes.assertThat; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; @SuppressWarnings("rawtypes") class OperatorTest { private final KubernetesClient kubernetesClient = MockKubernetesClient.client(ConfigMap.class); - private final ControllerConfiguration configuration = mock(ControllerConfiguration.class); private final Operator operator = new Operator(kubernetesClient); private final FooReconciler fooReconciler = new FooReconciler(); @@ -35,10 +32,9 @@ static void setUpConfigurationServiceProvider() { @Test @DisplayName("should register `Reconciler` to Controller") - @SuppressWarnings("unchecked") public void shouldRegisterReconcilerToController() { // given - when(configuration.getResourceClass()).thenReturn(ConfigMap.class); + final var configuration = MockControllerConfiguration.forResource(ConfigMap.class); // when operator.register(fooReconciler, configuration); diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/ControllerConfigurationOverriderTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/ControllerConfigurationOverriderTest.java index d0bb8916cb..497fafb897 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/ControllerConfigurationOverriderTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/ControllerConfigurationOverriderTest.java @@ -6,6 +6,7 @@ import org.junit.jupiter.api.Test; import io.fabric8.kubernetes.api.model.ConfigMap; +import io.javaoperatorsdk.operator.api.reconciler.Constants; import io.javaoperatorsdk.operator.api.reconciler.Context; import io.javaoperatorsdk.operator.api.reconciler.ControllerConfiguration; import io.javaoperatorsdk.operator.api.reconciler.Reconciler; @@ -107,6 +108,63 @@ private io.javaoperatorsdk.operator.api.config.ControllerConfiguration create return new AnnotationControllerConfiguration<>(reconciler); } + @ControllerConfiguration(namespaces = "foo") + private static class WatchCurrentReconciler implements Reconciler { + + @Override + public UpdateControl reconcile(ConfigMap resource, Context context) + throws Exception { + return null; + } + } + + @Test + void overridingNamespacesShouldWork() { + var configuration = createConfiguration(new WatchCurrentReconciler()); + assertEquals(Set.of("foo"), configuration.getNamespaces()); + assertFalse(configuration.watchAllNamespaces()); + assertFalse(configuration.watchCurrentNamespace()); + + configuration = ControllerConfigurationOverrider.override(configuration) + .addingNamespaces("foo", "bar") + .build(); + assertEquals(Set.of("foo", "bar"), configuration.getNamespaces()); + assertFalse(configuration.watchAllNamespaces()); + assertFalse(configuration.watchCurrentNamespace()); + + configuration = ControllerConfigurationOverrider.override(configuration) + .removingNamespaces("bar") + .build(); + assertEquals(Set.of("foo"), configuration.getNamespaces()); + assertFalse(configuration.watchAllNamespaces()); + assertFalse(configuration.watchCurrentNamespace()); + + configuration = ControllerConfigurationOverrider.override(configuration) + .removingNamespaces("foo") + .build(); + assertTrue(configuration.watchAllNamespaces()); + assertFalse(configuration.watchCurrentNamespace()); + + configuration = ControllerConfigurationOverrider.override(configuration) + .settingNamespace("foo") + .build(); + assertFalse(configuration.watchAllNamespaces()); + assertFalse(configuration.watchCurrentNamespace()); + assertEquals(Set.of("foo"), configuration.getNamespaces()); + + configuration = ControllerConfigurationOverrider.override(configuration) + .watchingOnlyCurrentNamespace() + .build(); + assertFalse(configuration.watchAllNamespaces()); + assertTrue(configuration.watchCurrentNamespace()); + + configuration = ControllerConfigurationOverrider.override(configuration) + .watchingAllNamespaces() + .build(); + assertTrue(configuration.watchAllNamespaces()); + assertFalse(configuration.watchCurrentNamespace()); + } + @Test void configuredDependentShouldNotChangeOnParentOverrideEvenWhenInitialConfigIsSame() { var configuration = createConfiguration(new OverriddenNSOnDepReconciler()); @@ -140,7 +198,7 @@ void dependentShouldWatchAllNamespacesIfParentDoesAsWell() { var config = extractFirstDependentKubernetesResourceConfig(configuration); // check that the DependentResource inherits the controller's configuration if applicable - assertEquals(0, config.namespaces().size()); + assertTrue(ResourceConfiguration.allNamespacesWatched(config.namespaces())); // override the NS final var newNS = "bar"; @@ -160,7 +218,7 @@ void shouldBePossibleToForceDependentToWatchAllNamespaces() { var config = extractFirstDependentKubernetesResourceConfig(configuration); // check that the DependentResource inherits the controller's configuration if applicable - assertEquals(0, config.namespaces().size()); + assertTrue(ResourceConfiguration.allNamespacesWatched(config.namespaces())); // override the NS final var newNS = "bar"; @@ -169,7 +227,7 @@ void shouldBePossibleToForceDependentToWatchAllNamespaces() { // check that dependent config is still configured to watch all NS config = extractFirstDependentKubernetesResourceConfig(configuration); - assertEquals(0, config.namespaces().size()); + assertTrue(ResourceConfiguration.allNamespacesWatched(config.namespaces())); } @Test @@ -224,7 +282,8 @@ void replaceNamedDependentResourceConfigShouldWork() { final var dependentResourceName = DependentResource.defaultNameFor(ReadOnlyDependent.class); assertTrue(dependents.stream().anyMatch(dr -> dr.getName().equals(dependentResourceName))); - var dependentSpec = dependents.stream().filter(dr -> dr.getName().equals(dependentResourceName)) + var dependentSpec = dependents.stream() + .filter(dr -> dr.getName().equals(dependentResourceName)) .findFirst().get(); assertEquals(ReadOnlyDependent.class, dependentSpec.getDependentResourceClass()); var maybeConfig = dependentSpec.getDependentResourceConfiguration(); @@ -292,7 +351,7 @@ public ReadOnlyDependent() { } } - @KubernetesDependent(namespaces = KubernetesDependent.WATCH_ALL_NAMESPACES) + @KubernetesDependent(namespaces = Constants.WATCH_ALL_NAMESPACES) private static class WatchAllNSDependent extends KubernetesDependentResource { diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/MockControllerConfiguration.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/MockControllerConfiguration.java new file mode 100644 index 0000000000..a58e3b5b73 --- /dev/null +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/MockControllerConfiguration.java @@ -0,0 +1,18 @@ +package io.javaoperatorsdk.operator.api.config; + +import io.fabric8.kubernetes.api.model.HasMetadata; + +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +public class MockControllerConfiguration { + @SuppressWarnings({"unchecked", "rawtypes"}) + public static ControllerConfiguration forResource( + Class resourceType) { + final ControllerConfiguration configuration = mock(ControllerConfiguration.class); + when(configuration.getResourceClass()).thenReturn(resourceType); + when(configuration.getNamespaces()).thenReturn(ResourceConfiguration.DEFAULT_NAMESPACES); + when(configuration.getEffectiveNamespaces()).thenCallRealMethod(); + return configuration; + } +} diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/ResourceConfigurationTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/ResourceConfigurationTest.java new file mode 100644 index 0000000000..013eec9cc0 --- /dev/null +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/ResourceConfigurationTest.java @@ -0,0 +1,71 @@ +package io.javaoperatorsdk.operator.api.config; + +import java.util.Collections; +import java.util.Set; + +import org.junit.jupiter.api.Test; + +import io.javaoperatorsdk.operator.api.reconciler.Constants; + +import static org.junit.jupiter.api.Assertions.*; + +class ResourceConfigurationTest { + + @Test + void allNamespacesWatched() { + assertThrows(IllegalArgumentException.class, + () -> ResourceConfiguration.allNamespacesWatched(null)); + assertThrows(IllegalArgumentException.class, () -> ResourceConfiguration.allNamespacesWatched( + Set.of(Constants.WATCH_CURRENT_NAMESPACE, Constants.WATCH_ALL_NAMESPACES, "foo"))); + assertThrows(IllegalArgumentException.class, () -> ResourceConfiguration.allNamespacesWatched( + Collections.emptySet())); + assertFalse(ResourceConfiguration.allNamespacesWatched(Set.of("foo", "bar"))); + assertTrue(ResourceConfiguration.allNamespacesWatched(Set.of(Constants.WATCH_ALL_NAMESPACES))); + assertFalse(ResourceConfiguration.allNamespacesWatched(Set.of("foo"))); + assertFalse( + ResourceConfiguration.allNamespacesWatched(Set.of(Constants.WATCH_CURRENT_NAMESPACE))); + } + + @Test + void currentNamespaceWatched() { + assertThrows(IllegalArgumentException.class, + () -> ResourceConfiguration.currentNamespaceWatched(null)); + assertThrows(IllegalArgumentException.class, + () -> ResourceConfiguration.currentNamespaceWatched( + Set.of(Constants.WATCH_CURRENT_NAMESPACE, Constants.WATCH_ALL_NAMESPACES, "foo"))); + assertThrows(IllegalArgumentException.class, + () -> ResourceConfiguration.currentNamespaceWatched(Collections.emptySet())); + assertFalse(ResourceConfiguration.currentNamespaceWatched(Set.of("foo", "bar"))); + assertFalse( + ResourceConfiguration.currentNamespaceWatched(Set.of(Constants.WATCH_ALL_NAMESPACES))); + assertFalse(ResourceConfiguration.currentNamespaceWatched(Set.of("foo"))); + assertTrue( + ResourceConfiguration.currentNamespaceWatched(Set.of(Constants.WATCH_CURRENT_NAMESPACE))); + } + + @Test + void nullLabelSelectorByDefault() { + assertNull(new ResourceConfiguration<>() {}.getLabelSelector()); + } + + @Test + void shouldWatchAllNamespacesByDefault() { + assertTrue(new ResourceConfiguration<>() {}.watchAllNamespaces()); + } + + @Test + void failIfNotValid() { + assertThrows(IllegalArgumentException.class, () -> ResourceConfiguration.failIfNotValid(null)); + assertThrows(IllegalArgumentException.class, + () -> ResourceConfiguration.failIfNotValid(Collections.emptySet())); + assertThrows(IllegalArgumentException.class, () -> ResourceConfiguration.failIfNotValid( + Set.of(Constants.WATCH_CURRENT_NAMESPACE, Constants.WATCH_ALL_NAMESPACES, "foo"))); + assertThrows(IllegalArgumentException.class, () -> ResourceConfiguration.failIfNotValid( + Set.of(Constants.WATCH_CURRENT_NAMESPACE, "foo"))); + assertThrows(IllegalArgumentException.class, () -> ResourceConfiguration.failIfNotValid( + Set.of(Constants.WATCH_ALL_NAMESPACES, "foo"))); + + // should work + ResourceConfiguration.failIfNotValid(Set.of("foo", "bar")); + } +} diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/ControllerTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/ControllerTest.java index 26fd763467..67e9112b27 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/ControllerTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/ControllerTest.java @@ -5,7 +5,7 @@ import io.fabric8.kubernetes.api.model.Secret; import io.javaoperatorsdk.operator.MockKubernetesClient; import io.javaoperatorsdk.operator.api.config.ConfigurationServiceProvider; -import io.javaoperatorsdk.operator.api.config.ControllerConfiguration; +import io.javaoperatorsdk.operator.api.config.MockControllerConfiguration; import io.javaoperatorsdk.operator.api.reconciler.Cleaner; import io.javaoperatorsdk.operator.api.reconciler.Reconciler; import io.javaoperatorsdk.operator.sample.simple.TestCustomResource; @@ -13,17 +13,15 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Mockito.*; -@SuppressWarnings("unchecked") +@SuppressWarnings({"unchecked", "rawtypes"}) class ControllerTest { - final ControllerConfiguration configuration = mock(ControllerConfiguration.class); final Reconciler reconciler = mock(Reconciler.class); @Test void crdShouldNotBeCheckedForNativeResources() { final var client = MockKubernetesClient.client(Secret.class); - - when(configuration.getResourceClass()).thenReturn(Secret.class); + final var configuration = MockControllerConfiguration.forResource(Secret.class); final var controller = new Controller(reconciler, configuration, client); controller.start(); @@ -33,7 +31,7 @@ void crdShouldNotBeCheckedForNativeResources() { @Test void crdShouldNotBeCheckedForCustomResourcesIfDisabled() { final var client = MockKubernetesClient.client(TestCustomResource.class); - when(configuration.getResourceClass()).thenReturn(TestCustomResource.class); + final var configuration = MockControllerConfiguration.forResource(TestCustomResource.class); try { ConfigurationServiceProvider.overrideCurrent(o -> o.checkingCRDAndValidateLocalModel(false)); @@ -49,10 +47,10 @@ void crdShouldNotBeCheckedForCustomResourcesIfDisabled() { @Test void usesFinalizerIfThereIfReconcilerImplementsCleaner() { Reconciler reconciler = mock(Reconciler.class, withSettings().extraInterfaces(Cleaner.class)); - when(configuration.getResourceClass()).thenReturn(TestCustomResource.class); + final var configuration = MockControllerConfiguration.forResource(Secret.class); - final var controller = new Controller(reconciler, - configuration, MockKubernetesClient.client(TestCustomResource.class)); + final var controller = new Controller(reconciler, configuration, + MockKubernetesClient.client(Secret.class)); assertThat(controller.useFinalizer()).isTrue(); } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventSourceManagerTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventSourceManagerTest.java index 42a9a75447..f09605d095 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventSourceManagerTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventSourceManagerTest.java @@ -7,7 +7,7 @@ import io.fabric8.kubernetes.api.model.HasMetadata; import io.javaoperatorsdk.operator.MockKubernetesClient; import io.javaoperatorsdk.operator.OperatorException; -import io.javaoperatorsdk.operator.api.config.ControllerConfiguration; +import io.javaoperatorsdk.operator.api.config.MockControllerConfiguration; import io.javaoperatorsdk.operator.api.reconciler.Reconciler; import io.javaoperatorsdk.operator.processing.Controller; import io.javaoperatorsdk.operator.processing.event.source.CachingEventSource; @@ -140,8 +140,7 @@ void retrievingAnEventSourceWhenMultipleAreRegisteredForATypeShouldRequireAQuali } private EventSourceManager initManager() { - final ControllerConfiguration configuration = mock(ControllerConfiguration.class); - when(configuration.getResourceClass()).thenReturn(HasMetadata.class); + final var configuration = MockControllerConfiguration.forResource(HasMetadata.class); final Controller controller = new Controller(mock(Reconciler.class), configuration, MockKubernetesClient.client(HasMetadata.class)); return new EventSourceManager(controller); diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java index d1b6883075..6afec04543 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java @@ -21,6 +21,7 @@ import io.javaoperatorsdk.operator.api.config.Cloner; import io.javaoperatorsdk.operator.api.config.ConfigurationServiceProvider; import io.javaoperatorsdk.operator.api.config.ControllerConfiguration; +import io.javaoperatorsdk.operator.api.config.MockControllerConfiguration; import io.javaoperatorsdk.operator.api.config.RetryConfiguration; import io.javaoperatorsdk.operator.api.reconciler.*; import io.javaoperatorsdk.operator.processing.Controller; @@ -64,15 +65,13 @@ static void classSetup() { * equals will fail on the two equal but NOT identical TestCustomResources because equals is not * implemented on TestCustomResourceSpec or TestCustomResourceStatus */ - ConfigurationServiceProvider.overrideCurrent(overrider -> { - overrider.checkingCRDAndValidateLocalModel(false) - .withResourceCloner(new Cloner() { - @Override - public R clone(R object) { - return object; - } - }); - }); + ConfigurationServiceProvider.overrideCurrent(overrider -> overrider + .checkingCRDAndValidateLocalModel(false).withResourceCloner(new Cloner() { + @Override + public R clone(R object) { + return object; + } + })); } @AfterAll @@ -92,17 +91,19 @@ private ReconciliationDispatcher init(R customResourc Reconciler reconciler, ControllerConfiguration configuration, CustomResourceFacade customResourceFacade, boolean useFinalizer) { - configuration = configuration == null ? mock(ControllerConfiguration.class) : configuration; + final Class resourceClass = (Class) customResource.getClass(); + configuration = configuration == null ? MockControllerConfiguration.forResource(resourceClass) + : configuration; when(configuration.getFinalizerName()).thenReturn(DEFAULT_FINALIZER); when(configuration.getName()).thenReturn("EventDispatcherTestController"); - when(configuration.getResourceClass()).thenReturn((Class) customResource.getClass()); + when(configuration.getResourceClass()).thenReturn(resourceClass); when(configuration.getRetryConfiguration()).thenReturn(RetryConfiguration.DEFAULT); when(configuration.reconciliationMaxInterval()) .thenReturn(Optional.of(Duration.ofHours(RECONCILIATION_MAX_INTERVAL))); Controller controller = new Controller<>(reconciler, configuration, - MockKubernetesClient.client(customResource.getClass())) { + MockKubernetesClient.client(resourceClass)) { @Override public boolean useFinalizer() { return useFinalizer; @@ -114,7 +115,7 @@ public boolean useFinalizer() { } @Test - void addFinalizerOnNewResource() throws Exception { + void addFinalizerOnNewResource() { assertFalse(testCustomResource.hasFinalizer(DEFAULT_FINALIZER)); reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); verify(reconciler, never()) @@ -126,7 +127,7 @@ void addFinalizerOnNewResource() throws Exception { } @Test - void callCreateOrUpdateOnNewResourceIfFinalizerSet() throws Exception { + void callCreateOrUpdateOnNewResourceIfFinalizerSet() { testCustomResource.addFinalizer(DEFAULT_FINALIZER); reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); verify(reconciler, times(1)) @@ -134,7 +135,7 @@ void callCreateOrUpdateOnNewResourceIfFinalizerSet() throws Exception { } @Test - void updatesOnlyStatusSubResourceIfFinalizerSet() throws Exception { + void updatesOnlyStatusSubResourceIfFinalizerSet() { testCustomResource.addFinalizer(DEFAULT_FINALIZER); reconciler.reconcile = (r, c) -> UpdateControl.updateStatus(testCustomResource); @@ -146,7 +147,7 @@ void updatesOnlyStatusSubResourceIfFinalizerSet() throws Exception { } @Test - void updatesBothResourceAndStatusIfFinalizerSet() throws Exception { + void updatesBothResourceAndStatusIfFinalizerSet() { testCustomResource.addFinalizer(DEFAULT_FINALIZER); reconciler.reconcile = (r, c) -> UpdateControl.updateResourceAndStatus(testCustomResource); @@ -187,7 +188,7 @@ void patchesStatus() { } @Test - void callCreateOrUpdateOnModifiedResourceIfFinalizerSet() throws Exception { + void callCreateOrUpdateOnModifiedResourceIfFinalizerSet() { testCustomResource.addFinalizer(DEFAULT_FINALIZER); reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); @@ -261,7 +262,7 @@ void doesNotRemovesTheSetFinalizerIfTheDeleteNotMethodInstructsIt() { } @Test - void doesNotUpdateTheResourceIfNoUpdateUpdateControlIfFinalizerSet() throws Exception { + void doesNotUpdateTheResourceIfNoUpdateUpdateControlIfFinalizerSet() { testCustomResource.addFinalizer(DEFAULT_FINALIZER); reconciler.reconcile = (r, c) -> UpdateControl.noUpdate(); @@ -272,7 +273,7 @@ void doesNotUpdateTheResourceIfNoUpdateUpdateControlIfFinalizerSet() throws Exce } @Test - void addsFinalizerIfNotMarkedForDeletionAndEmptyCustomResourceReturned() throws Exception { + void addsFinalizerIfNotMarkedForDeletionAndEmptyCustomResourceReturned() { removeFinalizers(testCustomResource); reconciler.reconcile = (r, c) -> UpdateControl.noUpdate(); @@ -295,8 +296,7 @@ void doesNotCallDeleteIfMarkedForDeletionButNotOurFinalizer() { } @Test - void executeControllerRegardlessGenerationInNonGenerationAwareModeIfFinalizerSet() - throws Exception { + void executeControllerRegardlessGenerationInNonGenerationAwareModeIfFinalizerSet() { testCustomResource.addFinalizer(DEFAULT_FINALIZER); reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); @@ -305,7 +305,7 @@ void executeControllerRegardlessGenerationInNonGenerationAwareModeIfFinalizerSet } @Test - void propagatesRetryInfoToContextIfFinalizerSet() throws Exception { + void propagatesRetryInfoToContextIfFinalizerSet() { testCustomResource.addFinalizer(DEFAULT_FINALIZER); reconciliationDispatcher.handleExecution( @@ -334,7 +334,7 @@ public boolean isLastAttempt() { } @Test - void setReScheduleToPostExecutionControlFromUpdateControl() throws Exception { + void setReScheduleToPostExecutionControlFromUpdateControl() { testCustomResource.addFinalizer(DEFAULT_FINALIZER); reconciler.reconcile = @@ -368,7 +368,7 @@ void setObservedGenerationForStatusIfNeeded() throws Exception { Reconciler reconciler = mock(Reconciler.class); ControllerConfiguration config = - mock(ControllerConfiguration.class); + MockControllerConfiguration.forResource(ObservedGenCustomResource.class); CustomResourceFacade facade = mock(CustomResourceFacade.class); var dispatcher = init(observedGenResource, reconciler, config, facade, true); @@ -389,8 +389,7 @@ void updatesObservedGenerationOnNoUpdateUpdateControl() throws Exception { var observedGenResource = createObservedGenCustomResource(); Reconciler reconciler = mock(Reconciler.class); - ControllerConfiguration config = - mock(ControllerConfiguration.class); + final var config = MockControllerConfiguration.forResource(ObservedGenCustomResource.class); CustomResourceFacade facade = mock(CustomResourceFacade.class); when(config.isGenerationAware()).thenReturn(true); when(reconciler.reconcile(any(), any())) @@ -410,8 +409,7 @@ void updateObservedGenerationOnCustomResourceUpdate() throws Exception { var observedGenResource = createObservedGenCustomResource(); Reconciler reconciler = mock(Reconciler.class); - ControllerConfiguration config = - mock(ControllerConfiguration.class); + final var config = MockControllerConfiguration.forResource(ObservedGenCustomResource.class); CustomResourceFacade facade = mock(CustomResourceFacade.class); when(config.isGenerationAware()).thenReturn(true); when(reconciler.reconcile(any(), any())) @@ -428,7 +426,7 @@ void updateObservedGenerationOnCustomResourceUpdate() throws Exception { } @Test - void callErrorStatusHandlerIfImplemented() throws Exception { + void callErrorStatusHandlerIfImplemented() { testCustomResource.addFinalizer(DEFAULT_FINALIZER); reconciler.reconcile = (r, c) -> { @@ -460,7 +458,7 @@ public boolean isLastAttempt() { } @Test - void callErrorStatusHandlerEvenOnFirstError() throws Exception { + void callErrorStatusHandlerEvenOnFirstError() { testCustomResource.addFinalizer(DEFAULT_FINALIZER); reconciler.reconcile = (r, c) -> { @@ -523,7 +521,7 @@ void errorHandlerCanInstructNoRetryNoUpdate() { } @Test - void schedulesReconciliationIfMaxDelayIsSet() throws Exception { + void schedulesReconciliationIfMaxDelayIsSet() { testCustomResource.addFinalizer(DEFAULT_FINALIZER); reconciler.reconcile = (r, c) -> UpdateControl.noUpdate(); @@ -536,7 +534,7 @@ void schedulesReconciliationIfMaxDelayIsSet() throws Exception { } @Test - void canSkipSchedulingMaxDelayIf() throws Exception { + void canSkipSchedulingMaxDelayIf() { testCustomResource.addFinalizer(DEFAULT_FINALIZER); reconciler.reconcile = (r, c) -> UpdateControl.noUpdate(); @@ -573,6 +571,7 @@ public ExecutionScope executionScopeWithCREvent(T res private class TestReconciler implements Reconciler, Cleaner, ErrorStatusHandler { + private BiFunction> reconcile; private BiFunction cleanup; private ErrorStatusHandler errorHandler; diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java index 7ac6779523..8563fe4d01 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSourceTest.java @@ -14,6 +14,7 @@ import io.fabric8.kubernetes.client.dsl.MixedOperation; import io.fabric8.kubernetes.client.informers.SharedIndexInformer; import io.fabric8.kubernetes.client.informers.cache.Indexer; +import io.javaoperatorsdk.operator.api.config.ResourceConfiguration; import io.javaoperatorsdk.operator.api.config.informer.InformerConfiguration; import io.javaoperatorsdk.operator.processing.event.EventHandler; import io.javaoperatorsdk.operator.processing.event.ResourceID; @@ -23,6 +24,7 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.*; +@SuppressWarnings({"rawtypes", "unchecked"}) class InformerEventSourceTest { private static final String PREV_RESOURCE_VERSION = "0"; @@ -30,16 +32,17 @@ class InformerEventSourceTest { private static final String NEXT_RESOURCE_VERSION = "2"; private InformerEventSource informerEventSource; - private KubernetesClient clientMock = mock(KubernetesClient.class); - private TemporaryResourceCache temporaryResourceCacheMock = + private final KubernetesClient clientMock = mock(KubernetesClient.class); + private final TemporaryResourceCache temporaryResourceCacheMock = mock(TemporaryResourceCache.class); - private EventHandler eventHandlerMock = mock(EventHandler.class); - private MixedOperation crClientMock = mock(MixedOperation.class); - private FilterWatchListMultiDeletable specificResourceClientMock = + private final EventHandler eventHandlerMock = mock(EventHandler.class); + private final MixedOperation crClientMock = mock(MixedOperation.class); + private final FilterWatchListMultiDeletable specificResourceClientMock = mock(FilterWatchListMultiDeletable.class); - private FilterWatchListDeletable labeledResourceClientMock = mock(FilterWatchListDeletable.class); - private SharedIndexInformer informer = mock(SharedIndexInformer.class); - private InformerConfiguration informerConfiguration = + private final FilterWatchListDeletable labeledResourceClientMock = + mock(FilterWatchListDeletable.class); + private final SharedIndexInformer informer = mock(SharedIndexInformer.class); + private final InformerConfiguration informerConfiguration = mock(InformerConfiguration.class); @BeforeEach @@ -51,6 +54,8 @@ void setup() { when(labeledResourceClientMock.runnableInformer(0)).thenReturn(informer); when(informer.getIndexer()).thenReturn(mock(Indexer.class)); + when(informerConfiguration.getEffectiveNamespaces()) + .thenReturn(ResourceConfiguration.DEFAULT_NAMESPACES); when(informerConfiguration.getSecondaryToPrimaryMapper()) .thenReturn(mock(SecondaryToPrimaryMapper.class)); From accae2e7864baf771184b67a7bce1d2f98b6655d Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Fri, 22 Apr 2022 15:50:29 +0200 Subject: [PATCH 2/2] refactor: DEPLOYED_NAMESPACE_ONLY -> CURRENT_NAMESPACE_ONLY --- .../operator/api/config/ControllerConfigurationOverrider.java | 2 +- .../operator/api/config/ResourceConfiguration.java | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfigurationOverrider.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfigurationOverrider.java index c098a53884..f045a4a2e3 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfigurationOverrider.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfigurationOverrider.java @@ -53,7 +53,7 @@ public ControllerConfigurationOverrider withGenerationAware(boolean generatio } public ControllerConfigurationOverrider watchingOnlyCurrentNamespace() { - this.namespaces = ResourceConfiguration.DEPLOYED_NAMESPACE_ONLY; + this.namespaces = ResourceConfiguration.CURRENT_NAMESPACE_ONLY; return this; } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ResourceConfiguration.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ResourceConfiguration.java index 50dbfeb3e1..fe85476735 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ResourceConfiguration.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ResourceConfiguration.java @@ -11,7 +11,7 @@ public interface ResourceConfiguration { Set DEFAULT_NAMESPACES = Set.of(Constants.WATCH_ALL_NAMESPACES); - Set DEPLOYED_NAMESPACE_ONLY = Set.of(Constants.WATCH_CURRENT_NAMESPACE); + Set CURRENT_NAMESPACE_ONLY = Set.of(Constants.WATCH_CURRENT_NAMESPACE); default String getResourceTypeName() { return ReconcilerUtils.getResourceTypeName(getResourceClass()); @@ -53,7 +53,7 @@ default boolean watchCurrentNamespace() { static boolean currentNamespaceWatched(Set namespaces) { failIfNotValid(namespaces); - return DEPLOYED_NAMESPACE_ONLY.equals(namespaces); + return CURRENT_NAMESPACE_ONLY.equals(namespaces); } static void failIfNotValid(Set namespaces) {