Skip to content

Commit 1e757cf

Browse files
tsullivanJoel Griffithelasticmachine
committed
[Reporting] Convert plugin setup and start to synchronous (#68326)
* [Reporting] Convert plugin setup and start to synchronous * revert class conversion * diff prettify * Fix test mocks + add some plugin async helpers where needed * no setupReady startReady needed * Add plugin test for sync/error cases * PR feedback on tests/setup logs * rename symbol Co-authored-by: Joel Griffith <joel.griffith@elastic.co> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
1 parent b66e462 commit 1e757cf

18 files changed

Lines changed: 324 additions & 163 deletions

File tree

‎x-pack/plugins/reporting/server/browsers/chromium/driver_factory/index.ts‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,22 +28,25 @@ import { getChromeLogLocation } from '../paths';
2828
import { puppeteerLaunch } from '../puppeteer';
2929
import { args } from './args';
3030

31-
type binaryPath = string;
3231
type BrowserConfig = CaptureConfig['browser']['chromium'];
3332
type ViewportConfig = CaptureConfig['viewport'];
3433

3534
export class HeadlessChromiumDriverFactory {
36-
private binaryPath: binaryPath;
35+
private binaryPath: string;
3736
private captureConfig: CaptureConfig;
3837
private browserConfig: BrowserConfig;
3938
private userDataDir: string;
4039
private getChromiumArgs: (viewport: ViewportConfig) => string[];
4140

42-
constructor(binaryPath: binaryPath, logger: LevelLogger, captureConfig: CaptureConfig) {
41+
constructor(binaryPath: string, captureConfig: CaptureConfig, logger: LevelLogger) {
4342
this.binaryPath = binaryPath;
4443
this.captureConfig = captureConfig;
4544
this.browserConfig = captureConfig.browser.chromium;
4645

46+
if (this.browserConfig.disableSandbox) {
47+
logger.warning(`Enabling the Chromium sandbox provides an additional layer of protection.`);
48+
}
49+
4750
this.userDataDir = fs.mkdtempSync(path.join(os.tmpdir(), 'chromium-'));
4851
this.getChromiumArgs = (viewport: ViewportConfig) =>
4952
args({

‎x-pack/plugins/reporting/server/browsers/chromium/index.ts‎

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -4,16 +4,14 @@
44
* you may not use this file except in compliance with the Elastic License.
55
*/
66

7+
import { BrowserDownload } from '../';
78
import { CaptureConfig } from '../../../server/types';
89
import { LevelLogger } from '../../lib';
910
import { HeadlessChromiumDriverFactory } from './driver_factory';
11+
import { paths } from './paths';
1012

11-
export { paths } from './paths';
12-
13-
export async function createDriverFactory(
14-
binaryPath: string,
15-
logger: LevelLogger,
16-
captureConfig: CaptureConfig
17-
): Promise<HeadlessChromiumDriverFactory> {
18-
return new HeadlessChromiumDriverFactory(binaryPath, logger, captureConfig);
19-
}
13+
export const chromium: BrowserDownload = {
14+
paths,
15+
createDriverFactory: (binaryPath: string, captureConfig: CaptureConfig, logger: LevelLogger) =>
16+
new HeadlessChromiumDriverFactory(binaryPath, captureConfig, logger),
17+
};

‎x-pack/plugins/reporting/server/browsers/create_browser_driver_factory.ts‎

Lines changed: 0 additions & 45 deletions
This file was deleted.

‎x-pack/plugins/reporting/server/browsers/download/ensure_downloaded.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ async function ensureDownloaded(browsers: BrowserDownload[], logger: LevelLogger
5656
const path = resolvePath(archivesPath, archiveFilename);
5757

5858
if (existsSync(path) && (await md5(path)) === archiveChecksum) {
59-
logger.info(`Browser archive exists in ${path}`);
59+
logger.debug(`Browser archive exists in ${path}`);
6060
return;
6161
}
6262

‎x-pack/plugins/reporting/server/browsers/index.ts‎

Lines changed: 24 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4,20 +4,27 @@
44
* you may not use this file except in compliance with the Elastic License.
55
*/
66

7-
import * as chromiumDefinition from './chromium';
7+
import { first } from 'rxjs/operators';
8+
import { LevelLogger } from '../lib';
9+
import { CaptureConfig } from '../types';
10+
import { chromium } from './chromium';
11+
import { HeadlessChromiumDriverFactory } from './chromium/driver_factory';
12+
import { installBrowser } from './install';
13+
import { ReportingConfig } from '..';
814

915
export { ensureAllBrowsersDownloaded } from './download';
10-
export { createBrowserDriverFactory } from './create_browser_driver_factory';
11-
1216
export { HeadlessChromiumDriver } from './chromium/driver';
1317
export { HeadlessChromiumDriverFactory } from './chromium/driver_factory';
18+
export { chromium } from './chromium';
1419

15-
export const chromium = {
16-
paths: chromiumDefinition.paths,
17-
createDriverFactory: chromiumDefinition.createDriverFactory,
18-
};
20+
type CreateDriverFactory = (
21+
binaryPath: string,
22+
captureConfig: CaptureConfig,
23+
logger: LevelLogger
24+
) => HeadlessChromiumDriverFactory;
1925

2026
export interface BrowserDownload {
27+
createDriverFactory: CreateDriverFactory;
2128
paths: {
2229
archivesPath: string;
2330
baseUrl: string;
@@ -30,3 +37,13 @@ export interface BrowserDownload {
3037
}>;
3138
};
3239
}
40+
41+
export const initializeBrowserDriverFactory = async (
42+
config: ReportingConfig,
43+
logger: LevelLogger
44+
) => {
45+
const { binaryPath$ } = installBrowser(chromium, config, logger);
46+
const binaryPath = await binaryPath$.pipe(first()).toPromise();
47+
const captureConfig = config.get('capture');
48+
return chromium.createDriverFactory(binaryPath, captureConfig, logger);
49+
};

‎x-pack/plugins/reporting/server/browsers/install.ts‎

Lines changed: 48 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,13 @@
66

77
import fs from 'fs';
88
import path from 'path';
9+
import * as Rx from 'rxjs';
10+
import { first } from 'rxjs/operators';
911
import { promisify } from 'util';
12+
import { ReportingConfig } from '../';
1013
import { LevelLogger } from '../lib';
1114
import { BrowserDownload } from './';
15+
import { ensureBrowserDownloaded } from './download';
1216
// @ts-ignore
1317
import { md5 } from './download/checksum';
1418
// @ts-ignore
@@ -19,37 +23,60 @@ const chmod = promisify(fs.chmod);
1923
interface Package {
2024
platforms: string[];
2125
}
22-
interface PathResponse {
23-
binaryPath: string;
24-
}
2526

2627
/**
2728
* "install" a browser by type into installs path by extracting the downloaded
2829
* archive. If there is an error extracting the archive an `ExtractError` is thrown
2930
*/
30-
export async function installBrowser(
31-
logger: LevelLogger,
31+
export function installBrowser(
3232
browser: BrowserDownload,
33-
installsPath: string
34-
): Promise<PathResponse> {
35-
const pkg = browser.paths.packages.find((p: Package) => p.platforms.includes(process.platform));
33+
config: ReportingConfig,
34+
logger: LevelLogger
35+
): { binaryPath$: Rx.Subject<string> } {
36+
const binaryPath$ = new Rx.Subject<string>();
37+
const backgroundInstall = async () => {
38+
const captureConfig = config.get('capture');
39+
const { autoDownload, type: browserType } = captureConfig.browser;
40+
if (autoDownload) {
41+
await ensureBrowserDownloaded(browserType, logger);
42+
}
43+
44+
const pkg = browser.paths.packages.find((p: Package) => p.platforms.includes(process.platform));
45+
if (!pkg) {
46+
throw new Error(`Unsupported platform: ${JSON.stringify(browser, null, 2)}`);
47+
}
48+
49+
const dataDir = await config.kbnConfig.get('path', 'data').pipe(first()).toPromise();
50+
const binaryPath = path.join(dataDir, pkg.binaryRelativePath);
51+
52+
try {
53+
const binaryChecksum = await md5(binaryPath).catch(() => '');
3654

37-
if (!pkg) {
38-
throw new Error(`Unsupported platform: ${JSON.stringify(browser, null, 2)}`);
39-
}
55+
if (binaryChecksum !== pkg.binaryChecksum) {
56+
const archive = path.join(browser.paths.archivesPath, pkg.archiveFilename);
57+
logger.info(`Extracting [${archive}] to [${binaryPath}]`);
58+
await extract(archive, dataDir);
59+
await chmod(binaryPath, '755');
60+
}
61+
} catch (error) {
62+
if (error.cause && ['EACCES', 'EEXIST'].includes(error.cause.code)) {
63+
logger.error(
64+
`Error code ${error.cause.code}: Insufficient permissions for extracting the browser archive. ` +
65+
`Make sure the Kibana data directory (path.data) is owned by the same user that is running Kibana.`
66+
);
67+
}
4068

41-
const binaryPath = path.join(installsPath, pkg.binaryRelativePath);
42-
const binaryChecksum = await md5(binaryPath).catch(() => '');
69+
throw error; // reject the promise with the original error
70+
}
71+
72+
logger.debug(`Browser executable: ${binaryPath}`);
73+
74+
binaryPath$.next(binaryPath); // subscribers wait for download and extract to complete
75+
};
4376

44-
if (binaryChecksum !== pkg.binaryChecksum) {
45-
const archive = path.join(browser.paths.archivesPath, pkg.archiveFilename);
46-
logger.debug(`Extracting [${archive}] to [${binaryPath}]`);
47-
await extract(archive, installsPath);
48-
await chmod(binaryPath, '755');
49-
}
77+
backgroundInstall();
5078

51-
logger.debug(`Browser installed at ${binaryPath}`);
5279
return {
53-
binaryPath,
80+
binaryPath$,
5481
};
5582
}

‎x-pack/plugins/reporting/server/config/config.ts‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,12 @@
44
* you may not use this file except in compliance with the Elastic License.
55
*/
66

7-
import { Observable } from 'rxjs';
87
import { get } from 'lodash';
9-
import { map } from 'rxjs/operators';
8+
import { Observable } from 'rxjs';
9+
import { first, map } from 'rxjs/operators';
1010
import { CoreSetup, PluginInitializerContext } from 'src/core/server';
11+
import { LevelLogger } from '../lib';
12+
import { createConfig$ } from './create_config';
1113
import { ReportingConfigType } from './schema';
1214

1315
// make config.get() aware of the value type it returns
@@ -55,11 +57,12 @@ export interface ReportingConfig extends Config<ReportingConfigType> {
5557
kbnConfig: Config<KbnServerConfigType>;
5658
}
5759

58-
export const buildConfig = (
60+
export const buildConfig = async (
5961
initContext: PluginInitializerContext<ReportingConfigType>,
6062
core: CoreSetup,
61-
reportingConfig: ReportingConfigType
62-
): ReportingConfig => {
63+
logger: LevelLogger
64+
): Promise<ReportingConfig> => {
65+
const config$ = initContext.config.create<ReportingConfigType>();
6366
const { http } = core;
6467
const serverInfo = http.getServerInfo();
6568

@@ -77,6 +80,8 @@ export const buildConfig = (
7780
},
7881
};
7982

83+
const reportingConfig$ = createConfig$(core, config$, logger);
84+
const reportingConfig = await reportingConfig$.pipe(first()).toPromise();
8085
return {
8186
get: (...keys: string[]) => get(reportingConfig, keys.join('.'), null), // spreading arguments as an array allows the return type to be known by the compiler
8287
kbnConfig: {

‎x-pack/plugins/reporting/server/config/create_config.test.ts‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,11 @@ describe('Reporting server createConfig$', () => {
4545
mockInitContext = makeMockInitContext({
4646
kibanaServer: {},
4747
});
48-
mockLogger = ({ warn: jest.fn(), debug: jest.fn() } as unknown) as LevelLogger;
48+
mockLogger = ({
49+
warn: jest.fn(),
50+
debug: jest.fn(),
51+
clone: jest.fn().mockImplementation(() => mockLogger),
52+
} as unknown) as LevelLogger;
4953
});
5054

5155
afterEach(() => {

‎x-pack/plugins/reporting/server/config/create_config.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,9 @@ import { ReportingConfigType } from './schema';
2323
export function createConfig$(
2424
core: CoreSetup,
2525
config$: Observable<ReportingConfigType>,
26-
logger: LevelLogger
26+
parentLogger: LevelLogger
2727
) {
28+
const logger = parentLogger.clone(['config']);
2829
return config$.pipe(
2930
map((config) => {
3031
// encryption key

‎x-pack/plugins/reporting/server/config/index.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@
77
import { PluginConfigDescriptor } from 'kibana/server';
88
import { ConfigSchema, ReportingConfigType } from './schema';
99
export { buildConfig } from './config';
10-
export { createConfig$ } from './create_config';
1110
export { ConfigSchema, ReportingConfigType };
1211

1312
export const config: PluginConfigDescriptor<ReportingConfigType> = {

0 commit comments

Comments
 (0)