From 43c52a7a64229116a74979dfffe37ec478a9db24 Mon Sep 17 00:00:00 2001 From: Dan Hermann Date: Tue, 24 Mar 2020 08:02:02 -0500 Subject: [PATCH 1/6] data stream names must not conflict with existing indices or aliases --- .../datastream/CreateDataStreamAction.java | 62 ++++++++++++++++-- .../CreateDataStreamRequestTests.java | 64 +++++++++++++++++++ 2 files changed, 121 insertions(+), 5 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/datastream/CreateDataStreamAction.java b/server/src/main/java/org/elasticsearch/action/admin/indices/datastream/CreateDataStreamAction.java index 37f190691f33a..09057f9225de0 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/indices/datastream/CreateDataStreamAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/indices/datastream/CreateDataStreamAction.java @@ -38,6 +38,7 @@ import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.Priority; import org.elasticsearch.common.Strings; +import org.elasticsearch.common.ValidationException; import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; @@ -47,7 +48,10 @@ import org.elasticsearch.transport.TransportService; import java.io.IOException; +import java.util.ArrayList; import java.util.Collections; +import java.util.List; +import java.util.Locale; import java.util.Objects; public class CreateDataStreamAction extends ActionType { @@ -161,15 +165,63 @@ public void clusterStateProcessed(String source, ClusterState oldState, ClusterS } static ClusterState createDataStream(ClusterState currentState, Request request) { + List validationErrors = new ArrayList<>(); if (currentState.metaData().dataStreams().containsKey(request.name)) { - throw new IllegalArgumentException("data_stream [" + request.name + "] already exists"); + validationErrors.add("data_stream [" + request.name + "] already exists"); } - MetaData.Builder builder = MetaData.builder(currentState.metaData()).put( - new DataStream(request.name, request.timestampFieldName, Collections.emptyList())); + validationErrors.addAll(validateDataStreamName(request.name)); - logger.info("adding data stream [{}]", request.name); - return ClusterState.builder(currentState).metaData(builder).build(); + if (currentState.metaData().hasIndex(request.name)) { + validationErrors.add("data_stream [" + request.name + "] conflicts with existing index"); + } + + if (currentState.metaData().hasAlias(request.name)) { + validationErrors.add("data_stream [" + request.name + "] conflicts with existing alias"); + } + + final String backingIndexPrefix = (request.name.startsWith(".") ? "" : ".") + request.name + "-"; + for (String indexName : currentState.metaData().getConcreteAllIndices()) { + if (indexName.startsWith(backingIndexPrefix)) { + validationErrors.add( + "data_stream [" + request.name + "] could create backing indices that conflict with existing indices"); + break; + } + } + + if (validationErrors.isEmpty()) { + MetaData.Builder builder = MetaData.builder(currentState.metaData()).put( + new DataStream(request.name, request.timestampFieldName, Collections.emptyList())); + logger.info("adding data stream [{}]", request.name); + return ClusterState.builder(currentState).metaData(builder).build(); + } else { + ValidationException ex = new ValidationException(); + ex.addValidationErrors(validationErrors); + throw new IllegalArgumentException(ex); + } + } + + private static List validateDataStreamName(String name) { + List validationErrors = new ArrayList<>(); + if (name.contains(" ")) { + validationErrors.add("name must not contain a space"); + } + if (name.contains(",")) { + validationErrors.add("name must not contain a ','"); + } + if (name.contains("#")) { + validationErrors.add("name must not contain a '#'"); + } + if (name.startsWith("_")) { + validationErrors.add("name must not start with '_'"); + } + if (name.endsWith("-")) { + validationErrors.add("name must not end with '-'"); + } + if (name.toLowerCase(Locale.ROOT).equals(name) == false) { + validationErrors.add("name must be lower cased"); + } + return validationErrors; } @Override diff --git a/server/src/test/java/org/elasticsearch/action/admin/indices/datastream/CreateDataStreamRequestTests.java b/server/src/test/java/org/elasticsearch/action/admin/indices/datastream/CreateDataStreamRequestTests.java index 6945811c8df23..95f1f3e391ca1 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/indices/datastream/CreateDataStreamRequestTests.java +++ b/server/src/test/java/org/elasticsearch/action/admin/indices/datastream/CreateDataStreamRequestTests.java @@ -18,11 +18,14 @@ */ package org.elasticsearch.action.admin.indices.datastream; +import org.elasticsearch.Version; import org.elasticsearch.action.ActionRequestValidationException; import org.elasticsearch.action.admin.indices.datastream.CreateDataStreamAction.Request; import org.elasticsearch.cluster.ClusterName; import org.elasticsearch.cluster.ClusterState; +import org.elasticsearch.cluster.metadata.AliasMetaData; import org.elasticsearch.cluster.metadata.DataStream; +import org.elasticsearch.cluster.metadata.IndexMetaData; import org.elasticsearch.cluster.metadata.MetaData; import org.elasticsearch.common.io.stream.Writeable; import org.elasticsearch.test.AbstractWireSerializingTestCase; @@ -82,4 +85,65 @@ public void testCreateDuplicateDataStream() { () -> CreateDataStreamAction.TransportAction.createDataStream(cs, req)); assertThat(e.getMessage(), containsString("data_stream [" + dataStreamName + "] already exists")); } + + public void testCreateDataStreamWithInvalidName() { + final String dataStreamName = "_My-da#ta- ,stream-"; + ClusterState cs = ClusterState.builder(new ClusterName("_name")).build(); + CreateDataStreamAction.Request req = new CreateDataStreamAction.Request(dataStreamName); + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, + () -> CreateDataStreamAction.TransportAction.createDataStream(cs, req)); + assertThat(e.getMessage(), containsString("name must not contain a space")); + assertThat(e.getMessage(), containsString("name must not contain a ','")); + assertThat(e.getMessage(), containsString("name must not contain a '#'")); + assertThat(e.getMessage(), containsString("name must not start with '_'")); + assertThat(e.getMessage(), containsString("name must not end with '-'")); + assertThat(e.getMessage(), containsString("name must be lower cased")); + } + + public void testCreateDataStreamThatConflictsWithIndex() { + final String dataStreamName = "my-data-stream"; + ClusterState cs = ClusterState.builder(new ClusterName("_name")) + .metaData(MetaData.builder().put(IndexMetaData.builder(dataStreamName) + .settings(settings(Version.CURRENT)) + .numberOfShards(1) + .numberOfReplicas(1) + .build(), false).build()).build(); + CreateDataStreamAction.Request req = new CreateDataStreamAction.Request(dataStreamName); + + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, + () -> CreateDataStreamAction.TransportAction.createDataStream(cs, req)); + assertThat(e.getMessage(), containsString("data_stream [" + dataStreamName + "] conflicts with existing index")); + } + + public void testCreateDataStreamThatConflictsWithAlias() { + final String dataStreamName = "my-data-stream"; + ClusterState cs = ClusterState.builder(new ClusterName("_name")) + .metaData(MetaData.builder().put(IndexMetaData.builder(dataStreamName + "z") + .settings(settings(Version.CURRENT)) + .numberOfShards(1) + .numberOfReplicas(1) + .putAlias(AliasMetaData.builder(dataStreamName).build()) + .build(), false).build()).build(); + CreateDataStreamAction.Request req = new CreateDataStreamAction.Request(dataStreamName); + + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, + () -> CreateDataStreamAction.TransportAction.createDataStream(cs, req)); + assertThat(e.getMessage(), containsString("data_stream [" + dataStreamName + "] conflicts with existing alias")); + } + + public void testCreateDataStreamWithConflictingBackingIndices() { + final String dataStreamName = "my-data-stream"; + ClusterState cs = ClusterState.builder(new ClusterName("_name")) + .metaData(MetaData.builder().put(IndexMetaData.builder("." + dataStreamName + "-00001") + .settings(settings(Version.CURRENT)) + .numberOfShards(1) + .numberOfReplicas(1) + .build(), false).build()).build(); + CreateDataStreamAction.Request req = new CreateDataStreamAction.Request(dataStreamName); + + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, + () -> CreateDataStreamAction.TransportAction.createDataStream(cs, req)); + assertThat(e.getMessage(), + containsString("data_stream [" + dataStreamName + "] could create backing indices that conflict with existing indices")); + } } From b2e4483604386c43d7b19a5b768d65917bec9d19 Mon Sep 17 00:00:00 2001 From: Dan Hermann Date: Wed, 25 Mar 2020 12:29:21 -0500 Subject: [PATCH 2/6] move validation to MetaData.Builder --- .../datastream/CreateDataStreamAction.java | 65 +++---------------- .../cluster/metadata/MetaData.java | 22 ++++++- .../CreateDataStreamRequestTests.java | 57 +--------------- .../cluster/metadata/MetaDataTests.java | 45 +++++++++++++ 4 files changed, 73 insertions(+), 116 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/datastream/CreateDataStreamAction.java b/server/src/main/java/org/elasticsearch/action/admin/indices/datastream/CreateDataStreamAction.java index 09057f9225de0..2a43388ed0dda 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/indices/datastream/CreateDataStreamAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/indices/datastream/CreateDataStreamAction.java @@ -35,10 +35,10 @@ import org.elasticsearch.cluster.metadata.DataStream; import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver; import org.elasticsearch.cluster.metadata.MetaData; +import org.elasticsearch.cluster.metadata.MetaDataCreateIndexService; import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.Priority; import org.elasticsearch.common.Strings; -import org.elasticsearch.common.ValidationException; import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; @@ -48,10 +48,7 @@ import org.elasticsearch.transport.TransportService; import java.io.IOException; -import java.util.ArrayList; import java.util.Collections; -import java.util.List; -import java.util.Locale; import java.util.Objects; public class CreateDataStreamAction extends ActionType { @@ -165,63 +162,17 @@ public void clusterStateProcessed(String source, ClusterState oldState, ClusterS } static ClusterState createDataStream(ClusterState currentState, Request request) { - List validationErrors = new ArrayList<>(); if (currentState.metaData().dataStreams().containsKey(request.name)) { - validationErrors.add("data_stream [" + request.name + "] already exists"); + throw new IllegalArgumentException("data_stream [" + request.name + "] already exists"); } - validationErrors.addAll(validateDataStreamName(request.name)); + MetaDataCreateIndexService.validateIndexOrAliasName(request.name, + (s1, s2) -> new IllegalArgumentException("data_stream [" + s1 + "] " + s2)); - if (currentState.metaData().hasIndex(request.name)) { - validationErrors.add("data_stream [" + request.name + "] conflicts with existing index"); - } - - if (currentState.metaData().hasAlias(request.name)) { - validationErrors.add("data_stream [" + request.name + "] conflicts with existing alias"); - } - - final String backingIndexPrefix = (request.name.startsWith(".") ? "" : ".") + request.name + "-"; - for (String indexName : currentState.metaData().getConcreteAllIndices()) { - if (indexName.startsWith(backingIndexPrefix)) { - validationErrors.add( - "data_stream [" + request.name + "] could create backing indices that conflict with existing indices"); - break; - } - } - - if (validationErrors.isEmpty()) { - MetaData.Builder builder = MetaData.builder(currentState.metaData()).put( - new DataStream(request.name, request.timestampFieldName, Collections.emptyList())); - logger.info("adding data stream [{}]", request.name); - return ClusterState.builder(currentState).metaData(builder).build(); - } else { - ValidationException ex = new ValidationException(); - ex.addValidationErrors(validationErrors); - throw new IllegalArgumentException(ex); - } - } - - private static List validateDataStreamName(String name) { - List validationErrors = new ArrayList<>(); - if (name.contains(" ")) { - validationErrors.add("name must not contain a space"); - } - if (name.contains(",")) { - validationErrors.add("name must not contain a ','"); - } - if (name.contains("#")) { - validationErrors.add("name must not contain a '#'"); - } - if (name.startsWith("_")) { - validationErrors.add("name must not start with '_'"); - } - if (name.endsWith("-")) { - validationErrors.add("name must not end with '-'"); - } - if (name.toLowerCase(Locale.ROOT).equals(name) == false) { - validationErrors.add("name must be lower cased"); - } - return validationErrors; + MetaData.Builder builder = MetaData.builder(currentState.metaData()).put( + new DataStream(request.name, request.timestampFieldName, Collections.emptyList())); + logger.info("adding data stream [{}]", request.name); + return ClusterState.builder(currentState).metaData(builder).build(); } @Override diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java index 73ab3ccab7176..5f84b94df541e 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java @@ -1277,7 +1277,7 @@ public Builder generateClusterUuidIfNeeded() { public MetaData build() { // TODO: We should move these datastructures to IndexNameExpressionResolver, this will give the following benefits: - // 1) The datastructures will only be rebuilded when needed. Now during serializing we rebuild these datastructures + // 1) The datastructures will be rebuilt only when needed. Now during serializing we rebuild these datastructures // while these datastructures aren't even used. // 2) The aliasAndIndexLookup can be updated instead of rebuilding it all the time. @@ -1315,7 +1315,7 @@ public MetaData build() { // iterate again and constructs a helpful message ArrayList duplicates = new ArrayList<>(); for (ObjectCursor cursor : indices.values()) { - for (String alias: duplicateAliasesIndices) { + for (String alias : duplicateAliasesIndices) { if (cursor.value.getAliases().containsKey(alias)) { duplicates.add(alias + " (alias of " + cursor.value.getIndex() + ")"); } @@ -1323,12 +1323,28 @@ public MetaData build() { } assert duplicates.size() > 0; throw new IllegalStateException("index and alias names need to be unique, but the following duplicates were found [" - + Strings.collectionToCommaDelimitedString(duplicates)+ "]"); + + Strings.collectionToCommaDelimitedString(duplicates) + "]"); } SortedMap aliasAndIndexLookup = Collections.unmodifiableSortedMap(buildAliasAndIndexLookup()); + DataStreamMetadata dsMetadata = (DataStreamMetadata) customs.get(DataStreamMetadata.TYPE); + if (dsMetadata != null) { + for (DataStream ds : dsMetadata.dataStreams().values()) { + if (aliasAndIndexLookup.containsKey(ds.getName())) { + throw new IllegalStateException("data stream [" + ds.getName() + "] conflicts with existing index or alias"); + } + + final String backingIndexPrefix = (ds.getName().startsWith(".") ? "" : ".") + ds.getName() + "-"; + for (String indexName : allIndices) { + if (indexName.startsWith(backingIndexPrefix)) { + throw new IllegalStateException( + "data stream [" + ds.getName() + "] could create backing indices that conflict with existing indices"); + } + } + } + } // build all concrete indices arrays: // TODO: I think we can remove these arrays. it isn't worth the effort, for operations on all indices. diff --git a/server/src/test/java/org/elasticsearch/action/admin/indices/datastream/CreateDataStreamRequestTests.java b/server/src/test/java/org/elasticsearch/action/admin/indices/datastream/CreateDataStreamRequestTests.java index 95f1f3e391ca1..8160b0d51ef73 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/indices/datastream/CreateDataStreamRequestTests.java +++ b/server/src/test/java/org/elasticsearch/action/admin/indices/datastream/CreateDataStreamRequestTests.java @@ -18,14 +18,11 @@ */ package org.elasticsearch.action.admin.indices.datastream; -import org.elasticsearch.Version; import org.elasticsearch.action.ActionRequestValidationException; import org.elasticsearch.action.admin.indices.datastream.CreateDataStreamAction.Request; import org.elasticsearch.cluster.ClusterName; import org.elasticsearch.cluster.ClusterState; -import org.elasticsearch.cluster.metadata.AliasMetaData; import org.elasticsearch.cluster.metadata.DataStream; -import org.elasticsearch.cluster.metadata.IndexMetaData; import org.elasticsearch.cluster.metadata.MetaData; import org.elasticsearch.common.io.stream.Writeable; import org.elasticsearch.test.AbstractWireSerializingTestCase; @@ -92,58 +89,6 @@ public void testCreateDataStreamWithInvalidName() { CreateDataStreamAction.Request req = new CreateDataStreamAction.Request(dataStreamName); IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> CreateDataStreamAction.TransportAction.createDataStream(cs, req)); - assertThat(e.getMessage(), containsString("name must not contain a space")); - assertThat(e.getMessage(), containsString("name must not contain a ','")); - assertThat(e.getMessage(), containsString("name must not contain a '#'")); - assertThat(e.getMessage(), containsString("name must not start with '_'")); - assertThat(e.getMessage(), containsString("name must not end with '-'")); - assertThat(e.getMessage(), containsString("name must be lower cased")); - } - - public void testCreateDataStreamThatConflictsWithIndex() { - final String dataStreamName = "my-data-stream"; - ClusterState cs = ClusterState.builder(new ClusterName("_name")) - .metaData(MetaData.builder().put(IndexMetaData.builder(dataStreamName) - .settings(settings(Version.CURRENT)) - .numberOfShards(1) - .numberOfReplicas(1) - .build(), false).build()).build(); - CreateDataStreamAction.Request req = new CreateDataStreamAction.Request(dataStreamName); - - IllegalArgumentException e = expectThrows(IllegalArgumentException.class, - () -> CreateDataStreamAction.TransportAction.createDataStream(cs, req)); - assertThat(e.getMessage(), containsString("data_stream [" + dataStreamName + "] conflicts with existing index")); - } - - public void testCreateDataStreamThatConflictsWithAlias() { - final String dataStreamName = "my-data-stream"; - ClusterState cs = ClusterState.builder(new ClusterName("_name")) - .metaData(MetaData.builder().put(IndexMetaData.builder(dataStreamName + "z") - .settings(settings(Version.CURRENT)) - .numberOfShards(1) - .numberOfReplicas(1) - .putAlias(AliasMetaData.builder(dataStreamName).build()) - .build(), false).build()).build(); - CreateDataStreamAction.Request req = new CreateDataStreamAction.Request(dataStreamName); - - IllegalArgumentException e = expectThrows(IllegalArgumentException.class, - () -> CreateDataStreamAction.TransportAction.createDataStream(cs, req)); - assertThat(e.getMessage(), containsString("data_stream [" + dataStreamName + "] conflicts with existing alias")); - } - - public void testCreateDataStreamWithConflictingBackingIndices() { - final String dataStreamName = "my-data-stream"; - ClusterState cs = ClusterState.builder(new ClusterName("_name")) - .metaData(MetaData.builder().put(IndexMetaData.builder("." + dataStreamName + "-00001") - .settings(settings(Version.CURRENT)) - .numberOfShards(1) - .numberOfReplicas(1) - .build(), false).build()).build(); - CreateDataStreamAction.Request req = new CreateDataStreamAction.Request(dataStreamName); - - IllegalArgumentException e = expectThrows(IllegalArgumentException.class, - () -> CreateDataStreamAction.TransportAction.createDataStream(cs, req)); - assertThat(e.getMessage(), - containsString("data_stream [" + dataStreamName + "] could create backing indices that conflict with existing indices")); + assertThat(e.getMessage(), containsString("must not contain the following characters")); } } diff --git a/server/src/test/java/org/elasticsearch/cluster/metadata/MetaDataTests.java b/server/src/test/java/org/elasticsearch/cluster/metadata/MetaDataTests.java index e3099ab8f9de3..34091c13a318b 100644 --- a/server/src/test/java/org/elasticsearch/cluster/metadata/MetaDataTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/metadata/MetaDataTests.java @@ -45,6 +45,7 @@ import java.io.IOException; import java.util.Arrays; +import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -908,6 +909,50 @@ public void testBuilderRejectsNullInCustoms() { assertThat(expectThrows(NullPointerException.class, () -> builder.customs(map)).getMessage(), containsString(key)); } + public void testBuilderRejectsDataStreamThatConflictsWithIndex() { + final String dataStreamName = "my-data-stream"; + MetaData.Builder b = MetaData.builder() + .put(IndexMetaData.builder(dataStreamName) + .settings(settings(Version.CURRENT)) + .numberOfShards(1) + .numberOfReplicas(1) + .build(), false) + .put(new DataStream(dataStreamName, "ts", Collections.emptyList())); + + IllegalStateException e = expectThrows(IllegalStateException.class, b::build); + assertThat(e.getMessage(), containsString("data stream [" + dataStreamName + "] conflicts with existing index or alias")); + } + + public void testBuilderRejectsDataStreamThatConflictsWithAlias() { + final String dataStreamName = "my-data-stream"; + MetaData.Builder b = MetaData.builder() + .put(IndexMetaData.builder(dataStreamName + "z") + .settings(settings(Version.CURRENT)) + .numberOfShards(1) + .numberOfReplicas(1) + .putAlias(AliasMetaData.builder(dataStreamName).build()) + .build(), false) + .put(new DataStream(dataStreamName, "ts", Collections.emptyList())); + + IllegalStateException e = expectThrows(IllegalStateException.class, b::build); + assertThat(e.getMessage(), containsString("data stream [" + dataStreamName + "] conflicts with existing index or alias")); + } + + public void testBuilderRejectsDataStreamWithConflictingBackingIndices() { + final String dataStreamName = "my-data-stream"; + MetaData.Builder b = MetaData.builder() + .put(IndexMetaData.builder("." + dataStreamName + "-00001") + .settings(settings(Version.CURRENT)) + .numberOfShards(1) + .numberOfReplicas(1) + .build(), false) + .put(new DataStream(dataStreamName, "ts", Collections.emptyList())); + + IllegalStateException e = expectThrows(IllegalStateException.class, b::build); + assertThat(e.getMessage(), + containsString("data stream [" + dataStreamName + "] could create backing indices that conflict with existing indices")); + } + public void testSerialization() throws IOException { final MetaData orig = randomMetaData(); final BytesStreamOutput out = new BytesStreamOutput(); From b67088e0a4aafb983045c0a2f1e3048561e712a4 Mon Sep 17 00:00:00 2001 From: Dan Hermann Date: Wed, 25 Mar 2020 16:03:10 -0500 Subject: [PATCH 3/6] review comments --- .../cluster/metadata/MetaData.java | 36 ++++++++++--------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java index 641875d46f151..1baf59d455b3d 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java @@ -1329,22 +1329,7 @@ public MetaData build() { SortedMap aliasAndIndexLookup = Collections.unmodifiableSortedMap(buildAliasAndIndexLookup()); - DataStreamMetadata dsMetadata = (DataStreamMetadata) customs.get(DataStreamMetadata.TYPE); - if (dsMetadata != null) { - for (DataStream ds : dsMetadata.dataStreams().values()) { - if (aliasAndIndexLookup.containsKey(ds.getName())) { - throw new IllegalStateException("data stream [" + ds.getName() + "] conflicts with existing index or alias"); - } - - final String backingIndexPrefix = (ds.getName().startsWith(".") ? "" : ".") + ds.getName() + "-"; - for (String indexName : allIndices) { - if (indexName.startsWith(backingIndexPrefix)) { - throw new IllegalStateException( - "data stream [" + ds.getName() + "] could create backing indices that conflict with existing indices"); - } - } - } - } + validateDataStreams(aliasAndIndexLookup); // build all concrete indices arrays: // TODO: I think we can remove these arrays. it isn't worth the effort, for operations on all indices. @@ -1387,6 +1372,25 @@ private SortedMap buildAliasAndIndexLookup() { return aliasAndIndexLookup; } + private void validateDataStreams(SortedMap aliasAndIndexLookup) { + DataStreamMetadata dsMetadata = (DataStreamMetadata) customs.get(DataStreamMetadata.TYPE); + if (dsMetadata != null) { + for (DataStream ds : dsMetadata.dataStreams().values()) { + if (aliasAndIndexLookup.containsKey(ds.getName())) { + throw new IllegalStateException("data stream [" + ds.getName() + "] conflicts with existing index or alias"); + } + + final String backingIndexPrefixFrom = (ds.getName().startsWith(".") ? "" : ".") + ds.getName() + "-"; + final String backingIndexPrefixTo = (ds.getName().startsWith(".") ? "" : ".") + ds.getName() + "."; + SortedMap map = aliasAndIndexLookup.subMap(backingIndexPrefixFrom, backingIndexPrefixTo); + if (map.size() != 0) { + throw new IllegalStateException( + "data stream [" + ds.getName() + "] could create backing indices that conflict with existing indices"); + } + } + } + } + public static void toXContent(MetaData metaData, XContentBuilder builder, ToXContent.Params params) throws IOException { XContentContext context = XContentContext.valueOf(params.param(CONTEXT_MODE_PARAM, CONTEXT_MODE_API)); From 3b56ebf39343226d2083bd08afe357b8e300f6e4 Mon Sep 17 00:00:00 2001 From: Dan Hermann Date: Wed, 25 Mar 2020 16:21:58 -0500 Subject: [PATCH 4/6] remove dot logic --- .../java/org/elasticsearch/cluster/metadata/MetaData.java | 8 +++----- .../org/elasticsearch/cluster/metadata/MetaDataTests.java | 6 +++--- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java index 1baf59d455b3d..68939a53cd442 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java @@ -1380,12 +1380,10 @@ private void validateDataStreams(SortedMap aliasAndIndexLo throw new IllegalStateException("data stream [" + ds.getName() + "] conflicts with existing index or alias"); } - final String backingIndexPrefixFrom = (ds.getName().startsWith(".") ? "" : ".") + ds.getName() + "-"; - final String backingIndexPrefixTo = (ds.getName().startsWith(".") ? "" : ".") + ds.getName() + "."; - SortedMap map = aliasAndIndexLookup.subMap(backingIndexPrefixFrom, backingIndexPrefixTo); + SortedMap map = aliasAndIndexLookup.subMap(ds.getName() + "-", ds.getName() + "."); // '.' is the char after '-' if (map.size() != 0) { - throw new IllegalStateException( - "data stream [" + ds.getName() + "] could create backing indices that conflict with existing indices"); + throw new IllegalStateException("data stream [" + ds.getName() + + "] could create backing indices that conflict with existing indices or aliases"); } } } diff --git a/server/src/test/java/org/elasticsearch/cluster/metadata/MetaDataTests.java b/server/src/test/java/org/elasticsearch/cluster/metadata/MetaDataTests.java index 5cda0f43e928a..7a837692db052 100644 --- a/server/src/test/java/org/elasticsearch/cluster/metadata/MetaDataTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/metadata/MetaDataTests.java @@ -940,7 +940,7 @@ public void testBuilderRejectsDataStreamThatConflictsWithAlias() { public void testBuilderRejectsDataStreamWithConflictingBackingIndices() { final String dataStreamName = "my-data-stream"; MetaData.Builder b = MetaData.builder() - .put(IndexMetaData.builder("." + dataStreamName + "-00001") + .put(IndexMetaData.builder(dataStreamName + "-00001") .settings(settings(Version.CURRENT)) .numberOfShards(1) .numberOfReplicas(1) @@ -948,8 +948,8 @@ public void testBuilderRejectsDataStreamWithConflictingBackingIndices() { .put(new DataStream(dataStreamName, "ts", Collections.emptyList())); IllegalStateException e = expectThrows(IllegalStateException.class, b::build); - assertThat(e.getMessage(), - containsString("data stream [" + dataStreamName + "] could create backing indices that conflict with existing indices")); + assertThat(e.getMessage(), containsString("data stream [" + dataStreamName + + "] could create backing indices that conflict with existing indices or aliases")); } public void testSerialization() throws IOException { From 4846c983feaa48cd6a41339030904437be39000d Mon Sep 17 00:00:00 2001 From: Dan Hermann Date: Thu, 26 Mar 2020 15:31:33 -0500 Subject: [PATCH 5/6] review comments --- .../test/indices.data_stream/10_basic.yml | 16 ++++++++++++++++ .../elasticsearch/cluster/metadata/MetaData.java | 3 ++- .../cluster/metadata/MetaDataTests.java | 5 +++-- 3 files changed, 21 insertions(+), 3 deletions(-) diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/indices.data_stream/10_basic.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/indices.data_stream/10_basic.yml index 2861c443bfa8e..80f4fb234b3b9 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/indices.data_stream/10_basic.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/indices.data_stream/10_basic.yml @@ -31,3 +31,19 @@ indices.delete_data_stream: name: simple-data-stream2 - is_true: acknowledged + +--- +"Create data stream with invalid name": + - skip: + version: " - 7.6.99" + reason: available only in 7.7+ + + - do: + catch: bad_request + indices.create_data_stream: + name: invalid-data-stream#-name + body: + timestamp_field: "@timestamp" + + - match: { status: 400 } + - match: { error.root_cause.0.type: "illegal_argument_exception" } diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java index 68939a53cd442..2e32ad47cfdd8 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java @@ -1383,7 +1383,8 @@ private void validateDataStreams(SortedMap aliasAndIndexLo SortedMap map = aliasAndIndexLookup.subMap(ds.getName() + "-", ds.getName() + "."); // '.' is the char after '-' if (map.size() != 0) { throw new IllegalStateException("data stream [" + ds.getName() + - "] could create backing indices that conflict with existing indices or aliases"); + "] could create backing indices that conflict with " + map.size() + " existing index(s) or alias(s)" + + " including '" + map.firstKey() + "'"); } } } diff --git a/server/src/test/java/org/elasticsearch/cluster/metadata/MetaDataTests.java b/server/src/test/java/org/elasticsearch/cluster/metadata/MetaDataTests.java index 7a837692db052..a6d2fb9183a07 100644 --- a/server/src/test/java/org/elasticsearch/cluster/metadata/MetaDataTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/metadata/MetaDataTests.java @@ -939,8 +939,9 @@ public void testBuilderRejectsDataStreamThatConflictsWithAlias() { public void testBuilderRejectsDataStreamWithConflictingBackingIndices() { final String dataStreamName = "my-data-stream"; + final String conflictingIndex = dataStreamName + "-00001"; MetaData.Builder b = MetaData.builder() - .put(IndexMetaData.builder(dataStreamName + "-00001") + .put(IndexMetaData.builder(conflictingIndex) .settings(settings(Version.CURRENT)) .numberOfShards(1) .numberOfReplicas(1) @@ -949,7 +950,7 @@ public void testBuilderRejectsDataStreamWithConflictingBackingIndices() { IllegalStateException e = expectThrows(IllegalStateException.class, b::build); assertThat(e.getMessage(), containsString("data stream [" + dataStreamName + - "] could create backing indices that conflict with existing indices or aliases")); + "] could create backing indices that conflict with 1 existing index(s) or alias(s) including '" + conflictingIndex + "'")); } public void testSerialization() throws IOException { From 7a6953101fbee2ced4d813ad54ac30a08c2a1b2d Mon Sep 17 00:00:00 2001 From: Dan Hermann Date: Thu, 26 Mar 2020 16:37:45 -0500 Subject: [PATCH 6/6] will add new test in separate PR --- .../test/indices.data_stream/10_basic.yml | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/indices.data_stream/10_basic.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/indices.data_stream/10_basic.yml index 80f4fb234b3b9..2861c443bfa8e 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/indices.data_stream/10_basic.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/indices.data_stream/10_basic.yml @@ -31,19 +31,3 @@ indices.delete_data_stream: name: simple-data-stream2 - is_true: acknowledged - ---- -"Create data stream with invalid name": - - skip: - version: " - 7.6.99" - reason: available only in 7.7+ - - - do: - catch: bad_request - indices.create_data_stream: - name: invalid-data-stream#-name - body: - timestamp_field: "@timestamp" - - - match: { status: 400 } - - match: { error.root_cause.0.type: "illegal_argument_exception" }