From f017eaacd97a7a575bc9b0d34d9e919caf152b32 Mon Sep 17 00:00:00 2001 From: Manasvini B Suryanarayana Date: Wed, 31 May 2023 17:16:06 -0700 Subject: [PATCH] Adds plugin manifest config to define OpenSearch plugin dependency and verifies if it is installed (#3116) Resolves Issue -https://github.com/opensearch-project/OpenSearch-Dashboards/issues/2799 Signed-off-by: Manasvini B Suryanarayana --- CHANGELOG.md | 1 + .../public/plugins/plugins_service.test.ts | 1 + .../discovery/plugin_manifest_parser.test.ts | 79 +++++++++++++++++++ .../discovery/plugin_manifest_parser.ts | 26 ++++++ .../integration_tests/plugins_service.test.ts | 3 + src/core/server/plugins/plugin.test.ts | 1 + src/core/server/plugins/plugin.ts | 2 + .../server/plugins/plugin_context.test.ts | 1 + .../server/plugins/plugins_service.test.ts | 3 + .../server/plugins/plugins_system.test.ts | 77 +++++++++++++++++- src/core/server/plugins/plugins_system.ts | 34 +++++++- src/core/server/plugins/types.ts | 6 ++ 12 files changed, 231 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b87dbfe7cfd..f67a110289a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -85,6 +85,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) - Add satisfaction survey link to help menu ([#3676] (https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3676)) - [Vis Builder] Add persistence to visualizations inner state ([#3751](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3751)) - [Table Visualization] Move format table, consolidate types and add unit tests ([#3397](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3397)) +- Add plugin manifest config to define OpenSearch plugin dependency and verify if it is installed on the cluster ([#3116](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3116)) - [Multiple Datasource] Support Amazon OpenSearch Serverless ([#3957](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3957)) - Add support for Node.js >=14.20.1 <19 ([#4071](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4071)) - Bundle Node.js 14 as a fallback for operating systems that cannot run Node.js 18 ([#4151](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4151)) diff --git a/src/core/public/plugins/plugins_service.test.ts b/src/core/public/plugins/plugins_service.test.ts index 5ca7c491f72..5fa3bf888b5 100644 --- a/src/core/public/plugins/plugins_service.test.ts +++ b/src/core/public/plugins/plugins_service.test.ts @@ -84,6 +84,7 @@ function createManifest( version: 'some-version', configPath: ['path'], requiredPlugins: required, + requiredOpenSearchPlugins: optional, optionalPlugins: optional, requiredBundles: [], }; diff --git a/src/core/server/plugins/discovery/plugin_manifest_parser.test.ts b/src/core/server/plugins/discovery/plugin_manifest_parser.test.ts index 18b715d9505..66fd1336a55 100644 --- a/src/core/server/plugins/discovery/plugin_manifest_parser.test.ts +++ b/src/core/server/plugins/discovery/plugin_manifest_parser.test.ts @@ -247,6 +247,79 @@ test('return error when manifest contains unrecognized properties', async () => }); }); +describe('requiredOpenSearchPlugins', () => { + test('return error when plugin `requiredOpenSearchPlugins` is a string and not an array of string', async () => { + mockReadFilePromise.mockResolvedValue( + Buffer.from( + JSON.stringify({ + id: 'id1', + version: '7.0.0', + server: true, + requiredOpenSearchPlugins: 'abc', + }) + ) + ); + + await expect(parseManifest(pluginPath, packageInfo, logger)).rejects.toMatchObject({ + message: `The "requiredOpenSearchPlugins" in plugin manifest for "id1" should be an array of strings. (invalid-manifest, ${pluginManifestPath})`, + type: PluginDiscoveryErrorType.InvalidManifest, + path: pluginManifestPath, + }); + }); + + test('return error when `requiredOpenSearchPlugins` is not a string', async () => { + mockReadFilePromise.mockResolvedValue( + Buffer.from(JSON.stringify({ id: 'id2', version: '7.0.0', requiredOpenSearchPlugins: 2 })) + ); + + await expect(parseManifest(pluginPath, packageInfo, logger)).rejects.toMatchObject({ + message: `The "requiredOpenSearchPlugins" in plugin manifest for "id2" should be an array of strings. (invalid-manifest, ${pluginManifestPath})`, + type: PluginDiscoveryErrorType.InvalidManifest, + path: pluginManifestPath, + }); + }); + + test('return error when plugin requiredOpenSearchPlugins is an array that contains non-string values', async () => { + mockReadFilePromise.mockResolvedValue( + Buffer.from( + JSON.stringify({ id: 'id3', version: '7.0.0', requiredOpenSearchPlugins: ['plugin1', 2] }) + ) + ); + + await expect(parseManifest(pluginPath, packageInfo, logger)).rejects.toMatchObject({ + message: `The "requiredOpenSearchPlugins" in plugin manifest for "id3" should be an array of strings. (invalid-manifest, ${pluginManifestPath})`, + type: PluginDiscoveryErrorType.InvalidManifest, + path: pluginManifestPath, + }); + }); + + test('Happy path when plugin `requiredOpenSearchPlugins` is an array of string', async () => { + mockReadFilePromise.mockResolvedValue( + Buffer.from( + JSON.stringify({ + id: 'id1', + version: '7.0.0', + server: true, + requiredOpenSearchPlugins: ['plugin1', 'plugin2'], + }) + ) + ); + + await expect(parseManifest(pluginPath, packageInfo, logger)).resolves.toEqual({ + id: 'id1', + configPath: 'id_1', + version: '7.0.0', + opensearchDashboardsVersion: '7.0.0', + optionalPlugins: [], + requiredPlugins: [], + requiredOpenSearchPlugins: ['plugin1', 'plugin2'], + requiredBundles: [], + server: true, + ui: false, + }); + }); +}); + describe('configPath', () => { test('falls back to plugin id if not specified', async () => { mockReadFilePromise.mockResolvedValue( @@ -301,6 +374,7 @@ test('set defaults for all missing optional fields', async () => { opensearchDashboardsVersion: '7.0.0', optionalPlugins: [], requiredPlugins: [], + requiredOpenSearchPlugins: [], requiredBundles: [], server: true, ui: false, @@ -317,6 +391,7 @@ test('return all set optional fields as they are in manifest', async () => { opensearchDashboardsVersion: '7.0.0', requiredPlugins: ['some-required-plugin', 'some-required-plugin-2'], optionalPlugins: ['some-optional-plugin'], + requiredOpenSearchPlugins: ['test-opensearch-plugin-1', 'test-opensearch-plugin-2'], ui: true, }) ) @@ -330,6 +405,7 @@ test('return all set optional fields as they are in manifest', async () => { optionalPlugins: ['some-optional-plugin'], requiredBundles: [], requiredPlugins: ['some-required-plugin', 'some-required-plugin-2'], + requiredOpenSearchPlugins: ['test-opensearch-plugin-1', 'test-opensearch-plugin-2'], server: false, ui: true, }); @@ -344,6 +420,7 @@ test('return manifest when plugin expected OpenSearch Dashboards version matches version: 'some-version', opensearchDashboardsVersion: '7.0.0-alpha2', requiredPlugins: ['some-required-plugin'], + requiredOpenSearchPlugins: [], server: true, }) ) @@ -356,6 +433,7 @@ test('return manifest when plugin expected OpenSearch Dashboards version matches opensearchDashboardsVersion: '7.0.0-alpha2', optionalPlugins: [], requiredPlugins: ['some-required-plugin'], + requiredOpenSearchPlugins: [], requiredBundles: [], server: true, ui: false, @@ -383,6 +461,7 @@ test('return manifest when plugin expected OpenSearch Dashboards version is `ope opensearchDashboardsVersion: 'opensearchDashboards', optionalPlugins: [], requiredPlugins: ['some-required-plugin'], + requiredOpenSearchPlugins: [], requiredBundles: [], server: true, ui: true, diff --git a/src/core/server/plugins/discovery/plugin_manifest_parser.ts b/src/core/server/plugins/discovery/plugin_manifest_parser.ts index 2a5ccf611f0..ee435d2e310 100644 --- a/src/core/server/plugins/discovery/plugin_manifest_parser.ts +++ b/src/core/server/plugins/discovery/plugin_manifest_parser.ts @@ -61,6 +61,7 @@ const KNOWN_MANIFEST_FIELDS = (() => { version: true, configPath: true, requiredPlugins: true, + requiredOpenSearchPlugins: true, optionalPlugins: true, ui: true, server: true, @@ -156,6 +157,28 @@ export async function parseManifest( ); } + if ( + manifest.requiredOpenSearchPlugins !== undefined && + (!Array.isArray(manifest.requiredOpenSearchPlugins) || + !manifest.requiredOpenSearchPlugins.every((plugin) => typeof plugin === 'string')) + ) { + throw PluginDiscoveryError.invalidManifest( + manifestPath, + new Error( + `The "requiredOpenSearchPlugins" in plugin manifest for "${manifest.id}" should be an array of strings.` + ) + ); + } + + if ( + Array.isArray(manifest.requiredOpenSearchPlugins) && + manifest.requiredOpenSearchPlugins.length > 0 + ) { + log.info( + `Plugin ${manifest.id} has a dependency on following OpenSearch plugin(s): "${manifest.requiredOpenSearchPlugins}".` + ); + } + const expectedOpenSearchDashboardsVersion = typeof manifest.opensearchDashboardsVersion === 'string' && manifest.opensearchDashboardsVersion ? manifest.opensearchDashboardsVersion @@ -198,6 +221,9 @@ export async function parseManifest( opensearchDashboardsVersion: expectedOpenSearchDashboardsVersion, configPath: manifest.configPath || snakeCase(manifest.id), requiredPlugins: Array.isArray(manifest.requiredPlugins) ? manifest.requiredPlugins : [], + requiredOpenSearchPlugins: Array.isArray(manifest.requiredOpenSearchPlugins) + ? manifest.requiredOpenSearchPlugins + : [], optionalPlugins: Array.isArray(manifest.optionalPlugins) ? manifest.optionalPlugins : [], requiredBundles: Array.isArray(manifest.requiredBundles) ? manifest.requiredBundles : [], ui: includesUiPlugin, diff --git a/src/core/server/plugins/integration_tests/plugins_service.test.ts b/src/core/server/plugins/integration_tests/plugins_service.test.ts index 34310ea3de3..4d1660f65c7 100644 --- a/src/core/server/plugins/integration_tests/plugins_service.test.ts +++ b/src/core/server/plugins/integration_tests/plugins_service.test.ts @@ -57,6 +57,7 @@ describe('PluginsService', () => { disabled = false, version = 'some-version', requiredPlugins = [], + requiredOpenSearchPlugins = [], requiredBundles = [], optionalPlugins = [], opensearchDashboardsVersion = '7.0.0', @@ -68,6 +69,7 @@ describe('PluginsService', () => { disabled?: boolean; version?: string; requiredPlugins?: string[]; + requiredOpenSearchPlugins?: string[]; requiredBundles?: string[]; optionalPlugins?: string[]; opensearchDashboardsVersion?: string; @@ -84,6 +86,7 @@ describe('PluginsService', () => { configPath: `${configPath}${disabled ? '-disabled' : ''}`, opensearchDashboardsVersion, requiredPlugins, + requiredOpenSearchPlugins, requiredBundles, optionalPlugins, server, diff --git a/src/core/server/plugins/plugin.test.ts b/src/core/server/plugins/plugin.test.ts index 9543d379493..04df84ab8d1 100644 --- a/src/core/server/plugins/plugin.test.ts +++ b/src/core/server/plugins/plugin.test.ts @@ -69,6 +69,7 @@ function createPluginManifest(manifestProps: Partial = {}): Plug configPath: 'path', opensearchDashboardsVersion: '7.0.0', requiredPlugins: ['some-required-dep'], + requiredOpenSearchPlugins: ['some-os-plugins'], optionalPlugins: ['some-optional-dep'], requiredBundles: [], server: true, diff --git a/src/core/server/plugins/plugin.ts b/src/core/server/plugins/plugin.ts index 89a2aecc82f..cc53e1b443e 100644 --- a/src/core/server/plugins/plugin.ts +++ b/src/core/server/plugins/plugin.ts @@ -66,6 +66,7 @@ export class PluginWrapper< public readonly configPath: PluginManifest['configPath']; public readonly requiredPlugins: PluginManifest['requiredPlugins']; public readonly optionalPlugins: PluginManifest['optionalPlugins']; + public readonly requiredOpenSearchPlugins: PluginManifest['requiredOpenSearchPlugins']; public readonly requiredBundles: PluginManifest['requiredBundles']; public readonly includesServerPlugin: PluginManifest['server']; public readonly includesUiPlugin: PluginManifest['ui']; @@ -95,6 +96,7 @@ export class PluginWrapper< this.configPath = params.manifest.configPath; this.requiredPlugins = params.manifest.requiredPlugins; this.optionalPlugins = params.manifest.optionalPlugins; + this.requiredOpenSearchPlugins = params.manifest.requiredOpenSearchPlugins; this.requiredBundles = params.manifest.requiredBundles; this.includesServerPlugin = params.manifest.server; this.includesUiPlugin = params.manifest.ui; diff --git a/src/core/server/plugins/plugin_context.test.ts b/src/core/server/plugins/plugin_context.test.ts index c55603679c3..ca46a97a237 100644 --- a/src/core/server/plugins/plugin_context.test.ts +++ b/src/core/server/plugins/plugin_context.test.ts @@ -56,6 +56,7 @@ function createPluginManifest(manifestProps: Partial = {}): Plug configPath: 'path', opensearchDashboardsVersion: '7.0.0', requiredPlugins: ['some-required-dep'], + requiredOpenSearchPlugins: ['some-backend-plugin'], requiredBundles: [], optionalPlugins: ['some-optional-dep'], server: true, diff --git a/src/core/server/plugins/plugins_service.test.ts b/src/core/server/plugins/plugins_service.test.ts index b020e56539a..c3ee05d60ed 100644 --- a/src/core/server/plugins/plugins_service.test.ts +++ b/src/core/server/plugins/plugins_service.test.ts @@ -78,6 +78,7 @@ const createPlugin = ( disabled = false, version = 'some-version', requiredPlugins = [], + requiredOpenSearchPlugins = [], requiredBundles = [], optionalPlugins = [], opensearchDashboardsVersion = '7.0.0', @@ -89,6 +90,7 @@ const createPlugin = ( disabled?: boolean; version?: string; requiredPlugins?: string[]; + requiredOpenSearchPlugins?: string[]; requiredBundles?: string[]; optionalPlugins?: string[]; opensearchDashboardsVersion?: string; @@ -105,6 +107,7 @@ const createPlugin = ( configPath: `${configPath}${disabled ? '-disabled' : ''}`, opensearchDashboardsVersion, requiredPlugins, + requiredOpenSearchPlugins, requiredBundles, optionalPlugins, server, diff --git a/src/core/server/plugins/plugins_system.test.ts b/src/core/server/plugins/plugins_system.test.ts index 2e8aa6b4a4c..286bb0d9784 100644 --- a/src/core/server/plugins/plugins_system.test.ts +++ b/src/core/server/plugins/plugins_system.test.ts @@ -53,9 +53,16 @@ function createPlugin( { required = [], optional = [], + requiredOSPlugin = [], server = true, ui = true, - }: { required?: string[]; optional?: string[]; server?: boolean; ui?: boolean } = {} + }: { + required?: string[]; + optional?: string[]; + requiredOSPlugin?: string[]; + server?: boolean; + ui?: boolean; + } = {} ) { return new PluginWrapper({ path: 'some-path', @@ -65,6 +72,7 @@ function createPlugin( configPath: 'path', opensearchDashboardsVersion: '7.0.0', requiredPlugins: required, + requiredOpenSearchPlugins: requiredOSPlugin, optionalPlugins: optional, requiredBundles: [], server, @@ -187,7 +195,7 @@ test('correctly orders plugins and returns exposed values for "setup" and "start } const plugins = new Map([ [ - createPlugin('order-4', { required: ['order-2'] }), + createPlugin('order-4', { required: ['order-2'], requiredOSPlugin: ['test-plugin'] }), { setup: { 'order-2': 'added-as-2' }, start: { 'order-2': 'started-as-2' }, @@ -244,6 +252,17 @@ test('correctly orders plugins and returns exposed values for "setup" and "start startContextMap.get(plugin.name) ); + const opensearch = startDeps.opensearch; + opensearch.client.asInternalUser.cat.plugins.mockResolvedValue({ + body: [ + { + name: 'node-1', + component: 'test-plugin', + version: 'v1', + }, + ], + } as any); + expect([...(await pluginsSystem.setupPlugins(setupDeps))]).toMatchInlineSnapshot(` Array [ Array [ @@ -484,6 +503,16 @@ describe('start', () => { afterAll(() => { jest.useRealTimers(); }); + const opensearch = startDeps.opensearch; + opensearch.client.asInternalUser.cat.plugins.mockResolvedValue({ + body: [ + { + name: 'node-1', + component: 'test-plugin', + version: 'v1', + }, + ], + } as any); it('throws timeout error if "start" was not completed in 30 sec.', async () => { const plugin: PluginWrapper = createPlugin('timeout-start'); jest.spyOn(plugin, 'setup').mockResolvedValue({}); @@ -517,4 +546,48 @@ describe('start', () => { const log = logger.get.mock.results[0].value as jest.Mocked; expect(log.info).toHaveBeenCalledWith(`Starting [2] plugins: [order-1,order-0]`); }); + + it('validates opensearch plugin installation when dependency is fulfilled', async () => { + [ + createPlugin('order-1', { requiredOSPlugin: ['test-plugin'] }), + createPlugin('order-2'), + ].forEach((plugin, index) => { + jest.spyOn(plugin, 'setup').mockResolvedValue(`setup-as-${index}`); + jest.spyOn(plugin, 'start').mockResolvedValue(`started-as-${index}`); + pluginsSystem.addPlugin(plugin); + }); + + await pluginsSystem.setupPlugins(setupDeps); + const pluginsStart = await pluginsSystem.startPlugins(startDeps); + expect(pluginsStart).toBeInstanceOf(Map); + expect(opensearch.client.asInternalUser.cat.plugins).toHaveBeenCalledTimes(1); + }); + + it('validates opensearch plugin installation and does not error out when plugin is not installed', async () => { + [ + createPlugin('id-1', { requiredOSPlugin: ['missing-opensearch-dep'] }), + createPlugin('id-2'), + ].forEach((plugin, index) => { + jest.spyOn(plugin, 'setup').mockResolvedValue(`setup-as-${index}`); + jest.spyOn(plugin, 'start').mockResolvedValue(`started-as-${index}`); + pluginsSystem.addPlugin(plugin); + }); + + await pluginsSystem.setupPlugins(setupDeps); + const pluginsStart = await pluginsSystem.startPlugins(startDeps); + expect(pluginsStart).toBeInstanceOf(Map); + expect(opensearch.client.asInternalUser.cat.plugins).toHaveBeenCalledTimes(1); + }); + + it('validates opensearch plugin installation and does not error out when there is no dependency', async () => { + [createPlugin('id-1'), createPlugin('id-2')].forEach((plugin, index) => { + jest.spyOn(plugin, 'setup').mockResolvedValue(`setup-as-${index}`); + jest.spyOn(plugin, 'start').mockResolvedValue(`started-as-${index}`); + pluginsSystem.addPlugin(plugin); + }); + await pluginsSystem.setupPlugins(setupDeps); + const pluginsStart = await pluginsSystem.startPlugins(startDeps); + expect(pluginsStart).toBeInstanceOf(Map); + expect(opensearch.client.asInternalUser.cat.plugins).toHaveBeenCalledTimes(1); + }); }); diff --git a/src/core/server/plugins/plugins_system.ts b/src/core/server/plugins/plugins_system.ts index b3adc71323f..6445f38f8e7 100644 --- a/src/core/server/plugins/plugins_system.ts +++ b/src/core/server/plugins/plugins_system.ts @@ -158,13 +158,45 @@ export class PluginsSystem { timeout: 30 * Sec, errorMessage: `Start lifecycle of "${pluginName}" plugin wasn't completed in 30sec. Consider disabling the plugin and re-start.`, }); - contracts.set(pluginName, contract); } + await this.healthCheckOpenSearchPlugins(deps); return contracts; } + private async healthCheckOpenSearchPlugins(deps: PluginsServiceStartDeps) { + // make _cat/plugins?format=json call to the OpenSearch instance + const opensearchPlugins = await this.getOpenSearchPlugins(deps); + for (const pluginName of this.satupPlugins) { + this.log.debug(`For plugin "${pluginName}"...`); + const plugin = this.plugins.get(pluginName)!; + const pluginBackendDeps = new Set([...plugin.requiredOpenSearchPlugins]); + pluginBackendDeps.forEach((opensearchPlugin) => { + if (!opensearchPlugins.find((obj) => obj.component === opensearchPlugin)) { + this.log.warn( + `OpenSearch plugin "${opensearchPlugin}" is not installed on the cluster for the OpenSearch Dashboards plugin to function as expected.` + ); + } + }); + } + } + + private async getOpenSearchPlugins(deps: PluginsServiceStartDeps) { + // Makes cat.plugin api call to fetch list of OpenSearch plugins installed on the cluster + try { + const { body } = await deps.opensearch.client.asInternalUser.cat.plugins({ + format: 'JSON', + }); + return body; + } catch (error) { + this.log.warn( + `Cat API call to OpenSearch to get list of plugins installed on the cluster has failed: ${error}` + ); + return []; + } + } + public async stopPlugins() { if (this.plugins.size === 0 || this.satupPlugins.length === 0) { return; diff --git a/src/core/server/plugins/types.ts b/src/core/server/plugins/types.ts index cf769b45daa..12b28f48f23 100644 --- a/src/core/server/plugins/types.ts +++ b/src/core/server/plugins/types.ts @@ -154,6 +154,12 @@ export interface PluginManifest { */ readonly requiredPlugins: readonly PluginName[]; + /** + * An optional list of component names of the backend OpenSearch plugins that **must be** installed on the cluster + * for this plugin to function properly. + */ + readonly requiredOpenSearchPlugins: readonly PluginName[]; + /** * List of plugin ids that this plugin's UI code imports modules from that are * not in `requiredPlugins`.