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

Allow optional OSS to X-Pack dependencies #107432

Merged
5 changes: 0 additions & 5 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -470,11 +470,6 @@ module.exports = {
errorMessage:
'Server modules cannot be imported into client modules or shared modules.',
},
{
target: ['src/**/*'],
from: ['x-pack/**/*'],
errorMessage: 'OSS cannot import x-pack files.',
},
{
target: ['src/core/**/*'],
from: ['plugins/**/*', 'src/plugins/**/*'],
Expand Down
121 changes: 121 additions & 0 deletions src/core/server/plugins/plugins_service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,21 @@ expect.addSnapshotSerializer(createAbsolutePathSerializer());
});
});

const OSS_PLUGIN_PATH_POSIX = '/kibana/src/plugins/ossPlugin';
const OSS_PLUGIN_PATH_WINDOWS = 'C:\\kibana\\src\\plugins\\ossPlugin';
const XPACK_PLUGIN_PATH_POSIX = '/kibana/x-pack/plugins/xPackPlugin';
const XPACK_PLUGIN_PATH_WINDOWS = 'C:\\kibana\\x-pack\\plugins\\xPackPlugin';
[
OSS_PLUGIN_PATH_POSIX,
OSS_PLUGIN_PATH_WINDOWS,
XPACK_PLUGIN_PATH_POSIX,
XPACK_PLUGIN_PATH_WINDOWS,
].forEach((path) => {
jest.doMock(join(path, 'server'), () => ({}), {
virtual: true,
});
});

const createPlugin = (
id: string,
{
Expand Down Expand Up @@ -247,6 +262,112 @@ describe('PluginsService', () => {
expect(standardMockPluginSystem.setupPlugins).not.toHaveBeenCalled();
});

describe('OSS to X-Pack dependencies', () => {
function mockDiscoveryResults({
ossPath,
xPackPath,
dependency,
}: {
ossPath: string;
xPackPath: string;
dependency: 'requiredPlugin' | 'requiredBundle' | 'optionalPlugin';
}) {
mockDiscover.mockReturnValue({
error$: from([]),
plugin$: from([
createPlugin('ossPlugin', {
path: ossPath,
version: 'some-version',
configPath: 'path',
requiredPlugins: dependency === 'requiredPlugin' ? ['xPackPlugin'] : [],
requiredBundles: dependency === 'requiredBundle' ? ['xPackPlugin'] : undefined,
optionalPlugins: dependency === 'optionalPlugin' ? ['xPackPlugin'] : [],
}),
createPlugin('xPackPlugin', {
path: xPackPath,
version: 'some-version',
configPath: 'path',
requiredPlugins: [],
optionalPlugins: [],
}),
]),
});
}

async function expectError() {
await expect(pluginsService.discover({ environment: environmentPreboot })).rejects.toThrow(
`X-Pack plugin or bundle with id "xPackPlugin" is required by OSS plugin "ossPlugin", which is prohibited.`
);
expect(standardMockPluginSystem.addPlugin).not.toHaveBeenCalled();
}
async function expectSuccess() {
await expect(pluginsService.discover({ environment: environmentPreboot })).resolves.toEqual(
expect.anything()
);
expect(standardMockPluginSystem.addPlugin).toHaveBeenCalledTimes(2);
}

describe('throws if an OSS plugin requires an X-Pack plugin', () => {
it('in POSIX', async () => {
mockDiscoveryResults({
ossPath: OSS_PLUGIN_PATH_POSIX,
xPackPath: XPACK_PLUGIN_PATH_POSIX,
dependency: 'requiredPlugin',
});
await expectError();
});

it('in Windows', async () => {
mockDiscoveryResults({
ossPath: OSS_PLUGIN_PATH_WINDOWS,
xPackPath: XPACK_PLUGIN_PATH_WINDOWS,
dependency: 'requiredPlugin',
});
await expectError();
});
});

describe('throws if an OSS plugin requires an X-Pack bundle', () => {
it('in POSIX', async () => {
mockDiscoveryResults({
ossPath: OSS_PLUGIN_PATH_POSIX,
xPackPath: XPACK_PLUGIN_PATH_POSIX,
dependency: 'requiredBundle',
});
await expectError();
});

it('in Windows', async () => {
mockDiscoveryResults({
ossPath: OSS_PLUGIN_PATH_WINDOWS,
xPackPath: XPACK_PLUGIN_PATH_WINDOWS,
dependency: 'requiredBundle',
});
await expectError();
});
});

describe('does not throw if an OSS plugin has an optional dependency on an X-Pack plugin', () => {
it('in POSIX', async () => {
mockDiscoveryResults({
ossPath: OSS_PLUGIN_PATH_POSIX,
xPackPath: XPACK_PLUGIN_PATH_POSIX,
dependency: 'optionalPlugin',
});
await expectSuccess();
});

it('in Windows', async () => {
mockDiscoveryResults({
ossPath: OSS_PLUGIN_PATH_WINDOWS,
xPackPath: XPACK_PLUGIN_PATH_WINDOWS,
dependency: 'optionalPlugin',
});
await expectSuccess();
});
});
});

it('properly detects plugins that should be disabled.', async () => {
jest
.spyOn(configService, 'isEnabledAtPath')
Expand Down
15 changes: 15 additions & 0 deletions src/core/server/plugins/plugins_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,9 @@ export interface PluginsServiceDiscoverDeps {
environment: InternalEnvironmentServicePreboot;
}

const OSS_PATH_REGEX = /[\/|\\]src[\/|\\]plugins[\/|\\]/; // Matches src/plugins directory on POSIX and Windows
const XPACK_PATH_REGEX = /[\/|\\]x-pack[\/|\\]plugins[\/|\\]/; // Matches x-pack/plugins directory on POSIX and Windows
mshustov marked this conversation as resolved.
Show resolved Hide resolved

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking it'd be nice if we added a source property to the PluginWrapper class inside src/core/server/plugins/plugin.ts and populated it in the constructor. Then we could use this information in other places as well rather than only inside the service.

I'm fairly confident that it would be a small change to this PR, WDYT?

Copy link
Contributor Author

@jportner jportner Aug 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to clarify: the PluginWrapper class already has a path field.

Do you envision the source field as a string literal such as 'x-pack' | 'oss'?

Edit: done in daebba1.

/** @internal */
export class PluginsService implements CoreService<PluginsServiceSetup, PluginsServiceStart> {
private readonly log: Logger;
Expand Down Expand Up @@ -336,6 +339,18 @@ export class PluginsService implements CoreService<PluginsServiceSetup, PluginsS
}
}

// validate that OSS plugins do not have required dependencies on X-Pack plugins
if (OSS_PATH_REGEX.test(plugin.path)) {
for (const id of [...plugin.requiredPlugins, ...plugin.requiredBundles]) {
const requiredPlugin = pluginEnableStatuses.get(id);
if (requiredPlugin && XPACK_PATH_REGEX.test(requiredPlugin.plugin.path)) {
throw new Error(
`X-Pack plugin or bundle with id "${id}" is required by OSS plugin "${pluginName}", which is prohibited.`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we extend the error message to suggest making it optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in daebba1 😄

);
}
}
}

const pluginEnablement = this.shouldEnablePlugin(pluginName, pluginEnableStatuses);

if (pluginEnablement.enabled) {
Expand Down