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 (opensearch-project#3116) (opensearch-project#4194)

Resolves Issue -opensearch-project#2799


(cherry picked from commit f017eaa)

Signed-off-by: Manasvini B Suryanarayana <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
  • Loading branch information
1 parent a7cb787 commit 7e46ea1
Show file tree
Hide file tree
Showing 11 changed files with 230 additions and 3 deletions.
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
79 changes: 79 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 @@ -247,6 +247,79 @@ 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(
Expand Down Expand Up @@ -301,6 +374,7 @@ test('set defaults for all missing optional fields', async () => {
opensearchDashboardsVersion: '7.0.0',
optionalPlugins: [],
requiredPlugins: [],
requiredOpenSearchPlugins: [],
requiredBundles: [],
server: true,
ui: false,
Expand All @@ -317,6 +391,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 @@ -330,6 +405,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 @@ -344,6 +420,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 @@ -356,6 +433,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 @@ -383,6 +461,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
26 changes: 26 additions & 0 deletions src/core/server/plugins/discovery/plugin_manifest_parser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ const KNOWN_MANIFEST_FIELDS = (() => {
version: true,
configPath: true,
requiredPlugins: true,
requiredOpenSearchPlugins: true,
optionalPlugins: true,
ui: true,
server: true,
Expand Down Expand Up @@ -156,6 +157,28 @@ 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
Expand Down Expand Up @@ -198,6 +221,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
77 changes: 75 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.mockResolvedValue({
body: [
{
name: 'node-1',
component: 'test-plugin',
version: 'v1',
},
],
} as any);

expect([...(await pluginsSystem.setupPlugins(setupDeps))]).toMatchInlineSnapshot(`
Array [
Array [
Expand Down Expand Up @@ -484,6 +503,16 @@ 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({});
Expand Down Expand Up @@ -517,4 +546,48 @@ 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 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);
});
});
Loading

0 comments on commit 7e46ea1

Please sign in to comment.