Add a config.strategy to enable overwrite of properties from env vars #620

andreaTP · 2019-02-27T12:53:19Z

This PR propose to introduce an alternative config.strategy to enable the overwrite of configurations entries with environment variables even if a substitution is not explicitly provided in the configuration files.

Rationale:

We want to be able to dynamically override potentially any configuration option with environment variables.
The current env variable substitution mechanism is verbose and have to be explicitly defined for every configuration ahead of time.
more on the topic is detailed here: https://github.com/codacy/env2props#rationale

Unfortunately the approach taken in env2props has a big drawback, getting the bash string substitution correct is not trivial and is error-prone.

Here I'm proposing to have an alternative implementation of the config.strategy that allow to obtain the very same behavior.

Notes:

the mangling of env vars is needed because in a default bash environment:

$ export A.B=1
bash: export: `A.B=1': not a valid identifier
$ export A-B=1
bash: export: `A-B=1': not a valid identifier

havocp · 2019-03-01T15:09:19Z

Useful context from gitter chat is that we're wanting to allow setting a JVM property flag one time, and then a DevOps or Ops person could add new overrides via environment variables at any time, without editing the config file itself. This is useful in cases like Docker or Heroku or Kubernetes which are often configured via env var.

Possible requirements:

this should be opt-in somehow (shouldn't start importing all env vars as overrides) for back compat reasons
should be able to override any config value no matter what's in the config file

I feel like the "strategy" notion is a little non-orthogonal (it potentially bundles together multiple behaviors), thinking it could be easier to think about this as a feature which is on or off.

One way it could look is:

by default, it stays how it is now
if you set -Dconfig.override_with_env_vars_prefix=CONFIG_FORCE_ then any env vars starting with CONFIG_FORCE_ are added to ConfigFactory.defaultOverrides

Possible questions:

is it really needed to have -Dconfig.override_with_env_vars_prefix defaulting to null, or could we simply always look at CONFIG_FORCE_ and figure nobody would have env vars starting with that right now
instead of a configurable prefix we could have a boolean and it's always the same prefix

The escaping situation is a little bit tough; config (and JSON) allow keys to be about anything, but many characters are awkward in an env variable. The PR so far invents a novel escaping mechanism to allow ., -, and _, which would need to be documented / isn't obvious / doesn't cover all characters such as spaces... I'm not loving any of the solutions to this that I can think of.

An option we've discussed in the past is an env var whose value would be parsed as HOCON and made into overrides, e.g. on #427 we mentioned it; an advantage to this is that values can also be unset and that any arbitrary key is possible. See #582

The downside to CONFIG_OVERRIDES="{ foo: null }" is that it's pretty annoying if you just want to set one key and don't want to figure out how to escape it etc.

It's possible we should do a combined approach? Like all CONFIG_FORCE_* env vars get added to defaultOverrides(), and also the value of CONFIG_OVERRIDES= is parsed as a document and also added to defaultOverrides() ? and then we don't support any fancy escaping or hard stuff with single CONFIG_FORCE, it only does simple cases, and the CONFIG_OVERRIDES mechanism is there for hard cases.

(minor detail if we did go that road maybe we should choose between FORCE and OVERRIDES for both, like CONFIG_OVERRIDE_* and CONFIG_OVERRIDES, instead of calling one force and one override.)

with the prefixes in the CONFIG_ namespace it kinda seems OK to me to enable this by default without a JVM flag...

andreaTP · 2019-03-01T17:00:03Z

I like your proposal and I will re-work this PR next week to follow.
On the trigger I personally prefer the option of having a simple boolean to enable it and fix the prefix to CONFIG_FORCE_, this looks the simpler and safer.

On the invents a novel escaping you are completely right but I'm short on alternatives, it is currently documented here: https://github.com/lightbend/config/pull/620/files#diff-23287583c575f1193e540c940d690b6aR11 but I'm open to other options as I don't love it as well, to note that this limitation is imposed by usability in bash and terminals in general since A.B or A-B are totally valid environment variables. This is a common issue (e.g. moby/moby#16585 (comment))

I personally don't love the CONFIG_OVERRIDES approach since writing json, hocon etc. in env variables is extremely annoying, un-setting keys looks to be the only valid use case, and looks pretty niche. Still I would prefer to have a single approach to the problem instead of 2 overlapping features.

with the prefixes in the CONFIG_ namespace it kinda seems OK to me to enable this by default without a JVM flag...

this is an assumption actually, debugging an issue caused by this will be incredibly hard and counter-intuitive, I will start with a flag and in case we remove it if there is enough community endorsement.

andreaTP · 2019-03-04T14:45:30Z

@havocp please have a look at the updated implementation.

dwijnand · 2019-03-06T15:27:03Z

👨‍🎨 Could the prefix be HOCON_ instead?

(I still believe we should rebrand this library as "hocon" - #514)

eed3si9n · 2019-03-06T15:29:13Z

I am not sure about the implementation details but the idea of ops person being able to override things via environment variables seems consistent with Twelve-Factor App (https://12factor.net/config), Kubernetes, etc ethos.

andreaTP · 2019-03-07T08:59:54Z

Thinking twice at it, having a prefix like HOCON_FORCE_ or HOCON_OVERRIDE_ we can most likely get rid of the JVM flag and enable it by default.

dwijnand · 2019-03-07T09:29:58Z

(Still in 👨‍🎨-mode) I'd rather avoid the extra FORCE/OVERRIDE.

For example, I'd love if I could specify with an environment variable akka discovery's method with just:

export HOCON_AKKA_DISCOVERY_METHOD=akka-dns

havocp · 2019-03-07T13:22:32Z

If there’s nothing after HOCON then we can’t use that namespace for any other purpose (env vars that affect the library itself for example). Maybe that’s OK.

It feels a bit odd to me to use this namespace here and config everywhere else. These env vars technically have nothing to do with HOCON, in fact if we stripped the library down to only json and properties and env var support, this feature would still work.

andreaTP · 2019-03-07T13:50:46Z

also your points are valid @havocp , to not stall the PR on this can we make it configurable via java properties and have a longish default like JAVA_CONFIG_OVERWRITE_ ?

dwijnand · 2019-03-07T14:15:06Z

if we stripped the library down to only json and properties and env var support, this feature would still work

(Sure, but I identify this library as the hocon library. 😄)

havocp

Might be helpful to add a note to the README or other docs where config.* props are already mentioned, about this new -Dconfig.override_with_env_vars

Overall I feel like this is good. I like the CONFIG_FORCE_ naming I guess, though I don't like the overly-generic name of the entire library, I don't know that we should address that piecemeal in this context. :-)

I'm fine with leaving this as opt-in behavior, at least to start. We could always make it the default down the road, I suppose.

havocp · 2019-03-07T19:04:18Z

config/src/main/java/com/typesafe/config/ConfigFactory.java

@@ -383,7 +384,11 @@ public static Config defaultReference(ClassLoader loader) {
     * @return the default override configuration
     */
    public static Config defaultOverrides() {
-        return systemProperties();
+        if (!getOverrideWithEnv()) {


Think it's easier to read if you reverse the if/else to avoid the !

havocp · 2019-03-07T19:05:49Z

config/src/main/java/com/typesafe/config/ConfigFactory.java

+    private static Boolean getOverrideWithEnv() {
+        String overrideWithEnv = System.getProperties().getProperty(OVERRIDE_WITH_ENV_PROPERTY_NAME);
+
+        return Boolean.parseBoolean(overrideWithEnv);


if the value of the property is not a boolean this will throw an ugly and likely-mysterious exception right? might want to catch it and say something like "the value of OVERRIDE_WITH_ENV_PROPERTY_NAME must be a boolean not '%s'"

Not sure if we already have this bug when looking at any other env var values

I've deliberately choose to use Boolean.parseBoolean because it doesn't throw exceptions.
Basically it always return false unless the string is (case-insensitive) true.

havocp · 2019-03-07T19:06:49Z

config/src/main/java/com/typesafe/config/ConfigFactory.java

+     * This method can return a global immutable singleton, so it's preferred
+     * over parsing system properties yourself.
+     * <p>
+     * {@link #defaultOverrides} will include the system system environment variables as


Suggested change

* {@link #defaultOverrides} will include the system system environment variables as

* {@link #defaultOverrides} will include the system environment variables as

havocp · 2019-03-07T19:11:34Z

Oh and thank you for working on this!

andreaTP · 2019-03-08T14:56:44Z

@havocp I followed your review and added a little section to the Readme feel free to suggest and re-wording and/or ask to move that section around.
Ready for next round!

andreaTP · 2019-03-14T08:58:18Z

@havocp ping?

andreaTP · 2019-03-19T10:11:16Z

For anyone else interested on the topic, I published Env4Config:
https://github.com/codacy/env4config
Based on the first implementation of this PR.
Until this PR find it's way to upstream it's a road unblocker.

as a side note (maybe for @dwijnand ) I found really misleading the fact that Play doesn't always load configuration in a standard way:
https://github.com/playframework/playframework/blob/ab6697f494a000ba8d19768e9d04eed3bb8cdbf7/core/play/src/main/scala/play/api/Configuration.scala#L73
The workaround there is to use config.url that fortunately is not re-mapped in the Play codebase, not sure if I should open an issue there or something ...

dwijnand · 2019-03-19T11:13:51Z

@andreaTP #619 which also about Play's difference in config loading.

havocp

I noticed one more thing that invalidateCaches should cover the new cached thing, but otherwise 💯
lgtm

havocp · 2019-03-20T23:24:55Z

README.md

+i.e. The environment variable `CONFIG_FORCE_a_b__c___d` set the
+configuration key `a.b-c_d`
+
+Rationale the name mangling:


I'd probably leave this section out, if people really care they can look in the code or commit messages (maybe it's worth moving it to one of those places)

moved to a comment in the code

havocp · 2019-03-20T23:28:00Z

config/src/main/java/com/typesafe/config/impl/ConfigImpl.java

+    }
+
+    private static class EnvVariablesOverridesHolder {
+        static volatile AbstractConfigObject envVariables = loadEnvVariablesOverrides();


I guess we need to make ConfigFactory.invalidateCaches clear this

config/config/src/main/java/com/typesafe/config/ConfigFactory.java

Line 495 in 64c60f0

public static void invalidateCaches() {

andreaTP · 2019-03-21T08:54:06Z

ready for next round :-)

andreaTP · 2019-03-21T08:58:56Z

Need to re-trigger the CI

andreaTP · 2019-03-21T09:08:34Z

Not sure why, but Travis was failing to download sbt-launcher 1.2.7, bumping it to 1.2.8 seems to solve the issue.

aaronp · 2019-03-21T09:32:31Z

config/checkstyle-config.xml

@@ -1,7 +1,7 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <!DOCTYPE module PUBLIC
    "-//Puppy Crawl//DTD Check Configuration 1.3//EN"


Shell we update/fix the 'Puppy Crawl' comment as well?

aaronp · 2019-03-21T09:32:57Z

config/checkstyle-suppressions.xml

@@ -1,6 +1,6 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <!DOCTYPE suppressions PUBLIC "-//Puppy Crawl//DTD Suppressions 1.1//EN"


And here still revers to Puppy Crawl as well

aaronp · 2019-03-21T09:42:07Z

config/src/main/java/com/typesafe/config/impl/ConfigImpl.java

@@ -360,6 +361,71 @@ public static void reloadEnvVariablesConfig() {
        EnvVariablesHolder.envVariables = loadEnvVariables();
    }

+    private static AbstractConfigObject loadEnvVariablesOverrides() {
+        Map<String, String> env = new HashMap(System.getenv());


I don't mean to bike shed -- especially as I've seen you've written tests against this. I just though I'd add a suggestion which could (1) perhaps make the different override cases more easily scannable/readable and (2) make the test cases easier:

Instead of conflating the 'System.getenv()' call within the same method which does the override logic, you might consider having something like:

static String envVariableAsProperty(envVariable : String, envVarOverridePrefix : String) : String = {
... your code from line 377 here
}

which you can then just test via:

assertEquals(envVariableAsProperty("PREFIX_A_B__C", "PREFIX_"), "a.b-c")
assertEquals(envVariableAsProperty("PREFIX_A_B___C", "PREFIX_"), "a.b_c")

etc

it makes totally sense, thanks for pushing me out from my laziness :-)

aaronp

Great work - thanks for (1) fixing the puppy crawl issue and (2) adding support for this feature, which obviates the need for my similar, but not as good implementation of the same thing in 'args4c'!

andreaTP · 2019-03-21T10:56:44Z

thanks for the review @aaronp ! ready for next round! :-)

aaronp

Great work!

havocp · 2019-04-03T23:27:32Z

Thanks! Sorry I'm so slow on the reviews.

TimMoore · 2019-04-04T02:56:27Z

@havocp I just wanted to say thank you for continuing to review and merge pull requests at all!

andreaTP · 2019-04-04T06:27:56Z

Thanks @havocp ! any ETA on the next release?

mkurz · 2020-05-02T13:37:04Z

@andreaTP Seems like there is an issue with this feature, please see this bug report: playframework/playframework#10206

andreaTP added 3 commits February 27, 2019 12:37

Add a config strategy to load from environment variables first.

d9d1f0a

Fix suppressions doctype

5c04377

Trailing blank line

30b6556

Env variables resolution moved to defaultOverrides

7fe4788

havocp approved these changes Mar 7, 2019

View reviewed changes

havocp mentioned this pull request Mar 8, 2019

add new Rust port to Other APIs section #621

Merged

andreaTP added 2 commits March 8, 2019 14:26

fixed review comments

e302b9a

Added a description of the new property in Readme

c14bb77

ekrich mentioned this pull request Mar 8, 2019

Port - Add a config.strategy to enable overwrite of properties from env vars ekrich/sconfig#17

Closed

Detail resons for _ sustitutions

4571c79

havocp approved these changes Mar 20, 2019

View reviewed changes

Move rationale to code and implement invalidate cache

87dc17d

andreaTP closed this Mar 21, 2019

andreaTP reopened this Mar 21, 2019

Bump sbt version

3a9a1d8

aaronp reviewed Mar 21, 2019

View reviewed changes

andreaTP added 4 commits March 21, 2019 10:01

Remove Puppy Crawl references

90e2465

Move env mangling to ConfigImplUtil

4937ee4

minor fix

fd0e418

fix again XML

4fad113

aaronp approved these changes Mar 21, 2019

View reviewed changes

havocp approved these changes Apr 3, 2019

View reviewed changes

havocp merged commit e24ff57 into lightbend:master Apr 3, 2019

2m added this to the 1.3.4 milestone Apr 18, 2019

mkurz mentioned this pull request Apr 18, 2019

Update typesafe:config to 1.3.4 playframework/playframework#9201

Merged

mkurz mentioned this pull request May 11, 2020

Fix: Avoid loading all the environment variables as configurations #686

Merged

mproch mentioned this pull request Aug 13, 2020

[Improvement] Upgrade typesafe config TouK/nussknacker#1151

Merged

dependabot bot mentioned this pull request Mar 18, 2021

Bump config from 1.3.3 to 1.4.1 checkmarx-ltd/checkmarx-spring-boot-java-sdk#105

Merged

mkurz mentioned this pull request Nov 12, 2021

CONFIG_FORCE_: Environment variables appear twice in Configuration playframework/playframework#10206

Closed

ekrich mentioned this pull request Jun 16, 2023

Port all non documentation commits from typesafe/config ekrich/sconfig#29

Open

12 tasks

	* {@link #defaultOverrides} will include the system system environment variables as
	* {@link #defaultOverrides} will include the system environment variables as

		@@ -1,6 +1,6 @@
		<?xml version="1.0" encoding="UTF-8"?>
		<!DOCTYPE suppressions PUBLIC "-//Puppy Crawl//DTD Suppressions 1.1//EN"

Add a config.strategy to enable overwrite of properties from env vars #620

Add a config.strategy to enable overwrite of properties from env vars #620

Conversation

andreaTP commented Feb 27, 2019

Rationale:

Notes:

havocp commented Mar 1, 2019

andreaTP commented Mar 1, 2019

andreaTP commented Mar 4, 2019

dwijnand commented Mar 6, 2019 • edited Loading

eed3si9n commented Mar 6, 2019

andreaTP commented Mar 7, 2019

dwijnand commented Mar 7, 2019

havocp commented Mar 7, 2019

andreaTP commented Mar 7, 2019

dwijnand commented Mar 7, 2019

havocp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

havocp commented Mar 7, 2019

andreaTP commented Mar 8, 2019

andreaTP commented Mar 14, 2019

andreaTP commented Mar 19, 2019

dwijnand commented Mar 19, 2019

havocp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreaTP commented Mar 21, 2019

andreaTP commented Mar 21, 2019

andreaTP commented Mar 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronp left a comment

Choose a reason for hiding this comment

andreaTP commented Mar 21, 2019

aaronp left a comment

Choose a reason for hiding this comment

havocp commented Apr 3, 2019

TimMoore commented Apr 4, 2019

andreaTP commented Apr 4, 2019

mkurz commented May 2, 2020

dwijnand commented Mar 6, 2019 •

edited

Loading