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 support for fine-grained settings #10116

Closed
wants to merge 17 commits into
base: 1.x
from

Conversation

Projects
None yet
4 participants
@javanna
Member

javanna commented Mar 17, 2015

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

This comment has been minimized.

Member

javanna commented Mar 17, 2015

@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 s1monw added v1.6.0 and removed v1.6.0 v1.5.0 labels Mar 17, 2015

@s1monw s1monw self-assigned this Mar 18, 2015

@s1monw

View changes

src/main/java/org/elasticsearch/script/ScriptMode.java Outdated
static ScriptMode parse(String input) {
input = input.toLowerCase(Locale.ROOT);
switch(input) {

This comment has been minimized.

@s1monw

s1monw Mar 18, 2015

Contributor

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

This comment has been minimized.

@javanna

javanna Mar 18, 2015

Member

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

This comment has been minimized.

@s1monw

s1monw Mar 23, 2015

Contributor

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

This comment has been minimized.

@javanna

javanna Mar 23, 2015

Member

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

View changes

src/main/java/org/elasticsearch/script/ScriptModes.java Outdated
import java.util.Set;
import java.util.TreeMap;
public class ScriptModes {

This comment has been minimized.

@s1monw

s1monw Mar 18, 2015

Contributor

can we have a javadoc comment here?

This comment has been minimized.

@s1monw

s1monw Mar 18, 2015

Contributor

I really like this class since it's so self-contained and has all the tests etc. no weird guice bloat etc. NICE!

This comment has been minimized.

@javanna

javanna Mar 18, 2015

Member

cool glad to hear, I like it too ;) ... adding javadocs

@s1monw

View changes

src/main/java/org/elasticsearch/script/ScriptModes.java Outdated
public class ScriptModes {
static final String SCRIPT_SETTINGS_PREFIX = "script.";

This comment has been minimized.

@s1monw

s1monw Mar 18, 2015

Contributor

can those be private constants?

This comment has been minimized.

@javanna

javanna Mar 18, 2015

Member

using them in tests

@s1monw

View changes

src/main/java/org/elasticsearch/script/ScriptModes.java Outdated
}
//read deprecated disable_dynamic setting, apply only if none of the new settings is used
String disableDynamicSetting = settings.get(ScriptService.DISABLE_DYNAMIC_SCRIPTING_SETTING);

This comment has been minimized.

@s1monw

s1monw Mar 18, 2015

Contributor

maybe we can break this up into several methods and test them separately as well?

This comment has been minimized.

@javanna

javanna Mar 18, 2015

Member

I broke it up in different methods that process their own part fo the settings in order. That said I am not sure about testing. I think how we currently test this class is pretty good and testing these methods separately would create some duplication, not sure it's really worth it. At the end of the day the coverage here is already high and the amount of test code is quite big too.

@s1monw

View changes

src/main/java/org/elasticsearch/script/ScriptModes.java Outdated
final ImmutableMap<String, ScriptMode> scriptModes;
ScriptModes(Map<String, ScriptEngineService> scriptEngines, Settings settings) {
this.logger = Loggers.getLogger(getClass(), settings);

This comment has been minimized.

@s1monw

s1monw Mar 18, 2015

Contributor

I think we should rather pass the logger to it or don't log here at all. we only use it for this:

 logger.warn("ignoring [disable_dynamic] setting value as conflicting scripting settings have been specified");

IMO we should rather collect the conflicting settings and provide it as a list and then log in the ScriptService with all the settings that are conflicting...

This comment has been minimized.

@javanna
private String getScriptFromIndex(Client client, String scriptLang, String id) {
String getScriptFromIndex(String scriptLang, String id) {
if (client == null) {
throw new ElasticsearchIllegalArgumentException("Got an indexed script with no Client registered.");

This comment has been minimized.

@s1monw

s1monw Mar 18, 2015

Contributor

this should be an IllegalStateException IMO?

This comment has been minimized.

@javanna

javanna Mar 18, 2015

Member

grrrr I didn't change this.... it's always been this way and to be honest I don't fully follow this whole optional client thing, and throwing an exception only from getScriptFromIndex but not from the other methods that use the client.... can we maybe clean this up in a separate PR?

This comment has been minimized.

@s1monw

s1monw Mar 23, 2015

Contributor

not really important

@s1monw

View changes

src/main/java/org/elasticsearch/script/ScriptedOp.java Outdated
* Operation/api that uses a script as part of its execution.
* Note that the suggest api is considered part of search for simplicity, as well as the percolate api.
*/
public enum ScriptedOp {

This comment has been minimized.

@s1monw

s1monw Mar 18, 2015

Contributor

again do we have to have different / alternative names? this can just be strict no? just go with whatever we have though...

This comment has been minimized.

@s1monw

s1monw Mar 18, 2015

Contributor

what is mapping here again? and don't we use it as well in the fetch phase so we should have one for this too?

This comment has been minimized.

@javanna

javanna Mar 18, 2015

Member

fetch falls under search, as well as percolator and suggester (this is documented). mapping covers the transfom mapping feature (#6599) . The rationale is try not to have too many categories, I discussed this with @clintongormley but maybe we can change this.

The thing around alternative names is mainly that I know people are going to either use mapping or mappings and I want to be nice to them since both have the same meaning :) same for aggs and aggregations, those are synonyms as far as I'm concerned

This comment has been minimized.

@s1monw

s1monw Mar 23, 2015

Contributor

don't add so many options one for each is fine nobody needs mappings vs mapping we can barf if it's wrong and provide a useful errormessage. please remove all the alternative names.

This comment has been minimized.

@javanna

javanna Mar 23, 2015

Member

There might be other settings that start with "script.". I don't think we can just barf whenever we find something that we don't recognize from within ScriptModes. I am ok with removing alternative names but we will end up ignoring what we don't know about I am afraid.

@s1monw

View changes

src/main/java/org/elasticsearch/script/ScriptedOp.java Outdated
/**
* Operation/api that uses a script as part of its execution.
* Note that the suggest api is considered part of search for simplicity, as well as the percolate api.
*/

This comment has been minimized.

@s1monw

s1monw Mar 18, 2015

Contributor

I really don't like this name, I think ExecutionContext or ScriptContext might be better names?

This comment has been minimized.

@javanna

javanna Mar 18, 2015

Member

dunno, I think *Context is too generic and we use it in other places with different meanings. To me this is really the operation/api that is trying to execute a script, hence why I came up with ScriptedOp. That said I don't have a super strong opinion about it.

This comment has been minimized.

@s1monw

s1monw Mar 23, 2015

Contributor

it is really the context under which the script is executed though IMO...

This comment has been minimized.

@javanna

javanna Mar 23, 2015

Member

renamed

@s1monw

This comment has been minimized.

Contributor

s1monw commented Mar 18, 2015

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

@javanna

This comment has been minimized.

Member

javanna commented Mar 18, 2015

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 javanna added v1.5.0 and removed v1.6.0 labels Mar 18, 2015

@javanna

View changes

src/main/java/org/elasticsearch/script/ScriptService.java Outdated
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";

This comment has been minimized.

@javanna

javanna Mar 19, 2015

Member

@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).

This comment has been minimized.

@clintongormley

clintongormley Mar 19, 2015

Member

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

This comment has been minimized.

@javanna

javanna Mar 19, 2015

Member

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

View changes

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

This comment has been minimized.

@clintongormley

clintongormley Mar 19, 2015

Member

Possibly mention that only expressions is sandboxed?

This comment has been minimized.

@javanna

javanna Mar 19, 2015

Member

mustache is too :) I will look into clarifying

@clintongormley

This comment has been minimized.

Member

clintongormley commented Mar 19, 2015

Docs look great @javanna

@javanna

This comment has been minimized.

Member

javanna commented Mar 19, 2015

@s1monw can you have another look please?

@javanna javanna added the v1.6.0 label Mar 20, 2015

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

javanna added a commit to javanna/elasticsearch that referenced this pull request Mar 26, 2015

Scripting: add support for fine-grained settings
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 javanna closed this in d9d1e6a Mar 26, 2015

@javanna

This comment has been minimized.

Member

javanna commented Mar 26, 2015

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 :)

@javanna javanna added the >breaking label Mar 26, 2015

javanna added a commit to javanna/elasticsearch that referenced this pull request Mar 31, 2015

Scripting: remove support for script.disable_dynamic setting
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
```

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

Change in `ScriptService#executable` parameters
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)

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

Change in `ScriptService#executable` parameters
Due to this change elastic/elasticsearch#10116 we need to update river plugin starting 1.6.0.

Was a breaking change.

Closes #95.

javanna added a commit to javanna/elasticsearch that referenced this pull request Mar 31, 2015

Scripting: remove support for script.disable_dynamic setting
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

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

Change in `ScriptService#executable` parameters
Due to this change elastic/elasticsearch#10116 we need to update river plugin starting 1.6.0.

Was a breaking change.

Closes #96.

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

Change in `ScriptService#executable` parameters
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)

javanna added a commit to javanna/elasticsearch that referenced this pull request Apr 1, 2015

Scripting: add plugins category to scripted operations
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 added a commit to javanna/elasticsearch that referenced this pull request Apr 1, 2015

Scripting: add plugins category to scripted operations
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

This comment has been minimized.

Member

javanna commented Apr 8, 2015

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 javanna removed the >breaking label Apr 8, 2015

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment