Allow optional OSS to X-Pack dependencies (elastic#107432)

Author: jportner

.eslintrc.js

@@ -474,11 +474,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/**/*'],

src/core/server/plugins/plugin.test.ts

@@ -39,6 +39,11 @@ jest.doMock(join('plugin-with-wrong-initializer-path', 'server'), () => ({ plugi
   virtual: true,
 });
 
+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';
+
 function createPluginManifest(manifestProps: Partial<PluginManifest> = {}): PluginManifest {
   return {
     id: 'some-plugin-id',
@@ -97,10 +102,49 @@ test('`constructor` correctly initializes plugin instance', () => {
   expect(plugin.name).toBe('some-plugin-id');
   expect(plugin.configPath).toBe('path');
   expect(plugin.path).toBe('some-plugin-path');
+  expect(plugin.source).toBe('external'); // see below for test cases for non-external sources (OSS and X-Pack)
   expect(plugin.requiredPlugins).toEqual(['some-required-dep']);
   expect(plugin.optionalPlugins).toEqual(['some-optional-dep']);
 });
 
+describe('`constructor` correctly sets non-external source', () => {
+  function createPlugin(path: string) {
+    const manifest = createPluginManifest();
+    const opaqueId = Symbol();
+    return new PluginWrapper({
+      path,
+      manifest,
+      opaqueId,
+      initializerContext: createPluginInitializerContext(
+        coreContext,
+        opaqueId,
+        manifest,
+        instanceInfo
+      ),
+    });
+  }
+
+  test('for OSS plugin in POSIX', () => {
+    const plugin = createPlugin(OSS_PLUGIN_PATH_POSIX);
+    expect(plugin.source).toBe('oss');
+  });
+
+  test('for OSS plugin in Windows', () => {
+    const plugin = createPlugin(OSS_PLUGIN_PATH_WINDOWS);
+    expect(plugin.source).toBe('oss');
+  });
+
+  test('for X-Pack plugin in POSIX', () => {
+    const plugin = createPlugin(XPACK_PLUGIN_PATH_POSIX);
+    expect(plugin.source).toBe('x-pack');
+  });
+
+  test('for X-Pack plugin in Windows', () => {
+    const plugin = createPlugin(XPACK_PLUGIN_PATH_WINDOWS);
+    expect(plugin.source).toBe('x-pack');
+  });
+});
+
 test('`setup` fails if `plugin` initializer is not exported', () => {
   const manifest = createPluginManifest();
   const opaqueId = Symbol();

src/core/server/plugins/plugin.ts

@@ -27,6 +27,9 @@ import {
 } from './types';
 import { CorePreboot, CoreSetup, CoreStart } from '..';
 
+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
+
 /**
  * Lightweight wrapper around discovered plugin that is responsible for instantiating
  * plugin and dispatching proper context and dependencies into plugin's lifecycle hooks.
@@ -40,6 +43,7 @@ export class PluginWrapper<
   TPluginsStart extends object = object
 > {
   public readonly path: string;
+  public readonly source: 'oss' | 'x-pack' | 'external';
   public readonly manifest: PluginManifest;
   public readonly opaqueId: PluginOpaqueId;
   public readonly name: PluginManifest['id'];
@@ -70,6 +74,7 @@ export class PluginWrapper<
     }
   ) {
     this.path = params.path;
+    this.source = getPluginSource(params.path);
     this.manifest = params.manifest;
     this.opaqueId = params.opaqueId;
     this.initializerContext = params.initializerContext;
@@ -204,3 +209,12 @@ export class PluginWrapper<
     return this.manifest.type === PluginType.preboot;
   }
 }
+
+function getPluginSource(path: string): 'oss' | 'x-pack' | 'external' {
+  if (OSS_PATH_REGEX.test(path)) {
+    return 'oss';
+  } else if (XPACK_PATH_REGEX.test(path)) {
+    return 'x-pack';
+  }
+  return 'external';
+}

src/core/server/plugins/plugins_service.test.ts

@@ -52,6 +52,15 @@ expect.addSnapshotSerializer(createAbsolutePathSerializer());
   });
 });
 
+const OSS_PLUGIN_PATH = '/kibana/src/plugins/ossPlugin';
+const XPACK_PLUGIN_PATH = '/kibana/x-pack/plugins/xPackPlugin';
+const EXTERNAL_PLUGIN_PATH = '/kibana/plugins/externalPlugin';
+[OSS_PLUGIN_PATH, XPACK_PLUGIN_PATH, EXTERNAL_PLUGIN_PATH].forEach((path) => {
+  jest.doMock(join(path, 'server'), () => ({}), {
+    virtual: true,
+  });
+});
+
 const createPlugin = (
   id: string,
   {
@@ -247,6 +256,75 @@ describe('PluginsService', () => {
       expect(standardMockPluginSystem.setupPlugins).not.toHaveBeenCalled();
     });
 
+    describe('X-Pack dependencies', () => {
+      function mockDiscoveryResults(params: { sourcePluginPath: string; dependencyType: string }) {
+        const { sourcePluginPath, dependencyType } = params;
+        // Each plugin's source is derived from its path; the PluginWrapper test suite contains more detailed test cases around the paths (for both POSIX and Windows)
+        mockDiscover.mockReturnValue({
+          error$: from([]),
+          plugin$: from([
+            createPlugin('sourcePlugin', {
+              path: sourcePluginPath,
+              version: 'some-version',
+              configPath: 'path',
+              requiredPlugins: dependencyType === 'requiredPlugin' ? ['xPackPlugin'] : [],
+              requiredBundles: dependencyType === 'requiredBundle' ? ['xPackPlugin'] : undefined,
+              optionalPlugins: dependencyType === 'optionalPlugin' ? ['xPackPlugin'] : [],
+            }),
+            createPlugin('xPackPlugin', {
+              path: XPACK_PLUGIN_PATH,
+              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 "sourcePlugin", which is prohibited. Consider making this an optional dependency instead.`
+        );
+        expect(standardMockPluginSystem.addPlugin).not.toHaveBeenCalled();
+      }
+      async function expectSuccess() {
+        await expect(pluginsService.discover({ environment: environmentPreboot })).resolves.toEqual(
+          expect.anything()
+        );
+        expect(standardMockPluginSystem.addPlugin).toHaveBeenCalled();
+      }
+
+      it('throws if an OSS plugin requires an X-Pack plugin or bundle', async () => {
+        for (const dependencyType of ['requiredPlugin', 'requiredBundle']) {
+          mockDiscoveryResults({ sourcePluginPath: OSS_PLUGIN_PATH, dependencyType });
+          await expectError();
+        }
+      });
+
+      it('does not throw if an OSS plugin has an optional dependency on an X-Pack plugin', async () => {
+        mockDiscoveryResults({
+          sourcePluginPath: OSS_PLUGIN_PATH,
+          dependencyType: 'optionalPlugin',
+        });
+        await expectSuccess();
+      });
+
+      it('does not throw if an X-Pack plugin depends on an X-Pack plugin or bundle', async () => {
+        for (const dependencyType of ['requiredPlugin', 'requiredBundle', 'optionalPlugin']) {
+          mockDiscoveryResults({ sourcePluginPath: XPACK_PLUGIN_PATH, dependencyType });
+          await expectSuccess();
+        }
+      });
+
+      it('does not throw if an external plugin depends on an X-Pack plugin or bundle', async () => {
+        for (const dependencyType of ['requiredPlugin', 'requiredBundle', 'optionalPlugin']) {
+          mockDiscoveryResults({ sourcePluginPath: EXTERNAL_PLUGIN_PATH, dependencyType });
+          await expectSuccess();
+        }
+      });
+    });
+
     it('properly detects plugins that should be disabled.', async () => {
       jest
         .spyOn(configService, 'isEnabledAtPath')

src/core/server/plugins/plugins_service.ts

@@ -314,27 +314,7 @@ export class PluginsService implements CoreService<PluginsServiceSetup, PluginsS
       .toPromise();
 
     for (const [pluginName, { plugin, isEnabled }] of pluginEnableStatuses) {
-      // validate that `requiredBundles` ids point to a discovered plugin which `includesUiPlugin`
-      for (const requiredBundleId of plugin.requiredBundles) {
-        if (!pluginEnableStatuses.has(requiredBundleId)) {
-          throw new Error(
-            `Plugin bundle with id "${requiredBundleId}" is required by plugin "${pluginName}" but it is missing.`
-          );
-        }
-
-        const requiredPlugin = pluginEnableStatuses.get(requiredBundleId)!.plugin;
-        if (!requiredPlugin.includesUiPlugin) {
-          throw new Error(
-            `Plugin bundle with id "${requiredBundleId}" is required by plugin "${pluginName}" but it doesn't have a UI bundle.`
-          );
-        }
-
-        if (requiredPlugin.manifest.type !== plugin.manifest.type) {
-          throw new Error(
-            `Plugin bundle with id "${requiredBundleId}" is required by plugin "${pluginName}" and expected to have "${plugin.manifest.type}" type, but its type is "${requiredPlugin.manifest.type}".`
-          );
-        }
-      }
+      this.validatePluginDependencies(plugin, pluginEnableStatuses);
 
       const pluginEnablement = this.shouldEnablePlugin(pluginName, pluginEnableStatuses);
 
@@ -358,6 +338,48 @@ export class PluginsService implements CoreService<PluginsServiceSetup, PluginsS
     this.log.debug(`Discovered ${pluginEnableStatuses.size} plugins.`);
   }
 
+  /** Throws an error if the plugin's dependencies are invalid. */
+  private validatePluginDependencies(
+    plugin: PluginWrapper,
+    pluginEnableStatuses: Map<PluginName, { plugin: PluginWrapper; isEnabled: boolean }>
+  ) {
+    const { name, manifest, requiredBundles, requiredPlugins } = plugin;
+
+    // validate that `requiredBundles` ids point to a discovered plugin which `includesUiPlugin`
+    for (const requiredBundleId of requiredBundles) {
+      if (!pluginEnableStatuses.has(requiredBundleId)) {
+        throw new Error(
+          `Plugin bundle with id "${requiredBundleId}" is required by plugin "${name}" but it is missing.`
+        );
+      }
+
+      const requiredPlugin = pluginEnableStatuses.get(requiredBundleId)!.plugin;
+      if (!requiredPlugin.includesUiPlugin) {
+        throw new Error(
+          `Plugin bundle with id "${requiredBundleId}" is required by plugin "${name}" but it doesn't have a UI bundle.`
+        );
+      }
+
+      if (requiredPlugin.manifest.type !== plugin.manifest.type) {
+        throw new Error(
+          `Plugin bundle with id "${requiredBundleId}" is required by plugin "${name}" and expected to have "${manifest.type}" type, but its type is "${requiredPlugin.manifest.type}".`
+        );
+      }
+    }
+
+    // validate that OSS plugins do not have required dependencies on X-Pack plugins
+    if (plugin.source === 'oss') {
+      for (const id of [...requiredPlugins, ...requiredBundles]) {
+        const requiredPlugin = pluginEnableStatuses.get(id);
+        if (requiredPlugin && requiredPlugin.plugin.source === 'x-pack') {
+          throw new Error(
+            `X-Pack plugin or bundle with id "${id}" is required by OSS plugin "${name}", which is prohibited. Consider making this an optional dependency instead.`
+          );
+        }
+      }
+    }
+  }
+
   private shouldEnablePlugin(
     pluginName: PluginName,
     pluginEnableStatuses: Map<PluginName, { plugin: PluginWrapper; isEnabled: boolean }>,