Change default value of action.destructive_requires_name to True.

[YashJipkate]

This PR sets the default value of action.destructive_requires_name to True. Fixes #61074.

[elasticmachine]

Pinging @elastic/es-core-infra (Team:Core/Infra)

[matriv]

@YashJipkate Please check references of this setting in the docs and other places like for example: distribution/src/config/elasticsearch.yml and make the appropriate changes.

[YashJipkate]

@matriv There are some changes I made to the doc. Please let me know if you find any issues.

[matriv]

I think we should also change in docs/reference/indices/open-close.asciidoc L53 the phrasing since now it's by default true. Also in docs/reference/modules/indices/index_management.asciidoc L16.

[matriv]

@javanna Could you please take a look as well?

[YashJipkate]

I think we should also change in docs/reference/indices/open-close.asciidoc L53 the phrasing since now it's by default true. Also in docs/reference/modules/indices/index_management.asciidoc L16.

I too thought of rephrasing them at first, but they didn't say anything about 'changing' the setting to true. They merely state a condition, which is now satisfied by default.

Although I'm open to suggestions on how to phrase it even better.

[javanna]

jenkins test this please

[YashJipkate]

@javanna @matriv I am not able to understand the cause for the failure of so many tests. I only changed one line in actual code here. Could you please help me resolve these.

[javanna]

@YashJipkate I quickly looked and I think that our integration tests rely on deleting all indices without specifying their names. This is done before every single test, which explains why every single integration test fails ;)

[rjernst]

Separate from the tests, I have two comments:

• Please make the PR title more precise, referring to the actual setting being change and what the value is changing to
• Since this is a change in behavior, this should only be done in a major version. We may want to give a deprecation warning for this upcoming change in behavior in 7.x. @javanna wdyt?

[YashJipkate]

Please make the PR title more precise, referring to the actual setting being change and what the value is changing to

@rjernst Done.

[YashJipkate]

@YashJipkate I quickly looked and I think that our integration tests rely on deleting all indices without specifying their names. This is done before every single test, which explains why every single integration test fails ;)

In that case, could you please guide me as to what can be done to resolve this?

[javanna]

@rjernst I was wondering the same about this change in behaviour. I agree it should be done in a major. I don't think I will get to properly review this PR, would you mind finding somebody to help moving this forward please?

[williamrandolph]

In order to fix the tests, we now need to set action.destructive_requires_name cluster to false for our test clusters. It looks to me like the easiest place to do that is in the code for the test cluster setup, in the file buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/ElasticsearchNode.java. Specifically, I added:

baseConfig.put("action.destructive_requires_name", "false");

...after line 1187 of that file, and that let some of the failing integration tests pass.

I'm not sure if this will fix everything, so we may have to iterate a little bit to allow wildcard deletions in every place where the tests expect them.

I requested some wording changes in other comments and meant for those requests to be part of this review.

Thank you for working with us on this PR!

[YashJipkate]

@williamrandolph I've done the changes requested by you.

Also, can you start the Jenkins here since the tests are being flaky on my PC (also takes too long and freezes the system, times out), as it isn't running automatically for the subsequent pushes on this PR. Or is there any other way I can carry out the tests?

[williamrandolph]

@spalger I have tried a couple of versions of trying to reproduce this with and without other indices or hidden indices and can't get the warning in the way you reported it. Is there anything else you can provide to help me reproduce it?

# create index
➜  curl -s -XPUT "localhost:9200/functional-test-actions-index"
{
  "acknowledged":true,
  "shards_acknowledged":true, 
  "index":"functional-test-actions-index"
}

# index is there to delete                           
➜  curl -XDELETE 'localhost:9200/functional-test-actions-index?ignore_unavailable=true' | jq '.'
{
  "acknowledged": true
}

# index is not there, but ignore-unavailable works
➜  curl -s -XDELETE 'localhost:9200/functional-test-actions-index?ignore_unavailable=true' | jq '.'
{
  "acknowledged": true
}

# using a wildcard - destructive_requires_name is set correctly
➜  curl -s -XDELETE 'localhost:9200/functional-*?ignore_unavailable=true' | jq '.'
{
  "error": {
    "root_cause": [
      {
        "type": "illegal_argument_exception",
        "reason": "Wildcard expressions or all indices are not allowed"
      }
    ],
    "type": "illegal_argument_exception",
    "reason": "Wildcard expressions or all indices are not allowed"
  },
  "status": 400
}

[spalger]

I'm running a master snapshot built by Kibana's ES Snapshot mechanism, which we use to allow tracking master without breaking CI when breaking changes are merged into ES. The snapshot can be downloaded from https://storage.googleapis.com/kibana-ci-es-snapshots-daily/8.0.0/archives/20210124-194645_63c85ff3e84/elasticsearch-8.0.0-SNAPSHOT-darwin-x86_64.tar.gz

We are currently running ES with security enabled, which maybe has something to do with it, and the CLI options: -E indices.query.bool.max_nested_depth=100 -E action.destructive_requires_name=true

I can reproduce the issue with a single request:

curl -s -XDELETE 'http://elastic:changeme@localhost:9200/functional-test-actions-index?ignore_unavailable=true&pretty'
{
  "error" : {
    "root_cause" : [
      {
        "type" : "illegal_argument_exception",
        "reason" : "Wildcard expressions or all indices are not allowed"
      }
    ],
    "type" : "illegal_argument_exception",
    "reason" : "Wildcard expressions or all indices are not allowed"
  },
  "status" : 400
}

[williamrandolph]

@spalger Thanks for the additional information and clean reproduction line! It looks like having security enabled makes the difference. I've reproduced the issue on the 7.10.2 release, so it's not specific to this PR and I won't consider it a blocker for merging. I will, however, make sure we have an issue covering the bug and link it back to this PR.

[spalger]

On second thought I don't think it's a good idea to update Kibana in a bunch of places to handle ES responding with 400 instead of 404. If you decide to merge this PR before #67958 is fixed then Kibana will only work with action.destructive_requires_name=false, which we can use by default for most Kibana dev and testing, but it means Kibana master breaks in pretty major ways when run against ES master which is a state we don't want to be in for very long. Basically, I don't think I'm in any way eligible to dictate priorities here but it would make things a good deal more stable for us if we could fix #67958 first.

[williamrandolph]

@spalger Thanks for the feedback. I've got PR #68021 in progress to fix the issue with unexpected wildcard warnings and Elasticsearch security, and I'll try to get it in ASAP so that you can verify the fix before this one is merged.

[williamrandolph]

@elasticmachine update branch

[williamrandolph]

@spalger I merged #68021 to master and 7.x — does that unblock things on your end?

[spalger]

I think that makes us g2g, much appreciated!

[mark-vieira]

@elasticmachine update branch