-
Notifications
You must be signed in to change notification settings - Fork 24.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Do not return all indices if a specific alias is requested via get aliases api. #29538
Do not return all indices if a specific alias is requested via get aliases api. #29538
Conversation
Pinging @elastic/es-core-infra |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am afraid I am missing something. The api change seems really subtle to me.
AFAICS depending on how the request is constructed different results are returned.
new GetAliasesRequest().aliases("alias")
will have matchAllAliases=true
. Could such request return more results that intended ?
new GetAliasesRequest("alias");
/ new GetAliasesRequest().setAliases("alias");
will work as expected.
@@ -107,4 +121,17 @@ public ActionRequestValidationException validate() { | |||
public void readFrom(StreamInput in) throws IOException { | |||
throw new UnsupportedOperationException("usage of Streamable is to be replaced by Writeable"); | |||
} | |||
|
|||
private static boolean matchAllAliases(final String[] aliases) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAICS only {"_all"}
/ empty
/ null
will return true
and {"*"}
-> false
.
Would this violate #25114 ? ( see comment below )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests in #25114 use wildcard in index names and that is why I think no test failed. But I think it makes sense to also return true
if *
is used as alias name.
mapBuilder.put(index, Collections.unmodifiableList(filteredValues)); | ||
} else if (matchAllAliases) { | ||
// in case all aliases are requested then it is desired to return the concrete index with no aliases (#25114): | ||
mapBuilder.put(index, Collections.emptyList()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if *
is used, is it possible that only indices with aliases will be added to the result ?
@olcbean I agree, this is a tricky change.
I'm not happy about this change either, but it is the best I came up with to fix #27763 |
@martijnvg thanks for the context ! Can you modify the x-pack call ? If so maybe you can add an explicit |
@@ -88,6 +92,16 @@ public GetAliasesRequest indicesOptions(IndicesOptions indicesOptions) { | |||
return aliases; | |||
} | |||
|
|||
public GetAliasesRequest setAliases(String... aliases) { | |||
this.aliases = aliases; | |||
this.matchAllAliases = matchAllAliases(aliases); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I understand in the REST layer, we don't print out any index for which there are no aliases to return, but only in case the alias (name) parameter was provided.(https://github.com/elastic/elasticsearch/blob/master/server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetAliasesAction.java#L139). I believe this change tries to mimic the same behaviour for the transport client, but in the REST layer we don't look at what the name parameter matches, only at whether it's provided or not:
curl localhost:9200/_alias?pretty
{
"index2" : {
"aliases" : { }
},
"index" : {
"aliases" : {
"alias" : { }
}
}
}
curl localhost:9200/_alias/_all?pretty
{
"index" : {
"aliases" : {
"alias" : { }
}
}
}
That may be right or wrong, but I think we should try to return the same results if we make this change, so I'd say without changing the behaviour at REST (although we may want to discuss what the right thing to do is) the best we can do at transport is to only look at whether any specific alias was requested rather than whether the expression matched all or not.
Furthermore, I'd expect that once these changes are made to MetaData
, the REST action should be updated as some logic can be removed? By the way, related but it should not affect this PR, there's also #28799 under review that is moving the REST logic to the transport layer, yet the REST logic remains in GetAliasesResponse#toXContent
which doesn't change what the transport client returns.
I would probably consider renaming the AliasesRequest#aliases
method (to replaceAliases
?) and make sure that it's only used internally (although it needs to be public), and have users call the current setter. We clearly can't have both call the same method or we lose information about what was set in the first place. That would make it possible to keep track of whether specific aliases were requested or not through a flag, similarly to what you do now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That may be right or wrong, but I think we should try to return the same results if we make this change, so I'd say without changing the behaviour at REST (although we may want to discuss what the right thing to do is) the best we can do at transport is to only look at whether any specific alias was requested rather than whether the expression matched all or not.
Makes sense. I will make sure that the transport client only care about whether aliases was provided or not like in the rest action. And like you mentioned in chat, the matchAllAliases
should then be renamed to something else to reflect this (wasAliasExpressionSet
?).
Furthermore, I'd expect that once these changes are made to MetaData, the REST action should be updated as some logic can be removed? By the way, related but it should not affect this PR, there's also #28799 under review that is moving the REST logic to the transport layer, yet the REST logic remains in GetAliasesResponse#toXContent which doesn't change what the transport client returns.
Agreed. I will wait for #28799 to get in before working on that.
I would probably consider renaming the AliasesRequest#aliases method (to replaceAliases?) and make sure that it's only used internally (although it needs to be public), and have users call the current setter. We clearly can't have both call the same method or we lose information about what was set in the first place. That would make it possible to keep track of whether specific aliases were requested or not through a flag, similarly to what you do now.
Yes, I totally agree here.
@olcbean Sorry for the late reply.
Yes, that is what @javanna suggested too.
I think we shouldn't do that otherwise users of the Java client and we in the rest action may forget to set it. |
060539f
to
0e06e1f
Compare
0e06e1f
to
0007612
Compare
@javanna I've updated the PR and addressed your first feedback point. Regarding the other feedback points:
I think we should that in follow up PRs in order to make the changes digestible. As the PR it now it should fix the problem in #27763. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a couple of comments, I am not convinced that this change is enough, it does make our tests work but I am not sure if/how it will help users after all. I think that we should work on the two needed methods on this PR in order to have a complete solution.
ImmutableOpenMap.Builder<String, List<AliasMetaData>> mapBuilder = ImmutableOpenMap.builder(); | ||
Iterable<String> intersection = HppcMaps.intersection(ObjectHashSet.from(concreteIndices), indices.keys()); | ||
for (String index : intersection) { | ||
IndexMetaData indexMetaData = indices.get(index); | ||
List<AliasMetaData> filteredValues = new ArrayList<>(); | ||
for (ObjectCursor<AliasMetaData> cursor : indexMetaData.getAliases().values()) { | ||
AliasMetaData value = cursor.value; | ||
if (matchAllAliases || Regex.simpleMatch(aliases, value.alias())) { | ||
if (Strings.isAllOrWildcard(aliases) || Regex.simpleMatch(aliases, value.alias())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not exactly the same as the previous matchAllAliases method. I always wondered why, so maybe a good time to fix it, but this will change the behaviour in subtle ways when a '*' is provided or when an array the contains '_all' with more than one item is provided..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but this will change the behaviour in subtle ways when a '*' is provided or when an array the contains '_all' with more than one item is provided
If _all, * with other alias names is provided then Strings.isAllOrWildcard(...)
return false, so I think that is ok? I will also look into if it is possible to change TransportGetAliasesAction so that we don't have to change this method, like I mentioned in the other comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but the previous matchAllAliases didn't look for the wildcard, and only looked at whether _all was anywhere within the array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and only looked at whether _all was anywhere within the array
Or whether the array was an empty array, which sort of matched with the logic inside RestGetAliasesAction
.
If you take a look at RestGetAliasesAction
at line 81 where paramAsStringArrayOrEmptyIfAll(...)
gets execute, it will replace _all
and *
with an empty array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point. ok then we can say that we are making transport client work the same way as REST, that should be good
mapBuilder.put(index, Collections.unmodifiableList(filteredValues)); | ||
} else if (aliasesProvided == false) { | ||
// in case all aliases are requested then it is desired to return the concrete index with no aliases (#25114): | ||
mapBuilder.put(index, Collections.emptyList()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
given how weird this API is, I wonder if we could keep this logic contained in TransportGetAliasesAction, similar to what we have in RestGetAliasesAction, rather than introducing this flag in MetaData and exposing it to potentially other API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I will try to move it to TransportGetAliasesAction.
@@ -88,6 +93,20 @@ public GetAliasesRequest indicesOptions(IndicesOptions indicesOptions) { | |||
return aliases; | |||
} | |||
|
|||
public GetAliasesRequest setAliases(String... aliases) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is enough for transport client users. We are trying to fix the transport client behaviour, but effectively for this to work, transport client users need to migrate to the new setter, while the previous "setter" method is still around. How will users know that they have to move away from it? I would rather move our own code (x-pack) to use a different method to replace aliases, and leave the original setter alone, but have it set the flag that we need. Would that be possible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was afraid that this change would be much larger compared to what is currently done in the PR. But taking a second look at it, I think is is ok. I will rename the existing aliases(...) setter to replaceAliases(...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great!
2c3ae98
to
2f6c14c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few nits, some of which will mostly get irrelevant once the aliases(...)
and setAliases(...)
are renamed ( #29538 (comment) ).
I would really like to avoid any possible code change on the side of the transport client users.
@@ -1317,11 +1317,11 @@ public void testExistsAlias() { | |||
boolean hasIndices = indices != null && indices.length > 0; | |||
String[] aliases; | |||
if (hasIndices) { | |||
aliases = randomBoolean() ? null : randomIndicesNames(0, 5); | |||
aliases = randomBoolean() ? Strings.EMPTY_ARRAY : randomIndicesNames(0, 5); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that you can simplify this to aliases = randomIndicesNames(0, 5);
as it randomly returns an empty array as well.
this.aliases = new String[]{alias}; | ||
public GetAliasesRequest(String... aliases) { | ||
this.aliases = Objects.requireNonNull(aliases); | ||
this.aliasesProvided = aliases.length != 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about calling setAliases(aliases)
instead ?
|
||
/** | ||
* @return Whether aliases where originally provided by the user via {@link #setAliases(String...)} or | ||
* {@link #GetAliasesRequest(String...)}. If this is not the case and there are aliases then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you check this comment ;)
* Finds the specific index aliases that match with the specified aliases directly or partially via wildcards and | ||
* that point to the specified concrete indices or match partially with the indices via wildcards. | ||
* | ||
* @param aliasesProvided Whether concrete aliases where provided by the user in the original request. This information |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/where/were
private IndicesOptions indicesOptions = IndicesOptions.strictExpand(); | ||
private boolean aliasesProvided = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I keep on having problems remembering what aliasesProvided
means as with either aliases
or setAliases
aliases can be specified ...
What about adding some comments to the aliases
and setAliases
to make the difference more obvious to the users of this class ?
public GetAliasesRequest(String alias) { | ||
this.aliases = new String[]{alias}; | ||
public GetAliasesRequest(String... aliases) { | ||
this.aliases = Objects.requireNonNull(aliases); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this lead to user code change? ( as you changed the tests above )
Yes, that is what @javanna wants too. The idea is that changing the |
2f6c14c
to
08fb505
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left some comments, thanks a lot @martijnvg !
listener.onResponse(new GetAliasesResponse(result)); | ||
|
||
// in case all aliases are requested then it is desired to return the concrete index with no aliases (#25114): | ||
boolean aliasesProvided = Strings.isAllOrWildcard(request.getOriginalAliases()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be in case any alias was requested (what namesProvided does in RestGetAliasesAction), meaning if aliases are null or empty array? I know this is subtle and we may want to change it in the future, but I think we should do exactly what the corresponding REST action does.
@@ -263,7 +263,7 @@ public boolean equalsAliases(MetaData other) { | |||
return ImmutableOpenMap.of(); | |||
} | |||
|
|||
boolean matchAllAliases = matchAllAliases(aliases); | |||
boolean matchAllAliases = Strings.isAllOrWildcard(aliases); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small thing, but I'd prefer if we kept this change out of this PR. It is not required and it changes the behaviour ever so slightly.
} | ||
} | ||
|
||
listener.onResponse(new GetAliasesResponse(mapBuilder.build())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would you mind writing a small unit test for this transport action?
@@ -200,7 +200,7 @@ ResolvedIndices resolveIndicesAndAliases(IndicesRequest indicesRequest, MetaData | |||
if (aliasesRequest.expandAliasesWildcards()) { | |||
List<String> aliases = replaceWildcardsWithAuthorizedAliases(aliasesRequest.aliases(), | |||
loadAuthorizedAliases(authorizedIndices.get(), metaData)); | |||
aliasesRequest.aliases(aliases.toArray(new String[aliases.size()])); | |||
aliasesRequest.replaceAliases(aliases.toArray(new String[aliases.size()])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++
* Sets the array of aliases that the action relates to | ||
* Replaces the aliases that the action relates to | ||
* | ||
* This is an internal method. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can be more open on this and say: Used when wildcards expressions need to be replaced with concrete aliases throughout the execution of the request.
/** | ||
* Returns aliases originally specified by the user | ||
*/ | ||
String[] getOriginalAliases() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could as well make this public, I don't see harm in that. After all it's what the user set.
if (result.get(index) == null && aliasesProvided) { | ||
mapBuilder.put(index, Collections.emptyList()); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, the new logic replaces the corresponding logic that was only applied at REST up until now, hence I would expect a change to RestGetAliasesAction as well. I think that the REST logic though is still needed in a mixed cluster, in case the node that runs the transport action is on an older version and does not have the new logic. It would be nice though to isolate/add comments to the REST action to identify what we can/should remove in the future, so we don't forget to do so. Maybe add an assertion that fails once current version is 8 so we adapt the REST action and remove the code that's not needed (once we know for sure we cannot talk to a node that has the older version of the transport action)
// in case all aliases are requested then it is desired to return the concrete index with no aliases (#25114): | ||
boolean aliasesProvided = Strings.isAllOrWildcard(request.getOriginalAliases()); | ||
ImmutableOpenMap.Builder<String, List<AliasMetaData>> mapBuilder = ImmutableOpenMap.builder(result); | ||
Iterable<String> intersection = HppcMaps.intersection(ObjectHashSet.from(concreteIndices), state.metaData().indices().keys()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure why we need the intersection here. We have just resolved the indices to concrete indices based on the same cluster state, so it should all be existing indices?
Iterable<String> intersection = HppcMaps.intersection(ObjectHashSet.from(concreteIndices), state.metaData().indices().keys()); | ||
for (String index : intersection) { | ||
if (result.get(index) == null && aliasesProvided) { | ||
mapBuilder.put(index, Collections.emptyList()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we want to assert that the index was not already there, just to make sure?
f81ab5c
to
4691d41
Compare
…iases api. If a get alias api call requests a specific alias pattern then indices not having any matching aliases should not be included in the response. This is a second attempt to fix this (first attempt was elastic#28294). The reason that the first attempt was reverted is because when xpack security is enabled then index expression (like * or _all) are resolved prior to when a request is processed in the get aliases transport action, then `MetaData#findAliases` can't know whether requested all where requested since it was already expanded in concrete alias names. This change replaces aliases(...) replaceAliases(...) method on AliasesRequests class and leave the aliases(...) method on subclasses. So there is a distinction between when xpack security replaces aliases and a user setting aliases via the transport or high level http client. Closes elastic#27763
keep track the original aliases
…ys contains indices that exist
36d2b54
to
4eb05f4
Compare
@javanna Thanks for reviewing. I've updated the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thanks @martijnvg !!!
…iases api. (#29538) If a get alias api call requests a specific alias pattern then indices not having any matching aliases should not be included in the response. This is a second attempt to fix this (first attempt was #28294). The reason that the first attempt was reverted is because when xpack security is enabled then index expression (like * or _all) are resolved prior to when a request is processed in the get aliases transport action, then `MetaData#findAliases` can't know whether requested all where requested since it was already expanded in concrete alias names. This change replaces aliases(...) replaceAliases(...) method on AliasesRequests class and leave the aliases(...) method on subclasses. So there is a distinction between when xpack security replaces aliases and a user setting aliases via the transport or high level http client. Closes #27763
* 6.x: [ML] Fix master node deadlock during ML daily maintenance (#31836) Build: Switch integ-test-zip to OSS-only (#31866) Build: Fix detection of Eclipse Compiler Server (#31838) SQL: Remove restriction for single column grouping (#31818) Docs: Inconsistency between description and example (#31858) Fix and reenable TribeIntegrationTests QA: build improvements related to SQL projects (#31862) muted test [Docs] Add clarification to analysis example (#31826) Check timeZone() argument in AbstractSqlQueryRequest (#31822) Remove obsolete parameters from analyze rest spec (#31795) SQL: Fix incorrect HAVING equality (#31820) Smaller aesthetic fixes to InternalTestCluster (#31831) [Docs] Clarify accepted sort case (#31605) Do not return all indices if a specific alias is requested via get aliases api. (#29538) [Docs] Fix wrong link in Korean analyzer docs (#31815) Fix profiling of ordered terms aggs (#31814) Fix handling of points_only with term strategy in geo_shape (#31766) Docs: Explain _bulk?refresh shard targeting REST high-level client: add get index API (#31703)
* master: [ML] Fix master node deadlock during ML daily maintenance (#31836) Build: Switch integ-test-zip to OSS-only (#31866) SQL: Remove restriction for single column grouping (#31818) Build: Fix detection of Eclipse Compiler Server (#31838) Docs: Inconsistency between description and example (#31858) Re-enable bwc tests now that #29538 has been backported and 6.x intake build succeeded. QA: build improvements related to SQL projects (#31862) [Docs] Add clarification to analysis example (#31826) Check timeZone() argument in AbstractSqlQueryRequest (#31822) SQL: Fix incorrect HAVING equality (#31820) Smaller aesthetic fixes to InternalTestCluster (#31831) [Docs] Clarify accepted sort case (#31605) Temporarily disable bwc test in order to backport #29538 Remove obsolete parameters from analyze rest spec (#31795) [Docs] Fix wrong link in Korean analyzer docs (#31815) Fix profiling of ordered terms aggs (#31814) Properly mute test involving JDK11 closes #31739 Do not return all indices if a specific alias is requested via get aliases api. (#29538) Get snapshot rest client cleanups (#31740) Docs: Explain _bulk?refresh shard targeting Fix handling of points_only with term strategy in geo_shape (#31766)
If a get alias api call requests a specific alias pattern then
indices not having any matching aliases should not be included in the response.
This is a second attempt to fix this (first attempt was #28294).
The reason that the first attempt was reverted is because when xpack
security is enabled then index expression (like * or _all) are resolved
prior to when a request is processed in the get aliases transport action,
then
MetaData#findAliases
can't know whether requested all whererequested since it was already expanded in concrete alias names. This
change now adds an additional field to the request calss that keeps track
whether all aliases where requested.
Closes #27763