From 8370a0c24db68b3d040a77f8ad859e35a8edf067 Mon Sep 17 00:00:00 2001 From: Gordon Brown Date: Tue, 30 Oct 2018 13:28:08 -0600 Subject: [PATCH 01/11] Enforce index-name rules for ILM policy names For consistency, ILM policy names must now follow the same rules as index names. --- .../core/indexlifecycle/LifecyclePolicy.java | 40 +++++++++++++++++++ .../indexlifecycle/LifecyclePolicyTests.java | 18 +++++++++ .../TimeSeriesLifecycleActionsIT.java | 36 +++++++++++++++++ .../action/TransportPutLifecycleAction.java | 4 +- 4 files changed, 97 insertions(+), 1 deletion(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicy.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicy.java index a56818355c3e5..3a9fa361f4c49 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicy.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicy.java @@ -7,6 +7,7 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import org.elasticsearch.ElasticsearchException; import org.elasticsearch.client.Client; import org.elasticsearch.cluster.AbstractDiffable; import org.elasticsearch.cluster.Diffable; @@ -21,12 +22,14 @@ import org.elasticsearch.xpack.core.indexlifecycle.Step.StepKey; import java.io.IOException; +import java.io.UnsupportedEncodingException; import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.ListIterator; import java.util.Map; import java.util.Objects; +import java.util.function.BiFunction; import java.util.function.Function; import java.util.stream.Collectors; @@ -40,6 +43,7 @@ public class LifecyclePolicy extends AbstractDiffable implements ToXContentObject, Diffable { private static final Logger logger = LogManager.getLogger(LifecyclePolicy.class); + private static final int MAX_INDEX_NAME_BYTES = 255; public static final ParseField PHASES_FIELD = new ParseField("phases"); @@ -241,6 +245,42 @@ public boolean isActionSafe(StepKey stepKey) { } } + /** + * Validate the name for an policy against some static rules. Intended to match + * {@link org.elasticsearch.cluster.metadata.MetaDataCreateIndexService#validateIndexOrAliasName(String, BiFunction)} + * @param policy the policy name to validate + * @throws IllegalArgumentException if the name is invalid + */ + public static void validatePolicyName(String policy) { + if (!Strings.validFileName(policy)) { + throw new IllegalArgumentException("invalid policy name [" + policy + "]: must not contain the following characters " + + Strings.INVALID_FILENAME_CHARS); + } + if (policy.contains("#")) { + throw new IllegalArgumentException("invalid policy name [" + policy + "]: must not contain '#'"); + } + if (policy.contains(":")) { + throw new IllegalArgumentException("invalid policy name [" + policy + "]: must not contain ':'"); + } + if (policy.charAt(0) == '_' || policy.charAt(0) == '-' || policy.charAt(0) == '+') { + throw new IllegalArgumentException("invalid policy name [" + policy + "]: must not start with '_', '-', or '+'"); + } + int byteCount = 0; + try { + byteCount = policy.getBytes("UTF-8").length; + } catch (UnsupportedEncodingException e) { + // UTF-8 should always be supported, but rethrow this if it is not for some reason + throw new ElasticsearchException("Unable to determine length of index name", e); + } + if (byteCount > MAX_INDEX_NAME_BYTES) { + throw new IllegalArgumentException("invalid policy name [" + policy + "]: name is too long, (" + byteCount + " > " + + MAX_INDEX_NAME_BYTES + ")"); + } + if (policy.equals(".") || policy.equals("..")) { + throw new IllegalArgumentException("invalid policy name [" + policy + "]: must not be '.' or '..'"); + } + } + @Override public int hashCode() { return Objects.hash(name, phases); diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicyTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicyTests.java index 9d90cc025b0e3..548104c166c73 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicyTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicyTests.java @@ -9,6 +9,7 @@ import org.elasticsearch.cluster.ClusterModule; import org.elasticsearch.common.Nullable; import org.elasticsearch.common.ParseField; +import org.elasticsearch.common.Strings; import org.elasticsearch.common.io.stream.NamedWriteableRegistry; import org.elasticsearch.common.io.stream.Writeable.Reader; import org.elasticsearch.common.unit.TimeValue; @@ -316,4 +317,21 @@ public void testIsActionSafe() { assertTrue(policy.isActionSafe(new StepKey("new", randomAlphaOfLength(10), randomAlphaOfLength(10)))); } + + public void testValidatePolicyName() { + for (Character badChar : Strings.INVALID_FILENAME_CHARS) { + expectThrows(IllegalArgumentException.class, () -> LifecyclePolicy.validatePolicyName(randomAlphaOfLengthBetween(0,10) + + badChar + randomAlphaOfLengthBetween(0,10))); + } + expectThrows(IllegalArgumentException.class, () -> LifecyclePolicy.validatePolicyName(randomAlphaOfLengthBetween(0,10) + + "#" + randomAlphaOfLengthBetween(0,10))); + expectThrows(IllegalArgumentException.class, () -> LifecyclePolicy.validatePolicyName(randomAlphaOfLengthBetween(0,10) + + ":" + randomAlphaOfLengthBetween(0,10))); + expectThrows(IllegalArgumentException.class, () -> LifecyclePolicy.validatePolicyName("_" + randomAlphaOfLengthBetween(1, 20))); + expectThrows(IllegalArgumentException.class, () -> LifecyclePolicy.validatePolicyName("-" + randomAlphaOfLengthBetween(1, 20))); + expectThrows(IllegalArgumentException.class, () -> LifecyclePolicy.validatePolicyName("+" + randomAlphaOfLengthBetween(1, 20))); + expectThrows(IllegalArgumentException.class, () -> LifecyclePolicy.validatePolicyName(".")); + expectThrows(IllegalArgumentException.class, () -> LifecyclePolicy.validatePolicyName("..")); + expectThrows(IllegalArgumentException.class, () -> LifecyclePolicy.validatePolicyName(randomAlphaOfLengthBetween(256, 1000))); + } } diff --git a/x-pack/plugin/ilm/qa/multi-node/src/test/java/org/elasticsearch/xpack/indexlifecycle/TimeSeriesLifecycleActionsIT.java b/x-pack/plugin/ilm/qa/multi-node/src/test/java/org/elasticsearch/xpack/indexlifecycle/TimeSeriesLifecycleActionsIT.java index 3b455a2287d2f..e55e23c934c9e 100644 --- a/x-pack/plugin/ilm/qa/multi-node/src/test/java/org/elasticsearch/xpack/indexlifecycle/TimeSeriesLifecycleActionsIT.java +++ b/x-pack/plugin/ilm/qa/multi-node/src/test/java/org/elasticsearch/xpack/indexlifecycle/TimeSeriesLifecycleActionsIT.java @@ -35,6 +35,8 @@ import java.io.IOException; import java.io.InputStream; +import java.io.UnsupportedEncodingException; +import java.net.URLEncoder; import java.util.Collections; import java.util.HashMap; import java.util.List; @@ -44,6 +46,7 @@ import static java.util.Collections.singletonMap; import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.greaterThan; import static org.hamcrest.Matchers.not; @@ -345,6 +348,39 @@ public void testNonexistentPolicy() throws Exception { } + public void testInvalidPolicyNames() throws UnsupportedEncodingException { + ResponseException ex; + for (Character badChar : Strings.INVALID_FILENAME_CHARS) { + policy = URLEncoder.encode(randomAlphaOfLengthBetween(0,10) + badChar + randomAlphaOfLengthBetween(0,10), "UTF-8"); + ex = expectThrows(ResponseException.class, () -> createNewSingletonPolicy("delete", new DeleteAction())); + assertThat(ex.getCause().getMessage(), containsString("invalid policy name")); + } + + policy = randomAlphaOfLengthBetween(0,10) + ":" + randomAlphaOfLengthBetween(0,10); + ex = expectThrows(ResponseException.class, () -> createNewSingletonPolicy("delete", new DeleteAction())); + assertThat(ex.getMessage(), containsString("invalid policy name")); + policy = "_" + randomAlphaOfLengthBetween(1, 20); + ex = expectThrows(ResponseException.class, () -> createNewSingletonPolicy("delete", new DeleteAction())); + assertThat(ex.getMessage(), containsString("invalid policy name")); + policy = "-" + randomAlphaOfLengthBetween(1, 20); + ex = expectThrows(ResponseException.class, () -> createNewSingletonPolicy("delete", new DeleteAction())); + assertThat(ex.getMessage(), containsString("invalid policy name")); + policy = "+" + randomAlphaOfLengthBetween(1, 20); + ex = expectThrows(ResponseException.class, () -> createNewSingletonPolicy("delete", new DeleteAction())); + assertThat(ex.getMessage(), containsString("invalid policy name")); + + policy = "."; + ex = expectThrows(ResponseException.class, () -> createNewSingletonPolicy("delete", new DeleteAction())); + assertThat(ex.getMessage(), containsString("invalid policy name")); + policy = ".."; + ex = expectThrows(ResponseException.class, () -> createNewSingletonPolicy("delete", new DeleteAction())); + assertThat(ex.getMessage(), containsString("invalid policy name")); + + policy = randomAlphaOfLengthBetween(256, 1000); + ex = expectThrows(ResponseException.class, () -> createNewSingletonPolicy("delete", new DeleteAction())); + assertThat(ex.getMessage(), containsString("invalid policy name")); + } + private void createFullPolicy(TimeValue hotTime) throws IOException { Map warmActions = new HashMap<>(); warmActions.put(ForceMergeAction.NAME, new ForceMergeAction(1)); diff --git a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/indexlifecycle/action/TransportPutLifecycleAction.java b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/indexlifecycle/action/TransportPutLifecycleAction.java index 2a56f179f39a5..9d3dc8fbca190 100644 --- a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/indexlifecycle/action/TransportPutLifecycleAction.java +++ b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/indexlifecycle/action/TransportPutLifecycleAction.java @@ -18,12 +18,13 @@ import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.xpack.core.indexlifecycle.OperationMode; import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.transport.TransportService; import org.elasticsearch.xpack.core.ClientHelper; import org.elasticsearch.xpack.core.indexlifecycle.IndexLifecycleMetadata; +import org.elasticsearch.xpack.core.indexlifecycle.LifecyclePolicy; import org.elasticsearch.xpack.core.indexlifecycle.LifecyclePolicyMetadata; +import org.elasticsearch.xpack.core.indexlifecycle.OperationMode; import org.elasticsearch.xpack.core.indexlifecycle.action.PutLifecycleAction; import org.elasticsearch.xpack.core.indexlifecycle.action.PutLifecycleAction.Request; import org.elasticsearch.xpack.core.indexlifecycle.action.PutLifecycleAction.Response; @@ -66,6 +67,7 @@ protected void masterOperation(Request request, ClusterState state, ActionListen Map filteredHeaders = threadPool.getThreadContext().getHeaders().entrySet().stream() .filter(e -> ClientHelper.SECURITY_HEADER_FILTERS.contains(e.getKey())) .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)); + LifecyclePolicy.validatePolicyName(request.getPolicy().getName()); clusterService.submitStateUpdateTask("put-lifecycle-" + request.getPolicy().getName(), new AckedClusterStateUpdateTask(request, listener) { @Override From 782379d2e21d4e9720b345717ad6677245afba7b Mon Sep 17 00:00:00 2001 From: Gordon Brown Date: Tue, 30 Oct 2018 13:53:34 -0600 Subject: [PATCH 02/11] Missed one "index"->"policy" --- .../xpack/core/indexlifecycle/LifecyclePolicy.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicy.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicy.java index 3a9fa361f4c49..89824f259dd42 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicy.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicy.java @@ -270,7 +270,7 @@ public static void validatePolicyName(String policy) { byteCount = policy.getBytes("UTF-8").length; } catch (UnsupportedEncodingException e) { // UTF-8 should always be supported, but rethrow this if it is not for some reason - throw new ElasticsearchException("Unable to determine length of index name", e); + throw new ElasticsearchException("Unable to determine length of policy name", e); } if (byteCount > MAX_INDEX_NAME_BYTES) { throw new IllegalArgumentException("invalid policy name [" + policy + "]: name is too long, (" + byteCount + " > " + From 27ce24b75c2be1a904b67caac72a44b0d59cb651 Mon Sep 17 00:00:00 2001 From: Gordon Brown Date: Wed, 31 Oct 2018 13:29:26 -0600 Subject: [PATCH 03/11] Add tests for valid policy names --- .../xpack/core/indexlifecycle/LifecyclePolicyTests.java | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicyTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicyTests.java index 548104c166c73..01c41fba76f41 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicyTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicyTests.java @@ -333,5 +333,14 @@ public void testValidatePolicyName() { expectThrows(IllegalArgumentException.class, () -> LifecyclePolicy.validatePolicyName(".")); expectThrows(IllegalArgumentException.class, () -> LifecyclePolicy.validatePolicyName("..")); expectThrows(IllegalArgumentException.class, () -> LifecyclePolicy.validatePolicyName(randomAlphaOfLengthBetween(256, 1000))); + + LifecyclePolicy.validatePolicyName(randomAlphaOfLengthBetween(1,10) + "_" + randomAlphaOfLengthBetween(0,10)); + LifecyclePolicy.validatePolicyName(randomAlphaOfLengthBetween(1,10) + "-" + randomAlphaOfLengthBetween(0,10)); + LifecyclePolicy.validatePolicyName(randomAlphaOfLengthBetween(1,10) + "+" + randomAlphaOfLengthBetween(0,10)); + + LifecyclePolicy.validatePolicyName(randomAlphaOfLengthBetween(0,10) + "." + randomAlphaOfLengthBetween(1,10)); + LifecyclePolicy.validatePolicyName(randomAlphaOfLengthBetween(0,10) + ".." + randomAlphaOfLengthBetween(1,10)); + + LifecyclePolicy.validatePolicyName(randomAlphaOfLengthBetween(1,255)); } } From 4e3d9894592b70c24da557e6b4e42a9eefcfc9b1 Mon Sep 17 00:00:00 2001 From: Gordon Brown Date: Wed, 31 Oct 2018 18:54:47 -0600 Subject: [PATCH 04/11] Refactor name checks into a separate class --- .../cluster/metadata/AliasValidator.java | 3 +- .../metadata/MetaDataCreateIndexService.java | 3 +- .../java/org/elasticsearch/common/Names.java | 66 +++++++++++++++++++ .../org/elasticsearch/common/NamesTests.java | 65 ++++++++++++++++++ .../elasticsearch/indexing/IndexActionIT.java | 6 +- .../core/indexlifecycle/LifecyclePolicy.java | 40 ----------- .../indexlifecycle/LifecyclePolicyTests.java | 27 -------- .../action/TransportPutLifecycleAction.java | 6 +- 8 files changed, 142 insertions(+), 74 deletions(-) create mode 100644 server/src/main/java/org/elasticsearch/common/Names.java create mode 100644 server/src/test/java/org/elasticsearch/common/NamesTests.java diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/AliasValidator.java b/server/src/main/java/org/elasticsearch/cluster/metadata/AliasValidator.java index 766b35307cdd2..4e831b35a532d 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/AliasValidator.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/AliasValidator.java @@ -20,6 +20,7 @@ package org.elasticsearch.cluster.metadata; import org.elasticsearch.action.admin.indices.alias.Alias; +import org.elasticsearch.common.Names; import org.elasticsearch.common.Nullable; import org.elasticsearch.common.Strings; import org.elasticsearch.common.component.AbstractComponent; @@ -105,7 +106,7 @@ void validateAliasStandalone(String alias, String indexRouting) { if (!Strings.hasText(alias)) { throw new IllegalArgumentException("alias name is required"); } - MetaDataCreateIndexService.validateIndexOrAliasName(alias, InvalidAliasNameException::new); + Names.validateTopLevelName(alias, InvalidAliasNameException::new); if (indexRouting != null && indexRouting.indexOf(',') != -1) { throw new IllegalArgumentException("alias [" + alias + "] has several index routing values associated with it"); } diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java index d0199e838438e..b4608b90dc0fc 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java @@ -47,6 +47,7 @@ import org.elasticsearch.cluster.routing.ShardRoutingState; import org.elasticsearch.cluster.routing.allocation.AllocationService; import org.elasticsearch.cluster.service.ClusterService; +import org.elasticsearch.common.Names; import org.elasticsearch.common.Priority; import org.elasticsearch.common.Strings; import org.elasticsearch.common.UUIDs; @@ -143,7 +144,7 @@ public MetaDataCreateIndexService( * 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); + Names.validateTopLevelName(index, InvalidIndexNameException::new); if (!index.toLowerCase(Locale.ROOT).equals(index)) { throw new InvalidIndexNameException(index, "must be lowercase"); } diff --git a/server/src/main/java/org/elasticsearch/common/Names.java b/server/src/main/java/org/elasticsearch/common/Names.java new file mode 100644 index 0000000000000..67cd89aa81303 --- /dev/null +++ b/server/src/main/java/org/elasticsearch/common/Names.java @@ -0,0 +1,66 @@ +/* + * 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.common; + +import org.elasticsearch.ElasticsearchException; + +import java.io.UnsupportedEncodingException; +import java.util.function.BiFunction; + +/** + * A set of utilities for names. + */ +public class Names { + + public static final int MAX_INDEX_NAME_BYTES = 255; + + private Names() {} + + /** + * Validates a name by the rules used for Index names. + */ + public static void validateTopLevelName(String name, BiFunction exceptionCtor) { + if (!Strings.validFileName(name)) { + throw exceptionCtor.apply(name, "must not contain the following characters " + Strings.INVALID_FILENAME_CHARS); + } + if (name.contains("#")) { + throw exceptionCtor.apply(name, "must not contain '#'"); + } + if (name.contains(":")) { + throw exceptionCtor.apply(name, "must not contain ':'"); + } + if (name.charAt(0) == '_' || name.charAt(0) == '-' || name.charAt(0) == '+') { + throw exceptionCtor.apply(name, "must not start with '_', '-', or '+'"); + } + int byteCount = 0; + try { + byteCount = name.getBytes("UTF-8").length; + } catch (UnsupportedEncodingException e) { + // UTF-8 should always be supported, but rethrow this if it is not for some reason + throw new ElasticsearchException("Unable to determine length of name", e); + } + if (byteCount > MAX_INDEX_NAME_BYTES) { + throw exceptionCtor.apply(name, "name is too long, (" + byteCount + " > " + MAX_INDEX_NAME_BYTES + ")"); + } + if (name.equals(".") || name.equals("..")) { + throw exceptionCtor.apply(name, "must not be '.' or '..'"); + } + } +} diff --git a/server/src/test/java/org/elasticsearch/common/NamesTests.java b/server/src/test/java/org/elasticsearch/common/NamesTests.java new file mode 100644 index 0000000000000..d03192b8f1fc6 --- /dev/null +++ b/server/src/test/java/org/elasticsearch/common/NamesTests.java @@ -0,0 +1,65 @@ +/* + * 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.common; + +import org.elasticsearch.indices.InvalidAliasNameException; +import org.elasticsearch.indices.InvalidIndexNameException; +import org.elasticsearch.test.ESTestCase; +import org.junit.Before; + +import java.util.function.BiFunction; + +public class NamesTests extends ESTestCase { + + private BiFunction exceptionCtor; + + @Before + public void setExceptionCtor() { + exceptionCtor = randomFrom(InvalidIndexNameException::new, + InvalidAliasNameException::new, + (name, msg) -> new IllegalArgumentException(name + " is invalid: " + msg)); + } + + public void testValidateTopLevelName() { + for (Character badChar : Strings.INVALID_FILENAME_CHARS) { + expectThrows(RuntimeException.class, () -> Names.validateTopLevelName(randomAlphaOfLengthBetween(0, 10) + + badChar + randomAlphaOfLengthBetween(0, 10), exceptionCtor)); + } + expectThrows(RuntimeException.class, () -> Names.validateTopLevelName(randomAlphaOfLengthBetween(0, 10) + + "#" + randomAlphaOfLengthBetween(0, 10), exceptionCtor)); + expectThrows(RuntimeException.class, () -> Names.validateTopLevelName(randomAlphaOfLengthBetween(0, 10) + + ":" + randomAlphaOfLengthBetween(0, 10), exceptionCtor)); + expectThrows(RuntimeException.class, () -> Names.validateTopLevelName("_" + randomAlphaOfLengthBetween(1, 20), exceptionCtor)); + expectThrows(RuntimeException.class, () -> Names.validateTopLevelName("-" + randomAlphaOfLengthBetween(1, 20), exceptionCtor)); + expectThrows(RuntimeException.class, () -> Names.validateTopLevelName("+" + randomAlphaOfLengthBetween(1, 20), exceptionCtor)); + expectThrows(RuntimeException.class, () -> Names.validateTopLevelName(".", exceptionCtor)); + expectThrows(RuntimeException.class, () -> Names.validateTopLevelName("..", exceptionCtor)); + expectThrows(RuntimeException.class, () -> Names.validateTopLevelName(randomAlphaOfLengthBetween(256, 1000), exceptionCtor)); + + Names.validateTopLevelName(randomAlphaOfLengthBetween(1, 10) + "_" + randomAlphaOfLengthBetween(0, 10), exceptionCtor); + Names.validateTopLevelName(randomAlphaOfLengthBetween(1, 10) + "-" + randomAlphaOfLengthBetween(0, 10), exceptionCtor); + Names.validateTopLevelName(randomAlphaOfLengthBetween(1, 10) + "+" + randomAlphaOfLengthBetween(0, 10), exceptionCtor); + + Names.validateTopLevelName(randomAlphaOfLengthBetween(0, 10) + "." + randomAlphaOfLengthBetween(1, 10), exceptionCtor); + Names.validateTopLevelName(randomAlphaOfLengthBetween(0, 10) + ".." + randomAlphaOfLengthBetween(1, 10), exceptionCtor); + + Names.validateTopLevelName(randomAlphaOfLengthBetween(1, 255), exceptionCtor); + } +} diff --git a/server/src/test/java/org/elasticsearch/indexing/IndexActionIT.java b/server/src/test/java/org/elasticsearch/indexing/IndexActionIT.java index ea2bf3f7199ef..e49abdd9d90b7 100644 --- a/server/src/test/java/org/elasticsearch/indexing/IndexActionIT.java +++ b/server/src/test/java/org/elasticsearch/indexing/IndexActionIT.java @@ -198,7 +198,7 @@ public void testCreateIndexWithLongName() { fail("exception should have been thrown on too-long index name"); } catch (InvalidIndexNameException e) { assertThat("exception contains message about index name too long: " + e.getMessage(), - e.getMessage().contains("index name is too long,"), equalTo(true)); + e.getMessage().contains("name is too long,"), equalTo(true)); } try { @@ -206,7 +206,7 @@ public void testCreateIndexWithLongName() { fail("exception should have been thrown on too-long index name"); } catch (InvalidIndexNameException e) { assertThat("exception contains message about index name too long: " + e.getMessage(), - e.getMessage().contains("index name is too long,"), equalTo(true)); + e.getMessage().contains("name is too long,"), equalTo(true)); } try { @@ -217,7 +217,7 @@ public void testCreateIndexWithLongName() { fail("exception should have been thrown on too-long index name"); } catch (InvalidIndexNameException e) { assertThat("exception contains message about index name too long: " + e.getMessage(), - e.getMessage().contains("index name is too long,"), equalTo(true)); + e.getMessage(), containsString("name is too long")); } // we can create an index of max length diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicy.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicy.java index 89824f259dd42..a56818355c3e5 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicy.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicy.java @@ -7,7 +7,6 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; -import org.elasticsearch.ElasticsearchException; import org.elasticsearch.client.Client; import org.elasticsearch.cluster.AbstractDiffable; import org.elasticsearch.cluster.Diffable; @@ -22,14 +21,12 @@ import org.elasticsearch.xpack.core.indexlifecycle.Step.StepKey; import java.io.IOException; -import java.io.UnsupportedEncodingException; import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.ListIterator; import java.util.Map; import java.util.Objects; -import java.util.function.BiFunction; import java.util.function.Function; import java.util.stream.Collectors; @@ -43,7 +40,6 @@ public class LifecyclePolicy extends AbstractDiffable implements ToXContentObject, Diffable { private static final Logger logger = LogManager.getLogger(LifecyclePolicy.class); - private static final int MAX_INDEX_NAME_BYTES = 255; public static final ParseField PHASES_FIELD = new ParseField("phases"); @@ -245,42 +241,6 @@ public boolean isActionSafe(StepKey stepKey) { } } - /** - * Validate the name for an policy against some static rules. Intended to match - * {@link org.elasticsearch.cluster.metadata.MetaDataCreateIndexService#validateIndexOrAliasName(String, BiFunction)} - * @param policy the policy name to validate - * @throws IllegalArgumentException if the name is invalid - */ - public static void validatePolicyName(String policy) { - if (!Strings.validFileName(policy)) { - throw new IllegalArgumentException("invalid policy name [" + policy + "]: must not contain the following characters " + - Strings.INVALID_FILENAME_CHARS); - } - if (policy.contains("#")) { - throw new IllegalArgumentException("invalid policy name [" + policy + "]: must not contain '#'"); - } - if (policy.contains(":")) { - throw new IllegalArgumentException("invalid policy name [" + policy + "]: must not contain ':'"); - } - if (policy.charAt(0) == '_' || policy.charAt(0) == '-' || policy.charAt(0) == '+') { - throw new IllegalArgumentException("invalid policy name [" + policy + "]: must not start with '_', '-', or '+'"); - } - int byteCount = 0; - try { - byteCount = policy.getBytes("UTF-8").length; - } catch (UnsupportedEncodingException e) { - // UTF-8 should always be supported, but rethrow this if it is not for some reason - throw new ElasticsearchException("Unable to determine length of policy name", e); - } - if (byteCount > MAX_INDEX_NAME_BYTES) { - throw new IllegalArgumentException("invalid policy name [" + policy + "]: name is too long, (" + byteCount + " > " + - MAX_INDEX_NAME_BYTES + ")"); - } - if (policy.equals(".") || policy.equals("..")) { - throw new IllegalArgumentException("invalid policy name [" + policy + "]: must not be '.' or '..'"); - } - } - @Override public int hashCode() { return Objects.hash(name, phases); diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicyTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicyTests.java index 01c41fba76f41..9d90cc025b0e3 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicyTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicyTests.java @@ -9,7 +9,6 @@ import org.elasticsearch.cluster.ClusterModule; import org.elasticsearch.common.Nullable; import org.elasticsearch.common.ParseField; -import org.elasticsearch.common.Strings; import org.elasticsearch.common.io.stream.NamedWriteableRegistry; import org.elasticsearch.common.io.stream.Writeable.Reader; import org.elasticsearch.common.unit.TimeValue; @@ -317,30 +316,4 @@ public void testIsActionSafe() { assertTrue(policy.isActionSafe(new StepKey("new", randomAlphaOfLength(10), randomAlphaOfLength(10)))); } - - public void testValidatePolicyName() { - for (Character badChar : Strings.INVALID_FILENAME_CHARS) { - expectThrows(IllegalArgumentException.class, () -> LifecyclePolicy.validatePolicyName(randomAlphaOfLengthBetween(0,10) + - badChar + randomAlphaOfLengthBetween(0,10))); - } - expectThrows(IllegalArgumentException.class, () -> LifecyclePolicy.validatePolicyName(randomAlphaOfLengthBetween(0,10) + - "#" + randomAlphaOfLengthBetween(0,10))); - expectThrows(IllegalArgumentException.class, () -> LifecyclePolicy.validatePolicyName(randomAlphaOfLengthBetween(0,10) + - ":" + randomAlphaOfLengthBetween(0,10))); - expectThrows(IllegalArgumentException.class, () -> LifecyclePolicy.validatePolicyName("_" + randomAlphaOfLengthBetween(1, 20))); - expectThrows(IllegalArgumentException.class, () -> LifecyclePolicy.validatePolicyName("-" + randomAlphaOfLengthBetween(1, 20))); - expectThrows(IllegalArgumentException.class, () -> LifecyclePolicy.validatePolicyName("+" + randomAlphaOfLengthBetween(1, 20))); - expectThrows(IllegalArgumentException.class, () -> LifecyclePolicy.validatePolicyName(".")); - expectThrows(IllegalArgumentException.class, () -> LifecyclePolicy.validatePolicyName("..")); - expectThrows(IllegalArgumentException.class, () -> LifecyclePolicy.validatePolicyName(randomAlphaOfLengthBetween(256, 1000))); - - LifecyclePolicy.validatePolicyName(randomAlphaOfLengthBetween(1,10) + "_" + randomAlphaOfLengthBetween(0,10)); - LifecyclePolicy.validatePolicyName(randomAlphaOfLengthBetween(1,10) + "-" + randomAlphaOfLengthBetween(0,10)); - LifecyclePolicy.validatePolicyName(randomAlphaOfLengthBetween(1,10) + "+" + randomAlphaOfLengthBetween(0,10)); - - LifecyclePolicy.validatePolicyName(randomAlphaOfLengthBetween(0,10) + "." + randomAlphaOfLengthBetween(1,10)); - LifecyclePolicy.validatePolicyName(randomAlphaOfLengthBetween(0,10) + ".." + randomAlphaOfLengthBetween(1,10)); - - LifecyclePolicy.validatePolicyName(randomAlphaOfLengthBetween(1,255)); - } } diff --git a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/indexlifecycle/action/TransportPutLifecycleAction.java b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/indexlifecycle/action/TransportPutLifecycleAction.java index 9d3dc8fbca190..f35eea94292e5 100644 --- a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/indexlifecycle/action/TransportPutLifecycleAction.java +++ b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/indexlifecycle/action/TransportPutLifecycleAction.java @@ -16,13 +16,13 @@ import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver; import org.elasticsearch.cluster.metadata.MetaData; import org.elasticsearch.cluster.service.ClusterService; +import org.elasticsearch.common.Names; import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.transport.TransportService; import org.elasticsearch.xpack.core.ClientHelper; import org.elasticsearch.xpack.core.indexlifecycle.IndexLifecycleMetadata; -import org.elasticsearch.xpack.core.indexlifecycle.LifecyclePolicy; import org.elasticsearch.xpack.core.indexlifecycle.LifecyclePolicyMetadata; import org.elasticsearch.xpack.core.indexlifecycle.OperationMode; import org.elasticsearch.xpack.core.indexlifecycle.action.PutLifecycleAction; @@ -30,6 +30,7 @@ import org.elasticsearch.xpack.core.indexlifecycle.action.PutLifecycleAction.Response; import java.time.Instant; +import java.util.Locale; import java.util.Map; import java.util.SortedMap; import java.util.TreeMap; @@ -67,7 +68,8 @@ protected void masterOperation(Request request, ClusterState state, ActionListen Map filteredHeaders = threadPool.getThreadContext().getHeaders().entrySet().stream() .filter(e -> ClientHelper.SECURITY_HEADER_FILTERS.contains(e.getKey())) .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)); - LifecyclePolicy.validatePolicyName(request.getPolicy().getName()); + Names.validateTopLevelName(request.getPolicy().getName(), + (name, message) -> new IllegalArgumentException(String.format(Locale.ROOT, "invalid policy name [%s]: %s", name, message))); clusterService.submitStateUpdateTask("put-lifecycle-" + request.getPolicy().getName(), new AckedClusterStateUpdateTask(request, listener) { @Override From 18276bff0cf7f740aee97e5b3847eb0dc9ad9525 Mon Sep 17 00:00:00 2001 From: Gordon Brown Date: Wed, 31 Oct 2018 19:04:54 -0600 Subject: [PATCH 05/11] A bit of extra cleanup that I missed --- .../cluster/metadata/AliasValidator.java | 2 +- .../metadata/MetaDataCreateIndexService.java | 2 +- .../java/org/elasticsearch/common/Names.java | 4 +-- .../org/elasticsearch/common/NamesTests.java | 30 +++++++++---------- .../action/TransportPutLifecycleAction.java | 2 +- 5 files changed, 20 insertions(+), 20 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/AliasValidator.java b/server/src/main/java/org/elasticsearch/cluster/metadata/AliasValidator.java index 4e831b35a532d..0f4d5b99b136b 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/AliasValidator.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/AliasValidator.java @@ -106,7 +106,7 @@ void validateAliasStandalone(String alias, String indexRouting) { if (!Strings.hasText(alias)) { throw new IllegalArgumentException("alias name is required"); } - Names.validateTopLevelName(alias, InvalidAliasNameException::new); + Names.validateNameWithIndexNameRules(alias, InvalidAliasNameException::new); if (indexRouting != null && indexRouting.indexOf(',') != -1) { throw new IllegalArgumentException("alias [" + alias + "] has several index routing values associated with it"); } diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java index ab99a3b4a0062..4bad88a7f38f9 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java @@ -146,7 +146,7 @@ public MetaDataCreateIndexService( * Validate the name for an index against some static rules and a cluster state. */ public static void validateIndexName(String index, ClusterState state) { - Names.validateTopLevelName(index, InvalidIndexNameException::new); + Names.validateNameWithIndexNameRules(index, InvalidIndexNameException::new); if (!index.toLowerCase(Locale.ROOT).equals(index)) { throw new InvalidIndexNameException(index, "must be lowercase"); } diff --git a/server/src/main/java/org/elasticsearch/common/Names.java b/server/src/main/java/org/elasticsearch/common/Names.java index 67cd89aa81303..726158d624694 100644 --- a/server/src/main/java/org/elasticsearch/common/Names.java +++ b/server/src/main/java/org/elasticsearch/common/Names.java @@ -34,9 +34,9 @@ public class Names { private Names() {} /** - * Validates a name by the rules used for Index names. + * Validates a name by the rules used for Index names. Used for some things that are not indexes for consistency. */ - public static void validateTopLevelName(String name, BiFunction exceptionCtor) { + public static void validateNameWithIndexNameRules(String name, BiFunction exceptionCtor) { if (!Strings.validFileName(name)) { throw exceptionCtor.apply(name, "must not contain the following characters " + Strings.INVALID_FILENAME_CHARS); } diff --git a/server/src/test/java/org/elasticsearch/common/NamesTests.java b/server/src/test/java/org/elasticsearch/common/NamesTests.java index d03192b8f1fc6..bd94951dca210 100644 --- a/server/src/test/java/org/elasticsearch/common/NamesTests.java +++ b/server/src/test/java/org/elasticsearch/common/NamesTests.java @@ -39,27 +39,27 @@ public void setExceptionCtor() { public void testValidateTopLevelName() { for (Character badChar : Strings.INVALID_FILENAME_CHARS) { - expectThrows(RuntimeException.class, () -> Names.validateTopLevelName(randomAlphaOfLengthBetween(0, 10) + + expectThrows(RuntimeException.class, () -> Names.validateNameWithIndexNameRules(randomAlphaOfLengthBetween(0, 10) + badChar + randomAlphaOfLengthBetween(0, 10), exceptionCtor)); } - expectThrows(RuntimeException.class, () -> Names.validateTopLevelName(randomAlphaOfLengthBetween(0, 10) + + expectThrows(RuntimeException.class, () -> Names.validateNameWithIndexNameRules(randomAlphaOfLengthBetween(0, 10) + "#" + randomAlphaOfLengthBetween(0, 10), exceptionCtor)); - expectThrows(RuntimeException.class, () -> Names.validateTopLevelName(randomAlphaOfLengthBetween(0, 10) + + expectThrows(RuntimeException.class, () -> Names.validateNameWithIndexNameRules(randomAlphaOfLengthBetween(0, 10) + ":" + randomAlphaOfLengthBetween(0, 10), exceptionCtor)); - expectThrows(RuntimeException.class, () -> Names.validateTopLevelName("_" + randomAlphaOfLengthBetween(1, 20), exceptionCtor)); - expectThrows(RuntimeException.class, () -> Names.validateTopLevelName("-" + randomAlphaOfLengthBetween(1, 20), exceptionCtor)); - expectThrows(RuntimeException.class, () -> Names.validateTopLevelName("+" + randomAlphaOfLengthBetween(1, 20), exceptionCtor)); - expectThrows(RuntimeException.class, () -> Names.validateTopLevelName(".", exceptionCtor)); - expectThrows(RuntimeException.class, () -> Names.validateTopLevelName("..", exceptionCtor)); - expectThrows(RuntimeException.class, () -> Names.validateTopLevelName(randomAlphaOfLengthBetween(256, 1000), exceptionCtor)); + expectThrows(RuntimeException.class, () -> Names.validateNameWithIndexNameRules("_" + randomAlphaOfLengthBetween(1, 20), exceptionCtor)); + expectThrows(RuntimeException.class, () -> Names.validateNameWithIndexNameRules("-" + randomAlphaOfLengthBetween(1, 20), exceptionCtor)); + expectThrows(RuntimeException.class, () -> Names.validateNameWithIndexNameRules("+" + randomAlphaOfLengthBetween(1, 20), exceptionCtor)); + expectThrows(RuntimeException.class, () -> Names.validateNameWithIndexNameRules(".", exceptionCtor)); + expectThrows(RuntimeException.class, () -> Names.validateNameWithIndexNameRules("..", exceptionCtor)); + expectThrows(RuntimeException.class, () -> Names.validateNameWithIndexNameRules(randomAlphaOfLengthBetween(256, 1000), exceptionCtor)); - Names.validateTopLevelName(randomAlphaOfLengthBetween(1, 10) + "_" + randomAlphaOfLengthBetween(0, 10), exceptionCtor); - Names.validateTopLevelName(randomAlphaOfLengthBetween(1, 10) + "-" + randomAlphaOfLengthBetween(0, 10), exceptionCtor); - Names.validateTopLevelName(randomAlphaOfLengthBetween(1, 10) + "+" + randomAlphaOfLengthBetween(0, 10), exceptionCtor); + Names.validateNameWithIndexNameRules(randomAlphaOfLengthBetween(1, 10) + "_" + randomAlphaOfLengthBetween(0, 10), exceptionCtor); + Names.validateNameWithIndexNameRules(randomAlphaOfLengthBetween(1, 10) + "-" + randomAlphaOfLengthBetween(0, 10), exceptionCtor); + Names.validateNameWithIndexNameRules(randomAlphaOfLengthBetween(1, 10) + "+" + randomAlphaOfLengthBetween(0, 10), exceptionCtor); - Names.validateTopLevelName(randomAlphaOfLengthBetween(0, 10) + "." + randomAlphaOfLengthBetween(1, 10), exceptionCtor); - Names.validateTopLevelName(randomAlphaOfLengthBetween(0, 10) + ".." + randomAlphaOfLengthBetween(1, 10), exceptionCtor); + Names.validateNameWithIndexNameRules(randomAlphaOfLengthBetween(0, 10) + "." + randomAlphaOfLengthBetween(1, 10), exceptionCtor); + Names.validateNameWithIndexNameRules(randomAlphaOfLengthBetween(0, 10) + ".." + randomAlphaOfLengthBetween(1, 10), exceptionCtor); - Names.validateTopLevelName(randomAlphaOfLengthBetween(1, 255), exceptionCtor); + Names.validateNameWithIndexNameRules(randomAlphaOfLengthBetween(1, 255), exceptionCtor); } } diff --git a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/indexlifecycle/action/TransportPutLifecycleAction.java b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/indexlifecycle/action/TransportPutLifecycleAction.java index f35eea94292e5..17de14f6f7c84 100644 --- a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/indexlifecycle/action/TransportPutLifecycleAction.java +++ b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/indexlifecycle/action/TransportPutLifecycleAction.java @@ -68,7 +68,7 @@ protected void masterOperation(Request request, ClusterState state, ActionListen Map filteredHeaders = threadPool.getThreadContext().getHeaders().entrySet().stream() .filter(e -> ClientHelper.SECURITY_HEADER_FILTERS.contains(e.getKey())) .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)); - Names.validateTopLevelName(request.getPolicy().getName(), + Names.validateNameWithIndexNameRules(request.getPolicy().getName(), (name, message) -> new IllegalArgumentException(String.format(Locale.ROOT, "invalid policy name [%s]: %s", name, message))); clusterService.submitStateUpdateTask("put-lifecycle-" + request.getPolicy().getName(), new AckedClusterStateUpdateTask(request, listener) { From ebd551d911271adc6919be62fb2e46032916fc40 Mon Sep 17 00:00:00 2001 From: Gordon Brown Date: Wed, 31 Oct 2018 19:47:14 -0600 Subject: [PATCH 06/11] More cleanup --- .../java/org/elasticsearch/common/NamesTests.java | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/common/NamesTests.java b/server/src/test/java/org/elasticsearch/common/NamesTests.java index bd94951dca210..772c149f4d04e 100644 --- a/server/src/test/java/org/elasticsearch/common/NamesTests.java +++ b/server/src/test/java/org/elasticsearch/common/NamesTests.java @@ -46,12 +46,16 @@ public void testValidateTopLevelName() { "#" + randomAlphaOfLengthBetween(0, 10), exceptionCtor)); expectThrows(RuntimeException.class, () -> Names.validateNameWithIndexNameRules(randomAlphaOfLengthBetween(0, 10) + ":" + randomAlphaOfLengthBetween(0, 10), exceptionCtor)); - expectThrows(RuntimeException.class, () -> Names.validateNameWithIndexNameRules("_" + randomAlphaOfLengthBetween(1, 20), exceptionCtor)); - expectThrows(RuntimeException.class, () -> Names.validateNameWithIndexNameRules("-" + randomAlphaOfLengthBetween(1, 20), exceptionCtor)); - expectThrows(RuntimeException.class, () -> Names.validateNameWithIndexNameRules("+" + randomAlphaOfLengthBetween(1, 20), exceptionCtor)); + expectThrows(RuntimeException.class, () -> Names.validateNameWithIndexNameRules("_" + randomAlphaOfLengthBetween(1, 20), + exceptionCtor)); + expectThrows(RuntimeException.class, () -> Names.validateNameWithIndexNameRules("-" + randomAlphaOfLengthBetween(1, 20), + exceptionCtor)); + expectThrows(RuntimeException.class, () -> Names.validateNameWithIndexNameRules("+" + randomAlphaOfLengthBetween(1, 20), + exceptionCtor)); expectThrows(RuntimeException.class, () -> Names.validateNameWithIndexNameRules(".", exceptionCtor)); expectThrows(RuntimeException.class, () -> Names.validateNameWithIndexNameRules("..", exceptionCtor)); - expectThrows(RuntimeException.class, () -> Names.validateNameWithIndexNameRules(randomAlphaOfLengthBetween(256, 1000), exceptionCtor)); + expectThrows(RuntimeException.class, () -> Names.validateNameWithIndexNameRules(randomAlphaOfLengthBetween(256, 1000), + exceptionCtor)); Names.validateNameWithIndexNameRules(randomAlphaOfLengthBetween(1, 10) + "_" + randomAlphaOfLengthBetween(0, 10), exceptionCtor); Names.validateNameWithIndexNameRules(randomAlphaOfLengthBetween(1, 10) + "-" + randomAlphaOfLengthBetween(0, 10), exceptionCtor); From 92db083ee58ea56d730317f3a1db42e7a607b7fb Mon Sep 17 00:00:00 2001 From: Gordon Brown Date: Wed, 31 Oct 2018 21:02:57 -0600 Subject: [PATCH 07/11] Back out changes to shared classes --- .../cluster/metadata/AliasValidator.java | 3 +- .../metadata/MetaDataCreateIndexService.java | 3 +- .../java/org/elasticsearch/common/Names.java | 66 ------------------ .../org/elasticsearch/common/NamesTests.java | 69 ------------------- .../elasticsearch/indexing/IndexActionIT.java | 6 +- .../core/indexlifecycle/LifecyclePolicy.java | 40 +++++++++++ .../indexlifecycle/LifecyclePolicyTests.java | 27 ++++++++ .../action/TransportPutLifecycleAction.java | 6 +- 8 files changed, 74 insertions(+), 146 deletions(-) delete mode 100644 server/src/main/java/org/elasticsearch/common/Names.java delete mode 100644 server/src/test/java/org/elasticsearch/common/NamesTests.java diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/AliasValidator.java b/server/src/main/java/org/elasticsearch/cluster/metadata/AliasValidator.java index 0f4d5b99b136b..766b35307cdd2 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/AliasValidator.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/AliasValidator.java @@ -20,7 +20,6 @@ package org.elasticsearch.cluster.metadata; import org.elasticsearch.action.admin.indices.alias.Alias; -import org.elasticsearch.common.Names; import org.elasticsearch.common.Nullable; import org.elasticsearch.common.Strings; import org.elasticsearch.common.component.AbstractComponent; @@ -106,7 +105,7 @@ void validateAliasStandalone(String alias, String indexRouting) { if (!Strings.hasText(alias)) { throw new IllegalArgumentException("alias name is required"); } - Names.validateNameWithIndexNameRules(alias, InvalidAliasNameException::new); + MetaDataCreateIndexService.validateIndexOrAliasName(alias, InvalidAliasNameException::new); if (indexRouting != null && indexRouting.indexOf(',') != -1) { throw new IllegalArgumentException("alias [" + alias + "] has several index routing values associated with it"); } diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java index 4bad88a7f38f9..41d57323a0597 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java @@ -47,7 +47,6 @@ import org.elasticsearch.cluster.routing.ShardRoutingState; import org.elasticsearch.cluster.routing.allocation.AllocationService; import org.elasticsearch.cluster.service.ClusterService; -import org.elasticsearch.common.Names; import org.elasticsearch.common.Priority; import org.elasticsearch.common.Strings; import org.elasticsearch.common.UUIDs; @@ -146,7 +145,7 @@ public MetaDataCreateIndexService( * Validate the name for an index against some static rules and a cluster state. */ public static void validateIndexName(String index, ClusterState state) { - Names.validateNameWithIndexNameRules(index, InvalidIndexNameException::new); + validateIndexOrAliasName(index, InvalidIndexNameException::new); if (!index.toLowerCase(Locale.ROOT).equals(index)) { throw new InvalidIndexNameException(index, "must be lowercase"); } diff --git a/server/src/main/java/org/elasticsearch/common/Names.java b/server/src/main/java/org/elasticsearch/common/Names.java deleted file mode 100644 index 726158d624694..0000000000000 --- a/server/src/main/java/org/elasticsearch/common/Names.java +++ /dev/null @@ -1,66 +0,0 @@ -/* - * 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.common; - -import org.elasticsearch.ElasticsearchException; - -import java.io.UnsupportedEncodingException; -import java.util.function.BiFunction; - -/** - * A set of utilities for names. - */ -public class Names { - - public static final int MAX_INDEX_NAME_BYTES = 255; - - private Names() {} - - /** - * Validates a name by the rules used for Index names. Used for some things that are not indexes for consistency. - */ - public static void validateNameWithIndexNameRules(String name, BiFunction exceptionCtor) { - if (!Strings.validFileName(name)) { - throw exceptionCtor.apply(name, "must not contain the following characters " + Strings.INVALID_FILENAME_CHARS); - } - if (name.contains("#")) { - throw exceptionCtor.apply(name, "must not contain '#'"); - } - if (name.contains(":")) { - throw exceptionCtor.apply(name, "must not contain ':'"); - } - if (name.charAt(0) == '_' || name.charAt(0) == '-' || name.charAt(0) == '+') { - throw exceptionCtor.apply(name, "must not start with '_', '-', or '+'"); - } - int byteCount = 0; - try { - byteCount = name.getBytes("UTF-8").length; - } catch (UnsupportedEncodingException e) { - // UTF-8 should always be supported, but rethrow this if it is not for some reason - throw new ElasticsearchException("Unable to determine length of name", e); - } - if (byteCount > MAX_INDEX_NAME_BYTES) { - throw exceptionCtor.apply(name, "name is too long, (" + byteCount + " > " + MAX_INDEX_NAME_BYTES + ")"); - } - if (name.equals(".") || name.equals("..")) { - throw exceptionCtor.apply(name, "must not be '.' or '..'"); - } - } -} diff --git a/server/src/test/java/org/elasticsearch/common/NamesTests.java b/server/src/test/java/org/elasticsearch/common/NamesTests.java deleted file mode 100644 index 772c149f4d04e..0000000000000 --- a/server/src/test/java/org/elasticsearch/common/NamesTests.java +++ /dev/null @@ -1,69 +0,0 @@ -/* - * 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.common; - -import org.elasticsearch.indices.InvalidAliasNameException; -import org.elasticsearch.indices.InvalidIndexNameException; -import org.elasticsearch.test.ESTestCase; -import org.junit.Before; - -import java.util.function.BiFunction; - -public class NamesTests extends ESTestCase { - - private BiFunction exceptionCtor; - - @Before - public void setExceptionCtor() { - exceptionCtor = randomFrom(InvalidIndexNameException::new, - InvalidAliasNameException::new, - (name, msg) -> new IllegalArgumentException(name + " is invalid: " + msg)); - } - - public void testValidateTopLevelName() { - for (Character badChar : Strings.INVALID_FILENAME_CHARS) { - expectThrows(RuntimeException.class, () -> Names.validateNameWithIndexNameRules(randomAlphaOfLengthBetween(0, 10) + - badChar + randomAlphaOfLengthBetween(0, 10), exceptionCtor)); - } - expectThrows(RuntimeException.class, () -> Names.validateNameWithIndexNameRules(randomAlphaOfLengthBetween(0, 10) + - "#" + randomAlphaOfLengthBetween(0, 10), exceptionCtor)); - expectThrows(RuntimeException.class, () -> Names.validateNameWithIndexNameRules(randomAlphaOfLengthBetween(0, 10) + - ":" + randomAlphaOfLengthBetween(0, 10), exceptionCtor)); - expectThrows(RuntimeException.class, () -> Names.validateNameWithIndexNameRules("_" + randomAlphaOfLengthBetween(1, 20), - exceptionCtor)); - expectThrows(RuntimeException.class, () -> Names.validateNameWithIndexNameRules("-" + randomAlphaOfLengthBetween(1, 20), - exceptionCtor)); - expectThrows(RuntimeException.class, () -> Names.validateNameWithIndexNameRules("+" + randomAlphaOfLengthBetween(1, 20), - exceptionCtor)); - expectThrows(RuntimeException.class, () -> Names.validateNameWithIndexNameRules(".", exceptionCtor)); - expectThrows(RuntimeException.class, () -> Names.validateNameWithIndexNameRules("..", exceptionCtor)); - expectThrows(RuntimeException.class, () -> Names.validateNameWithIndexNameRules(randomAlphaOfLengthBetween(256, 1000), - exceptionCtor)); - - Names.validateNameWithIndexNameRules(randomAlphaOfLengthBetween(1, 10) + "_" + randomAlphaOfLengthBetween(0, 10), exceptionCtor); - Names.validateNameWithIndexNameRules(randomAlphaOfLengthBetween(1, 10) + "-" + randomAlphaOfLengthBetween(0, 10), exceptionCtor); - Names.validateNameWithIndexNameRules(randomAlphaOfLengthBetween(1, 10) + "+" + randomAlphaOfLengthBetween(0, 10), exceptionCtor); - - Names.validateNameWithIndexNameRules(randomAlphaOfLengthBetween(0, 10) + "." + randomAlphaOfLengthBetween(1, 10), exceptionCtor); - Names.validateNameWithIndexNameRules(randomAlphaOfLengthBetween(0, 10) + ".." + randomAlphaOfLengthBetween(1, 10), exceptionCtor); - - Names.validateNameWithIndexNameRules(randomAlphaOfLengthBetween(1, 255), exceptionCtor); - } -} diff --git a/server/src/test/java/org/elasticsearch/indexing/IndexActionIT.java b/server/src/test/java/org/elasticsearch/indexing/IndexActionIT.java index e49abdd9d90b7..ea2bf3f7199ef 100644 --- a/server/src/test/java/org/elasticsearch/indexing/IndexActionIT.java +++ b/server/src/test/java/org/elasticsearch/indexing/IndexActionIT.java @@ -198,7 +198,7 @@ public void testCreateIndexWithLongName() { fail("exception should have been thrown on too-long index name"); } catch (InvalidIndexNameException e) { assertThat("exception contains message about index name too long: " + e.getMessage(), - e.getMessage().contains("name is too long,"), equalTo(true)); + e.getMessage().contains("index name is too long,"), equalTo(true)); } try { @@ -206,7 +206,7 @@ public void testCreateIndexWithLongName() { fail("exception should have been thrown on too-long index name"); } catch (InvalidIndexNameException e) { assertThat("exception contains message about index name too long: " + e.getMessage(), - e.getMessage().contains("name is too long,"), equalTo(true)); + e.getMessage().contains("index name is too long,"), equalTo(true)); } try { @@ -217,7 +217,7 @@ public void testCreateIndexWithLongName() { fail("exception should have been thrown on too-long index name"); } catch (InvalidIndexNameException e) { assertThat("exception contains message about index name too long: " + e.getMessage(), - e.getMessage(), containsString("name is too long")); + e.getMessage().contains("index name is too long,"), equalTo(true)); } // we can create an index of max length diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicy.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicy.java index a56818355c3e5..89824f259dd42 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicy.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicy.java @@ -7,6 +7,7 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import org.elasticsearch.ElasticsearchException; import org.elasticsearch.client.Client; import org.elasticsearch.cluster.AbstractDiffable; import org.elasticsearch.cluster.Diffable; @@ -21,12 +22,14 @@ import org.elasticsearch.xpack.core.indexlifecycle.Step.StepKey; import java.io.IOException; +import java.io.UnsupportedEncodingException; import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.ListIterator; import java.util.Map; import java.util.Objects; +import java.util.function.BiFunction; import java.util.function.Function; import java.util.stream.Collectors; @@ -40,6 +43,7 @@ public class LifecyclePolicy extends AbstractDiffable implements ToXContentObject, Diffable { private static final Logger logger = LogManager.getLogger(LifecyclePolicy.class); + private static final int MAX_INDEX_NAME_BYTES = 255; public static final ParseField PHASES_FIELD = new ParseField("phases"); @@ -241,6 +245,42 @@ public boolean isActionSafe(StepKey stepKey) { } } + /** + * Validate the name for an policy against some static rules. Intended to match + * {@link org.elasticsearch.cluster.metadata.MetaDataCreateIndexService#validateIndexOrAliasName(String, BiFunction)} + * @param policy the policy name to validate + * @throws IllegalArgumentException if the name is invalid + */ + public static void validatePolicyName(String policy) { + if (!Strings.validFileName(policy)) { + throw new IllegalArgumentException("invalid policy name [" + policy + "]: must not contain the following characters " + + Strings.INVALID_FILENAME_CHARS); + } + if (policy.contains("#")) { + throw new IllegalArgumentException("invalid policy name [" + policy + "]: must not contain '#'"); + } + if (policy.contains(":")) { + throw new IllegalArgumentException("invalid policy name [" + policy + "]: must not contain ':'"); + } + if (policy.charAt(0) == '_' || policy.charAt(0) == '-' || policy.charAt(0) == '+') { + throw new IllegalArgumentException("invalid policy name [" + policy + "]: must not start with '_', '-', or '+'"); + } + int byteCount = 0; + try { + byteCount = policy.getBytes("UTF-8").length; + } catch (UnsupportedEncodingException e) { + // UTF-8 should always be supported, but rethrow this if it is not for some reason + throw new ElasticsearchException("Unable to determine length of policy name", e); + } + if (byteCount > MAX_INDEX_NAME_BYTES) { + throw new IllegalArgumentException("invalid policy name [" + policy + "]: name is too long, (" + byteCount + " > " + + MAX_INDEX_NAME_BYTES + ")"); + } + if (policy.equals(".") || policy.equals("..")) { + throw new IllegalArgumentException("invalid policy name [" + policy + "]: must not be '.' or '..'"); + } + } + @Override public int hashCode() { return Objects.hash(name, phases); diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicyTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicyTests.java index 9d90cc025b0e3..01c41fba76f41 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicyTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicyTests.java @@ -9,6 +9,7 @@ import org.elasticsearch.cluster.ClusterModule; import org.elasticsearch.common.Nullable; import org.elasticsearch.common.ParseField; +import org.elasticsearch.common.Strings; import org.elasticsearch.common.io.stream.NamedWriteableRegistry; import org.elasticsearch.common.io.stream.Writeable.Reader; import org.elasticsearch.common.unit.TimeValue; @@ -316,4 +317,30 @@ public void testIsActionSafe() { assertTrue(policy.isActionSafe(new StepKey("new", randomAlphaOfLength(10), randomAlphaOfLength(10)))); } + + public void testValidatePolicyName() { + for (Character badChar : Strings.INVALID_FILENAME_CHARS) { + expectThrows(IllegalArgumentException.class, () -> LifecyclePolicy.validatePolicyName(randomAlphaOfLengthBetween(0,10) + + badChar + randomAlphaOfLengthBetween(0,10))); + } + expectThrows(IllegalArgumentException.class, () -> LifecyclePolicy.validatePolicyName(randomAlphaOfLengthBetween(0,10) + + "#" + randomAlphaOfLengthBetween(0,10))); + expectThrows(IllegalArgumentException.class, () -> LifecyclePolicy.validatePolicyName(randomAlphaOfLengthBetween(0,10) + + ":" + randomAlphaOfLengthBetween(0,10))); + expectThrows(IllegalArgumentException.class, () -> LifecyclePolicy.validatePolicyName("_" + randomAlphaOfLengthBetween(1, 20))); + expectThrows(IllegalArgumentException.class, () -> LifecyclePolicy.validatePolicyName("-" + randomAlphaOfLengthBetween(1, 20))); + expectThrows(IllegalArgumentException.class, () -> LifecyclePolicy.validatePolicyName("+" + randomAlphaOfLengthBetween(1, 20))); + expectThrows(IllegalArgumentException.class, () -> LifecyclePolicy.validatePolicyName(".")); + expectThrows(IllegalArgumentException.class, () -> LifecyclePolicy.validatePolicyName("..")); + expectThrows(IllegalArgumentException.class, () -> LifecyclePolicy.validatePolicyName(randomAlphaOfLengthBetween(256, 1000))); + + LifecyclePolicy.validatePolicyName(randomAlphaOfLengthBetween(1,10) + "_" + randomAlphaOfLengthBetween(0,10)); + LifecyclePolicy.validatePolicyName(randomAlphaOfLengthBetween(1,10) + "-" + randomAlphaOfLengthBetween(0,10)); + LifecyclePolicy.validatePolicyName(randomAlphaOfLengthBetween(1,10) + "+" + randomAlphaOfLengthBetween(0,10)); + + LifecyclePolicy.validatePolicyName(randomAlphaOfLengthBetween(0,10) + "." + randomAlphaOfLengthBetween(1,10)); + LifecyclePolicy.validatePolicyName(randomAlphaOfLengthBetween(0,10) + ".." + randomAlphaOfLengthBetween(1,10)); + + LifecyclePolicy.validatePolicyName(randomAlphaOfLengthBetween(1,255)); + } } diff --git a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/indexlifecycle/action/TransportPutLifecycleAction.java b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/indexlifecycle/action/TransportPutLifecycleAction.java index 17de14f6f7c84..9d3dc8fbca190 100644 --- a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/indexlifecycle/action/TransportPutLifecycleAction.java +++ b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/indexlifecycle/action/TransportPutLifecycleAction.java @@ -16,13 +16,13 @@ import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver; import org.elasticsearch.cluster.metadata.MetaData; import org.elasticsearch.cluster.service.ClusterService; -import org.elasticsearch.common.Names; import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.transport.TransportService; import org.elasticsearch.xpack.core.ClientHelper; import org.elasticsearch.xpack.core.indexlifecycle.IndexLifecycleMetadata; +import org.elasticsearch.xpack.core.indexlifecycle.LifecyclePolicy; import org.elasticsearch.xpack.core.indexlifecycle.LifecyclePolicyMetadata; import org.elasticsearch.xpack.core.indexlifecycle.OperationMode; import org.elasticsearch.xpack.core.indexlifecycle.action.PutLifecycleAction; @@ -30,7 +30,6 @@ import org.elasticsearch.xpack.core.indexlifecycle.action.PutLifecycleAction.Response; import java.time.Instant; -import java.util.Locale; import java.util.Map; import java.util.SortedMap; import java.util.TreeMap; @@ -68,8 +67,7 @@ protected void masterOperation(Request request, ClusterState state, ActionListen Map filteredHeaders = threadPool.getThreadContext().getHeaders().entrySet().stream() .filter(e -> ClientHelper.SECURITY_HEADER_FILTERS.contains(e.getKey())) .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)); - Names.validateNameWithIndexNameRules(request.getPolicy().getName(), - (name, message) -> new IllegalArgumentException(String.format(Locale.ROOT, "invalid policy name [%s]: %s", name, message))); + LifecyclePolicy.validatePolicyName(request.getPolicy().getName()); clusterService.submitStateUpdateTask("put-lifecycle-" + request.getPolicy().getName(), new AckedClusterStateUpdateTask(request, listener) { @Override From ea7db68b47f54a69338437b3f23dcf538d1e6a19 Mon Sep 17 00:00:00 2001 From: Gordon Brown Date: Fri, 2 Nov 2018 12:18:53 -0600 Subject: [PATCH 08/11] Switch to looser set of name restrictions --- .../core/indexlifecycle/LifecyclePolicy.java | 16 +++---------- .../indexlifecycle/LifecyclePolicyTests.java | 19 +++------------ .../TimeSeriesLifecycleActionsIT.java | 24 +++---------------- 3 files changed, 9 insertions(+), 50 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicy.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicy.java index 89824f259dd42..17994f0ab5641 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicy.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicy.java @@ -252,17 +252,10 @@ public boolean isActionSafe(StepKey stepKey) { * @throws IllegalArgumentException if the name is invalid */ public static void validatePolicyName(String policy) { - if (!Strings.validFileName(policy)) { - throw new IllegalArgumentException("invalid policy name [" + policy + "]: must not contain the following characters " + - Strings.INVALID_FILENAME_CHARS); + if (policy.contains(",")) { + throw new IllegalArgumentException("invalid policy name [" + policy + "]: must not contain ','"); } - if (policy.contains("#")) { - throw new IllegalArgumentException("invalid policy name [" + policy + "]: must not contain '#'"); - } - if (policy.contains(":")) { - throw new IllegalArgumentException("invalid policy name [" + policy + "]: must not contain ':'"); - } - if (policy.charAt(0) == '_' || policy.charAt(0) == '-' || policy.charAt(0) == '+') { + if (policy.charAt(0) == '_') { throw new IllegalArgumentException("invalid policy name [" + policy + "]: must not start with '_', '-', or '+'"); } int byteCount = 0; @@ -276,9 +269,6 @@ public static void validatePolicyName(String policy) { throw new IllegalArgumentException("invalid policy name [" + policy + "]: name is too long, (" + byteCount + " > " + MAX_INDEX_NAME_BYTES + ")"); } - if (policy.equals(".") || policy.equals("..")) { - throw new IllegalArgumentException("invalid policy name [" + policy + "]: must not be '.' or '..'"); - } } @Override diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicyTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicyTests.java index 01c41fba76f41..476d77c7a62c1 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicyTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicyTests.java @@ -9,7 +9,6 @@ import org.elasticsearch.cluster.ClusterModule; import org.elasticsearch.common.Nullable; import org.elasticsearch.common.ParseField; -import org.elasticsearch.common.Strings; import org.elasticsearch.common.io.stream.NamedWriteableRegistry; import org.elasticsearch.common.io.stream.Writeable.Reader; import org.elasticsearch.common.unit.TimeValue; @@ -319,27 +318,15 @@ public void testIsActionSafe() { } public void testValidatePolicyName() { - for (Character badChar : Strings.INVALID_FILENAME_CHARS) { - expectThrows(IllegalArgumentException.class, () -> LifecyclePolicy.validatePolicyName(randomAlphaOfLengthBetween(0,10) + - badChar + randomAlphaOfLengthBetween(0,10))); - } - expectThrows(IllegalArgumentException.class, () -> LifecyclePolicy.validatePolicyName(randomAlphaOfLengthBetween(0,10) + - "#" + randomAlphaOfLengthBetween(0,10))); expectThrows(IllegalArgumentException.class, () -> LifecyclePolicy.validatePolicyName(randomAlphaOfLengthBetween(0,10) + - ":" + randomAlphaOfLengthBetween(0,10))); + "," + randomAlphaOfLengthBetween(0,10))); expectThrows(IllegalArgumentException.class, () -> LifecyclePolicy.validatePolicyName("_" + randomAlphaOfLengthBetween(1, 20))); - expectThrows(IllegalArgumentException.class, () -> LifecyclePolicy.validatePolicyName("-" + randomAlphaOfLengthBetween(1, 20))); - expectThrows(IllegalArgumentException.class, () -> LifecyclePolicy.validatePolicyName("+" + randomAlphaOfLengthBetween(1, 20))); - expectThrows(IllegalArgumentException.class, () -> LifecyclePolicy.validatePolicyName(".")); - expectThrows(IllegalArgumentException.class, () -> LifecyclePolicy.validatePolicyName("..")); expectThrows(IllegalArgumentException.class, () -> LifecyclePolicy.validatePolicyName(randomAlphaOfLengthBetween(256, 1000))); LifecyclePolicy.validatePolicyName(randomAlphaOfLengthBetween(1,10) + "_" + randomAlphaOfLengthBetween(0,10)); - LifecyclePolicy.validatePolicyName(randomAlphaOfLengthBetween(1,10) + "-" + randomAlphaOfLengthBetween(0,10)); - LifecyclePolicy.validatePolicyName(randomAlphaOfLengthBetween(1,10) + "+" + randomAlphaOfLengthBetween(0,10)); - LifecyclePolicy.validatePolicyName(randomAlphaOfLengthBetween(0,10) + "." + randomAlphaOfLengthBetween(1,10)); - LifecyclePolicy.validatePolicyName(randomAlphaOfLengthBetween(0,10) + ".." + randomAlphaOfLengthBetween(1,10)); + LifecyclePolicy.validatePolicyName(randomAlphaOfLengthBetween(0,10) + "-" + randomAlphaOfLengthBetween(0,10)); + LifecyclePolicy.validatePolicyName(randomAlphaOfLengthBetween(0,10) + "+" + randomAlphaOfLengthBetween(0,10)); LifecyclePolicy.validatePolicyName(randomAlphaOfLengthBetween(1,255)); } diff --git a/x-pack/plugin/ilm/qa/multi-node/src/test/java/org/elasticsearch/xpack/indexlifecycle/TimeSeriesLifecycleActionsIT.java b/x-pack/plugin/ilm/qa/multi-node/src/test/java/org/elasticsearch/xpack/indexlifecycle/TimeSeriesLifecycleActionsIT.java index 2d10ab994259b..cb2bd32beaf01 100644 --- a/x-pack/plugin/ilm/qa/multi-node/src/test/java/org/elasticsearch/xpack/indexlifecycle/TimeSeriesLifecycleActionsIT.java +++ b/x-pack/plugin/ilm/qa/multi-node/src/test/java/org/elasticsearch/xpack/indexlifecycle/TimeSeriesLifecycleActionsIT.java @@ -36,7 +36,6 @@ import java.io.IOException; import java.io.InputStream; import java.io.UnsupportedEncodingException; -import java.net.URLEncoder; import java.util.Collections; import java.util.HashMap; import java.util.List; @@ -370,29 +369,12 @@ public void testNonexistentPolicy() throws Exception { public void testInvalidPolicyNames() throws UnsupportedEncodingException { ResponseException ex; - for (Character badChar : Strings.INVALID_FILENAME_CHARS) { - policy = URLEncoder.encode(randomAlphaOfLengthBetween(0,10) + badChar + randomAlphaOfLengthBetween(0,10), "UTF-8"); - ex = expectThrows(ResponseException.class, () -> createNewSingletonPolicy("delete", new DeleteAction())); - assertThat(ex.getCause().getMessage(), containsString("invalid policy name")); - } - policy = randomAlphaOfLengthBetween(0,10) + ":" + randomAlphaOfLengthBetween(0,10); - ex = expectThrows(ResponseException.class, () -> createNewSingletonPolicy("delete", new DeleteAction())); - assertThat(ex.getMessage(), containsString("invalid policy name")); - policy = "_" + randomAlphaOfLengthBetween(1, 20); - ex = expectThrows(ResponseException.class, () -> createNewSingletonPolicy("delete", new DeleteAction())); - assertThat(ex.getMessage(), containsString("invalid policy name")); - policy = "-" + randomAlphaOfLengthBetween(1, 20); + policy = randomAlphaOfLengthBetween(0,10) + "," + randomAlphaOfLengthBetween(0,10); ex = expectThrows(ResponseException.class, () -> createNewSingletonPolicy("delete", new DeleteAction())); - assertThat(ex.getMessage(), containsString("invalid policy name")); - policy = "+" + randomAlphaOfLengthBetween(1, 20); - ex = expectThrows(ResponseException.class, () -> createNewSingletonPolicy("delete", new DeleteAction())); - assertThat(ex.getMessage(), containsString("invalid policy name")); + assertThat(ex.getCause().getMessage(), containsString("invalid policy name")); - policy = "."; - ex = expectThrows(ResponseException.class, () -> createNewSingletonPolicy("delete", new DeleteAction())); - assertThat(ex.getMessage(), containsString("invalid policy name")); - policy = ".."; + policy = "_" + randomAlphaOfLengthBetween(1, 20); ex = expectThrows(ResponseException.class, () -> createNewSingletonPolicy("delete", new DeleteAction())); assertThat(ex.getMessage(), containsString("invalid policy name")); From bc8b1ed96dc32ae1bb06a78167b9064b9eff2fb6 Mon Sep 17 00:00:00 2001 From: Gordon Brown Date: Mon, 5 Nov 2018 14:38:30 -0700 Subject: [PATCH 09/11] Use UTF-8 Charset instead of "UTF-8" --- .../xpack/core/indexlifecycle/LifecyclePolicy.java | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicy.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicy.java index 17994f0ab5641..aaf2ec831ee1b 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicy.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicy.java @@ -7,7 +7,6 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; -import org.elasticsearch.ElasticsearchException; import org.elasticsearch.client.Client; import org.elasticsearch.cluster.AbstractDiffable; import org.elasticsearch.cluster.Diffable; @@ -22,7 +21,7 @@ import org.elasticsearch.xpack.core.indexlifecycle.Step.StepKey; import java.io.IOException; -import java.io.UnsupportedEncodingException; +import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.Collections; import java.util.List; @@ -259,12 +258,7 @@ public static void validatePolicyName(String policy) { throw new IllegalArgumentException("invalid policy name [" + policy + "]: must not start with '_', '-', or '+'"); } int byteCount = 0; - try { - byteCount = policy.getBytes("UTF-8").length; - } catch (UnsupportedEncodingException e) { - // UTF-8 should always be supported, but rethrow this if it is not for some reason - throw new ElasticsearchException("Unable to determine length of policy name", e); - } + byteCount = policy.getBytes(StandardCharsets.UTF_8).length; if (byteCount > MAX_INDEX_NAME_BYTES) { throw new IllegalArgumentException("invalid policy name [" + policy + "]: name is too long, (" + byteCount + " > " + MAX_INDEX_NAME_BYTES + ")"); From a0f7a9269e747c24e02c4e0c2f399ad176594fcb Mon Sep 17 00:00:00 2001 From: Gordon Brown Date: Tue, 6 Nov 2018 16:40:04 -0700 Subject: [PATCH 10/11] Restrict spaces in policy names --- .../xpack/core/indexlifecycle/LifecyclePolicy.java | 3 +++ .../xpack/core/indexlifecycle/LifecyclePolicyTests.java | 2 ++ .../xpack/indexlifecycle/TimeSeriesLifecycleActionsIT.java | 4 ++++ 3 files changed, 9 insertions(+) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicy.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicy.java index aaf2ec831ee1b..b887694a16840 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicy.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicy.java @@ -254,6 +254,9 @@ public static void validatePolicyName(String policy) { if (policy.contains(",")) { throw new IllegalArgumentException("invalid policy name [" + policy + "]: must not contain ','"); } + if (policy.contains(" ")) { + throw new IllegalArgumentException("invalid policy name [" + policy + "]: must not contain spaces"); + } if (policy.charAt(0) == '_') { throw new IllegalArgumentException("invalid policy name [" + policy + "]: must not start with '_', '-', or '+'"); } diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicyTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicyTests.java index 476d77c7a62c1..f61d0189782d7 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicyTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicyTests.java @@ -320,6 +320,8 @@ public void testIsActionSafe() { public void testValidatePolicyName() { expectThrows(IllegalArgumentException.class, () -> LifecyclePolicy.validatePolicyName(randomAlphaOfLengthBetween(0,10) + "," + randomAlphaOfLengthBetween(0,10))); + expectThrows(IllegalArgumentException.class, () -> LifecyclePolicy.validatePolicyName(randomAlphaOfLengthBetween(0,10) + + " " + randomAlphaOfLengthBetween(0,10))); expectThrows(IllegalArgumentException.class, () -> LifecyclePolicy.validatePolicyName("_" + randomAlphaOfLengthBetween(1, 20))); expectThrows(IllegalArgumentException.class, () -> LifecyclePolicy.validatePolicyName(randomAlphaOfLengthBetween(256, 1000))); diff --git a/x-pack/plugin/ilm/qa/multi-node/src/test/java/org/elasticsearch/xpack/indexlifecycle/TimeSeriesLifecycleActionsIT.java b/x-pack/plugin/ilm/qa/multi-node/src/test/java/org/elasticsearch/xpack/indexlifecycle/TimeSeriesLifecycleActionsIT.java index f34b168e587fb..94609128c94c3 100644 --- a/x-pack/plugin/ilm/qa/multi-node/src/test/java/org/elasticsearch/xpack/indexlifecycle/TimeSeriesLifecycleActionsIT.java +++ b/x-pack/plugin/ilm/qa/multi-node/src/test/java/org/elasticsearch/xpack/indexlifecycle/TimeSeriesLifecycleActionsIT.java @@ -407,6 +407,10 @@ public void testInvalidPolicyNames() throws UnsupportedEncodingException { policy = randomAlphaOfLengthBetween(0,10) + "," + randomAlphaOfLengthBetween(0,10); ex = expectThrows(ResponseException.class, () -> createNewSingletonPolicy("delete", new DeleteAction())); assertThat(ex.getCause().getMessage(), containsString("invalid policy name")); + + policy = randomAlphaOfLengthBetween(0,10) + "%20" + randomAlphaOfLengthBetween(0,10); + ex = expectThrows(ResponseException.class, () -> createNewSingletonPolicy("delete", new DeleteAction())); + assertThat(ex.getCause().getMessage(), containsString("invalid policy name")); policy = "_" + randomAlphaOfLengthBetween(1, 20); ex = expectThrows(ResponseException.class, () -> createNewSingletonPolicy("delete", new DeleteAction())); From 6c58f5e0b15ff682ac3f42a6a8359edf3813b0d4 Mon Sep 17 00:00:00 2001 From: Gordon Brown Date: Wed, 7 Nov 2018 11:56:12 -0700 Subject: [PATCH 11/11] Fix error message with leftover constraints --- .../xpack/core/indexlifecycle/LifecyclePolicy.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicy.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicy.java index b887694a16840..380f9888668b6 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicy.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicy.java @@ -258,7 +258,7 @@ public static void validatePolicyName(String policy) { throw new IllegalArgumentException("invalid policy name [" + policy + "]: must not contain spaces"); } if (policy.charAt(0) == '_') { - throw new IllegalArgumentException("invalid policy name [" + policy + "]: must not start with '_', '-', or '+'"); + throw new IllegalArgumentException("invalid policy name [" + policy + "]: must not start with '_'"); } int byteCount = 0; byteCount = policy.getBytes(StandardCharsets.UTF_8).length;