From 9e654f1106110bfae0d71f9e3ce6e54bf4b70d27 Mon Sep 17 00:00:00 2001 From: Timothy Sullivan Date: Thu, 4 Jun 2020 16:30:55 -0700 Subject: [PATCH] [Reporting] Convert plugin setup and start to synchronous --- .../browsers/chromium/driver_factory/index.ts | 9 +- .../server/browsers/chromium/index.ts | 16 ++-- .../browsers/create_browser_driver_factory.ts | 45 ---------- .../browsers/download/ensure_downloaded.ts | 2 +- .../reporting/server/browsers/index.ts | 32 +++++-- .../reporting/server/browsers/install.ts | 71 ++++++++++------ .../plugins/reporting/server/config/config.ts | 15 ++-- .../reporting/server/config/create_config.ts | 3 +- .../plugins/reporting/server/config/index.ts | 1 - x-pack/plugins/reporting/server/core.ts | 11 ++- .../reporting/server/lib/validate/index.ts | 5 +- x-pack/plugins/reporting/server/plugin.ts | 83 ++++++++++--------- .../create_mock_browserdriverfactory.ts | 10 ++- 13 files changed, 161 insertions(+), 142 deletions(-) delete mode 100644 x-pack/plugins/reporting/server/browsers/create_browser_driver_factory.ts diff --git a/x-pack/plugins/reporting/server/browsers/chromium/driver_factory/index.ts b/x-pack/plugins/reporting/server/browsers/chromium/driver_factory/index.ts index 246c605f4bfe6..3ce5329e42517 100644 --- a/x-pack/plugins/reporting/server/browsers/chromium/driver_factory/index.ts +++ b/x-pack/plugins/reporting/server/browsers/chromium/driver_factory/index.ts @@ -28,22 +28,25 @@ import { getChromeLogLocation } from '../paths'; import { puppeteerLaunch } from '../puppeteer'; import { args } from './args'; -type binaryPath = string; type BrowserConfig = CaptureConfig['browser']['chromium']; type ViewportConfig = CaptureConfig['viewport']; export class HeadlessChromiumDriverFactory { - private binaryPath: binaryPath; + private binaryPath: string; private captureConfig: CaptureConfig; private browserConfig: BrowserConfig; private userDataDir: string; private getChromiumArgs: (viewport: ViewportConfig) => string[]; - constructor(binaryPath: binaryPath, logger: LevelLogger, captureConfig: CaptureConfig) { + constructor(binaryPath: string, captureConfig: CaptureConfig, logger: LevelLogger) { this.binaryPath = binaryPath; this.captureConfig = captureConfig; this.browserConfig = captureConfig.browser.chromium; + if (this.browserConfig.disableSandbox) { + logger.warning(`Enabling the Chromium sandbox provides an additional layer of protection.`); + } + this.userDataDir = fs.mkdtempSync(path.join(os.tmpdir(), 'chromium-')); this.getChromiumArgs = (viewport: ViewportConfig) => args({ diff --git a/x-pack/plugins/reporting/server/browsers/chromium/index.ts b/x-pack/plugins/reporting/server/browsers/chromium/index.ts index 5f89662c94da2..cebcd228b01c3 100644 --- a/x-pack/plugins/reporting/server/browsers/chromium/index.ts +++ b/x-pack/plugins/reporting/server/browsers/chromium/index.ts @@ -4,16 +4,14 @@ * you may not use this file except in compliance with the Elastic License. */ +import { BrowserDownload } from '../'; import { CaptureConfig } from '../../../server/types'; import { LevelLogger } from '../../lib'; import { HeadlessChromiumDriverFactory } from './driver_factory'; +import { paths } from './paths'; -export { paths } from './paths'; - -export async function createDriverFactory( - binaryPath: string, - logger: LevelLogger, - captureConfig: CaptureConfig -): Promise { - return new HeadlessChromiumDriverFactory(binaryPath, logger, captureConfig); -} +export const chromium: BrowserDownload = { + paths, + createDriverFactory: (binaryPath: string, captureConfig: CaptureConfig, logger: LevelLogger) => + new HeadlessChromiumDriverFactory(binaryPath, captureConfig, logger), +}; diff --git a/x-pack/plugins/reporting/server/browsers/create_browser_driver_factory.ts b/x-pack/plugins/reporting/server/browsers/create_browser_driver_factory.ts deleted file mode 100644 index f3486a48ba7b1..0000000000000 --- a/x-pack/plugins/reporting/server/browsers/create_browser_driver_factory.ts +++ /dev/null @@ -1,45 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License; - * you may not use this file except in compliance with the Elastic License. - */ - -import { first } from 'rxjs/operators'; -import { ReportingConfig } from '../'; -import { LevelLogger } from '../lib'; -import { HeadlessChromiumDriverFactory } from './chromium/driver_factory'; -import { ensureBrowserDownloaded } from './download'; -import { chromium } from './index'; -import { installBrowser } from './install'; - -export async function createBrowserDriverFactory( - config: ReportingConfig, - logger: LevelLogger -): Promise { - const captureConfig = config.get('capture'); - const browserConfig = captureConfig.browser.chromium; - const browserAutoDownload = captureConfig.browser.autoDownload; - const browserType = captureConfig.browser.type; - const dataDir = await config.kbnConfig.get('path', 'data').pipe(first()).toPromise(); - - if (browserConfig.disableSandbox) { - logger.warning(`Enabling the Chromium sandbox provides an additional layer of protection.`); - } - if (browserAutoDownload) { - await ensureBrowserDownloaded(browserType, logger); - } - - try { - const { binaryPath } = await installBrowser(logger, chromium, dataDir); - return chromium.createDriverFactory(binaryPath, logger, captureConfig); - } catch (error) { - if (error.cause && ['EACCES', 'EEXIST'].includes(error.cause.code)) { - logger.error( - `Error code ${error.cause.code}: Insufficient permissions for extracting the browser archive. ` + - `Make sure the Kibana data directory (path.data) is owned by the same user that is running Kibana.` - ); - } - - throw error; // reject the promise with the original error - } -} diff --git a/x-pack/plugins/reporting/server/browsers/download/ensure_downloaded.ts b/x-pack/plugins/reporting/server/browsers/download/ensure_downloaded.ts index b334510d71947..add14448e2f1d 100644 --- a/x-pack/plugins/reporting/server/browsers/download/ensure_downloaded.ts +++ b/x-pack/plugins/reporting/server/browsers/download/ensure_downloaded.ts @@ -56,7 +56,7 @@ async function ensureDownloaded(browsers: BrowserDownload[], logger: LevelLogger const path = resolvePath(archivesPath, archiveFilename); if (existsSync(path) && (await md5(path)) === archiveChecksum) { - logger.info(`Browser archive exists in ${path}`); + logger.debug(`Browser archive exists in ${path}`); return; } diff --git a/x-pack/plugins/reporting/server/browsers/index.ts b/x-pack/plugins/reporting/server/browsers/index.ts index 7f6e40fb433b6..966b000f83ea3 100644 --- a/x-pack/plugins/reporting/server/browsers/index.ts +++ b/x-pack/plugins/reporting/server/browsers/index.ts @@ -4,20 +4,28 @@ * you may not use this file except in compliance with the Elastic License. */ -import * as chromiumDefinition from './chromium'; +import { first } from 'rxjs/operators'; +import { LevelLogger } from '../lib'; +import { CaptureConfig } from '../types'; +import { chromium } from './chromium'; +import { HeadlessChromiumDriverFactory } from './chromium/driver_factory'; +import { BrowserInstaller } from './install'; +import { ReportingConfig } from '..'; +export { BrowserInstaller } from './install'; export { ensureAllBrowsersDownloaded } from './download'; -export { createBrowserDriverFactory } from './create_browser_driver_factory'; - export { HeadlessChromiumDriver } from './chromium/driver'; export { HeadlessChromiumDriverFactory } from './chromium/driver_factory'; +export { chromium } from './chromium'; -export const chromium = { - paths: chromiumDefinition.paths, - createDriverFactory: chromiumDefinition.createDriverFactory, -}; +type CreateDriverFactory = ( + binaryPath: string, + captureConfig: CaptureConfig, + logger: LevelLogger +) => HeadlessChromiumDriverFactory; export interface BrowserDownload { + createDriverFactory: CreateDriverFactory; paths: { archivesPath: string; baseUrl: string; @@ -30,3 +38,13 @@ export interface BrowserDownload { }>; }; } + +export const initializeBrowserDriverFactory = async ( + config: ReportingConfig, + logger: LevelLogger +) => { + const { binaryPath$ } = new BrowserInstaller(chromium, config, logger); + const binaryPath = await binaryPath$.pipe(first()).toPromise(); + const captureConfig = config.get('capture'); + return chromium.createDriverFactory(binaryPath, captureConfig, logger); +}; diff --git a/x-pack/plugins/reporting/server/browsers/install.ts b/x-pack/plugins/reporting/server/browsers/install.ts index 01526af307022..ca51df5730162 100644 --- a/x-pack/plugins/reporting/server/browsers/install.ts +++ b/x-pack/plugins/reporting/server/browsers/install.ts @@ -6,9 +6,13 @@ import fs from 'fs'; import path from 'path'; +import * as Rx from 'rxjs'; +import { first } from 'rxjs/operators'; import { promisify } from 'util'; +import { ReportingConfig } from '../'; import { LevelLogger } from '../lib'; import { BrowserDownload } from './'; +import { ensureBrowserDownloaded } from './download'; // @ts-ignore import { md5 } from './download/checksum'; // @ts-ignore @@ -19,37 +23,56 @@ const chmod = promisify(fs.chmod); interface Package { platforms: string[]; } -interface PathResponse { - binaryPath: string; -} /** * "install" a browser by type into installs path by extracting the downloaded * archive. If there is an error extracting the archive an `ExtractError` is thrown */ -export async function installBrowser( - logger: LevelLogger, - browser: BrowserDownload, - installsPath: string -): Promise { - const pkg = browser.paths.packages.find((p: Package) => p.platforms.includes(process.platform)); - - if (!pkg) { - throw new Error(`Unsupported platform: ${JSON.stringify(browser, null, 2)}`); +export class BrowserInstaller { + public readonly binaryPath$: Rx.Subject; + + constructor(browser: BrowserDownload, config: ReportingConfig, logger: LevelLogger) { + this.binaryPath$ = new Rx.Subject(); + this.backgroundInstall(browser, config, logger); } - const binaryPath = path.join(installsPath, pkg.binaryRelativePath); - const binaryChecksum = await md5(binaryPath).catch(() => ''); + async backgroundInstall(browser: BrowserDownload, config: ReportingConfig, logger: LevelLogger) { + const captureConfig = config.get('capture'); + const { autoDownload, type: browserType } = captureConfig.browser; + if (autoDownload) { + await ensureBrowserDownloaded(browserType, logger); + } - if (binaryChecksum !== pkg.binaryChecksum) { - const archive = path.join(browser.paths.archivesPath, pkg.archiveFilename); - logger.debug(`Extracting [${archive}] to [${binaryPath}]`); - await extract(archive, installsPath); - await chmod(binaryPath, '755'); - } + const pkg = browser.paths.packages.find((p: Package) => p.platforms.includes(process.platform)); + if (!pkg) { + throw new Error(`Unsupported platform: ${JSON.stringify(browser, null, 2)}`); + } + + const dataDir = await config.kbnConfig.get('path', 'data').pipe(first()).toPromise(); + const binaryPath = path.join(dataDir, pkg.binaryRelativePath); + + try { + const binaryChecksum = await md5(binaryPath).catch(() => ''); - logger.debug(`Browser installed at ${binaryPath}`); - return { - binaryPath, - }; + if (binaryChecksum !== pkg.binaryChecksum) { + const archive = path.join(browser.paths.archivesPath, pkg.archiveFilename); + logger.info(`Extracting [${archive}] to [${binaryPath}]`); + await extract(archive, dataDir); + await chmod(binaryPath, '755'); + } + } catch (error) { + if (error.cause && ['EACCES', 'EEXIST'].includes(error.cause.code)) { + logger.error( + `Error code ${error.cause.code}: Insufficient permissions for extracting the browser archive. ` + + `Make sure the Kibana data directory (path.data) is owned by the same user that is running Kibana.` + ); + } + + throw error; // reject the promise with the original error + } + + logger.debug(`Browser executable: ${binaryPath}`); + + this.binaryPath$.next(binaryPath); // subscribers wait for download and extract to complete + } } diff --git a/x-pack/plugins/reporting/server/config/config.ts b/x-pack/plugins/reporting/server/config/config.ts index 4142ab6f0ae43..2a09ebea9619c 100644 --- a/x-pack/plugins/reporting/server/config/config.ts +++ b/x-pack/plugins/reporting/server/config/config.ts @@ -4,10 +4,12 @@ * you may not use this file except in compliance with the Elastic License. */ -import { Observable } from 'rxjs'; import { get } from 'lodash'; -import { map } from 'rxjs/operators'; +import { Observable } from 'rxjs'; +import { first, map } from 'rxjs/operators'; import { CoreSetup, PluginInitializerContext } from 'src/core/server'; +import { LevelLogger } from '../lib'; +import { createConfig$ } from './create_config'; import { ReportingConfigType } from './schema'; // make config.get() aware of the value type it returns @@ -55,11 +57,12 @@ export interface ReportingConfig extends Config { kbnConfig: Config; } -export const buildConfig = ( +export const buildConfig = async ( initContext: PluginInitializerContext, core: CoreSetup, - reportingConfig: ReportingConfigType -): ReportingConfig => { + logger: LevelLogger +): Promise => { + const config$ = initContext.config.create(); const { http } = core; const serverInfo = http.getServerInfo(); @@ -77,6 +80,8 @@ export const buildConfig = ( }, }; + const reportingConfig$ = createConfig$(core, config$, logger); + const reportingConfig = await reportingConfig$.pipe(first()).toPromise(); return { get: (...keys: string[]) => get(reportingConfig, keys.join('.'), null), // spreading arguments as an array allows the return type to be known by the compiler kbnConfig: { diff --git a/x-pack/plugins/reporting/server/config/create_config.ts b/x-pack/plugins/reporting/server/config/create_config.ts index 67df7dacc77ab..5c66bd599dd9a 100644 --- a/x-pack/plugins/reporting/server/config/create_config.ts +++ b/x-pack/plugins/reporting/server/config/create_config.ts @@ -23,8 +23,9 @@ import { ReportingConfigType } from './schema'; export function createConfig$( core: CoreSetup, config$: Observable, - logger: LevelLogger + parentLogger: LevelLogger ) { + const logger = parentLogger.clone(['config']); return config$.pipe( map((config) => { // encryption key diff --git a/x-pack/plugins/reporting/server/config/index.ts b/x-pack/plugins/reporting/server/config/index.ts index caa64a7414005..a89b952702e1b 100644 --- a/x-pack/plugins/reporting/server/config/index.ts +++ b/x-pack/plugins/reporting/server/config/index.ts @@ -7,7 +7,6 @@ import { PluginConfigDescriptor } from 'kibana/server'; import { ConfigSchema, ReportingConfigType } from './schema'; export { buildConfig } from './config'; -export { createConfig$ } from './create_config'; export { ConfigSchema, ReportingConfigType }; export const config: PluginConfigDescriptor = { diff --git a/x-pack/plugins/reporting/server/core.ts b/x-pack/plugins/reporting/server/core.ts index e7786b3b753fb..94b138ffcae0b 100644 --- a/x-pack/plugins/reporting/server/core.ts +++ b/x-pack/plugins/reporting/server/core.ts @@ -26,7 +26,6 @@ import { ESQueueInstance } from './lib/create_queue'; import { EnqueueJobFn } from './lib/enqueue_job'; export interface ReportingInternalSetup { - browserDriverFactory: HeadlessChromiumDriverFactory; elasticsearch: ElasticsearchServiceSetup; licensing: LicensingPluginSetup; basePath: BasePath['get']; @@ -44,6 +43,7 @@ interface ReportingInternalStart { export class ReportingCore { private pluginSetupDeps?: ReportingInternalSetup; private pluginStartDeps?: ReportingInternalStart; + private browserDriverFactory?: HeadlessChromiumDriverFactory; private readonly pluginSetup$ = new Rx.ReplaySubject(); private readonly pluginStart$ = new Rx.ReplaySubject(); private exportTypesRegistry = getExportTypesRegistry(); @@ -63,6 +63,10 @@ export class ReportingCore { return this.pluginStart$.pipe(first(), mapTo(true)).toPromise(); } + public setBrowserDriverFactory(browserDriverFactory: HeadlessChromiumDriverFactory) { + this.browserDriverFactory = browserDriverFactory; + } + /* * Internal module dependencies */ @@ -93,7 +97,10 @@ export class ReportingCore { } public getScreenshotsObservable(): ScreenshotsObservableFn { - const { browserDriverFactory } = this.getPluginSetupDeps(); + const { browserDriverFactory } = this; + if (!browserDriverFactory) { + throw new Error(`"browserDriverFactory" dependency hasn't initialized yet`); + } return screenshotsObservableFactory(this.config.get('capture'), browserDriverFactory); } diff --git a/x-pack/plugins/reporting/server/lib/validate/index.ts b/x-pack/plugins/reporting/server/lib/validate/index.ts index 404cbcda31a09..7c439d6023d5f 100644 --- a/x-pack/plugins/reporting/server/lib/validate/index.ts +++ b/x-pack/plugins/reporting/server/lib/validate/index.ts @@ -7,8 +7,8 @@ import { i18n } from '@kbn/i18n'; import { ElasticsearchServiceSetup } from 'kibana/server'; import { ReportingConfig } from '../../'; -import { LevelLogger } from '../../lib'; import { HeadlessChromiumDriverFactory } from '../../browsers/chromium/driver_factory'; +import { LevelLogger } from '../../lib'; import { validateBrowser } from './validate_browser'; import { validateMaxContentLength } from './validate_max_content_length'; @@ -16,8 +16,9 @@ export async function runValidations( config: ReportingConfig, elasticsearch: ElasticsearchServiceSetup, browserFactory: HeadlessChromiumDriverFactory, - logger: LevelLogger + parentLogger: LevelLogger ) { + const logger = parentLogger.clone(['validations']); try { await Promise.all([ validateBrowser(browserFactory, logger), diff --git a/x-pack/plugins/reporting/server/plugin.ts b/x-pack/plugins/reporting/server/plugin.ts index d0d25f6d9e0ae..ed4939fcc74f3 100644 --- a/x-pack/plugins/reporting/server/plugin.ts +++ b/x-pack/plugins/reporting/server/plugin.ts @@ -4,13 +4,10 @@ * you may not use this file except in compliance with the Elastic License. */ -import { Observable } from 'rxjs'; -import { first } from 'rxjs/operators'; import { CoreSetup, CoreStart, Plugin, PluginInitializerContext } from 'src/core/server'; -import { ReportingCore } from './core'; -import { ReportingConfigType } from './config'; -import { createBrowserDriverFactory } from './browsers'; -import { buildConfig, createConfig$ } from './config'; +import { ReportingCore } from './'; +import { initializeBrowserDriverFactory } from './browsers'; +import { buildConfig, ReportingConfigType } from './config'; import { createQueueFactory, enqueueJobFactory, LevelLogger, runValidations } from './lib'; import { registerRoutes } from './routes'; import { setFieldFormats } from './services'; @@ -22,61 +19,69 @@ export class ReportingPlugin private readonly initializerContext: PluginInitializerContext; private logger: LevelLogger; private reportingCore?: ReportingCore; - private config$: Observable; constructor(context: PluginInitializerContext) { this.logger = new LevelLogger(context.logger.get()); this.initializerContext = context; - this.config$ = context.config.create(); } - public async setup(core: CoreSetup, plugins: ReportingSetupDeps) { + public setup(core: CoreSetup, plugins: ReportingSetupDeps) { const { elasticsearch, http } = core; const { licensing, security } = plugins; const { initializerContext: initContext } = this; const router = http.createRouter(); const basePath = http.basePath.get; - const coreConfig = await createConfig$(core, this.config$, this.logger) - .pipe(first()) - .toPromise(); // apply computed defaults to config - const reportingConfig = buildConfig(initContext, core, coreConfig); // combine kbnServer configs - this.reportingCore = new ReportingCore(reportingConfig); - - const browserDriverFactory = await createBrowserDriverFactory(reportingConfig, this.logger); - - this.reportingCore.pluginSetup({ - browserDriverFactory, - elasticsearch, - licensing, - basePath, - router, - security, - }); + // async background setup + buildConfig(initContext, core, this.logger).then((config) => { + const reportingCore = new ReportingCore(config); + + reportingCore.pluginSetup({ + elasticsearch, + licensing, + basePath, + router, + security, + }); + + registerReportingUsageCollector(reportingCore, plugins); + registerRoutes(reportingCore, this.logger); + this.reportingCore = reportingCore; - runValidations(reportingConfig, elasticsearch, browserDriverFactory, this.logger); - registerReportingUsageCollector(this.reportingCore, plugins); - registerRoutes(this.reportingCore, this.logger); + this.logger.debug('Setup complete'); + }); return {}; } - public async start(core: CoreStart, plugins: ReportingStartDeps) { + public start(core: CoreStart, plugins: ReportingStartDeps) { + // use data plugin for csv formats + setFieldFormats(plugins.data.fieldFormats); + const { logger } = this; const reportingCore = this.getReportingCore(); + const config = reportingCore.getConfig(); + const { elasticsearch } = reportingCore.getPluginSetupDeps(); - const esqueue = await createQueueFactory(reportingCore, logger); - const enqueueJob = enqueueJobFactory(reportingCore, logger); + // async background start + initializeBrowserDriverFactory(config, logger).then(async (browserDriverFactory) => { + reportingCore.setBrowserDriverFactory(browserDriverFactory); - reportingCore.pluginStart({ - savedObjects: core.savedObjects, - uiSettings: core.uiSettings, - esqueue, - enqueueJob, - }); + const esqueue = await createQueueFactory(reportingCore, logger); + const enqueueJob = enqueueJobFactory(reportingCore, logger); - setFieldFormats(plugins.data.fieldFormats); - logger.info('reporting plugin started'); + reportingCore.pluginStart({ + savedObjects: core.savedObjects, + uiSettings: core.uiSettings, + esqueue, + enqueueJob, + }); + + // run self-check validations + runValidations(config, elasticsearch, browserDriverFactory, this.logger); + + this.logger.debug('Start complete'); + }); return {}; } diff --git a/x-pack/plugins/reporting/server/test_helpers/create_mock_browserdriverfactory.ts b/x-pack/plugins/reporting/server/test_helpers/create_mock_browserdriverfactory.ts index 5b0d740e031ab..0e8be7578e988 100644 --- a/x-pack/plugins/reporting/server/test_helpers/create_mock_browserdriverfactory.ts +++ b/x-pack/plugins/reporting/server/test_helpers/create_mock_browserdriverfactory.ts @@ -7,7 +7,7 @@ import { Page } from 'puppeteer'; import * as Rx from 'rxjs'; import { HeadlessChromiumDriver, HeadlessChromiumDriverFactory } from '../browsers'; -import { createDriverFactory } from '../browsers/chromium'; +import { chromium } from '../browsers'; import * as contexts from '../export_types/common/lib/screenshots/constants'; import { LevelLogger } from '../lib'; import { CaptureConfig, ElementsPositionAndAttribute } from '../types'; @@ -113,8 +113,12 @@ export const createMockBrowserDriverFactory = async ( maxAttempts: 1, }; - const binaryPath = '/usr/local/share/common/secure/'; - const mockBrowserDriverFactory = await createDriverFactory(binaryPath, logger, captureConfig); + const binaryPath = '/usr/local/share/common/secure/super_awesome_binary'; + const mockBrowserDriverFactory = await chromium.createDriverFactory( + binaryPath, + captureConfig, + logger + ); const mockPage = {} as Page; const mockBrowserDriver = new HeadlessChromiumDriver(mockPage, { inspect: true,