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.

Sign up for GitHub

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

Jump to bottom

Disable dynamic scripting by default #5943

Merged
merged 1 commit into from Apr 25, 2014
Merged

Disable dynamic scripting by default #5943

merged 1 commit into from Apr 25, 2014

Conversation

dakrone
Copy link
Member

@dakrone dakrone commented Apr 25, 2014

Also modifies the documentation for this, adding an example of using a preloaded script.

Closes #5853

@dakrone
Copy link
Member Author

dakrone commented Apr 25, 2014

Added some commits adding a .templatingOnly() method to the ScriptEngineService interface so Mustache can be enabled regardless of dynamic scripting settings. Also enabled dynamic scripting in tests since it is used in many places.

@s1monw
Copy link
Contributor

s1monw commented Apr 25, 2014

cool stuff

  • I think the docs need to state that this came with ES 1.2.0
  • can we maybe name the getter sandboxed() instead of templateingOnly() because this is what we essentially checking if the script lang / type is sandboxed?

@dakrone
Copy link
Member Author

dakrone commented Apr 25, 2014

@s1monw good ideas! I've added both changes. The sandboxed name is much better and opens the path for sandboxed groovy scripts in the future.

@s1monw
Copy link
Contributor

s1monw commented Apr 25, 2014

LGTM

@dakrone dakrone merged commit 81e83cc into elastic:master Apr 25, 2014
@jpountz jpountz removed the review label Apr 30, 2014
karmi added a commit to elastic/elasticsearch-ruby that referenced this pull request May 19, 2014
@karmi
[EXT] Enabled dynamic scripting by default in the test cluster
ae965bf 
See:

* http://www.elasticsearch.org/guide/en/elasticsearch/reference/current/modules-scripting.html#_enabling_dynamic_scripting
* elastic/elasticsearch#5853
* elastic/elasticsearch#5943
@EricMCornelius EricMCornelius mentioned this pull request May 23, 2014
Closed
damm
damm reviewed Jun 2, 2014
View reviewed changes
src/test/java/org/elasticsearch/test/TestCluster.java
@@ -200,6 +200,7 @@ public TestCluster(long clusterSeed, int minNumNodes, int maxNumNodes, String cl
builder.put("path.data", dataPath.toString());
}
}
builder.put("script.disable_dynamic", false);
Copy link

Choose a reason for hiding this comment

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

shouldn't this be set to true to disable scripts by default? like the pull request states?

Copy link
Member

Choose a reason for hiding this comment

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

Since scripting is disabled by default, we re-enable it back when we start each node in our test infrastructure, just because we have quite some tests that need it on. We are thinking about re-enabling it only for the tests that rely on it though...

Copy link

Choose a reason for hiding this comment

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

@javanna thank you that helps a lot.

@gibrown gibrown mentioned this pull request Jun 5, 2014
Closed
@dakrone dakrone deleted the 5853-disable-dynamic-scripting branch September 9, 2014 13:48
@clintongormley clintongormley added the :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache label Jun 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
>breaking :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache v1.2.0 v2.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Change default for script.disable_dynamic
6 participants
@dakrone @s1monw @damm @javanna @clintongormley @jpountz