Dynamic templates should be deduplicated

[kostasb]

Bug Report / Feature request:

Check for identical entries in the list of dynamic templates and deduplicate or reject them to avoid bloating.

Describe the issue:

It is currently possible to have the same dynamic template definition appended multiple times in the dynamic templates list of a mapping or a template, which can result to significant performance degradation.

See example template of an actual problematic 300.000 lines+ configuration installed in a cluster due to a repetition when putting the dynamic templates. This eventually resulted in a slowdown, tasks piling up, causing rejected bulks etc.

Elasticsearch version: issue exists up to current - 6.2.2

Steps to reproduce:

PUT the same dynamic template definition multiple times when defining a mapping or a template:

PUT my_index
{
  "mappings": {
    "_doc": {
      "dynamic_templates": [
        {
          "docker.container.labels": {
            "mapping": {
              "type": "keyword"
            },
            "match_mapping_type": "string",
            "path_match": "docker.container.labels.*"
          }
        },
        {
          "docker.container.labels": {
            "mapping": {
              "type": "keyword"
            },
            "match_mapping_type": "string",
            "path_match": "docker.container.labels.*"
          }
        }
      ]
    }
  }
}

[elasticmachine]

Pinging @elastic/es-search-aggs

[jtibshirani]

Perhaps we should actually throw an error in the case of duplicate templates, as this would likely not be intentional and could indicate a mistake on the part of the user.

[cbuescher]

As pointed out in the PR above, simply disallowing duplicate names - although it might still be desireable - is not as straight forward since users like APM (see #29200) might rely on this, although it also creates issues on their side. Just mentioning here to cross-link for the next time this issue is visited.

[cbuescher]

Simply erroring on duplicate template names seems to be to fragile, but looking at DynamicTemplate it should be possible to detect similar templates, e.g. by implementing some equals method on the properties and disallow similar definitions. The problem described here is caused by accidentally repeating the same definition many times. Guarding against that should be possible without disallowing duplicate names altogether so usages like the one in #29200 are not affected.

[cbuescher]

We discussed this with the team and the problems create when there are multiple duplicate keys in the dynamic_template mapping
lead us to think that this should be disallowed altogether.
Apart from the issue presented here there are problems when merging templates that contain duplicate keys (the duplicates don't have to be present in all the merged templates, one seems to be enough). In this case only one is picked, which creates problems as described in elastic/apm-server#2693 (comment).
We'd like to dissallow this kind of template definition altogether but need to do this in a backwards-compatible way that allows for existing templates like the ones mentioned in #29200 (comment) to emit warnings and potentially be updated by appending unique prefixes to the duplicated names. I will open a new issue around this to discuss the details.

[cbuescher]

I opened #53326 with some more detail about out discussion around deprecating and removing the possibility of having the same template names in dynamic_template.

[cbuescher]

Since the plan for the full removal of duplicate dynamic template names spans multiple versions, maybe we could add #52062 still as a short-term solution for at least the problem described here (accidentally programmatically adding the same section over and over again?). @jtibshirani wdyt?

[jtibshirani]

@cbuescher my intuition is that we should just focus on #53326 (and close #28988 and #52062 in favor of that issue). I could see some users accidentally having duplicate entries in their templates, especially with long templates or those that are created programmatically. So if we added #52062, I think we would also want a deprecation period before we disallow duplicate templates entirely. It seems simplest for both us and users if we only performed the combined change described in #53326.

[jtibshirani]

Closing in favor of #53326.