Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

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

Already on GitHub? Sign in to your account

Enforce index-name rules for ILM policy names #34928

Closed
talevy opened this issue Oct 26, 2018 · 14 comments
Closed

Enforce index-name rules for ILM policy names #34928

talevy opened this issue Oct 26, 2018 · 14 comments
Assignees
Labels
blocker :Data Management/ILM+SLM Index and Snapshot lifecycle management

Comments

@talevy
Copy link
Contributor

talevy commented Oct 26, 2018

Index names have restrictions due to inconveniences to dealing with things like really long index names, or commas, or "_" prefixed names.

ILM Policies should adhere to the same naming conventions

@talevy talevy added blocker :Data Management/ILM+SLM Index and Snapshot lifecycle management labels Oct 26, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@jasontedor
Copy link
Member

I am curious what the motivation for this is? We are not making the same restriction in CCR with the names of auto-follow patterns (I'm not saying ILM is wrong, maybe CCR should be following the lead (pun!) here).

@dakrone
Copy link
Member

dakrone commented Oct 30, 2018

@jasontedor I think it stems from trying to cut off any potential issues before they happen, really long names (causing URL issues for browsers), commas and _ prefixed names are some of the ones. I think it makes sense to try and enforce the rules for naming "things" (index names, ids, etc) with the same consistent rules so that they are different in different parts of the software.

@jasontedor
Copy link
Member

A few comments:

  • we have inconsistent rules for IDs all over the system
    • e.g., alias names can contain uppercase letters, index names can not
    • e.g., datafeed IDs are not subject to the same rules as index names
    • e.g., document IDs can contain spaces, index names can not
    • e.g., repository names
    • ...
  • rules for index names are to a certain extent legacy from the fact that the name of the index use to be the name of the directory on disk
  • we have received negative feedback on the limitations that we apply on index names today

The goal of having consistent names is noble, but this isn't giving us that. It's tying us to a naming scheme with quite some legacy behind it. It doesn't make the system any more predictable.

My feeling is unless something horrible is going to happen without a specific restriction, then the restrictions we enforce here will appear arbitrary. If we feel strongly that we should apply a restriction, then it should be one that is easy to understand. Index naming is borderline incomprehensible, because of its legacy in filesystem limitations.

@tvernum
Copy link
Contributor

tvernum commented Nov 1, 2018

we have received negative feedback on the limitations that we apply on index names today

My anecdotal experience is that most of the negative feedback is about changing the limitations.
If we launched features with clear, documented and consistent naming limitations from the start we would get less push back. e.g. It would be better to reject unreasonably long names now than have to introducing a breaking change in the future when we realise they cause problems somewhere.

@jasontedor
Copy link
Member

If we launched features with clear, documented and consistent naming limitations from the start we would get less push back.

I almost completely agree with this. What is missing for me is simple, and index naming limitations are not. As I said:

If we feel strongly that we should apply a restriction, then it should be one that is easy to understand.

To be clear, I am okay with a limitation, but index naming limitations are not the right one for me.

@dakrone
Copy link
Member

dakrone commented Nov 1, 2018

To be clear, I am okay with a limitation, but index naming limitations are not the right one for me.

That's fine with me too. I am in favor of a limitation for the reasons Tim mentioned as well, and since we had an existing one it seemed right to use it. We can discuss alternative limitations here if we want to go with that.

@colings86
Copy link
Contributor

The original motivation for this change was based on limiting the length of the policy name and rejecting policy names starting with _ (since we tend to reserve _X in API endpoints to prevent conflicts with other APIs. Maybe we should just enforce these rules only?

Another point that was raised was around users creating policies with potential problematic characters (e.g. a policy named 😀. We could further restrict the names to ascii characters but I'm not sure if that is too far at this stage?

@gwbrown
Copy link
Contributor

gwbrown commented Nov 1, 2018

I'm for a narrower set of restrictions - the index name restrictions do include rather a lot that we don't need.

As a first cut, how does this sound:

  1. Length < 255 bytes (or another reasonable value)
  2. Cannot start with _
  3. 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)

Anything else we should restrict? Other comments?

@colings86
Copy link
Contributor

I'm happy to go with the above restrictions

@dakrone
Copy link
Member

dakrone commented Nov 2, 2018

Are we okay with spaces in the policy name? (we currently allow it, just want to make sure we're fine with that)

@gwbrown
Copy link
Contributor

gwbrown commented Nov 2, 2018

I haven't seen any issues in some (fairly minimal) manual testing with spaces in policy names. The only reason I can see for disallowing spaces is consistency, which could very well be a valid argument.

@gwbrown
Copy link
Contributor

gwbrown commented Nov 5, 2018

Since there have been no objections to the limitations I posted above, I've updated #35104 to apply those restrictions, and also edited the title and description to reflect that it no longer enforces index-name restrictions.

Does anyone have any more input on whether we should add "no spaces" to the list of limitations? I don't believe there are any (current) technical reasons, but we typically do not allow spaces in identifiers, and consistency may well be a good enough reason to disallow them. I could go either way.

@jasontedor
Copy link
Member

Let us remove spaces too, they are not worth the trouble. We do not need to worry about, for example, policy names coming from an external system that might be allowing spaces (like, say, document IDs).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocker :Data Management/ILM+SLM Index and Snapshot lifecycle management
Projects
None yet
Development

No branches or pull requests

7 participants