Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Groovy as a scripting language, switching default from Mvel -> Groovy #6106

Closed
wants to merge 3 commits into from

Conversation

dakrone
Copy link
Member

@dakrone dakrone commented May 9, 2014

I'm currently working on sandboxing Groovy, but I wanted to get the discussion started/confirmed for where this is landing, 1.2+, or only the master (2.0) branch.

Also changes default scripting language from mvel to groovy

Fixes all tests to run with Groovy instead of mvel
public GroovyScriptEngineService(Settings settings) {
super(settings);
ImportCustomizer imports = new ImportCustomizer();
imports.addStarImports("java.util", "org.joda.time");
Copy link
Member

Choose a reason for hiding this comment

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

joda gets shaded into an "org.es..." package, maybe we should add both? Also, can we somehow add Math so there will be no need to do Math.floor, just floor?

Copy link
Member Author

Choose a reason for hiding this comment

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

Importing the shaded version org.elasticsearch.common.joda.* doesn't work, so I am importing the same thing that the Mvel version does (org.joda.time).

Added a commit to import everything from Math.*, so you can use floor(), pow(), etc.

@kimchy
Copy link
Member

kimchy commented May 9, 2014

in terms of packing, I made a note that it should be optional in the pom file, so people can run it without (and handle the case where its not available in the classpath).

Also, I believe we will need to add an include in the main/assembly/common-bin.xml and also in the dev/rpm packages to make sure the groovy lib is part of all the distributions we have.

@hbs
Copy link

hbs commented May 9, 2014

How do you intend to prevent rogue scripts?

@kimchy
Copy link
Member

kimchy commented May 9, 2014

@hbs the safest way is to disable dynamic scripting (default in 1.2), and rely on pre defined set of scripts. As for sandboxing, this is still in the explorative mode on how well we can sandbox groovy for cases where dynamic scripts are allowed.

@nik9000
Copy link
Member

nik9000 commented May 9, 2014

@hbs the safest way is to disable dynamic scripting (default in 1.2), and rely on pre defined set of scripts. As for sandboxing, this is still in the explorative mode on how well we can sandbox groovy for cases where dynamic scripts are allowed.

If it turns out Groovy can't be sandboxed is it still worth switching to?

@nik9000
Copy link
Member

nik9000 commented May 9, 2014

If it turns out Groovy can't be sandboxed is it still worth switching to?

Though to make my position clear: if it can be sandboxed it is most definitely worth switching to the default.

@kimchy
Copy link
Member

kimchy commented May 9, 2014

@nik9000 this is what we are exploring now, if it can be properly sandboxed, thats why I expect this to be delayed to 1.3 to properly make a decision with a decent time frame. Even if it can't be sandboxed, it does has the benefit of (1) a properly maintained language compared to mvel (2) speed, based on our tests, its faster than mvel and different javascript implementations (3) great story around concurrent execution and clean integration when running on the JVM

@hbs
Copy link

hbs commented May 11, 2014

My experience with previous versions of Groovy is that it cannot be sandboxed easily, if at all. The use of SecureASTCustomizer proves difficult, especially if you want to allow anything else than basic arithmetic functions, and the use of a JVM wide SecurityManager is just infeasible.

@dakrone
Copy link
Member Author

dakrone commented May 12, 2014

@hbs yes, the SecureASTCustomizer is not really nice for security, more for locking down the available language features. I am looking into using a separate classloader with sandboxing as an alternative, but still researching this subject currently.

@nik9000 it's definitely worth switching, even if we can't sandbox groovy, because mvel has some serious compilation issues (see: https://gist.githubusercontent.com/dakrone/b193c3510073e6a22f42/raw/83a9f7614821a6fb917356324d2a788d33b981e1/gistfile1.txt for an example). Additionally, groovy is much faster than mvel, so even if we keep the same amount of functionality, the speed improvement would be nice.

@dakrone
Copy link
Member Author

dakrone commented May 13, 2014

Closing this for now, as we've decided to postpone this until 1.3 to get sandboxing in. I'll re-open when I get the sandboxing in a testable state.

@dakrone dakrone closed this May 13, 2014
@s1monw s1monw removed the review label Jun 18, 2014
@clintongormley clintongormley added >feature :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache labels Jun 8, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Scripting Scripting abstractions, Painless, and Mustache >feature v2.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants