Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Backport 2.x] Adds plugin manifest config to define OpenSearch plugin dependency and verifies if it is installed on the cluster #4194

Merged
merged 1 commit into from
Jun 1, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
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