[Reporting] Convert plugin setup and start to synchronous (elastic#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 <[email protected]> Co-authored-by: Elastic Machine <[email protected]>
tsullivan · Jun 11, 2020 · 1e757cf · 1e757cf
1 parent b66e462
commit 1e757cf
Show file tree

Hide file tree

Showing 18 changed files with 324 additions and 163 deletions.
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
@@ -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
@@ -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<HeadlessChromiumDriverFactory> {
-  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
diff --git 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
@@ -4,20 +4,27 @@
  * 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 { installBrowser } from './install';
+import { ReportingConfig } from '..';
 
 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 +37,13 @@ export interface BrowserDownload {
     }>;
   };
 }
+
+export const initializeBrowserDriverFactory = async (
+  config: ReportingConfig,
+  logger: LevelLogger
+) => {
+  const { binaryPath$ } = installBrowser(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
@@ -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,60 @@ 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,
+export function installBrowser(
   browser: BrowserDownload,
-  installsPath: string
-): Promise<PathResponse> {
-  const pkg = browser.paths.packages.find((p: Package) => p.platforms.includes(process.platform));
+  config: ReportingConfig,
+  logger: LevelLogger
+): { binaryPath$: Rx.Subject<string> } {
+  const binaryPath$ = new Rx.Subject<string>();
+  const backgroundInstall = async () => {
+    const captureConfig = config.get('capture');
+    const { autoDownload, type: browserType } = captureConfig.browser;
+    if (autoDownload) {
+      await ensureBrowserDownloaded(browserType, logger);
+    }
+
+    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(() => '');
 
-  if (!pkg) {
-    throw new Error(`Unsupported platform: ${JSON.stringify(browser, null, 2)}`);
-  }
+      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.`
+        );
+      }
 
-  const binaryPath = path.join(installsPath, pkg.binaryRelativePath);
-  const binaryChecksum = await md5(binaryPath).catch(() => '');
+      throw error; // reject the promise with the original error
+    }
+
+    logger.debug(`Browser executable: ${binaryPath}`);
+
+    binaryPath$.next(binaryPath); // subscribers wait for download and extract to complete
+  };
 
-  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');
-  }
+  backgroundInstall();
 
-  logger.debug(`Browser installed at ${binaryPath}`);
   return {
-    binaryPath,
+    binaryPath$,
   };
 }
diff --git 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<ReportingConfigType> {
   kbnConfig: Config<KbnServerConfigType>;
 }
 
-export const buildConfig = (
+export const buildConfig = async (
   initContext: PluginInitializerContext<ReportingConfigType>,
   core: CoreSetup,
-  reportingConfig: ReportingConfigType
-): ReportingConfig => {
+  logger: LevelLogger
+): Promise<ReportingConfig> => {
+  const config$ = initContext.config.create<ReportingConfigType>();
   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.test.ts b/x-pack/plugins/reporting/server/config/create_config.test.ts
@@ -45,7 +45,11 @@ describe('Reporting server createConfig$', () => {
     mockInitContext = makeMockInitContext({
       kibanaServer: {},
     });
-    mockLogger = ({ warn: jest.fn(), debug: jest.fn() } as unknown) as LevelLogger;
+    mockLogger = ({
+      warn: jest.fn(),
+      debug: jest.fn(),
+      clone: jest.fn().mockImplementation(() => mockLogger),
+    } as unknown) as LevelLogger;
   });
 
   afterEach(() => {

diff --git 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<ReportingConfigType>,
-  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
@@ -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<ReportingConfigType> = {

diff --git 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<ReportingInternalSetup>();
   private readonly pluginStart$ = new Rx.ReplaySubject<ReportingInternalStart>();
   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
@@ -7,17 +7,18 @@
 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';
 
 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),