Implement recursive plugin discovery #68811

pgayvallet · 2020-06-10T20:21:31Z

Summary

Update core plugin discovery system and the kibana platform optimizer to look for plugin in sub directories.

Checklist

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

pgayvallet · 2020-06-10T20:34:47Z

packages/kbn-optimizer/src/optimizer/kibana_platform_plugins.ts

- ...scanDirs.map((dir) => `${dir}/*/kibana.json`),
+ ...scanDirs.map((dir) => `${dir}/**/kibana.json`),


Comparing this diff with the changes in plugins_discovery.ts made me cry a little.

Note that the optimizer does not exactly reproduce the logic used in the discovery system:

It doesn't handle maxDepth

It will discover plugins nested inside other plugins

However, I think this is acceptable. Worse case some plugins will be build that are not going to be discovered by core's discovery system. But that would be detected during the development phase anyway. (and implementing the exact same logic would cause findKibanaPlatformPlugins to become way more complex than its currently is)

Wonder if we could have some sort of maxDepth by using a (very unreadable) glob like:

`${dir}/{*,*/*,*/*/*,*/*/*/*,*/*/*/*/*}/kibana.json`

Either that or

...scanDirs .map((dir) => [ `${dir}/*/kibana.json`, `${dir}/*/*/kibana.json`, `${dir}/*/*/*/kibana.json`, `${dir}/*/*/*/*/kibana.json`, `${dir}/*/*/*/*/*/kibana.json`, ]) .flat(),

Which is a little longer (even if it can be extracted to a function), but probably more explicit.

Not 100% sure this is really needed (as it doesn't address the second point), but I have no problem implementing that if we think it's a little better can current implem.

wdyt?

Let's err on the side of caution here and go ahead and do that. Optimizer is already slow as it is, let's be defensive.

src/core/server/plugins/discovery/plugins_discovery.ts

pgayvallet · 2020-06-10T20:44:53Z

src/core/server/plugins/discovery/plugins_discovery.test.ts

+ // jest relies on the filesystem to get sourcemaps when using console.log
+ // which breaks with the mocked FS, see https://github.com/tschaub/mock-fs/issues/234
+ // hijacking logging to process.stdout as a workaround for this suite.
+ jest.spyOn(console, 'log').mockImplementation((...args) => {
+ process.stdout.write(args + '\n');
+ });


Self explicit comment. TIL, jest patches console.log to add source-map capabilities to the output. But it uses fs.lstat at some point in the process, and we are mocking the whole FS for this suite.

There is an open issue to implements layering to mock-fs, but until then, the process.stdout seems like an acceptable workaround. (This doesn't affect the testing behavior in any way)

pgayvallet · 2020-06-10T20:51:35Z

src/core/server/plugins/discovery/plugins_discovery.test.ts

+ mockFs(
+ {
+ [KIBANA_ROOT]: {
+ src: {
+ plugins: {
+ plugin_a: Plugins.valid('pluginA'),
+ },
+ },
+ plugins: {
+ plugin_b: Plugins.valid('pluginB'),
+ },


Why mock-fs:

Better test data isolation. The mocked FS is described in each test, which makes it way easier to understand what we are doing that having to see the content of a fixture folder (at least ihmo)

ATM, we don't have any tool to mock the Env class, and the pluginSearchPaths are hardcoded in it's constructor, meaning that discover will necessarily scan {cwd}/src/plugins. We can't have it use another root folder without dirty tricks. mock-fs allow to easily mock the content of the kibana directory

More powerful than persistent test folders committed in the repository (can define permissions on a file/folder in the fs definition for example. Doing the same on a real FS always leads to troubles)

IMO it's a bit easier to grok this if we used regular directory paths as keys rather than nested objects (which mock-fs supports):

mockFs({ [KIBANA_ROOT]: { 'src/plugins/plugin_a': Plugins.valid('pluginA'), 'plugins/plugin_b': /** */, 'x-pack/plugins/plugin_c': /** */, } })

It's supposed to, however I'm having failures when running your changes.

When looking at the actual implementation of mock-fs's filesystem, it seems the populate logic does not accept nested multi-path keys (...)

This works:

mockFs( { [`${KIBANA_ROOT}/src/plugins/plugin_a`]: Plugins.valid('pluginA'), [`${KIBANA_ROOT}/plugins/plugin_b`]: Plugins.valid('pluginB'), }, { createCwd: false } );

but this doesn't:

mockFs( { [KIBANA_ROOT]: { 'src/plugins/plugin_a': Plugins.valid('pluginA'), 'plugins/plugin_b': Plugins.valid('pluginB'), }, }, { createCwd: false } );

wdyt? Do we still prefer the long

[`${KIBANA_ROOT}/src/plugins/plugin_a`]: Plugins.valid('pluginA'),

syntax over the one currently used?

I think that would be slight more clear, yes.

…ve-plugin-discovery

elasticmachine · 2020-06-11T08:00:40Z

Pinging @elastic/kibana-platform (Team:Platform)

spalger · 2020-06-11T22:58:33Z

@pgayvallet Out of curiosity, as described by #59189 this doesn't support plugins inside of plugins right? (kind of implied by "recursive plugin discovery")

pgayvallet · 2020-06-12T06:30:40Z

this doesn't support plugins inside of plugins right?

Right. The PR title might be a little misleading. It scans sub-directories until it finds a plugin, and then won't go further on this specific branch.

(Even if this specific rule is not 'respected' by the way i modified the optimizer, see #68811 (comment))

…ve-plugin-discovery

spalger

@kbn/optimizer changes LGTM

joshdover

Tested locally without any issues. A few comments, but no major problems stood out to me.

src/core/server/plugins/discovery/plugins_discovery.ts

joshdover · 2020-06-18T20:49:32Z

packages/kbn-optimizer/src/optimizer/kibana_platform_plugins.ts

- ...scanDirs.map((dir) => `${dir}/*/kibana.json`),
+ ...scanDirs.map((dir) => `${dir}/**/kibana.json`),


Wonder if we could have some sort of maxDepth by using a (very unreadable) glob like:

`${dir}/{*,*/*,*/*/*,*/*/*/*,*/*/*/*/*}/kibana.json`

joshdover · 2020-06-18T20:53:09Z

src/core/server/plugins/discovery/plugins_discovery.test.ts

+ mockFs(
+ {
+ [KIBANA_ROOT]: {
+ src: {
+ plugins: {
+ plugin_a: Plugins.valid('pluginA'),
+ },
+ },
+ plugins: {
+ plugin_b: Plugins.valid('pluginB'),
+ },


IMO it's a bit easier to grok this if we used regular directory paths as keys rather than nested objects (which mock-fs supports):

mockFs({ [KIBANA_ROOT]: { 'src/plugins/plugin_a': Plugins.valid('pluginA'), 'plugins/plugin_b': /** */, 'x-pack/plugins/plugin_c': /** */, } })

joshdover · 2020-06-18T20:54:11Z

src/core/server/plugins/discovery/plugins_discovery.test.ts

-
- await expect(
- error$
+ it('return errors when the plugin search path is not accessible', async () => {


Should we have a similar test for if the kibana.json file is inaccessible?

Added. However even in 000 mode, the stat operation returns in success (which is normal). The error is therefor catched a little late, in parseManifest. It's probably still acceptable (and I don't really want to perform a try-read just to catch the error sooner)

joshdover · 2020-06-18T20:56:39Z

src/core/server/plugins/discovery/plugins_discovery.test.ts

+const manifestPath = (...pluginPath: string[]) =>
+ resolve(KIBANA_ROOT, 'src', 'plugins', ...pluginPath, 'kibana.json');
+
+describe('plugins discovery system', () => {


Should we have a test that uses symlinks? I think that's probably a common use-case for 3rd party plugins. Good news is it looks like mock-fs supports symlinks.

What do you have in mind exactly? A symlink from a nested plugin directory to an arbitrary other path on the FS containing the plugin files?

Main use case doesn't really have to do with nested directories, but I think it's probably common for deployments to symlink the <kibana root>/plugins directory for custom plugins.

…ve-plugin-discovery

…fest

…ve-plugin-discovery

joshdover

LGTM after #68811 (comment) and #68811 (comment) are resolved. Lots of interest in this change, it's going be very helpful!

…ve-plugin-discovery

pgayvallet · 2020-06-29T08:14:53Z

retest

…ve-plugin-discovery

kibanamachine · 2020-06-29T23:05:17Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 39f2765

Build metrics

✅ unchanged

History

💚 Build #57118 succeeded 9b67058
💔 Build #57010 failed c05880a
💔 Build #56575 failed f25b051
💚 Build #55823 succeeded fa308bf
💚 Build #55456 succeeded 5c00548

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

* implements recursive scanning in plugin discovery system * update optimizer to find plugins in sub-directories * update renovate * update optimizer IT snapshot * refactor processPluginSearchPaths$ and add test for inaccessible manifest * add symlink test * add maxDepth to the optimizer * adapt mockFs definitions * remove `flat` usage # Conflicts: # renovate.json5

…ata-streams * 'master' of github.com:elastic/kibana: (50 commits) [Logs UI] [Alerting] "Group by" functionality (elastic#68250) [Discover] Deangularize Skip to bottom button (elastic#69811) Implement recursive plugin discovery (elastic#68811) Use ts-expect-error in platform code (elastic#69883) [SIEM][Detection Engine][Lists] Moves getQueryFilter to common folder for use by both front and backend [Ingest Manager][SECURITY SOLUTION] adjust config reassign link and add roundtrip to Reassignment flow (elastic#70208) [Security][Lists] Add API functions and react hooks for value list APIs (elastic#69603) [ILM] Fix bug when clearing priority field (elastic#70154) [Platform][Security] Updates cluster_manager ignorePaths to include security scripts (elastic#70139) [IngestManager] Allow to filter agent by packages (elastic#69731) [code coverage] exclude folders: test_helpers, tests_bundle (elastic#70199) [Metrics UI] UX improvements for saved views (elastic#69910) [APM] docs: unique transaction troubleshooting (elastic#69831) Cross cluster search functional test with minimun privileges assigned to the test_user (elastic#70007) [Maps] choropleth layer wizard (elastic#69699) Make custom errors by extending Error (elastic#69966) [Ingest Manager] Support updated package output structure (elastic#69864) Resolver test coverage (elastic#70246) Async Discover search test (elastic#64388) [ui-shared-deps] include styled-components (elastic#69322) ... # Conflicts: # x-pack/plugins/snapshot_restore/server/types.ts

…bana into alerting/consumer-based-rbac * 'alerting/consumer-based-rbac' of github.com:gmmorris/kibana: (49 commits) [Discover] Deangularize Skip to bottom button (elastic#69811) Implement recursive plugin discovery (elastic#68811) Use ts-expect-error in platform code (elastic#69883) [SIEM][Detection Engine][Lists] Moves getQueryFilter to common folder for use by both front and backend [Ingest Manager][SECURITY SOLUTION] adjust config reassign link and add roundtrip to Reassignment flow (elastic#70208) [Security][Lists] Add API functions and react hooks for value list APIs (elastic#69603) [ILM] Fix bug when clearing priority field (elastic#70154) [Platform][Security] Updates cluster_manager ignorePaths to include security scripts (elastic#70139) [IngestManager] Allow to filter agent by packages (elastic#69731) [code coverage] exclude folders: test_helpers, tests_bundle (elastic#70199) [Metrics UI] UX improvements for saved views (elastic#69910) [APM] docs: unique transaction troubleshooting (elastic#69831) Cross cluster search functional test with minimun privileges assigned to the test_user (elastic#70007) [Maps] choropleth layer wizard (elastic#69699) Make custom errors by extending Error (elastic#69966) [Ingest Manager] Support updated package output structure (elastic#69864) Resolver test coverage (elastic#70246) Async Discover search test (elastic#64388) [ui-shared-deps] include styled-components (elastic#69322) SECURITY-ENDPOINT: add host properties (elastic#70238) ...

* implements recursive scanning in plugin discovery system * update optimizer to find plugins in sub-directories * update renovate * update optimizer IT snapshot * refactor processPluginSearchPaths$ and add test for inaccessible manifest * add symlink test * add maxDepth to the optimizer * adapt mockFs definitions * remove `flat` usage

implements recursive scanning in plugin discovery system

debfb67

pgayvallet added Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.9.0 v8.0.0 labels Jun 10, 2020

update optimizer to find plugins in sub-directories

8228bcd

pgayvallet commented Jun 10, 2020

View reviewed changes

pgayvallet added 3 commits June 10, 2020 23:02

update renovate

0cf4f90

update optimizer IT snapshot

5645711

Merge remote-tracking branch 'upstream/master' into kbn-59189-recursi…

1a398ca

…ve-plugin-discovery

pgayvallet marked this pull request as ready for review June 11, 2020 08:00

pgayvallet requested review from a team as code owners June 11, 2020 08:00

pgayvallet added the release_note:skip Skip the PR/issue when compiling release notes label Jun 11, 2020

joshdover mentioned this pull request Jun 11, 2020

[GS] add application result provider #68488

Merged

2 tasks

spalger mentioned this pull request Jun 13, 2020

[kbn/optimizer] share all plugin bundles #68986

Merged

pgayvallet added 2 commits June 15, 2020 08:27

Merge remote-tracking branch 'upstream/master' into kbn-59189-recursi…

7f68ba1

…ve-plugin-discovery

Merge remote-tracking branch 'upstream/master' into kbn-59189-recursi…

c709bed

…ve-plugin-discovery

spalger approved these changes Jun 17, 2020

View reviewed changes

joshdover reviewed Jun 18, 2020

View reviewed changes

pgayvallet added 4 commits June 23, 2020 08:04

Merge remote-tracking branch 'upstream/master' into kbn-59189-recursi…

f011c93

…ve-plugin-discovery

refactor processPluginSearchPaths$ and add test for inaccessible mani…

5c00548

…fest

Merge remote-tracking branch 'upstream/master' into kbn-59189-recursi…

3cea30e

…ve-plugin-discovery

add symlink test

fa308bf

joshdover self-requested a review June 25, 2020 13:23

joshdover approved these changes Jun 25, 2020

View reviewed changes

pgayvallet added 4 commits June 26, 2020 08:21

add maxDepth to the optimizer

245cc4f

adapt mockFs definitions

6bc7cc8

Merge remote-tracking branch 'upstream/master' into kbn-59189-recursi…

f25b051

…ve-plugin-discovery

Merge remote-tracking branch 'upstream/master' into kbn-59189-recursi…

c05880a

…ve-plugin-discovery

pgayvallet added 3 commits June 29, 2020 10:19

remove flat usage

9dfbf16

Merge remote-tracking branch 'upstream/master' into kbn-59189-recursi…

9b67058

…ve-plugin-discovery

Merge remote-tracking branch 'upstream/master' into kbn-59189-recursi…

39f2765

…ve-plugin-discovery

pgayvallet merged commit 8485d2f into elastic:master Jun 30, 2020

This was referenced Jun 30, 2020

[7.x] Implement recursive plugin discovery (#68811) #70286

Merged

[Discuss] Ideal domain hierarchy and folder structure #58262

Closed

timroes mentioned this pull request Jul 1, 2020

Nested structure for Kibana App plugins #70452

Closed

joshdover mentioned this pull request Jul 13, 2020

[discuss] Move to domain-based directory structure #71566

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement recursive plugin discovery #68811

Implement recursive plugin discovery #68811

pgayvallet commented Jun 10, 2020 •

edited

Loading

pgayvallet Jun 10, 2020 •

edited

Loading

joshdover Jun 18, 2020

pgayvallet Jun 23, 2020

joshdover Jun 25, 2020

pgayvallet Jun 10, 2020

pgayvallet Jun 10, 2020

joshdover Jun 18, 2020

pgayvallet Jun 23, 2020

joshdover Jun 25, 2020

elasticmachine commented Jun 11, 2020

spalger commented Jun 11, 2020

pgayvallet commented Jun 12, 2020

spalger left a comment

joshdover left a comment

joshdover Jun 18, 2020

joshdover Jun 18, 2020

joshdover Jun 18, 2020

pgayvallet Jun 23, 2020

joshdover Jun 18, 2020

pgayvallet Jun 23, 2020 •

edited

Loading

joshdover Jun 23, 2020

pgayvallet Jun 24, 2020

joshdover left a comment

pgayvallet commented Jun 29, 2020

kibanamachine commented Jun 29, 2020

		...scanDirs.map((dir) => `${dir}/*/kibana.json`),
		...scanDirs.map((dir) => `${dir}/**/kibana.json`),

Implement recursive plugin discovery #68811

Implement recursive plugin discovery #68811

Conversation

pgayvallet commented Jun 10, 2020 • edited Loading

Summary

Checklist

pgayvallet Jun 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Jun 11, 2020

spalger commented Jun 11, 2020

pgayvallet commented Jun 12, 2020

spalger left a comment

Choose a reason for hiding this comment

joshdover left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgayvallet Jun 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshdover left a comment

Choose a reason for hiding this comment

pgayvallet commented Jun 29, 2020

kibanamachine commented Jun 29, 2020

💚 Build Succeeded

Build metrics

History

pgayvallet commented Jun 10, 2020 •

edited

Loading

pgayvallet Jun 10, 2020 •

edited

Loading

pgayvallet Jun 23, 2020 •

edited

Loading