Skip to content

Commit bb8a2e1

Browse files
csviricsviri
committed
feat: add missing default-filters and comparable-resource-versions config keys
`withDefaultFilters(boolean)` had no binding in ConfigLoader, and `comparableResourceVersions` existed on the informer config builder but had neither an overrider method nor a config key, making the informer options asymmetric. - add `josdk.controller.<name>.default-filters` - add `ControllerConfigurationOverrider#withComparableResourceVersions` and `josdk.controller.<name>.informer.comparable-resource-versions` The binding coverage tests did not catch the missing `default-filters` key because they matched bindings to setters by parameter type only, so any `Boolean` binding made every `boolean` setter look covered. They now use an explicit setter-name to key mapping, which fails when a scalar setter is added without a key, and check that every mapped key is actually looked up.
1 parent 5bde0f5 commit bb8a2e1

4 files changed

Lines changed: 193 additions & 64 deletions

File tree

‎docs/content/en/docs/documentation/operations/configuration.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -350,6 +350,7 @@ All controller-level keys are prefixed with `josdk.controller.<controller-name>.
350350
| `josdk.controller.<name>.max-reconciliation-interval` | `Duration` | Maximum interval between reconciliations even without events |
351351
| `josdk.controller.<name>.field-manager` | `String` | Field manager name used for SSA operations |
352352
| `josdk.controller.<name>.trigger-reconciler-on-all-events` | `Boolean` | Trigger reconciliation on every event, not only meaningful changes |
353+
| `josdk.controller.<name>.default-filters` | `Boolean` | When `false`, JOSDK's internal update filters (generation-aware, finalizer-needed, marked-for-deletion) are not applied and the user's `onUpdateFilter` becomes the sole filter |
353354

354355
#### Watched Namespaces
355356

@@ -383,6 +384,7 @@ josdk.controller.mycontroller.namespaces=team-a,team-b
383384
| `josdk.controller.<name>.informer.label-selector` | `String` | Label selector for the primary resource informer (alias for `label-selector`) |
384385
| `josdk.controller.<name>.informer.shard-selector` | `String` | Shard selector for the primary resource informer (alias for `shard-selector`) |
385386
| `josdk.controller.<name>.informer.list-limit` | `Long` | Page size for paginated informer list requests; omit for no pagination |
387+
| `josdk.controller.<name>.informer.comparable-resource-versions` | `Boolean` | Whether the resource versions of the primary resource can be treated as integers and thus compared |
386388

387389
#### Retry
388390

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -198,6 +198,18 @@ public ControllerConfigurationOverrider<R> withDefaultFilters(boolean defaultFil
198198
return this;
199199
}
200200

201+
/**
202+
* Sets whether the resource versions of the watched primary resource can be considered integers,
203+
* and thus compared to each other.
204+
*
205+
* @see io.javaoperatorsdk.operator.api.config.informer.Informer#comparableResourceVersions()
206+
*/
207+
public ControllerConfigurationOverrider<R> withComparableResourceVersions(
208+
boolean comparableResourceVersions) {
209+
config.withComparableResourceVersions(comparableResourceVersions);
210+
return this;
211+
}
212+
201213
/**
202214
* Sets a max page size limit when starting the informer. This will result in pagination while
203215
* populating the cache. This means that longer lists will take multiple requests to fetch. See

‎operator-framework/src/main/java/io/javaoperatorsdk/operator/config/loader/ConfigLoader.java‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -171,6 +171,10 @@ public static ConfigLoader getDefault() {
171171
"trigger-reconciler-on-all-events",
172172
Boolean.class,
173173
ControllerConfigurationOverrider::withTriggerReconcilerOnAllEvents),
174+
new ConfigBinding<>(
175+
"default-filters",
176+
Boolean.class,
177+
ControllerConfigurationOverrider::withDefaultFilters),
174178
new ConfigBinding<>(
175179
"informer.label-selector",
176180
String.class,
@@ -182,7 +186,11 @@ public static ConfigLoader getDefault() {
182186
new ConfigBinding<>(
183187
"informer.list-limit",
184188
Long.class,
185-
ControllerConfigurationOverrider::withInformerListLimit));
189+
ControllerConfigurationOverrider::withInformerListLimit),
190+
new ConfigBinding<>(
191+
"informer.comparable-resource-versions",
192+
Boolean.class,
193+
ControllerConfigurationOverrider::withComparableResourceVersions));
186194

187195
private final ConfigProvider configProvider;
188196

‎operator-framework/src/test/java/io/javaoperatorsdk/operator/config/loader/ConfigLoaderTest.java‎

Lines changed: 170 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -216,9 +216,11 @@ public <T> Optional<T> getValue(String key, Class<T> type) {
216216
"josdk.controller.ctrl.max-reconciliation-interval",
217217
"josdk.controller.ctrl.field-manager",
218218
"josdk.controller.ctrl.trigger-reconciler-on-all-events",
219+
"josdk.controller.ctrl.default-filters",
219220
"josdk.controller.ctrl.informer.label-selector",
220221
"josdk.controller.ctrl.informer.shard-selector",
221222
"josdk.controller.ctrl.informer.list-limit",
223+
"josdk.controller.ctrl.informer.comparable-resource-versions",
222224
"josdk.controller.ctrl.namespaces",
223225
"josdk.controller.ctrl.rate-limiter.refresh-period",
224226
"josdk.controller.ctrl.rate-limiter.limit-for-period");
@@ -273,60 +275,129 @@ public <T> Optional<T> getValue(String key, Class<T> type) {
273275
Duration.class,
274276
String.class);
275277

278+
/**
279+
* Maps every scalar setter of {@link ConfigurationServiceOverrider} to the property key that is
280+
* expected to drive it. Matching on the setter name (instead of only on the parameter type) is
281+
* what makes the coverage test below able to detect a setter that has no key at all.
282+
*/
283+
private static final Map<String, String> EXPECTED_OPERATOR_KEYS_BY_SETTER =
284+
Map.ofEntries(
285+
Map.entry("checkingCRDAndValidateLocalModel", "check-crd"),
286+
Map.entry("withReconciliationTerminationTimeout", "reconciliation.termination-timeout"),
287+
Map.entry("withConcurrentReconciliationThreads", "reconciliation.concurrent-threads"),
288+
Map.entry("withConcurrentWorkflowExecutorThreads", "workflow.executor-threads"),
289+
Map.entry("withCloseClientOnStop", "close-client-on-stop"),
290+
Map.entry(
291+
"withStopOnInformerErrorDuringStartup", "informer.stop-on-error-during-startup"),
292+
Map.entry("withCacheSyncTimeout", "informer.cache-sync-timeout"),
293+
Map.entry(
294+
"withSSABasedCreateUpdateMatchForDependentResources",
295+
"dependent-resources.ssa-based-create-update-match"),
296+
Map.entry("withUseSSAToPatchPrimaryResource", "use-ssa-to-patch-primary-resource"),
297+
Map.entry(
298+
"withCloneSecondaryResourcesWhenGettingFromCache",
299+
"clone-secondary-resources-when-getting-from-cache"),
300+
Map.entry("withClusterScopedEventNamespace", "events.cluster-scoped-namespace"));
301+
302+
/**
303+
* Maps every scalar setter of {@link ControllerConfigurationOverrider} to the property key suffix
304+
* that is expected to drive it. Setters that are intentionally not configurable are listed in
305+
* {@link #CONTROLLER_SETTERS_WITHOUT_KEY} instead.
306+
*/
307+
private static final Map<String, String> EXPECTED_CONTROLLER_KEYS_BY_SETTER =
308+
Map.ofEntries(
309+
Map.entry("withFinalizer", "finalizer"),
310+
Map.entry("withGenerationAware", "generation-aware"),
311+
Map.entry("withLabelSelector", "label-selector"),
312+
Map.entry("withShardSelector", "shard-selector"),
313+
Map.entry("withReconciliationMaxInterval", "max-reconciliation-interval"),
314+
Map.entry("withFieldManager", "field-manager"),
315+
Map.entry("withTriggerReconcilerOnAllEvents", "trigger-reconciler-on-all-events"),
316+
Map.entry("withDefaultFilters", "default-filters"),
317+
Map.entry("withInformerListLimit", "informer.list-limit"),
318+
Map.entry("withComparableResourceVersions", "informer.comparable-resource-versions"),
319+
// not a plain binding: the value is a comma-separated list of namespaces
320+
Map.entry("settingNamespace", "namespaces"));
321+
322+
/** Scalar setters that intentionally have no property key. */
323+
private static final Set<String> CONTROLLER_SETTERS_WITHOUT_KEY =
324+
// the controller name is part of the key itself, so it cannot be configured by a key
325+
Set.of("withName");
326+
327+
private static Set<String> scalarSetterNames(Class<?> overriderClass) {
328+
return Arrays.stream(overriderClass.getMethods())
329+
.filter(m -> m.getParameterCount() == 1)
330+
.filter(m -> SUPPORTED_TYPES.contains(m.getParameterTypes()[0]))
331+
.filter(m -> m.getReturnType() == overriderClass)
332+
.filter(m -> m.getAnnotation(Deprecated.class) == null)
333+
.map(java.lang.reflect.Method::getName)
334+
.collect(Collectors.toSet());
335+
}
336+
337+
private static java.util.List<String> queriedKeys(
338+
java.util.function.Consumer<ConfigLoader> usage) {
339+
var keys = new ArrayList<String>();
340+
usage.accept(
341+
new ConfigLoader(
342+
new ConfigProvider() {
343+
@Override
344+
public <T> Optional<T> getValue(String key, Class<T> type) {
345+
keys.add(key);
346+
return Optional.empty();
347+
}
348+
}));
349+
return keys;
350+
}
351+
352+
@Test
353+
void everyScalarSetterOnConfigurationServiceOverriderIsMappedToAKey() {
354+
assertThat(EXPECTED_OPERATOR_KEYS_BY_SETTER.keySet())
355+
.as(
356+
"Every scalar setter on ConfigurationServiceOverrider must be mapped to a property key."
357+
+ " Add the new setter here and bind it in ConfigLoader.OPERATOR_BINDINGS.")
358+
.containsExactlyInAnyOrderElementsOf(
359+
scalarSetterNames(ConfigurationServiceOverrider.class));
360+
}
361+
362+
@Test
363+
void operatorBindingsUseTheExpectedKeysAndTypes() {
364+
var boundKeys =
365+
ConfigLoader.OPERATOR_BINDINGS.stream().map(ConfigBinding::key).collect(Collectors.toSet());
366+
367+
assertThat(boundKeys)
368+
.as("Every mapped operator key must be bound")
369+
.containsAll(EXPECTED_OPERATOR_KEYS_BY_SETTER.values());
370+
assertThat(ConfigLoader.OPERATOR_BINDINGS)
371+
.allSatisfy(b -> assertThat(SUPPORTED_TYPES).contains(b.type()));
372+
assertThat(queriedKeys(ConfigLoader::applyConfigs))
373+
.as("Every bound operator key must actually be looked up")
374+
.containsAll(boundKeys.stream().map(k -> "josdk." + k).collect(Collectors.toSet()));
375+
}
376+
276377
@Test
277-
void operatorBindingsCoverAllSingleScalarSettersOnConfigurationServiceOverrider() {
278-
Set<String> expectedSetters =
279-
Arrays.stream(ConfigurationServiceOverrider.class.getMethods())
280-
.filter(m -> m.getParameterCount() == 1)
281-
.filter(m -> SUPPORTED_TYPES.contains(m.getParameterTypes()[0]))
282-
.filter(m -> m.getReturnType() == ConfigurationServiceOverrider.class)
283-
.map(java.lang.reflect.Method::getName)
284-
.collect(Collectors.toSet());
285-
286-
Set<String> boundMethodNames =
287-
ConfigLoader.OPERATOR_BINDINGS.stream()
288-
.flatMap(
289-
b ->
290-
Arrays.stream(ConfigurationServiceOverrider.class.getMethods())
291-
.filter(m -> m.getParameterCount() == 1)
292-
.filter(m -> isTypeCompatible(m.getParameterTypes()[0], b.type()))
293-
.filter(m -> m.getReturnType() == ConfigurationServiceOverrider.class)
294-
.map(java.lang.reflect.Method::getName))
295-
.collect(Collectors.toSet());
296-
297-
assertThat(boundMethodNames)
298-
.as("Every scalar setter on ConfigurationServiceOverrider must be covered by a binding")
299-
.containsExactlyInAnyOrderElementsOf(expectedSetters);
300-
}
301-
302-
@Test
303-
void controllerBindingsCoverAllSingleScalarSettersOnControllerConfigurationOverrider() {
304-
Set<String> expectedSetters =
305-
Arrays.stream(ControllerConfigurationOverrider.class.getMethods())
306-
.filter(m -> m.getParameterCount() == 1)
307-
.filter(m -> SUPPORTED_TYPES.contains(m.getParameterTypes()[0]))
308-
.filter(m -> m.getReturnType() == ControllerConfigurationOverrider.class)
309-
.filter(m -> m.getAnnotation(Deprecated.class) == null)
310-
.map(java.lang.reflect.Method::getName)
311-
.collect(Collectors.toSet());
312-
313-
Set<String> boundMethodNames =
314-
ConfigLoader.CONTROLLER_BINDINGS.stream()
315-
.flatMap(
316-
b ->
317-
Arrays.stream(ControllerConfigurationOverrider.class.getMethods())
318-
.filter(m -> m.getParameterCount() == 1)
319-
.filter(m -> isTypeCompatible(m.getParameterTypes()[0], b.type()))
320-
.filter(m -> m.getReturnType() == ControllerConfigurationOverrider.class)
321-
.filter(m -> m.getAnnotation(Deprecated.class) == null)
322-
.map(java.lang.reflect.Method::getName))
323-
.collect(Collectors.toSet());
324-
325-
assertThat(boundMethodNames)
378+
void everyScalarSetterOnControllerConfigurationOverriderIsMappedToAKey() {
379+
var mapped = new java.util.HashSet<>(EXPECTED_CONTROLLER_KEYS_BY_SETTER.keySet());
380+
mapped.addAll(CONTROLLER_SETTERS_WITHOUT_KEY);
381+
382+
assertThat(mapped)
326383
.as(
327-
"Every scalar setter on ControllerConfigurationOverrider should be covered by a"
328-
+ " binding")
329-
.containsExactlyInAnyOrderElementsOf(expectedSetters);
384+
"Every scalar setter on ControllerConfigurationOverrider must be mapped to a property"
385+
+ " key. Add the new setter here and bind it in ConfigLoader.CONTROLLER_BINDINGS,"
386+
+ " or list it in CONTROLLER_SETTERS_WITHOUT_KEY if it is not configurable.")
387+
.containsExactlyInAnyOrderElementsOf(
388+
scalarSetterNames(ControllerConfigurationOverrider.class));
389+
}
390+
391+
@Test
392+
void controllerBindingsUseTheExpectedKeysAndTypes() {
393+
assertThat(ConfigLoader.CONTROLLER_BINDINGS)
394+
.allSatisfy(b -> assertThat(SUPPORTED_TYPES).contains(b.type()));
395+
assertThat(queriedKeys(loader -> loader.applyControllerConfigs("ctrl")))
396+
.as("Every mapped controller key must actually be looked up")
397+
.containsAll(
398+
EXPECTED_CONTROLLER_KEYS_BY_SETTER.values().stream()
399+
.map(k -> "josdk.controller.ctrl." + k)
400+
.collect(Collectors.toSet()));
330401
}
331402

332403
// -- leader election --------------------------------------------------------
@@ -655,16 +726,52 @@ void namespacesAreIsolatedPerControllerName() {
655726
.containsExactlyInAnyOrder("beta-ns1", "beta-ns2");
656727
}
657728

658-
private static boolean isTypeCompatible(Class<?> methodParam, Class<?> bindingType) {
659-
if (methodParam == bindingType) return true;
660-
if (methodParam == boolean.class && bindingType == Boolean.class) return true;
661-
if (methodParam == Boolean.class && bindingType == boolean.class) return true;
662-
if (methodParam == int.class && bindingType == Integer.class) return true;
663-
if (methodParam == Integer.class && bindingType == int.class) return true;
664-
if (methodParam == long.class && bindingType == Long.class) return true;
665-
if (methodParam == Long.class && bindingType == long.class) return true;
666-
if (methodParam == double.class && bindingType == Double.class) return true;
667-
if (methodParam == Double.class && bindingType == double.class) return true;
668-
return false;
729+
// -- informer and filter flags ----------------------------------------------
730+
731+
private static io.javaoperatorsdk.operator.api.config.ControllerConfiguration<
732+
io.fabric8.kubernetes.api.model.ConfigMap>
733+
applyAndBuild(
734+
java.util.function.Consumer<
735+
ControllerConfigurationOverrider<io.fabric8.kubernetes.api.model.ConfigMap>>
736+
consumer) {
737+
var overrider = ControllerConfigurationOverrider.override(baseControllerConfig());
738+
consumer.accept(overrider);
739+
return overrider.build();
740+
}
741+
742+
@Test
743+
void defaultFiltersAreLeftUntouchedWhenPropertyIsAbsent() {
744+
var loader = new ConfigLoader(mapProvider(Map.of()));
745+
assertThat(applyAndBuild(loader.applyControllerConfigs("ctrl")).isDefaultFilters()).isTrue();
746+
}
747+
748+
@Test
749+
void defaultFiltersCanBeDisabled() {
750+
var loader =
751+
new ConfigLoader(mapProvider(Map.of("josdk.controller.ctrl.default-filters", false)));
752+
assertThat(applyAndBuild(loader.applyControllerConfigs("ctrl")).isDefaultFilters()).isFalse();
753+
}
754+
755+
@Test
756+
void comparableResourceVersionsAreLeftUntouchedWhenPropertyIsAbsent() {
757+
var loader = new ConfigLoader(mapProvider(Map.of()));
758+
assertThat(
759+
applyAndBuild(loader.applyControllerConfigs("ctrl"))
760+
.getInformerConfig()
761+
.isComparableResourceVersions())
762+
.isEqualTo(baseControllerConfig().getInformerConfig().isComparableResourceVersions());
763+
}
764+
765+
@Test
766+
void comparableResourceVersionsCanBeDisabled() {
767+
var loader =
768+
new ConfigLoader(
769+
mapProvider(
770+
Map.of("josdk.controller.ctrl.informer.comparable-resource-versions", false)));
771+
assertThat(
772+
applyAndBuild(loader.applyControllerConfigs("ctrl"))
773+
.getInformerConfig()
774+
.isComparableResourceVersions())
775+
.isFalse();
669776
}
670777
}

0 commit comments

Comments
 (0)