From 018808704b9a3b8df811e9239195a2af37d4f05b Mon Sep 17 00:00:00 2001 From: Manasvini B Suryanarayana Date: Thu, 24 Aug 2023 16:43:45 -0700 Subject: [PATCH] Allow plugin manifest config to define semver compatible OpenSearch plugin and verify if it is installed on the cluster (#4612) Signed-off-by: Manasvini B Suryanarayana --- CHANGELOG.md | 1 + .../public/plugins/plugins_service.test.ts | 2 +- .../discovery/plugin_manifest_parser.test.ts | 90 ++++++++++++++----- .../discovery/plugin_manifest_parser.ts | 71 ++++++++++----- .../integration_tests/plugins_service.test.ts | 8 +- src/core/server/plugins/plugin.test.ts | 5 +- src/core/server/plugins/plugin.ts | 4 +- .../server/plugins/plugin_context.test.ts | 4 +- .../server/plugins/plugins_service.test.ts | 8 +- .../server/plugins/plugins_system.test.ts | 79 ++++++++++++---- src/core/server/plugins/plugins_system.ts | 33 +++++-- src/core/server/plugins/types.ts | 7 +- 12 files changed, 231 insertions(+), 81 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c250aa2309a5..560f91aa1866 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) - [Discover] Update styles to compatible with OUI `next` theme ([#4644](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4644)) - Remove visualization editor sidebar background ([#4719](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4719)) - [Vis Colors] Remove customized colors from sample visualizations and dashboards ([#4741](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4741)) +- [Decouple] Allow plugin manifest config to define semver compatible OpenSearch plugin and verify if it is installed on the cluster([#4612](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4612)) ### 🐛 Bug Fixes diff --git a/src/core/public/plugins/plugins_service.test.ts b/src/core/public/plugins/plugins_service.test.ts index 5fa3bf888b5c..b2cf4e8880cf 100644 --- a/src/core/public/plugins/plugins_service.test.ts +++ b/src/core/public/plugins/plugins_service.test.ts @@ -84,7 +84,7 @@ function createManifest( version: 'some-version', configPath: ['path'], requiredPlugins: required, - requiredOpenSearchPlugins: optional, + requiredEnginePlugins: 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 66fd1336a554..5ef01f88f3f7 100644 --- a/src/core/server/plugins/discovery/plugin_manifest_parser.test.ts +++ b/src/core/server/plugins/discovery/plugin_manifest_parser.test.ts @@ -247,72 +247,110 @@ 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 () => { +describe('requiredEnginePlugins', () => { + test('return error when plugin `requiredEnginePlugins` is a string and not an object', async () => { mockReadFilePromise.mockResolvedValue( Buffer.from( JSON.stringify({ - id: 'id1', + id: 'invalid-manifest-plugin', version: '7.0.0', server: true, - requiredOpenSearchPlugins: 'abc', + requiredEnginePlugins: '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})`, + message: `The "requiredEnginePlugins" in plugin manifest for "invalid-manifest-plugin" should be an object that maps a plugin name to a version range. (invalid-manifest, ${pluginManifestPath})`, type: PluginDiscoveryErrorType.InvalidManifest, path: pluginManifestPath, }); }); - test('return error when `requiredOpenSearchPlugins` is not a string', async () => { + test('return error when plugin `requiredEnginePlugins` is an array and not an object', async () => { mockReadFilePromise.mockResolvedValue( - Buffer.from(JSON.stringify({ id: 'id2', version: '7.0.0', requiredOpenSearchPlugins: 2 })) + Buffer.from( + JSON.stringify({ + id: 'invalid-manifest-plugin', + version: '7.0.0', + server: true, + requiredEnginePlugins: [{ 'plugin-1': '1.0.0.0', 'plugin-2': '^2.0.1' }], + }) + ) ); 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})`, + message: `The "requiredEnginePlugins" in plugin manifest for "invalid-manifest-plugin" should be an object that maps a plugin name to a version range. (invalid-manifest, ${pluginManifestPath})`, type: PluginDiscoveryErrorType.InvalidManifest, path: pluginManifestPath, }); }); - test('return error when plugin requiredOpenSearchPlugins is an array that contains non-string values', async () => { + test('return error when plugin requiredEnginePlugins is an object but contains non-string version value', async () => { mockReadFilePromise.mockResolvedValue( Buffer.from( - JSON.stringify({ id: 'id3', version: '7.0.0', requiredOpenSearchPlugins: ['plugin1', 2] }) + JSON.stringify({ + id: 'test-invalid-version-type-plugin', + version: '7.0.0', + requiredEnginePlugins: { 'invalid-plugin-version-range': 2 }, + }) + ) + ); + + await expect(parseManifest(pluginPath, packageInfo, logger)).rejects.toMatchObject({ + message: `The "requiredEnginePlugins" in plugin manifest for "test-invalid-version-type-plugin" should be an object that maps a plugin name to a version range. (invalid-manifest, ${pluginManifestPath})`, + type: PluginDiscoveryErrorType.InvalidManifest, + path: pluginManifestPath, + }); + }); + + test('return error when plugin requiredEnginePlugins contains invalid version range', async () => { + mockReadFilePromise.mockResolvedValue( + Buffer.from( + JSON.stringify({ + id: 'test-invalid-version-range-plugin', + version: '7.0.0', + requiredEnginePlugins: { + 'invalid-version-range-plugin': '?1.0.0', + 'valid-version-range-plugin': '^1.0.0', + }, + }) ) ); 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})`, + message: `The "requiredEnginePlugins" in the plugin manifest for "test-invalid-version-range-plugin" contains invalid version ranges: ?1.0.0 for invalid-version-range-plugin (invalid-manifest, ${pluginManifestPath})`, type: PluginDiscoveryErrorType.InvalidManifest, path: pluginManifestPath, }); }); - test('Happy path when plugin `requiredOpenSearchPlugins` is an array of string', async () => { + test('Happy path when plugin `requiredEnginePlugins` is a map of plugin name to its compatible version ranges', async () => { mockReadFilePromise.mockResolvedValue( Buffer.from( JSON.stringify({ - id: 'id1', + id: 'test-plugin-version-range-sanity', version: '7.0.0', server: true, - requiredOpenSearchPlugins: ['plugin1', 'plugin2'], + requiredEnginePlugins: { + 'valid-plugin-1': '^1.0.0', + 'valid-plugin-2': '1.0.0', + }, }) ) ); await expect(parseManifest(pluginPath, packageInfo, logger)).resolves.toEqual({ - id: 'id1', - configPath: 'id_1', + id: 'test-plugin-version-range-sanity', + configPath: 'test_plugin_version_range_sanity', version: '7.0.0', opensearchDashboardsVersion: '7.0.0', optionalPlugins: [], requiredPlugins: [], - requiredOpenSearchPlugins: ['plugin1', 'plugin2'], + requiredEnginePlugins: { + 'valid-plugin-1': '^1.0.0', + 'valid-plugin-2': '1.0.0', + }, requiredBundles: [], server: true, ui: false, @@ -374,7 +412,7 @@ test('set defaults for all missing optional fields', async () => { opensearchDashboardsVersion: '7.0.0', optionalPlugins: [], requiredPlugins: [], - requiredOpenSearchPlugins: [], + requiredEnginePlugins: {}, requiredBundles: [], server: true, ui: false, @@ -391,7 +429,10 @@ 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'], + requiredEnginePlugins: { + 'test-opensearch-plugin-1': '^1.0.0', + 'test-opensearch-plugin-2': '>=1.0.0', + }, ui: true, }) ) @@ -405,7 +446,10 @@ 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'], + requiredEnginePlugins: { + 'test-opensearch-plugin-1': '^1.0.0', + 'test-opensearch-plugin-2': '>=1.0.0', + }, server: false, ui: true, }); @@ -420,7 +464,7 @@ test('return manifest when plugin expected OpenSearch Dashboards version matches version: 'some-version', opensearchDashboardsVersion: '7.0.0-alpha2', requiredPlugins: ['some-required-plugin'], - requiredOpenSearchPlugins: [], + requiredEnginePlugins: {}, server: true, }) ) @@ -433,7 +477,7 @@ test('return manifest when plugin expected OpenSearch Dashboards version matches opensearchDashboardsVersion: '7.0.0-alpha2', optionalPlugins: [], requiredPlugins: ['some-required-plugin'], - requiredOpenSearchPlugins: [], + requiredEnginePlugins: {}, requiredBundles: [], server: true, ui: false, @@ -461,7 +505,7 @@ test('return manifest when plugin expected OpenSearch Dashboards version is `ope opensearchDashboardsVersion: 'opensearchDashboards', optionalPlugins: [], requiredPlugins: ['some-required-plugin'], - requiredOpenSearchPlugins: [], + requiredEnginePlugins: {}, 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 ee435d2e310a..cfc23f0dc197 100644 --- a/src/core/server/plugins/discovery/plugin_manifest_parser.ts +++ b/src/core/server/plugins/discovery/plugin_manifest_parser.ts @@ -29,6 +29,7 @@ */ import { readFile, stat } from 'fs/promises'; +import semver from 'semver'; import { resolve } from 'path'; import { coerce } from 'semver'; import { snakeCase } from 'lodash'; @@ -61,7 +62,7 @@ const KNOWN_MANIFEST_FIELDS = (() => { version: true, configPath: true, requiredPlugins: true, - requiredOpenSearchPlugins: true, + requiredEnginePlugins: true, optionalPlugins: true, ui: true, server: true, @@ -157,26 +158,42 @@ 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 ('requiredEnginePlugins' in manifest) { + if ( + typeof manifest.requiredEnginePlugins !== 'object' || + !Object.entries(manifest.requiredEnginePlugins).every( + ([pluginId, pluginVersion]) => + typeof pluginId === 'string' && typeof pluginVersion === 'string' ) - ); - } + ) { + throw PluginDiscoveryError.invalidManifest( + manifestPath, + new Error( + `The "requiredEnginePlugins" in plugin manifest for "${manifest.id}" should be an object that maps a plugin name to a version range.` + ) + ); + } + const invalidPluginVersions: string[] = []; + for (const [pluginName, versionRange] of Object.entries(manifest.requiredEnginePlugins)) { + log.info( + `Plugin ${manifest.id} has a dependency on engine plugin: [${pluginName}@${versionRange}]` + ); - if ( - Array.isArray(manifest.requiredOpenSearchPlugins) && - manifest.requiredOpenSearchPlugins.length > 0 - ) { - log.info( - `Plugin ${manifest.id} has a dependency on following OpenSearch plugin(s): "${manifest.requiredOpenSearchPlugins}".` - ); + if (!isOpenSearchPluginVersionRangeValid(versionRange)) { + invalidPluginVersions.push(`${versionRange} for ${pluginName}`); + } + } + + if (invalidPluginVersions.length > 0) { + throw PluginDiscoveryError.invalidManifest( + manifestPath, + new Error( + `The "requiredEnginePlugins" in the plugin manifest for "${ + manifest.id + }" contains invalid version ranges: ${invalidPluginVersions.join(', ')}` + ) + ); + } } const expectedOpenSearchDashboardsVersion = @@ -221,9 +238,8 @@ 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 - : [], + requiredEnginePlugins: + manifest.requiredEnginePlugins !== undefined ? manifest.requiredEnginePlugins : {}, optionalPlugins: Array.isArray(manifest.optionalPlugins) ? manifest.optionalPlugins : [], requiredBundles: Array.isArray(manifest.requiredBundles) ? manifest.requiredBundles : [], ui: includesUiPlugin, @@ -276,3 +292,14 @@ function isVersionCompatible( 0 ); } +/** + * Checks whether specified version range is valid. + * @param versionRange Version range to be checked. + */ +function isOpenSearchPluginVersionRangeValid(versionRange: string) { + try { + return semver.validRange(versionRange); + } catch (err) { + return false; + } +} 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 4d1660f65c75..ea400ddcd913 100644 --- a/src/core/server/plugins/integration_tests/plugins_service.test.ts +++ b/src/core/server/plugins/integration_tests/plugins_service.test.ts @@ -42,7 +42,7 @@ import { config } from '../plugins_config'; import { loggingSystemMock } from '../../logging/logging_system.mock'; import { environmentServiceMock } from '../../environment/environment_service.mock'; import { coreMock } from '../../mocks'; -import { Plugin } from '../types'; +import { Plugin, CompatibleEnginePluginVersions } from '../types'; import { PluginWrapper } from '../plugin'; describe('PluginsService', () => { @@ -57,7 +57,7 @@ describe('PluginsService', () => { disabled = false, version = 'some-version', requiredPlugins = [], - requiredOpenSearchPlugins = [], + requiredEnginePlugins = {}, requiredBundles = [], optionalPlugins = [], opensearchDashboardsVersion = '7.0.0', @@ -69,7 +69,7 @@ describe('PluginsService', () => { disabled?: boolean; version?: string; requiredPlugins?: string[]; - requiredOpenSearchPlugins?: string[]; + requiredEnginePlugins?: CompatibleEnginePluginVersions; requiredBundles?: string[]; optionalPlugins?: string[]; opensearchDashboardsVersion?: string; @@ -86,7 +86,7 @@ describe('PluginsService', () => { configPath: `${configPath}${disabled ? '-disabled' : ''}`, opensearchDashboardsVersion, requiredPlugins, - requiredOpenSearchPlugins, + requiredEnginePlugins, requiredBundles, optionalPlugins, server, diff --git a/src/core/server/plugins/plugin.test.ts b/src/core/server/plugins/plugin.test.ts index 04df84ab8d12..69b781031c7a 100644 --- a/src/core/server/plugins/plugin.test.ts +++ b/src/core/server/plugins/plugin.test.ts @@ -69,7 +69,10 @@ function createPluginManifest(manifestProps: Partial = {}): Plug configPath: 'path', opensearchDashboardsVersion: '7.0.0', requiredPlugins: ['some-required-dep'], - requiredOpenSearchPlugins: ['some-os-plugins'], + requiredEnginePlugins: { + 'test-os-plugin1': '^2.2.1', + 'test-os-plugin2': '2.2.1 || 2.2.2', + }, optionalPlugins: ['some-optional-dep'], requiredBundles: [], server: true, diff --git a/src/core/server/plugins/plugin.ts b/src/core/server/plugins/plugin.ts index cc53e1b443e9..0aa13e53d650 100644 --- a/src/core/server/plugins/plugin.ts +++ b/src/core/server/plugins/plugin.ts @@ -66,7 +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 requiredEnginePlugins: PluginManifest['requiredEnginePlugins']; public readonly requiredBundles: PluginManifest['requiredBundles']; public readonly includesServerPlugin: PluginManifest['server']; public readonly includesUiPlugin: PluginManifest['ui']; @@ -96,7 +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.requiredEnginePlugins = params.manifest.requiredEnginePlugins; 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 ca46a97a237e..48c9eb6d6823 100644 --- a/src/core/server/plugins/plugin_context.test.ts +++ b/src/core/server/plugins/plugin_context.test.ts @@ -56,7 +56,9 @@ function createPluginManifest(manifestProps: Partial = {}): Plug configPath: 'path', opensearchDashboardsVersion: '7.0.0', requiredPlugins: ['some-required-dep'], - requiredOpenSearchPlugins: ['some-backend-plugin'], + requiredEnginePlugins: { + 'test-os-plugin1': '^2.2.1', + }, 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 c3ee05d60ed8..36c594908845 100644 --- a/src/core/server/plugins/plugins_service.test.ts +++ b/src/core/server/plugins/plugins_service.test.ts @@ -47,7 +47,7 @@ import { PluginsService } from './plugins_service'; import { PluginsSystem } from './plugins_system'; import { config } from './plugins_config'; import { take } from 'rxjs/operators'; -import { DiscoveredPlugin } from './types'; +import { DiscoveredPlugin, CompatibleEnginePluginVersions } from './types'; const { join } = posix; const MockPluginsSystem: jest.Mock = PluginsSystem as any; @@ -78,7 +78,7 @@ const createPlugin = ( disabled = false, version = 'some-version', requiredPlugins = [], - requiredOpenSearchPlugins = [], + requiredEnginePlugins = {}, requiredBundles = [], optionalPlugins = [], opensearchDashboardsVersion = '7.0.0', @@ -90,7 +90,7 @@ const createPlugin = ( disabled?: boolean; version?: string; requiredPlugins?: string[]; - requiredOpenSearchPlugins?: string[]; + requiredEnginePlugins?: CompatibleEnginePluginVersions; requiredBundles?: string[]; optionalPlugins?: string[]; opensearchDashboardsVersion?: string; @@ -107,7 +107,7 @@ const createPlugin = ( configPath: `${configPath}${disabled ? '-disabled' : ''}`, opensearchDashboardsVersion, requiredPlugins, - requiredOpenSearchPlugins, + requiredEnginePlugins, requiredBundles, optionalPlugins, server, diff --git a/src/core/server/plugins/plugins_system.test.ts b/src/core/server/plugins/plugins_system.test.ts index 286bb0d97846..dba7388f83b3 100644 --- a/src/core/server/plugins/plugins_system.test.ts +++ b/src/core/server/plugins/plugins_system.test.ts @@ -42,7 +42,7 @@ import { CoreContext } from '../core_context'; import { loggingSystemMock } from '../logging/logging_system.mock'; import { PluginWrapper } from './plugin'; -import { PluginName } from './types'; +import { PluginName, CompatibleEnginePluginVersions } from './types'; import { PluginsSystem } from './plugins_system'; import { coreMock } from '../mocks'; import { Logger } from '../logging'; @@ -53,13 +53,13 @@ function createPlugin( { required = [], optional = [], - requiredOSPlugin = [], + requiredOSPlugin = {}, server = true, ui = true, }: { required?: string[]; optional?: string[]; - requiredOSPlugin?: string[]; + requiredOSPlugin?: CompatibleEnginePluginVersions; server?: boolean; ui?: boolean; } = {} @@ -72,7 +72,7 @@ function createPlugin( configPath: 'path', opensearchDashboardsVersion: '7.0.0', requiredPlugins: required, - requiredOpenSearchPlugins: requiredOSPlugin, + requiredEnginePlugins: requiredOSPlugin, optionalPlugins: optional, requiredBundles: [], server, @@ -195,7 +195,12 @@ test('correctly orders plugins and returns exposed values for "setup" and "start } const plugins = new Map([ [ - createPlugin('order-4', { required: ['order-2'], requiredOSPlugin: ['test-plugin'] }), + createPlugin('order-4', { + required: ['order-2'], + requiredOSPlugin: { + 'test-plugin-1': '^1.1.1', + }, + }), { setup: { 'order-2': 'added-as-2' }, start: { 'order-2': 'started-as-2' }, @@ -253,12 +258,12 @@ test('correctly orders plugins and returns exposed values for "setup" and "start ); const opensearch = startDeps.opensearch; - opensearch.client.asInternalUser.cat.plugins.mockResolvedValue({ + opensearch.client.asInternalUser.cat.plugins.mockResolvedValueOnce({ body: [ { name: 'node-1', - component: 'test-plugin', - version: 'v1', + component: 'test-plugin-1', + version: '1.9.9', }, ], } as any); @@ -509,7 +514,7 @@ describe('start', () => { { name: 'node-1', component: 'test-plugin', - version: 'v1', + version: '2.1.0', }, ], } as any); @@ -549,8 +554,12 @@ describe('start', () => { it('validates opensearch plugin installation when dependency is fulfilled', async () => { [ - createPlugin('order-1', { requiredOSPlugin: ['test-plugin'] }), - createPlugin('order-2'), + createPlugin('dependency-fulfilled-plugin', { + requiredOSPlugin: { + 'test-plugin': '^2.0.0', + }, + }), + createPlugin('no-dependency-plugin'), ].forEach((plugin, index) => { jest.spyOn(plugin, 'setup').mockResolvedValue(`setup-as-${index}`); jest.spyOn(plugin, 'start').mockResolvedValue(`started-as-${index}`); @@ -561,12 +570,18 @@ describe('start', () => { const pluginsStart = await pluginsSystem.startPlugins(startDeps); expect(pluginsStart).toBeInstanceOf(Map); expect(opensearch.client.asInternalUser.cat.plugins).toHaveBeenCalledTimes(1); + const log = logger.get.mock.results[0].value as jest.Mocked; + expect(log.warn).toHaveBeenCalledTimes(0); }); 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'), + createPlugin('dependency-missing-plugin', { + requiredOSPlugin: { + 'missing-opensearch-dep': '^2.0.0', + }, + }), + createPlugin('no-dependency-plugin'), ].forEach((plugin, index) => { jest.spyOn(plugin, 'setup').mockResolvedValue(`setup-as-${index}`); jest.spyOn(plugin, 'start').mockResolvedValue(`started-as-${index}`); @@ -577,17 +592,51 @@ describe('start', () => { const pluginsStart = await pluginsSystem.startPlugins(startDeps); expect(pluginsStart).toBeInstanceOf(Map); expect(opensearch.client.asInternalUser.cat.plugins).toHaveBeenCalledTimes(1); + const log = logger.get.mock.results[0].value as jest.Mocked; + expect(log.warn).toHaveBeenCalledTimes(1); + expect(log.warn).toHaveBeenCalledWith( + `OpenSearch plugin "missing-opensearch-dep" is not installed on the engine for the OpenSearch Dashboards plugin to function as expected.` + ); }); - 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) => { + it('validates opensearch plugin installation and log warning when plugin exist but version is incompatible', async () => { + [ + createPlugin('version-mismatch-plugin', { + requiredOSPlugin: { + 'test-plugin-version-mismatch': '^1.0.0', + }, + }), + createPlugin('no-dependency-plugin'), + ].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); + const log = logger.get.mock.results[0].value as jest.Mocked; + expect(log.warn).toHaveBeenCalledTimes(1); + expect(log.warn).toHaveBeenCalledWith( + `OpenSearch plugin "test-plugin-version-mismatch" is not installed on the engine for the OpenSearch Dashboards plugin to function as expected.` + ); + }); + + it('validates opensearch plugin installation and does not warn when there is no dependency', async () => { + [createPlugin('no-dependency-plugin-1'), createPlugin('no-dependency-plugin-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); + const log = logger.get.mock.results[0].value as jest.Mocked; + expect(log.warn).toHaveBeenCalledTimes(0); }); }); diff --git a/src/core/server/plugins/plugins_system.ts b/src/core/server/plugins/plugins_system.ts index 6445f38f8e70..07aa88e9db31 100644 --- a/src/core/server/plugins/plugins_system.ts +++ b/src/core/server/plugins/plugins_system.ts @@ -29,6 +29,8 @@ */ import { withTimeout } from '@osd/std'; +import semver from 'semver'; +import { CatPluginsResponse } from '@opensearch-project/opensearch/api/types'; import { CoreContext } from '../core_context'; import { Logger } from '../logging'; import { PluginWrapper } from './plugin'; @@ -167,21 +169,40 @@ export class PluginsSystem { private async healthCheckOpenSearchPlugins(deps: PluginsServiceStartDeps) { // make _cat/plugins?format=json call to the OpenSearch instance - const opensearchPlugins = await this.getOpenSearchPlugins(deps); + const opensearchInstalledPlugins = 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)) { + const pluginOpenSearchDeps = Object.entries(plugin.requiredEnginePlugins); + for (const [enginePluginName, versionRange] of pluginOpenSearchDeps) { + // add check to see if the installing Dashboards plugin version is compatible with installed OpenSearch plugin + if ( + !this.isVersionCompatibleOSPluginInstalled( + opensearchInstalledPlugins, + enginePluginName, + versionRange + ) + ) { this.log.warn( - `OpenSearch plugin "${opensearchPlugin}" is not installed on the cluster for the OpenSearch Dashboards plugin to function as expected.` + `OpenSearch plugin "${enginePluginName}" is not installed on the engine for the OpenSearch Dashboards plugin to function as expected.` ); } - }); + } } } + private isVersionCompatibleOSPluginInstalled( + opensearchInstalledPlugins: CatPluginsResponse, + depPlugin: string, + versionRange: string + ) { + return opensearchInstalledPlugins.find( + (obj) => + obj.component === depPlugin && + semver.satisfies(semver.coerce(obj.version)!.version, versionRange) + ); + } + private async getOpenSearchPlugins(deps: PluginsServiceStartDeps) { // Makes cat.plugin api call to fetch list of OpenSearch plugins installed on the cluster try { diff --git a/src/core/server/plugins/types.ts b/src/core/server/plugins/types.ts index 12b28f48f237..66c8115efc4a 100644 --- a/src/core/server/plugins/types.ts +++ b/src/core/server/plugins/types.ts @@ -105,6 +105,9 @@ export type PluginName = string; /** @public */ export type PluginOpaqueId = symbol; +/** @public */ +export type CompatibleEnginePluginVersions = Record; + /** @internal */ export interface PluginDependencies { asNames: ReadonlyMap; @@ -155,10 +158,10 @@ 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 + * An optional list of the OpenSearch plugins that **must be** installed on the cluster * for this plugin to function properly. */ - readonly requiredOpenSearchPlugins: readonly PluginName[]; + readonly requiredEnginePlugins: CompatibleEnginePluginVersions; /** * List of plugin ids that this plugin's UI code imports modules from that are