Override config.php values through environment variables #3966

MorrisJobke · 2017-03-21T03:13:10Z

added functionality to override config.php values with 'OC_' prefixed environment variables
use getenv to read environment variables since apache does not set $_ENV variables, fixed test
fixes Read Env variables to configure Nextcloud #3926
downstream of Override config.php values through ENV variables owncloud/core#26570

@LukasReschke @nickvergessen @rullzer @karlitschek Should we rename the env variables to NC_ or should leave them?

mention-bot · 2017-03-21T03:13:12Z

@MorrisJobke, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nickvergessen, @LukasReschke and @bartv2 to be potential reviewers.

karlitschek · 2017-03-21T03:17:41Z

i think it should be nc_

MorrisJobke · 2017-03-21T03:30:52Z

i think it should be nc_

Your wish shall be granted 🎆

ChristophWurst · 2017-03-21T07:04:20Z

lib/private/Config.php

 	 *
 	 * @param string $key key
 	 * @param mixed $default = null default value
 	 * @return mixed the value or $default
 	 */
 	public function getValue($key, $default = null) {
+		$envValue = getenv(self::ENV_PREFIX . $key);
+		if ($envValue) {


from http://php.net/manual/en/function.getenv.php

Returns the value of the environment variable varname, or FALSE if the environment variable varname does not exist.

hence I would suggest to explicitly check for that with if ($envValue !== false). Not sure which string values PHP considers truthy, but to me this looks error-prone otherwise.

Yes, also all int values we have with 0 (log level and rotation, ... ) can not be set otherwise

Good point. Do we need casting here? E.g. if you set NC_LOGLEVEL=0, it will return the string "0".

What about values that should actually be set to false 🙈

getenv will always return a string, according to the documentation, so in case of NC_xxx=false it returns "false" (string), not false (bool) 😉

But for those options we also rely on the fact that there is actual boolean returned if there is a boolean in the config.php 🙈

Fixed and added unit tests.

Yup, that's the same problem as in #3966 (comment). Can we handle this somehow? We don't know what the consuming code expects 🤔

nickvergessen

As per above

icewind1991 · 2017-03-21T13:12:10Z

lib/private/Config.php

 	 *
-	 * If it does not exist, $default will be returned.
+	 * gets its value from an `OC_` prefixed environment variable


OC -> NC

tests/lib/ConfigTest.php

Fixed

MorrisJobke · 2017-03-22T22:12:31Z

@rullzer @icewind1991 @nickvergessen All issues addressed - ready for review

MorrisJobke · 2017-03-22T22:12:40Z

👍 from me on this

icewind1991 · 2017-03-23T14:54:51Z

I feel that it would be better to try and case non-string values to their proper types.

maybe try a json_decode and return original string if that fails.

That would also allow settings things like arrays trough ENV

nickvergessen · 2017-03-23T15:37:10Z

lib/private/Config.php

 	 *
 	 * @param string $key key
 	 * @param mixed $default = null default value
 	 * @return mixed the value or $default
 	 */
 	public function getValue($key, $default = null) {
+		$envValue = getenv(self::ENV_PREFIX . $key);


Should we cache this as well? I think it makes sense. otherwise we might call this hunderd times

The only problem is: How do we know that we need to cache a ENV variable? I can move it below the cache statement, but then the ENV variable would not overwrite config.php values. I guess this is fine. Don't set the config option in config.php, but only in the ENV and it will be used from there.

Implemented like this.

Could also just add $this->envCache

But this would not solve anything for empty env variables, because they are not cached then and the getenv would happen all the time for them.

nickvergessen · 2017-03-24T10:15:58Z

lib/private/Config.php

 		if (isset($this->cache[$key])) {
 			return $this->cache[$key];
 		}

+		$envValue = getenv(self::ENV_PREFIX . $key);
+		if ($envValue !== false) {
+			$this->cache[$key] = $envValue;


The problem is, that now since the value is in cache, it will be written to the disk when any other config is changed/saved and therefor future changes on the env will not be reflected.
I'd just add it to a second cacheEnv array instead.

Anyway - this second cache array doesn't solve the actual problem: For all entries, that don't have a env variable set the getenv is still called. And I would say, that most env variables are not used at all -> still the overhead and we don't need to investigate into the cache way of doing it.

I removed this again. I would say that we just check the env variable without any caching.

* added functionality to override config.php values with 'OC_' prefixed environment variables * use getenv to read environment variables since apache does not set $_ENV variables, fixed test Signed-off-by: Morris Jobke <hey@morrisjobke.de>

Signed-off-by: Morris Jobke <hey@morrisjobke.de>

Addressed

MorrisJobke · 2017-04-13T06:06:23Z

@ChristophWurst @rullzer @nickvergessen @LukasReschke Ready for review

nickvergessen · 2017-04-13T07:33:09Z

Should add to the docs that this is possible, but not recommended

LukasReschke

Not a big fan but well…

TechupBusiness · 2019-04-24T16:47:02Z

Great change thanks! Do you guys have a suggestion how we could resolve arrays? I'm struggling with setting trusted_domains... this happens when using docker and automatic installation feature. I can do a pull request but I would like to hear how you guys would solve it. Checking for a prefix like "array:" in the ENV and then separating the values by an unusual delimiter like {;} ?
@LukasReschke @MorrisJobke

kesselb · 2019-04-24T17:23:14Z

Keep in mind that config options with "." does not work. You might use another approach for them. I'm not a fan of this at all (and neither the official vm image or docker image use it). It's at least one additonal function call for every system variable.

TechupBusiness · 2019-04-25T16:16:21Z

Unfortunately it's common nowadays to use container and set the variables (db credentials, etc.) via enviornment variables. Therefore I would really like to see a better support here in nextcloud.

kesselb · 2019-04-25T18:09:31Z

I think its a better approach to enable getenv only for certain configuration variables like the docker image does: https://github.com/nextcloud/docker/tree/master/.config

TechupBusiness · 2019-10-10T13:23:48Z

Keep in mind that config options with "." does not work. You might use another approach for them. I'm not a fan of this at all (and neither the official vm image or docker image use it). It's at least one additonal function call for every system variable.

Well you could simply read $_ENV ($_SERVER should usually also contain it) if you want avoid another function call. Even though I don't think that a function call to ENV variables would have a measurable performance impact.

I think its a better approach to enable getenv only for certain configuration variables like the docker image does: https://github.com/nextcloud/docker/tree/master/.config

I can't agree on that. Isn't a configuration of an application there to configure it for the environment? So probably most configurations (that are not accessible) are relevant.

ChristophWurst · 2019-10-10T15:07:21Z

Please use https://help.nextcloud.com/ for discussion or open a new ticket for bug reports. Thanks.

MorrisJobke added 3. to review Waiting for reviews downstream enhancement labels Mar 21, 2017

MorrisJobke added this to the Nextcloud 12.0 milestone Mar 21, 2017

MorrisJobke requested review from rullzer, nickvergessen, LukasReschke, icewind1991 and blizzz March 21, 2017 03:13

MorrisJobke changed the title ~~Override config.php values through ENV variables (#26570)~~ Override config.php values through environment variables Mar 21, 2017

MorrisJobke mentioned this pull request Mar 21, 2017

Read Env variables to configure Nextcloud #3926

Closed

ChristophWurst reviewed Mar 21, 2017

View reviewed changes

nickvergessen previously requested changes Mar 21, 2017

View reviewed changes

icewind1991 previously requested changes Mar 21, 2017

View reviewed changes

MorrisJobke force-pushed the downstream-26570 branch 2 times, most recently from 1ced627 to 6198b22 Compare March 21, 2017 20:53

nickvergessen reviewed Mar 23, 2017

View reviewed changes

nickvergessen previously requested changes Mar 24, 2017

View reviewed changes

phisch and others added 3 commits April 4, 2017 15:56

OC_ -> NC_

0fcb37a

Signed-off-by: Morris Jobke <hey@morrisjobke.de>

Check for boolean false and add tests

95a21e2

Signed-off-by: Morris Jobke <hey@morrisjobke.de>

MorrisJobke force-pushed the downstream-26570 branch from 29245e6 to 95a21e2 Compare April 4, 2017 20:56

nickvergessen approved these changes Apr 13, 2017

View reviewed changes

LukasReschke approved these changes Apr 13, 2017

View reviewed changes

LukasReschke merged commit 01f3698 into master Apr 13, 2017

LukasReschke deleted the downstream-26570 branch April 13, 2017 08:51

MorrisJobke mentioned this pull request Apr 23, 2017

ENV variables to overwrite config values nextcloud/documentation#426

Open

MorrisJobke mentioned this pull request Jan 22, 2018

Support for config files outside the installation #7976

Closed

pgnd mentioned this pull request Jul 19, 2019

enable additional/external config location -- outside of install/web-root #16472

Closed

nextcloud locked as resolved and limited conversation to collaborators Oct 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Override config.php values through environment variables #3966

Override config.php values through environment variables #3966

MorrisJobke commented Mar 21, 2017 •

edited

Loading

mention-bot commented Mar 21, 2017

karlitschek commented Mar 21, 2017

MorrisJobke commented Mar 21, 2017

ChristophWurst Mar 21, 2017

nickvergessen Mar 21, 2017

ChristophWurst Mar 21, 2017

MorrisJobke Mar 21, 2017

ChristophWurst Mar 21, 2017

MorrisJobke Mar 21, 2017

MorrisJobke Mar 21, 2017

ChristophWurst Mar 21, 2017

nickvergessen left a comment

icewind1991 Mar 21, 2017

MorrisJobke commented Mar 22, 2017

MorrisJobke commented Mar 22, 2017

icewind1991 commented Mar 23, 2017

nickvergessen Mar 23, 2017

MorrisJobke Mar 23, 2017

MorrisJobke Mar 23, 2017

nickvergessen Mar 23, 2017

MorrisJobke Mar 23, 2017

nickvergessen Mar 24, 2017

MorrisJobke Mar 24, 2017

MorrisJobke Apr 4, 2017

MorrisJobke commented Apr 13, 2017

nickvergessen commented Apr 13, 2017

LukasReschke left a comment

TechupBusiness commented Apr 24, 2019

kesselb commented Apr 24, 2019

TechupBusiness commented Apr 25, 2019

kesselb commented Apr 25, 2019

TechupBusiness commented Oct 10, 2019 •

edited

Loading

ChristophWurst commented Oct 10, 2019

Override config.php values through environment variables #3966

Override config.php values through environment variables #3966

Conversation

MorrisJobke commented Mar 21, 2017 • edited Loading

mention-bot commented Mar 21, 2017

karlitschek commented Mar 21, 2017

MorrisJobke commented Mar 21, 2017

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

Choose a reason for hiding this comment

nickvergessen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MorrisJobke commented Mar 22, 2017

MorrisJobke commented Mar 22, 2017

icewind1991 commented Mar 23, 2017

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

Choose a reason for hiding this comment

MorrisJobke commented Apr 13, 2017

nickvergessen commented Apr 13, 2017

LukasReschke left a comment

Choose a reason for hiding this comment

TechupBusiness commented Apr 24, 2019

kesselb commented Apr 24, 2019

TechupBusiness commented Apr 25, 2019

kesselb commented Apr 25, 2019

TechupBusiness commented Oct 10, 2019 • edited Loading

ChristophWurst commented Oct 10, 2019

MorrisJobke commented Mar 21, 2017 •

edited

Loading

TechupBusiness commented Oct 10, 2019 •

edited

Loading