moditect / layrry

A Runner and API for Layered Java Applications
Apache License 2.0
332 stars 33 forks source link

Support additional property sources during configuration #45

Closed aalmiray closed 3 years ago

aalmiray commented 3 years ago

During a brainstorm for removing the explicit runner found in https://github.com/moditect/layrry-examples/tree/master/modular-tiles I thought it might be a good idea to provide the Layrry launcher with an addition source of properties that can be used for placeholder substitution, something akin to

`java -jar layrry-launcher.jar --layers-config layers.yml --properties layrry.properties`

The launcher would then read the properties file and place all properties into the Mustache scope, allowing placeholder substitutions. This could turn a somewhat static Layers config file into a "self updatable" config source, as [groupId, artifactId, version] could be provided by external properties. No need to provide a similar layers.yml file to update a version (say from groovy-json:3.0.5 to groovy-json:3.0.6) but also may handle artifact relocation.

Example: Groovy 3 is currently published with groupId = org.codehaus.groovy. Groovy 4 will change to groupId = org.apache.groovy.

A layers file that requires Groovy could declare a module definition as

"{{groovy.groupId}}:groovy-json:{{groovy.version}}"

Explicit definition of an additional properties source makes sense for locally launched applications but may pose a security risk with remotely launched applications (see #37) where you'd want the property source to come from the same trusted host (and adjacent location perhaps) as the layers config file. An implicit mode could be implemented

  1. Given the path of the config layers file replace the filename extension with .properties. If a match is found, use it.
  2. Given the path of the config layers file, locate a file named layrry.properties. If a match is found, use it.
  3. If there are no matches in 1) and 2) then no implicit property sources will be used.

Of course, explicit declaration such as --properties my-props.properties win over implicit, such that if the explicit file is not found then no implicit search will be performed.

gunnarmorling commented 3 years ago

Ha, funny, this is pretty much what I wanted to capture in an issue to 👍 . This makes all lots of sense to me. I still think being able to set properties into the Mustache context via system properties is reasonable and not something we should drop. We'd only have to decide on the priority in case a variable is given both as system property and within a properties file. Re your resolution algorithm above, perhaps let's just drop step 2 for now? Trying to keep things simple :)

aalmiray commented 3 years ago

OK, let's drop 2) for now.

Regarding potential conflicts with System properties:

  1. layers.properties is the source of truth and overrides any matching System properties.
  2. System properties may override values from layers.properties, allowing for quick fix/tests without changing files (files coming from remote sources perhaps) with -Ddep.version=1.2.3.
  3. Layrry could offer a feature (in API) for custom Launchers that disallow System overrides or allow partial System overrides.

Case 1) treats the app config files as final. That's what the developer intended you to consume. Case 2) lets you tweak values without touching files. things might break but you're on your own here. Case 3) A compromise between 1) and 2) that lets the producer define the rules.

gunnarmorling commented 3 years ago

+1 for 1 and 2. Use case for 3 still is a bit blurry to me. I like simple :)

aalmiray commented 3 years ago

But the thing is you can either get 1 & 3, or 2 & 3. 1 & 2 are exclusive with one another. Or another way to see it, pick 1 or 2, skip 3.

gunnarmorling commented 3 years ago

Ah, sorry, I misread. So I like 2 best: system properties override the properties file; I'd leave 3 for later.

aalmiray commented 3 years ago

OK, to summarize:

Finding additional sources

  1. support explicit property definition via --properties.
  2. if no explicit --properties is given then assume <layerFileName>.properties. Use it if found.

Handling definition priority

  1. Values from explicit/implicit properties (if any)
  2. Values from detected OS
  3. JavaFX classifier goes
  4. System properties

The override chain is then 4 > 3 > 2 > 1 allowing System properties the maximum preference and explicit/implicit properties the lowest.

gunnarmorling commented 3 years ago

Sounds great; only that I think it should be 4 > 1 > 3 > 2; i.e. I'd consider any variables given in the properties file more important than what's implicitly derived from the environment.

aalmiray commented 3 years ago

That could potentially break things. If OS is detected as "windows" and explicit props say javafx.os.classifier is 'mac' then you can only fix it with an explicit System prop. With 4> 3> 2 > 1 you reduce the chances of having an accident like the one described. Of course I can still create havoc with -Djavafx.os.classifier=boom.

Which makes me think perhaps 3 & 2 > 4 > 1 may be the way to go.

gunnarmorling commented 3 years ago

explicit props say javafx.os.classifier is 'mac'

The question is why would you set this explicitly in a properties file? Obtaining that platform classifier in a portable way really is the purpose for these implicitly provided variables. What other properties are implicitly provided via that Maven plug-in? Does the order of 1 vs. 3/2 match really in the end, as the sets of properties provided there should be distinct really in practice? In any case I think it's in the spirit of the principle of least surprise to give precedence to any explicitly given variables.

gunnarmorling commented 3 years ago

Which makes me think perhaps 3 & 2 > 4 > 1 may be the way to go.

But then we'd have no way whatsoever to override the implicitly provided values. Say the JavaFX artifacts change the classifier they use (unlikely, I know...), then one would need a new release of Layrry to accomodate for that, where otherwise an override in the properties file or system property could be done by the user.

aalmiray commented 3 years ago

The question is not why would you explicitly set javafx.os.classifier in the properties file but rather, can you do that? Because you know someone will try whether they are aware of the consequences or not. Also thinking in potential security or injection risks. I'd prefer detected OS properties to be left outside of the set of properties that can be manipulated by the consumer.

aalmiray commented 3 years ago

Yes, the JavaFX classifier are for now the only magic values Layrry has to explicitly manage. Those 3 properties are the only anchors. OS properties will be provided by the environment so we should trust them, every other property is fair game.

gunnarmorling commented 3 years ago

Ok, so I think that giving the user the ability to override any implicit properties would be the right thing. But after all, we always can loosen the rules later on and allow for such override if the requirement comes up (whereas stricten the rules later on would be more disruptive). So we can go as you suggest, if you feel strongly about it.

aalmiray commented 3 years ago

Thanks for understanding, I've got a gut feeling that 3&2 > 4 > 1 is good enough where as 4 > 2&3 > 1 is too risky. As you said, if we need to loosen up at a later date then 4 > 2&3 > 1 is the way to go.

gunnarmorling commented 3 years ago

I've got a gut feeling that 3&2 > 4 > 1 is good enough

👍

if we need to loosen up at a later date then 4 > 2&3 > 1 is the way to go.

I still think it should then be 4 > 1 > 2&3, but let's cross that bridge when we get there :)

gunnarmorling commented 3 years ago

Done.