Add per-index setting to limit number of nested fields

[ywelsch]

Indexing a document with 100 nested fields actually indexes 101 documents as each nested
document is indexed as a separate document. To safeguard against ill-defined mappings
the number of nested fields that can be defined per index has been limited to 50. This
default limit can be changed with the index setting index.mapping.nested_fields.limit.

Closes #14983

[jpountz]

This looks good to me. One thing I'm wondering is whether we should only apply the limit to indices created on or after 2.3 in order to not fail recoveries of existing indices if they happen to cross the limit (unless maybe it's something that we want to do so that they know they are in a bad situation). cc @clintongormley

[clintongormley]

@jpountz i agree that this should be for new indices (>= 2.3)

[ywelsch]

Thinking a bit more about this, I noticed that the case where we update the new settings to a value lower than the current number of nested fields is not covered. After discussing with @jpountz and @s1monw, two possible ways to think about the nested fields limit came up:

1. The setting index.mapping.nested_fields.limit guarantees that the number of nested fields in the mapping is never above that limit. This means that updating index.mapping.nested_fields.limit to a value lower than the current number of nested fields must fail the index settings update.

or

2. The setting index.mapping.nested_fields.limit guarantees that adding new nested fields fails if the number of nested fields goes over the current limit. Updating index.mapping.nested_fields.limit to a value lower than the current number of nested fields always succeeds.

In case we go with 2), I would drop the requirement that this only applies to new indices (>= 2.3).

@jpountz @clintongormley thoughts?

[clintongormley]

Good point - I think I prefer option 2

[jpountz]

I think 2 is more practical too.

[ywelsch]

I agree that option 2 is better.

Pushed a new set of changes, can you have a look again @jpountz ?

Instead of adding an(other) ad-hoc parameter to merge, I created a MergeOptions class (maybe we should call it ValidationOptions?)

Other options might be added there. In MetaDataMappingService.PutMappingExecutor.applyRequest I spotted for example a validation check that checks if _parent field points to an already existing type.

[jpountz]

I am not sure I like giving an option to bypass this check. I would rather like to have a new enum parameter in the merge method that gives the context of the mapping update (type creation, mapping update or restoring an existing mapping) and apply the nested count check whenever it makes sense (type creation and mapping updates). I expect that it would also allow us to get rid of the applyDefault parameter since the defaults only need to be applied upon type creation.

[jpountz]

If I'm not mistaken it would also work for the parent check which should only be applied for mapping updates.

[ywelsch]

The reason I chose to set the options externally is that the options for such a context enum seem too complex to me. Looking at the current code on master, I identify 9 call sites:

• creating a new index (2 calls, case distinction on default mapping)
• receiving cluster state update from master with new/updated mapping (1 call)
• adding an alias (to parse the filter) (2 calls, case distinction on default mapping)
• check compatibility during possible upgrade of index metadata (e.g. when restoring from an old snapshot) (1 call)
• refresh of mapping triggered by node (1 call)
• update of mapping triggered by user or by system (e.g. with dynamic mappings) (2 calls, one where we create the mapping from the state before we update and another where we merge the updated mapping)

I'm a bit hesitant on moving these case distinctions into MapperService. @jpountz how should we proceed?

[ywelsch]

@jpountz Thanks for the change in #16059. I have updated this PR accordingly. Can you have another look?

[jpountz]

LGTM

[jpountz]

For the record, this setting was also made updatable via 6ca7909