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

Introduce support for the server-side new platform plugins. #25473

Merged
merged 17 commits into from
Dec 4, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 6 additions & 5 deletions src/cli/serve/integration_tests/invalid_config.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,11 @@ describe('cli invalid config support', function () {
cwd: ROOT_DIR
});

const logLines = stdout.toString('utf8')
const [fatalLogLine] = stdout.toString('utf8')
.split('\n')
.filter(Boolean)
.map(JSON.parse)
.filter(line => line.tags.includes('fatal'))
.map(obj => ({
...obj,
pid: '## PID ##',
Expand All @@ -45,9 +46,9 @@ describe('cli invalid config support', function () {

expect(error).toBe(undefined);
expect(status).toBe(64);
expect(logLines[0].message).toMatch('{ Error: "unknown.key", "other.unknown.key", "other.third", "some.flat.key", and "' +
'some.array" settings were not applied. Check for spelling errors and ensure that expected plugins are installed.');
expect(logLines[0].tags).toEqual(['fatal', 'root']);
expect(logLines[0].type).toEqual('log');
expect(fatalLogLine.message).toMatch('{ Error: "unknown.key", "other.unknown.key", "other.third", "some.flat.key", and "' +
'some.array" settings were not applied. Check for spelling errors and ensure that expected plugins are installed.');
expect(fatalLogLine.tags).toEqual(['fatal', 'root']);
expect(fatalLogLine.type).toEqual('log');
}, 20 * 1000);
});
6 changes: 1 addition & 5 deletions src/core/server/__snapshots__/index.test.ts.snap
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,7 @@ Object {
"fatal": Array [],
"info": Array [],
"log": Array [],
"trace": Array [
Array [
"some config paths are not handled by the core: [\\"some.path\\",\\"another.path\\"]",
],
],
"trace": Array [],
"warn": Array [],
}
`;
30 changes: 30 additions & 0 deletions src/core/server/config/__snapshots__/env.test.ts.snap
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@ Env {
"buildSha": "XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX",
"version": "v1",
},
"pluginSearchPaths": Array [
"/test/cwd/src/plugins",
"/test/cwd/plugins",
"/test/cwd/../kibana-extra",
Copy link
Member Author

Choose a reason for hiding this comment

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

note: don't be confused, .. is preserved because I mock path.resolve in tests.

],
"staticFilesDir": "/test/cwd/ui",
}
`;
Expand Down Expand Up @@ -68,6 +73,11 @@ Env {
"buildSha": "XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX",
"version": "v1",
},
"pluginSearchPaths": Array [
"/test/cwd/src/plugins",
"/test/cwd/plugins",
"/test/cwd/../kibana-extra",
],
"staticFilesDir": "/test/cwd/ui",
}
`;
Expand Down Expand Up @@ -103,6 +113,11 @@ Env {
"buildSha": "XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX",
"version": "some-version",
},
"pluginSearchPaths": Array [
"/test/cwd/src/plugins",
"/test/cwd/plugins",
"/test/cwd/../kibana-extra",
],
"staticFilesDir": "/test/cwd/ui",
}
`;
Expand Down Expand Up @@ -138,6 +153,11 @@ Env {
"buildSha": "feature-v1-build-sha",
"version": "v1",
},
"pluginSearchPaths": Array [
"/test/cwd/src/plugins",
"/test/cwd/plugins",
"/test/cwd/../kibana-extra",
],
"staticFilesDir": "/test/cwd/ui",
}
`;
Expand Down Expand Up @@ -173,6 +193,11 @@ Env {
"buildSha": "XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX",
"version": "v1",
},
"pluginSearchPaths": Array [
"/test/cwd/src/plugins",
"/test/cwd/plugins",
"/test/cwd/../kibana-extra",
],
"staticFilesDir": "/test/cwd/ui",
}
`;
Expand Down Expand Up @@ -208,6 +233,11 @@ Env {
"buildSha": "feature-v1-build-sha",
"version": "v1",
},
"pluginSearchPaths": Array [
"/some/home/dir/src/plugins",
"/some/home/dir/plugins",
"/some/home/dir/../kibana-extra",
],
"staticFilesDir": "/some/home/dir/ui",
}
`;
18 changes: 18 additions & 0 deletions src/core/server/config/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,26 @@

export type ConfigPath = string | string[];

/**
* Checks whether specified value can be considered as config path.
* @param value Value to check.
* @internal
*/
export function isConfigPath(value: unknown): value is ConfigPath {
if (!value) {
return false;
}

if (typeof value === 'string') {
return true;
}

return Array.isArray(value) && value.every(segment => typeof segment === 'string');
}

/**
* Represents config store.
* @internal
*/
export interface Config {
/**
Expand Down
10 changes: 9 additions & 1 deletion src/core/server/config/config_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import { distinctUntilChanged, first, map } from 'rxjs/operators';
import { Config, ConfigPath, ConfigWithSchema, Env } from '.';
import { Logger, LoggerFactory } from '../logging';

/** @internal */
export class ConfigService {
private readonly log: Logger;

Expand Down Expand Up @@ -107,13 +108,20 @@ export class ConfigService {
return true;
}

public async getUnusedPaths(): Promise<string[]> {
public async getUnusedPaths() {
const config = await this.config$.pipe(first()).toPromise();
const handledPaths = this.handledPaths.map(pathToString);

return config.getFlattenedPaths().filter(path => !isPathHandled(path, handledPaths));
}

public async getUsedPaths() {
Copy link
Member Author

Choose a reason for hiding this comment

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

note: need this for the legacy platform so that it doesn't complain about keys that core knows about (new keys maybe, plugin keys as well).

We'll still need to figure out what to do with the new platform plugin config keys that are not used immediately during startup (like in the testbed plugin in this PR). Maybe we should forbid that and supply config as part of the PluginCore when we start plugin so that these keys are always marked as used....

const config = await this.config$.pipe(first()).toPromise();
const handledPaths = this.handledPaths.map(pathToString);

return config.getFlattenedPaths().filter(path => isPathHandled(path, handledPaths));
}

private createConfig<TSchema extends Type<any>, TConfig>(
path: ConfigPath,
config: Record<string, any>,
Expand Down
19 changes: 18 additions & 1 deletion src/core/server/config/env.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,18 +29,20 @@ export interface PackageInfo {
buildSha: string;
}

interface EnvironmentMode {
export interface EnvironmentMode {
name: 'development' | 'production';
dev: boolean;
prod: boolean;
}

/** @internal */
Copy link
Member Author

Choose a reason for hiding this comment

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

note: I'm adding a bunch of @internal here and there as I'm exposing core types and noticing types that shouldn't be exposed.

export interface EnvOptions {
configs: string[];
cliArgs: CliArgs;
isDevClusterMaster: boolean;
}

/** @internal */
export interface CliArgs {
dev: boolean;
envName?: string;
Expand All @@ -61,10 +63,16 @@ export class Env {
return new Env(process.cwd(), options);
}

/** @internal */
public readonly configDir: string;
/** @internal */
public readonly binDir: string;
/** @internal */
public readonly logDir: string;
/** @internal */
public readonly staticFilesDir: string;
/** @internal */
public readonly pluginSearchPaths: ReadonlyArray<string>;

/**
* Information about Kibana package (version, build number etc.).
Expand All @@ -78,16 +86,19 @@ export class Env {

/**
* Arguments provided through command line.
* @internal
*/
public readonly cliArgs: Readonly<CliArgs>;

/**
* Paths to the configuration files.
* @internal
*/
public readonly configs: ReadonlyArray<string>;

/**
* Indicates that this Kibana instance is run as development Node Cluster master.
* @internal
*/
public readonly isDevClusterMaster: boolean;

Expand All @@ -100,6 +111,12 @@ export class Env {
this.logDir = resolve(this.homeDir, 'log');
this.staticFilesDir = resolve(this.homeDir, 'ui');

this.pluginSearchPaths = [
resolve(this.homeDir, 'src', 'plugins'),
resolve(this.homeDir, 'plugins'),
resolve(this.homeDir, '..', 'kibana-extra'),
];

this.cliArgs = Object.freeze(options.cliArgs);
this.configs = Object.freeze(options.configs);
this.isDevClusterMaster = options.isDevClusterMaster;
Expand Down
10 changes: 8 additions & 2 deletions src/core/server/config/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,16 @@
* under the License.
*/

/** @internal */
export { ConfigService } from './config_service';
/** @internal */
export { RawConfigService } from './raw_config_service';
export { Config, ConfigPath } from './config';
/** @internal */
export { Config, ConfigPath, isConfigPath } from './config';
/** @internal */
export { ObjectToConfigAdapter } from './object_to_config_adapter';
export { Env, CliArgs, PackageInfo } from './env';
/** @internal */
export { CliArgs } from './env';

export { Env, EnvironmentMode, PackageInfo } from './env';
export { ConfigWithSchema } from './config_with_schema';
1 change: 1 addition & 0 deletions src/core/server/config/raw_config_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import { Config } from './config';
import { ObjectToConfigAdapter } from './object_to_config_adapter';
import { getConfigFromFiles } from './read_config';

/** @internal */
export class RawConfigService {
/**
* The stream of configs read from the config file.
Expand Down
1 change: 1 addition & 0 deletions src/core/server/config/read_config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ function merge(target: Record<string, any>, value: any, key?: string) {
return target;
}

/** @internal */
export const getConfigFromFiles = (configFiles: ReadonlyArray<string>) => {
let mergedYaml = {};

Expand Down
6 changes: 5 additions & 1 deletion src/core/server/http/http_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,17 @@
import { Observable, Subscription } from 'rxjs';
import { first } from 'rxjs/operators';

import { CoreService } from '../../types/core_service';
import { CoreService } from '../../types';
import { Logger, LoggerFactory } from '../logging';
import { HttpConfig } from './http_config';
import { HttpServer, HttpServerInfo } from './http_server';
import { HttpsRedirectServer } from './https_redirect_server';
import { Router } from './router';

/** @internal */
export type HttpServiceStartContract = HttpServerInfo;
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't really related to this PR, but I wanted to raise the concern while I saw it so we can address it later: I don't like that HttpServiceStartContract exposes hapi's server object. It seems like we're doing it with HttpServerInfo just so we can support the legacy integration with KbnServer, and that's OK with me because HttpServerInfo is an internal detail to the core system. But we should treat the contracts we return from the service lifecycle functions as public interfaces and ultimately expose them as such to the corresponding lifecycle function on plugins. This convention gives us consistency across services for what functionality gets exposed to plugins, and within core itself you can always pass the entire service to another service if you want to take advantage of core-specific details.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, totally agree, there is no doubt that we shouldn't ever expose something like this (Joi, Hapi or any other internals). We make this nasty exception to support compatibility bridge between new and legacy HTTP servers.


/** @internal */
export class HttpService implements CoreService<HttpServerInfo> {
private readonly httpServer: HttpServer;
private readonly httpsRedirectServer: HttpsRedirectServer;
Expand Down
4 changes: 2 additions & 2 deletions src/core/server/http/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,11 @@ import { Observable } from 'rxjs';

import { LoggerFactory } from '../logging';
import { HttpConfig } from './http_config';
import { HttpService } from './http_service';
import { HttpService, HttpServiceStartContract } from './http_service';
import { Router } from './router';

export { Router, KibanaRequest } from './router';
export { HttpService };
export { HttpService, HttpServiceStartContract };
export { HttpServerInfo } from './http_server';
export { BasePathProxyServer } from './base_path_proxy_server';

Expand Down
12 changes: 9 additions & 3 deletions src/core/server/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,9 @@ test('starts services on "start"', async () => {
const mockHttpServiceStartContract = { something: true };
mockHttpService.start.mockReturnValue(Promise.resolve(mockHttpServiceStartContract));

const mockPluginsServiceStartContract = new Map([['some-plugin', 'some-value']]);
mockPluginsService.start.mockReturnValue(Promise.resolve(mockPluginsServiceStartContract));

const server = new Server(mockConfigService as any, logger, env);

expect(mockHttpService.start).not.toHaveBeenCalled();
Expand All @@ -71,7 +74,10 @@ test('starts services on "start"', async () => {
expect(mockHttpService.start).toHaveBeenCalledTimes(1);
expect(mockPluginsService.start).toHaveBeenCalledTimes(1);
expect(mockLegacyService.start).toHaveBeenCalledTimes(1);
expect(mockLegacyService.start).toHaveBeenCalledWith(mockHttpServiceStartContract);
expect(mockLegacyService.start).toHaveBeenCalledWith({
http: mockHttpServiceStartContract,
plugins: mockPluginsServiceStartContract,
});
});

test('does not fail on "start" if there are unused paths detected', async () => {
Expand All @@ -93,7 +99,7 @@ test('does not start http service is `autoListen:false`', async () => {

expect(mockHttpService.start).not.toHaveBeenCalled();
expect(mockLegacyService.start).toHaveBeenCalledTimes(1);
expect(mockLegacyService.start).toHaveBeenCalledWith(undefined);
expect(mockLegacyService.start).toHaveBeenCalledWith({});
});

test('does not start http service if process is dev cluster master', async () => {
Expand All @@ -109,7 +115,7 @@ test('does not start http service if process is dev cluster master', async () =>

expect(mockHttpService.start).not.toHaveBeenCalled();
expect(mockLegacyService.start).toHaveBeenCalledTimes(1);
expect(mockLegacyService.start).toHaveBeenCalledWith(undefined);
expect(mockLegacyService.start).toHaveBeenCalledWith({});
});

test('stops services on "stop"', async () => {
Expand Down
Loading