Overriding a config property with hypen at runtime fails with an exception at startup

[ahofmeister]

Starting a Quarkus application with version 1.13.5.Final+ breaks the configuration of config keys with hyphens that are overriden with environment variables.
Example: mp.messaging.incoming.keycloak-events.connector and MP_MESSAGING_INCOMING_KEYCLOAK_EVENTS_CONNECTOR=smallrye-in-memory

I found out that in the end, the ConfigSource ends up with two properties.

smallrye-config/1.13.1/smallrye-config-1.13.1-sources.jar!/io/smallrye/config/EnvConfigSource.java

The problem is, that MP_MESSAGING_INCOMING_KEYCLOAK_EVENTS_CONNECTOR is converted to mp.messaging.incoming.keycloak.events.connector, but in the application.properties file the key is mp.messaging.incoming.keycloak-events.connector. So the problem is keycloak vs keycloak-events.

I have added a reproducer: https://github.com/ahofmeister/mp-messaging-config-hyphen-reproducer

Please note: It works with Quarkus 1.11.6.Final. (Also note, the reproducer will fail, because of SRMSG00072: Unknown connector forkeycloak-events., which is fine in this case!)

CC @radcortez

[cescoffier]

Did it ou try with Quarkus 2? The new Config api let's us be a bit more precise.

[ahofmeister]

@cescoffier Yes. In fact, I found this bug while migrating from 1.11.6 to 2.0.0.CR3. I think it is broken since 1.13.5.

[cescoffier]

Looking at it more deeply, seems to be something in Smallrye config. @radcortez did we change anything in config?

[ahofmeister]

I think (but @radcortez declined it :D), that it was introduced with smallrye/smallrye-config#563

[cescoffier]

Sounds related.
@radcortez what makes you think it's not related?

[radcortez]

I didn't decline it :)

I've just said that I need to check on how the properties are being read, because the only thing we did there was to add a best effort equivalent to the getPropertyNames. We added it because when we populate maps with getPropertyNames and we get a property in the format X_Y_Z we couldn't find a mapping back to the config object, so we needed to transform it somehow to the x.y.z equivalente. Of course, you loose precision on the conversion because you don't know if the _ refers to a . or a -.

This was mostly an easy fix for the Map problem, or else we would have to rewrite all the mapping logic around getPropertyNames (both in SR and Quarkus).

Anyway, let me have a look.

[radcortez]

@ahofmeister Check this out:
https://github.com/ahofmeister/mp-messaging-config-hyphen-reproducer/pull/1/files

Is this what you expect? I still have to look into the SR RM code, but I believe the main Config code is work as expected.

[knutwannheden]

I just stumbled across the same problem as well (although for another config property with hyphens in its name). I couldn't figure out where the wrong property names reported by ConfigDiagnostic#unknown(String) were coming from. Please let me know if there is any more details I can provide.

[radcortez]

@knutwannheden are you getting an error? or just a warning in the logs?

[knutwannheden]

Just warnings of the form Unrecognized configuration key "xxx" was provided; it will be ignored; verify that the dependency extension for this configuration is set or that you did not make a typo.

[radcortez]

It should be a false warning, which is fixed here: quarkusio/quarkus#17704

Can you confirm that the configuration is still being applied successfully?

[knutwannheden]

Can you confirm that the configuration is still being applied successfully?

Thanks for the pointer. Yes, the configuration is correctly applied.

[ahofmeister]

@radcortez Thanks for the PR, good idea to include a test case! I think, the excpetion only occurs, IF the config ends up with mp.messaging.incoming.keycloak.events.connector in it, it then results in Invalid channel configuration - the connector attribute must be set for channel keycloak. The problem is, that .keycloak. ends up as channel, but this 'new' channel does not have a connector set.

So, no, that is not what I expect. I've added a custom test with another config property with the result I am expecting. I hope it's clear (I added a different case, because the applicaton will not fail on startup in this case)

[radcortez]

Thanks. Let me have a look.

[radcortez]

As I suspected, we suffer from the same issue we had with a corner case in getPropertyNames.

For instance, if you only define an env variable, we cannot reach it because the logic around getPropertyNames is only looking for dotted properties:

smallrye-reactive-messaging/smallrye-reactive-messaging-provider/src/main/java/io/smallrye/reactive/messaging/impl/ConfiguredChannelFactory.java

Lines 91 to 109 in 207b94a

	static Map<String, ConnectorConfig> extractConfigurationFor(String prefix, Config root) {
	Iterable<String> names = root.getPropertyNames();
	Map<String, ConnectorConfig> configs = new HashMap<>();
	names.forEach(key -> {
	// $prefix$name.key=value (the prefix ends with a .)
	if (key.startsWith(prefix)) {
	// Extract the name
	String name = key.substring(prefix.length());
	if (name.charAt(0) == '"') { // Check if the name is enclosed by double quotes
	name = name.substring(1, name.lastIndexOf('"'));
	} else if (name.contains(".")) { // We must remove the part after the first dot
	String tmp = name;
	name = tmp.substring(0, tmp.indexOf('.'));
	}
	configs.put(name, new ConnectorConfig(prefix, root, name));
	}
	});
	return configs;
	}

That was the reason why I added a dotted version of the env key, so they could be found by logic like this. Of course, this doesn't play very well because you loose information from a regular key to an env key and then getPropertyNames return two different key names (the dotted version and the dashed version), which are the same, but at the same time different :)

I think one possible solution is from the SR Config side to just add a dotted env version if we can't find the same dotted version in non env sources to provide the correct name, instead of blindly adding a dotted version for all the env keys.

[radcortez]

To clarify further, without the change in SR Config we can't set up configuration using getPropertyNames that is only set in env variables. But the change in SR Config, as we noticed has some additional unwanted side effects :)

[radcortez]

I believe smallrye/smallrye-config#600 should do the trick.

[cescoffier]

Doing the update right now.

[ahofmeister]

@cescoffier Is there any workaround available? As I mentioned, we are using Quarkus 2.0.0.Final and It uses still 3.5.0 instead of 3.6.0.

Yes, we could rename the topic, but this would be a lot of work, because it's linked in many different locations.

[cescoffier]

The fix is in config not in messaging.

…

On Fri 2 Jul 2021 at 19:37, Alexander Hofmeister ***@***.***> wrote: @cescoffier <https://github.com/cescoffier> Is there any workaround available? As I mentioned, we are using Quarkus 2.0.0.Final and It uses still 3.5.0 instead of 3.6.0. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1264 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADCG7LQNYDRJY5GEOY7JKTTVX2MVANCNFSM462EKSMA> .

[radcortez]

@cescoffier Is there any workaround available? As I mentioned, we are using Quarkus 2.0.0.Final and It uses still 3.5.0 instead of 3.6.0.

Yes, we could rename the topic, but this would be a lot of work, because it's linked in many different locations.

Thr SR Config version 2.4.1 should be compatible with Quarkus 2.0.0.Final. So if you manually update the SR Config version it should work. I believe you may need to also update the SR Config Common.

[radcortez]

Try this:

        <dependency>
            <groupId>io.smallrye.config</groupId>
            <artifactId>smallrye-config-common</artifactId>
            <version>2.4.1</version>
        </dependency>
        <dependency>
            <groupId>io.smallrye.config</groupId>
            <artifactId>smallrye-config</artifactId>
            <version>2.4.1</version>
        </dependency>

[ahofmeister]

@radcortez Thanks! It work's. :)

[kdurnoga]

@radcortez Sorry for resurrecting an old issue but I'm running into exactly the same problem as the OP did, this time with Quarkus 3.12.2 and SR Config 3.8.3. That is, both hyphened (e.g., keycloak-events) and dotted (e.g., keycloak.events) config properties are present when examining the output of getPropertyNames() in ConfiguredChannelFactory.extractConfigurationFor(), if some channel attribute (an MQTT host in my case) is configured via an environment variable.

PropertyNamesConfigSourceInterceptor.java introduced in smallrye/smallrye-config#600 is no longer there (deleted in smallrye/smallrye-config#1000) and I haven't figured what happened with that logic yet.

I'd like to ask: what is the current approach to converting environment variables to properties? Is the approach from smallrye/smallrye-config#600 (introduce a dotted property only if necessary) still valid?

[radcortez]

We had some major rewrites to improve the performance... maybe something was lost.

I'd like to ask: what is the current approach to converting environment variables to properties? Is the approach from smallrye/smallrye-config#600 (introduce a dotted property only if necessary) still valid?

In the case of dynamic property names, we have no way of knowing if a separator is a dot or a dash, so a dotted property name is always required to disambiguate the case: https://quarkus.io/guides/config-reference#environment-variables