Get Aliases with wildcard exclusion expression #34230

albertzaharovits · 2018-10-02T14:13:52Z

#33518 added support for wildcard expressions for aliases resolution.
However, the REST handler will re-parse the expression on its own to
identify aliases missing from the response and issue a 404 and message.
This logic does not deal with wildcard exclusions (-...*). Consequently,
extraneous 404s were returned for aliases that were truthfully excluded,
as opposed to be missing.

Example:

curl -X PUT "localhost:9200/index" -H "Content-Type: application/json" -d'
{
  "aliases": {
    "alias1": {},
    "alias2": {}
  }
}
'
curl -X GET "localhost:9200/_alias/alias*,-alias2?pretty"
{
  "error" : "alias [-alias2] missing",
  "status" : 404,
  "index" : {
    "aliases" : {
      "alias1" : { }
    }
  }
}

The result is correct but the 404 is not.

This PR adds the code in the REST layer that will parse exclusion wildcard
expressions, together with REST tests.
The existing code issued 404s for wildcards as well as explicit indices.
In an expression with exclude wildcards (-...*) ~~followed by~~ following wildcards,
there is no way to tell if the include wildcard produced no results or
they were subsequently excluded (in the general case).
Therefore, the proposed change is breaking the behavior of 404s for
wildcards. Specifically, no 404s will be returned for wildcards, even
if they are not followed by exclude wildcards or the exclude wildcards
could not possibly exclude what has previously been included.

Depends on: #34144
Relates: #33805

elasticmachine · 2018-10-02T14:13:54Z

Pinging @elastic/es-core-infra

javanna · 2018-10-03T09:11:23Z

Thanks a lot for your effort @albertzaharovits and the super clear description!

I haven't reviewed this thoroughly yet, but given that we are breaking the behaviour of get aliases API in some circumstances in order to fix a bug, I wonder if it's worth starting the discussion on #30536 with the team, just to make sure that we move in the right direction long-term. Also, I believe aliases exclusions were not released yet, so we can always consider to back them off of 6.5 while we figure out what to do with get aliases (though this sounds a bit extreme).

Would it be possible to have a couple of examples about the scenarios affected by the breakage, and the difference before and after the change? We may also come to the conclusion that it's not that bad.

albertzaharovits · 2018-10-03T09:38:34Z

Thanks for the speedy feedback @javanna !

I will add a description of the problem about missing
wildcards in the issue you have pointed me to.

Another thing I just realized is that the REST tests I have
added are failing. Right now, they are failing because
the Security code isn't digging wildcard exclusions, dealt
with in #34144 . But, moving the tests there would not solve
it because these tests cover functionality implemented in
this PR. So, I'll be marking this PR as dependent on #34144.

jakelandis · 2019-01-14T17:28:24Z

@albertzaharovits - What do you need to move this PR forward ? Also, is this breaking change ?

albertzaharovits · 2019-01-15T12:27:38Z

Hi @jakelandis

What do you need to move this PR forward ?

A review :)

Also, is this breaking change ?

Yes, wildcards that are not present in the response will not be reported as missing anymore.

Therefore, the proposed change is breaking the behavior of 404s for
wildcards. Specifically, no 404s will be returned for wildcards, even
if they are not followed by exclude wildcards or the exclude wildcards
could not possibly exclude what has previously been included.

original-brownbear · 2019-01-16T08:20:02Z

Jenkins retest this please

javanna · 2019-01-16T08:50:40Z

heya @albertzaharovits thanks for updating the description, could you also add some examples about the scenarios affected by the breakage please, with responses before and after your change? Although it's a bug-fix, I guess we need to document the breakage somewhere?

albertzaharovits · 2019-01-21T14:08:16Z

Hi @javanna ,

What this changes is the response of the get alias request. From what I can tell, It is not described (documented) anywhere (though it is parsed by yml REST tests), this is why I have not described the breaking behavior in the documentation.
Indeed, I think it is better to describe the change with examples:

# setup
curl -X PUT "localhost:9200/index" -H "Content-Type: application/json" -d'
{
  "aliases": {
    "foo": {},
    "foobar": {}
  }
}'

# BEFORE

curl -X GET "localhost:9200/_alias/baz*?pretty"
{
  "error" : "alias [baz*] missing",
  "status" : 404
}

curl -X GET "localhost:9200/_alias/baz*,foobar*?pretty"
{
  "error" : "alias [baz*] missing",
  "status" : 404,
  "index" : {
    "aliases" : {
      "foobar" : { }
    }
  }
}

curl -X GET "localhost:9200/_alias/baz*,foo*?pretty"
{
  "error" : "alias [baz*] missing",
  "status" : 404,
  "index" : {
    "aliases" : {
      "foo" : { },
      "foobar" : { }
    }
  }
}

# But the `error` message is completely off when using exclusions (wildcards or not)

curl -X GET "localhost:9200/_alias/foob*,-foo*?pretty"
{
  "error" : "alias [-foo*] missing",
  "status" : 404,
  "index" : {
    "aliases" : {
      "foobar" : { }
    }
  }
}

curl -X GET "localhost:9200/_alias/foo*,-foob*?pretty"
{
  "error" : "alias [-foob*] missing",
  "status" : 404,
  "index" : {
    "aliases" : {
      "foo" : { },
      "foobar" : { }
    }
  }
}

# AFTER 

curl -X GET "localhost:9200/_alias/baz*?pretty"
{ }

curl -X GET "localhost:9200/_alias/baz*,foobar*?pretty"
{
  "index" : {
    "aliases" : {
      "foobar" : { }
    }
  }
}

curl -X GET "localhost:9200/_alias/baz*,foo*?pretty"
{
  "index" : {
    "aliases" : {
      "foo" : { },
      "foobar" : { }
    }
  }
}

curl -X GET "localhost:9200/_alias/foob*,-foo*?pretty"
{ }

curl -X GET "localhost:9200/_alias/foo*,-foob*?pretty"
{
  "index" : {
    "aliases" : {
      "foo" : { }
    }
  }
}

As you can see, the breaking change is that -alias* OR alias* are not reported as missing (error message and 404).

albertzaharovits · 2019-01-21T14:09:32Z

16:04:22 Tests with failures:
16:04:22 - org.elasticsearch.client.indices.PutMappingRequestTests.testFromXContent

run gradle build tests 1

albertzaharovits · 2019-01-21T17:54:18Z

https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+pull-request-2/5210/console

17:12:08 Tests with failures:
17:12:08 - org.elasticsearch.upgrades.CCRIT.testAutoFollowing

run gradle build tests 2

albertzaharovits · 2019-01-22T08:37:14Z

https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+pull-request-2/5245/console
FAILURE 0.02s J4 | PutRoleRequestTests.testSerialization <<< FAILURES!

run gradle build tests 2

albertzaharovits · 2019-01-22T10:49:52Z

./gradlew :server:unitTest -Dtests.seed=10ECBEF210621EE7 -Dtests.class=org.elasticsearch.index.engine.InternalEngineTests -Dtests.method="testEngineMaxTimestampIsInitialized" -Dtests.security.manager=true -Dtests.locale=fr-FR -Dtests.timezone=Asia/Yangon -Dcompiler.java=11 -Druntime.java=8

Probably tracked by #37684 (comment)

run gradle build tests 1

albertzaharovits · 2019-01-22T10:51:21Z

run gradle build tests 1

albertzaharovits · 2019-01-22T11:14:50Z

@elasticmachine run elasticsearch-ci/1

albertzaharovits · 2019-01-24T08:39:48Z

https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+pull-request-2/5654/consoleFull

08:33:54
08:33:54 > Task :x-pack:plugin:ccr:internalClusterTest FAILED
08:33:54 Tests with failures:
08:33:54 - org.elasticsearch.xpack.ccr.FollowerFailOverIT.testReadRequestsReturnsLatestMappingVersion
08:33:54 - org.elasticsearch.xpack.ccr.FollowerFailOverIT.testFailOverOnFollower

@elasticmachine run elasticsearch-ci/2

javanna

I am good with the change, I don't think we can avoid the breakage and it's not a big deal. I do wish that we went forward discussing #30536 meanwhile, given that this PR was first opened months ago. Can that be discussed at some point soon? Thanks for fixing this!!

javanna · 2019-01-25T15:50:13Z

server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetAliasesAction.java

-                            if (Regex.simpleMatch(pattern, aliasName)) {
-                                matches.add(pattern);
-                                continue outer;
+                // compute explicitly requested aliases that have are not returned in the result


it would be great to have a unit test for this code. Otherwise we only rely on yaml tests to test it. Could be done as a followup.

albertzaharovits · 2019-01-29T12:28:31Z

@javanna I have added the unit tests https://github.com/elastic/elasticsearch/pull/34230/files#diff-a4a22111aebdc6f8578f82fdf9bd5617

They test the alias wildcard logic in the scope of this PR.
I'll be grinding through CI and then merge.

This commit adds the code in the HTTP layer that will parse exclusion wildcard expressions. The existing code issues 404s for wildcards as well as explicit indices. But, in general, in an expression with exclude wildcards (-...*) following other include wildcards, there is no way to tell if the include wildcard produced no results or they were subsequently excluded. Therefore, the proposed change is breaking the behavior of 404s for wildcards. Specifically, no 404s will be returned for wildcards, even if they are not followed by exclude wildcards or the exclude wildcards could not possibly exclude what has previously been included. Only explicitly requested aliases will be called out as missing.

albertzaharovits added 2 commits October 2, 2018 14:33

Main code

aca87b0

Tests

d37ee10

albertzaharovits added >bug review :Data Management/Indices APIs APIs to create and manage indices and templates v7.0.0 v6.5.0 labels Oct 2, 2018

albertzaharovits self-assigned this Oct 2, 2018

albertzaharovits requested review from javanna and original-brownbear October 2, 2018 14:13

albertzaharovits added a commit to albertzaharovits/elasticsearch that referenced this pull request Oct 2, 2018

Revert changes delegated to elastic#34230

9c1e3da

albertzaharovits mentioned this pull request Oct 3, 2018

Revise get aliases response #30536

Closed

Merge branch 'master' into rest-get-alias

c6024cd

albertzaharovits mentioned this pull request Oct 5, 2018

Pattern exclusion not working properly on aliases when security is enabled #33805

Closed

rjernst removed the review label Oct 10, 2018

colings86 added v6.6.0 and removed v6.5.0 labels Oct 25, 2018

jasontedor added v6.7.0 and removed v6.6.0 labels Dec 19, 2018

albertzaharovits added 2 commits January 21, 2019 02:36

Merge branch 'master' into rest-get-alias

1e2238a

Replace version node_selector

9896e94

Merge branch 'master' into rest-get-alias

a56df8f

Merge branch 'master' into rest-get-alias

018eab2

javanna approved these changes Jan 25, 2019

View reviewed changes

albertzaharovits added 4 commits January 29, 2019 12:38

Merge branch 'master' into rest-get-alias

3d7f585

Move response builder to static method for tests

8680ef8

Added unit tests RestGetAliasesActionTests

eab88df

Checkstyle

2fe35b1

albertzaharovits added 2 commits January 29, 2019 14:34

Checkstyle

9941067

Merge branch 'master' into rest-get-alias

0839eee

albertzaharovits merged commit d05a4b9 into elastic:master Jan 29, 2019

albertzaharovits deleted the rest-get-alias branch January 29, 2019 16:56

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get Aliases with wildcard exclusion expression #34230

Get Aliases with wildcard exclusion expression #34230

albertzaharovits commented Oct 2, 2018 •

edited

Loading

elasticmachine commented Oct 2, 2018

javanna commented Oct 3, 2018

albertzaharovits commented Oct 3, 2018

jakelandis commented Jan 14, 2019 •

edited

Loading

albertzaharovits commented Jan 15, 2019

original-brownbear commented Jan 16, 2019

javanna commented Jan 16, 2019

albertzaharovits commented Jan 21, 2019 •

edited

Loading

albertzaharovits commented Jan 21, 2019

albertzaharovits commented Jan 21, 2019

albertzaharovits commented Jan 22, 2019

albertzaharovits commented Jan 22, 2019

albertzaharovits commented Jan 22, 2019

albertzaharovits commented Jan 22, 2019

albertzaharovits commented Jan 24, 2019

javanna left a comment

javanna Jan 25, 2019

albertzaharovits commented Jan 29, 2019

Get Aliases with wildcard exclusion expression #34230

Get Aliases with wildcard exclusion expression #34230

Conversation

albertzaharovits commented Oct 2, 2018 • edited Loading

elasticmachine commented Oct 2, 2018

javanna commented Oct 3, 2018

albertzaharovits commented Oct 3, 2018

jakelandis commented Jan 14, 2019 • edited Loading

albertzaharovits commented Jan 15, 2019

original-brownbear commented Jan 16, 2019

javanna commented Jan 16, 2019

albertzaharovits commented Jan 21, 2019 • edited Loading

albertzaharovits commented Jan 21, 2019

albertzaharovits commented Jan 21, 2019

albertzaharovits commented Jan 22, 2019

albertzaharovits commented Jan 22, 2019

albertzaharovits commented Jan 22, 2019

albertzaharovits commented Jan 22, 2019

albertzaharovits commented Jan 24, 2019

javanna left a comment

Choose a reason for hiding this comment

javanna Jan 25, 2019

Choose a reason for hiding this comment

albertzaharovits commented Jan 29, 2019

albertzaharovits commented Oct 2, 2018 •

edited

Loading

jakelandis commented Jan 14, 2019 •

edited

Loading

albertzaharovits commented Jan 21, 2019 •

edited

Loading