Fix .security-* indices auto-create

[albertzaharovits]

.security-7 and .security-tokens-7 are auto-created when they do not exist, using settings and mappings in the plugin resources. This bug is caused by the way we construct the create-index request.

Specifically, when there exists an index template that matches these indices, the mappings in the template override the mappings of the index (not vice-versa), even if the mappings in the template are empty. The reasons are entirely tied to the intricacies of mapping merging after the typeless remodeling.

For example:

curl -u elastic:password -X PUT "localhost:9200/_template/template_1" -H 'Content-Type: application/json' -d'

{
  "index_patterns": [
    "*"
  ],
  "order": 0,
  "mappings": {
    "_meta": {
      "damn": "man",
      "security-version": "8.0.0"
    },
    "properties": {
      "a": {
        "type": "keyword"
      },
      "doc_type": {
        "type": "boolean"
      }
    }
  }
}
'

curl -u elastic:password -X POST "localhost:9200/_security/oauth2/token" -H 'Content-Type: application/json' -d'
{
  "grant_type" : "client_credentials"
}
'

// .security-tokens-7 is recreated , but errors...

curl -u elastic:password -XGET "localhost:9200/.security-tokens-7?pretty"
{
  ".security-tokens-7" : {
    "aliases" : {
      ".security-tokens" : { }
    },
    "mappings" : {
      "_meta" : {
        "damn" : "man",
        "security-version" : "8.0.0"
      },
      "properties" : {
        "a" : {
          "type" : "keyword"
        },
        "doc_type" : {
          "type" : "boolean" // this is conflicting with "keyword"
        }
      }
    },
    "settings" : {
      "index" : {
        "number_of_shards" : "1",
        "auto_expand_replicas" : "0-1",
        "provided_name" : ".security-tokens-7",
        "format" : "7",
        "creation_date" : "1564162015100",
        "priority" : "1000",
        "number_of_replicas" : "0",
        "uuid" : "b1wluz-jQlqZzg19ioZoeQ",
        "version" : {
          "created" : "8000099"
        }
      }
    }
  }
}

[elasticmachine]

Pinging @elastic/es-security

[albertzaharovits]

@elasticmachine run elasticsearch-ci/bwc

[albertzaharovits]

@elasticmachine run elasticsearch-ci/bwc

[tvernum]

I don't understand why the proposed fix is the correct solution.

From memory, the existing code is there to strip out the mapping type because the put mapping call requires a typeless mapping when we upgrade the mapping.
It looks like the new version just reverts that code.

It the absense of any tests, I can't tell why the new behaviour should be considered better than the old version.

[albertzaharovits]

When we construct the create index request we have the option to include or not the type name (_doc) for the mapping. We choose not to include it. The REST handler, when constructing the transport request, DOES include it. Both options are OK, as long as there is no index template. When there is an index template, the mapping merge algorithm does not work as expected. This happens when, of the mappings to be merged, some include the type name and some do not.

What do you think a test should look like? Should we validate that the create index API creates the index with the correct mapping? What granularity of validation of the created index is appropriate, eg should we check every mapping field, just the _meta, any other settings? This seems to me like distrusting our internal APIs. I believe tests should not cover scenarios where our internal APIs are not to be trusted. I think we should review the interface of the create index transport request, and correct this behavior there, but this is orthogonal to the fix proposed here; the proposed fix constructs the create-index transport request the same way the REST handler does it.

[albertzaharovits]

@elasticmachine run elasticsearch-ci/bwc

[tvernum]

What do you think a test should look like?

The purpose of this change is to fix something that is broken. But nothing in the PR demonstrates that the broken behaviour is fixed. Next week someone might raise a PR to revert your change, and if they do, all the tests will pass.
If we know that something is broken, then should be able to write a test that proves we have fixed it and that we won't revert the behaviour.

When there is an index template, the mapping merge algorithm does not work as expected. This happens when, of the mappings to be merged, some include the type name and some do not.

That sounds like a bug in core. Is that behaviour intentional? Shouldn't we be fixing this in the mapping merge instead of just ignoring broken behaviour?

[albertzaharovits]

@tvernum I am going to retract this PR, thank you for the thought you put in it.
I believe the root of the problem is the "interface" of the create index at the transport level. I have raised #45120 to attempt to fix it.