Enforce limitations on ILM policy names #35104

gwbrown · 2018-10-30T19:33:09Z

Enforces restrictions on ILM policy names to ensure we don't accept
policy names the system can't handle, or may reserve for future use.

As of now, the limitations are:

Length < 255 bytes
Cannot start with _
Cannot contain , (as we allow comma-separated names in e.g.
GET _ilm/one,two - currently you can PUT, but not GET
a policy named contains,comma as far as I can tell, although
other special characters work fine)
Cannot contain spaces.

Resolves #34928

For consistency, ILM policy names must now follow the same rules as
index names.

I ended up copying most of the logic, as there was a little bit of index-specific
code in the original, and also it felt weird to call intoMetaDataCreateIndexService
from here. If anyone strongly objects it would be possible to do this without any
duplication, though, with only minor tweaks to the index-name checking code.

For consistency, ILM policy names must now follow the same rules as index names.

elasticmachine · 2018-10-30T19:33:14Z

Pinging @elastic/es-core-infra

colings86

@gwbrown I left some comments

...k/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicy.java

...gin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicyTests.java

server/src/main/java/org/elasticsearch/common/Names.java

jasontedor

We are still discussing on #34928 whether or not this is the right restriction to apply, if any. Please do not merge this until that discussion is resolved.

gwbrown · 2018-11-01T03:37:47Z

Marking this as WIP for now as more review is pointless until we decide on a higher-level direction.

martijnvg · 2018-11-05T06:43:48Z

...k/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicy.java

+        }
+        int byteCount = 0;
+        try {
+            byteCount = policy.getBytes("UTF-8").length;


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.

colings86

I left one small comment but otherwise this LGTM

colings86 · 2018-11-07T02:12:31Z

...k/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicy.java

+        }
+        if (policy.charAt(0) == '_') {
+            throw new IllegalArgumentException("invalid policy name [" + policy + "]: must not start with '_', '-', or '+'");
+        }


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?

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.

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

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

ok lets leave it as is then

I'm on board with leaving this as separate messages.

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

colings86 · 2018-11-07T02:14:39Z

Now ILM is merged into master and 6.x we should label the PRs with non-issue and the relevant version labels so its easy to see what versions the changes made it into

jasontedor · 2018-11-07T02:21:21Z

My understanding, and is how we've been operating in CCR, is that we should also leave the version labels off of these as they show up in the weekly changelog otherwise. We want to continue to not report these individual changes until after the feature is delivered. (They are meaningless until then.)

colings86 · 2018-11-07T02:35:46Z

I am ok with that approach for now given thats how the weekly changelog works but I think we should discuss how this should be for the future as I think its useful to be able to see on PRs what versions a change went into quickly regardless of whether the PR is listed on changelogs/release notes

jasontedor · 2018-11-07T12:44:47Z

I am ok with that approach for now given thats how the weekly changelog works but I think we should discuss how this should be for the future as I think its useful to be able to see on PRs what versions a change went into quickly regardless of whether the PR is listed on changelogs/release notes

Let's offline that, that's not germane to this PR.

Enforces restrictions on ILM policy names to ensure we don't accept policy names the system can't handle, or may reserve for future use.

Enforce index-name rules for ILM policy names

8370a0c

For consistency, ILM policy names must now follow the same rules as index names.

gwbrown added the :Data Management/ILM+SLM Index and Snapshot lifecycle management label Oct 30, 2018

gwbrown requested review from dakrone, colings86 and talevy October 30, 2018 19:33

gwbrown added the blocker label Oct 30, 2018

Missed one "index"->"policy"

782379d

colings86 requested changes Oct 31, 2018

View reviewed changes

...k/plugin/core/src/main/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicy.java Outdated Show resolved Hide resolved

...gin/core/src/test/java/org/elasticsearch/xpack/core/indexlifecycle/LifecyclePolicyTests.java Show resolved Hide resolved

gwbrown added 4 commits October 31, 2018 13:29

Add tests for valid policy names

27ce24b

Refactor name checks into a separate class

4e3d989

Merge branch 'index-lifecycle' into ilm/enforce-policy-name-restrictions

d8cd5f7

A bit of extra cleanup that I missed

18276bf

gwbrown commented Nov 1, 2018

View reviewed changes

server/src/main/java/org/elasticsearch/common/Names.java Outdated Show resolved Hide resolved

More cleanup

ebd551d

jasontedor requested changes Nov 1, 2018

View reviewed changes

Back out changes to shared classes

92db083

gwbrown added the WIP label Nov 1, 2018

gwbrown changed the base branch from index-lifecycle to master November 2, 2018 13:59

gwbrown added 4 commits November 2, 2018 07:59

Merge branch 'master' into ilm/enforce-policy-name-restrictions

a95d940

Switch to looser set of name restrictions

ea7db68

Merge branch 'master' into ilm/enforce-policy-name-restrictions

1d5f20d

Merge branch 'master' into ilm/enforce-policy-name-restrictions

246328f

martijnvg reviewed Nov 5, 2018

View reviewed changes

gwbrown added 2 commits November 5, 2018 14:22

Merge branch 'master' into ilm/enforce-policy-name-restrictions

97e5701

Use UTF-8 Charset instead of "UTF-8"

bc8b1ed

gwbrown changed the title ~~Enforce index-name rules for ILM policy names~~ Enforce limitations on ILM policy names Nov 5, 2018

gwbrown removed the WIP label Nov 5, 2018

gwbrown mentioned this pull request Nov 5, 2018

Enforce index-name rules for ILM policy names #34928

Closed

gwbrown added 2 commits November 6, 2018 15:29

Merge branch 'master' into ilm/enforce-policy-name-restrictions

b2be88a

Restrict spaces in policy names

a0f7a92

colings86 approved these changes Nov 7, 2018

View reviewed changes

gwbrown added 2 commits November 7, 2018 11:56

Fix error message with leftover constraints

6c58f5e

Merge branch 'master' into ilm/enforce-policy-name-restrictions

0ff4c4f

gwbrown added the >non-issue label Nov 7, 2018

Merge branch 'master' into ilm/enforce-policy-name-restrictions

a941312

gwbrown merged commit 67f9e8f into elastic:master Nov 9, 2018

gwbrown added the backport pending label Nov 9, 2018

gwbrown mentioned this pull request Nov 9, 2018

[Backport] Enforce limitations on ILM policy names (#35104) #35417

Merged

gwbrown added a commit that referenced this pull request Nov 12, 2018

Enforce limitations on ILM policy names (#35104)

0607178

Enforces restrictions on ILM policy names to ensure we don't accept policy names the system can't handle, or may reserve for future use.

gwbrown removed the backport pending label Nov 12, 2018

gwbrown deleted the ilm/enforce-policy-name-restrictions branch December 7, 2018 04:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforce limitations on ILM policy names #35104

Enforce limitations on ILM policy names #35104

gwbrown commented Oct 30, 2018 •

edited

Loading

elasticmachine commented Oct 30, 2018

colings86 left a comment

jasontedor left a comment

gwbrown commented Nov 1, 2018

martijnvg Nov 5, 2018

gwbrown Nov 5, 2018

colings86 left a comment

colings86 Nov 7, 2018

jasontedor Nov 7, 2018

colings86 Nov 7, 2018

jasontedor Nov 7, 2018

colings86 Nov 7, 2018

gwbrown Nov 7, 2018

colings86 commented Nov 7, 2018

jasontedor commented Nov 7, 2018

colings86 commented Nov 7, 2018

jasontedor commented Nov 7, 2018

Enforce limitations on ILM policy names #35104

Enforce limitations on ILM policy names #35104

Conversation

gwbrown commented Oct 30, 2018 • edited Loading

elasticmachine commented Oct 30, 2018

colings86 left a comment

Choose a reason for hiding this comment

jasontedor left a comment

Choose a reason for hiding this comment

gwbrown commented Nov 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colings86 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colings86 commented Nov 7, 2018

jasontedor commented Nov 7, 2018

colings86 commented Nov 7, 2018

jasontedor commented Nov 7, 2018

gwbrown commented Oct 30, 2018 •

edited

Loading