feat: configuration adapters

[csviri]

Abstraction and implementation to load configurations from env and system properties.

[csviri]

@xstefank @metacosm pls take a look on this (description in related issue).

1. Maybe we should have this set by default in the background for an operator?
2. Not sure if we need also ConfigLoader as an interface, now only the ConfigProvider is abstracted away, but maybe we need both to cover all possible extension cases.

thank you!

[Copilot]

Pull request overview

Introduces a configuration-loading abstraction (env vars + system properties) and wires it into the sample Tomcat operator to apply operator- and controller-level configuration overrides.

Changes:

• Added ConfigProvider/DefaultConfigProvider plus ConfigLoader to bind config keys to configuration overriders.
• Updated sample reconcilers/operators to use explicit controller names and apply controller-specific configs.
• Added unit tests covering config provider conversion and loader binding behavior.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
sample-operators/tomcat-operator/src/main/java/io/javaoperatorsdk/operator/sample/WebappReconciler.java	Adds an explicit controller name constant and uses it in @ControllerConfiguration.
sample-operators/tomcat-operator/src/main/java/io/javaoperatorsdk/operator/sample/TomcatReconciler.java	Adds an explicit controller name constant and uses it in @ControllerConfiguration.
sample-operators/tomcat-operator/src/main/java/io/javaoperatorsdk/operator/sample/TomcatOperator.java	Uses ConfigLoader to apply operator/controller configuration at startup.
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/loader/ConfigProvider.java	Introduces a typed key/value config provider abstraction.
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/loader/DefaultConfigProvider.java	Implements provider backed by env vars + system properties with type conversion.
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/loader/ConfigBinding.java	Adds a binding primitive to connect keys, types, and overrider setters.
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/loader/ConfigLoader.java	Implements binding-driven application of operator/controller config, including retry overrides.
operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/loader/DefaultConfigProviderTest.java	Tests env/sysprop precedence and value conversion.
operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/loader/ConfigLoaderTest.java	Tests operator/controller config application and key construction.
operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/loader/ConfigBindingTest.java	Tests ConfigBinding stores key/type/setter correctly.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The unchecked double-cast here is a maintenance hazard and the comment mentions covariance, but BiConsumer is invariant in its type parameters. A safer approach is to adjust ConfigBinding to accept BiConsumer<? super O, ? super T> and/or define CONTROLLER_BINDINGS with a type that doesn’t require casting (e.g., keep it raw/erased in one place and only cast the final Consumer once), minimizing the scope of unchecked operations.

Suggested change

	// Cast is safe: the setter BiConsumer<ControllerConfigurationOverrider<?>, T> is covariant in
	// its first parameter for our usage – we only ever call it with
	// ControllerConfigurationOverrider<R>.
	List<ConfigBinding<ControllerConfigurationOverrider<R>, ?>> bindings =
	(List<ConfigBinding<ControllerConfigurationOverrider<R>, ?>>) (List<?>) CONTROLLER_BINDINGS;
	Consumer<ControllerConfigurationOverrider<R>> consumer = buildConsumer(bindings, prefix);
	// Build the consumer from the shared controller bindings using a raw list to avoid
	// propagating unchecked generic casts. We then narrow it in a single place below.
	Consumer<?> rawConsumer = buildConsumer((List) CONTROLLER_BINDINGS, prefix);
	Consumer<ControllerConfigurationOverrider<R>> consumer =
	(Consumer<ControllerConfigurationOverrider<R>>) rawConsumer;

[Copilot]

Retry override behavior is new but isn’t covered by tests yet. Add unit tests that set one or more retry keys and assert that the returned controller consumer calls withRetry(...) and that the resulting GenericRetry instance reflects only the configured overrides (including Double parsing for retry.interval-multiplier).

[metacosm]

To be honest, I'm not sure this is something we want to support.

For one thing, this makes the code less understandable, imo.

Also, there are already lots of configuration mechanisms and while I understand the intent behind this PR, creating a wrapper for these mechanisms is not straightforward and will almost inevitably lead to feature creep and/or implementation leaks as users will most likely want to support specific config mechanism features.

Another issue is also maintenance where lots of values are hardcoded which would make refactoring more difficult. Also, if a new configuration option is added, one will have to remember to add it to the ConfigLoader implementation(s).

Finally, I'm not convinced this actually answers a need from the community. Has there been users asking for this? It seems that downstream clients of the SDK are already providing native support for their own configuration systems which, I think, would feel more natural to their users.

[csviri]

Also, there are already lots of configuration mechanisms and while I understand the intent behind this PR, creating a wrapper for these mechanisms is not straightforward and will almost inevitably lead to feature creep and/or implementation leaks as users will most likely want to support specific config mechanism features.

No that is not the intention; from the issue:

Out of the box, we can provide an implementation (no external dependency) that reads environment variables and system properties. Mainly a naming scheme.

So the only mechanism I want to support in this repo is what is in this PR.

The issue is that I'm now maintaining a few operators, and for each, we would like to fine-tune some configurations in prod using environment variables (very common in k8s deployments). But now there is no way to do it out of the box with core JOSDK. So I would like to have an out of the box solution that where it is easy to make: like a one liner as in this example usage in the PR.

So it is quite painful now since I have to replicate the same logic accross multiple operators (read the env variables and handle it to config overrider), while this is a generic problem. That is why Spring + Quarkus extensions solve this out of the box, but for some projects, we simply cannot use those frameworks, unfortunately.

Another issue is also maintenance where lots of values are hardcoded which would make refactoring more difficult. Also, if a new configuration option is added, one will have to remember to add it to the ConfigLoader implementation(s).

yes but that is rare, also I can add a unit tests that checks that dynamically. It is actually not that painful.

[metacosm]

So the only mechanism I want to support in this repo is what is in this PR.

So the issue is that I'm maintaining now a few operators, and for each, we would like to fine-tune some configurations in prod with environment variables (very common in k8s deployments). But now there is no way to do it out of the box with core JOSDK. So I would like to have an out of the box solution that where it is easy to make: like a one liner as in this example usage in the PR.

This could be easily solved by a small lib that would do pretty much what is in this PR that could be shared among your operators, without having that code in the SDK itself. At the very least, this shouldn't be part of the core module if people think this is a useful addition to the SDK.

So it is quite painful now since I have to replicate the same logic accross multiple operators, while this is a generic problem. That is why Spring + Quarkus extensions solve this out of the box, but for some projects, we simply cannot use those frameworks, unfortunately.

The problem is that with this PR there will be a competing configuration mechanism with what frameworks support, adding a potential source of confusion for users. Also, what should happen if a user mistakenly uses the default ConfigLoader in addition to a framework-native solution? This might lead to unexpected results.

Another issue is also maintenance where lots of values are hardcoded which would make refactoring more difficult. Also, if a new configuration option is added, one will have to remember to add it to the ConfigLoader implementation(s).

yes but that is rare, also I can add a unit tests that checks that dynamically. It is actually not that painful.

If we decide to go ahead with this, please add such a test.

[csviri]

This could be easily solved by a small lib that would do pretty much what is in this PR that could be shared among your operators, without having that code in the SDK itself. At the very least, this shouldn't be part of the core module if people think this is a useful addition to the SDK.

We could say that also to quarkus extension. Why not to solve a common subproblem in the core, that is what framework is about?

he problem is that with this PR there will be a competing configuration mechanism with what frameworks support, adding a potential source of confusion for users. Also, what should happen if a user mistakenly uses the default ConfigLoader in addition to a framework-native solution? This might lead to unexpected results.

In the current form it does not, it builds on top of it:

    var configLoader = ConfigLoader.getDefault();
    Operator operator = new Operator(configLoader.applyConfigs());
    operator.register(
        new TomcatReconciler(), configLoader.applyControllerConfigs(TomcatReconciler.NAME));
    operator.register(
        new WebappReconciler(operator.getKubernetesClient()),
        configLoader.applyControllerConfigs(WebappReconciler.NAME));
    operator.start();

It uses the same configuration mechanism that you would write on top of current config approach. How do you see this breaking anything? pls elaborate

[csviri]

I added a unit test to check if we cover all the methods.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The JavaDoc incorrectly states "Returns {@code null} when no binding has a matching value". The implementation on line 157 calls buildConsumer which always returns a non-null Consumer (line 236 returns a no-op consumer o -> {} when no values are found). The JavaDoc should be updated to reflect that this method always returns a non-null Consumer.

[Copilot]

The JavaDoc states that this method "Returns {@code null} when no binding has a matching value", but the implementation always returns a non-null Consumer. When no bindings match, buildConsumer returns a no-op consumer (o -> {}), not null. The JavaDoc should be updated to reflect that this method always returns a non-null Consumer, which may be a no-op if no configuration values are found.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The convert method only handles boxed types (Boolean.class, Integer.class, etc.) but not primitive types (boolean.class, int.class, etc.). While Java's auto-boxing handles this when calling methods that accept primitives, the method should also handle primitive types explicitly for completeness and to match the test expectations in ConfigLoaderTest.SUPPORTED_TYPES (lines 222-233) which includes both boxed and primitive types. Consider adding explicit handling for primitive types to make the implementation more robust and self-documenting.

Suggested change

	} else if (type == Boolean.class) {
	converted = Boolean.parseBoolean(raw);
	} else if (type == Integer.class) {
	converted = Integer.parseInt(raw);
	} else if (type == Long.class) {
	converted = Long.parseLong(raw);
	} else if (type == Double.class) {
	converted = Double.parseDouble(raw);
	} else if (type == Duration.class) {
	converted = Duration.parse(raw);
	} else {
	throw new IllegalArgumentException("Unsupported config type: " + type.getName());
	}
	} else if (type == Boolean.class || type == boolean.class) {
	converted = Boolean.parseBoolean(raw);
	} else if (type == Integer.class || type == int.class) {
	converted = Integer.parseInt(raw);
	} else if (type == Long.class || type == long.class) {
	converted = Long.parseLong(raw);
	} else if (type == Double.class || type == double.class) {
	converted = Double.parseDouble(raw);
	} else if (type == Duration.class) {
	converted = Duration.parse(raw);
	} else {
	throw new IllegalArgumentException("Unsupported config type: " + type.getName());
	}
	if (type.isPrimitive()) {
	@SuppressWarnings("unchecked")
	final T result = (T) converted;
	return result;
	}

[Copilot]

The retry configuration logic (buildRetryConsumer method) lacks test coverage. Consider adding tests that verify retry properties (retry.max-attempts, retry.initial-interval, retry.interval-multiplier, retry.max-interval) are correctly loaded from the ConfigProvider and applied to the ControllerConfigurationOverrider. This would ensure the retry configuration works as expected and prevent regressions.

[Copilot]

The javadoc states "Returns {@code null} when no binding has a matching value" but this is incorrect. The method always returns a non-null Consumer. When no properties are configured, it returns a no-op consumer (from buildConsumer at line 236). The javadoc should be updated to reflect that this method always returns a non-null Consumer, which may be a no-op consumer if no configuration properties are found.

Suggested change

	* as {@code josdk.controller.<controllerName>.<property>}. Returns {@code null} when no binding
	* has a matching value.
	* as {@code josdk.controller.<controllerName>.<property>}. This method never returns
	* {@code null}; if no binding has a matching value, it returns a no-op consumer.

[metacosm]

This could be easily solved by a small lib that would do pretty much what is in this PR that could be shared among your operators, without having that code in the SDK itself. At the very least, this shouldn't be part of the core module if people think this is a useful addition to the SDK.

We could say that also to quarkus extension. Why not to solve a common subproblem in the core, that is what framework is about?

Except that JOSDK is a SDK, not a framework, or at least, it shouldn't be, despite already providing more things than a simple SDK.

As I said, if people think that this is a valuable addition, at the very least, it shouldn't be in the core module so that it requires users to make an explicit decision to use that configuration option to make things less confusing for people who consume the SDK via a framework which already provide native configuration options.

he problem is that with this PR there will be a competing configuration mechanism with what frameworks support, adding a potential source of confusion for users. Also, what should happen if a user mistakenly uses the default ConfigLoader in addition to a framework-native solution? This might lead to unexpected results.

In the current form it does not, it builds on top of it:

    var configLoader = ConfigLoader.getDefault();
    Operator operator = new Operator(configLoader.applyConfigs());
    operator.register(
        new TomcatReconciler(), configLoader.applyControllerConfigs(TomcatReconciler.NAME));
    operator.register(
        new WebappReconciler(operator.getKubernetesClient()),
        configLoader.applyControllerConfigs(WebappReconciler.NAME));
    operator.start();

It uses the same configuration mechanism that you would write on top of current config approach. How do you see this breaking anything? pls elaborate

If you use the Quarkus extension which provides an external configuration mechanism but also use the code above, which, granted, should not happen, the resulting configuration might not be trivial to understand. However, the SDK documentation will state that you can configure things this way, while the Quarkus configuration (or Spring for that matter) might be done another way, leading to user confusion.

[csviri]

Except that JOSDK is a SDK, not a framework, or at least, it shouldn't be, despite already providing more things than a simple SDK.

@metacosm I really don't want to into idealogical disucssion, also things that are matter of definitions, but I think if enough if I say that if you take a look on the modules all core modules start with "operator-framework*" :) no offense.
Also part of JOSDK is also josdk-webhooks project and some others.

it shouldn't be in the core module so that it requires users to make an explicit decision to use that configuration option to make things less confusing for people who consume the SDK via a framework which already provide native configuration options.

But users already have to write explicit code, that is why I asked your and others opnion here:
#3177 (comment)
If we should go further.
So it is not like is a feature switch,users have to explicitly write code to add it. I don't think it would create any confusion.

while the Quarkus configuration (or Spring for that matter) might be done another way, leading to user confusion.

Yes would be nice to sync the naming, but not sure if that is possible at this point. But if we explain this in the docs I think this is what users will easily understand.

it shouldn't be in the core module

If also others think that this should be a separate module fine. But shouldn't we than also separate for example dependent resource as a separate module for v6, since users have to make a decision to use it or not?