Use 'name' from plugin descriptor file to determine plugin name #12775

cbuescher · 2015-08-10T15:35:06Z

At the moment, when installing from an url, a user provides a plugin name on the command line like:

bin/plugin install [plugin-name] --url [url]

This can lead to confusion when picking an already existing name from another plugin, which might even overwrite plugins already installed with that name.

In order to remedy this, this PR introduces an additional mandatory name property to the plugin descriptor file which overwrites the name provided by the user after the plugin was loaded and the descriptor file is unpacked.

Relates to #12715

cbuescher · 2015-08-10T15:40:09Z

Some open questions I have with the original issue which are not addressed by this PR so far:

What should happen with the [plugin-name] CLI argument. We can't remove it, since it is used and needed when no url is provided. As far as I understand, in this case the name is going to be resolved from the CLI argument depending on various rules.
When the name is specified in the descriptor file, I think that also that name needs to be used to remove the plugin. Not sure how much this is in conflict with existing behaviour

dadoonet · 2015-08-10T16:06:15Z

core/src/main/java/org/elasticsearch/plugins/PluginInfo.java

+        String pluginName = props.getProperty("plugin.name");
+        if (pluginName == null) {
+            throw new IllegalArgumentException("Property [plugin.name] is missing for plugin [" + name + "]");
+        }


I'm wondering if in that case we should not fallback to the previous convention (use zip name as the plugin name) and just "warn" the user?

Yes, I wasn't too sure about the original issues goals here, so I didn't elaborate any further.

I think the whole point of this PR is to add this new parameter and make it required, so I don't think we should fall back? Otherwise the leniency that we are trying to fix is still here.

In particular, since we now require that plugins have exactly the same version as elasticsearch, plugins will have to be re-packaged anyway, so I don't think bw compat is a concern?

Not a big deal. I was just thinking of community plugin developers but it's not hard for them to fix. We should may be add this in BWC doc.

We should also check the name matches that in jvm plugins. As a follow up, we should at least remove description from jvm Plugin interface, and possibly also name (possibly a little harder, just requires passing around PluginInfo instead of Plugin I think).

Actually I just checked and removing name and description from the Plugin interface should be easy. The only thing to think about is what to give for those properties when plugins are loaded from the classpath (plugin.types). I think here the name should just be the classname, and description something like "plugin loaded from classpath"? I don't know what other info we really have.

@rmuir added element to the enforcer plugin, but I'm not sure what check to insert in pluginservice and how that service actually works. Can this be added in a follow up PR?

@rjernst Ryan, not sure what you mean by jvm plugins and the checks your suggested, can this be done outside of this PR? Otherwise where do I need to look?

@rjernst since you already seem to have some additional checks in mind, could you open that in a follow up issue or let me know what really needs to be done in this PR.

dadoonet · 2015-08-10T16:34:01Z

Some thoughts that come to mind.

Now we know that if the user set a url like:

bin/plugin install foo --url http://link.to/myplugin.zip

And that myplugin.zip contains in its descriptor a plugin.name equal to bar, we will end up installing bar plugin.

So it does not make sense anymore to support this --url option IMO.

We should have:

# download from elastic.co, maven central or github
bin/plugin install foo
# download from a URI (URL)
bin/plugin install http://link.to/foo.zip
# download from a URI (file)
bin/plugin install file:.target/releases/foo.zip

@spinscale @clintongormley WDYT?

rjernst · 2015-08-10T18:29:36Z

Hrm, @dadoonet Can we be explicit instead of having to infer what was passed as the argument to install? What you suggest would require even more heuristics. Instead, why don't we remove the fallback to trying as a url, change http:// to require using --url, and add a --file that requires a local path?

rjernst · 2015-08-10T18:33:11Z

core/src/main/java/org/elasticsearch/plugins/PluginInfo.java

@@ -95,6 +95,10 @@ public static PluginInfo readFromProperties(Path dir) throws IOException {
        if (version == null) {
            throw new IllegalArgumentException("Property [version] is missing for plugin [" + name + "]");
        }
+        String pluginName = props.getProperty("plugin.name");


Hrm, can we just call this "name"? None of the other settings are prefixed with plugin.

Sure, changed the property to just 'name'.

rjernst · 2015-08-10T18:35:18Z

Also, when we download a plugin, if it was downloaded by name, we should verify the name matches?

clintongormley · 2015-08-11T15:46:04Z

I agree with being explicit:

# download from elastic.co, maven central or github
bin/plugin install foo
# download from a URI (URL)
bin/plugin install --url http://link.to/foo.zip
# download from a URI (file)
bin/plugin install --file .target/releases/foo.zip

dadoonet · 2015-08-11T16:04:37Z

@spinscale Does CLI allows that?

I think we might need to do something like:

# download from elastic.co, maven central or github
bin/plugin install foo
# download from a URI (URL)
bin/plugin install http://link.to/foo.zip --url 
# download from a URI (file)
bin/plugin install .target/releases/foo.zip --file

spinscale · 2015-08-11T16:06:30Z

the last two syntaxes are really confusing IMO, one can change to allow for that, but it doesnt feel nice

clintongormley · 2015-08-11T16:18:50Z

If we can't easily support the version in #12775 (comment) then let's use the version in #12775 (comment)

The heuristic would simply be: anything matching this regex is a URL ^\w+:

rjernst · 2015-08-11T17:19:17Z

Wasn't @spinscale talking about the odd syntax of adding options after the thing the option is pointing to? Surely having command line parameter that takes an argument is supported by the CliTool? We should simply not use heuristics; they are not needed, and only lead to confusing errors.

clintongormley · 2015-08-11T17:22:13Z

@rjernst yes he was. @dadoonet was not sure if the CLI parser we use would support that style of argument parsing. I'm saying that, if it doesn't (and is not easy to fix) then we should rather use the simple heuristic than the confusing syntax.

But I agree, I can't see why the CLI parser wouldn't support it.

spinscale · 2015-08-12T08:26:00Z

Looks good me to regarding support... One last thing though:

# downloads official plugin
bin/plugin install analysis-kuromoji 
# downloads github plugin
bin/plugin install lmenezes/elasticsearch-kopf
# URL
bin/plugin install http://link.to/foo.zip
bin/plugin install file://.target/releases/foo.zip
# local
bin/plugin install --file .target/releases/foo.zip

Do we really need a --url and --file switch? How about just providing an URL by default, which also works if you provide file://. As we try to remove leniency, rather than add it at the moment, we could still have the --file switch, which automatically prepends file:// to the specified path to create an URI instead of trying to be smart and fall back to do this automatically if parsing as an URL fails.

cbuescher · 2015-08-12T08:42:57Z

@spinscale Thanks for that suggestions, looking at the implementation details and given that -u currently also covers installation from file I'd also prefer not adding yet another switch. Will try this first without the --file switch.

rjernst · 2015-08-12T22:06:07Z

@spinscale I like this approach. In this case we shoudln't provide --file at all (we didn't before right? we only had --url?).

So to confirm, the logic would be:

if (argument-parses-as-uri) {
  ...use URI
} else {
  ...use as name with download service
}

rjernst · 2015-08-12T22:07:01Z

But that means we should remove --url too, otherwise we still would have the issue of the user passing a name with a url.

spinscale · 2015-08-13T06:35:43Z

@rjernst depening on our impl, logic would be

if (argument-parses-as-uri) {
  ...use URI
} else {
   if (argument-parses-as-uri-with-file://-prefix) {
  } else { 
    ...use as name with download service
  }
}

and yes, we could totally drop the --url in that case and just have a single argument, the identifier, being a name, local path or URL

rjernst · 2015-08-13T06:37:54Z

Why would the inner if be necessary? file:// should parse as a uri right?

spinscale · 2015-08-13T07:21:09Z

@rjernst sure, but you might want to use paths like /my/path/to/my/plugin.zip without specifying file:// in front

rjernst · 2015-08-13T07:24:32Z

We should not do that. That is leniency. Using a local file is expert, let's just stick to a uri, and not have to deal with is-this-thing-a-file-or-should-we-try-the-download-service

rjernst · 2015-08-13T07:25:44Z

It's also no different than what users already had to do, using --url. We can just remove --url, since we can always clearly tell if something is a uri.

spinscale · 2015-08-13T07:26:06Z

valid argument, +1

nik9000 · 2015-08-17T16:58:28Z

core/src/main/java/org/elasticsearch/plugins/PluginManagerCliParser.java

+                pluginUrl = new URL(name);
+                name = null;
+            } catch (MalformedURLException e) {
+                // ignore


This seems strange - maybe explain why its ok to ignore this here?

If we do plugin install [nameOrUrl] and the argument doesn't parse to a valid url, we go on treating it as on of the other three options of a plugin name (e.g. analysis-icu, user/repo etc...). I can update the comment to make this clearer.

Ah ok - yeah. Please update the comment. It'll help people just scanning the code.

dadoonet · 2015-08-18T09:42:15Z

@cbuescher I think this file needs also to be updated with your change: https://github.com/elastic/elasticsearch/blob/master/docs/plugins/plugin-script.asciidoc#custom-url-or-file-system

cbuescher · 2015-08-18T13:01:55Z

Updated the PR, rebased and added comments, changes in logging suggested by @nik9000 and changed docs.

dadoonet · 2015-08-18T13:13:00Z

core/src/main/resources/org/elasticsearch/plugins/plugin-install.help


    plugin install elasticsearch/shield/latest

    plugin install lmenezes/elasticsearch-kopf

+    plugin install http://download.elasticsearch.org/elasticsearch/elasticsearch-analysis-kuromoji/elasticsearch-analysis-kuromoji-2.7.0.zip
+
+    plugin install file:///path/to/plugin/elasticsearch-analysis-kuromoji-2.7.0.zip


Sometimes non java devs are confused by that and I don't think // is required. May be this?

plugin install file:/path/to/plugin/elasticsearch-analysis-kuromoji-2.7.0.zip or plugin install file:./path/to/plugin/elasticsearch-analysis-kuromoji-2.7.0.zip

Or may be adding a windows user example like this (not tested) ?

plugin install file:c:/path/to/plugin/elasticsearch-analysis-kuromoji-2.7.0.zip

Thanks for the suggestions, will test this.

dadoonet · 2015-08-18T13:15:10Z

I left a small comment but the change looks good to me.

dadoonet · 2015-08-18T13:38:01Z

@cbuescher I merged #12879 so you can now change pom.xml files to use maven artifactId instead of setting manually elasticsearch.plugin.name.

cbuescher · 2015-08-18T17:04:49Z

@dadoonet thanks, I updated the poms using the artifactId instead of hardcoding the name, added some documentation and rebased. I'll sqash this again before merging with master.

dadoonet · 2015-08-18T17:08:55Z

I left one small comment.

I think we are really close now!

cbuescher · 2015-08-18T17:47:54Z

@dadoonet thanks for the review, didn't know artifactId would be resolved in the submodules if placeholder is defined in the parent pom, but seems to work. I updated the PR accordingly.

dadoonet · 2015-08-18T17:53:47Z

LGTM!

…lugin name At the moment, when installing from an url, a user provides the plugin name on the command line like: * bin/plugin install [plugin-name] --url [url] This can lead to problems when picking an already existing name from another plugin, and can potentially overwrite plugins already installed with that name. This, this PR introduces a mandatory `name` property to the plugin descriptor file which replaces the name formerly provided by the user. With the addition of the `name` property to the plugin descriptor file, the user does not need to specify the plugin name any longer when installing from a file or url. Because of this, all arguments to `plugin install` command are now either treated as a symbolic name, a URL or a file without the need to specify this with an explicit option. The new syntax for `plugin install` is now: bin/plugin install [name or url] * downloads official plugin bin/plugin install analysis-kuromoji * downloads github plugin bin/plugin install lmenezes/elasticsearch-kopf * install from URL or file bin/plugin install http://link.to/foo.zip bin/plugin install file:/path/to/foo.zip If the argument does not parse to a valid URL, it is assumed to be a name and the download location is resolved like before. Regardless of the source location of the plugin, it is extracted to a temporary directory and the `name` property from the descriptor file is used to determine the final install location. Relates to elastic#12715

cbuescher · 2015-08-19T10:32:12Z

@dadoonet Rebased and squashed, will merge this to master so it can be tested there before we might move this to the branch.

Use 'name' from plugin descriptor file to determine plugin name

cbuescher · 2015-08-20T09:30:49Z

Also backported to 2.0 branch with d236192

cbuescher · 2015-08-20T09:33:25Z

@clintongormley should this be labeled breaking since it affects the CLI syntax and adds mandatory properties to the plugin descriptor file?

nik9000 · 2015-08-24T20:29:26Z

Update #12768 ?

cbuescher added review :Core/Infra/Plugins Plugin API and infrastructure labels Aug 10, 2015

cbuescher mentioned this pull request Aug 10, 2015

Add "name" to the plugin descriptor file #12715

Closed

dadoonet reviewed Aug 10, 2015
View reviewed changes

rjernst reviewed Aug 10, 2015
View reviewed changes

cbuescher force-pushed the fix/12715 branch from fdf1ee6 to 3d3c182 Compare August 11, 2015 09:24

clintongormley changed the title ~~Use 'plugin.name' from plugin descriptor file to determin plugin name~~ Use 'plugin.name' from plugin descriptor file to determine plugin name Aug 11, 2015

cbuescher changed the title ~~Use 'plugin.name' from plugin descriptor file to determine plugin name~~ Use 'name' from plugin descriptor file to determine plugin name Aug 11, 2015

clintongormley mentioned this pull request Aug 13, 2015

Testing and packaging changes for 2.0 #12768

Closed

16 tasks

cbuescher force-pushed the fix/12715 branch from 3d3c182 to f6d163c Compare August 13, 2015 18:09

nik9000 reviewed Aug 17, 2015
View reviewed changes

cbuescher force-pushed the fix/12715 branch from 4e3e315 to 604c69a Compare August 18, 2015 12:59

dadoonet reviewed Aug 18, 2015
View reviewed changes

cbuescher force-pushed the fix/12715 branch from 604c69a to 53ce25a Compare August 18, 2015 16:54

cbuescher added v2.1.0 v2.0.0 labels Aug 18, 2015

cbuescher force-pushed the fix/12715 branch from 1af2235 to 8454d49 Compare August 19, 2015 10:30

cbuescher removed the v2.0.0 label Aug 19, 2015

cbuescher added a commit that referenced this pull request Aug 19, 2015

Merge pull request #12775 from cbuescher/fix/12715

4410287

Use 'name' from plugin descriptor file to determine plugin name

cbuescher merged commit 4410287 into elastic:master Aug 19, 2015

cbuescher added the v2.0.0 label Aug 20, 2015

colings86 added v2.0.0-beta1 and removed v2.0.0 labels Aug 21, 2015

clintongormley added the >breaking label Aug 24, 2015

clintongormley removed the v2.1.0 label Nov 22, 2015

cbuescher deleted the fix/12715 branch March 31, 2016 09:27

Use 'name' from plugin descriptor file to determine plugin name #12775

Use 'name' from plugin descriptor file to determine plugin name #12775

Conversation

cbuescher commented Aug 10, 2015

cbuescher commented Aug 10, 2015

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

Choose a reason for hiding this comment

dadoonet commented Aug 10, 2015

rjernst commented Aug 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjernst commented Aug 10, 2015

clintongormley commented Aug 11, 2015

dadoonet commented Aug 11, 2015

spinscale commented Aug 11, 2015

clintongormley commented Aug 11, 2015

rjernst commented Aug 11, 2015

clintongormley commented Aug 11, 2015

spinscale commented Aug 12, 2015

cbuescher commented Aug 12, 2015

rjernst commented Aug 12, 2015

rjernst commented Aug 12, 2015

spinscale commented Aug 13, 2015

rjernst commented Aug 13, 2015

spinscale commented Aug 13, 2015

rjernst commented Aug 13, 2015

rjernst commented Aug 13, 2015

spinscale commented Aug 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dadoonet commented Aug 18, 2015

cbuescher commented Aug 18, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dadoonet commented Aug 18, 2015

dadoonet commented Aug 18, 2015

cbuescher commented Aug 18, 2015

dadoonet commented Aug 18, 2015

cbuescher commented Aug 18, 2015

dadoonet commented Aug 18, 2015

cbuescher commented Aug 19, 2015

cbuescher commented Aug 20, 2015

cbuescher commented Aug 20, 2015

nik9000 commented Aug 24, 2015