Check plugin version on startup by BigFunger · Pull Request #8283 · elastic/kibana

BigFunger · 2016-09-14T18:44:38Z

Adds a version check to the Kibana startup process.

Applies the same version checks that are applied during the plugin install process (including Adds concept of kibanaVersion to plugin installer #8222 changes)
If the version of the plugin (pack) fails this check, a warning message is displayed in the console and the plugin is disabled.

Added a disable function to the Plugin_Collection class that allows for the disabling (deletion) of plugins

Moves logic that checks configuration to see if a plugin is disabled into its own mixin

I also changed the version value of some test fixtures to 'kibana' instead of '0.0.0'. The version checker will ignore (and not disable) any plugins that have a version number of 'kibana'.

Bargs

I tested this with xpack and ran into some possible issues (or maybe this behavior is intended?). When I changed the version number in the main xpack package.json, I got the warning message as expected. But when I changed the version number of an individual plugin (reporting) it didn't report any errors and did not disabled the plugin.

A couple other general thoughts:

Did you consider putting this logic in the ./plugins/scan module? It seems like that would be a natural place for this version check, so that invalid plugins never get added to the plugin list in the first place.
Beyond the scope of this PR, but I'm just curious if there's been talk of allowing ranges in a plugin's kibana.version

Bargs · 2016-09-15T15:51:40Z

.gitignore

 .htpasswd
 .eslintcache
-plugins
+/plugins


This change promoted me to look more into the .gitignore rules. I'm guessing this rule was mistakenly matching more than just the root level plugins directory? While we're tightening things down, should we make it /plugins/ so that it specifically matches a directory? http://stackoverflow.com/questions/24139478/when-to-use-leading-slash-in-gitignore

I'm all for tightening up this file. I'm guessing we're ignoring more that we intend here. I don't think this PR is the right place to do it, and that it deserves it's own. I modified the plugin ignore setting because it directly affects this PR.

Yup, I only meant this one line, sorry if it sounded like I was implying we change the whole file

Love how I dragged my feet about your suggestion, and then didn't actually apply it. updated.

To clarify, I meant we could change this one line to /plugins/

Bargs · 2016-09-15T15:52:51Z

src/server/plugins/check_version.js

@@ -0,0 +1,34 @@
+import pluginInit from './plugin_init';


This looks to be an unused import

right you are, Ken

Bargs · 2016-09-15T15:53:21Z

src/server/plugins/check_version.js

+import { cleanVersion, versionSatisfies } from '../../utils/version';
+import _ from 'lodash';
+
+module.exports = async function (kbnServer, server, config) {


Use ES6 export syntax?

Bargs · 2016-09-15T15:59:57Z

src/server/plugins/check_version.js

+    const version = _.get(plugin, 'pkg.kibana.version', _.get(plugin, 'pkg.version'));
+    const name = _.get(plugin, 'pkg.name');
+
+    if (version === 'kibana') continue;


Using continue can make loops really difficult to reason about. Why not include this bit of logic in versionSatisfies? Then any calling code won't have to worry about this special condition.

Since both the plugin install process and this check use the version.js file, I don't want to move the check into versionSatisfies. I'll clean it up in check_version.js

Bargs · 2016-09-15T16:03:37Z

src/server/plugins/check_version.js

+      cleanVersion(version),
+      cleanVersion(kbnServer.version))) {
+      warningMessages.add(`Plugin "${name}" expected Kibana version "${version}" and was disabled.`);
+      plugins.delete(plugin);


The indentation here is really hard for me to read. Would something like this be better?

if (!versionSatisfies( cleanVersion(version), cleanVersion(kbnServer.version))) { warningMessages.add(`Plugin "${name}" expected Kibana version "${version}" and was disabled.`); plugins.delete(plugin); }

Bargs · 2016-09-15T16:05:04Z

src/server/plugins/check_version.js

+    if (!versionSatisfies(
+      cleanVersion(version),
+      cleanVersion(kbnServer.version))) {
+      warningMessages.add(`Plugin "${name}" expected Kibana version "${version}" and was disabled.`);


Maybe include the actual Kibana version in the error message, just to users don't have to go digging for it?

I thought about it, but was worried that it would be too wordy.

Plugin "${name}" was disabled because it expected Kibana version "${version}", and found "${kbnServer.version}". ?

Bargs · 2016-09-15T16:15:36Z

src/server/plugins/check_version.js

+  }
+
+  //because a plugin pack can contain more than one actual plugin, (for example x-pack)
+  //we make sure that the warning messages are unique


I'm not really sure what this comment means. The code below doesn't seem to do any de-duping.

I'm using a Set() object to collect the warning messages. I'll move the comment to the variable declaration.

BigFunger · 2016-09-15T16:51:30Z

I tested this with xpack and ran into some possible issues (or maybe this behavior is intended?)

Correct. It only warns once. x-pack is installed as a collection of plugins. The root package.json file describes the version of the pack as a whole, and is therefor what I should be checking, IMO. @w33ble, @tsullivan can you chime in on this?

Did you consider putting this logic in the ./plugins/scan module?

I did, and decided that putting it into it's own check after scanning all of the plugins was a cleaner way of doing it. @spalger, what do you think?

Beyond the scope of this PR, but I'm just curious if there's been talk of allowing ranges in a plugin's kibana.version

At the moment, it is intentionally a direct match. That is one of the reasons I have this in another module though, because it should make it easier to implement once we widen that version range.

Bargs · 2016-09-15T17:27:40Z

Changes look great!

I guess at the moment the only reason a plugin would have a different version than its pack is due to a build error. If someone has a broken pack, it seems like we should warn them and disable the entire pack? If @tsullivan and @w33ble are cool with it as is though, it's cool with me.

Same goes for moving the logic into the scan module, if @spalger thinks it is appropriate where it is currently, that's fine with me. I like the idea of keeping more of the plugin logic in one place, but I don't feel super strongly about it.

spalger

one question, but LGTM

spalger · 2016-09-15T18:29:48Z

src/server/plugins/check_version.js

+  }
+
+  for (let message of warningMessages) {
+    server.log(['warning'], message);


Why not just log this in the loop above?

for multi-plugin packs, I'm checking the version of the pack as a whole, but there will be one plugin object for each plugin in the pack. I only want to show one message for the overall pack.

spalger · 2016-09-15T18:53:17Z

src/server/plugins/check_version.js

+    if (!compatibleWithKibana(kbnServer, version)) {
+      const message = `Plugin "${name}" was disabled because it expected Kibana version "${version}", and found "${kbnServer.version}".`;
+      warningMessages.add(message);
+      plugins.delete(plugin);


Now that @Bargs has pointed it out, I took a deeper look and discovered that Plugin#readConfig() is doing a very different job that it should. Removing the config schema of the plugin from the config service is something that should also happen when a plugin is disabled.

tsullivan · 2016-09-15T19:01:07Z

when I changed the version number of an individual plugin (reporting) it didn't report any errors and did not disabled the plugin

I don't really think this is a use case we need to support. Only Elastic is responsible for generating the X-Pack builds, and the code only refers to the main package.json, and it only does that for generating the build artifact.

To be honest, I'm not really sure why we even have the x-plugins/kibana/plugins/*/package.json files

spalger · 2016-09-15T19:03:43Z

src/server/plugins/check_version.js

+    // the version of kibana down to the patch level. If these two versions need
+    // to diverge, they can specify a kibana.version to indicate the version of
+    // kibana the plugin is intended to work with.
+    const version = get(plugin, 'pkg.kibana.version', get(plugin, 'pkg.version'));


plugin objects have a version property that is already resolved correctly, we should use it here.

get(plugin, 'pkg.kibana.version', get(plugin, 'version')) or should I initialize the kibana version in the plugin class as well?

…llection

w33ble · 2016-09-15T21:36:52Z

But when I changed the version number of an individual plugin (reporting) it didn't report any errors and did not disabled the plugin.

IMO, that's how it should work. Correct me if I'm wrong, but I'm pretty sure the top-level "pack" version is the only version that's required. If there are other "plugins" bundled in the "pack", they aren't required to have a package.json file, so we shouldn't be checking them even if they exist.

I guess at the moment the only reason a plugin would have a different version than its pack is due to a build error.

Or the internals are developed differently and their versions aren't in line with the pack's. But that's really up to the pack author to keep straight then.

If someone has a broken pack, it seems like we should warn them and disable the entire pack?

This I agree with, but only if you are defining "broken" as "the top level version doesn't pass the test." If that's the case, then yes, the whole pack should be disabled.

spalger · 2016-09-15T22:34:12Z

src/server/plugins/plugin.js

+    return schema || defaultConfigSchema;
+  }
+
+  get enabled() {


Let's just use a function, getters for this type of stuff can be deceptive.

spalger · 2016-09-15T22:46:40Z

src/server/plugins/plugin_collection.js


-      throw new TypeError('unexpected plugin export ' + inspect(product));
+      await this.add(plugin);
+      if (!plugin.enabled) this.delete(plugin);


It's really unclear why these two lines are necessary, I think it would be clearer if we added and removed the configuration from the config schema right here.

spalger · 2016-09-15T23:10:22Z

idk how reviews work yet

…e a plugin

spalger · 2016-09-16T18:17:29Z

LGTM!

spalger · 2016-09-16T18:17:43Z

@Bargs can you take another look?

Bargs · 2016-09-16T23:17:49Z

LGTM

--------- **Commit 1:** fixes plugin path in .gitignore * Original sha: f74eb9b * Authored by Jim Unger <bigfunger@gmail.com> on 2016-09-14T18:40:21Z **Commit 2:** Moves version from plugin installer to utils * Original sha: ae492ff * Authored by Jim Unger <bigfunger@gmail.com> on 2016-09-14T18:41:19Z **Commit 3:** Adds plugin version check to kibana startup * Original sha: 83d0821 * Authored by Jim Unger <bigfunger@gmail.com> on 2016-09-14T18:41:40Z **Commit 4:** Changes plugin version to 'kibana' in text fixture * Original sha: 922c04a * Authored by Jim Unger <bigfunger@gmail.com> on 2016-09-14T19:07:09Z **Commit 5:** Merge branch 'master' into check-plugin-version-on-startup * Original sha: 5da33ad * Authored by Jim Unger <bigfunger@gmail.com> on 2016-09-15T14:45:01Z **Commit 6:** review changes to check_version * Original sha: 2802410 * Authored by Jim Unger <bigfunger@gmail.com> on 2016-09-15T16:53:28Z **Commit 7:** reworked logic to remove config when deleting a plugin from plugin_collection * Original sha: 2f52be6 * Authored by Jim Unger <bigfunger@gmail.com> on 2016-09-15T20:11:43Z **Commit 8:** Adds a kibanaVersion property to the Plugin class * Original sha: e920bca * Authored by Jim Unger <bigfunger@gmail.com> on 2016-09-15T21:04:41Z **Commit 9:** move enabled check into it's own mixin, and cleaned up how you disable a plugin * Original sha: 049c029 * Authored by Jim Unger <bigfunger@gmail.com> on 2016-09-16T17:22:47Z

[backport] PR #8283 to 5.0

[backport] PR #8283 to 5.x

--------- **Commit 1:** fixes plugin path in .gitignore * Original sha: 8d8745a2d68838777929c5859dd20fbb640e9abd [formerly f74eb9b] * Authored by Jim Unger <bigfunger@gmail.com> on 2016-09-14T18:40:21Z **Commit 2:** Moves version from plugin installer to utils * Original sha: 53906f1dadbfefc5e0f0235908611e1b6bee382e [formerly ae492ff] * Authored by Jim Unger <bigfunger@gmail.com> on 2016-09-14T18:41:19Z **Commit 3:** Adds plugin version check to kibana startup * Original sha: b7cec7ca8a688d7b1eeb13aef6d49983ff2d6010 [formerly 83d0821] * Authored by Jim Unger <bigfunger@gmail.com> on 2016-09-14T18:41:40Z **Commit 4:** Changes plugin version to 'kibana' in text fixture * Original sha: 898642602f305bf93b71b76d62dba68c200230eb [formerly 922c04a] * Authored by Jim Unger <bigfunger@gmail.com> on 2016-09-14T19:07:09Z **Commit 5:** Merge branch 'master' into check-plugin-version-on-startup * Original sha: cbbfc82b5ced04c374ffbc88b06c12ba0a2ee557 [formerly 5da33ad] * Authored by Jim Unger <bigfunger@gmail.com> on 2016-09-15T14:45:01Z **Commit 6:** review changes to check_version * Original sha: 6e57b59fae033d18c7a99ede8978f2b873e7d8f4 [formerly 2802410] * Authored by Jim Unger <bigfunger@gmail.com> on 2016-09-15T16:53:28Z **Commit 7:** reworked logic to remove config when deleting a plugin from plugin_collection * Original sha: b1f1f6833ae58abad301cb3eb02918486d733d20 [formerly 2f52be6] * Authored by Jim Unger <bigfunger@gmail.com> on 2016-09-15T20:11:43Z **Commit 8:** Adds a kibanaVersion property to the Plugin class * Original sha: 7da9b40fabddbf787e340f515ada2fd3d6d29408 [formerly e920bca] * Authored by Jim Unger <bigfunger@gmail.com> on 2016-09-15T21:04:41Z **Commit 9:** move enabled check into it's own mixin, and cleaned up how you disable a plugin * Original sha: 2136468d85de1f1a40db1f2e18275a468f4ec4ae [formerly 049c029] * Authored by Jim Unger <bigfunger@gmail.com> on 2016-09-16T17:22:47Z Former-commit-id: 246b069

[backport] PR elastic#8283 to 5.x Former-commit-id: 97960b0

BigFunger added 3 commits September 14, 2016 13:40

fixes plugin path in .gitignore

f74eb9b

Moves version from plugin installer to utils

ae492ff

Adds plugin version check to kibana startup

83d0821

BigFunger added the review label Sep 14, 2016

spalger self-assigned this Sep 14, 2016

BigFunger force-pushed the check-plugin-version-on-startup branch from bd08c6c to 92f1b90 Compare September 14, 2016 19:31

BigFunger assigned Bargs Sep 15, 2016

Changes plugin version to 'kibana' in text fixture

922c04a

BigFunger force-pushed the check-plugin-version-on-startup branch from 92f1b90 to 922c04a Compare September 15, 2016 14:38

Merge branch 'master' into check-plugin-version-on-startup

5da33ad

Bargs suggested changes Sep 15, 2016

View reviewed changes

review changes to check_version

2802410

BigFunger force-pushed the check-plugin-version-on-startup branch from e88ecca to 2802410 Compare September 15, 2016 17:05

Bargs approved these changes Sep 15, 2016

View reviewed changes

spalger approved these changes Sep 15, 2016

View reviewed changes

spalger suggested changes Sep 15, 2016

View reviewed changes

BigFunger added 2 commits September 15, 2016 15:11

reworked logic to remove config when deleting a plugin from plugin_co…

2f52be6

…llection

Adds a kibanaVersion property to the Plugin class

e920bca

spalger suggested changes Sep 15, 2016

View reviewed changes

spalger approved these changes Sep 15, 2016

View reviewed changes

move enabled check into it's own mixin, and cleaned up how you disabl…

049c029

…e a plugin

Bargs assigned BigFunger Sep 16, 2016

BigFunger merged commit 3b03ec3 into elastic:master Sep 19, 2016

This was referenced Sep 19, 2016

[backport] PR #8283 to 5.0 - Check plugin version on startup #8334

Merged

[backport] PR #8283 to 5.x - Check plugin version on startup #8335

Merged

BigFunger added a commit that referenced this pull request Sep 19, 2016

Merge pull request #8334 from elastic/jasper/backport/8283/5.0

c4f6c2d

[backport] PR #8283 to 5.0

BigFunger added a commit that referenced this pull request Sep 19, 2016

Merge pull request #8335 from elastic/jasper/backport/8283/5.x

97960b0

[backport] PR #8283 to 5.x

epixa added the v5.0.0-beta1 label Sep 19, 2016

tylersmalley mentioned this pull request Oct 10, 2016

Explicitly ignore plugin directories #8316

Merged

epixa added the v5.0.0 label Oct 26, 2016

airow pushed a commit to airow/kibana that referenced this pull request Feb 16, 2017

Merge pull request elastic#8335 from elastic/jasper/backport/8283/5.x

0217d52

[backport] PR elastic#8283 to 5.x Former-commit-id: 97960b0

Comments

Conversation

BigFunger commented Sep 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bargs left a comment

Choose a reason for hiding this comment

Uh oh!

Bargs Sep 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BigFunger Sep 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BigFunger commented Sep 15, 2016

Uh oh!

Bargs commented Sep 15, 2016

Uh oh!

spalger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsullivan commented Sep 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

w33ble commented Sep 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spalger commented Sep 15, 2016

Uh oh!

spalger commented Sep 16, 2016

Uh oh!

spalger commented Sep 16, 2016

Uh oh!

Bargs commented Sep 16, 2016

BigFunger commented Sep 14, 2016 •

edited

Loading

Bargs Sep 15, 2016 •

edited

Loading

BigFunger Sep 15, 2016 •

edited

Loading