From d71ced271b1cddc6495e6b8052ca83e119188253 Mon Sep 17 00:00:00 2001 From: Kawika Avilla Date: Thu, 1 Jun 2023 20:44:44 +0000 Subject: [PATCH] Revert "Adds plugin manifest config to define OpenSearch plugin dependency and verifies if it is installed (#3116) (#4195)" This reverts commit e85ea81b4274450a69548bb8a6d14a8c59281606. Signed-off-by: Kawika Avilla --- .../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 -- 11 files changed, 3 insertions(+), 230 deletions(-) diff --git a/src/core/public/plugins/plugins_service.test.ts b/src/core/public/plugins/plugins_service.test.ts index 5fa3bf888b5c..5ca7c491f72f 100644 --- a/src/core/public/plugins/plugins_service.test.ts +++ b/src/core/public/plugins/plugins_service.test.ts @@ -84,7 +84,6 @@ 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 66fd1336a554..18b715d95050 100644 --- a/src/core/server/plugins/discovery/plugin_manifest_parser.test.ts +++ b/src/core/server/plugins/discovery/plugin_manifest_parser.test.ts @@ -247,79 +247,6 @@ 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( @@ -374,7 +301,6 @@ test('set defaults for all missing optional fields', async () => { opensearchDashboardsVersion: '7.0.0', optionalPlugins: [], requiredPlugins: [], - requiredOpenSearchPlugins: [], requiredBundles: [], server: true, ui: false, @@ -391,7 +317,6 @@ 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, }) ) @@ -405,7 +330,6 @@ 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, }); @@ -420,7 +344,6 @@ 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, }) ) @@ -433,7 +356,6 @@ 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, @@ -461,7 +383,6 @@ 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 ee435d2e310a..2a5ccf611f0c 100644 --- a/src/core/server/plugins/discovery/plugin_manifest_parser.ts +++ b/src/core/server/plugins/discovery/plugin_manifest_parser.ts @@ -61,7 +61,6 @@ const KNOWN_MANIFEST_FIELDS = (() => { version: true, configPath: true, requiredPlugins: true, - requiredOpenSearchPlugins: true, optionalPlugins: true, ui: true, server: true, @@ -157,28 +156,6 @@ 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 @@ -221,9 +198,6 @@ 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 4d1660f65c75..34310ea3de37 100644 --- a/src/core/server/plugins/integration_tests/plugins_service.test.ts +++ b/src/core/server/plugins/integration_tests/plugins_service.test.ts @@ -57,7 +57,6 @@ describe('PluginsService', () => { disabled = false, version = 'some-version', requiredPlugins = [], - requiredOpenSearchPlugins = [], requiredBundles = [], optionalPlugins = [], opensearchDashboardsVersion = '7.0.0', @@ -69,7 +68,6 @@ describe('PluginsService', () => { disabled?: boolean; version?: string; requiredPlugins?: string[]; - requiredOpenSearchPlugins?: string[]; requiredBundles?: string[]; optionalPlugins?: string[]; opensearchDashboardsVersion?: string; @@ -86,7 +84,6 @@ 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 04df84ab8d12..9543d379493c 100644 --- a/src/core/server/plugins/plugin.test.ts +++ b/src/core/server/plugins/plugin.test.ts @@ -69,7 +69,6 @@ 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 cc53e1b443e9..89a2aecc82f3 100644 --- a/src/core/server/plugins/plugin.ts +++ b/src/core/server/plugins/plugin.ts @@ -66,7 +66,6 @@ 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']; @@ -96,7 +95,6 @@ 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 ca46a97a237e..c55603679c35 100644 --- a/src/core/server/plugins/plugin_context.test.ts +++ b/src/core/server/plugins/plugin_context.test.ts @@ -56,7 +56,6 @@ 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 c3ee05d60ed8..b020e56539a7 100644 --- a/src/core/server/plugins/plugins_service.test.ts +++ b/src/core/server/plugins/plugins_service.test.ts @@ -78,7 +78,6 @@ const createPlugin = ( disabled = false, version = 'some-version', requiredPlugins = [], - requiredOpenSearchPlugins = [], requiredBundles = [], optionalPlugins = [], opensearchDashboardsVersion = '7.0.0', @@ -90,7 +89,6 @@ const createPlugin = ( disabled?: boolean; version?: string; requiredPlugins?: string[]; - requiredOpenSearchPlugins?: string[]; requiredBundles?: string[]; optionalPlugins?: string[]; opensearchDashboardsVersion?: string; @@ -107,7 +105,6 @@ 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 286bb0d97846..2e8aa6b4a4c0 100644 --- a/src/core/server/plugins/plugins_system.test.ts +++ b/src/core/server/plugins/plugins_system.test.ts @@ -53,16 +53,9 @@ function createPlugin( { required = [], optional = [], - requiredOSPlugin = [], server = true, ui = true, - }: { - required?: string[]; - optional?: string[]; - requiredOSPlugin?: string[]; - server?: boolean; - ui?: boolean; - } = {} + }: { required?: string[]; optional?: string[]; server?: boolean; ui?: boolean } = {} ) { return new PluginWrapper({ path: 'some-path', @@ -72,7 +65,6 @@ function createPlugin( configPath: 'path', opensearchDashboardsVersion: '7.0.0', requiredPlugins: required, - requiredOpenSearchPlugins: requiredOSPlugin, optionalPlugins: optional, requiredBundles: [], server, @@ -195,7 +187,7 @@ 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'] }), { setup: { 'order-2': 'added-as-2' }, start: { 'order-2': 'started-as-2' }, @@ -252,17 +244,6 @@ 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 [ @@ -503,16 +484,6 @@ 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({}); @@ -546,48 +517,4 @@ 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 6445f38f8e70..b3adc71323f9 100644 --- a/src/core/server/plugins/plugins_system.ts +++ b/src/core/server/plugins/plugins_system.ts @@ -158,45 +158,13 @@ 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 12b28f48f237..cf769b45daa3 100644 --- a/src/core/server/plugins/types.ts +++ b/src/core/server/plugins/types.ts @@ -154,12 +154,6 @@ 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`.