Add support for fine-grained settings #10116

javanna · 2015-03-17T00:01:49Z

Note that this PR is against 1.x.

Allow to enable/disable scripting based on their source (where they get loaded from), the operation that executes them and their language.

The settings cover the following combinations:

mode: on, off, sandbox
source: indexed, dynamic, file
engine: groovy, expression, mustache, etc
operation: update, search, aggs, mapping

The following settings are supported for every engine:

script.engine.groovy.indexed.update:    sandbox/on/off
script.engine.groovy.indexed.search:    sandbox/on/off
script.engine.groovy.indexed.aggs:      sandbox/on/off
script.engine.groovy.indexed.mapping:   sandbox/on/off
script.engine.groovy.dynamic.update:    sandbox/on/off
script.engine.groovy.dynamic.search:    sandbox/on/off
script.engine.groovy.dynamic.aggs:      sandbox/on/off
script.engine.groovy.dynamic.mapping:   sandbox/on/off
script.engine.groovy.file.update:       sandbox/on/off
script.engine.groovy.file.search:       sandbox/on/off
script.engine.groovy.file.aggs:         sandbox/on/off
script.engine.groovy.file.mapping:      sandbox/on/off

For ease of use, the following more generic settings are supported too:

script.indexed: sandbox/on/off
script.dynamic: sandbox/on/off
script.file:    sandbox/on/off

and

script.update:  sandbox/on/off
script.search:  sandbox/on/off
script.aggs:    sandbox/on/off
script.mapping: sandbox/on/off

These will be used to calculate the more specific settings, using the stricter setting of each combination. Operation based settings have precedence over conflicting source based ones.

Note that the mustache engine is affected by generic settings applied to any language, while native scripts aren't as they are static by definition.

Also, the previous script.disable_dynamic setting can now be deprecated.

Closes #6418
Closes #10274

javanna · 2015-03-17T00:23:06Z

@s1monw can you have a look please? @clintongormley can you please review the updated docs and the new settings api-wise?

@dadoonet have time to double check that this breaks only potentially rivers, similar to #9992 ?

s1monw · 2015-03-18T06:13:20Z

src/main/java/org/elasticsearch/script/ScriptMode.java

+
+    static ScriptMode parse(String input) {
+        input = input.toLowerCase(Locale.ROOT);
+        switch(input) {


do we have to have a gazillion different options here? can we just use valueOf(input.toUpperCase())

we can but we usually tend to be "understanding" with user input. I don't think this is a big problem, I actually did it because the suggested syntax ("enable" and "disable") differs from what we do in other places. Also whenever we accept a boolean we accept on, off, true and false if I remember correctly

then use org.elasticsearch.common.Booleans in these cases. This one has 3 option can't we just have the 3 options and be done with it? I don't get why we need to complicate things with a gazillion options

sounds good to me I am going to use Booleans, much better. @clintongormley that means I am going to drop support for enable and disable as values, as we never supported those anywhere else. We support on,true,1,yes,off,false,0,no or sandbox. I will update the docs too

s1monw · 2015-03-18T06:29:26Z

Overall this looks very very good @javanna good job! I left a bunch of comments...

javanna · 2015-03-18T18:18:38Z

I pushed a few more commits to address review comments, and replied to all of them. Sorry if the comments got messed up, I rebased by mistake, not intentionally...

javanna · 2015-03-19T10:56:36Z

src/main/java/org/elasticsearch/script/ScriptService.java

+        public String toString() {
+            //ScriptModes looks for 'dynamic' rather than 'inline' when reading settings, we should rename but we want to keep bw comp for now.
+            if (this == INLINE) {
+                return "dynamic";


@clintongormley we use inline instead of dynamic to refer to dynamic scripts in core (also Java API)... I added this special case to convert ScriptType.INLINE to the dynamic string in a bw comp manner. At some point we will have to choose for one of the two. Wondering which one we prefer: either use inline in settings here or rename ScriptType.INLINE to DYNAMIC in the future (master, it's a breaking change).

Honestly I prefer inline, but all the user-facing stuff has been called dynamic, so I'd probably stick with that.

oh boy that means that we should keep this distinction in master too, cause it's not just about bw comp. we prefer inline internally but we always called it dynamic when talking to the outside world... but I see your point, coming up with inline now among settings is confusing.

clintongormley · 2015-03-19T18:18:17Z

docs/reference/modules/scripting.asciidoc

+| `enable`  |scripting is turned on, in the context of the setting being set.
+| `sandbox` |scripts may be executed only for languages that are sandboxed.
+|=======================================================================
+


Possibly mention that only expressions is sandboxed?

mustache is too :) I will look into clarifying

clintongormley · 2015-03-19T18:19:12Z

Docs look great @javanna

Allow to on/off scripting based on their source (where they get loaded from), the operation that executes them and their language. The settings cover the following combinations: - mode: on, off, sandbox - source: indexed, dynamic, file - engine: groovy, expressions, mustache, etc - operation: update, search, aggs, mapping The following settings are supported for every engine: script.engine.groovy.indexed.update: sandbox/on/off script.engine.groovy.indexed.search: sandbox/on/off script.engine.groovy.indexed.aggs: sandbox/on/off script.engine.groovy.indexed.mapping: sandbox/on/off script.engine.groovy.dynamic.update: sandbox/on/off script.engine.groovy.dynamic.search: sandbox/on/off script.engine.groovy.dynamic.aggs: sandbox/on/off script.engine.groovy.dynamic.mapping: sandbox/on/off script.engine.groovy.file.update: sandbox/on/off script.engine.groovy.file.search: sandbox/on/off script.engine.groovy.file.aggs: sandbox/on/off script.engine.groovy.file.mapping: sandbox/on/off For ease of use, the following more generic settings are supported too: script.indexed: sandbox/on/off script.dynamic: sandbox/on/off script.file: sandbox/on/off script.update: sandbox/on/off script.search: sandbox/on/off script.aggs: sandbox/on/off script.mapping: sandbox/on/off These will be used to calculate the more specific settings, using the stricter setting of each combination. Operation based settings have precedence over conflicting source based ones. Note that the `mustache` engine is affected by generic settings applied to any language, while native scripts aren't as they are static by definition. Also, the previous `script.disable_dynamic` setting can now be deprecated. Closes elastic#6418 Closes elastic#10116 Closes elastic#10274

javanna · 2015-03-26T19:01:50Z

marking as breaking as it breaks plugins that make use of ScriptService, for instance rivers that might allow to modify documents via scripts before indexing them. It doesn't break lang plugins though. ing @dadoonet :)

Now that fine-grained script settings are supported (elastic#10116) we can remove support for the script.disable_dynamic setting. Same result as `script.disable_dynamic: false` can be obtained as follows: ``` script.inline: on script.indexed: on ```

Due to this change elastic/elasticsearch#10116 we need to update river plugin starting 1.6.0. Was a breaking change. Closes #95. (cherry picked from commit 361b613)

Due to this change elastic/elasticsearch#10116 we need to update river plugin starting 1.6.0. Was a breaking change. Closes #95.

Now that fine-grained script settings are supported (elastic#10116) we can remove support for the script.disable_dynamic setting. Same result as `script.disable_dynamic: false` can be obtained as follows: ``` script.inline: on script.indexed: on ``` An exception is thrown at startup when the old setting is set, so we make sure we tell users they have to change it rather than ignoring the setting. Closes elastic#10286

Due to this change elastic/elasticsearch#10116 we need to update river plugin starting 1.6.0. Was a breaking change. Closes #96.

Due to this change elastic/elasticsearch#10116 we need to update river plugin starting 1.6.0. Was a breaking change. Closes #96. (cherry picked from commit 3a157b6)

Plugins can make use of scripts and expose custom scripting features. Fine-grained settings introduced with elastic#10116 don't support any custom use of scripts, hence plugins are forced to choose between update, mapping, search and aggs. Introduced a new generic plugins category that can be used by plugins. Fine-grained settings apply as well: `script.plugins: off` disable any script type, for any language, only when scripts are used from plugins. `script.engine.groovy.inline.plugins: off` disables scripts when used as part of plugins, only for inline scripts and groovy language. Relates to elastic#10347

javanna · 2015-04-08T09:58:22Z

Removing the breaking label, this is not breaking anymore after #10419. This change itself used to be breaking, but the breakage was never released, thus 1.6 won't break anything around ScriptService.

javanna added :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache v1.5.0 v2.0.0-beta1 >feature review labels Mar 17, 2015

javanna force-pushed the enhancement/fine_grained_script_settings_1x branch from 4970d11 to 6450c98 Compare March 17, 2015 00:08

s1monw added v1.6.0 and removed v1.6.0 v1.5.0 labels Mar 17, 2015

s1monw self-assigned this Mar 18, 2015

s1monw reviewed Mar 18, 2015
View reviewed changes

javanna force-pushed the enhancement/fine_grained_script_settings_1x branch from 6450c98 to 80b01bf Compare March 18, 2015 17:25

javanna added v1.5.0 and removed v1.6.0 labels Mar 18, 2015

javanna reviewed Mar 19, 2015
View reviewed changes

javanna mentioned this pull request Mar 19, 2015

What should we do with groovy sandboxing? #10156

Closed

clintongormley reviewed Mar 19, 2015
View reviewed changes

added deprecation warning in logs when script.disable_dynamic is used

821d360

javanna mentioned this pull request Mar 26, 2015

Deprecate script.disable_dynamic setting #10274

Closed

javanna added >feature and removed >feature review labels Mar 26, 2015

javanna closed this in d9d1e6a Mar 26, 2015

javanna added the >breaking label Mar 26, 2015

javanna mentioned this pull request Mar 26, 2015

Remove support for script.disable_dynamic setting #10286

Closed

dadoonet added a commit to elastic/elasticsearch-river-rabbitmq that referenced this pull request Mar 31, 2015

Change in ScriptService#executable parameters

361b613

Due to this change elastic/elasticsearch#10116 we need to update river plugin starting 1.6.0. Was a breaking change. Closes #95.

dadoonet added a commit to elastic/elasticsearch-river-couchdb that referenced this pull request Mar 31, 2015

Change in ScriptService#executable parameters

3a157b6

Due to this change elastic/elasticsearch#10116 we need to update river plugin starting 1.6.0. Was a breaking change. Closes #96.

javanna mentioned this pull request Mar 31, 2015

Better categories for scripted operations #10347

Closed

javanna mentioned this pull request Apr 1, 2015

Scripting: add plugins category to supported scripted ops and related fine-grained settings #10374

Closed

javanna removed the >breaking label Apr 8, 2015

clintongormley changed the title ~~Scripting: add support for fine-grained settings~~ Add support for fine-grained settings May 30, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for fine-grained settings #10116

Add support for fine-grained settings #10116

javanna commented Mar 17, 2015

javanna commented Mar 17, 2015

s1monw Mar 18, 2015

javanna Mar 18, 2015

s1monw Mar 23, 2015

javanna Mar 23, 2015

s1monw commented Mar 18, 2015

javanna commented Mar 18, 2015

javanna Mar 19, 2015

clintongormley Mar 19, 2015

javanna Mar 19, 2015

clintongormley Mar 19, 2015

javanna Mar 19, 2015

clintongormley commented Mar 19, 2015

javanna commented Mar 26, 2015

javanna commented Apr 8, 2015

Add support for fine-grained settings #10116

Add support for fine-grained settings #10116

Conversation

javanna commented Mar 17, 2015

javanna commented Mar 17, 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

s1monw commented Mar 18, 2015

javanna commented Mar 18, 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

clintongormley commented Mar 19, 2015

javanna commented Mar 26, 2015

javanna commented Apr 8, 2015