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

Un-hardcode SecurityIndexManager to handle generic indices #40064

Conversation

albertzaharovits
Copy link
Contributor

SecurityIndexManager is hardcoded to handle only the .security-.security-7 alias-index pair.
This commit removes the hardcoded bits, so that the SecurityIndexManager can be reused for other indices, such as the planned token index (.security-tokens-7).

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security

@jaymode jaymode requested a review from tvernum March 14, 2019 18:57
Copy link
Contributor

@tvernum tvernum left a comment

Choose a reason for hiding this comment

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

I'm good with this as a step forward, but I think we're going to need to split the class again (see #30526) if we want to manage multiple indices.

We can tackle that problem when we get to it, but I think it's going to get messy when we have 2 instances of this class being injected into constructors of other classes (and there's a few of them), and calls to checkIndexVersionThenExecute (etc) will need to make sure they're hitting the correct instance for the index that they're after.

Even something simple like making the injected component be

public class SecurityIndices() {

   public SecurityIndexManager() mainIndex() { ... }
   public SecurityIndexManager() tokenIndex() { ... }
}

We don't need to decide now, but my prediction is that when you have two SecurityIndexManager instances floating around, we'll want to do something to bring a bit more clarity to it.

@albertzaharovits
Copy link
Contributor Author

albertzaharovits commented Mar 17, 2019

You're right Tim that having a SecurityIndexManager for the tokens index is confusing, especially when you have another SecurityIndexManager for the security index floating around.

My plan was to have the two of them hidden behind static getters similar to your suggestion, although I don't know how it would play out with #30526 (comment) (if it is still relevant). Another thing to consider is that there would not be both "floating around" because the tokens one would be buried inside the TokenService when that's enabled ... possibly... I'll see to it in the follow-up.

@albertzaharovits albertzaharovits merged commit 3844125 into elastic:master Mar 17, 2019
albertzaharovits added a commit that referenced this pull request Mar 17, 2019
`SecurityIndexManager` is hardcoded to handle only the `.security`-`.security-7` alias-index pair.
This commit removes the hardcoded bits, so that the `SecurityIndexManager` can be reused
for other indices, such as the planned security tokens index (`.security-tokens-7`).
@albertzaharovits albertzaharovits deleted the security_index_manager_refactor_2 branch March 17, 2019 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants