Replaced Quarkus Config Profiles and Expansion with SmallRye Config

[radcortez]

No description provided.

[gsmet]

@geoand do you want to have a quick look at that one? Given we have all tests passing, I think we are probably good.

[geoand]

Well, given that the tests were run a month ago, I would feel a lot better if the PR was rebased onto master

[geoand]

I definitely like it and want it in :). If the tests pass on latest master, I'm +1

[radcortez]

Let me rebase.

[radcortez]

Well it failed, but it seems more of an issue of the CI:

2020-06-11T13:07:09.9404210Z [ERROR] Tests run: 4, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 337.459 s <<< FAILURE! - in io.quarkus.maven.it.JarRunnerIT
2020-06-11T13:07:09.9404896Z [ERROR] testThatMutableFastJarWorks  Time elapsed: 308.77 s  <<< ERROR!
2020-06-11T13:07:09.9405246Z org.awaitility.core.ConditionTimeoutException: Condition with io.quarkus.maven.it.JarRunnerIT was not fulfilled within 1 minutes.

I didn't run the full suite locally, but executed tests for this module and it is fine. Any ideas @geoand ?

[geoand]

I'll take a look, but it probably is an infra issue more than anything else

[geoand]

I am good with this, but I'll let @dmlloyd have the final say

[radcortez]

Sure. I've released SmallRye Config 1.8.1 today and updated the PR with it.

CI build looks betters, JDK 14 seems to have failed due to not executors available, but JDK 8 and 11 went fine.

[radcortez]

🎉

[knutwannheden]

This PR is causing a problem in a custom ConfigSource of ours, which is similar to VaultConfigSource in quarkus-vault as we copied parts from there. Specifically we have a problem with how getVaultProperty() is used to load the source's own configuration:

quarkus/extensions/vault/runtime/src/main/java/io/quarkus/vault/runtime/config/VaultConfigSource.java

Lines 313 to 338 in 3aa4346

	private String getVaultProperty(String key, String defaultValue) {
	String propertyName = PROPERTY_PREFIX + key;
	return getConfigSourceStream()
	.map(configSource -> configSource.getValue(propertyName))
	.filter(value -> value != null && value.length() != 0)
	.map(String::trim)
	.findFirst()
	.orElse(defaultValue);
	}
	private Map<String, List<String>> getSecretConfigKvPrefixPaths() {
	return getConfigSourceStream()
	.flatMap(configSource -> configSource.getPropertyNames().stream())
	.map(this::getSecretConfigKvPrefixPathName)
	.filter(Objects::nonNull)
	.distinct()
	.map(this::createNameSecretConfigKvPrefixPathPair)
	.collect(toMap(SimpleEntry::getKey, SimpleEntry::getValue));
	}
	private Stream<ConfigSource> getConfigSourceStream() {
	Config config = ConfigProviderResolver.instance().getConfig();
	return StreamSupport.stream(config.getConfigSources().spliterator(), false).filter(this::retain);
	}

In Quarkus 1.5.0 there were DeploymentProfileConfigSource instances which made sure the config property values were loaded from the correct profile. This type no longer exists in Quarkus 1.6.0.CR1 (I imagine this logic was moved elsewhere) and as a result the profile-specific config property value isn't found. I imagine the VaultConfigSource has the same problem, but you don't have a test for this scenario.

As a temporary solution I will change our code to detect the recursive call of ConfigSource#getValue(String) and instead work directly with Config rather than the ConfigSources.

[geoand]

cc @radcortez ^

[radcortez]

I'll have a look.

[knutwannheden]

FWIW, when adding logic to detect any recursive call and just return null it works as expected again.

[radcortez]

@knutwannheden is this only happening in test / dev mode? Does this happen in runtime?

[knutwannheden]

@radcortez I didn't test it in production mode, but AFAICT it would be a problem there as well if a config source like VaultConfigSource were to read its own config properties the way it does and expect the values to be read from a non-default MP Config profile.

[radcortez]

The DeploymentProfileConfigSource is now supplied as a feature in SmallRye Config, so that is why it got removed. In its place, the profile resolution of SmallRye Config was plugged in, but I'm wondering if DeploymentProfileConfigSource was being applied to dev mode or test, because it doesn't seem to be the case.

[knutwannheden]

I see. I only tested with the test profile, as I had a failing test after upgrading to Quarkus 1.6.0.CR1.

[knutwannheden]

What I was suspecting was that the problem is how VaultConfigSource tries to read values using ConfigProviderResolver.instance().getConfig().getConfigSources(). If it helps I could create a test case for Quarkus.

[radcortez]

What I'm wondering is if DeploymentProfileConfigSource was part of the ConfigSources in that case. By looking into the code it doesn't seem so, but if you find some sort of difference, we need to check that out.

To clarify, you are unable to load profile based configuration in your ConfigSource, right?

[radcortez]

#10322 Should fix this.

@knutwannheden do you have any chance to pull it, build it locally and test it with your code? That would be super helpful. Thank you :)

[knutwannheden]

Sorry, I didn't get around to test the fix yet. I will try to do it tomorrow.

[knutwannheden]

I just tried this with the latest Quarkus SNAPSHOT build and my test still fails. I am however not convinced by the approach of VaultConfigSource anyway. Even if it would properly be able to resolve the value for the config profile, it would still have problems if the value requires property expansion (as in e.g. foo=${bar}). So I think the VaultConfigSource should instead by using the Config API to read its configuration rather than work with the ConfigSource objects. I will report an issue for this.

[radcortez]

Thanks @knutwannheden. Was this an issue with Quarkus before this PR? Or was already there?

[knutwannheden]

The problem with the property expansion was already there before. But our test case (for our own config source) worked in Quarkus 1.5 and fails with Quarkus 1.6.0.CR1. AFAICT the problem is that Config#getConfigSources() can no longer be used to automatically find the config value for the current profile. But personally I don't consider this a valid use case anyway. I think clients should be required to call Config#getValue().

[radcortez]

Yes, one of the reasons is that Quarkus "artificially" supported Profiles and Expansion with a wrapped ConfigSources. This in my opinion was wrong, because they are not truly ConfigSources but just transformations happening on top of sources.

In the last version of SmallRye Config, we added Config interceptors to be able to support these cases and more easily implement behaviour on top of a configuration lookup (logging, relocation, fallback, etc).

We also identified the issue about ConfigSources configuring themselves, and there was no good mechanism to do it, other than accessing the Sources or Config and then protecting the code to not fall into a cycle. Moving forward, once we release a new version of SmallRye Config, the recommended way is to use the https://github.com/smallrye/smallrye-config/blob/master/implementation/src/main/java/io/smallrye/config/ConfigSourceFactory.java, which should make implementing sources that configure themselves way cleaner.