From e7d883441a9b36472650762f149cd461dcc817c3 Mon Sep 17 00:00:00 2001 From: Walter Rafelsberger Date: Fri, 26 Jun 2020 16:00:08 +0200 Subject: [PATCH 1/3] [ML] Fix license subscription race condition. --- x-pack/plugins/ml/public/application/app.tsx | 4 -- .../application/license/check_license.tsx | 8 ++- x-pack/plugins/ml/public/plugin.ts | 68 +++++++++++++------ 3 files changed, 52 insertions(+), 28 deletions(-) diff --git a/x-pack/plugins/ml/public/application/app.tsx b/x-pack/plugins/ml/public/application/app.tsx index 3df176ff25cb4..8b50d794d070d 100644 --- a/x-pack/plugins/ml/public/application/app.tsx +++ b/x-pack/plugins/ml/public/application/app.tsx @@ -13,7 +13,6 @@ import { Storage } from '../../../../../src/plugins/kibana_utils/public'; import { KibanaContextProvider } from '../../../../../src/plugins/kibana_react/public'; import { setDependencyCache, clearCache } from './util/dependency_cache'; -import { setLicenseCache } from './license'; import { MlSetupDependencies, MlStartDependencies } from '../plugin'; import { MlRouter } from './routing'; @@ -80,14 +79,11 @@ export const renderApp = ( deps.kibanaLegacy.loadFontAwesome(); - const mlLicense = setLicenseCache(deps.licensing); - appMountParams.onAppLeave((actions) => actions.default()); ReactDOM.render(, appMountParams.element); return () => { - mlLicense.unsubscribe(); clearCache(); ReactDOM.unmountComponentAtNode(appMountParams.element); }; diff --git a/x-pack/plugins/ml/public/application/license/check_license.tsx b/x-pack/plugins/ml/public/application/license/check_license.tsx index 3584ee8fbee4b..583eec7d75414 100644 --- a/x-pack/plugins/ml/public/application/license/check_license.tsx +++ b/x-pack/plugins/ml/public/application/license/check_license.tsx @@ -5,6 +5,7 @@ */ import { LicensingPluginSetup } from '../../../../licensing/public'; +import { MlLicense } from '../../../common/license'; import { MlClientLicense } from './ml_client_license'; let mlLicense: MlClientLicense | null = null; @@ -16,9 +17,12 @@ let mlLicense: MlClientLicense | null = null; * @param {LicensingPluginSetup} licensingSetup * @returns {MlClientLicense} */ -export function setLicenseCache(licensingSetup: LicensingPluginSetup) { +export function setLicenseCache( + licensingSetup: LicensingPluginSetup, + postInitFunctions?: Array<(lic: MlLicense) => void> +) { mlLicense = new MlClientLicense(); - mlLicense.setup(licensingSetup.license$); + mlLicense.setup(licensingSetup.license$, postInitFunctions); return mlLicense; } diff --git a/x-pack/plugins/ml/public/plugin.ts b/x-pack/plugins/ml/public/plugin.ts index 7f7544a44efa7..1659d8fa4f0f5 100644 --- a/x-pack/plugins/ml/public/plugin.ts +++ b/x-pack/plugins/ml/public/plugin.ts @@ -31,6 +31,7 @@ import { registerEmbeddables } from './embeddables'; import { UiActionsSetup } from '../../../../src/plugins/ui_actions/public'; import { registerMlUiActions } from './ui_actions'; import { KibanaLegacyStart } from '../../../../src/plugins/kibana_legacy/public'; +import { setLicenseCache } from './application/license'; export interface MlStartDependencies { data: DataPublicPluginStart; @@ -67,29 +68,52 @@ export class MlPlugin implements Plugin { const [coreStart, pluginsStart] = await core.getStartServices(); const kibanaVersion = this.initializerContext.env.packageInfo.version; const { renderApp } = await import('./application/app'); - return renderApp( - coreStart, - { - data: pluginsStart.data, - share: pluginsStart.share, - kibanaLegacy: pluginsStart.kibanaLegacy, - security: pluginsSetup.security, - licensing: pluginsSetup.licensing, - management: pluginsSetup.management, - usageCollection: pluginsSetup.usageCollection, - licenseManagement: pluginsSetup.licenseManagement, - home: pluginsSetup.home, - embeddable: pluginsSetup.embeddable, - uiActions: pluginsSetup.uiActions, - kibanaVersion, + + // Creates a mutable object containing + // a boilerplate callback function which + // we'll update with the real callback once + // the license listener mounts the app. + const returnRef = { callback: () => {} }; + + // We pass the code to mount the app as a `postInitFunctions` + // callback to setLicenseCache/MlLicense.setup() so we make + // sure the app get mounted only after we received the + // license information. + const mlLicense = setLicenseCache(pluginsSetup.licensing, [ + () => { + returnRef.callback = renderApp( + coreStart, + { + data: pluginsStart.data, + share: pluginsStart.share, + kibanaLegacy: pluginsStart.kibanaLegacy, + security: pluginsSetup.security, + licensing: pluginsSetup.licensing, + management: pluginsSetup.management, + usageCollection: pluginsSetup.usageCollection, + licenseManagement: pluginsSetup.licenseManagement, + home: pluginsSetup.home, + embeddable: pluginsSetup.embeddable, + uiActions: pluginsSetup.uiActions, + kibanaVersion, + }, + { + element: params.element, + appBasePath: params.appBasePath, + onAppLeave: params.onAppLeave, + history: params.history, + } + ); }, - { - element: params.element, - appBasePath: params.appBasePath, - onAppLeave: params.onAppLeave, - history: params.history, - } - ); + ]); + + // Finally we return the unmount callback which includes + // unsubscribing from the license listener as well as + // the overall unmount callback from renderApp(). + return () => { + mlLicense.unsubscribe(); + returnRef.callback(); + }; }, }); From dde9f5480e5b7252d3f295c94d792c0dd0ba3b9c Mon Sep 17 00:00:00 2001 From: Walter Rafelsberger Date: Fri, 26 Jun 2020 16:34:59 +0200 Subject: [PATCH 2/3] [ML] Adds unit test. --- .../license/ml_client_license.test.ts | 59 +++++++++++++++++++ x-pack/plugins/ml/public/plugin.ts | 2 +- 2 files changed, 60 insertions(+), 1 deletion(-) create mode 100644 x-pack/plugins/ml/public/application/license/ml_client_license.test.ts diff --git a/x-pack/plugins/ml/public/application/license/ml_client_license.test.ts b/x-pack/plugins/ml/public/application/license/ml_client_license.test.ts new file mode 100644 index 0000000000000..b37d7cfaa00aa --- /dev/null +++ b/x-pack/plugins/ml/public/application/license/ml_client_license.test.ts @@ -0,0 +1,59 @@ +/* + * 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 { Observable, Subject } from 'rxjs'; +import { ILicense } from '../../../../licensing/common/types'; + +import { MlClientLicense } from './ml_client_license'; + +describe('MlClientLicense', () => { + test('should miss the license update when initialized without postInitFunction', () => { + const mlLicense = new MlClientLicense(); + + // upon instantiation the full license doesn't get set + expect(mlLicense.isFullLicense()).toBe(false); + + const license$ = new Subject(); + + mlLicense.setup(license$ as Observable); + + // if the observable wasn't triggered the full license is still not set + expect(mlLicense.isFullLicense()).toBe(false); + + license$.next({ + check: () => ({ state: 'valid' }), + getFeature: () => ({ isEnabled: true }), + status: 'valid', + }); + + // once the observable triggered the license should be set + expect(mlLicense.isFullLicense()).toBe(true); + }); + + test('should not miss the license update when initialized with postInitFunction', (done) => { + const mlLicense = new MlClientLicense(); + + // upon instantiation the full license doesn't get set + expect(mlLicense.isFullLicense()).toBe(false); + + const license$ = new Subject(); + + mlLicense.setup(license$ as Observable, [ + (license) => { + // when passed in via postInitFunction callback, the license should be valid + // even if the license$ observable gets triggered after this setup. + expect(license.isFullLicense()).toBe(true); + done(); + }, + ]); + + license$.next({ + check: () => ({ state: 'valid' }), + getFeature: () => ({ isEnabled: true }), + status: 'valid', + }); + }); +}); diff --git a/x-pack/plugins/ml/public/plugin.ts b/x-pack/plugins/ml/public/plugin.ts index 1659d8fa4f0f5..a299efd48874b 100644 --- a/x-pack/plugins/ml/public/plugin.ts +++ b/x-pack/plugins/ml/public/plugin.ts @@ -77,7 +77,7 @@ export class MlPlugin implements Plugin { // We pass the code to mount the app as a `postInitFunctions` // callback to setLicenseCache/MlLicense.setup() so we make - // sure the app get mounted only after we received the + // sure the app gets mounted only after we received the // license information. const mlLicense = setLicenseCache(pluginsSetup.licensing, [ () => { From c91b10d9a42e6ca1b4c9c456228f94e7889e4f8c Mon Sep 17 00:00:00 2001 From: Walter Rafelsberger Date: Fri, 26 Jun 2020 17:06:14 +0200 Subject: [PATCH 3/3] [ML] Review feedback: Simplify code structure. --- x-pack/plugins/ml/public/application/app.tsx | 6 +- x-pack/plugins/ml/public/plugin.ts | 68 +++++++------------- 2 files changed, 27 insertions(+), 47 deletions(-) diff --git a/x-pack/plugins/ml/public/application/app.tsx b/x-pack/plugins/ml/public/application/app.tsx index 8b50d794d070d..9539d530bab04 100644 --- a/x-pack/plugins/ml/public/application/app.tsx +++ b/x-pack/plugins/ml/public/application/app.tsx @@ -13,6 +13,7 @@ import { Storage } from '../../../../../src/plugins/kibana_utils/public'; import { KibanaContextProvider } from '../../../../../src/plugins/kibana_react/public'; import { setDependencyCache, clearCache } from './util/dependency_cache'; +import { setLicenseCache } from './license'; import { MlSetupDependencies, MlStartDependencies } from '../plugin'; import { MlRouter } from './routing'; @@ -81,9 +82,12 @@ export const renderApp = ( appMountParams.onAppLeave((actions) => actions.default()); - ReactDOM.render(, appMountParams.element); + const mlLicense = setLicenseCache(deps.licensing, [ + () => ReactDOM.render(, appMountParams.element), + ]); return () => { + mlLicense.unsubscribe(); clearCache(); ReactDOM.unmountComponentAtNode(appMountParams.element); }; diff --git a/x-pack/plugins/ml/public/plugin.ts b/x-pack/plugins/ml/public/plugin.ts index a299efd48874b..7f7544a44efa7 100644 --- a/x-pack/plugins/ml/public/plugin.ts +++ b/x-pack/plugins/ml/public/plugin.ts @@ -31,7 +31,6 @@ import { registerEmbeddables } from './embeddables'; import { UiActionsSetup } from '../../../../src/plugins/ui_actions/public'; import { registerMlUiActions } from './ui_actions'; import { KibanaLegacyStart } from '../../../../src/plugins/kibana_legacy/public'; -import { setLicenseCache } from './application/license'; export interface MlStartDependencies { data: DataPublicPluginStart; @@ -68,52 +67,29 @@ export class MlPlugin implements Plugin { const [coreStart, pluginsStart] = await core.getStartServices(); const kibanaVersion = this.initializerContext.env.packageInfo.version; const { renderApp } = await import('./application/app'); - - // Creates a mutable object containing - // a boilerplate callback function which - // we'll update with the real callback once - // the license listener mounts the app. - const returnRef = { callback: () => {} }; - - // We pass the code to mount the app as a `postInitFunctions` - // callback to setLicenseCache/MlLicense.setup() so we make - // sure the app gets mounted only after we received the - // license information. - const mlLicense = setLicenseCache(pluginsSetup.licensing, [ - () => { - returnRef.callback = renderApp( - coreStart, - { - data: pluginsStart.data, - share: pluginsStart.share, - kibanaLegacy: pluginsStart.kibanaLegacy, - security: pluginsSetup.security, - licensing: pluginsSetup.licensing, - management: pluginsSetup.management, - usageCollection: pluginsSetup.usageCollection, - licenseManagement: pluginsSetup.licenseManagement, - home: pluginsSetup.home, - embeddable: pluginsSetup.embeddable, - uiActions: pluginsSetup.uiActions, - kibanaVersion, - }, - { - element: params.element, - appBasePath: params.appBasePath, - onAppLeave: params.onAppLeave, - history: params.history, - } - ); + return renderApp( + coreStart, + { + data: pluginsStart.data, + share: pluginsStart.share, + kibanaLegacy: pluginsStart.kibanaLegacy, + security: pluginsSetup.security, + licensing: pluginsSetup.licensing, + management: pluginsSetup.management, + usageCollection: pluginsSetup.usageCollection, + licenseManagement: pluginsSetup.licenseManagement, + home: pluginsSetup.home, + embeddable: pluginsSetup.embeddable, + uiActions: pluginsSetup.uiActions, + kibanaVersion, }, - ]); - - // Finally we return the unmount callback which includes - // unsubscribing from the license listener as well as - // the overall unmount callback from renderApp(). - return () => { - mlLicense.unsubscribe(); - returnRef.callback(); - }; + { + element: params.element, + appBasePath: params.appBasePath, + onAppLeave: params.onAppLeave, + history: params.history, + } + ); }, });