Skip to content

Wrong behavior deleting alias #23997

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.

Sign up for GitHub

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

Merged
javanna merged 8 commits into from
Jun 6, 2017
Merged

Wrong behavior deleting alias #23997

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
78d0c53
Resolve indices only against indices
olcbean Apr 8, 2017
2bc39bb
Added more descriptive comments in the tests
olcbean Apr 11, 2017
bfddcd7
Revert the changes to the IndexNameExpressionResolver and introducing
olcbean Apr 24, 2017
f733cb0
Introduce 'ignoreAliases' IndicesOption
olcbean May 1, 2017
3cc0deb
integrating remarks
olcbean May 12, 2017
b913036
adding tests and integrating remarks
olcbean May 18, 2017
5d60f4d
integrating remarks
olcbean May 26, 2017
45f83a5
adding a remove_index test
olcbean May 27, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions core/src/main/java/org/elasticsearch/action/admin/indices/alias/IndicesAliasesRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,10 @@
public class IndicesAliasesRequest extends AcknowledgedRequest<IndicesAliasesRequest> {
private List<AliasActions> allAliasActions = new ArrayList<>();

//indices options that require every specified index to exist, expand wildcards only to open indices and
//don't allow that no indices are resolved from wildcard expressions
private static final IndicesOptions INDICES_OPTIONS = IndicesOptions.fromOptions(false, false, true, false);
// indices options that require every specified index to exist, expand wildcards only to open
// indices, don't allow that no indices are resolved from wildcard expressions and resolve the
// expressions only against indices
private static final IndicesOptions INDICES_OPTIONS = IndicesOptions.fromOptions(false, false, true, false, true, false, true);

public IndicesAliasesRequest() {

Expand Down
49 changes: 38 additions & 11 deletions core/src/main/java/org/elasticsearch/action/support/IndicesOptions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
package org.elasticsearch.action.support;


import org.elasticsearch.Version;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.rest.RestRequest;
Expand All @@ -43,6 +44,7 @@ public class IndicesOptions {
private static final byte EXPAND_WILDCARDS_CLOSED = 8;
private static final byte FORBID_ALIASES_TO_MULTIPLE_INDICES = 16;
private static final byte FORBID_CLOSED_INDICES = 32;
private static final byte IGNORE_ALIASES = 64;

private static final byte STRICT_EXPAND_OPEN = 6;
private static final byte LENIENT_EXPAND_OPEN = 7;
Expand All @@ -51,10 +53,10 @@ public class IndicesOptions {
private static final byte STRICT_SINGLE_INDEX_NO_EXPAND_FORBID_CLOSED = 48;

static {
byte max = 1 << 6;
short max = 1 << 7;
VALUES = new IndicesOptions[max];
for (byte id = 0; id < max; id++) {
VALUES[id] = new IndicesOptions(id);
for (short id = 0; id < max; id++) {
VALUES[id] = new IndicesOptions((byte)id);
}
}

Expand Down Expand Up @@ -106,18 +108,31 @@ public boolean forbidClosedIndices() {
* @return whether aliases pointing to multiple indices are allowed
*/
public boolean allowAliasesToMultipleIndices() {
//true is default here, for bw comp we keep the first 16 values
//in the array same as before + the default value for the new flag
// true is default here, for bw comp we keep the first 16 values
// in the array same as before + the default value for the new flag
return (id & FORBID_ALIASES_TO_MULTIPLE_INDICES) == 0;
}

/**
* @return whether aliases should be ignored (when resolving a wildcard)
*/
public boolean ignoreAliases() {
return (id & IGNORE_ALIASES) != 0;
}

public void writeIndicesOptions(StreamOutput out) throws IOException {
out.write(id);
if (out.getVersion().onOrAfter(Version.V_6_0_0_alpha2)) {
out.write(id);
} else {
// if we are talking to a node that doesn't support the newly added flag (ignoreAliases)
// flip to 0 all the bits starting from the 7th
out.write(id & 0x3f);
}
}

public static IndicesOptions readIndicesOptions(StreamInput in) throws IOException {
//if we read from a node that doesn't support the newly added flag (allowAliasesToMultipleIndices)
//we just receive the old corresponding value with the new flag set to true (default)
//if we read from a node that doesn't support the newly added flag (ignoreAliases)
//we just receive the old corresponding value with the new flag set to false (default)
byte id = in.readByte();
if (id >= VALUES.length) {
throw new IllegalArgumentException("No valid missing index type id: " + id);
Expand All @@ -133,8 +148,16 @@ public static IndicesOptions fromOptions(boolean ignoreUnavailable, boolean allo
return fromOptions(ignoreUnavailable, allowNoIndices, expandToOpenIndices, expandToClosedIndices, defaultOptions.allowAliasesToMultipleIndices(), defaultOptions.forbidClosedIndices());
}

static IndicesOptions fromOptions(boolean ignoreUnavailable, boolean allowNoIndices, boolean expandToOpenIndices, boolean expandToClosedIndices, boolean allowAliasesToMultipleIndices, boolean forbidClosedIndices) {
byte id = toByte(ignoreUnavailable, allowNoIndices, expandToOpenIndices, expandToClosedIndices, allowAliasesToMultipleIndices, forbidClosedIndices);
public static IndicesOptions fromOptions(boolean ignoreUnavailable, boolean allowNoIndices, boolean expandToOpenIndices,
boolean expandToClosedIndices, boolean allowAliasesToMultipleIndices, boolean forbidClosedIndices) {
return fromOptions(ignoreUnavailable, allowNoIndices, expandToOpenIndices, expandToClosedIndices, allowAliasesToMultipleIndices,
forbidClosedIndices, false);
}

public static IndicesOptions fromOptions(boolean ignoreUnavailable, boolean allowNoIndices, boolean expandToOpenIndices,
Copy link
Member

@javanna javanna May 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you expand IndicesOptionsTests#testFromOptions using and testing this new flag too?

boolean expandToClosedIndices, boolean allowAliasesToMultipleIndices, boolean forbidClosedIndices, boolean ignoreAliases) {
byte id = toByte(ignoreUnavailable, allowNoIndices, expandToOpenIndices, expandToClosedIndices, allowAliasesToMultipleIndices,
forbidClosedIndices, ignoreAliases);
return VALUES[id];
}

Expand Down Expand Up @@ -246,7 +269,7 @@ public static IndicesOptions lenientExpandOpen() {
}

private static byte toByte(boolean ignoreUnavailable, boolean allowNoIndices, boolean wildcardExpandToOpen,
boolean wildcardExpandToClosed, boolean allowAliasesToMultipleIndices, boolean forbidClosedIndices) {
boolean wildcardExpandToClosed, boolean allowAliasesToMultipleIndices, boolean forbidClosedIndices, boolean ignoreAliases) {
byte id = 0;
if (ignoreUnavailable) {
id |= IGNORE_UNAVAILABLE;
Expand All @@ -268,6 +291,9 @@ private static byte toByte(boolean ignoreUnavailable, boolean allowNoIndices, bo
if (forbidClosedIndices) {
id |= FORBID_CLOSED_INDICES;
}
if (ignoreAliases) {
id |= IGNORE_ALIASES;
}
return id;
}

Expand All @@ -281,6 +307,7 @@ public String toString() {
", expand_wildcards_closed=" + expandWildcardsClosed() +
", allow_aliases_to_multiple_indices=" + allowAliasesToMultipleIndices() +
", forbid_closed_indices=" + forbidClosedIndices() +
", ignore_aliases=" + ignoreAliases() +
']';
}
}
34 changes: 24 additions & 10 deletions core/src/main/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
import java.util.Locale;
import java.util.Map;
import java.util.Set;
import java.util.SortedMap;
import java.util.function.Predicate;
import java.util.stream.Collectors;

Expand Down Expand Up @@ -104,7 +105,7 @@ public String[] concreteIndexNames(ClusterState state, IndicesOptions options, S
return concreteIndexNames(context, indexExpressions);
}

/**
/**
* Translates the provided index expression into actual concrete indices, properly deduplicated.
*
* @param state the cluster state containing all the data to resolve to expressions to concrete indices
Expand Down Expand Up @@ -181,7 +182,7 @@ Index[] concreteIndices(Context context, String... indexExpressions) {
final Set<Index> concreteIndices = new HashSet<>(expressions.size());
for (String expression : expressions) {
AliasOrIndex aliasOrIndex = metaData.getAliasAndIndexLookup().get(expression);
if (aliasOrIndex == null) {
if (aliasOrIndex == null || (aliasOrIndex.isAlias() && context.getOptions().ignoreAliases())) {
if (failNoIndices) {
IndexNotFoundException infe = new IndexNotFoundException(expression);
infe.setResources("index_expression", expression);
Expand Down Expand Up @@ -638,7 +639,7 @@ private Set<String> innerResolve(Context context, List<String> expressions, Indi
}

final IndexMetaData.State excludeState = excludeState(options);
final Map<String, AliasOrIndex> matches = matches(metaData, expression);
final Map<String, AliasOrIndex> matches = matches(context, metaData, expression);
Copy link
Member

@javanna javanna May 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here we are fixing wildcard matching, but shouldn't we also honour the new option when resolving provided concrete names? e.g. what if I specify an alias amongst the indices, that are supposed to be concrete indices names only? I think if we fix this, it would also help with #10106 as we would not allow to create an alias that points to another alias anymore (which does not do what users think it does).

Set<String> expand = expand(context, excludeState, matches);
if (add) {
result.addAll(expand);
Expand Down Expand Up @@ -693,31 +694,44 @@ private static IndexMetaData.State excludeState(IndicesOptions options) {
return excludeState;
}

private static Map<String, AliasOrIndex> matches(MetaData metaData, String expression) {
public static Map<String, AliasOrIndex> matches(Context context, MetaData metaData, String expression) {
Copy link
Member

@javanna javanna May 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could be package private rather than public?

Copy link
Contributor Author

@olcbean olcbean May 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to change it to public for the unit test : WildcardExpressionResolverTests#testConcreteIndicesWildcardAndAliases

Of course I can remove the test and revert to private

Copy link
Member

@javanna javanna May 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hadn't seen that, sorry

if (Regex.isMatchAllPattern(expression)) {
// Can only happen if the expressions was initially: '-*'
return metaData.getAliasAndIndexLookup();
if (context.getOptions().ignoreAliases()) {
return metaData.getAliasAndIndexLookup().entrySet().stream()
.filter(e -> e.getValue().isAlias() == false)
.collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
} else {
return metaData.getAliasAndIndexLookup();
}
} else if (expression.indexOf("*") == expression.length() - 1) {
return suffixWildcard(metaData, expression);
return suffixWildcard(context, metaData, expression);
} else {
return otherWildcard(metaData, expression);
return otherWildcard(context, metaData, expression);
}
}

private static Map<String, AliasOrIndex> suffixWildcard(MetaData metaData, String expression) {
private static Map<String, AliasOrIndex> suffixWildcard(Context context, MetaData metaData, String expression) {
assert expression.length() >= 2 : "expression [" + expression + "] should have at least a length of 2";
String fromPrefix = expression.substring(0, expression.length() - 1);
char[] toPrefixCharArr = fromPrefix.toCharArray();
toPrefixCharArr[toPrefixCharArr.length - 1]++;
String toPrefix = new String(toPrefixCharArr);
return metaData.getAliasAndIndexLookup().subMap(fromPrefix, toPrefix);
SortedMap<String,AliasOrIndex> subMap = metaData.getAliasAndIndexLookup().subMap(fromPrefix, toPrefix);
if (context.getOptions().ignoreAliases()) {
return subMap.entrySet().stream()
.filter(entry -> entry.getValue().isAlias() == false)
.collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
}
return subMap;
}

private static Map<String, AliasOrIndex> otherWildcard(MetaData metaData, String expression) {
private static Map<String, AliasOrIndex> otherWildcard(Context context, MetaData metaData, String expression) {
final String pattern = expression;
return metaData.getAliasAndIndexLookup()
.entrySet()
.stream()
.filter(e -> context.getOptions().ignoreAliases() == false || e.getValue().isAlias() == false)
.filter(e -> Regex.simpleMatch(pattern, e.getKey()))
.collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
}
Expand Down
13 changes: 11 additions & 2 deletions core/src/test/java/org/elasticsearch/action/support/IndicesOptionsTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ public void testSerialization() throws Exception {
int iterations = randomIntBetween(5, 20);
for (int i = 0; i < iterations; i++) {
IndicesOptions indicesOptions = IndicesOptions.fromOptions(
randomBoolean(), randomBoolean(), randomBoolean(), randomBoolean(), randomBoolean(), randomBoolean());
randomBoolean(), randomBoolean(), randomBoolean(), randomBoolean(), randomBoolean(), randomBoolean(), randomBoolean());

BytesStreamOutput output = new BytesStreamOutput();
Version outputVersion = randomVersion(random());
Expand All @@ -50,6 +50,12 @@ public void testSerialization() throws Exception {

assertThat(indicesOptions2.forbidClosedIndices(), equalTo(indicesOptions.forbidClosedIndices()));
assertThat(indicesOptions2.allowAliasesToMultipleIndices(), equalTo(indicesOptions.allowAliasesToMultipleIndices()));

if (output.getVersion().onOrAfter(Version.V_6_0_0_alpha2)) {
assertEquals(indicesOptions2.ignoreAliases(), indicesOptions.ignoreAliases());
} else {
assertFalse(indicesOptions2.ignoreAliases());
}
}
}

Expand All @@ -62,9 +68,11 @@ public void testFromOptions() {
boolean expandToClosedIndices = randomBoolean();
boolean allowAliasesToMultipleIndices = randomBoolean();
boolean forbidClosedIndices = randomBoolean();
boolean ignoreAliases = randomBoolean();

IndicesOptions indicesOptions = IndicesOptions.fromOptions(
ignoreUnavailable, allowNoIndices,expandToOpenIndices, expandToClosedIndices,
allowAliasesToMultipleIndices, forbidClosedIndices
allowAliasesToMultipleIndices, forbidClosedIndices, ignoreAliases
);

assertThat(indicesOptions.ignoreUnavailable(), equalTo(ignoreUnavailable));
Expand All @@ -74,6 +82,7 @@ public void testFromOptions() {
assertThat(indicesOptions.allowAliasesToMultipleIndices(), equalTo(allowAliasesToMultipleIndices));
assertThat(indicesOptions.allowAliasesToMultipleIndices(), equalTo(allowAliasesToMultipleIndices));
assertThat(indicesOptions.forbidClosedIndices(), equalTo(forbidClosedIndices));
assertEquals(ignoreAliases, indicesOptions.ignoreAliases());
}
}
}
55 changes: 53 additions & 2 deletions core/src/test/java/org/elasticsearch/aliases/IndexAliasesIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@

package org.elasticsearch.aliases;

import org.apache.lucene.search.join.ScoreMode;
import org.elasticsearch.action.admin.indices.alias.Alias;
import org.elasticsearch.action.admin.indices.alias.IndicesAliasesRequest.AliasActions;
import org.elasticsearch.action.admin.indices.alias.exists.AliasesExistResponse;
Expand All @@ -36,6 +35,7 @@
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.unit.TimeValue;
import org.elasticsearch.common.xcontent.XContentType;
import org.elasticsearch.index.IndexNotFoundException;
import org.elasticsearch.index.query.QueryBuilder;
import org.elasticsearch.index.query.QueryBuilders;
import org.elasticsearch.rest.action.admin.indices.AliasesNotFoundException;
Expand Down Expand Up @@ -63,7 +63,6 @@
import static org.elasticsearch.cluster.metadata.IndexMetaData.SETTING_BLOCKS_READ;
import static org.elasticsearch.cluster.metadata.IndexMetaData.SETTING_BLOCKS_WRITE;
import static org.elasticsearch.cluster.metadata.IndexMetaData.SETTING_READ_ONLY;
import static org.elasticsearch.index.query.QueryBuilders.matchAllQuery;
import static org.elasticsearch.index.query.QueryBuilders.rangeQuery;
import static org.elasticsearch.index.query.QueryBuilders.termQuery;
import static org.elasticsearch.test.hamcrest.CollectionAssertions.hasKey;
Expand Down Expand Up @@ -425,6 +424,23 @@ public void testDeleteAliases() throws Exception {

AliasesExistResponse response = admin().indices().prepareAliasesExist(aliases).get();
assertThat(response.exists(), equalTo(false));

logger.info("--> creating index [foo_foo] and [bar_bar]");
assertAcked(prepareCreate("foo_foo"));
assertAcked(prepareCreate("bar_bar"));
ensureGreen();

logger.info("--> adding [foo] alias to [foo_foo] and [bar_bar]");
assertAcked(admin().indices().prepareAliases().addAlias("foo_foo", "foo"));
assertAcked(admin().indices().prepareAliases().addAlias("bar_bar", "foo"));

assertAcked(admin().indices().prepareAliases().addAliasAction(AliasActions.remove().index("foo*").alias("foo")).execute().get());
Copy link
Member

@javanna javanna May 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe adding a get aliases request to the mix as well before the deletion so we make sure that it resolves things correctly?

Copy link
Contributor Author

@olcbean olcbean May 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately the GetAliasesRequest is not yet fixed ( will be done in a separate PR ) so I don't think that it would bring anything adding it here at this stage as it will resolve to both indices ...

But I do agree that it would be nice to add this check once the GetAliasesrequest is changed ;)

Copy link
Member

@javanna javanna May 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry I was almost sure I saw a change in GetAliasesRequest too :) I was wrong.


assertTrue(admin().indices().prepareAliasesExist("foo").get().exists());
assertFalse(admin().indices().prepareAliasesExist("foo").setIndices("foo_foo").get().exists());
assertTrue(admin().indices().prepareAliasesExist("foo").setIndices("bar_bar").get().exists());
expectThrows(IndexNotFoundException.class, () -> admin().indices().prepareAliases()
.addAliasAction(AliasActions.remove().index("foo").alias("foo")).execute().actionGet());
}

public void testWaitForAliasCreationMultipleShards() throws Exception {
Expand Down Expand Up @@ -785,6 +801,21 @@ public void testCreateIndexWithAliasesFilterNotValid() {
}
}

public void testAliasesCanBeAddedToIndicesOnly() throws Exception {
logger.info("--> creating index [2017-05-20]");
assertAcked(prepareCreate("2017-05-20"));
ensureGreen();

logger.info("--> adding [week_20] alias to [2017-05-20]");
assertAcked(admin().indices().prepareAliases().addAlias("2017-05-20", "week_20"));

IndexNotFoundException infe = expectThrows(IndexNotFoundException.class, () -> admin().indices().prepareAliases()
.addAliasAction(AliasActions.add().index("week_20").alias("tmp")).execute().actionGet());
assertEquals("week_20", infe.getIndex().getName());

assertAcked(admin().indices().prepareAliases().addAliasAction(AliasActions.add().index("2017-05-20").alias("tmp")).execute().get());
}

// Before 2.0 alias filters were parsed at alias creation time, in order
// for filters to work correctly ES required that fields mentioned in those
// filters exist in the mapping.
Expand Down Expand Up @@ -864,6 +895,26 @@ public void testAliasesWithBlocks() {
}
}

public void testAliasActionRemoveIndex() throws InterruptedException, ExecutionException {
assertAcked(prepareCreate("foo_foo"));
assertAcked(prepareCreate("bar_bar"));
assertAcked(admin().indices().prepareAliases().addAlias("foo_foo", "foo"));
assertAcked(admin().indices().prepareAliases().addAlias("bar_bar", "foo"));

expectThrows(IndexNotFoundException.class,
() -> client().admin().indices().prepareAliases().removeIndex("foo").execute().actionGet());

assertAcked(client().admin().indices().prepareAliases().removeIndex("foo*").execute().get());
assertFalse(client().admin().indices().prepareExists("foo_foo").execute().actionGet().isExists());
assertTrue(admin().indices().prepareAliasesExist("foo").get().exists());
assertTrue(client().admin().indices().prepareExists("bar_bar").execute().actionGet().isExists());
assertTrue(admin().indices().prepareAliasesExist("foo").setIndices("bar_bar").get().exists());

assertAcked(client().admin().indices().prepareAliases().removeIndex("bar_bar"));
assertFalse(admin().indices().prepareAliasesExist("foo").get().exists());
assertFalse(client().admin().indices().prepareExists("bar_bar").execute().actionGet().isExists());
}

public void testRemoveIndexAndReplaceWithAlias() throws InterruptedException, ExecutionException {
assertAcked(client().admin().indices().prepareCreate("test"));
indexRandom(true, client().prepareIndex("test_2", "test", "test").setSource("test", "test"));
Expand Down
Loading