Skip to content

Commit

Permalink
Adds plugin manifest config to define OpenSearch plugin dependency an…
Browse files Browse the repository at this point in the history
…d verifies if it is installed

Resolves Issue -opensearch-project#2799

Signed-off-by: Manasvini B Suryanarayana <manasvis@amazon.com>
  • Loading branch information
manasvinibs committed Dec 23, 2022
1 parent 100a41a commit 50e5056
Show file tree
Hide file tree
Showing 12 changed files with 218 additions and 3 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- [I18n] Register ru, ru-RU locale ([#2817](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2817))
- Add yarn opensearch arg to setup plugin dependencies ([#2544](https://github.com/opensearch-project/OpenSearch-Dashboards/issues/2544))
- [Multi DataSource] Test the connection to an external data source when creating or updating ([#2973](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2973))
- 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

Expand Down
1 change: 1 addition & 0 deletions src/core/public/plugins/plugins_service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ function createManifest(
version: 'some-version',
configPath: ['path'],
requiredPlugins: required,
requiredOpenSearchPlugins: optional,
optionalPlugins: optional,
requiredBundles: [],
};
Expand Down
60 changes: 60 additions & 0 deletions src/core/server/plugins/discovery/plugin_manifest_parser.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand Down Expand Up @@ -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,
Expand All @@ -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,
})
)
Expand All @@ -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,
});
Expand All @@ -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,
})
)
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
23 changes: 23 additions & 0 deletions src/core/server/plugins/discovery/plugin_manifest_parser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ const KNOWN_MANIFEST_FIELDS = (() => {
version: true,
configPath: true,
requiredPlugins: true,
requiredOpenSearchPlugins: true,
optionalPlugins: true,
ui: true,
server: true,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ describe('PluginsService', () => {
disabled = false,
version = 'some-version',
requiredPlugins = [],
requiredOpenSearchPlugins = [],
requiredBundles = [],
optionalPlugins = [],
opensearchDashboardsVersion = '7.0.0',
Expand All @@ -68,6 +69,7 @@ describe('PluginsService', () => {
disabled?: boolean;
version?: string;
requiredPlugins?: string[];
requiredOpenSearchPlugins?: string[];
requiredBundles?: string[];
optionalPlugins?: string[];
opensearchDashboardsVersion?: string;
Expand All @@ -84,6 +86,7 @@ describe('PluginsService', () => {
configPath: `${configPath}${disabled ? '-disabled' : ''}`,
opensearchDashboardsVersion,
requiredPlugins,
requiredOpenSearchPlugins,
requiredBundles,
optionalPlugins,
server,
Expand Down
1 change: 1 addition & 0 deletions src/core/server/plugins/plugin.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ function createPluginManifest(manifestProps: Partial<PluginManifest> = {}): Plug
configPath: 'path',
opensearchDashboardsVersion: '7.0.0',
requiredPlugins: ['some-required-dep'],
requiredOpenSearchPlugins: ['some-os-plugins'],
optionalPlugins: ['some-optional-dep'],
requiredBundles: [],
server: true,
Expand Down
2 changes: 2 additions & 0 deletions src/core/server/plugins/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'];
Expand Down Expand Up @@ -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;
Expand Down
1 change: 1 addition & 0 deletions src/core/server/plugins/plugin_context.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ function createPluginManifest(manifestProps: Partial<PluginManifest> = {}): Plug
configPath: 'path',
opensearchDashboardsVersion: '7.0.0',
requiredPlugins: ['some-required-dep'],
requiredOpenSearchPlugins: ['some-backend-plugin'],
requiredBundles: [],
optionalPlugins: ['some-optional-dep'],
server: true,
Expand Down
3 changes: 3 additions & 0 deletions src/core/server/plugins/plugins_service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ const createPlugin = (
disabled = false,
version = 'some-version',
requiredPlugins = [],
requiredOpenSearchPlugins = [],
requiredBundles = [],
optionalPlugins = [],
opensearchDashboardsVersion = '7.0.0',
Expand All @@ -89,6 +90,7 @@ const createPlugin = (
disabled?: boolean;
version?: string;
requiredPlugins?: string[];
requiredOpenSearchPlugins?: string[];
requiredBundles?: string[];
optionalPlugins?: string[];
opensearchDashboardsVersion?: string;
Expand All @@ -105,6 +107,7 @@ const createPlugin = (
configPath: `${configPath}${disabled ? '-disabled' : ''}`,
opensearchDashboardsVersion,
requiredPlugins,
requiredOpenSearchPlugins,
requiredBundles,
optionalPlugins,
server,
Expand Down
90 changes: 88 additions & 2 deletions src/core/server/plugins/plugins_system.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand All @@ -65,6 +72,7 @@ function createPlugin(
configPath: 'path',
opensearchDashboardsVersion: '7.0.0',
requiredPlugins: required,
requiredOpenSearchPlugins: requiredOSPlugin,
optionalPlugins: optional,
requiredBundles: [],
server,
Expand Down Expand Up @@ -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' },
Expand Down Expand Up @@ -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 [
Expand Down Expand Up @@ -517,4 +536,71 @@ describe('start', () => {
const log = logger.get.mock.results[0].value as jest.Mocked<Logger>;
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);
});
});
Loading

0 comments on commit 50e5056

Please sign in to comment.