Watcher: 5.x/6x Upgrade breaks with watches with stored scripts and lang field

[spinscale]

Elasticsearch version (bin/elasticsearch --version): 6.x

When a user adds a watch in 5.x, that contains a script and the script section contains a lang field, the watch will be broken after upgrading to 6.x. The original PR adding this check was done in #25610

Reproduction

Have a 5.x node where you add this script and this watch (or just index via bulk under 6.x as shown as well).

POST _scripts/my_script
{
  "script": {
    "lang": "painless",
    "source": "return true"
  }
}

# add under 5.x
PUT _xpack/watcher/watch/my_watch
{
  "trigger": {
    "schedule": {
      "interval": "10h"
    }
  },
  "input": {
    "simple": {
      "foo": "bar"
    }
  },
  "condition": {
    "script": {
      "id": "my_script",
      "lang": "painless"
    }
  },
  "actions": {
    "logme": {
      "logging": {
        "text": "{{ctx}}"
      }
    }
  }
}


# or emulate under 6.x
POST _xpack/watcher/_stop

PUT _bulk
{"index":{"_index":".watches","_type":"doc","_id":"my_watch"}}
{"trigger":{"schedule":{"interval":"10h"}},"input":{"simple":{"foo":"bar"}},"condition":{"script":{"id":"my_script","lang":"painless"}},"actions":{"logme":{"logging":{"text":"{{ctx}}"}}}}

POST _xpack/watcher/_start

# this is gonna fail
GET _xpack/watcher/watch/my_watch

with the following exception

{
  "error": {
    "root_cause": [
      {
        "type": "illegal_argument_exception",
        "reason": "illegally specified <lang> for a stored script"
      }
    ],
    "type": "illegal_argument_exception",
    "reason": "illegally specified <lang> for a stored script"
  },
  "status": 400
}

If you store the above watch in 5.x you will get a depreciation message though.

Discuss: Would it make sense to check for the lang field in the condition, the transform and the conditions and transforms in the action, and optionally remove the lang field as part of the reindex that is done when running the upgrade API?
This will not catch all the possible cases, but I think 99%.

[elasticmachine]

Pinging @elastic/es-core-infra

[spinscale]

we talked about this and this is our way forward:

We will fix the upgrade helper to also take care of this. Currently the upgrade helper just removes the _status by using a script in the reindex action.

This script needs to be extended to do the following

• Check if the condition is a script based one. If true check if the script is using a lang parameter and optionally remove this parameter
• Check if the transform is a script based one. If true check if the script is using a lang parameter and optionally remove this parameter
• For each action:
  • If condition is script based and check and remove lang parameter
  • If transform is script based and check and remove lang parameter

This also requires a 5.x backport.

The only possibility left over is a chain transform, which we could cover as well, but I'd start with the above and then check how this works. See https://www.elastic.co/guide/en/elastic-stack-overview/6.4/transform-chain.html

[spinscale]

Took a look at this one, and it is more complex than I originally thought.

First, the current check, if a reindex is required, is simply checking for the internal index format being on version 6. However, in order to find out if we need to do sth, we have to check every watch being currently indexed and see if it contains the above JSON.

The next problem is that it is not sufficient to run the upgrade API once. Imagine this setup

1. User runs the latest 5.6 version
2. User indexes above watch
3. User runs upgrade
4. User indexes watch again, which is perfectly valid because the way 5.x reads script works like that
5. User upgrades to 6.x
6. Watch will not run

I do not think that it makes a lot of sense for the user to rerun the upgrade API all the time before reindexing.

So here comes another proposal to use some form of read-repair in 6.x and not touch 5.x at all.

When a watch is parsed, when can manually parse script sections and omit the the scripting language if a script with an id reference is parsed. This way parsing will succeed. As we create a Script object out of it, storing the watch will not be a problem, as toXContent should be called properly.

The remaining problem with this approach is the fact, that you may still end up with a stored invalid watch, which needs to be fixed via the upgrade API before the next major release.

For the record the following reindex script could be used

ctx._type = "doc";
if (ctx._source.containsKey("_status") && !ctx._source.containsKey("status")) {
   ctx._source.status = ctx._source.remove("_status");
}
if (ctx._source.containsKey("condition") && ctx._source.condition.containsKey("script") && ctx._source.condition.script.containsKey("lang")) {
  ctx._source.condition.script.remove("lang");
}
if (ctx._source.containsKey("transform") && ctx._source.transform.containsKey("script") && ctx._source.transform.script.containsKey("lang")) {
  ctx._source.transform.script.remove("lang");
}
ctx._source.actions.entrySet().stream().forEach(e -> {
  def name = e.getKey();
  def action = e.getValue();
  if (action.containsKey("condition") && action.condition.containsKey("script") && action.condition.script.containsKey("lang")) {
    ctx._source.actions[name].condition.script.remove("lang");
  }"
  if (action.containsKey("transform") && action.transform.containsKey("script") && action.transform.script.containsKey("lang")) {
    ctx._source.actions[name].transform.script.remove("lang");
  }
  return null;
});

[dakrone]

This has been open for quite a while, and hasn't had a lot of interest. For now I'm going to close this as something we aren't planning on implementing. We can re-open it later if needed.