Fail to start if settings include unknown keys (#12554)

* [server/config/complete] parameterize test setup

* [server/config/complete] group tests into sub-suites

* [server/config/complete] fail startup if some settings are unused

* [cli/server] pretty-print InvalidConfig errors

* [cli/errors] allow startup errors to control process.exitCode

* [server/config/complete] update tests to check for throwing

* [server/config/complete] test deprecation support more generically

* [server/config/complete] test server.decorate() behavior

* [server/config/complete] test the shape of the thrown error

* [cli/serve] add integration test for invalid config handling

* [testUtils/kbnServer] add createServerWithCorePlugins()

* [uiExports/tests] fix bug this change exposed

* [server/config/complete] add special case for `env` config key

* [server/config/flattenWith] fail faster with less lodash

* [server/config/complete] add test for special "env" handling

* [server/config/complete] combine noun+verb creation

* [testUtils/kbnServer] fix typo

* [server/config/complete] use expressive sinon.assert helper

* [utils/getFlattenedObject] remove traverseArrays option

src/cli/serve/__tests__/fixtures/invalid_config.yml

@@ -0,0 +1,13 @@
+unknown:
+  key: 1
+
+other:
+  unknown.key: 2
+  third: 3
+
+some.flat.key: 4
+
+some.array:
+  - 1
+  - 2
+  - 3

src/cli/serve/__tests__/invalid_config.js

@@ -0,0 +1,39 @@
+import { spawnSync } from 'child_process';
+import { resolve } from 'path';
+
+import expect from 'expect.js';
+
+const ROOT_DIR = resolve(__dirname, '../../../../');
+const INVALID_CONFIG_PATH = resolve(__dirname, 'fixtures/invalid_config.yml');
+
+describe('cli invalid config support', function () {
+  this.timeout(20 * 1000);
+  this.slow(10 * 1000);
+
+  it('exits with statusCode 64 and logs a single line when config is invalid', function () {
+    const { error, status, stdout } = spawnSync(process.execPath, [
+      'src/cli',
+      '--config', INVALID_CONFIG_PATH
+    ], {
+      cwd: ROOT_DIR
+    });
+
+    const logLines = stdout.toString('utf8')
+      .split('\n')
+      .filter(Boolean)
+      .map(JSON.parse);
+
+    expect(error).to.be(undefined);
+    expect(status).to.be(64);
+
+    expect(logLines).to.have.length(1);
+    expect(logLines[0]).to.have.property('tags')
+      .eql(['fatal']);
+    expect(logLines[0]).to.have.property('message')
+      .contain('"unknown.key"')
+      .contain('"other.unknown.key"')
+      .contain('"other.third"')
+      .contain('"some.flat.key"')
+      .contain('"some.array"');
+  });
+});

src/cli/serve/serve.js

@@ -147,18 +147,27 @@ export default function (program) {
     try {
       kbnServer = new KbnServer(settings);
       await kbnServer.ready();
-    }
-    catch (err) {
+    } catch (error) {
       const { server } = kbnServer;
 
-      if (err.code === 'EADDRINUSE') {
-        logFatal(`Port ${err.port} is already in use. Another instance of Kibana may be running!`, server);
-      } else {
-        logFatal(err, server);
+      switch (error.code) {
+        case 'EADDRINUSE':
+          logFatal(`Port ${error.port} is already in use. Another instance of Kibana may be running!`, server);
+          break;
+
+        case 'InvalidConfig':
+          logFatal(error.message, server);
+          break;
+
+        default:
+          logFatal(error, server);
+          break;
       }
 
       kbnServer.close();
-      process.exit(1); // eslint-disable-line no-process-exit
+      const exitCode = error.processExitCode == null ? 1 : error.processExitCode;
+      // eslint-disable-next-line no-process-exit
+      process.exit(exitCode);
     }
 
     process.on('SIGHUP', function reloadConfig() {
@@ -175,6 +184,7 @@ export default function (program) {
 function logFatal(message, server) {
   if (server) {
     server.log(['fatal'], message);
+  } else {
+    console.error('FATAL', message);
   }
-  console.error('FATAL', message);
 }

src/core_plugins/elasticsearch/lib/__tests__/routes.js

@@ -1,7 +1,6 @@
 import { format } from 'util';
 
 import * as kbnTestServer from '../../../../test_utils/kbn_server';
-import { fromRoot } from '../../../../utils';
 
 describe('plugins/elasticsearch', function () {
   describe('routes', function () {
@@ -13,13 +12,7 @@ describe('plugins/elasticsearch', function () {
       // context, not to the parent context.
       this.timeout(60000);
 
-      kbnServer = kbnTestServer.createServer({
-        plugins: {
-          scanDirs: [
-            fromRoot('src/core_plugins')
-          ]
-        },
-      });
+      kbnServer = kbnTestServer.createServerWithCorePlugins();
 
       await kbnServer.ready();
       await kbnServer.server.plugins.elasticsearch.waitUntilReady();

src/core_plugins/kibana/server/lib/__tests__/manage_uuid.js

@@ -1,7 +1,6 @@
 import expect from 'expect.js';
 import sinon from 'sinon';
 import * as kbnTestServer from '../../../../../test_utils/kbn_server.js';
-import { fromRoot } from '../../../../../utils';
 import manageUuid from '../manage_uuid';
 
 describe('core_plugins/kibana/server/lib', function () {
@@ -13,13 +12,7 @@ describe('core_plugins/kibana/server/lib', function () {
     before(async function () {
       this.timeout(60000); // sometimes waiting for server takes longer than 10
 
-      kbnServer = kbnTestServer.createServer({
-        plugins: {
-          scanDirs: [
-            fromRoot('src/core_plugins')
-          ]
-        }
-      });
+      kbnServer = kbnTestServer.createServerWithCorePlugins();
 
       await kbnServer.ready();
     });

src/functional_test_runner/__tests__/lib/kibana.js

@@ -1,20 +1,12 @@
-import { resolve } from 'path';
-
-import { createServer } from '../../../test_utils/kbn_server';
+import { createServerWithCorePlugins } from '../../../test_utils/kbn_server';
 
 export async function startupKibana({ port, esUrl }) {
-  const server = createServer({
+  const server = createServerWithCorePlugins({
     server: {
       port,
       autoListen: true,
     },
 
-    plugins: {
-      scanDirs: [
-        resolve(__dirname, '../../../core_plugins')
-      ],
-    },
-
     elasticsearch: {
       url: esUrl
     }

src/server/config/__tests__/complete.js

@@ -1,105 +1,200 @@
-import complete from '../complete';
+import completeMixin from '../complete';
 import expect from 'expect.js';
-import { noop } from 'lodash';
 import sinon from 'sinon';
 
-describe('server config complete', function () {
-  it(`should call server.log when there's an unused setting`, function () {
-    const kbnServer = {
-      settings: {
-        unused: true
-      }
-    };
+/* eslint-disable import/no-duplicates */
+import * as transformDeprecationsNS from '../transform_deprecations';
+import { transformDeprecations } from '../transform_deprecations';
+/* eslint-enable import/no-duplicates */
+
+describe('server/config completeMixin()', function () {
+  const sandbox = sinon.sandbox.create();
+  afterEach(() => sandbox.restore());
+
+  const setup = (options = {}) => {
+    const {
+      settings = {},
+      configValues = {}
+    } = options;
 
     const server = {
-      decorate: noop,
-      log: sinon.spy()
+      decorate: sinon.stub()
     };
 
     const config = {
-      get: sinon.stub().returns({
-        used: true
-      })
+      get: sinon.stub().returns(configValues)
     };
 
-    complete(kbnServer, server, config);
+    const kbnServer = {
+      settings,
+      server,
+      config
+    };
 
-    expect(server.log.calledOnce).to.be(true);
+    const callCompleteMixin = () => {
+      completeMixin(kbnServer, server, config);
+    };
+
+    return { config, callCompleteMixin, server };
+  };
+
+  describe('server decoration', () => {
+    it('adds a config() function to the server', () => {
+      const { config, callCompleteMixin, server } = setup({
+        settings: {},
+        configValues: {}
+      });
+
+      callCompleteMixin();
+      sinon.assert.calledOnce(server.decorate);
+      sinon.assert.calledWith(server.decorate, 'server', 'config', sinon.match.func);
+      expect(server.decorate.firstCall.args[2]()).to.be(config);
+    });
   });
 
-  it(`shouldn't call server.log when there isn't an unused setting`, function () {
-    const kbnServer = {
-      settings: {
-        used: true
-      }
-    };
+  describe('all settings used', () => {
+    it('should not throw', function () {
+      const { callCompleteMixin } = setup({
+        settings: {
+          used: true
+        },
+        configValues: {
+          used: true
+        },
+      });
 
-    const server = {
-      decorate: noop,
-      log: sinon.spy()
-    };
+      callCompleteMixin();
+    });
 
-    const config = {
-      get: sinon.stub().returns({
-        used: true
-      })
-    };
+    describe('more config values than settings', () => {
+      it('should not throw', function () {
+        const { callCompleteMixin } = setup({
+          settings: {
+            used: true
+          },
+          configValues: {
+            used: true,
+            foo: 'bar'
+          }
+        });
 
-    complete(kbnServer, server, config);
-
-    expect(server.log.called).to.be(false);
+        callCompleteMixin();
+      });
+    });
   });
 
-  it(`shouldn't call server.log when there are more config values than settings`, function () {
-    const kbnServer = {
-      settings: {
-        used: true
-      }
-    };
-
-    const server = {
-      decorate: noop,
-      log: sinon.spy()
-    };
-
-    const config = {
-      get: sinon.stub().returns({
-        used: true,
-        foo: 'bar'
-      })
-    };
-
-    complete(kbnServer, server, config);
-    expect(server.log.called).to.be(false);
-  });
-
-  it('should transform server.ssl.cert to server.ssl.certificate', function () {
-    const kbnServer = {
-      settings: {
-        server: {
-          ssl: {
-            cert: 'path/to/cert'
+  describe('env setting specified', () => {
+    it('should not throw', () => {
+      const { callCompleteMixin } = setup({
+        settings: {
+          env: 'development'
+        },
+        configValues: {
+          env: {
+            name: 'development'
           }
         }
-      }
-    };
+      });
 
-    const server = {
-      decorate: noop,
-      log: sinon.spy()
-    };
+      callCompleteMixin();
+    });
+  });
 
-    const config = {
-      get: sinon.stub().returns({
-        server: {
-          ssl: {
-            certificate: 'path/to/cert'
-          }
+  describe('settings include non-default array settings', () => {
+    it('should not throw', () => {
+      const { callCompleteMixin } = setup({
+        settings: {
+          foo: [
+            'a',
+            'b'
+          ]
+        },
+        configValues: {
+          foo: []
         }
-      })
-    };
+      });
 
-    complete(kbnServer, server, config);
-    expect(server.log.called).to.be(false);
+      callCompleteMixin();
+    });
+  });
+
+  describe('some settings unused', () => {
+    it('should throw an error', function () {
+      const { callCompleteMixin } = setup({
+        settings: {
+          unused: true
+        },
+        configValues: {
+          used: true
+        }
+      });
+
+      expect(callCompleteMixin).to.throwError('"unused" not applied');
+    });
+
+    describe('error thrown', () => {
+      it('has correct code, processExitCode, and message', () => {
+        const { callCompleteMixin } = setup({
+          settings: {
+            unused: true,
+            foo: 'bar',
+            namespace: {
+              with: {
+                sub: {
+                  keys: true
+                }
+              }
+            }
+          }
+        });
+
+        expect(callCompleteMixin).to.throwError((error) => {
+          expect(error).to.have.property('code', 'InvalidConfig');
+          expect(error).to.have.property('processExitCode', 64);
+          expect(error.message).to.contain('"unused", "foo", and "namespace.with.sub.keys"');
+        });
+      });
+    });
+  });
+
+  describe('deprecation support', () => {
+    it('should transform settings when determining what is unused', function () {
+      sandbox.spy(transformDeprecationsNS, 'transformDeprecations');
+
+      const settings = {
+        foo: 1
+      };
+
+      const { callCompleteMixin } = setup({
+        settings,
+        configValues: {
+          ...settings
+        }
+      });
+
+      callCompleteMixin();
+      sinon.assert.calledOnce(transformDeprecations);
+      sinon.assert.calledWithExactly(transformDeprecations, settings);
+    });
+
+    it('should use transformed settings when considering what is used', function () {
+      sandbox.stub(transformDeprecationsNS, 'transformDeprecations', (settings) => {
+        settings.bar = settings.foo;
+        delete settings.foo;
+        return settings;
+      });
+
+      const { callCompleteMixin } = setup({
+        settings: {
+          foo: 1
+        },
+        configValues: {
+          bar: 1
+        }
+      });
+
+      callCompleteMixin();
+      sinon.assert.calledOnce(transformDeprecations);
+    });
   });
 });

src/server/config/__tests__/flatten_with.js

@@ -1,27 +0,0 @@
-import flattenWith from '../flatten_with';
-import expect from 'expect.js';
-
-describe('flatten_with(dot, nestedObj)', function () {
-
-  it('should flatten object with dot', function () {
-    const nestedObj = {
-      test: {
-        enable: true,
-        hosts: ['host-01', 'host-02'],
-        client: {
-          type: 'nosql',
-          pool: [{ port: 5051 }, { port: 5052 }]
-        }
-      }
-    };
-    expect(flattenWith('.', nestedObj)).to.eql({
-      'test.enable': true,
-      'test.hosts': ['host-01', 'host-02'],
-      'test.client.type': 'nosql',
-      'test.client.pool': [{ port: 5051 }, { port: 5052 }]
-    });
-  });
-
-});
-
-

src/server/config/complete.js

@@ -1,8 +1,23 @@
-import { difference, keys } from 'lodash';
+import { difference } from 'lodash';
 import { transformDeprecations } from './transform_deprecations';
+import { formatListAsProse, getFlattenedObject } from '../../utils';
 
-const getUnusedSettings = (settings, configValues) => {
-  return difference(keys(transformDeprecations(settings)), keys(configValues));
+const getFlattenedKeys = object => (
+  Object.keys(getFlattenedObject(object))
+);
+
+const getUnusedConfigKeys = (settings, configValues) => {
+  const inputKeys = getFlattenedKeys(transformDeprecations(settings));
+  const appliedKeys = getFlattenedKeys(configValues);
+
+  if (inputKeys.includes('env')) {
+    // env is a special case key, see https://github.com/elastic/kibana/blob/848bf17b/src/server/config/config.js#L74
+    // where it is deleted from the settings before being injected into the schema via context and
+    // then renamed to `env.name` https://github.com/elastic/kibana/blob/848bf17/src/server/config/schema.js#L17
+    inputKeys[inputKeys.indexOf('env')] = 'env.name';
+  }
+
+  return difference(inputKeys, appliedKeys);
 };
 
 export default function (kbnServer, server, config) {
@@ -11,7 +26,24 @@ export default function (kbnServer, server, config) {
     return kbnServer.config;
   });
 
-  for (const key of getUnusedSettings(kbnServer.settings, config.get())) {
-    server.log(['warning', 'config'], `Settings for "${key}" were not applied, check for spelling errors and ensure the plugin is loaded.`);
+  const unusedKeys = getUnusedConfigKeys(kbnServer.settings, config.get())
+    .map(key => `"${key}"`);
+
+  if (!unusedKeys.length) {
+    return;
   }
+
+  const desc = unusedKeys.length === 1
+    ? 'setting was'
+    : 'settings were';
+
+  const error = new Error(
+    `${formatListAsProse(unusedKeys)} ${desc} not applied. ` +
+    'Check for spelling errors and ensure that expected ' +
+    'plugins are installed and enabled.'
+  );
+
+  error.code = 'InvalidConfig';
+  error.processExitCode = 64;
+  throw error;
 }

src/server/config/flatten_with.js

@@ -1,18 +0,0 @@
-import _ from 'lodash';
-
-export default function (dot, nestedObj, flattenArrays) {
-  const stack = []; // track key stack
-  const flatObj = {};
-  (function flattenObj(obj) {
-    _.keys(obj).forEach(function (key) {
-      stack.push(key);
-      if (!flattenArrays && _.isArray(obj[key])) flatObj[stack.join(dot)] = obj[key];
-      else if (_.isObject(obj[key])) flattenObj(obj[key]);
-      else flatObj[stack.join(dot)] = obj[key];
-      stack.pop();
-    });
-  }(nestedObj));
-  return flatObj;
-}
-
-

src/server/config/override.js

@@ -1,11 +1,9 @@
 import _ from 'lodash';
-import flattenWith from './flatten_with';
 import explodeBy from './explode_by';
+import { getFlattenedObject } from '../../utils';
 
 export default function (target, source) {
-  const _target = flattenWith('.', target);
-  const _source = flattenWith('.', source);
+  const _target = getFlattenedObject(target);
+  const _source = getFlattenedObject(source);
   return explodeBy('.', _.defaults(_source, _target));
 }
-
-

src/server/http/__tests__/index.js

@@ -1,6 +1,5 @@
 import expect from 'expect.js';
 import * as kbnTestServer from '../../../test_utils/kbn_server';
-import { fromRoot } from '../../../utils';
 
 describe('routes', function () {
   this.slow(10000);
@@ -8,13 +7,7 @@ describe('routes', function () {
 
   let kbnServer;
   beforeEach(function () {
-    kbnServer = kbnTestServer.createServer({
-      plugins: {
-        scanDirs: [
-          fromRoot('src/core_plugins')
-        ]
-      }
-    });
+    kbnServer = kbnTestServer.createServerWithCorePlugins();
     return kbnServer.ready();
   });
   afterEach(function () {

src/server/plugins/plugin.js

@@ -117,9 +117,9 @@ export default class Plugin {
     const asyncRegister = async (server, options) => {
       this.server = server;
 
-      await Promise.all(this[extendInitFns].map(async fn => {
+      for (const fn of this[extendInitFns]) {
         await fn.call(this, server, options);
-      }));
+      }
 
       server.log(['plugins', 'debug'], {
         tmpl: 'Initializing plugin <%= plugin.toString() %>',

src/test_utils/kbn_server.js

@@ -1,11 +1,12 @@
 import url from 'url';
+import { resolve } from 'path';
 import { defaultsDeep, set } from 'lodash';
 import { header as basicAuthHeader } from './base_auth';
 import { kibanaUser, kibanaServer } from '../../test/shield';
 import { esTestServerUrlParts } from '../../test/es_test_server_url_parts';
 import KbnServer from '../../src/server/kbn_server';
 
-const SERVER_DEFAULTS = {
+const DEFAULTS_SETTINGS = {
   server: {
     autoListen: false,
     xsrf: {
@@ -19,6 +20,14 @@ const SERVER_DEFAULTS = {
   optimize: {
     enabled: false
   },
+};
+
+const DEFAULT_SETTINGS_WITH_CORE_PLUGINS = {
+  plugins: {
+    scanDirs: [
+      resolve(__dirname, '../core_plugins'),
+    ],
+  },
   elasticsearch: {
     url: url.format(esTestServerUrlParts),
     username: kibanaServer.username,
@@ -26,15 +35,27 @@ const SERVER_DEFAULTS = {
   }
 };
 
+
 /**
  * Creates an instance of KbnServer with default configuration
  * tailored for unit tests
  *
- * @param {object} params Any config overrides for this instance
+ * @param {Object} [settings={}] Any config overrides for this instance
+ * @return {KbnServer}
  */
-export function createServer(params = {}) {
-  params = defaultsDeep({}, params, SERVER_DEFAULTS);
-  return new KbnServer(params);
+export function createServer(settings = {}) {
+  return new KbnServer(defaultsDeep({}, settings, DEFAULTS_SETTINGS));
+}
+
+/**
+ *  Creates an instance of KbnServer, including all of the core plugins,
+ *  with default configuration tailored for unit tests
+ *
+ *  @param  {Object} [settings={}]
+ *  @return {KbnServer}
+ */
+export function createServerWithCorePlugins(settings = {}) {
+  return new KbnServer(defaultsDeep({}, settings, DEFAULT_SETTINGS_WITH_CORE_PLUGINS, DEFAULTS_SETTINGS));
 }
 
 /**

src/ui/__tests__/ui_exports.js

@@ -36,8 +36,8 @@ describe('UiExports', function () {
         kbnServer = kbnTestServer.createServer({
           plugins: {
             paths: [
+              resolve(__dirname, 'fixtures/plugin_foo'),
               resolve(__dirname, 'fixtures/plugin_bar'),
-              resolve(__dirname, 'fixtures/plugin_foo')
             ]
           },
 
@@ -75,7 +75,7 @@ describe('UiExports', function () {
             scanDirs: [],
             paths: [
               resolve(__dirname, 'fixtures/plugin_async_foo'),
-              resolve(__dirname, 'fixtures/plugin_foo')
+              resolve(__dirname, 'fixtures/plugin_bar'),
             ]
           },
 

src/ui/ui_exports.js

@@ -125,7 +125,7 @@ export default class UiExports {
       case 'injectDefaultVars':
         return (plugin, injector) => {
           plugin.extendInit(async (server, options) => {
-            _.merge(this.defaultInjectedVars, await injector.call(plugin, server, options));
+            _.defaults(this.defaultInjectedVars, await injector.call(plugin, server, options));
           });
         };
 

src/utils/__tests__/get_flattened_object.js

@@ -0,0 +1,26 @@
+import expect from 'expect.js';
+
+import { getFlattenedObject } from '../get_flattened_object';
+
+describe('getFlattenedObject()', () => {
+  it('throws when rootValue is not an object or is an array', () => {
+    expect(() => getFlattenedObject(1)).to.throwError('number');
+    expect(() => getFlattenedObject(Infinity)).to.throwError('number');
+    expect(() => getFlattenedObject(NaN)).to.throwError('number');
+    expect(() => getFlattenedObject(false)).to.throwError('boolean');
+    expect(() => getFlattenedObject(null)).to.throwError('null');
+    expect(() => getFlattenedObject(undefined)).to.throwError('undefined');
+    expect(() => getFlattenedObject([])).to.throwError('array');
+  });
+
+  it('flattens objects', () => {
+    expect(getFlattenedObject({ a: 'b' })).to.eql({ a: 'b' });
+    expect(getFlattenedObject({ a: { b: 'c' } })).to.eql({ 'a.b': 'c' });
+    expect(getFlattenedObject({ a: { b: 'c' }, d: { e: 'f' } })).to.eql({ 'a.b': 'c', 'd.e': 'f' });
+  });
+
+  it('does not flatten arrays', () => {
+    expect(getFlattenedObject({ a: ['b'] })).to.eql({ a: ['b'] });
+    expect(getFlattenedObject({ a: { b: ['c', 'd'] } })).to.eql({ 'a.b': ['c', 'd'] });
+  });
+});

src/utils/get_flattened_object.js

@@ -0,0 +1,35 @@
+
+function shouldReadKeys(value) {
+  return typeof value === 'object' && value !== null && !Array.isArray(value);
+}
+
+/**
+ *  Flattens a deeply nested object to a map of dot-separated
+ *  paths pointing to all primative values **and arrays**
+ *  from `rootValue`.
+ *
+ *  example:
+ *    getFlattenedObject({ a: { b: 1, c: [2,3] } })
+ *    // => { 'a.b': 1, 'a.c': [2,3] }
+ *
+ *  @param {Object} rootValue
+ *  @returns {Object}
+ */
+export function getFlattenedObject(rootValue) {
+  if (!shouldReadKeys(rootValue)) {
+    throw new TypeError(`Root value is not flatten-able, received ${rootValue}`);
+  }
+
+  return (function flatten(acc, prefix, object) {
+    return Object.keys(object).reduce((acc, key) => {
+      const value = object[key];
+      const path = prefix ? `${prefix}.${key}` : key;
+
+      if (shouldReadKeys(value)) {
+        return flatten(acc, path, value);
+      } else {
+        return { ...acc, [path]: value };
+      }
+    }, acc);
+  }({}, '', rootValue));
+}

src/utils/index.js

@@ -7,6 +7,7 @@ export { unset } from './unset';
 export { encodeQueryComponent } from './encode_query_component';
 export { modifyUrl } from './modify_url';
 export { createToolingLog } from './tooling_log';
+export { getFlattenedObject } from './get_flattened_object';
 
 export {
   getKbnTypeNames,