-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
Fix _alias/<alias> returning non-matching data streams #104145
Changes from 13 commits
8840d5b
48dc55d
ddad4d5
67ba648
a0394c6
6c2f36b
ad527e7
8e4899f
997ed07
cdb5715
b8a3b5f
7e0dd21
3ba3a55
ab9a20e
f06f842
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
pr: 104145 | ||
summary: Fix _alias/<alias> returning non-matching data streams | ||
area: Data streams | ||
type: bug | ||
issues: | ||
- 96589 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,7 +22,6 @@ | |
import org.elasticsearch.common.inject.Inject; | ||
import org.elasticsearch.common.logging.DeprecationCategory; | ||
import org.elasticsearch.common.logging.DeprecationLogger; | ||
import org.elasticsearch.common.regex.Regex; | ||
import org.elasticsearch.common.util.concurrent.ThreadContext; | ||
import org.elasticsearch.core.UpdateForV9; | ||
import org.elasticsearch.indices.SystemIndices; | ||
|
@@ -147,20 +146,17 @@ static Map<String, List<DataStreamAlias>> postProcess( | |
ClusterState state | ||
) { | ||
Map<String, List<DataStreamAlias>> result = new HashMap<>(); | ||
boolean noAliasesSpecified = request.getOriginalAliases() == null || request.getOriginalAliases().length == 0; | ||
List<String> requestedDataStreams = resolver.dataStreamNames(state, request.indicesOptions(), request.indices()); | ||
|
||
Map<String, List<DataStreamAlias>> dsAliases = state.metadata() | ||
.findDataStreamAliases(request.aliases(), requestedDataStreams.toArray(new String[0])); | ||
|
||
for (String requestedDataStream : requestedDataStreams) { | ||
List<DataStreamAlias> aliases = state.metadata() | ||
.dataStreamAliases() | ||
.values() | ||
.stream() | ||
.filter(alias -> alias.getDataStreams().contains(requestedDataStream)) | ||
.filter(alias -> noAliasesSpecified || Regex.simpleMatch(request.aliases(), alias.getName())) | ||
.toList(); | ||
if (aliases.isEmpty() == false) { | ||
result.put(requestedDataStream, aliases); | ||
if (dsAliases.containsKey(requestedDataStream)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that this check is redundant now, right? In Metadata, the setter only puts things in the map that's returned if they are the things you passed in as the second argument to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is, can you just return the result of |
||
result.put(requestedDataStream, dsAliases.get(requestedDataStream)); | ||
} | ||
} | ||
|
||
return result; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
* in compliance with, at your election, the Elastic License 2.0 or the Server | ||
* Side Public License, v 1. | ||
*/ | ||
|
||
package org.elasticsearch.cluster.metadata; | ||
|
||
/** | ||
* Used as a common interface for AliasMetadata and DataStreamAlias | ||
*/ | ||
interface AliasInfo { | ||
String getAlias(); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -234,6 +234,16 @@ default boolean isRestorable() { | |
|
||
private final IndexVersion oldestIndexVersion; | ||
|
||
// Used in the findAliases and findDataStreamAliases functions | ||
private interface AliasInfoGetter { | ||
List<? extends AliasInfo> get(String entityName); | ||
} | ||
|
||
// Used in the findAliases and findDataStreamAliases functions | ||
private interface AliasInfoSetter { | ||
void put(String entityName, List<AliasInfo> aliases); | ||
} | ||
|
||
private Metadata( | ||
String clusterUUID, | ||
boolean clusterUUIDCommitted, | ||
|
@@ -799,11 +809,63 @@ public Map<String, List<AliasMetadata>> findAllAliases(final String[] concreteIn | |
* aliases then the result will <b>not</b> include the index's key. | ||
*/ | ||
public Map<String, List<AliasMetadata>> findAliases(final String[] aliases, final String[] concreteIndices) { | ||
ImmutableOpenMap.Builder<String, List<AliasMetadata>> mapBuilder = ImmutableOpenMap.builder(); | ||
|
||
AliasInfoGetter getter = index -> indices.get(index).getAliases().values().stream().toList(); | ||
|
||
AliasInfoSetter setter = (index, foundAliases) -> { | ||
List<AliasMetadata> d = new ArrayList<>(); | ||
foundAliases.forEach(i -> d.add((AliasMetadata) i)); | ||
mapBuilder.put(index, d); | ||
}; | ||
|
||
findAliasInfo(aliases, concreteIndices, getter, setter); | ||
|
||
return mapBuilder.build(); | ||
} | ||
|
||
/** | ||
* Finds the specific data stream aliases that match with the specified aliases directly or partially via wildcards, and | ||
* that point to the specified data streams (directly or matching data streams via wildcards). | ||
* | ||
* @param aliases The aliases to look for. Might contain include or exclude wildcards. | ||
* @param dataStreams The data streams that the aliases must point to in order to be returned | ||
* @return A map of data stream name to the list of DataStreamAlias objects that match. If a data stream does not have matching | ||
* aliases then the result will <b>not</b> include the data stream's key. | ||
*/ | ||
public Map<String, List<DataStreamAlias>> findDataStreamAliases(final String[] aliases, final String[] dataStreams) { | ||
ImmutableOpenMap.Builder<String, List<DataStreamAlias>> mapBuilder = ImmutableOpenMap.builder(); | ||
Map<String, List<DataStreamAlias>> dataStreamAliases = dataStreamAliasesByDataStream(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm wondering if at some point we'll need to start computing this outside of this method if users have a lot of data stream aliases. But doing it once per request doesn't seem all that bad. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if doing the analysis and guidelines suggested in #104456 would help guide us here. Maybe we could store aliases differently in cluster state. |
||
|
||
AliasInfoGetter getter = dataStream -> dataStreamAliases.getOrDefault(dataStream, new ArrayList<>()); | ||
|
||
AliasInfoSetter setter = (dataStream, foundAliases) -> { | ||
List<DataStreamAlias> dsAliases = new ArrayList<>(); | ||
foundAliases.forEach(alias -> dsAliases.add((DataStreamAlias) alias)); | ||
mapBuilder.put(dataStream, dsAliases); | ||
}; | ||
|
||
findAliasInfo(aliases, dataStreams, getter, setter); | ||
|
||
return mapBuilder.build(); | ||
} | ||
|
||
/** | ||
* Find the aliases that point to the specified data streams or indices. Called from findAliases or findDataStreamAliases. | ||
* | ||
* @param aliases The aliases to look for. Might contain include or exclude wildcards. | ||
* @param possibleMatches The data streams or indices that the aliases must point to in order to be returned | ||
* @param getter A function that is used to get the alises for a given data stream or index | ||
* @param setter A function that is used to keep track of the found aliases | ||
*/ | ||
private void findAliasInfo(final String[] aliases, final String[] possibleMatches, AliasInfoGetter getter, AliasInfoSetter setter) { | ||
assert aliases != null; | ||
assert concreteIndices != null; | ||
if (concreteIndices.length == 0) { | ||
return ImmutableOpenMap.of(); | ||
assert possibleMatches != null; | ||
if (possibleMatches.length == 0) { | ||
return; | ||
} | ||
|
||
// create patterns to use to search for targets | ||
String[] patterns = new String[aliases.length]; | ||
boolean[] include = new boolean[aliases.length]; | ||
for (int i = 0; i < aliases.length; i++) { | ||
|
@@ -816,14 +878,16 @@ public Map<String, List<AliasMetadata>> findAliases(final String[] aliases, fina | |
include[i] = true; | ||
} | ||
} | ||
|
||
boolean matchAllAliases = patterns.length == 0; | ||
ImmutableOpenMap.Builder<String, List<AliasMetadata>> mapBuilder = ImmutableOpenMap.builder(); | ||
for (String index : concreteIndices) { | ||
IndexMetadata indexMetadata = indices.get(index); | ||
List<AliasMetadata> filteredValues = new ArrayList<>(); | ||
for (AliasMetadata aliasMetadata : indexMetadata.getAliases().values()) { | ||
|
||
for (String index : possibleMatches) { | ||
List<AliasInfo> filteredValues = new ArrayList<>(); | ||
|
||
List<? extends AliasInfo> entities = getter.get(index); | ||
for (AliasInfo aliasInfo : entities) { | ||
boolean matched = matchAllAliases; | ||
String alias = aliasMetadata.alias(); | ||
String alias = aliasInfo.getAlias(); | ||
for (int i = 0; i < patterns.length; i++) { | ||
if (include[i]) { | ||
if (matched == false) { | ||
|
@@ -835,16 +899,15 @@ public Map<String, List<AliasMetadata>> findAliases(final String[] aliases, fina | |
} | ||
} | ||
if (matched) { | ||
filteredValues.add(aliasMetadata); | ||
filteredValues.add(aliasInfo); | ||
} | ||
} | ||
if (filteredValues.isEmpty() == false) { | ||
// Make the list order deterministic | ||
CollectionUtil.timSort(filteredValues, Comparator.comparing(AliasMetadata::alias)); | ||
mapBuilder.put(index, Collections.unmodifiableList(filteredValues)); | ||
CollectionUtil.timSort(filteredValues, Comparator.comparing(AliasInfo::getAlias)); | ||
setter.put(index, Collections.unmodifiableList(filteredValues)); | ||
} | ||
} | ||
return mapBuilder.build(); | ||
} | ||
|
||
/** | ||
|
@@ -1273,6 +1336,25 @@ public Map<String, DataStreamAlias> dataStreamAliases() { | |
return this.custom(DataStreamMetadata.TYPE, DataStreamMetadata.EMPTY).getDataStreamAliases(); | ||
} | ||
|
||
/** | ||
* Return a map of DataStreamAlias objects by DataStream name | ||
* @return a map of DataStreamAlias objects by DataStream name | ||
*/ | ||
public Map<String, List<DataStreamAlias>> dataStreamAliasesByDataStream() { | ||
Map<String, List<DataStreamAlias>> dataStreamAliases = new HashMap<>(); | ||
|
||
for (DataStreamAlias dsAlias : dataStreamAliases().values()) { | ||
for (String dataStream : dsAlias.getDataStreams()) { | ||
if (dataStreamAliases.containsKey(dataStream) == false) { | ||
dataStreamAliases.put(dataStream, new ArrayList<>()); | ||
} | ||
dataStreamAliases.get(dataStream).add(dsAlias); | ||
} | ||
} | ||
|
||
return dataStreamAliases; | ||
} | ||
|
||
public NodesShutdownMetadata nodeShutdowns() { | ||
return custom(NodesShutdownMetadata.TYPE, NodesShutdownMetadata.EMPTY); | ||
} | ||
|
@@ -2441,7 +2523,7 @@ private static void collectAliasDuplicates( | |
reported = true; | ||
} | ||
} | ||
// This is for adding an error message for when a data steam alias has the same name as a data stream. | ||
// This is for adding an error message for when a data stream alias has the same name as a data stream. | ||
if (reported == false && dataStreamMetadata != null && dataStreamMetadata.dataStreams().containsKey(alias)) { | ||
duplicates.add("data stream alias and data stream have the same name (" + 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 currently pointed at main (8.13.0) and the PR isn't labeled with 8.12.1. We'll need to make sure to remember backport it, or change this version if this doesn't get backported in time for 8.12.1