Skip to content
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

Validation for data stream creation #54083

Merged
merged 9 commits into from
Mar 26, 2020
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
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;
Expand Down Expand Up @@ -165,9 +166,11 @@ static ClusterState createDataStream(ClusterState currentState, Request request)
throw new IllegalArgumentException("data_stream [" + request.name + "] already exists");
}

MetaDataCreateIndexService.validateIndexOrAliasName(request.name,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we rename the method to either "validateIndexSpaceName" or "validateAbstractIndexName"? Can be done in a follow-up when the naming discussion is clarified.

(s1, s2) -> new IllegalArgumentException("data_stream [" + s1 + "] " + s2));
Copy link
Contributor

Choose a reason for hiding this comment

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

The other usages of validateIndexOrAliasName throw a dedicated exception. I am not entirely sure if IllegalArgumentException results in the same RestStatus and message handling. Could we add this to the rest test suite?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a REST test to enforce that the same HTTP 400 response code is returned.


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();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down Expand Up @@ -1315,20 +1315,21 @@ public MetaData build() {
// iterate again and constructs a helpful message
ArrayList<String> duplicates = new ArrayList<>();
for (ObjectCursor<IndexMetaData> 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() + ")");
}
}
}
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<String, AliasOrIndex> aliasAndIndexLookup = Collections.unmodifiableSortedMap(buildAliasAndIndexLookup());

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.
Expand Down Expand Up @@ -1371,6 +1372,23 @@ private SortedMap<String, AliasOrIndex> buildAliasAndIndexLookup() {
return aliasAndIndexLookup;
}

private void validateDataStreams(SortedMap<String, AliasOrIndex> 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");
}

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");
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we include the list of conflicting names in the message or at least the first few?

}
}
}
}

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));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,4 +82,13 @@ 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("must not contain the following characters"));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,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;
Expand Down Expand Up @@ -907,6 +908,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 or aliases"));
}

public void testSerialization() throws IOException {
final MetaData orig = randomMetaData();
final BytesStreamOutput out = new BytesStreamOutput();
Expand Down