From 427039f2253043f72e2e63c320ba60ab5a8e16b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Fri, 18 Sep 2026 15:28:22 +0200 Subject: [PATCH] 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..default-filters` - add `ControllerConfigurationOverrider#withComparableResourceVersions` and `josdk.controller..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. --- .../documentation/operations/configuration.md | 2 + .../ControllerConfigurationOverrider.java | 12 + .../operator/config/loader/ConfigLoader.java | 10 +- .../config/loader/ConfigLoaderTest.java | 233 +++++++++++++----- 4 files changed, 193 insertions(+), 64 deletions(-) diff --git a/docs/content/en/docs/documentation/operations/configuration.md b/docs/content/en/docs/documentation/operations/configuration.md index e7d2f07990..cee9f65efa 100644 --- a/docs/content/en/docs/documentation/operations/configuration.md +++ b/docs/content/en/docs/documentation/operations/configuration.md @@ -350,6 +350,7 @@ All controller-level keys are prefixed with `josdk.controller.. | `josdk.controller..max-reconciliation-interval` | `Duration` | Maximum interval between reconciliations even without events | | `josdk.controller..field-manager` | `String` | Field manager name used for SSA operations | | `josdk.controller..trigger-reconciler-on-all-events` | `Boolean` | Trigger reconciliation on every event, not only meaningful changes | +| `josdk.controller..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 | #### Watched Namespaces @@ -383,6 +384,7 @@ josdk.controller.mycontroller.namespaces=team-a,team-b | `josdk.controller..informer.label-selector` | `String` | Label selector for the primary resource informer (alias for `label-selector`) | | `josdk.controller..informer.shard-selector` | `String` | Shard selector for the primary resource informer (alias for `shard-selector`) | | `josdk.controller..informer.list-limit` | `Long` | Page size for paginated informer list requests; omit for no pagination | +| `josdk.controller..informer.comparable-resource-versions` | `Boolean` | Whether the resource versions of the primary resource can be treated as integers and thus compared | #### Retry 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 1c1e03c870..bf0f54e41a 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 @@ -198,6 +198,18 @@ public ControllerConfigurationOverrider withDefaultFilters(boolean defaultFil return this; } + /** + * Sets whether the resource versions of the watched primary resource can be considered integers, + * and thus compared to each other. + * + * @see io.javaoperatorsdk.operator.api.config.informer.Informer#comparableResourceVersions() + */ + public ControllerConfigurationOverrider withComparableResourceVersions( + boolean comparableResourceVersions) { + config.withComparableResourceVersions(comparableResourceVersions); + return this; + } + /** * Sets a max page size limit when starting the informer. This will result in pagination while * populating the cache. This means that longer lists will take multiple requests to fetch. See diff --git a/operator-framework/src/main/java/io/javaoperatorsdk/operator/config/loader/ConfigLoader.java b/operator-framework/src/main/java/io/javaoperatorsdk/operator/config/loader/ConfigLoader.java index 3d0eb3abfe..8aed14da0d 100644 --- a/operator-framework/src/main/java/io/javaoperatorsdk/operator/config/loader/ConfigLoader.java +++ b/operator-framework/src/main/java/io/javaoperatorsdk/operator/config/loader/ConfigLoader.java @@ -171,6 +171,10 @@ public static ConfigLoader getDefault() { "trigger-reconciler-on-all-events", Boolean.class, ControllerConfigurationOverrider::withTriggerReconcilerOnAllEvents), + new ConfigBinding<>( + "default-filters", + Boolean.class, + ControllerConfigurationOverrider::withDefaultFilters), new ConfigBinding<>( "informer.label-selector", String.class, @@ -182,7 +186,11 @@ public static ConfigLoader getDefault() { new ConfigBinding<>( "informer.list-limit", Long.class, - ControllerConfigurationOverrider::withInformerListLimit)); + ControllerConfigurationOverrider::withInformerListLimit), + new ConfigBinding<>( + "informer.comparable-resource-versions", + Boolean.class, + ControllerConfigurationOverrider::withComparableResourceVersions)); private final ConfigProvider configProvider; diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/config/loader/ConfigLoaderTest.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/config/loader/ConfigLoaderTest.java index e3e3d51be8..1714a2b038 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/config/loader/ConfigLoaderTest.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/config/loader/ConfigLoaderTest.java @@ -216,9 +216,11 @@ public Optional getValue(String key, Class type) { "josdk.controller.ctrl.max-reconciliation-interval", "josdk.controller.ctrl.field-manager", "josdk.controller.ctrl.trigger-reconciler-on-all-events", + "josdk.controller.ctrl.default-filters", "josdk.controller.ctrl.informer.label-selector", "josdk.controller.ctrl.informer.shard-selector", "josdk.controller.ctrl.informer.list-limit", + "josdk.controller.ctrl.informer.comparable-resource-versions", "josdk.controller.ctrl.namespaces", "josdk.controller.ctrl.rate-limiter.refresh-period", "josdk.controller.ctrl.rate-limiter.limit-for-period"); @@ -273,60 +275,129 @@ public Optional getValue(String key, Class type) { Duration.class, String.class); + /** + * Maps every scalar setter of {@link ConfigurationServiceOverrider} to the property key that is + * expected to drive it. Matching on the setter name (instead of only on the parameter type) is + * what makes the coverage test below able to detect a setter that has no key at all. + */ + private static final Map EXPECTED_OPERATOR_KEYS_BY_SETTER = + Map.ofEntries( + Map.entry("checkingCRDAndValidateLocalModel", "check-crd"), + Map.entry("withReconciliationTerminationTimeout", "reconciliation.termination-timeout"), + Map.entry("withConcurrentReconciliationThreads", "reconciliation.concurrent-threads"), + Map.entry("withConcurrentWorkflowExecutorThreads", "workflow.executor-threads"), + Map.entry("withCloseClientOnStop", "close-client-on-stop"), + Map.entry( + "withStopOnInformerErrorDuringStartup", "informer.stop-on-error-during-startup"), + Map.entry("withCacheSyncTimeout", "informer.cache-sync-timeout"), + Map.entry( + "withSSABasedCreateUpdateMatchForDependentResources", + "dependent-resources.ssa-based-create-update-match"), + Map.entry("withUseSSAToPatchPrimaryResource", "use-ssa-to-patch-primary-resource"), + Map.entry( + "withCloneSecondaryResourcesWhenGettingFromCache", + "clone-secondary-resources-when-getting-from-cache"), + Map.entry("withClusterScopedEventNamespace", "events.cluster-scoped-namespace")); + + /** + * Maps every scalar setter of {@link ControllerConfigurationOverrider} to the property key suffix + * that is expected to drive it. Setters that are intentionally not configurable are listed in + * {@link #CONTROLLER_SETTERS_WITHOUT_KEY} instead. + */ + private static final Map EXPECTED_CONTROLLER_KEYS_BY_SETTER = + Map.ofEntries( + Map.entry("withFinalizer", "finalizer"), + Map.entry("withGenerationAware", "generation-aware"), + Map.entry("withLabelSelector", "label-selector"), + Map.entry("withShardSelector", "shard-selector"), + Map.entry("withReconciliationMaxInterval", "max-reconciliation-interval"), + Map.entry("withFieldManager", "field-manager"), + Map.entry("withTriggerReconcilerOnAllEvents", "trigger-reconciler-on-all-events"), + Map.entry("withDefaultFilters", "default-filters"), + Map.entry("withInformerListLimit", "informer.list-limit"), + Map.entry("withComparableResourceVersions", "informer.comparable-resource-versions"), + // not a plain binding: the value is a comma-separated list of namespaces + Map.entry("settingNamespace", "namespaces")); + + /** Scalar setters that intentionally have no property key. */ + private static final Set CONTROLLER_SETTERS_WITHOUT_KEY = + // the controller name is part of the key itself, so it cannot be configured by a key + Set.of("withName"); + + private static Set scalarSetterNames(Class overriderClass) { + return Arrays.stream(overriderClass.getMethods()) + .filter(m -> m.getParameterCount() == 1) + .filter(m -> SUPPORTED_TYPES.contains(m.getParameterTypes()[0])) + .filter(m -> m.getReturnType() == overriderClass) + .filter(m -> m.getAnnotation(Deprecated.class) == null) + .map(java.lang.reflect.Method::getName) + .collect(Collectors.toSet()); + } + + private static java.util.List queriedKeys( + java.util.function.Consumer usage) { + var keys = new ArrayList(); + usage.accept( + new ConfigLoader( + new ConfigProvider() { + @Override + public Optional getValue(String key, Class type) { + keys.add(key); + return Optional.empty(); + } + })); + return keys; + } + + @Test + void everyScalarSetterOnConfigurationServiceOverriderIsMappedToAKey() { + assertThat(EXPECTED_OPERATOR_KEYS_BY_SETTER.keySet()) + .as( + "Every scalar setter on ConfigurationServiceOverrider must be mapped to a property key." + + " Add the new setter here and bind it in ConfigLoader.OPERATOR_BINDINGS.") + .containsExactlyInAnyOrderElementsOf( + scalarSetterNames(ConfigurationServiceOverrider.class)); + } + + @Test + void operatorBindingsUseTheExpectedKeysAndTypes() { + var boundKeys = + ConfigLoader.OPERATOR_BINDINGS.stream().map(ConfigBinding::key).collect(Collectors.toSet()); + + assertThat(boundKeys) + .as("Every mapped operator key must be bound") + .containsAll(EXPECTED_OPERATOR_KEYS_BY_SETTER.values()); + assertThat(ConfigLoader.OPERATOR_BINDINGS) + .allSatisfy(b -> assertThat(SUPPORTED_TYPES).contains(b.type())); + assertThat(queriedKeys(ConfigLoader::applyConfigs)) + .as("Every bound operator key must actually be looked up") + .containsAll(boundKeys.stream().map(k -> "josdk." + k).collect(Collectors.toSet())); + } + @Test - void operatorBindingsCoverAllSingleScalarSettersOnConfigurationServiceOverrider() { - Set expectedSetters = - Arrays.stream(ConfigurationServiceOverrider.class.getMethods()) - .filter(m -> m.getParameterCount() == 1) - .filter(m -> SUPPORTED_TYPES.contains(m.getParameterTypes()[0])) - .filter(m -> m.getReturnType() == ConfigurationServiceOverrider.class) - .map(java.lang.reflect.Method::getName) - .collect(Collectors.toSet()); - - Set boundMethodNames = - ConfigLoader.OPERATOR_BINDINGS.stream() - .flatMap( - b -> - Arrays.stream(ConfigurationServiceOverrider.class.getMethods()) - .filter(m -> m.getParameterCount() == 1) - .filter(m -> isTypeCompatible(m.getParameterTypes()[0], b.type())) - .filter(m -> m.getReturnType() == ConfigurationServiceOverrider.class) - .map(java.lang.reflect.Method::getName)) - .collect(Collectors.toSet()); - - assertThat(boundMethodNames) - .as("Every scalar setter on ConfigurationServiceOverrider must be covered by a binding") - .containsExactlyInAnyOrderElementsOf(expectedSetters); - } - - @Test - void controllerBindingsCoverAllSingleScalarSettersOnControllerConfigurationOverrider() { - Set expectedSetters = - Arrays.stream(ControllerConfigurationOverrider.class.getMethods()) - .filter(m -> m.getParameterCount() == 1) - .filter(m -> SUPPORTED_TYPES.contains(m.getParameterTypes()[0])) - .filter(m -> m.getReturnType() == ControllerConfigurationOverrider.class) - .filter(m -> m.getAnnotation(Deprecated.class) == null) - .map(java.lang.reflect.Method::getName) - .collect(Collectors.toSet()); - - Set boundMethodNames = - ConfigLoader.CONTROLLER_BINDINGS.stream() - .flatMap( - b -> - Arrays.stream(ControllerConfigurationOverrider.class.getMethods()) - .filter(m -> m.getParameterCount() == 1) - .filter(m -> isTypeCompatible(m.getParameterTypes()[0], b.type())) - .filter(m -> m.getReturnType() == ControllerConfigurationOverrider.class) - .filter(m -> m.getAnnotation(Deprecated.class) == null) - .map(java.lang.reflect.Method::getName)) - .collect(Collectors.toSet()); - - assertThat(boundMethodNames) + void everyScalarSetterOnControllerConfigurationOverriderIsMappedToAKey() { + var mapped = new java.util.HashSet<>(EXPECTED_CONTROLLER_KEYS_BY_SETTER.keySet()); + mapped.addAll(CONTROLLER_SETTERS_WITHOUT_KEY); + + assertThat(mapped) .as( - "Every scalar setter on ControllerConfigurationOverrider should be covered by a" - + " binding") - .containsExactlyInAnyOrderElementsOf(expectedSetters); + "Every scalar setter on ControllerConfigurationOverrider must be mapped to a property" + + " key. Add the new setter here and bind it in ConfigLoader.CONTROLLER_BINDINGS," + + " or list it in CONTROLLER_SETTERS_WITHOUT_KEY if it is not configurable.") + .containsExactlyInAnyOrderElementsOf( + scalarSetterNames(ControllerConfigurationOverrider.class)); + } + + @Test + void controllerBindingsUseTheExpectedKeysAndTypes() { + assertThat(ConfigLoader.CONTROLLER_BINDINGS) + .allSatisfy(b -> assertThat(SUPPORTED_TYPES).contains(b.type())); + assertThat(queriedKeys(loader -> loader.applyControllerConfigs("ctrl"))) + .as("Every mapped controller key must actually be looked up") + .containsAll( + EXPECTED_CONTROLLER_KEYS_BY_SETTER.values().stream() + .map(k -> "josdk.controller.ctrl." + k) + .collect(Collectors.toSet())); } // -- leader election -------------------------------------------------------- @@ -655,16 +726,52 @@ void namespacesAreIsolatedPerControllerName() { .containsExactlyInAnyOrder("beta-ns1", "beta-ns2"); } - private static boolean isTypeCompatible(Class methodParam, Class bindingType) { - if (methodParam == bindingType) return true; - if (methodParam == boolean.class && bindingType == Boolean.class) return true; - if (methodParam == Boolean.class && bindingType == boolean.class) return true; - if (methodParam == int.class && bindingType == Integer.class) return true; - if (methodParam == Integer.class && bindingType == int.class) return true; - if (methodParam == long.class && bindingType == Long.class) return true; - if (methodParam == Long.class && bindingType == long.class) return true; - if (methodParam == double.class && bindingType == Double.class) return true; - if (methodParam == Double.class && bindingType == double.class) return true; - return false; + // -- informer and filter flags ---------------------------------------------- + + private static io.javaoperatorsdk.operator.api.config.ControllerConfiguration< + io.fabric8.kubernetes.api.model.ConfigMap> + applyAndBuild( + java.util.function.Consumer< + ControllerConfigurationOverrider> + consumer) { + var overrider = ControllerConfigurationOverrider.override(baseControllerConfig()); + consumer.accept(overrider); + return overrider.build(); + } + + @Test + void defaultFiltersAreLeftUntouchedWhenPropertyIsAbsent() { + var loader = new ConfigLoader(mapProvider(Map.of())); + assertThat(applyAndBuild(loader.applyControllerConfigs("ctrl")).isDefaultFilters()).isTrue(); + } + + @Test + void defaultFiltersCanBeDisabled() { + var loader = + new ConfigLoader(mapProvider(Map.of("josdk.controller.ctrl.default-filters", false))); + assertThat(applyAndBuild(loader.applyControllerConfigs("ctrl")).isDefaultFilters()).isFalse(); + } + + @Test + void comparableResourceVersionsAreLeftUntouchedWhenPropertyIsAbsent() { + var loader = new ConfigLoader(mapProvider(Map.of())); + assertThat( + applyAndBuild(loader.applyControllerConfigs("ctrl")) + .getInformerConfig() + .isComparableResourceVersions()) + .isEqualTo(baseControllerConfig().getInformerConfig().isComparableResourceVersions()); + } + + @Test + void comparableResourceVersionsCanBeDisabled() { + var loader = + new ConfigLoader( + mapProvider( + Map.of("josdk.controller.ctrl.informer.comparable-resource-versions", false))); + assertThat( + applyAndBuild(loader.applyControllerConfigs("ctrl")) + .getInformerConfig() + .isComparableResourceVersions()) + .isFalse(); } }