plugin system runs plugin config validation

src/core/server/config/config_service.test.ts

@@ -26,7 +26,7 @@ import { mockPackage } from './config_service.test.mocks';
 
 import { schema, Type, TypeOf } from '@kbn/config-schema';
 
-import { ConfigService, Env, ObjectToConfigAdapter, ConfigPath } from '.';
+import { ConfigService, Env, ObjectToConfigAdapter } from '.';
 import { loggingServiceMock } from '../logging/logging_service.mock';
 import { getEnvOptions } from './__mocks__/env';
 

src/core/server/plugins/discovery/plugin_discovery_error.ts

@@ -24,7 +24,6 @@ export enum PluginDiscoveryErrorType {
   InvalidPluginPath = 'invalid-plugin-path',
   InvalidManifest = 'invalid-manifest',
   MissingManifest = 'missing-manifest',
-  InvalidConfigSchema = 'invalid-config-schema',
 }
 
 /** @internal */
@@ -49,10 +48,6 @@ export class PluginDiscoveryError extends Error {
     return new PluginDiscoveryError(PluginDiscoveryErrorType.MissingManifest, path, cause);
   }
 
-  public static invalidConfigSchema(path: string, cause: Error) {
-    return new PluginDiscoveryError(PluginDiscoveryErrorType.InvalidConfigSchema, path, cause);
-  }
-
   /**
    * @param type Type of the discovery error (invalid directory, invalid manifest etc.)
    * @param path Path at which discovery error occurred.

src/core/server/plugins/discovery/plugins_discovery.test.ts

@@ -18,7 +18,6 @@
  */
 
 import { mockPackage, mockReaddir, mockReadFile, mockStat } from './plugins_discovery.test.mocks';
-import { schema } from '@kbn/config-schema';
 
 import { resolve } from 'path';
 import { BehaviorSubject } from 'rxjs';
@@ -36,30 +35,6 @@ const TEST_PLUGIN_SEARCH_PATHS = {
   nonExistentKibanaExtra: resolve(process.cwd(), '..', 'kibana-extra'),
 };
 const TEST_EXTRA_PLUGIN_PATH = resolve(process.cwd(), 'my-extra-plugin');
-const pluginDefinition = {
-  configDefinition: {
-    schema: schema.any(),
-  },
-};
-
-[
-  resolve(TEST_PLUGIN_SEARCH_PATHS.nonEmptySrcPlugins, '1', 'server'),
-  resolve(TEST_PLUGIN_SEARCH_PATHS.nonEmptySrcPlugins, '3', 'server'),
-  resolve(TEST_EXTRA_PLUGIN_PATH, 'server'),
-].forEach(path => jest.doMock(path, () => pluginDefinition, { virtual: true }));
-
-const pluginDefinitionBad = {
-  configDefinition: {
-    schema: {
-      validate: undefined,
-    },
-  },
-};
-jest.doMock(
-  resolve(TEST_PLUGIN_SEARCH_PATHS.nonEmptySrcPlugins, '6-no-schema', 'server'),
-  () => pluginDefinitionBad,
-  { virtual: true }
-);
 
 const logger = loggingServiceMock.create();
 beforeEach(() => {
@@ -71,7 +46,7 @@ beforeEach(() => {
         '3',
         '4-incomplete-manifest',
         '5-invalid-manifest',
-        '6-no-schema',
+        '6',
         '7-non-dir',
         '8-incompatible-manifest',
         '9-inaccessible-dir',
@@ -155,19 +130,19 @@ test('properly iterates through plugin search locations', async () => {
   const { plugin$, error$ } = discover(pluginsConfig, { configService, env, logger });
 
   const plugins = await plugin$.pipe(toArray()).toPromise();
-  expect(plugins).toHaveLength(3);
+  expect(plugins).toHaveLength(4);
 
   for (const path of [
     resolve(TEST_PLUGIN_SEARCH_PATHS.nonEmptySrcPlugins, '1'),
     resolve(TEST_PLUGIN_SEARCH_PATHS.nonEmptySrcPlugins, '3'),
+    resolve(TEST_PLUGIN_SEARCH_PATHS.nonEmptySrcPlugins, '6'),
     TEST_EXTRA_PLUGIN_PATH,
   ]) {
     const discoveredPlugin = plugins.find(plugin => plugin.path === path)!;
     expect(discoveredPlugin).toBeInstanceOf(PluginWrapper);
     expect(discoveredPlugin.configPath).toEqual(['core', 'config']);
     expect(discoveredPlugin.requiredPlugins).toEqual(['a', 'b']);
     expect(discoveredPlugin.optionalPlugins).toEqual(['c', 'd']);
-    expect(discoveredPlugin.schema).toEqual(pluginDefinition.configDefinition.schema);
   }
 
   await expect(
@@ -203,11 +178,6 @@ test('properly iterates through plugin search locations', async () => {
       '8-incompatible-manifest',
       'kibana.json'
     )})`,
-    `Error: The config definition for plugin did not contain \"schema\" field, which is required for config validation (invalid-config-schema, ${resolve(
-      TEST_PLUGIN_SEARCH_PATHS.nonEmptySrcPlugins,
-      '6-no-schema',
-      'server'
-    )})`,
   ]);
 
   expect(loggingServiceMock.collect(logger).warn).toMatchInlineSnapshot(`

src/core/server/plugins/discovery/plugins_discovery.ts

@@ -18,14 +18,12 @@
  */
 
 import { readdir, stat } from 'fs';
-import { resolve, join } from 'path';
+import { resolve } from 'path';
 import { bindNodeCallback, from, merge } from 'rxjs';
 import { catchError, filter, mergeMap, shareReplay } from 'rxjs/operators';
-import { Type } from '@kbn/config-schema';
-
 import { CoreContext } from '../../core_context';
 import { Logger } from '../../logging';
-import { PluginWrapper, PluginManifest, PluginConfigSchema } from '../plugin';
+import { PluginWrapper } from '../plugin';
 import { createPluginInitializerContext } from '../plugin_context';
 import { PluginsConfig } from '../plugins_config';
 import { PluginDiscoveryError } from './plugin_discovery_error';
@@ -107,34 +105,6 @@ function processPluginSearchPaths$(pluginDirs: ReadonlyArray<string>, log: Logge
   );
 }
 
-function readSchemaMaybe(
-  pluginPath: string,
-  manifest: PluginManifest,
-  log: Logger
-): PluginConfigSchema {
-  if (!manifest.server) return null;
-  const pluginPathServer = join(pluginPath, 'server');
-  // eslint-disable-next-line @typescript-eslint/no-var-requires
-  const pluginDefinition = require(pluginPathServer);
-
-  if (!('configDefinition' in pluginDefinition)) {
-    log.debug(`"${pluginPathServer}" does not export "configDefinition".`);
-    return null;
-  }
-
-  const configSchema = pluginDefinition.configDefinition.schema;
-  if (configSchema instanceof Type) {
-    return configSchema;
-  }
-
-  throw PluginDiscoveryError.invalidConfigSchema(
-    pluginPathServer,
-    new Error(
-      'The config definition for plugin did not contain "schema" field, which is required for config validation'
-    )
-  );
-}
-
 /**
  * Tries to load and parse the plugin manifest file located at the provided plugin
  * directory path and produces an error result if it fails to do so or plugin manifest
@@ -151,7 +121,6 @@ function createPlugin$(path: string, log: Logger, coreContext: CoreContext) {
       return new PluginWrapper(
         path,
         manifest,
-        readSchemaMaybe(path, manifest, log),
         createPluginInitializerContext(coreContext, manifest)
       );
     }),

src/core/server/plugins/plugin.test.ts

@@ -19,6 +19,8 @@
 
 import { join } from 'path';
 import { BehaviorSubject } from 'rxjs';
+import { schema } from '@kbn/config-schema';
+
 import { Env } from '../config';
 import { getEnvOptions } from '../config/__mocks__/env';
 import { CoreContext } from '../core_context';
@@ -82,7 +84,6 @@ test('`constructor` correctly initializes plugin instance', () => {
   const plugin = new PluginWrapper(
     'some-plugin-path',
     manifest,
-    null,
     createPluginInitializerContext(coreContext, manifest)
   );
 
@@ -98,7 +99,6 @@ test('`setup` fails if `plugin` initializer is not exported', async () => {
   const plugin = new PluginWrapper(
     'plugin-without-initializer-path',
     manifest,
-    null,
     createPluginInitializerContext(coreContext, manifest)
   );
 
@@ -114,7 +114,6 @@ test('`setup` fails if plugin initializer is not a function', async () => {
   const plugin = new PluginWrapper(
     'plugin-with-wrong-initializer-path',
     manifest,
-    null,
     createPluginInitializerContext(coreContext, manifest)
   );
 
@@ -130,7 +129,6 @@ test('`setup` fails if initializer does not return object', async () => {
   const plugin = new PluginWrapper(
     'plugin-with-initializer-path',
     manifest,
-    null,
     createPluginInitializerContext(coreContext, manifest)
   );
 
@@ -148,7 +146,6 @@ test('`setup` fails if object returned from initializer does not define `setup`
   const plugin = new PluginWrapper(
     'plugin-with-initializer-path',
     manifest,
-    null,
     createPluginInitializerContext(coreContext, manifest)
   );
 
@@ -165,12 +162,7 @@ test('`setup` fails if object returned from initializer does not define `setup`
 test('`setup` initializes plugin and calls appropriate lifecycle hook', async () => {
   const manifest = createPluginManifest();
   const initializerContext = createPluginInitializerContext(coreContext, manifest);
-  const plugin = new PluginWrapper(
-    'plugin-with-initializer-path',
-    manifest,
-    null,
-    initializerContext
-  );
+  const plugin = new PluginWrapper('plugin-with-initializer-path', manifest, initializerContext);
 
   const mockPluginInstance = { setup: jest.fn().mockResolvedValue({ contract: 'yes' }) };
   mockPluginInitializer.mockReturnValue(mockPluginInstance);
@@ -191,7 +183,6 @@ test('`start` fails if setup is not called first', async () => {
   const plugin = new PluginWrapper(
     'some-plugin-path',
     manifest,
-    null,
     createPluginInitializerContext(coreContext, manifest)
   );
 
@@ -205,7 +196,6 @@ test('`start` calls plugin.start with context and dependencies', async () => {
   const plugin = new PluginWrapper(
     'plugin-with-initializer-path',
     manifest,
-    null,
     createPluginInitializerContext(coreContext, manifest)
   );
   const context = { any: 'thing' } as any;
@@ -231,7 +221,6 @@ test('`stop` fails if plugin is not set up', async () => {
   const plugin = new PluginWrapper(
     'plugin-with-initializer-path',
     manifest,
-    null,
     createPluginInitializerContext(coreContext, manifest)
   );
 
@@ -249,7 +238,6 @@ test('`stop` does nothing if plugin does not define `stop` function', async () =
   const plugin = new PluginWrapper(
     'plugin-with-initializer-path',
     manifest,
-    null,
     createPluginInitializerContext(coreContext, manifest)
   );
 
@@ -264,7 +252,6 @@ test('`stop` calls `stop` defined by the plugin instance', async () => {
   const plugin = new PluginWrapper(
     'plugin-with-initializer-path',
     manifest,
-    null,
     createPluginInitializerContext(coreContext, manifest)
   );
 
@@ -275,3 +262,65 @@ test('`stop` calls `stop` defined by the plugin instance', async () => {
   await expect(plugin.stop()).resolves.toBeUndefined();
   expect(mockPluginInstance.stop).toHaveBeenCalledTimes(1);
 });
+
+describe('#getConfigSchema()', () => {
+  it('reads config schema from plugin', () => {
+    const pluginSchema = schema.any();
+    jest.doMock(
+      'plugin-with-schema/server',
+      () => ({
+        configDefinition: {
+          schema: pluginSchema,
+        },
+      }),
+      { virtual: true }
+    );
+    const manifest = createPluginManifest();
+    const plugin = new PluginWrapper(
+      'plugin-with-schema',
+      manifest,
+      createPluginInitializerContext(coreContext, manifest)
+    );
+
+    expect(plugin.getConfigSchema()).toBe(pluginSchema);
+  });
+  it('returns null if configDefinition not specified', () => {
+    jest.doMock('plugin-with-no-definition/server', () => ({}), { virtual: true });
+    const manifest = createPluginManifest();
+    const plugin = new PluginWrapper(
+      'plugin-with-no-definition',
+      manifest,
+      createPluginInitializerContext(coreContext, manifest)
+    );
+    expect(plugin.getConfigSchema()).toBe(null);
+  });
+  it('returns null for plugins without a server part', () => {
+    const manifest = createPluginManifest({ server: false });
+    const plugin = new PluginWrapper(
+      'plugin-with-no-definition',
+      manifest,
+      createPluginInitializerContext(coreContext, manifest)
+    );
+    expect(plugin.getConfigSchema()).toBe(null);
+  });
+  it('throws if plugin contains invalid schema', () => {
+    jest.doMock(
+      'plugin-invalid-schema/server',
+      () => ({
+        configDefinition: {
+          schema: () => null,
+        },
+      }),
+      { virtual: true }
+    );
+    const manifest = createPluginManifest();
+    const plugin = new PluginWrapper(
+      'plugin-invalid-schema',
+      manifest,
+      createPluginInitializerContext(coreContext, manifest)
+    );
+    expect(() => plugin.getConfigSchema()).toThrowErrorMatchingInlineSnapshot(
+      `"Configuration schema expected to be an instance of Type"`
+    );
+  });
+});

src/core/server/plugins/plugin.ts

@@ -184,7 +184,6 @@ export class PluginWrapper<
   constructor(
     public readonly path: string,
     public readonly manifest: PluginManifest,
-    public readonly schema: PluginConfigSchema,
     private readonly initializerContext: PluginInitializerContext
   ) {
     this.log = initializerContext.logger.get();
@@ -243,6 +242,24 @@ export class PluginWrapper<
     this.instance = undefined;
   }
 
+  public getConfigSchema(): PluginConfigSchema {
+    if (!this.manifest.server) return null;
+    const pluginPathServer = join(this.path, 'server');
+    // eslint-disable-next-line @typescript-eslint/no-var-requires
+    const pluginDefinition = require(pluginPathServer);
+
+    if (!('configDefinition' in pluginDefinition)) {
+      this.log.debug(`"${pluginPathServer}" does not export "configDefinition".`);
+      return null;
+    }
+
+    const configSchema = pluginDefinition.configDefinition.schema;
+    if (!(configSchema instanceof Type)) {
+      throw new Error('Configuration schema expected to be an instance of Type');
+    }
+    return configSchema;
+  }
+
   private createPluginInstance() {
     this.log.debug('Initializing plugin');