Allow plugin manifest config to define semver compatible OpenSearch p…

…lugin and verify if it is installed on the cluster Signed-off-by: Manasvini B Suryanarayana <[email protected]>
opensearch-project · Aug 23, 2023 · f688747 · f688747
1 parent d7c5577
commit f688747
Show file tree

Hide file tree

Showing 12 changed files with 236 additions and 79 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -27,6 +27,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
@@ -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
@@ -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 a map of plugin name to its compatible version ranges. (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 a map of plugin name to its compatible version ranges. (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 "requiredOpenSearchPlugins" in plugin manifest for "id3" should be an array of strings. (invalid-manifest, ${pluginManifestPath})`,
+      message: `The "requiredEnginePlugins" in plugin manifest for "test-invalid-version-type-plugin" should be a map of plugin name to its compatible version ranges. (invalid-manifest, ${pluginManifestPath})`,
       type: PluginDiscoveryErrorType.InvalidManifest,
       path: pluginManifestPath,
     });
   });
 
-  test('Happy path when plugin `requiredOpenSearchPlugins` is an array of string', async () => {
+  test('return error when plugin requiredEnginePlugins contains invalid version range', async () => {
     mockReadFilePromise.mockResolvedValue(
       Buffer.from(
         JSON.stringify({
-          id: 'id1',
+          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 "requiredEnginePlugins" in plugin manifest for "test-invalid-version-range-plugin" has an invalid version range defined for the following plugins: invalid-version-range-plugin (invalid-manifest, ${pluginManifestPath})`,
+      type: PluginDiscoveryErrorType.InvalidManifest,
+      path: pluginManifestPath,
+    });
+  });
+
+  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: '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,
@@ -385,27 +423,33 @@ test('return all set optional fields as they are in manifest', async () => {
   mockReadFilePromise.mockResolvedValue(
     Buffer.from(
       JSON.stringify({
-        id: 'someId',
+        id: 'someId-100',
         configPath: ['some', 'path'],
         version: 'some-version',
         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,
       })
     )
   );
 
   await expect(parseManifest(pluginPath, packageInfo, logger)).resolves.toEqual({
-    id: 'someId',
+    id: 'someId-100',
     configPath: ['some', 'path'],
     version: 'some-version',
     opensearchDashboardsVersion: '7.0.0',
     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
@@ -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,49 @@ 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 (manifest.requiredEnginePlugins !== undefined) {
+    for (const [pluginId, pluginVersion] of Object.entries(manifest.requiredEnginePlugins)) {
+      if (
+        typeof manifest.requiredEnginePlugins !== 'object' ||
+        typeof pluginId !== 'string' ||
+        typeof pluginVersion !== 'string'
+      ) {
+        throw PluginDiscoveryError.invalidManifest(
+          manifestPath,
+          new Error(
+            `The "requiredEnginePlugins" in plugin manifest for "${manifest.id}" should be a map of plugin name to its compatible version ranges.`
+          )
+        );
+      }
+    }
   }
 
   if (
-    Array.isArray(manifest.requiredOpenSearchPlugins) &&
-    manifest.requiredOpenSearchPlugins.length > 0
+    manifest.requiredEnginePlugins !== undefined &&
+    Object.keys(manifest.requiredEnginePlugins).length > 0
   ) {
     log.info(
-      `Plugin ${manifest.id} has a dependency on following OpenSearch plugin(s): "${manifest.requiredOpenSearchPlugins}".`
+      `Plugin ${manifest.id} has a dependency on following OpenSearch plugin(s): [${Object.keys(
+        manifest.requiredEnginePlugins
+      )}]`
     );
+
+    const invalidPluginVersions: string[] = [];
+    for (const [pluginName, versionRange] of Object.entries(manifest.requiredEnginePlugins)) {
+      if (!isOpenSearchPluginVersionRangeValid(versionRange)) {
+        invalidPluginVersions.push(pluginName);
+      }
+    }
+
+    if (invalidPluginVersions.length > 0) {
+      const pluginNames = invalidPluginVersions.join(', ');
+      throw PluginDiscoveryError.invalidManifest(
+        manifestPath,
+        new Error(
+          `The "requiredEnginePlugins" in plugin manifest for "${manifest.id}" has an invalid version range defined for the following plugins: ${pluginNames}`
+        )
+      );
+    }
   }
 
   const expectedOpenSearchDashboardsVersion =
@@ -221,9 +245,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 +299,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
@@ -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
@@ -69,7 +69,10 @@ function createPluginManifest(manifestProps: Partial<PluginManifest> = {}): 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
@@ -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;