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] [Decouple] Allow plugin manifest config to define semver compatible OpenSearch plugin and verify if it is installed on the cluster #4817

Merged
merged 2 commits into from
Aug 29, 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
2 changes: 1 addition & 1 deletion src/core/public/plugins/plugins_service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ function createManifest(
version: 'some-version',
configPath: ['path'],
requiredPlugins: required,
requiredOpenSearchPlugins: optional,
requiredEnginePlugins: optional,
optionalPlugins: optional,
requiredBundles: [],
};
Expand Down
90 changes: 67 additions & 23 deletions src/core/server/plugins/discovery/plugin_manifest_parser.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 an object that maps a plugin name to a version range. (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 an object that maps a plugin name to a version range. (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 "requiredEnginePlugins" in plugin manifest for "test-invalid-version-type-plugin" should be an object that maps a plugin name to a version range. (invalid-manifest, ${pluginManifestPath})`,
type: PluginDiscoveryErrorType.InvalidManifest,
path: pluginManifestPath,
});
});

test('return error when plugin requiredEnginePlugins contains invalid version range', async () => {
mockReadFilePromise.mockResolvedValue(
Buffer.from(
JSON.stringify({
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 "requiredOpenSearchPlugins" in plugin manifest for "id3" should be an array of strings. (invalid-manifest, ${pluginManifestPath})`,
message: `The "requiredEnginePlugins" in the plugin manifest for "test-invalid-version-range-plugin" contains invalid version ranges: ?1.0.0 for invalid-version-range-plugin (invalid-manifest, ${pluginManifestPath})`,
type: PluginDiscoveryErrorType.InvalidManifest,
path: pluginManifestPath,
});
});

test('Happy path when plugin `requiredOpenSearchPlugins` is an array of string', async () => {
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: 'id1',
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,
Expand Down Expand Up @@ -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,
Expand All @@ -391,7 +429,10 @@ 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'],
requiredEnginePlugins: {
'test-opensearch-plugin-1': '^1.0.0',
'test-opensearch-plugin-2': '>=1.0.0',
},
ui: true,
})
)
Expand All @@ -405,7 +446,10 @@ 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'],
requiredEnginePlugins: {
'test-opensearch-plugin-1': '^1.0.0',
'test-opensearch-plugin-2': '>=1.0.0',
},
server: false,
ui: true,
});
Expand All @@ -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,
})
)
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
71 changes: 49 additions & 22 deletions src/core/server/plugins/discovery/plugin_manifest_parser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -61,7 +62,7 @@ const KNOWN_MANIFEST_FIELDS = (() => {
version: true,
configPath: true,
requiredPlugins: true,
requiredOpenSearchPlugins: true,
requiredEnginePlugins: true,
optionalPlugins: true,
ui: true,
server: true,
Expand Down Expand Up @@ -157,26 +158,42 @@ 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 ('requiredEnginePlugins' in manifest) {
if (
typeof manifest.requiredEnginePlugins !== 'object' ||
!Object.entries(manifest.requiredEnginePlugins).every(
([pluginId, pluginVersion]) =>
typeof pluginId === 'string' && typeof pluginVersion === 'string'
)
);
}
) {
throw PluginDiscoveryError.invalidManifest(
manifestPath,
new Error(
`The "requiredEnginePlugins" in plugin manifest for "${manifest.id}" should be an object that maps a plugin name to a version range.`
)
);
}
const invalidPluginVersions: string[] = [];
for (const [pluginName, versionRange] of Object.entries(manifest.requiredEnginePlugins)) {
log.info(
`Plugin ${manifest.id} has a dependency on engine plugin: [${pluginName}@${versionRange}]`
);

if (
Array.isArray(manifest.requiredOpenSearchPlugins) &&
manifest.requiredOpenSearchPlugins.length > 0
) {
log.info(
`Plugin ${manifest.id} has a dependency on following OpenSearch plugin(s): "${manifest.requiredOpenSearchPlugins}".`
);
if (!isOpenSearchPluginVersionRangeValid(versionRange)) {
invalidPluginVersions.push(`${versionRange} for ${pluginName}`);
}
}

if (invalidPluginVersions.length > 0) {
throw PluginDiscoveryError.invalidManifest(
manifestPath,
new Error(
`The "requiredEnginePlugins" in the plugin manifest for "${
manifest.id
}" contains invalid version ranges: ${invalidPluginVersions.join(', ')}`
)
);
}
}

const expectedOpenSearchDashboardsVersion =
Expand Down Expand Up @@ -221,9 +238,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,
Expand Down Expand Up @@ -276,3 +292,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;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand All @@ -57,7 +57,7 @@ describe('PluginsService', () => {
disabled = false,
version = 'some-version',
requiredPlugins = [],
requiredOpenSearchPlugins = [],
requiredEnginePlugins = {},
requiredBundles = [],
optionalPlugins = [],
opensearchDashboardsVersion = '7.0.0',
Expand All @@ -69,7 +69,7 @@ describe('PluginsService', () => {
disabled?: boolean;
version?: string;
requiredPlugins?: string[];
requiredOpenSearchPlugins?: string[];
requiredEnginePlugins?: CompatibleEnginePluginVersions;
requiredBundles?: string[];
optionalPlugins?: string[];
opensearchDashboardsVersion?: string;
Expand All @@ -86,7 +86,7 @@ describe('PluginsService', () => {
configPath: `${configPath}${disabled ? '-disabled' : ''}`,
opensearchDashboardsVersion,
requiredPlugins,
requiredOpenSearchPlugins,
requiredEnginePlugins,
requiredBundles,
optionalPlugins,
server,
Expand Down
5 changes: 4 additions & 1 deletion src/core/server/plugins/plugin.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
4 changes: 2 additions & 2 deletions src/core/server/plugins/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'];
Expand Down Expand Up @@ -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;
Expand Down
4 changes: 3 additions & 1 deletion src/core/server/plugins/plugin_context.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,9 @@ function createPluginManifest(manifestProps: Partial<PluginManifest> = {}): Plug
configPath: 'path',
opensearchDashboardsVersion: '7.0.0',
requiredPlugins: ['some-required-dep'],
requiredOpenSearchPlugins: ['some-backend-plugin'],
requiredEnginePlugins: {
'test-os-plugin1': '^2.2.1',
},
requiredBundles: [],
optionalPlugins: ['some-optional-dep'],
server: true,
Expand Down
Loading