[New Platform] Validate config upfront (elastic#35453)

* Introduce new convention for config definition. We need to define a way to acquire configuration schema as a part of plugin definition. Having schema we can split steps of config validation and plugin instantiation. * Discover plugins, read their schema and validate the config. Config validation finished before core services and plugins read from it. That allows us to fail fast and have predictable validation results. * Instantiate plugins using DiscoveredPluginsDefinitions. * Update tests for new API. * test server is not created if config validation fails * move plugin discovery to plugin service pre-setup stage. Set validation schemes in ConfigService.preSetup stage. * fix eslint problem * generate docs * address Rudolfs comments * separate core services and plugins validation * rename files for consistency * address comments for root.js * address comments #1 * useSchema everywhere for consistency. get rid of validateAll * plugin system runs plugin config validation * rename configDefinition * move plugin schema registration in plugins plugins service plugins system is not setup when kibana is run in optimizer mode, so config keys aren't marked as applied. * cleanup * update docs * address comments
sorenlouv · May 10, 2019 · 76ac2b2 · 76ac2b2
1 parent b9799b5
commit 76ac2b2
Show file tree

Hide file tree

Showing 33 changed files with 524 additions and 233 deletions.
diff --git a/docs/development/core/server/kibana-plugin-server.configservice.setschema.md b/docs/development/core/server/kibana-plugin-server.configservice.setschema.md
@@ -0,0 +1,25 @@
+<!-- Do not edit this file. It is automatically generated by API Documenter. -->
+
+[Home](./index) &gt; [kibana-plugin-server](./kibana-plugin-server.md) &gt; [ConfigService](./kibana-plugin-server.configservice.md) &gt; [setSchema](./kibana-plugin-server.configservice.setschema.md)
+
+## ConfigService.setSchema() method
+
+Set config schema for a path and performs its validation
+
+<b>Signature:</b>
+
+```typescript
+setSchema(path: ConfigPath, schema: Type<unknown>): Promise<void>;
+```
+
+## Parameters
+
+|  Parameter | Type | Description |
+|  --- | --- | --- |
+|  path | <code>ConfigPath</code> |  |
+|  schema | <code>Type&lt;unknown&gt;</code> |  |
+
+<b>Returns:</b>
+
+`Promise<void>`
+
diff --git a/src/core/server/__snapshots__/server.test.ts.snap b/src/core/server/__snapshots__/server.test.ts.snap
diff --git a/src/core/server/config/__snapshots__/config_service.test.ts.snap b/src/core/server/config/__snapshots__/config_service.test.ts.snap
diff --git a/src/core/server/config/config_service.mock.ts b/src/core/server/config/config_service.mock.ts
@@ -22,15 +22,16 @@ import { ObjectToConfigAdapter } from './object_to_config_adapter';
 
 import { ConfigService } from './config_service';
 
-type ConfigSericeContract = PublicMethodsOf<ConfigService>;
+type ConfigServiceContract = PublicMethodsOf<ConfigService>;
 const createConfigServiceMock = () => {
-  const mocked: jest.Mocked<ConfigSericeContract> = {
+  const mocked: jest.Mocked<ConfigServiceContract> = {
     atPath: jest.fn(),
     getConfig$: jest.fn(),
     optionalAtPath: jest.fn(),
     getUsedPaths: jest.fn(),
     getUnusedPaths: jest.fn(),
     isEnabledAtPath: jest.fn(),
+    setSchema: jest.fn(),
   };
   mocked.atPath.mockReturnValue(new BehaviorSubject({}));
   mocked.getConfig$.mockReturnValue(new BehaviorSubject(new ObjectToConfigAdapter({})));

diff --git a/src/core/server/config/config_service.test.ts b/src/core/server/config/config_service.test.ts
@@ -34,9 +34,16 @@ const emptyArgv = getEnvOptions();
 const defaultEnv = new Env('/kibana', emptyArgv);
 const logger = loggingServiceMock.create();
 
+class ExampleClassWithStringSchema {
+  public static schema = schema.string();
+
+  constructor(readonly value: string) {}
+}
+
 test('returns config at path as observable', async () => {
   const config$ = new BehaviorSubject(new ObjectToConfigAdapter({ key: 'foo' }));
   const configService = new ConfigService(config$, defaultEnv, logger);
+  await configService.setSchema('key', schema.string());
 
   const configs = configService.atPath('key', ExampleClassWithStringSchema);
   const exampleConfig = await configs.pipe(first()).toPromise();
@@ -45,22 +52,45 @@ test('returns config at path as observable', async () => {
 });
 
 test('throws if config at path does not match schema', async () => {
-  expect.assertions(1);
-
   const config$ = new BehaviorSubject(new ObjectToConfigAdapter({ key: 123 }));
 
   const configService = new ConfigService(config$, defaultEnv, logger);
-  const configs = configService.atPath('key', ExampleClassWithStringSchema);
 
-  try {
-    await configs.pipe(first()).toPromise();
-  } catch (e) {
-    expect(e.message).toMatchSnapshot();
-  }
+  await expect(
+    configService.setSchema('key', schema.string())
+  ).rejects.toThrowErrorMatchingInlineSnapshot(
+    `"[key]: expected value of type [string] but got [number]"`
+  );
+});
+
+test('re-validate config when updated', async () => {
+  const config$ = new BehaviorSubject(new ObjectToConfigAdapter({ key: 'value' }));
+
+  const configService = new ConfigService(config$, defaultEnv, logger);
+  configService.setSchema('key', schema.string());
+
+  const valuesReceived: any[] = [];
+  await configService.atPath('key', ExampleClassWithStringSchema).subscribe(
+    config => {
+      valuesReceived.push(config.value);
+    },
+    error => {
+      valuesReceived.push(error);
+    }
+  );
+
+  config$.next(new ObjectToConfigAdapter({ key: 123 }));
+
+  await expect(valuesReceived).toMatchInlineSnapshot(`
+Array [
+  "value",
+  [Error: [key]: expected value of type [string] but got [number]],
+]
+`);
 });
 
 test("returns undefined if fetching optional config at a path that doesn't exist", async () => {
-  const config$ = new BehaviorSubject(new ObjectToConfigAdapter({ foo: 'bar' }));
+  const config$ = new BehaviorSubject(new ObjectToConfigAdapter({}));
   const configService = new ConfigService(config$, defaultEnv, logger);
 
   const configs = configService.optionalAtPath('unique-name', ExampleClassWithStringSchema);
@@ -72,6 +102,7 @@ test("returns undefined if fetching optional config at a path that doesn't exist
 test('returns observable config at optional path if it exists', async () => {
   const config$ = new BehaviorSubject(new ObjectToConfigAdapter({ value: 'bar' }));
   const configService = new ConfigService(config$, defaultEnv, logger);
+  await configService.setSchema('value', schema.string());
 
   const configs = configService.optionalAtPath('value', ExampleClassWithStringSchema);
   const exampleConfig: any = await configs.pipe(first()).toPromise();
@@ -83,6 +114,7 @@ test('returns observable config at optional path if it exists', async () => {
 test("does not push new configs when reloading if config at path hasn't changed", async () => {
   const config$ = new BehaviorSubject(new ObjectToConfigAdapter({ key: 'value' }));
   const configService = new ConfigService(config$, defaultEnv, logger);
+  await configService.setSchema('key', schema.string());
 
   const valuesReceived: any[] = [];
   configService.atPath('key', ExampleClassWithStringSchema).subscribe(config => {
@@ -97,6 +129,7 @@ test("does not push new configs when reloading if config at path hasn't changed"
 test('pushes new config when reloading and config at path has changed', async () => {
   const config$ = new BehaviorSubject(new ObjectToConfigAdapter({ key: 'value' }));
   const configService = new ConfigService(config$, defaultEnv, logger);
+  await configService.setSchema('key', schema.string());
 
   const valuesReceived: any[] = [];
   configService.atPath('key', ExampleClassWithStringSchema).subscribe(config => {
@@ -108,21 +141,27 @@ test('pushes new config when reloading and config at path has changed', async ()
   expect(valuesReceived).toEqual(['value', 'new value']);
 });
 
-test("throws error if config class does not implement 'schema'", async () => {
-  expect.assertions(1);
-
+test("throws error if 'schema' is not defined for a key", async () => {
   class ExampleClass {}
 
   const config$ = new BehaviorSubject(new ObjectToConfigAdapter({ key: 'value' }));
   const configService = new ConfigService(config$, defaultEnv, logger);
 
   const configs = configService.atPath('key', ExampleClass as any);
 
-  try {
-    await configs.pipe(first()).toPromise();
-  } catch (e) {
-    expect(e).toMatchSnapshot();
-  }
+  await expect(configs.pipe(first()).toPromise()).rejects.toMatchInlineSnapshot(
+    `[Error: No validation schema has been defined for key]`
+  );
+});
+
+test("throws error if 'setSchema' called several times for the same key", async () => {
+  const config$ = new BehaviorSubject(new ObjectToConfigAdapter({ key: 'value' }));
+  const configService = new ConfigService(config$, defaultEnv, logger);
+  const addSchema = async () => await configService.setSchema('key', schema.string());
+  await addSchema();
+  await expect(addSchema()).rejects.toMatchInlineSnapshot(
+    `[Error: Validation schema for key was already registered.]`
+  );
 });
 
 test('tracks unhandled paths', async () => {
@@ -178,28 +217,25 @@ test('correctly passes context', async () => {
   const env = new Env('/kibana', getEnvOptions());
 
   const config$ = new BehaviorSubject(new ObjectToConfigAdapter({ foo: {} }));
+  const schemaDefinition = schema.object({
+    branchRef: schema.string({
+      defaultValue: schema.contextRef('branch'),
+    }),
+    buildNumRef: schema.number({
+      defaultValue: schema.contextRef('buildNum'),
+    }),
+    buildShaRef: schema.string({
+      defaultValue: schema.contextRef('buildSha'),
+    }),
+    devRef: schema.boolean({ defaultValue: schema.contextRef('dev') }),
+    prodRef: schema.boolean({ defaultValue: schema.contextRef('prod') }),
+    versionRef: schema.string({
+      defaultValue: schema.contextRef('version'),
+    }),
+  });
   const configService = new ConfigService(config$, env, logger);
-  const configs = configService.atPath(
-    'foo',
-    createClassWithSchema(
-      schema.object({
-        branchRef: schema.string({
-          defaultValue: schema.contextRef('branch'),
-        }),
-        buildNumRef: schema.number({
-          defaultValue: schema.contextRef('buildNum'),
-        }),
-        buildShaRef: schema.string({
-          defaultValue: schema.contextRef('buildSha'),
-        }),
-        devRef: schema.boolean({ defaultValue: schema.contextRef('dev') }),
-        prodRef: schema.boolean({ defaultValue: schema.contextRef('prod') }),
-        versionRef: schema.string({
-          defaultValue: schema.contextRef('version'),
-        }),
-      })
-    )
-  );
+  await configService.setSchema('foo', schemaDefinition);
+  const configs = configService.atPath('foo', createClassWithSchema(schemaDefinition));
 
   expect(await configs.pipe(first()).toPromise()).toMatchSnapshot();
 });
@@ -278,9 +314,3 @@ function createClassWithSchema(s: Type<any>) {
     constructor(readonly value: TypeOf<typeof s>) {}
   };
 }
-
-class ExampleClassWithStringSchema {
-  public static schema = schema.string();
-
-  constructor(readonly value: string) {}
-}
diff --git a/src/core/server/config/config_service.ts b/src/core/server/config/config_service.ts
@@ -34,6 +34,7 @@ export class ConfigService {
    * then list all unhandled config paths when the startup process is completed.
    */
   private readonly handledPaths: ConfigPath[] = [];
+  private readonly schemas = new Map<string, Type<unknown>>();
 
   constructor(
     private readonly config$: Observable<Config>,
@@ -43,6 +44,22 @@ export class ConfigService {
     this.log = logger.get('config');
   }
 
+  /**
+   * Set config schema for a path and performs its validation
+   */
+  public async setSchema(path: ConfigPath, schema: Type<unknown>) {
+    const namespace = pathToString(path);
+    if (this.schemas.has(namespace)) {
+      throw new Error(`Validation schema for ${path} was already registered.`);
+    }
+
+    this.schemas.set(namespace, schema);
+
+    await this.validateConfig(path)
+      .pipe(first())
+      .toPromise();
+  }
+
   /**
    * Returns the full config object observable. This is not intended for
    * "normal use", but for features that _need_ access to the full object.
@@ -59,13 +76,11 @@ export class ConfigService {
    * @param ConfigClass - A class (not an instance of a class) that contains a
    * static `schema` that we validate the config at the given `path` against.
    */
-  public atPath<TSchema extends Type<any>, TConfig>(
+  public atPath<TSchema extends Type<unknown>, TConfig>(
     path: ConfigPath,
     ConfigClass: ConfigWithSchema<TSchema, TConfig>
   ) {
-    return this.getDistinctConfig(path).pipe(
-      map(config => this.createConfig(path, config, ConfigClass))
-    );
+    return this.validateConfig(path).pipe(map(config => this.createConfig(config, ConfigClass)));
   }
 
   /**
@@ -79,9 +94,11 @@ export class ConfigService {
     ConfigClass: ConfigWithSchema<TSchema, TConfig>
   ) {
     return this.getDistinctConfig(path).pipe(
-      map(config =>
-        config === undefined ? undefined : this.createConfig(path, config, ConfigClass)
-      )
+      map(config => {
+        if (config === undefined) return undefined;
+        const validatedConfig = this.validate(path, config);
+        return this.createConfig(validatedConfig, ConfigClass);
+      })
     );
   }
 
@@ -122,24 +139,13 @@ export class ConfigService {
     return config.getFlattenedPaths().filter(path => isPathHandled(path, handledPaths));
   }
 
-  private createConfig<TSchema extends Type<any>, TConfig>(
-    path: ConfigPath,
-    config: Record<string, any>,
-    ConfigClass: ConfigWithSchema<TSchema, TConfig>
-  ) {
-    const namespace = Array.isArray(path) ? path.join('.') : path;
-
-    const configSchema = ConfigClass.schema;
-
-    if (configSchema === undefined || typeof configSchema.validate !== 'function') {
-      throw new Error(
-        `The config class [${
-          ConfigClass.name
-        }] did not contain a static 'schema' field, which is required when creating a config instance`
-      );
+  private validate(path: ConfigPath, config: Record<string, unknown>) {
+    const namespace = pathToString(path);
+    const schema = this.schemas.get(namespace);
+    if (!schema) {
+      throw new Error(`No validation schema has been defined for ${namespace}`);
     }
-
-    const validatedConfig = ConfigClass.schema.validate(
+    return schema.validate(
       config,
       {
         dev: this.env.mode.dev,
@@ -148,9 +154,19 @@ export class ConfigService {
       },
       namespace
     );
+  }
+
+  private createConfig<TSchema extends Type<unknown>, TConfig>(
+    validatedConfig: unknown,
+    ConfigClass: ConfigWithSchema<TSchema, TConfig>
+  ) {
     return new ConfigClass(validatedConfig, this.env);
   }
 
+  private validateConfig(path: ConfigPath) {
+    return this.getDistinctConfig(path).pipe(map(config => this.validate(path, config)));
+  }
+
   private getDistinctConfig(path: ConfigPath) {
     this.markAsHandled(path);
 

diff --git a/src/core/server/dev/dev_config.ts b/src/core/server/dev/dev_config.ts
@@ -25,8 +25,12 @@ const createDevSchema = schema.object({
   }),
 });
 
-type DevConfigType = TypeOf<typeof createDevSchema>;
+export const config = {
+  path: 'dev',
+  schema: createDevSchema,
+};
 
+export type DevConfigType = TypeOf<typeof createDevSchema>;
 export class DevConfig {
   /**
    * @internal
@@ -38,7 +42,7 @@ export class DevConfig {
   /**
    * @internal
    */
-  constructor(config: DevConfigType) {
-    this.basePathProxyTargetPort = config.basePathProxyTarget;
+  constructor(rawConfig: DevConfigType) {
+    this.basePathProxyTargetPort = rawConfig.basePathProxyTarget;
   }
 }