Skip to content

Commit dca90ed

Browse files
metacosmmetacosm
authored
feat: make default watched namespaces behavior explicit (#1177)
* 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 * refactor: DEPLOYED_NAMESPACE_ONLY -> CURRENT_NAMESPACE_ONLY
1 parent d9ebace commit dca90ed

16 files changed

+256
-92
lines changed

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/AnnotationControllerConfiguration.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,8 @@ public boolean isGenerationAware() {
6666

6767
@Override
6868
public Set<String> getNamespaces() {
69-
return Set.of(valueOrDefault(annotation, ControllerConfiguration::namespaces, new String[] {}));
69+
return Set.of(valueOrDefault(annotation, ControllerConfiguration::namespaces,
70+
new String[] {Constants.WATCH_ALL_NAMESPACES}));
7071
}
7172

7273
@Override

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfigurationOverrider.java‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ public class ControllerConfigurationOverrider<R extends HasMetadata> {
1919

2020
private String finalizer;
2121
private boolean generationAware;
22-
private final Set<String> namespaces;
22+
private Set<String> namespaces;
2323
private RetryConfiguration retry;
2424
private String labelSelector;
2525
private ResourceEventFilter<R> customResourcePredicate;
@@ -52,8 +52,8 @@ public ControllerConfigurationOverrider<R> withGenerationAware(boolean generatio
5252
return this;
5353
}
5454

55-
public ControllerConfigurationOverrider<R> withCurrentNamespace() {
56-
namespaces.clear();
55+
public ControllerConfigurationOverrider<R> watchingOnlyCurrentNamespace() {
56+
this.namespaces = ResourceConfiguration.CURRENT_NAMESPACE_ONLY;
5757
return this;
5858
}
5959

@@ -64,6 +64,9 @@ public ControllerConfigurationOverrider<R> addingNamespaces(String... namespaces
6464

6565
public ControllerConfigurationOverrider<R> removingNamespaces(String... namespaces) {
6666
List.of(namespaces).forEach(this.namespaces::remove);
67+
if (this.namespaces.isEmpty()) {
68+
this.namespaces = ResourceConfiguration.DEFAULT_NAMESPACES;
69+
}
6770
return this;
6871
}
6972

@@ -73,6 +76,11 @@ public ControllerConfigurationOverrider<R> settingNamespace(String namespace) {
7376
return this;
7477
}
7578

79+
public ControllerConfigurationOverrider<R> watchingAllNamespaces() {
80+
this.namespaces = ResourceConfiguration.DEFAULT_NAMESPACES;
81+
return this;
82+
}
83+
7684
public ControllerConfigurationOverrider<R> withRetry(RetryConfiguration retry) {
7785
this.retry = retry;
7886
return this;

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/DefaultResourceConfiguration.java‎

Lines changed: 5 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
package io.javaoperatorsdk.operator.api.config;
22

3-
import java.util.Collections;
43
import java.util.Set;
54

65
import io.fabric8.kubernetes.api.model.HasMetadata;
@@ -10,21 +9,22 @@ public class DefaultResourceConfiguration<R extends HasMetadata>
109

1110
private final String labelSelector;
1211
private final Set<String> namespaces;
13-
private final boolean watchAllNamespaces;
1412
private final Class<R> resourceClass;
1513

1614
public DefaultResourceConfiguration(String labelSelector, Class<R> resourceClass,
1715
String... namespaces) {
1816
this(labelSelector, resourceClass,
19-
namespaces != null ? Set.of(namespaces) : Collections.emptySet());
17+
namespaces == null || namespaces.length == 0 ? ResourceConfiguration.DEFAULT_NAMESPACES
18+
: Set.of(namespaces));
2019
}
2120

2221
public DefaultResourceConfiguration(String labelSelector, Class<R> resourceClass,
2322
Set<String> namespaces) {
2423
this.labelSelector = labelSelector;
2524
this.resourceClass = resourceClass;
26-
this.namespaces = namespaces != null ? namespaces : Collections.emptySet();
27-
this.watchAllNamespaces = this.namespaces.isEmpty();
25+
this.namespaces =
26+
namespaces == null || namespaces.isEmpty() ? ResourceConfiguration.DEFAULT_NAMESPACES
27+
: namespaces;
2828
}
2929

3030
@Override
@@ -42,11 +42,6 @@ public Set<String> getNamespaces() {
4242
return namespaces;
4343
}
4444

45-
@Override
46-
public boolean watchAllNamespaces() {
47-
return watchAllNamespaces;
48-
}
49-
5045
@Override
5146
public Class<R> getResourceClass() {
5247
return resourceClass;

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ResourceConfiguration.java‎

Lines changed: 27 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -10,15 +10,18 @@
1010

1111
public interface ResourceConfiguration<R extends HasMetadata> {
1212

13+
Set<String> DEFAULT_NAMESPACES = Set.of(Constants.WATCH_ALL_NAMESPACES);
14+
Set<String> CURRENT_NAMESPACE_ONLY = Set.of(Constants.WATCH_CURRENT_NAMESPACE);
15+
1316
default String getResourceTypeName() {
1417
return ReconcilerUtils.getResourceTypeName(getResourceClass());
1518
}
1619

1720
/**
1821
* Retrieves the label selector that is used to filter which resources are actually watched by the
19-
* associated event source. See
20-
* https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/ for more details on
21-
* syntax.
22+
* associated event source. See the official documentation on the
23+
* <a href="https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/">topic</a>
24+
* for more details on syntax.
2225
*
2326
* @return the label selector filtering watched resources
2427
*/
@@ -32,25 +35,41 @@ default Class<R> getResourceClass() {
3235
}
3336

3437
default Set<String> getNamespaces() {
35-
return Collections.emptySet();
38+
return DEFAULT_NAMESPACES;
3639
}
3740

3841
default boolean watchAllNamespaces() {
3942
return allNamespacesWatched(getNamespaces());
4043
}
4144

4245
static boolean allNamespacesWatched(Set<String> namespaces) {
43-
return namespaces == null || namespaces.isEmpty();
46+
failIfNotValid(namespaces);
47+
return DEFAULT_NAMESPACES.equals(namespaces);
4448
}
4549

4650
default boolean watchCurrentNamespace() {
4751
return currentNamespaceWatched(getNamespaces());
4852
}
4953

5054
static boolean currentNamespaceWatched(Set<String> namespaces) {
51-
return namespaces != null
52-
&& namespaces.size() == 1
53-
&& namespaces.contains(Constants.WATCH_CURRENT_NAMESPACE);
55+
failIfNotValid(namespaces);
56+
return CURRENT_NAMESPACE_ONLY.equals(namespaces);
57+
}
58+
59+
static void failIfNotValid(Set<String> namespaces) {
60+
if (namespaces != null && !namespaces.isEmpty()) {
61+
final var present = namespaces.contains(Constants.WATCH_CURRENT_NAMESPACE)
62+
|| namespaces.contains(Constants.WATCH_ALL_NAMESPACES);
63+
if (!present || namespaces.size() == 1) {
64+
return;
65+
}
66+
}
67+
68+
throw new IllegalArgumentException(
69+
"Must specify namespaces. To watch all namespaces, use only '"
70+
+ Constants.WATCH_ALL_NAMESPACES
71+
+ "'. To watch only the namespace in which the operator is deployed, use only '"
72+
+ Constants.WATCH_CURRENT_NAMESPACE + "'");
5473
}
5574

5675
/**

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/Constants.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ public final class Constants {
44

55
public static final String NO_VALUE_SET = "";
66
public static final String WATCH_CURRENT_NAMESPACE = "JOSDK_WATCH_CURRENT";
7+
public static final String WATCH_ALL_NAMESPACES = "JOSDK_ALL_NAMESPACES";
78
public static final long NO_RECONCILIATION_MAX_INTERVAL = -1L;
89

910
private Constants() {}

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ControllerConfiguration.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@
3939
*
4040
* @return the list of namespaces this controller monitors
4141
*/
42-
String[] namespaces() default {};
42+
String[] namespaces() default Constants.WATCH_ALL_NAMESPACES;
4343

4444
/**
4545
* Optional label selector used to identify the set of custom resources the controller will acc

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependent.java‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@
1111
@Target({ElementType.TYPE})
1212
public @interface KubernetesDependent {
1313
String SAME_AS_PARENT = "JOSDK_SAME_AS_PARENT";
14-
String WATCH_ALL_NAMESPACES = "JOSDK_ALL_NAMESPACES";
1514

1615
String[] DEFAULT_NAMESPACES = {SAME_AS_PARENT};
1716

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResourceConfig.java‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -37,11 +37,7 @@ public KubernetesDependentResourceConfig setLabelSelector(String labelSelector)
3737
}
3838

3939
public Set<String> namespaces() {
40-
if (!namespaces.contains(KubernetesDependent.WATCH_ALL_NAMESPACES)) {
41-
return namespaces;
42-
} else {
43-
return Collections.emptySet();
44-
}
40+
return namespaces;
4541
}
4642

4743
public String labelSelector() {

‎operator-framework-core/src/test/java/io/javaoperatorsdk/operator/OperatorTest.java‎

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,20 +10,17 @@
1010
import io.fabric8.kubernetes.client.KubernetesClient;
1111
import io.javaoperatorsdk.operator.api.config.AbstractConfigurationService;
1212
import io.javaoperatorsdk.operator.api.config.ConfigurationServiceProvider;
13-
import io.javaoperatorsdk.operator.api.config.ControllerConfiguration;
13+
import io.javaoperatorsdk.operator.api.config.MockControllerConfiguration;
1414
import io.javaoperatorsdk.operator.api.reconciler.Context;
1515
import io.javaoperatorsdk.operator.api.reconciler.Reconciler;
1616
import io.javaoperatorsdk.operator.api.reconciler.UpdateControl;
1717

1818
import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
19-
import static org.mockito.Mockito.mock;
20-
import static org.mockito.Mockito.when;
2119

2220
@SuppressWarnings("rawtypes")
2321
class OperatorTest {
2422

2523
private final KubernetesClient kubernetesClient = MockKubernetesClient.client(ConfigMap.class);
26-
private final ControllerConfiguration configuration = mock(ControllerConfiguration.class);
2724
private final Operator operator = new Operator(kubernetesClient);
2825
private final FooReconciler fooReconciler = new FooReconciler();
2926

@@ -35,10 +32,9 @@ static void setUpConfigurationServiceProvider() {
3532

3633
@Test
3734
@DisplayName("should register `Reconciler` to Controller")
38-
@SuppressWarnings("unchecked")
3935
public void shouldRegisterReconcilerToController() {
4036
// given
41-
when(configuration.getResourceClass()).thenReturn(ConfigMap.class);
37+
final var configuration = MockControllerConfiguration.forResource(ConfigMap.class);
4238

4339
// when
4440
operator.register(fooReconciler, configuration);

‎operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/ControllerConfigurationOverriderTest.java‎

Lines changed: 64 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
import org.junit.jupiter.api.Test;
77

88
import io.fabric8.kubernetes.api.model.ConfigMap;
9+
import io.javaoperatorsdk.operator.api.reconciler.Constants;
910
import io.javaoperatorsdk.operator.api.reconciler.Context;
1011
import io.javaoperatorsdk.operator.api.reconciler.ControllerConfiguration;
1112
import io.javaoperatorsdk.operator.api.reconciler.Reconciler;
@@ -107,6 +108,63 @@ private io.javaoperatorsdk.operator.api.config.ControllerConfiguration<?> create
107108
return new AnnotationControllerConfiguration<>(reconciler);
108109
}
109110

111+
@ControllerConfiguration(namespaces = "foo")
112+
private static class WatchCurrentReconciler implements Reconciler<ConfigMap> {
113+
114+
@Override
115+
public UpdateControl<ConfigMap> reconcile(ConfigMap resource, Context<ConfigMap> context)
116+
throws Exception {
117+
return null;
118+
}
119+
}
120+
121+
@Test
122+
void overridingNamespacesShouldWork() {
123+
var configuration = createConfiguration(new WatchCurrentReconciler());
124+
assertEquals(Set.of("foo"), configuration.getNamespaces());
125+
assertFalse(configuration.watchAllNamespaces());
126+
assertFalse(configuration.watchCurrentNamespace());
127+
128+
configuration = ControllerConfigurationOverrider.override(configuration)
129+
.addingNamespaces("foo", "bar")
130+
.build();
131+
assertEquals(Set.of("foo", "bar"), configuration.getNamespaces());
132+
assertFalse(configuration.watchAllNamespaces());
133+
assertFalse(configuration.watchCurrentNamespace());
134+
135+
configuration = ControllerConfigurationOverrider.override(configuration)
136+
.removingNamespaces("bar")
137+
.build();
138+
assertEquals(Set.of("foo"), configuration.getNamespaces());
139+
assertFalse(configuration.watchAllNamespaces());
140+
assertFalse(configuration.watchCurrentNamespace());
141+
142+
configuration = ControllerConfigurationOverrider.override(configuration)
143+
.removingNamespaces("foo")
144+
.build();
145+
assertTrue(configuration.watchAllNamespaces());
146+
assertFalse(configuration.watchCurrentNamespace());
147+
148+
configuration = ControllerConfigurationOverrider.override(configuration)
149+
.settingNamespace("foo")
150+
.build();
151+
assertFalse(configuration.watchAllNamespaces());
152+
assertFalse(configuration.watchCurrentNamespace());
153+
assertEquals(Set.of("foo"), configuration.getNamespaces());
154+
155+
configuration = ControllerConfigurationOverrider.override(configuration)
156+
.watchingOnlyCurrentNamespace()
157+
.build();
158+
assertFalse(configuration.watchAllNamespaces());
159+
assertTrue(configuration.watchCurrentNamespace());
160+
161+
configuration = ControllerConfigurationOverrider.override(configuration)
162+
.watchingAllNamespaces()
163+
.build();
164+
assertTrue(configuration.watchAllNamespaces());
165+
assertFalse(configuration.watchCurrentNamespace());
166+
}
167+
110168
@Test
111169
void configuredDependentShouldNotChangeOnParentOverrideEvenWhenInitialConfigIsSame() {
112170
var configuration = createConfiguration(new OverriddenNSOnDepReconciler());
@@ -140,7 +198,7 @@ void dependentShouldWatchAllNamespacesIfParentDoesAsWell() {
140198
var config = extractFirstDependentKubernetesResourceConfig(configuration);
141199

142200
// check that the DependentResource inherits the controller's configuration if applicable
143-
assertEquals(0, config.namespaces().size());
201+
assertTrue(ResourceConfiguration.allNamespacesWatched(config.namespaces()));
144202

145203
// override the NS
146204
final var newNS = "bar";
@@ -160,7 +218,7 @@ void shouldBePossibleToForceDependentToWatchAllNamespaces() {
160218
var config = extractFirstDependentKubernetesResourceConfig(configuration);
161219

162220
// check that the DependentResource inherits the controller's configuration if applicable
163-
assertEquals(0, config.namespaces().size());
221+
assertTrue(ResourceConfiguration.allNamespacesWatched(config.namespaces()));
164222

165223
// override the NS
166224
final var newNS = "bar";
@@ -169,7 +227,7 @@ void shouldBePossibleToForceDependentToWatchAllNamespaces() {
169227

170228
// check that dependent config is still configured to watch all NS
171229
config = extractFirstDependentKubernetesResourceConfig(configuration);
172-
assertEquals(0, config.namespaces().size());
230+
assertTrue(ResourceConfiguration.allNamespacesWatched(config.namespaces()));
173231
}
174232

175233
@Test
@@ -224,7 +282,8 @@ void replaceNamedDependentResourceConfigShouldWork() {
224282
final var dependentResourceName = DependentResource.defaultNameFor(ReadOnlyDependent.class);
225283
assertTrue(dependents.stream().anyMatch(dr -> dr.getName().equals(dependentResourceName)));
226284

227-
var dependentSpec = dependents.stream().filter(dr -> dr.getName().equals(dependentResourceName))
285+
var dependentSpec = dependents.stream()
286+
.filter(dr -> dr.getName().equals(dependentResourceName))
228287
.findFirst().get();
229288
assertEquals(ReadOnlyDependent.class, dependentSpec.getDependentResourceClass());
230289
var maybeConfig = dependentSpec.getDependentResourceConfiguration();
@@ -292,7 +351,7 @@ public ReadOnlyDependent() {
292351
}
293352
}
294353

295-
@KubernetesDependent(namespaces = KubernetesDependent.WATCH_ALL_NAMESPACES)
354+
@KubernetesDependent(namespaces = Constants.WATCH_ALL_NAMESPACES)
296355
private static class WatchAllNSDependent
297356
extends KubernetesDependentResource<ConfigMap, ConfigMap> {
298357

0 commit comments

Comments
 (0)