feat: configuration adapters

[csviri]

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

TODO:

• add integratio test rather than updating tomcat sample? or both
• add docs

[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]

ConfigLoader.applyConfigs() / applyControllerConfigs(...) can return null (by design per tests). Passing these directly into Operator(...) / operator.register(..., ...) makes the sample rely on downstream null-handling and makes it easy to introduce NPEs when composing consumers. Consider changing the ConfigLoader API to return a no-op Consumer when nothing is configured (or alternatively return Optional<Consumer<...>>) so call sites can be null-free.

[Copilot]

The retry-related keys don’t document units/semantics for interval values (e.g., are the Long values milliseconds? seconds?). Add Javadoc near these suffix constants (or on buildRetryConsumer) that clearly states expected units and acceptable ranges; alternatively consider using Duration for interval properties (and converting to whatever GenericRetry expects) for consistency with other duration configs.

[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).

[Copilot]

Indentation here is inconsistent with the rest of the class (extra leading spaces). Align these members with the project’s standard formatting to keep diffs clean and avoid style/checkstyle issues.

Suggested change

	public static final String TOMCAT_CONTROLLER_NAME = "tomcat";
	private final Logger log = LoggerFactory.getLogger(getClass());
	public static final String TOMCAT_CONTROLLER_NAME = "tomcat";
	private final Logger log = LoggerFactory.getLogger(getClass());

[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]

When parsing fails (e.g., Integer.parseInt throws NumberFormatException for invalid integer values, or Duration.parse throws DateTimeParseException for invalid duration format), the exception will bubble up without context about which configuration key caused the problem. Consider wrapping parse exceptions with more informative error messages. This could be done by modifying getValue to catch parse exceptions from convert and re-throw them with the key name included, helping users diagnose configuration errors more easily.

[Copilot]

System.getProperty can return an empty string for a property that exists but has no value. Currently, an empty string will be passed to convert, which will throw a parsing exception for numeric types and Duration. Consider treating empty strings the same as null values by returning Optional.empty(), as empty strings are typically not valid configuration values and would just cause parsing errors.

[Copilot]

Environment variables can also be set to empty strings. Similar to the system property case, empty strings should likely be treated as absent values by checking if the returned envValue is empty and returning Optional.empty() in that case, to avoid parsing errors.

Suggested change

	if (envValue != null) {
	return envValue;
	}
	return System.getProperty(key);
	if (envValue != null && !envValue.isEmpty()) {
	return envValue;
	}
	String propertyValue = System.getProperty(key);
	if (propertyValue != null && !propertyValue.isEmpty()) {
	return propertyValue;
	}
	return null;

[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 method does not validate that controllerName is not null. If null is passed, a NullPointerException will be thrown at line 169 when concatenating strings. Consider adding a null check with an appropriate exception message, such as: Objects.requireNonNull(controllerName, "controllerName must not be null").

[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?

[metacosm]

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?

Maybe dependent resources should have been in a different module, indeed. However, that ship has sailed quite a long time ago already and dependent resources are now quite a central feature of the SDK. Moving it to a different module at this point would not help and rather make people change their code for no real benefit.

The situation we're discussing here, on the other hand, is completely different. The configuration support you're proposing is not a central feature. Moreover, this is a feature that directly "competes" with native solutions provided by frameworks that consume JOSDK. Putting that configuration support in a separate module rather than the core therefore makes sense as there shouldn't be any possible confusion on which configuration mechanism should be used when consuming JOSDK via a framework.

For that matter, the current implementation is rather opinionated and might not suit the needs of many people. For example, people might rather use a configuration file than environment variables or maybe a ConfigMap or some other mechanism. This makes its applicability rather limited in my opinion and again justifies it not being part of the core module. Maybe it should even be a separate add-on project just like the webhook support or the Spring Boot starter and not part of the SDK itself.