Validate alias names the same as index names

Applied (almost) the same rules we use to validate index names
to new alias names. The only rule not applies it "must be lowercase".
We have tests that don't follow that rule and I except there are lots
of examples of camelCase alias names in the wild. We can add that
validation but I'm not sure it is worth it.

Closes #20748

Adds an alias that starts with `#` to the BWC index and validates
that you can read from it and remove it. Starting with `#` isn't
allowed after 5.1.0/6.0.0 so we don't create the alias or check it
after those versions.

.gitignore

@@ -33,6 +33,7 @@ dependency-reduced-pom.xml
 # testing stuff
 **/.local*
 .vagrant/
+/logs/
 
 # osx stuff
 .DS_Store

core/src/main/java/org/elasticsearch/cluster/metadata/AliasValidator.java

@@ -99,10 +99,11 @@ public class AliasValidator extends AbstractComponent {
         }
     }
 
-    private void validateAliasStandalone(String alias, String indexRouting) {
+    void validateAliasStandalone(String alias, String indexRouting) {
         if (!Strings.hasText(alias)) {
             throw new IllegalArgumentException("alias name is required");
         }
+        MetaDataCreateIndexService.validateIndexOrAliasName(alias, InvalidAliasNameException::new);
         if (indexRouting != null && indexRouting.indexOf(',') != -1) {
             throw new IllegalArgumentException("alias [" + alias + "] has several index routing values associated with it");
         }

core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java

@@ -88,6 +88,7 @@ import java.util.Locale;
 import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.atomic.AtomicInteger;
+import java.util.function.BiFunction;
 import java.util.function.Predicate;
 
 import static org.elasticsearch.cluster.metadata.IndexMetaData.SETTING_AUTO_EXPAND_REPLICAS;
@@ -127,24 +128,37 @@ public class MetaDataCreateIndexService extends AbstractComponent {
         this.activeShardsObserver = new ActiveShardsObserver(settings, clusterService, threadPool);
     }
 
+    /**
+     * Validate the name for an index against some static rules and a cluster state.
+     */
     public static void validateIndexName(String index, ClusterState state) {
+        validateIndexOrAliasName(index, InvalidIndexNameException::new);
+        if (!index.toLowerCase(Locale.ROOT).equals(index)) {
+            throw new InvalidIndexNameException(index, "must be lowercase");
+        }
         if (state.routingTable().hasIndex(index)) {
             throw new IndexAlreadyExistsException(state.routingTable().index(index).getIndex());
         }
         if (state.metaData().hasIndex(index)) {
             throw new IndexAlreadyExistsException(state.metaData().index(index).getIndex());
         }
+        if (state.metaData().hasAlias(index)) {
+            throw new InvalidIndexNameException(index, "already exists as alias");
+        }
+    }
+
+    /**
+     * Validate the name for an index or alias against some static rules.
+     */
+    public static void validateIndexOrAliasName(String index, BiFunction<String, String, ? extends RuntimeException> exceptionCtor) {
         if (!Strings.validFileName(index)) {
-            throw new InvalidIndexNameException(index, "must not contain the following characters " + Strings.INVALID_FILENAME_CHARS);
+            throw exceptionCtor.apply(index, "must not contain the following characters " + Strings.INVALID_FILENAME_CHARS);
         }
         if (index.contains("#")) {
-            throw new InvalidIndexNameException(index, "must not contain '#'");
+            throw exceptionCtor.apply(index, "must not contain '#'");
         }
         if (index.charAt(0) == '_' || index.charAt(0) == '-' || index.charAt(0) == '+') {
-            throw new InvalidIndexNameException(index, "must not start with '_', '-', or '+'");
-        }
-        if (!index.toLowerCase(Locale.ROOT).equals(index)) {
-            throw new InvalidIndexNameException(index, "must be lowercase");
+            throw exceptionCtor.apply(index, "must not start with '_', '-', or '+'");
         }
         int byteCount = 0;
         try {
@@ -154,15 +168,10 @@ public class MetaDataCreateIndexService extends AbstractComponent {
             throw new ElasticsearchException("Unable to determine length of index name", e);
         }
         if (byteCount > MAX_INDEX_NAME_BYTES) {
-            throw new InvalidIndexNameException(index,
-                    "index name is too long, (" + byteCount +
-                            " > " + MAX_INDEX_NAME_BYTES + ")");
-        }
-        if (state.metaData().hasAlias(index)) {
-            throw new InvalidIndexNameException(index, "already exists as alias");
+            throw exceptionCtor.apply(index, "index name is too long, (" + byteCount + " > " + MAX_INDEX_NAME_BYTES + ")");
         }
         if (index.equals(".") || index.equals("..")) {
-            throw new InvalidIndexNameException(index, "must not be '.' or '..'");
+            throw exceptionCtor.apply(index, "must not be '.' or '..'");
         }
     }
 

core/src/main/java/org/elasticsearch/indices/InvalidAliasNameException.java

@@ -33,6 +33,10 @@ public class InvalidAliasNameException extends ElasticsearchException {
         setIndex(index);
     }
 
+    public InvalidAliasNameException(String name, String description) {
+        super("Invalid alias name [{}]: {}", name, description);
+    }
+
     public InvalidAliasNameException(StreamInput in) throws IOException{
         super(in);
     }

core/src/test/java/org/elasticsearch/bwcompat/OldIndexBackwardsCompatibilityIT.java

@@ -23,6 +23,7 @@ import org.apache.lucene.search.Explanation;
 import org.apache.lucene.util.LuceneTestCase;
 import org.apache.lucene.util.TestUtil;
 import org.elasticsearch.Version;
+import org.elasticsearch.VersionTests;
 import org.elasticsearch.action.admin.indices.get.GetIndexResponse;
 import org.elasticsearch.action.admin.indices.recovery.RecoveryResponse;
 import org.elasticsearch.action.admin.indices.segments.IndexSegments;
@@ -84,6 +85,7 @@ import java.util.TreeSet;
 import static org.elasticsearch.test.OldIndexUtils.assertUpgradeWorks;
 import static org.elasticsearch.test.OldIndexUtils.getIndexDir;
 import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
+import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertHitCount;
 import static org.hamcrest.Matchers.greaterThanOrEqualTo;
 
 // needs at least 2 nodes since it bumps replicas to 1
@@ -244,6 +246,7 @@ public class OldIndexBackwardsCompatibilityIT extends ESIntegTestCase {
         assertUpgradeWorks(client(), indexName, version);
         assertDeleteByQueryWorked(indexName, version);
         assertPositionIncrementGapDefaults(indexName, version);
+        assertAliasWithBadName(indexName, version);
         unloadIndex(indexName);
     }
 
@@ -429,6 +432,31 @@ public class OldIndexBackwardsCompatibilityIT extends ESIntegTestCase {
         }
     }
 
+    private static final Version VERSION_5_1_0_UNRELEASED = Version.fromString("5.1.0");
+
+    public void testUnreleasedVersion() {
+        VersionTests.assertUnknownVersion(VERSION_5_1_0_UNRELEASED);
+    }
+
+    /**
+     * Search on an alias that contains illegal characters that would prevent it from being created after 5.1.0. It should still be
+     * search-able though.
+     */
+    void assertAliasWithBadName(String indexName, Version version) throws Exception {
+        if (version.onOrAfter(VERSION_5_1_0_UNRELEASED)) {
+            return;
+        }
+        // We can read from the alias just like we can read from the index.
+        String aliasName = "#" + indexName;
+        long totalDocs = client().prepareSearch(indexName).setSize(0).get().getHits().totalHits();
+        assertHitCount(client().prepareSearch(aliasName).setSize(0).get(), totalDocs);
+        assertThat(totalDocs, greaterThanOrEqualTo(2000L));
+
+        // We can remove the alias.
+        assertAcked(client().admin().indices().prepareAliases().removeAlias(indexName, aliasName).get());
+        assertFalse(client().admin().indices().prepareAliasesExist(aliasName).get().exists());
+    }
+
     private Path getNodeDir(String indexFile) throws IOException {
         Path unzipDir = createTempDir();
         Path unzipDataDir = unzipDir.resolve("data");

core/src/test/java/org/elasticsearch/cluster/metadata/AliasValidatorTests.java

@@ -0,0 +1,47 @@
+/*
+ * Licensed to Elasticsearch under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.elasticsearch.cluster.metadata;
+
+import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.indices.InvalidAliasNameException;
+import org.elasticsearch.test.ESTestCase;
+
+import static org.hamcrest.Matchers.startsWith;
+
+public class AliasValidatorTests extends ESTestCase {
+    public void testValidatesAliasNames() {
+        AliasValidator validator = new AliasValidator(Settings.EMPTY);
+        Exception e = expectThrows(InvalidAliasNameException.class, () -> validator.validateAliasStandalone(".", null));
+        assertEquals("Invalid alias name [.]: must not be '.' or '..'", e.getMessage());
+        e = expectThrows(InvalidAliasNameException.class, () -> validator.validateAliasStandalone("..", null));
+        assertEquals("Invalid alias name [..]: must not be '.' or '..'", e.getMessage());
+        e = expectThrows(InvalidAliasNameException.class, () -> validator.validateAliasStandalone("_cat", null));
+        assertEquals("Invalid alias name [_cat]: must not start with '_', '-', or '+'", e.getMessage());
+        e = expectThrows(InvalidAliasNameException.class, () -> validator.validateAliasStandalone("-cat", null));
+        assertEquals("Invalid alias name [-cat]: must not start with '_', '-', or '+'", e.getMessage());
+        e = expectThrows(InvalidAliasNameException.class, () -> validator.validateAliasStandalone("+cat", null));
+        assertEquals("Invalid alias name [+cat]: must not start with '_', '-', or '+'", e.getMessage());
+        e = expectThrows(InvalidAliasNameException.class, () -> validator.validateAliasStandalone("c*t", null));
+        assertThat(e.getMessage(), startsWith("Invalid alias name [c*t]: must not contain the following characters "));
+
+        // Doesn't throw an exception because we allow upper case alias names
+        validator.validateAliasStandalone("CAT", null);
+    }
+}

dev-tools/create_bwc_index.py

@@ -103,7 +103,7 @@ def delete_by_query(es, version, index_name, doc_type):
     return
 
   deleted_count = es.count(index=index_name, doc_type=doc_type, body=query)['count']
-    
+
   result = es.delete_by_query(index=index_name,
                               doc_type=doc_type,
                               body=query)
@@ -113,9 +113,13 @@ def delete_by_query(es, version, index_name, doc_type):
 
   logging.info('Deleted %d docs' % deleted_count)
 
-def run_basic_asserts(es, index_name, type, num_docs):
+def run_basic_asserts(es, version, index_name, type, num_docs):
   count = es.count(index=index_name)['count']
   assert count == num_docs, 'Expected %r but got %r documents' % (num_docs, count)
+  if parse_version(version) < parse_version('5.1.0'):
+    # This alias isn't allowed to be created after 5.1 so we can verify that we can still use it
+    count = es.count(index='#' + index_name)['count']
+    assert count == num_docs, 'Expected %r but got %r documents' % (num_docs, count)
   for _ in range(0, num_docs):
     random_doc_id = random.randint(0, num_docs-1)
     doc = es.get(index=index_name, doc_type=type, id=random_doc_id)
@@ -360,8 +364,11 @@ def generate_index(client, version, index_name):
     # see https://github.com/elastic/elasticsearch/issues/5817
     num_docs = int(num_docs / 10)
   index_documents(client, index_name, 'doc', num_docs, supports_dots_in_field_names)
+  if parse_version(version) < parse_version('5.1.0'):
+    logging.info("Adding a alias that can't be created in 5.1+ so we can assert that we can still use it")
+    client.indices.put_alias(index=index_name, name='#' + index_name)
   logging.info('Running basic asserts on the data added')
-  run_basic_asserts(client, index_name, 'doc', num_docs)
+  run_basic_asserts(client, version, index_name, 'doc', num_docs)
   return num_docs, supports_dots_in_field_names
 
 def snapshot_index(client, version, repo_dir):
@@ -494,7 +501,7 @@ def create_bwc_index(cfg, version):
     if node is not None:
       # This only happens if we've hit an exception:
       shutdown_node(node)
-      
+
     shutil.rmtree(tmp_dir)
 
 def shutdown_node(node):
@@ -533,4 +540,3 @@ if __name__ == '__main__':
     main()
   except KeyboardInterrupt:
     print('Caught keyboard interrupt, exiting...')
-

rest-api-spec/src/main/resources/rest-api-spec/test/indices.put_alias/10_basic.yaml

@@ -27,3 +27,32 @@
         name: test_alias
 
   - match: {test_index.aliases.test_alias: {}}
+
+---
+"Can't create alias with invalid characters":
+
+  - do:
+      indices.create:
+        index: test_index
+
+  - do:
+      catch: request
+      indices.put_alias:
+        index: test_index
+        name: test_*
+
+---
+"Can't create alias with the same name as an index":
+
+  - do:
+      indices.create:
+        index: test_index
+  - do:
+      indices.create:
+        index: foo
+
+  - do:
+      catch: request
+      indices.put_alias:
+        index: test_index
+        name: foo