Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[New Platform] Validate config upfront #35453

Merged
merged 24 commits into from
May 10, 2019
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
c8f3ce6
Introduce new convention for config definition.
mshustov Apr 23, 2019
1d37d47
Discover plugins, read their schema and validate the config.
mshustov Apr 23, 2019
0b9d5a8
Instantiate plugins using DiscoveredPluginsDefinitions.
mshustov Apr 23, 2019
532089a
Update tests for new API.
mshustov Apr 23, 2019
4d805ad
test server is not created if config validation fails
mshustov Apr 23, 2019
001efc6
Merge branch 'master' into issue-20303-validate-config-upfront
mshustov May 3, 2019
a09520b
move plugin discovery to plugin service pre-setup stage.
mshustov May 3, 2019
75d01ee
fix eslint problem
mshustov May 5, 2019
1c3dfa7
Merge branch 'master' into issue-20303-validate-config-upfront
mshustov May 6, 2019
924fc22
generate docs
mshustov May 6, 2019
be8dc61
address Rudolfs comments
mshustov May 6, 2019
c0a98eb
separate core services and plugins validation
mshustov May 8, 2019
4b582d1
Merge branch 'master' into issue-20303-validate-config-upfront
mshustov May 8, 2019
be4e4d3
rename files for consistency
mshustov May 9, 2019
09cf5f2
address comments for root.js
mshustov May 9, 2019
19c02c4
address comments #1
mshustov May 9, 2019
e622cd3
useSchema everywhere for consistency. get rid of validateAll
mshustov May 9, 2019
f90711a
plugin system runs plugin config validation
mshustov May 9, 2019
12d9947
rename configDefinition
mshustov May 9, 2019
2a01ed7
move plugin schema registration in plugins plugins service
mshustov May 9, 2019
1345904
cleanup
mshustov May 9, 2019
48c99cb
update docs
mshustov May 9, 2019
b8cc18a
Merge branch 'master' into issue-20303-validate-config-upfront
mshustov May 9, 2019
de341ac
address comments
mshustov May 10, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 0 additions & 17 deletions src/core/server/__snapshots__/server.test.ts.snap

This file was deleted.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions src/core/server/config/config_service.mock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ const createConfigServiceMock = () => {
getUsedPaths: jest.fn(),
getUnusedPaths: jest.fn(),
isEnabledAtPath: jest.fn(),
preSetup: jest.fn(),
};
mocked.atPath.mockReturnValue(new BehaviorSubject({}));
mocked.getConfig$.mockReturnValue(new BehaviorSubject(new ObjectToConfigAdapter({})));
Expand Down
85 changes: 50 additions & 35 deletions src/core/server/config/config_service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,18 @@ const emptyArgv = getEnvOptions();
const defaultEnv = new Env('/kibana', emptyArgv);
const logger = loggingServiceMock.create();

class ExampleClassWithStringSchema {
public static schema = schema.string();

constructor(readonly value: string) {}
}

const stringSchemaFor = (key: string) => new Map([[key, schema.string()]]);

test('returns config at path as observable', async () => {
const config$ = new BehaviorSubject(new ObjectToConfigAdapter({ key: 'foo' }));
const configService = new ConfigService(config$, defaultEnv, logger);
configService.preSetup(stringSchemaFor('key'));

const configs = configService.atPath('key', ExampleClassWithStringSchema);
const exampleConfig = await configs.pipe(first()).toPromise();
Expand All @@ -50,6 +59,8 @@ test('throws if config at path does not match schema', async () => {
const config$ = new BehaviorSubject(new ObjectToConfigAdapter({ key: 123 }));

const configService = new ConfigService(config$, defaultEnv, logger);
configService.preSetup(stringSchemaFor('key'));

const configs = configService.atPath('key', ExampleClassWithStringSchema);

try {
Expand All @@ -62,6 +73,7 @@ test('throws if config at path does not match schema', async () => {
test("returns undefined if fetching optional config at a path that doesn't exist", async () => {
const config$ = new BehaviorSubject(new ObjectToConfigAdapter({ foo: 'bar' }));
const configService = new ConfigService(config$, defaultEnv, logger);
configService.preSetup(stringSchemaFor('unique-name'));

const configs = configService.optionalAtPath('unique-name', ExampleClassWithStringSchema);
const exampleConfig = await configs.pipe(first()).toPromise();
Expand All @@ -72,6 +84,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);
configService.preSetup(stringSchemaFor('value'));

const configs = configService.optionalAtPath('value', ExampleClassWithStringSchema);
const exampleConfig: any = await configs.pipe(first()).toPromise();
Expand All @@ -83,6 +96,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);
configService.preSetup(stringSchemaFor('key'));

const valuesReceived: any[] = [];
configService.atPath('key', ExampleClassWithStringSchema).subscribe(config => {
Expand All @@ -97,6 +111,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);
configService.preSetup(stringSchemaFor('key'));

const valuesReceived: any[] = [];
configService.atPath('key', ExampleClassWithStringSchema).subscribe(config => {
Expand All @@ -108,21 +123,29 @@ 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);
mshustov marked this conversation as resolved.
Show resolved Hide resolved

test('throws error if reads config before schema is set', 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);

expect(configs.pipe(first()).toPromise()).rejects.toMatchInlineSnapshot(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: I believe this test will pass even if you change rejects to resolves since you don't await on expect 😉 And in test below.

Actually I'm surprised that we don't have any automatic linter-like check for cases like this :/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alright, I expected that jest marks test as async if I call rejects/resolves in assertion

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

linter rule is not implemented 😞 jest-community/eslint-plugin-jest#54

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh, too bad

`[Error: No validation schema has been defined]`
);
});

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);
configService.preSetup(stringSchemaFor('no-key'));
const configs = configService.atPath('key', ExampleClass as any);

try {
await configs.pipe(first()).toPromise();
} catch (e) {
expect(e).toMatchSnapshot();
}
expect(configs.pipe(first()).toPromise()).rejects.toMatchInlineSnapshot(
`[Error: No config schema defined for key]`
);
});

test('tracks unhandled paths', async () => {
Expand Down Expand Up @@ -178,28 +201,26 @@ 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'),
}),
})
)
);
configService.preSetup(new Map([['foo', schemaDefinition]]));

const configs = configService.atPath('foo', createClassWithSchema(schemaDefinition));

expect(await configs.pipe(first()).toPromise()).toMatchSnapshot();
});
Expand Down Expand Up @@ -278,9 +299,3 @@ function createClassWithSchema(s: Type<any>) {
constructor(readonly value: TypeOf<typeof s>) {}
};
}

class ExampleClassWithStringSchema {
public static schema = schema.string();

constructor(readonly value: string) {}
}
62 changes: 40 additions & 22 deletions src/core/server/config/config_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ export class ConfigService {
* then list all unhandled config paths when the startup process is completed.
*/
private readonly handledPaths: ConfigPath[] = [];
private schemas?: Map<string, Type<any>>;

constructor(
private readonly config$: Observable<Config>,
Expand All @@ -42,6 +43,21 @@ export class ConfigService {
) {
this.log = logger.get('config');
}
/**
* Defines a validation schema for an appropriate path in config object.
* Performs validation for initial values.
* @internal
*/
public async preSetup(schemas: Map<ConfigPath, Type<any>>) {
this.schemas = new Map();
for (const [path, schema] of schemas) {
const namespace = pathToString(path);
this.schemas.set(namespace, schema);
await this.getValidatedConfig(namespace)
.pipe(first())
.toPromise();
}
}

/**
* Returns the full config object observable. This is not intended for
Expand All @@ -63,8 +79,8 @@ export class ConfigService {
path: ConfigPath,
ConfigClass: ConfigWithSchema<TSchema, TConfig>
) {
return this.getDistinctConfig(path).pipe(
map(config => this.createConfig(path, config, ConfigClass))
return this.getValidatedConfig(path).pipe(
map(config => this.createConfig(config, ConfigClass))
);
}

Expand All @@ -79,9 +95,7 @@ export class ConfigService {
ConfigClass: ConfigWithSchema<TSchema, TConfig>
) {
return this.getDistinctConfig(path).pipe(
map(config =>
config === undefined ? undefined : this.createConfig(path, config, ConfigClass)
)
map(config => (config === undefined ? undefined : this.createConfig(config, ConfigClass)))
);
}

Expand Down Expand Up @@ -122,24 +136,16 @@ 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 validateConfig(path: ConfigPath, config: Record<string, any>) {
if (!this.schemas) {
throw new Error('No validation schema has been defined');
}

const validatedConfig = ConfigClass.schema.validate(
const namespace = pathToString(path);
const schema = this.schemas.get(namespace);
if (!schema) {
throw new Error(`No config schema defined for ${namespace}`);
}
const validatedConfig = schema.validate(
config,
{
dev: this.env.mode.dev,
Expand All @@ -148,9 +154,21 @@ export class ConfigService {
},
namespace
);

return validatedConfig;
}

private createConfig<TSchema extends Type<any>, TConfig>(
validatedConfig: Record<string, any>,
ConfigClass: ConfigWithSchema<TSchema, TConfig>
) {
return new ConfigClass(validatedConfig, this.env);
}

private getValidatedConfig(path: ConfigPath) {
return this.getDistinctConfig(path).pipe(map(config => this.validateConfig(path, config)));
}

private getDistinctConfig(path: ConfigPath) {
this.markAsHandled(path);

Expand Down
6 changes: 5 additions & 1 deletion src/core/server/dev/dev_config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,12 @@ const createDevSchema = schema.object({
}),
});

type DevConfigType = TypeOf<typeof createDevSchema>;
export const configDefinition = {
configPath: 'dev',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: configPath ---> path as config part seems to be obvious here?

I don't have a strong opinion on the name, but curious if we can be a tad more consistent and have something like this instead (here and maybe everywhere else)?

export const DevConfigDefinition = Object.freeze({
  configPath: 'dev',
  schema: schema.object({
    basePathProxyTarget: schema.number({
      defaultValue: 5603,
    }),
  }),
});

export type DevConfigType = TypeOf<typeof DevConfigDefinition.schema>;

export class DevConfig {
  public static schema = DevConfigDefinition.schema;
  constructor(config: DevConfigType) {}
}

When you simplify config handling in the follow-ups we'll be able to drop DevConfig completely.

But at the same time I understand that you want it to look the same as we do it for plugins, so I'm torn on this one

schema: createDevSchema,
};

export type DevConfigType = TypeOf<typeof createDevSchema>;
export class DevConfig {
/**
* @internal
Expand Down
2 changes: 1 addition & 1 deletion src/core/server/dev/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,4 @@
* under the License.
*/

export { DevConfig } from './dev_config';
export { DevConfig, configDefinition } from './dev_config';
7 changes: 7 additions & 0 deletions src/core/server/elasticsearch/elasticsearch_config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,13 @@ const configSchema = schema.object({

type SslConfigSchema = TypeOf<typeof configSchema>['ssl'];

export type ElasticsearchConfigType = TypeOf<typeof configSchema>;
mshustov marked this conversation as resolved.
Show resolved Hide resolved

export const configDefinition = {
configPath: 'elasticsearch',
schema: configSchema,
};

export class ElasticsearchConfig {
public static schema = configSchema;

Expand Down
28 changes: 15 additions & 13 deletions src/core/server/elasticsearch/elasticsearch_service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,32 +22,34 @@ import { first } from 'rxjs/operators';
import { MockClusterClient } from './elasticsearch_service.test.mocks';

import { BehaviorSubject, combineLatest } from 'rxjs';
import { Config, ConfigService, Env, ObjectToConfigAdapter } from '../config';
import { Env } from '../config';
import { getEnvOptions } from '../config/__mocks__/env';
import { CoreContext } from '../core_context';
import { configServiceMock } from '../config/config_service.mock';
import { loggingServiceMock } from '../logging/logging_service.mock';
import { ElasticsearchConfig } from './elasticsearch_config';
import { ElasticsearchService } from './elasticsearch_service';

let elasticsearchService: ElasticsearchService;
let configService: ConfigService;
const configService = configServiceMock.create();
configService.atPath.mockReturnValue(
new BehaviorSubject(
new ElasticsearchConfig({
hosts: ['http://1.2.3.4'],
username: 'jest',
healthCheck: {},
ssl: {},
} as any)
)
);

let env: Env;
let coreContext: CoreContext;
const logger = loggingServiceMock.create();
beforeEach(() => {
env = Env.createDefault(getEnvOptions());

configService = new ConfigService(
new BehaviorSubject<Config>(
new ObjectToConfigAdapter({
elasticsearch: { hosts: ['http://1.2.3.4'], username: 'jest' },
})
),
env,
logger
);

coreContext = { env, logger, configService };
coreContext = { env, logger, configService: configService as any };
elasticsearchService = new ElasticsearchService(coreContext);
});

Expand Down
1 change: 1 addition & 0 deletions src/core/server/elasticsearch/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,4 @@ export { ElasticsearchServiceSetup, ElasticsearchService } from './elasticsearch
export { CallAPIOptions, ClusterClient } from './cluster_client';
export { ScopedClusterClient, Headers, APICaller } from './scoped_cluster_client';
export { ElasticsearchClientConfig } from './elasticsearch_client_config';
export { configDefinition } from './elasticsearch_config';
5 changes: 5 additions & 0 deletions src/core/server/http/http_config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,11 @@ const createHttpSchema = schema.object(

export type HttpConfigType = TypeOf<typeof createHttpSchema>;

export const configDefinition = {
configPath: 'server',
schema: createHttpSchema,
};

export class HttpConfig {
/**
* @internal
Expand Down
Loading