Extract ConfigSource implementations from core

[kenfinnigan]

No description provided.

[kenfinnigan]

@jmesnil @dmlloyd @phillip-kruger I've extracted out the various ConfigSource implementations in the main code base, and then added them as dependencies.

As the main code base makes direct reference to them, it doesn't require a service provider file for them to be found. However, it's needed if they're used outside SmallRye.

I wasn't sure if it would cause issues by adding service files for each of the separate config sources. Would it just mean they're discovered twice? Once in our "default" list and then again in "discovered". Does that matter?

[kenfinnigan]

@jmesnil is DirConfigSource used by anything at present?

Wondering if we actually need it?

[dmlloyd]

We don't want config sources to be discovered twice, for sure. The "default" vs "discovered" distinction was probably an error, but I think it's an error we'll have to live with: the default config sources should not have service files.

If it becomes a cyclic dependency problem though (such that you need ServiceLoader or something like it to resolve), you could create a SmallRye-specific DefaultConfigSource interface which extends ConfigSource (or DefaultConfigSourceProvider which extends ConfigSourceProvider) and publish a service file for that instead. Then the defaults can also be discovered using a similar mechanism to the official discoverable ones.

[phillip-kruger]

@dmlloyd - Just out of interest, why should the default config source not have service files ?

[Ladicek]

@kenfinnigan DirConfigSource is used in Thorntail and WildFly.

[jmesnil]

@kenfinnigan DirConfigSource is used in Thorntail and WildFly. It provides access to Kubernetes ConfigMaps

[kenfinnigan]

@Ladicek @jmesnil how is it used at present in Thorntail and WildFly?

I ask because it has a non-default constructor, which causes a failure of a service file is added to "discover it".

I was wondering if we needed a DirConfigSourceProvider that could use a property list of directories to then create the appropriate DirConfigSource instances?

[Ladicek]

I'll defer to @jmesnil, but my understanding is that the WildFly subsystem allows configuring a set of config sources, and instantiates them itself.

[dmlloyd]

@dmlloyd - Just out of interest, why should the default config source not have service files ?

It's a problem when both addDefaultSources and addDiscoveredSources are called. The former would add the list of default sources, and the latter would add all the sources with service files. So if the default sources had services, they might be added twice (depending on the composition of the class path).

Now you could say "well, it's easy, simply don't add the default sources if addDiscoveredSources was called", however this only would work in the case where the default services were visible from the class loader that the config builder is attached to, which is not guaranteed by any means.

Therefore default sources shouldn't use the same service file mechanism as discovered sources.

You might wonder though: "but you suggested using a different service file for default sources!" However I will point out that in this case, with no specification telling us what to do, we can use our own class loader to locate the special service files for the default sources, rather than the builder's target class loader.

Granted this becomes slightly complicated if we move the all of the built-in sources to their own submodules (something I'm only about 50% sure is a good idea; why not do the same for converters then?). However I don't think this puts an unreasonable burden on container vendors (the only people crazy enough to do these class loader gymnastics in the first place) - they simply have to ensure that the built in config sources are available to the class loader of the SmallRye Config API if they want to have a shot at passing the TCK.

[dmlloyd]

@kenfinnigan DirConfigSource is used in Thorntail and WildFly. It provides access to Kubernetes ConfigMaps

Maybe we ought to rename it to "KubernetesConfigMapConfigSource" or at least "FileSystemConfigSource". Every time I forget what it is, and I end up thinking it has something to do with LDAP! 🙂

[kenfinnigan]

I'm not opposed to renaming either if we can find a good one

[kenfinnigan]

I couldn't find DirConfigSource used in Thorntail at all.

Can anyone shed light on how it's used in WF, want to make sure I'm not making a mess!

[Ladicek]

Thorntail uses (a fork of) the WildFly subsystem for MP Config: https://github.com/thorntail/microprofile-config-wildfly/

[kenfinnigan]

doh, separate project, no wonder I didn't find it, thanks.

This is what happens when it's been a long time since I've looked at Thorntail, I forget everything!

[jmesnil]

@kenfinnigan WildFly / Thorntail directly instantiates DirConfigSource from the management model (https://github.com/thorntail/microprofile-config-wildfly/blob/1.x/extension/src/main/java/org/wildfly/extension/microprofile/config/ConfigSourceDefinition.java#L114).
This config source is then added to the Config object in the DUP https://github.com/thorntail/microprofile-config-wildfly/blob/1.x/extension/src/main/java/org/wildfly/extension/microprofile/config/deployment/SubsystemDeploymentProcessor.java#L84

[kenfinnigan]

@jmesnil would it break things if there was a service loadable ConfigSourceProvider?

From a quick look through the WF code I couldn't see it doing any of that type of loading

[jmesnil]

I'm not opposed to rename it but -1 for KubernetesConfigMapConfigSource has it does not depend on any K8S concept (it just scans a FS director)

[kenfinnigan]

What about FileSystemConfigSource?

[jmesnil]

@kenfinnigan are you taking about a service loadable ConfigSourceProvider for DirConfigSource?

[kenfinnigan]

@jmesnil right, otherwise it's a ConfigSource that you have to manually set up, which seems kind of "odd"

[jmesnil]

I suppose we can make a service loadable version that would get the directory parameter from a config property...
But I'd like to keep the current non-default ctor that we use in WildFly / Thorntail

[kenfinnigan]

We can have a non default ctor, it would just mean we need a ConfigSourceProvider to create it.

Currently I had a service file for DirConfigSource which requires a default ctor, but I can wrap in a provider instead.

My concern was whether that would then be automatically picked up by WF/TT

[dmlloyd]

It seems almost always better to use ConfigSourceProvider whenever possible AFAICT.

[kenfinnigan]

Now i've got myself confused as to how a ConfigSourceProvider for this could get config with a list of directories.

Am I chicken and egging it here, or am I missing something obvious?

Side note, is it the weekend yet?!

[jmesnil]

@kenfinnigan that'd be quite similar to our zookeeper implementation https://github.com/smallrye/smallrye-config/tree/master/sources/zookeeper in getCuratorClient()

The ConfigSourceProvider would look for a well known property (whatever.directories) that would contain a comma-separated lists of directories and would create a DirConfigSource for each directory

A key design is that ConfigSourceProvider implementation can call ConfigProvider.getConfig() to get their config from other ConfigSources.

Does that make sense? and please reply yes, I've a 3-day week-end starting in 1/2 hour :)

[kenfinnigan]

Is that design enshrined anywhere? There's nothing in the javadoc about that.

If it's "safe" for ConfigSourceProvider to retrieve config from another ConfigSource that's great, but I couldn't find anything indicating that was the case

[dmlloyd]

@kenfinnigan that'd be quite similar to our zookeeper implementation https://github.com/smallrye/smallrye-config/tree/master/sources/zookeeper in getCuratorClient()

The ConfigSourceProvider would look for a well known property (whatever.directories) that would contain a comma-separated lists of directories and would create a DirConfigSource for each directory

A key design is that ConfigSourceProvider implementation can call ConfigProvider.getConfig() to get their config from other ConfigSources.

Does that make sense? and please reply yes, I've a 3-day week-end starting in 1/2 hour :)

It does make sense for a config provider to be able to recursively probe config from the getValue method - as long as it can handle reentrancy - however the Config won't yet be built at the time that the config source provider is asked to produce ConfigSource instances. This means that you're going to get a default Config - which might not be what you want - or in the case of Quarkus, you'll get an IllegalStateException instead. So I would not recommend using that pattern.

[kenfinnigan]

That's what I was suspecting, so back to chicken and egg then.

How are you meant to have ConfigSourceProviders that can create things based on "something" other than a hard coded in the class value?

For now we may have to have a "disembodied" FileSystemConfigSource that requires manual instantiation

[dmlloyd]

That's what I was suspecting, so back to chicken and egg then.

How are you meant to have ConfigSourceProviders that can create things based on "something" other than a hard coded in the class value?

You can't really. Well I mean there are system properties and so forth but there's no "proper" way to do it. I was thinking it'd be nice if there was though... maybe some way for a config source to reference config from a sort of "view" of higher-ordinal sources?

For now we may have to have a "disembodied" FileSystemConfigSource that requires manual instantiation

That's fine to start with IMO.

[kenfinnigan]

I created eclipse/microprofile-config#470 to cover offering something in the spec re ConfigSourceProvider using config

[jmesnil]

David is right, this pattern requires additional constraints:

• the lookup for the configuration properties required to config this config source must occurred from the getValue method and not when the config source is instantiated
• one has to be careful to explicitly avoid lookup to the configuration properties for this config source from the getValue otherwise an infinite loop will occur (see

  smallrye-config/sources/zookeeper/src/main/java/io/smallrye/configsource/ZooKeeperConfigSource.java

  Line 92 in 7bf0e7c

  if (key.startsWith(IGNORED_PREFIX)) {

  )

[kenfinnigan]

@jmesnil what you describe are restrictions based on the current workings right?

If we had the means to register new ConfigSource instances with already registered ones, that would be fine right?