Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ability to use configuration from an already populated source in order to configure a new one #215

Closed
geoand opened this issue Dec 18, 2019 · 37 comments

Comments

@geoand
Copy link
Contributor

geoand commented Dec 18, 2019

As mentioned by @dmlloyd I am creating a separate issue for the comment I made here:

The use case I have in mind is to be able to use configuration from some source in order to then be able to instantiate a class that will create another source based on some data it will fetch (most likely over the wire)

@geoand
Copy link
Contributor Author

geoand commented Dec 18, 2019

@dmlloyd is there any similar work that has been done that you know about (in order to base this on) or should I start experimenting?

@dmlloyd
Copy link
Contributor

dmlloyd commented Dec 18, 2019

The solution I was really thinking about in the context of Quarkus is to have an additional configuration phase TBH. But alternatively, there is an API in SmallRye Config that allows configuration sources to be added after the fact. It currently doesn't support wrappers though; maybe that support could be added.

@geoand
Copy link
Contributor Author

geoand commented Dec 18, 2019

Interesting! Which strategy would you propose I explore?

@dmlloyd
Copy link
Contributor

dmlloyd commented Dec 18, 2019

I think adding additional sources at run time is the most achievable goal. Without the wrapper support, the added config source won't have support for property expansion or profiles, but that might not be important. If you do decide to work out wrapper support, then I'd recommend that you add an overloaded method which accepts a boolean indicating whether wrappers are to be applied.

@geoand
Copy link
Contributor Author

geoand commented Dec 18, 2019

Thanks @dmlloyd. I'll start looking into this tomorrow but with the holidays approaching I don't know when I'll have anything worthwhile to get back to you with.

@geoand
Copy link
Contributor Author

geoand commented Jan 2, 2020

I haven't had time to look into this yet unfortunately

@geoand
Copy link
Contributor Author

geoand commented Jan 6, 2020

I was thinking about this over the weekend and the more I ponder it, the more believe that having an additional configuration phase in Quarkus is the way to go.
I am thinking that we would have a new (for example "bootstrap") phase which would be configurable via a bootstrap.properties file which would be used to drive whatever implementation is needed.
What each implementation would do is simply query the remote configuration store and when the properties are retrieved, Quarkus would create a new ConfigSource (containing the retrieved properties) and configure SmallRyeConfig to use it in addition to all the other ConfigSource objects.

@dmlloyd does that sounds reasonable?

@dmlloyd
Copy link
Contributor

dmlloyd commented Jan 6, 2020

It makes sense, but I suspect that having a separate configuration file for that phase is going to backfire in multiple ways; particularly, it would be strange to have build time and run time config properties in one file but then have "in between" properties in some other file.

If we could just mark some config roots as belonging to the earlier phase, then the corresponding configuration objects could be made available to steps which run before the main configuration is read. Then the only effective difference between the bootstrap configuration phase and the runtime phase is that the former has some properties that cannot be specified by configuration sources which are configured by that phase.

The bootstrap configuration parser would need to ignore run time properties, but we already have logic and support for doing that since we do so already for build time properties.

@geoand
Copy link
Contributor Author

geoand commented Jan 6, 2020

OK thanks! I'll start working on it when we get back from f2f

@geoand
Copy link
Contributor Author

geoand commented Jan 13, 2020

We also need some contract or SPI that will tie in the bootstrap configuration with whatever code actually consumes that configuration to finally provide some new config values. This is something new to this phase and I really think we should try and get it right in order to make this bootstrap phase generally usable and easy to implement by other implementations (aside from the initial Spring Config Server one).

I haven't thought of it yet, I'm still pondering different options in my head. Any ideas would be welcome :)

@dmlloyd
Copy link
Contributor

dmlloyd commented Jan 13, 2020

Here's a question: do we want config sources to load at the earlier or later stage by default? If we load at the earlier stage by default, we'd have to explicitly specify that configurable sources be delayed, which makes some sense... but there might be more than expected. For example the filesystem-based application.properties might be loaded later, because the command line could specify an alternate location for it.

The question of how the source is delayed is a bit harder. Normally we're using service loader to load all run time sources at the same time, so that would need something of an overhaul. But I'm not sure it's realistic to expect providers to use anything other than the current existent mechanism. We might have to specify some kind of filter or predicate based system to determine which sources should be made available at which time. This is also complicated by the fact that there is more than one type used to load config sources (the ConfigSource type itself and also ConfigSourceProvider).

@geoand
Copy link
Contributor Author

geoand commented Jan 13, 2020

My (probably simplistic) idea was to create an MP Config object based on the application.properties, the env vars and system properties used, use that to populate the bootstrap config object which would then be pushed to whatever boostrap code handles it and get a list of initialized MP ConfigSource objects as a result (this MP Config would also be released once the MP ConfigSources are initialized).
This would be done before the runtime config properties are handled, and the result of the process above would just contribute MP ConfigSource objects to the runtime properties handling (by adding them to the SmallryeConfigBuilder).

Now I admit I hand't thought of the command-line properties at all but I assume that when we have support for them, they would fit in to the first part of the model I described.

@dmlloyd
Copy link
Contributor

dmlloyd commented Jan 13, 2020

So you propose to discontinue discovery of configuration sources altogether, and require that sources be explicitly specified (by extensions) for both phases?

@geoand
Copy link
Contributor Author

geoand commented Jan 13, 2020

Well not exactly, the way I see it the runtime phase would be the same as it is now (with the addition of the sources that were resolved from the bootstrap phase).
The bootstrap phase I think should be limited to just the file system properties, the env vars and system properties (with the addition of the command-line properties when we have them).

I have looked at the config internal but I am by no means an expert, so if what I'm saying sounds off, then it's probably because it is :)

@dmlloyd
Copy link
Contributor

dmlloyd commented Jan 13, 2020

I'd suggest that the file system properties be loaded at the later stage. So at the early stage we'd have:

  • In-JAR application.properties and microprofile-config.properties, and their YAML equivalents
  • Build time config value sources
  • Generated BT & RT default value sources
  • Command-line properties (when we have them)
  • Env vars
  • System properties
  • No config source discovery

At the second stage we'd have:

  • All of the above sources which are configured during the first phase
  • Filesystem application.properties (I don't think we have support for filesystem microprofile-config.properties but if I'm wrong we'd load that here too) and YAML equivalent(s)
  • RunTimeConfigSourceBuildItem (currently this seems to amount to vault only; maybe we should change this to just use discovery)
  • All discovered sources (ServiceLoader)

The config source wrappers would apply at both stages, as would converters (because they're not configurable).

Did I miss anything you can think of?

@geoand
Copy link
Contributor Author

geoand commented Jan 13, 2020

Just one question, by discovered you mean all sources discovered via ServiceLoader, correct?

@geoand
Copy link
Contributor Author

geoand commented Jan 13, 2020

So your proposal means that the bootstrap stuff will be discovered via ServiceLoader, right?

@dmlloyd
Copy link
Contributor

dmlloyd commented Jan 13, 2020

So your proposal means that the bootstrap stuff will be discovered via ServiceLoader, right?

No, the opposite; the bootstrap stuff (the first phase) would all be explicit and the later phase would also include the ServiceLoader stuff.

@geoand
Copy link
Contributor Author

geoand commented Jan 13, 2020

Okay got it.

Then I would that that in the proposal of the second stage we should add "whatever sources are created from the first phase".
That's what confused me I think.

@dmlloyd
Copy link
Contributor

dmlloyd commented Jan 13, 2020

Updated.

@geoand
Copy link
Contributor Author

geoand commented Jan 13, 2020

Thanks!

@dmlloyd
Copy link
Contributor

dmlloyd commented Jan 13, 2020

I guess this is more of a Quarkus discussion than a SmallRye Config discussion at this point. Sorry about that :)

@geoand
Copy link
Contributor Author

geoand commented Jan 14, 2020

@dmlloyd is there a way perhaps you can move the issue to Quarkus? I know I certainly can't but maybe you can? Or can issues only be moved between repos of the same org perhaps...

@geoand
Copy link
Contributor Author

geoand commented Jan 14, 2020

The previous discussion still leaves out how we would discover / take into account the code that will consume the phase 1 config.
We need a way to identify this code (a dedicated build item perhaps) so it can then be invoked with the phase 1 config and produce some new sources that will be used in phase 2.

@geoand
Copy link
Contributor Author

geoand commented Jan 14, 2020

I think I need to read up on the discussion at eclipse/microprofile-config#470 since it seems to be related

@dmlloyd
Copy link
Contributor

dmlloyd commented Jan 14, 2020

Unfortunately I cannot move the issue outside of the smallrye org.

@dmlloyd
Copy link
Contributor

dmlloyd commented Jan 14, 2020

I don't think we need to do anything special (from a user perspective) to allow injection of phase 1 config. I think we can calculate the dependency automatically. We'd just need a build item for phase 1 config sources, and that's about it I think.

@geoand
Copy link
Contributor Author

geoand commented Jan 14, 2020

Unfortunately I cannot move the issue outside of the smallrye org.

that's too bad...

@geoand
Copy link
Contributor Author

geoand commented Jan 14, 2020

So let's say I have the following configuration:

@ConfigRoot(phase = ConfigPhase.BOOTSTRAP)
public class SomeServiceConfiguration {
    public String url;
}

Then we have some code such as the following that consumes the configuration and produces a list of ConfigSource objects like:

public class SomeServiceGateway {

    public List<ConfigSource> resolve(SomeServiceConfiguration configuration) {
       // do whatever
    }
}

I assume that we expect SomeServiceGateway will be "registered" in Quarkus with some new BuildItem and that will associate SomeServiceGateway with SomeServiceConfiguration (perhaps with a MethodInfo from Jandex).
Then Quarkus would (after performing the necessary validation) generate code to invoke SomeServiceGateway#resolve (the generated output would be placed in between the creation of the Boostrap Config and the Runtime Config - which would obviously then additionally use the List<ConfigSource> result).

How does this sound?

@dmlloyd
Copy link
Contributor

dmlloyd commented Jan 14, 2020

I would not do it substantially differently from the way run time configuration is currently done. We do need a new config phase, but it should be possible to have a regular run time recorded build step that consumes a bootstrap config and produces a source build item, something like this:

    @BuildStep
    @Record(ExecutionTime.RUNTIME_INIT)
    RunTimeConfigurationSourceValueBuildItem setUpNewConfigSource(SomeServiceConfiguration config, MyRecorder recorded) {
       return new RunTimeConfigurationSourceValueBuildItem(recorder.setUpConfig(config));
    }

and then the recorder method would be:

@Recorder
public class MyRecorder {
    // ...
    public RuntimeValue<ConfigSourceProvider> setUpConfig(SomeServiceConfiguration config) {
        return new RuntimeValue<>(...);
    }
}

The setup code in ExtensionLoader should already be flexible enough to automatically order the steps correctly, but some new build items will be needed for the bootstrap config and the main method step will have to be updated as well. The run time config generator will also have to generate separate parse methods for the bootstrap configs.

@geoand
Copy link
Contributor Author

geoand commented Jan 14, 2020

I'm not at my machine right now, but reading the above I have a question:

Normally the recorders are "run" after all the runtime config has been created. In this case we need to have only the recorders associated with the bootstrap config run before the runtime config (but of course use the bootstrap config stuff).
So I assume that by changing the main class generation, among other things you mean that we should detect the bootstrap related recorders and run the in the appropriate place?

@dmlloyd
Copy link
Contributor

dmlloyd commented Jan 14, 2020

I'm not at my machine right now, but reading the above I have a question:

Normally the recorders are "run" after all the runtime config has been created. In this case we need to have only the recorders associated with the bootstrap config run before the runtime config (but of course use the bootstrap config stuff).
So I assume that by changing the main class generation, among other things you mean that we should detect the bootstrap related recorders and run the in the appropriate place?

Yes, one way or another. One possibility might be to move the existing run time config setup back into a build step so that dependency resolution can work out the order.

@geoand
Copy link
Contributor Author

geoand commented Jan 14, 2020 via email

@geoand
Copy link
Contributor Author

geoand commented Jan 22, 2020

Yes, one way or another. One possibility might be to move the existing run time config setup back into a build step so that dependency resolution can work out the order.

I would like to know what the reason was for moving the config setup out of the build step in the first place before contemplating whether it should be moved back in :)

@geoand
Copy link
Contributor Author

geoand commented Jan 30, 2020

* `RunTimeConfigSourceBuildItem` (currently this seems to amount to vault only; maybe we should change this to just use discovery)

Yeah, when we have the bootstrap configuration this should definitely go away...

@geoand
Copy link
Contributor Author

geoand commented Feb 10, 2020

@dmlloyd I suggest we close this as we took care of it in Quarkus. OK?

@dmlloyd
Copy link
Contributor

dmlloyd commented Feb 10, 2020

Sounds good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants