Add support for environment variable injection in kibana.yml #16988

fbaligand · 2018-03-05T23:22:10Z

This PR lets support for environment variable injection in kibana.yml, using syntax ${MY_ENV_VAR}.

elasticmachine · 2018-03-05T23:22:12Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

tylersmalley · 2018-03-06T01:02:13Z

src/server/config/config.js

    }

    // attempt to validate the config value
    this._commit(config);
  }

+  _replaceEnvVars(value) {


Can we add tests for all of these cases in addition to the env variable not being specified?

Hi @tylersmalley

OK, I will add a test when env variable is not specified.

Then, for additional cases, currently, _.isString(value) and _.isArray(value) are covered. Only _.isPlainObject(value) is not covered. I will change the test to cover it.

fbaligand · 2018-03-06T09:51:41Z

src/server/config/config.js

@@ -61,13 +61,25 @@ export class Config {
    if (_.isPlainObject(key)) {
      config = override(config, key);


@tylersmalley

Question : should I replace override(config, key) by override(config, _replaceEnvVars(key)) ?

I'm not entirely sure what it's used for, maybe testing. @spalger do you know?

Not sure it is used. Gut feeling is it was just an "expected" behavior of a method like this.

@spalger
So, should I replace override(config, key) by override(config, _replaceEnvVars(key)) ?

My recommendation from #16988 (comment) is to move all of this into https://github.com/elastic/kibana/blob/master/src/cli/serve/read_yaml_config.js and do it once when reading kibana.yml from the file system

@spalger
OK, I will do so.
Thanks for this first review.

fbaligand · 2018-03-07T23:06:07Z

@tylersmalley
What behaviour do you wish when environment variable is not defined ?
Currently, it replaces env var reference by undefined

fbaligand · 2018-03-08T22:16:49Z

Hi @tylersmalley,

I increased test coverage (all 'replaceEnvVars()' lines are covered)
I raise an error if referenced environment variable does not exist

Is it OK for you ?

fbaligand · 2018-03-18T17:47:50Z

Hi @tylersmalley,
Could you review my changes ?

tylersmalley · 2018-03-26T21:32:20Z

@fbaligand, thanks for the updates. Hope to get to reviewing this soon.

fbaligand · 2018-03-26T21:53:22Z

Thanks, I hope so too ! :)

spalger · 2018-04-05T19:30:06Z

Just took a cursory look, but think I'd prefer that this was implemented in https://github.com/elastic/kibana/blob/master/src/cli/serve/read_yaml_config.js so that replacement like this only occured when reading the config file. I would be quite surprised by this behavior if I was calling config.set() directly from just about anywhere.

fbaligand · 2018-04-05T20:39:38Z

I just rebased my code to merge conflicts with master branch.

fbaligand · 2018-04-07T18:09:07Z

@spalger
I just moved the code to read_yaml_config.js as requested.
Could you review the code ?

spalger

I'm into it, LGTM

fbaligand · 2018-04-08T13:47:42Z

Great @spalger !

=> @tylersmalley @jbudz could you review/approve the code ?

spalger · 2018-04-08T20:36:56Z

Jenkins, test this

elasticmachine · 2018-04-08T21:27:55Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

jbudz · 2018-04-09T15:08:00Z

src/cli/serve/read_yaml_config.js

+function _replaceEnvVarRefs(val) {
+  return val.replace(/\$\{(\w+)\}/g, (match, envVarName) => {
+    if (process.env[envVarName] !== undefined) {
+      return process.env[envVarName];


I would prefer if we _.escaped this by default, and any configuration that needs it unescaped can do so. Thoughts?

@jbudz
So you want to replace return process.env[envVarName];
by return _.escape(process.env[envVarName]); ?

No problem for me, but can you explain me why ? I mean : at this point, the yaml file is already parsed.

Maybe it could be an issue for server.basePath option if user wants to use & character ?

Yeah, I don't think we want to escape HTML entities in the config values. People using this feature need to take care that they inject the correct value and Joi will take care of ensuring that it's a valid value.

I was initially concerned about non-string values, but realized Joi also parses JSON values in an attempt to coerce the value to the desired type, so to set a boolean value you can do:

some.boolean: "${MY_ENV_VAR}"

and

MY_ENV_VAR=true bin/kibana

And Joi will correctly convert the valid JSON so that

config.get('some.boolean') === true

in practice I can't think of anything realistic, it's mostly how I operate. I'l defer on this.

I agree with @spalger
In an option like elasticsearch.customHeaders, people want that the final value is exactly what they type. And value can contain some special characters like &.

spalger · 2018-04-09T18:10:00Z

src/cli/serve/read_yaml_config.js

@@ -19,6 +28,10 @@ export function merge(sources) {
        return;
      }

+      if (isString(val)) {
+    	  val = _replaceEnvVarRefs(val);


Looks like this line is mixing tabs and spaces. Also, would you mind just inlining the _replaceEnvVarRefs() function here or removing the leading _ from it's name?

fbaligand · 2018-04-09T19:59:44Z

@spalger
You're right, there was a remaining tab, sorry for that, I thought that linting protects against this kind of mistake.
I removed the tab and removed the leading _ to replaceEnvVarRefs method.
Is it OK for you ?

spalger

LGTM

fbaligand · 2018-04-10T21:11:07Z

Hi @spalger @jbudz
I saw that the PR was in conflict with master => I rebased my code and migrated my test to the new test conventions.

=> Could you launch jenkins tests to check that everything is still OK ?

jbudz · 2018-04-10T21:17:36Z

test it please

elasticmachine · 2018-04-10T22:08:01Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

fbaligand · 2018-04-10T22:25:22Z

Jenkins build is OK => nice !

=> @tylersmalley, could you approve the PR ?

tylersmalley · 2018-04-12T22:01:32Z

6.x/6.3: 6d70217

tylersmalley · 2018-04-12T22:01:46Z

Thanks @fbaligand!

fbaligand · 2018-04-13T04:19:45Z

Thanks @tylersmalley for the merge !

tylersmalley suggested changes Mar 6, 2018

View reviewed changes

tylersmalley added the Team:Operations Team label for Operations Team label Mar 6, 2018

fbaligand commented Mar 6, 2018

View reviewed changes

tylersmalley requested a review from jbudz April 3, 2018 16:43

fbaligand force-pushed the env-var-injection branch from 892b934 to 6c61584 Compare April 5, 2018 20:38

fbaligand force-pushed the env-var-injection branch from 6c61584 to 68e1da5 Compare April 7, 2018 18:06

spalger approved these changes Apr 8, 2018

View reviewed changes

jbudz reviewed Apr 9, 2018

View reviewed changes

spalger reviewed Apr 9, 2018

View reviewed changes

spalger approved these changes Apr 9, 2018

View reviewed changes

jbudz approved these changes Apr 9, 2018

View reviewed changes

spalger added v7.0.0 v6.3.0 labels Apr 9, 2018

fbaligand force-pushed the env-var-injection branch from 086539b to ce122fb Compare April 10, 2018 21:08

Add support for environment variable injection in kibana.yml

ce122fb

tylersmalley merged commit 977cd51 into elastic:master Apr 12, 2018

tylersmalley pushed a commit that referenced this pull request Apr 12, 2018

Add support for environment variable injection in kibana.yml (#16988)

6d70217

fbaligand deleted the env-var-injection branch April 14, 2018 17:18

fbaligand mentioned this pull request Apr 22, 2018

add doc for environment variable injection #17833

Merged

russcam mentioned this pull request Aug 20, 2020

Escape environment variable interpolation in kibana.yml #75515

Closed

		@@ -61,13 +61,25 @@ export class Config {
		if (_.isPlainObject(key)) {
		config = override(config, key);

Add support for environment variable injection in kibana.yml #16988

Add support for environment variable injection in kibana.yml #16988

Conversation

fbaligand commented Mar 5, 2018 • edited Loading

elasticmachine commented Mar 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbaligand Mar 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbaligand Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

spalger Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbaligand commented Mar 7, 2018

fbaligand commented Mar 8, 2018

fbaligand commented Mar 18, 2018

tylersmalley commented Mar 26, 2018

fbaligand commented Mar 26, 2018 • edited Loading

spalger commented Apr 5, 2018

fbaligand commented Apr 5, 2018 • edited Loading

fbaligand commented Apr 7, 2018

spalger left a comment • edited Loading

Choose a reason for hiding this comment

fbaligand commented Apr 8, 2018

spalger commented Apr 8, 2018

elasticmachine commented Apr 8, 2018

💚 Build Succeeded

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

fbaligand commented Apr 9, 2018

spalger left a comment

Choose a reason for hiding this comment

fbaligand commented Apr 10, 2018 • edited Loading

jbudz commented Apr 10, 2018

elasticmachine commented Apr 10, 2018

💚 Build Succeeded

fbaligand commented Apr 10, 2018

tylersmalley commented Apr 12, 2018

tylersmalley commented Apr 12, 2018

fbaligand commented Apr 13, 2018

fbaligand commented Mar 5, 2018 •

edited

Loading

fbaligand Mar 6, 2018 •

edited

Loading

fbaligand Apr 5, 2018 •

edited

Loading

spalger Apr 5, 2018 •

edited

Loading

fbaligand commented Mar 26, 2018 •

edited

Loading

fbaligand commented Apr 5, 2018 •

edited

Loading

spalger left a comment •

edited

Loading

fbaligand commented Apr 10, 2018 •

edited

Loading