Only negate index expression on all indices with preceding wildcard #20898

dakrone · 2016-10-12T21:26:57Z

There is currently a very confusing behavior in Elasticsearch for the
following:

Given the indices: [test1, test2, -foo1, -foo2]

DELETE /-foo*

Will cause the test1 and test2 indices to be deleted, when what is
usually intended is to delete the -foo1 and -foo2 indices.

Previously we added a change in #20033 to disallow creating indices
starting with - or +, which will help with this situation. However,
users may have existing indices starting with these characters.

This changes the negation to only take effect in a wildcard (*) has
been seen somewhere in the expression, so in order to delete -foo1 and
-foo2 the following now works:

DELETE /-foo*

As well as:

DELETE /-foo1,-foo2

so in order to actually delete everything except for the "foo" indices
(ie, test1 and test2) a user would now issue:

DELETE /*,--foo*

Relates to #19800

There is currently a very confusing behavior in Elasticsearch for the following: Given the indices: `[test1, test2, -foo1, -foo2]` ``` DELETE /-foo* ``` Will cause the `test1` and `test2` indices to be deleted, when what is usually intended is to delete the `-foo1` and `-foo2` indices. Previously we added a change in elastic#20033 to disallow creating indices starting with `-` or `+`, which will help with this situation. However, users may have existing indices starting with these characters. This changes the negation to only take effect in a wildcard (`*`) has been seen somewhere in the expression, so in order to delete `-foo1` and `-foo2` the following now works: ``` DELETE /-foo* ``` As well as: ``` DELETE /-foo1,-foo2 ``` so in order to actually delete everything except for the "foo" indices (ie, `test1` and `test2`) a user would now issue: ``` DELETE /*,--foo* ``` Relates to elastic#19800

nik9000 · 2016-10-13T18:36:47Z

Wheh. Pure negatives getting an implicit * before them is very much the way we do things but it is so dangerous for stuff like this....

nik9000 · 2016-10-13T18:37:24Z

So, yeah, I think this is a good idea. Though I expect it needs docs updates.

nik9000 · 2016-10-13T18:39:13Z

I wonder if it'd be more clear to just reject pure negative index patterns entirely.....

javanna · 2016-10-14T16:48:15Z

I wonder if it'd be more clear to just reject pure negative index patterns entirely.....

I tend to agree, it is confusing to accept -foo* but we need to as long as there can be existing indices with name starting with -, otherwise you can't refer to them anymore. We can reject such requests once we are sure that such indices are not there anymore, and that will be from 6.0 on I believe (as such indices can only come from 2.x, which will need reindexing to go to 6.0).

dakrone · 2016-10-14T16:50:35Z

We can reject such requests once we are sure that such indices are not there anymore, and that will be from 6.0 on

I can open up a followup after this for rejecting these in the master branch only, once we get this one hammered out.

javanna

left a couple of comments, I think we may want to update api-conventions.asciidoc also, and look if we have this negation anywhere in our docs, not sure.

javanna · 2016-10-14T16:53:59Z

core/src/main/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolver.java

                    throw infe(expression);
                }
+
+                if (expression.contains("*")) {


what happens with +test1, +test2, +test3,-test2 ? it looks silly but should be supported and resolve to test1 and test3 only. I think not only a wildcard expression can be before a negation, but there has to be something, anything that is not a negation?

At this point I wonder if the order should still matter as it used to. Maybe negations should just subtract from the rest of the indices mentioned in the expression, with the requirement that an expression cannot be a negation only, otherwise we interpret it as an explicit index name that contains -. Should -test1,test* resolve to the same indices as test*,-test1? What other edge cases am I missing here? I also wonder why we support + as it is implicit anyway when missing.

what happens with +test1, +test2, +test3,-test2

That would resolve to: ["test1", "test2", "test3", "-test2"], assuming a -test2 index exists, which I think is the correct behavior?

If -test2 doesn't exist then it won't be negated and in that case I'm not sure what we want to do, do you think we should support this (adding and then removing an index without wildcards)?

+ as it is implicit anyway when missing

+1 to remove + as an operator in a followup for 6.0-only, it's just confusing

That would resolve to: ["test1", "test2", "test3", "-test2"], assuming a -test2 index exists, which I think is the correct behavior?

I see, yea that's ok, just different compared to before (we would previously add and remove test2, but there was no way to refer to -test2 if existing I believe), but correct. Do we have a test for this too?

I think documenting all this under the api conventions page would clear things up.

How about the ordering thing that I mentioned before? Should -test1,test* resolve to the same indices as test*,-test1? It doesn't now right?

Should -test1,test* resolve to the same indices as test*,-test1? It doesn't now right?

It doesn't right now, I think it would depend on if a -test1 index existed.

I don't think it should though, since the intended behavior would be different regardless if you swapped the ordering of negations? What do you think?

Maybe instead we should start with a list of indices and work backwards towards what we expect for certain combinations?

I'm afraid we need to rely on the order if we want to be able to distinguish between negations (applied when a wildcard expression appears before the negation) and referring to indices that start with -. We will be able to get rid of it in 6.0 only when we will be sure such indices are not around anymore. I opened #20962.

Can we also have a test where the wildcard expression is not the first expression but still before the negation? e.g. test1,test2,index*,-index1

Also -test1,*test2*,-test20 or something along those lines? :)

Sure, I've added both of those tests :)

javanna · 2016-10-14T17:04:47Z

I can open up a followup after this for rejecting these in the master branch only, once we get this one hammered out.

sounds great thanks

javanna · 2016-10-17T20:47:30Z

core/src/main/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolver.java

+            boolean wildcardSeen = false;
            for (int i = 0; i < expressions.size(); i++) {
                String expression = expressions.get(i);
                if (aliasOrIndexExists(metaData, expression)) {


do you understand this if block? I suspect it made sense before your change, to make it possible to refer to existing indices or aliases that started with - rather than treating the name as a negation. That said, I can't quite follow why it makes sense in some cases for result to be null. This was already there, not your doing but I wonder if it's related and may be cleaned up.

I don't know why it doesn't initialize the result set like the other null checks do, but I do think all of this code should be cleaned up after this (I hesitate to touch more code as this is supposed to be backported all the way back to 2.4)

javanna · 2016-10-17T20:49:41Z

core/src/main/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolver.java

                    throw infe(expression);
                }
+
+                if (expression.contains("*")) {


can we use the existing Regex.isSimpleMatchPattern instead?

Certainly, pushed something that does that

javanna · 2016-10-17T20:51:20Z

core/src/main/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolver.java

                }
+
+                if (expression.contains("*")) {
+                    wildcardSeen = true;


I wonder if we should set this to true only if options.expandWildcardsOpen || options.expandWildcardsClosed . Otherwise wildcard expressions don't get expanded...

I agree, I've changed the if to check these

…expand=true

javanna · 2016-10-17T21:44:37Z

core/src/main/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolver.java


-                if (expression.contains("*")) {
+                if (Regex.isSimpleMatchPattern(expression) &&
+                        (options.expandWildcardsOpen() || options.expandWildcardsClosed())) {


add some tests around checking the options?

I actually reverted the change to add the check, because if wildcards are both disabled then this check is already handled at a higher level, in resolve instead of innerResolve:

elasticsearch/core/src/main/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolver.java

Lines 559 to 561 in e57720e

if (options.expandWildcardsClosed() == false && options.expandWildcardsOpen() == false) {

return expressions;

}

…20898) * Only negate index expression on all indices with preceding wildcard There is currently a very confusing behavior in Elasticsearch for the following: Given the indices: `[test1, test2, -foo1, -foo2]` ``` DELETE /-foo* ``` Will cause the `test1` and `test2` indices to be deleted, when what is usually intended is to delete the `-foo1` and `-foo2` indices. Previously we added a change in #20033 to disallow creating indices starting with `-` or `+`, which will help with this situation. However, users may have existing indices starting with these characters. This changes the negation to only take effect in a wildcard (`*`) has been seen somewhere in the expression, so in order to delete `-foo1` and `-foo2` the following now works: ``` DELETE /-foo* ``` As well as: ``` DELETE /-foo1,-foo2 ``` so in order to actually delete everything except for the "foo" indices (ie, `test1` and `test2`) a user would now issue: ``` DELETE /*,--foo* ``` Relates to #19800

…lastic#20898) * Only negate index expression on all indices with preceding wildcard There is currently a very confusing behavior in Elasticsearch for the following: Given the indices: `[test1, test2, -foo1, -foo2]` ``` DELETE /-foo* ``` Will cause the `test1` and `test2` indices to be deleted, when what is usually intended is to delete the `-foo1` and `-foo2` indices. Previously we added a change in elastic#20033 to disallow creating indices starting with `-` or `+`, which will help with this situation. However, users may have existing indices starting with these characters. This changes the negation to only take effect in a wildcard (`*`) has been seen somewhere in the expression, so in order to delete `-foo1` and `-foo2` the following now works: ``` DELETE /-foo* ``` As well as: ``` DELETE /-foo1,-foo2 ``` so in order to actually delete everything except for the "foo" indices (ie, `test1` and `test2`) a user would now issue: ``` DELETE /*,--foo* ``` Relates to elastic#19800

dakrone added >bug v6.0.0-alpha1 v5.1.1 v5.0.0 v2.4.2 labels Oct 12, 2016

Fix WildcardExpressionResolverTests

a7f4af5

clintongormley added the :Data Management/Indices APIs APIs to create and manage indices and templates label Oct 13, 2016

dakrone added the review label Oct 13, 2016

dakrone added 2 commits October 14, 2016 10:01

Assert that non-existent negated indices are correctly non-expanded

5a2df4e

Add more tests for corner cases

7453fc5

javanna requested changes Oct 14, 2016

View reviewed changes

Add additional test cases

e4d5ee2

javanna mentioned this pull request Oct 17, 2016

Index expression matching improvements #20962

Closed

Add more additional tests

12b957b

javanna reviewed Oct 17, 2016

View reviewed changes

Check expression with Regex.isSimpleMatchPattern and only check when …

5f186a9

…expand=true

javanna reviewed Oct 17, 2016

View reviewed changes

No need to check for expandWildcardsOpen and expandWildcardsClosed

6982a9c

javanna approved these changes Oct 18, 2016

View reviewed changes

javanna merged commit c1721c6 into elastic:master Oct 18, 2016

javanna mentioned this pull request Dec 7, 2016

http://example.com/ba*,-bar,-baz/_search will search over index foo #3839

Closed

clintongormley removed the v5.1.1 label Dec 8, 2016

dakrone deleted the only-negate-after-wildcard branch January 23, 2017 17:23

javanna mentioned this pull request Jun 16, 2017

Aliases and index exclusion does not work as expected #24621

Closed

dakrone mentioned this pull request Nov 9, 2020

Fix inconsistent excludes in IndexNameExpressionResolver #64767

Closed

	if (options.expandWildcardsClosed() == false && options.expandWildcardsOpen() == false) {
	return expressions;
	}

Only negate index expression on all indices with preceding wildcard #20898

Only negate index expression on all indices with preceding wildcard #20898

Uh oh!

Conversation

dakrone commented Oct 12, 2016

Uh oh!

nik9000 commented Oct 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nik9000 commented Oct 13, 2016

Uh oh!

nik9000 commented Oct 13, 2016

Uh oh!

javanna commented Oct 14, 2016

Uh oh!

dakrone commented Oct 14, 2016

Uh oh!

javanna left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javanna commented Oct 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nik9000 commented Oct 13, 2016 •

edited

Loading