From aa0bb27a0be8deec3916b9b198eb35f32426cb07 Mon Sep 17 00:00:00 2001 From: Manasvini B Suryanarayana Date: Fri, 16 Dec 2022 20:54:34 +0000 Subject: [PATCH] Adds plugin manifest config to define OpenSearch plugin dependency and verifies if it is installed 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 | 60 +++++++++++++ .../discovery/plugin_manifest_parser.ts | 23 +++++ .../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 | 90 ++++++++++++++++++- src/core/server/plugins/plugins_system.ts | 30 ++++++- src/core/server/plugins/types.ts | 6 ++ 12 files changed, 218 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 13cb0174d943..9ee186f01643 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -50,6 +50,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) - [Multi DataSource] Test the connection to an external data source when creating or updating ([#2973](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2973)) - [Doc] Add current plugin persistence implementation readme ([#3081](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3081)) - [Table Visualization] Refactor table visualization using React and DataGrid component ([#2863](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2863)) +- Adds plugin manifest config to define OpenSearch plugin dependency and verifies if it is installed on the cluster ([#3116](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3116)) ### 🐛 Bug Fixes diff --git a/src/core/public/plugins/plugins_service.test.ts b/src/core/public/plugins/plugins_service.test.ts index 5ca7c491f72f..5fa3bf888b5c 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 6103b2d748b6..7097f2b52b27 100644 --- a/src/core/server/plugins/discovery/plugin_manifest_parser.test.ts +++ b/src/core/server/plugins/discovery/plugin_manifest_parser.test.ts @@ -279,6 +279,60 @@ test('return error when manifest contains unrecognized properties', async () => }); }); +test('return error when plugin `requiredOpenSearchPlugins` is a string and not an array of string', async () => { + mockReadFile.mockImplementation((path, cb) => { + cb( + null, + 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 () => { + mockReadFile.mockImplementation((path, cb) => { + cb( + null, + 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 () => { + mockReadFile.mockImplementation((path, cb) => { + cb( + null, + 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, + }); +}); + describe('configPath', () => { test('falls back to plugin id if not specified', async () => { mockReadFile.mockImplementation((path, cb) => { @@ -339,6 +393,7 @@ test('set defaults for all missing optional fields', async () => { opensearchDashboardsVersion: '7.0.0', optionalPlugins: [], requiredPlugins: [], + requiredOpenSearchPlugins: [], requiredBundles: [], server: true, ui: false, @@ -357,6 +412,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, }) ) @@ -371,6 +427,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, }); @@ -387,6 +444,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, }) ) @@ -400,6 +458,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, @@ -430,6 +489,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 0f39f14b0ce5..4dc8036776fc 100644 --- a/src/core/server/plugins/discovery/plugin_manifest_parser.ts +++ b/src/core/server/plugins/discovery/plugin_manifest_parser.ts @@ -65,6 +65,7 @@ const KNOWN_MANIFEST_FIELDS = (() => { version: true, configPath: true, requiredPlugins: true, + requiredOpenSearchPlugins: true, optionalPlugins: true, ui: true, server: true, @@ -160,6 +161,25 @@ 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)) { + log.warn( + `Plugin ${manifest.id} has a dependency on OpenSearch plugin(s). All dependency plugins have to be installed on the cluster to be able to start OpenSearch Dashboard.` + ); + } + const expectedOpenSearchDashboardsVersion = typeof manifest.opensearchDashboardsVersion === 'string' && manifest.opensearchDashboardsVersion ? manifest.opensearchDashboardsVersion @@ -202,6 +222,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 34310ea3de37..4d1660f65c75 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 9543d379493c..04df84ab8d12 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 89a2aecc82f3..cc53e1b443e9 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 c55603679c35..ca46a97a237e 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 b020e56539a7..c3ee05d60ed8 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 2e8aa6b4a4c0..892f74cf5d0c 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.mockResolvedValueOnce({ + body: [ + { + name: 'node-1', + component: 'test-plugin', + version: 'v1', + }, + ], + } as any); + expect([...(await pluginsSystem.setupPlugins(setupDeps))]).toMatchInlineSnapshot(` Array [ Array [ @@ -517,4 +536,71 @@ 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', 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); + }); + + const opensearch = startDeps.opensearch; + opensearch.client.asInternalUser.cat.plugins.mockResolvedValueOnce({ + body: [ + { + name: 'node-1', + component: 'test-plugin', + version: 'v1', + }, + ], + } as any); + await pluginsSystem.setupPlugins(setupDeps); + await pluginsSystem.startPlugins(startDeps); + expect(opensearch.client.asInternalUser.cat.plugins).toHaveBeenCalledTimes(1); + }); + + it('validates opensearch plugin installation and errors 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); + }); + const opensearch = startDeps.opensearch; + opensearch.client.asInternalUser.cat.plugins.mockResolvedValueOnce({ + body: [ + { + name: 'node-1', + component: 'test-plugin1', + version: 'v1', + }, + ], + } as any); + await pluginsSystem.setupPlugins(setupDeps); + + await expect(pluginsSystem.startPlugins(startDeps)).rejects.toMatchInlineSnapshot( + `[Error: Plugin "id-1" has a dependency on OpenSearch data source plugin "missing-opensearch-dep" which needs to be installed.]` + ); + }); + + it('validates opensearch plugin installation and does not errors 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); + }); + const opensearch = startDeps.opensearch; + opensearch.client.asInternalUser.cat.plugins.mockResolvedValueOnce({ + body: [], + } as any); + 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 b3adc71323f9..7af87d7a7558 100644 --- a/src/core/server/plugins/plugins_system.ts +++ b/src/core/server/plugins/plugins_system.ts @@ -158,13 +158,41 @@ 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.error( + `OpenSearch plugin "${opensearchPlugin}" is not installed on the cluster for the OpenSearch Dashboard plugin to function as expected.` + ); + throw new Error( + `Plugin "${pluginName}" has a dependency on OpenSearch plugin "${opensearchPlugin}" which is not installed on the cluster.` + ); + } + }); + } + } + + private async getOpensearchPlugins(deps: PluginsServiceStartDeps) { + // Makes cat.plugin api call to fetch list of OpenSearch plugins installed on the cluster + const { body } = await deps.opensearch.client.asInternalUser.cat.plugins({ + format: 'JSON', + }); + return body; + } + 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 cf769b45daa3..e41ba540e908 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 data source 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`.