Skip to content

Conversation

@rjernst
Copy link
Member

@rjernst rjernst commented Dec 4, 2015

With the work to lock down permissions of scripts, we removed expressions and groovy from core, into plugins. However, we would still like the ability to include these languages, and future extractions of features to plugins, in distributions of elasticsearch.

This change adds the concept of a "module" to the elasticsearch repository. These are simply plugins that come included with distributions, but in a way that does not affect plugin management. They cannot be removed, or even show up, with bin/plugin. Also added here is a new distribution, integ-test-zip. This is the minimal distribution that includes no modules, which plugins use for their integ tests. QA tests use the full zip as their distribution.

Note that we will attempt to backport this to 2.x, but it will be done in a separate PR.

closes #15233

@rjernst rjernst added :Delivery/Build Build or test infrastructure v5.0.0-alpha1 labels Dec 4, 2015
@rjernst rjernst changed the title Build: Add modules to distributions, and move lang-expression and lang-groovy to them Add modules to distributions, and move lang-expression and lang-groovy to them Dec 4, 2015
Copy link
Contributor

Choose a reason for hiding this comment

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

Small typo.

Copy link
Contributor

Choose a reason for hiding this comment

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

But quite a funny one :)

@dadoonet
Copy link
Contributor

dadoonet commented Dec 4, 2015

I gave a first look and it looks really great.

Some thoughts:

  • we can have users who already installed groovy plugin in 2.0 or 2.1. We need to either detect that and fail starting elasticsearch in that case, asking them to remove the plugin.
  • And/or we need to document it in breaking changes
  • we need to adapt the documentation (move from docs/plugins dir to docs/modules?) That could obviously come within another PR. cc @clintongormley

@rjernst
Copy link
Member Author

rjernst commented Dec 4, 2015

Thanks for looking @dadoonet.

we can have users who already installed groovy plugin in 2.0 or 2.1. We need to either detect that and fail starting elasticsearch in that case, asking them to remove the plugin.

This is not true. We did not move groovy to a plugin until after 2.1. There has not been any release of 2.0 or 2.1 that did not include groovy and expressions.

@dadoonet
Copy link
Contributor

dadoonet commented Dec 4, 2015

@rjernst This is not true.

You are 100% right!

@rmuir
Copy link
Contributor

rmuir commented Dec 4, 2015

  • If someone installed the groovy plugin from 2.0 or 2.1, we will already fail because it is for the wrong elasticsearch version.
  • That is not a breaking change because that behavior exists already, this PR does not change the situation. We don't start with old plugins.
  • I don't think we need to document modules. The big picture here is that after discussion with @nik9000 , we realized the most important goal is for packaging to differentiate modules, which do not require any user interaction and are packaging's responsibility from plugins, which require interaction with the user. This allows us to not cause pain in the upgrade experience: we are using the plugin "format" and mechanism as an implementation detail at the moment: it could change in the future to be something else (such as new features provided by java 9). Plugins are currently problematic (e.g. we don't yet have distribution packages for them and upgrades are less than stellar), so a "transparent" solution would introduce problems: but modules don't need to have these problems: they are system code just like lib/ and we don't document that directory. If we want to document these directories then i can only recommend NO TOUCHY.

@rjernst
Copy link
Member Author

rjernst commented Dec 4, 2015

we can have users who already installed groovy plugin in 2.0 or 2.1. We need to either detect that and fail starting elasticsearch in that case, asking them to remove the plugin.

This is not true. We did not move groovy to a plugin until after 2.1. There has not been any release of 2.0 or 2.1 that did not include groovy and expressions.

However, we can and should improve the error messaging. In 3.0 (this change) we won't be publishing zips for modules. But with the backport to 2.x, we have no choice with maven (I think?). So for 2.x, we should look at having probably a static list to throw an error on if the user tries installing the plugin.

@rmuir
Copy link
Contributor

rmuir commented Dec 4, 2015

There is already a check in PluginManager in this PR for that case.

@rjernst
Copy link
Member Author

rjernst commented Dec 4, 2015

So for 2.x, we should look at having probably a static list to throw an error on if the user tries installing the plugin.

There is already a check in PluginManager in this PR for that case.

Ah yes I forgot!

@dadoonet
Copy link
Contributor

dadoonet commented Dec 4, 2015

we need to adapt the documentation (move from docs/plugins dir to docs/modules?)

I was wrong. You can totally ignore my comment apart that is great! :D

@rmuir
Copy link
Contributor

rmuir commented Dec 4, 2015

I also want to mention the downsides of this approach (versus just preinstalling plugins):

  • there isn't yet really supported way to remove modules. we just want to be able to provide more isolation, reliability, security to the system, and remove core third party dependencies. But we don't offer this capability today, and the workaround is to use the rm command, still easier than removing groovy scripting today.
  • we may have confusion caused by the fact that, lang-groovy and lang-expressions may exist in maven central as published plugins for the 2.x series. Keep in mind they are never documented as being plugins, so its not the end of the world, and its temporary: I think this is simply unavoidable with the maven backport if we want to be practical about things.
  • we have a little more complexity (code changes in plugin code here), than if we just did the straightforward transparent approach of simply preinstalling plugins. But its not too terrible, we only special case enough so that we can report back minimal information in startup logs and nodes api, so things can be debuggable. Otherwise its all under the hood still.

IMO these are minor compared to the advantages we get of better packaging and upgrade behavior.

@bleskes
Copy link
Contributor

bleskes commented Dec 4, 2015

Thanks @rmuir . I was thinking about those downsides as well. Another use case which I thing is valid, is something wanting to have no groovy installed at all. Just doesn't need scripting and doesn't want to have any overhead/potential security risk. If understand things correctly, this will be impossible with the current suggestion.

What are the downsides of the "just preinstalled plugins" approach?

@rjernst
Copy link
Member Author

rjernst commented Dec 4, 2015

something wanting to have no groovy installed at all. Just doesn't need scripting and doesn't want to have any overhead/potential security risk. If understand things correctly, this will be impossible with the current suggestion.

It is possible, just not using bin/plugin. The user just deletes the directory from modules. But this is a super advanced use case, so I think having to manually remove a directory (like manually removing an optional dependency in lib that ES includes) is fine.

@rjernst
Copy link
Member Author

rjernst commented Dec 4, 2015

What are the downsides of the "just preinstalled plugins" approach?

I think this was already mentioned in comments above?

The big picture here is that after discussion with @nik9000 , we realized the most important goal is for packaging to differentiate modules, which do not require any user interaction and are packaging's responsibility from plugins, which require interaction with the user. This allows us to not cause pain in the upgrade experience

@rmuir
Copy link
Contributor

rmuir commented Dec 4, 2015

Thanks @rmuir . I was thinking about those downsides as well. Another use case which I thing is valid, is something wanting to have no groovy installed at all. Just doesn't need scripting and doesn't want to have any overhead/potential security risk. If understand things correctly, this will be impossible with the current suggestion.

its not impossible, they have to use rm. But the real bug is us shipping such an insecure thing by default IMO. I am intentionally steering clear of this problem on this issue, its not related, we don't offer this capability today, and @jdconrad has been working on the right solution.

What are the downsides of the "just preinstalled plugins" approach?

As mentioned above, IMO the biggest problem is that upgrades would be rocky with our packaging. That is because plugins are "under the users control". So we want clear separation: modules are an implementation detail for us just like lib only with more isolation, plugins are configurable by the user.

@bleskes
Copy link
Contributor

bleskes commented Dec 4, 2015

But the real bug is us shipping such an insecure thing by default IMO. I am intentionally steering clear of this problem on this issue, its not related, we don't offer this capability today, and @jdconrad has been working on the right solution.

Ok, so I read this to say - once we're ready, we will move groovy to a plugin. If so, I'm fine. Thanks.

As mentioned above, IMO the biggest problem is that upgrades would be rocky with our packaging. That is because plugins are "under the users control". So we want clear separation: modules are an implementation detail for us just like lib only with more isolation, plugins are configurable by the user.

I see the added value of being to just override directories when upgrading, without worrying too much about what's in them. Thanks for clarifying.

Copy link
Member

Choose a reason for hiding this comment

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

I think I'd do it as String packaging = ['tar': 'tar.gz', 'integ-test-zip': 'zip'][distro] ?: distro.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think what is here is much more readable.

Copy link
Member

Choose a reason for hiding this comment

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

Sure.

@nik9000
Copy link
Member

nik9000 commented Dec 4, 2015

\o/

Left minor comment buts it looks good to me. I wonder if we should poke anyone else who might have an opinion on this?

rjernst added a commit that referenced this pull request Dec 4, 2015
Add modules to distributions, and move lang-expression and lang-groovy to them
@rjernst rjernst merged commit 70107c5 into elastic:master Dec 4, 2015
@rjernst rjernst deleted the jigsaw branch December 4, 2015 19:44
@mark-vieira mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Delivery/Build Build or test infrastructure Team:Delivery Meta label for Delivery team v5.0.0-alpha1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants