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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Enforce limitations on ILM policy names #35104

Merged
merged 19 commits into from
Nov 9, 2018
Merged

Enforce limitations on ILM policy names #35104

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
8370a0c
Enforce index-name rules for ILM policy names
gwbrown Oct 30, 2018
782379d
Missed one "index"->"policy"
gwbrown Oct 30, 2018
27ce24b
Add tests for valid policy names
gwbrown Oct 31, 2018
4e3d989
Refactor name checks into a separate class
gwbrown Nov 1, 2018
d8cd5f7
Merge branch 'index-lifecycle' into ilm/enforce-policy-name-restrictions
gwbrown Nov 1, 2018
18276bf
A bit of extra cleanup that I missed
gwbrown Nov 1, 2018
ebd551d
More cleanup
gwbrown Nov 1, 2018
92db083
Back out changes to shared classes
gwbrown Nov 1, 2018
a95d940
Merge branch 'master' into ilm/enforce-policy-name-restrictions
gwbrown Nov 2, 2018
ea7db68
Switch to looser set of name restrictions
gwbrown Nov 2, 2018
1d5f20d
Merge branch 'master' into ilm/enforce-policy-name-restrictions
gwbrown Nov 2, 2018
246328f
Merge branch 'master' into ilm/enforce-policy-name-restrictions
gwbrown Nov 2, 2018
97e5701
Merge branch 'master' into ilm/enforce-policy-name-restrictions
gwbrown Nov 5, 2018
bc8b1ed
Use UTF-8 Charset instead of "UTF-8"
gwbrown Nov 5, 2018
b2be88a
Merge branch 'master' into ilm/enforce-policy-name-restrictions
gwbrown Nov 6, 2018
a0f7a92
Restrict spaces in policy names
gwbrown Nov 6, 2018
6c58f5e
Fix error message with leftover constraints
gwbrown Nov 7, 2018
0ff4c4f
Merge branch 'master' into ilm/enforce-policy-name-restrictions
gwbrown Nov 7, 2018
a941312
Merge branch 'master' into ilm/enforce-policy-name-restrictions
gwbrown Nov 8, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions ...lugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

Expand All @@ -40,6 +43,7 @@
public class LifecyclePolicy extends AbstractDiffable<LifecyclePolicy>
implements ToXContentObject, Diffable<LifecyclePolicy> {
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");

Expand Down Expand Up @@ -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 '+'");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it would be nicer for the user if we just have a single message here that describes all the characters that we don't allow in a policy name? This might avoid frustration if the user tries something like _foo bar as the policy name since they will be able to fix everything in one step rather than two?

Copy link
Member

Choose a reason for hiding this comment

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

Only if we don't end up with something too complicated. As-is, it's simple to read and validate that it follows the rules when encoded this way. The rules aren't so onerous that I expect any and certainly not multiple to be broken on a routine basis, they will be the exception. We can document the rules in the ILM API guide too, that would help.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking it would be ok as

"invalid policy name [" + policy + "]: must not start with '_', '-', or '+'" and must not contain any spaces or `,`."

But I don't feel too strongly about this, I agree the likelihood of multiple violations is low since there are too many rules here

Copy link
Member

Choose a reason for hiding this comment

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

I would argue that's harder to consume for the case of a single violation (the common case contingent on any violation) as now as a user I am left wondering, wait, which rule did I break?

It's like those awful password dialog boxes. Password doesn't meet the required criteria:

  • must be between 8 and 16 characters on Monday--Friday
  • must be between 7 and 17 characters on weekends
  • contains none of !@*&#
  • contains at least two of ()_+-
  • contains at least four digits
  • contains at least one upper-case letter
  • contains at least two lower-case letters
  • not contain three of the same character
  • not contain any adjacent repeating characters
  • not contain adjacent consecutive digits or letters
  • unless in reverse order

Copy link
Contributor

Choose a reason for hiding this comment

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

ok lets leave it as is then

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. I'm on board with leaving this as separate messages.
  2. This particular message was out of date, I've updated it to only say that policy names cannot start with _ (the other characters are okay).

int byteCount = 0;
try {
byteCount = policy.getBytes("UTF-8").length;
Copy link
Member

Choose a reason for hiding this comment

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

I think this is cleaner: name.getBytes(StandardCharsets.UTF_8) and then there is no need to catch UnsupportedEncodingException.

Also I think that utf-8 should always be available in a Java runtime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

} 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 '..'");
}
gwbrown marked this conversation as resolved.
Show resolved Hide resolved
}

@Override
public int hashCode() {
return Objects.hash(name, phases);
Expand Down
18 changes: 18 additions & 0 deletions .../core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicyTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)));
gwbrown marked this conversation as resolved.
Show resolved Hide resolved
}
}
36 changes: 36 additions & 0 deletions ...de/src/test/java/org/elasticsearch/xpack/indexlifecycle/TimeSeriesLifecycleActionsIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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<String, LifecycleAction> warmActions = new HashMap<>();
warmActions.put(ForceMergeAction.NAME, new ForceMergeAction(1));
Expand Down
4 changes: 3 additions & 1 deletion .../main/java/org/elasticsearch/xpack/indexlifecycle/action/TransportPutLifecycleAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -66,6 +67,7 @@ protected void masterOperation(Request request, ClusterState state, ActionListen
Map<String, String> 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<Response>(request, listener) {
@Override
Expand Down