MetaData Builder doesn't properly prevent an alias with the same name as an index #26804
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… of index Elasticsearch doesn't allow having an index alias named with the same name as an existing index. We currently have logic that tries to prevents that in the `MetaData.Builder#build()` method. Sadly that logic is flawed. Depending on iteration order, we may allow the above to happen (if we encounter the alias before the index). This commit fixes the above and improves the error message while at it.
bleskes
added
:Core/Infra/Core
dakrone
requested changes
Sep 27, 2017
| } | ||
| assert duplicates.size() > 0; | ||
| throw new IllegalStateException("index and alias names need to be unique, but the following duplicates were found [" | ||
| + Strings.join(duplicates, ", ")+ "]"); |
There was a problem hiding this comment.
I think this should be Strings.collectionToCommaDelimitedString and then you can use our Strings.java instead of JOpt's version
|
|
||
| List<String> allOpenIndicesLst = new ArrayList<>(); | ||
| List<String> allClosedIndicesLst = new ArrayList<>(); | ||
| final Set<String> allIndices = new HashSet<>(); |
There was a problem hiding this comment.
Minor nit, this can be new HashSet<>(indices.size())
| } | ||
| } | ||
| } | ||
| assert duplicates.size() > 0; |
There was a problem hiding this comment.
Perhaps add the duplicate size to the assert message here
There was a problem hiding this comment.
whoops I read it backwards, so yeah, not really necessary
| final Set<String> allIndices = new HashSet<>(); | ||
| final List<String> allOpenIndices = new ArrayList<>(); | ||
| final List<String> allClosedIndices = new ArrayList<>(); | ||
| final Set<String> duplicateAliasesIndices = new HashSet<>(); |
There was a problem hiding this comment.
This can just be named allAliasNames since it's just a set of the alias names, the "duplicate" part was throwing me off
There was a problem hiding this comment.
I started that way, but since Java doesn't have a normal non set modifying set difference I had to mutate this set. I felt it's dangerous to just call it allAliases and then essentially make it empty.
|
|
||
| // build all indices map | ||
| SortedMap<String, AliasOrIndex> aliasAndIndexLookup = new TreeMap<>(); | ||
| for (ObjectCursor<IndexMetaData> cursor : indices.values()) { | ||
| IndexMetaData indexMetaData = cursor.value; | ||
| aliasAndIndexLookup.put(indexMetaData.getIndex().getName(), new AliasOrIndex.Index(indexMetaData)); | ||
| AliasOrIndex exiting = aliasAndIndexLookup.put(indexMetaData.getIndex().getName(), new AliasOrIndex.Index(indexMetaData)); |
| } else { | ||
| throw new IllegalStateException("unexpected alias [" + aliasMetaData.getAlias() + "][" + aliasOrIndex + "]"); | ||
| assert alias instanceof AliasOrIndex.Alias : alias.getClass().getName(); | ||
| ((AliasOrIndex.Alias) alias).addIndex(indexMetaData); |
There was a problem hiding this comment.
I think this would be cleaner as
aliasAndIndexLookup.compute(aliasMetaData.getAlias(), (aliasName, alias) -> {
if (alias == null) {
return new AliasOrIndex.Alias(aliasMetaData, indexMetaData);
} else {
((AliasOrIndex.Alias) alias).addIndex(indexMetaData);
return alias;
}
});
|
thx @dakrone |
bleskes
added a commit
that referenced
this pull request
Sep 29, 2017
… as an index (#26804) Elasticsearch doesn't allow having an index alias named with the same name as an existing index. We currently have logic that tries to prevents that in the `MetaData.Builder#build()` method. Sadly that logic is flawed. Depending on iteration order, we may allow the above to happen (if we encounter the alias before the index). This commit fixes the above and improves the error message while at it. Note that we have a lot of protections in place before we end up relying on the metadata builder (validating this when we process APIs). I takes quite an abuse of the cluster to get that far.
bleskes
added a commit
that referenced
this pull request
Sep 29, 2017
… as an index (#26804) Elasticsearch doesn't allow having an index alias named with the same name as an existing index. We currently have logic that tries to prevents that in the `MetaData.Builder#build()` method. Sadly that logic is flawed. Depending on iteration order, we may allow the above to happen (if we encounter the alias before the index). This commit fixes the above and improves the error message while at it. Note that we have a lot of protections in place before we end up relying on the metadata builder (validating this when we process APIs). I takes quite an abuse of the cluster to get that far.
bleskes
added a commit
that referenced
this pull request
Sep 29, 2017
… as an index (#26804) Elasticsearch doesn't allow having an index alias named with the same name as an existing index. We currently have logic that tries to prevents that in the `MetaData.Builder#build()` method. Sadly that logic is flawed. Depending on iteration order, we may allow the above to happen (if we encounter the alias before the index). This commit fixes the above and improves the error message while at it. Note that we have a lot of protections in place before we end up relying on the metadata builder (validating this when we process APIs). I takes quite an abuse of the cluster to get that far.
jasontedor
added a commit
to jasontedor/elasticsearch
that referenced
this pull request
Oct 1, 2017
* xdcr: Maybe die before trying to log cause Log cause when a write and flush fails Die if write listener fails due to fatal error RecoveryIT.testHistoryUUIDIsGenerated should reduce unassigned shards delay instead of ensure green. Replace group map settings with affix setting (elastic#26819) Fix references to vm.max_map_count in Docker docs Add more instructions about jar hell (elastic#26837) Forbid negative values for index.unassigned.node_left.delayed_timeout (elastic#26828) Nitpicking typos in comments (elastic#26831) MetaData Builder doesn't properly prevent an alias with the same name as an index (elastic#26804)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Elasticsearch doesn't allow having an index alias named with the same name as an existing index. We currently have logic that tries to prevents that in the
MetaData.Builder#build()method. Sadly that logic is flawed. Depending on iteration order, we may allow the above to happen (if we encounter the alias before the index).This commit fixes the above and improves the error message while at it.
Note that we have a lot of protections in place before we end up relying on the metadata builder (validating this when we process APIs). I takes quite an abuse of the cluster to get that far.