-
Notifications
You must be signed in to change notification settings - Fork 274
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
[FEATURE] Decouple security configuration from user data storage #2443
Comments
[Triage] @scrawfor99 Would you mind taking a look? |
Update: I have started looking at resolving this issue and the associated one linked. I am going through the steps to reproduce the problem and then will start looking at the best solution. Edit: I have reproduced and am now looking at InternalAuthBackend to see about splitting the configuration storage. Proposed changes 1: For splitting, I am considering adding a Edit 2: Linking some design work for a potential correction of how we store configuration settings. The new intention is to go further then the proposed changes 1 and actually construct an improved storage system for settings. Linking here |
I like your proposal and I would go even farther, I think we should store the new index |
@peternied What is a metadata index? |
Finally did my homework... thanks for bearing with me How it works todaySystemIndexPlugin is the mechanism to declare index(es) that are used by a plugin. The Security Plugin is using this capability already. The Security Plugin uses a single index, shard'ed onto every node, with N documents, a document is a json version of the yml configuration files in the repository. ConfigurationRepository handles initialization and retrieval/caching, the schema modeling is handled by ConfigurationLoaderSecurity7. DynamicConfigFactory listens for change updates from ConfigurationRepository and then push them out via a pub/sub event bus. AbstractApiAction is the base action for security plugin actions, for any put/delete action a saveAnUpdateConfigs(...) call puts an updates document in the index, and then TransportConfigUpdateAction fans out to all nodes so ConfigurationRepository flushes its cache and reloads. The Security Plugin stores its configuration data on the index in a in-memory cache, which it can also use to create instances of its objects dynamically. Cache invalidation issues are avoided by invalidating the entire cache whenever a request could possibly have updated the security configuration. Potential problemAs the amounts of mutable data grows in the Security Plugin, invalidation will happen more often and take longer to resolve. That data also has to be held in memory and higher JVM memory pressure takes down nodes. To date I have seen several anecdotes around tight loops interacting with security objects, but not what I would classify as evidence to justify a change. Potential next steps
|
@peternied thank you for your detailed notes on this. I have updated the Permission Storage design I was working on to include the parts that overlap with what you found. Am I correct in understanding your third sentence in the "Potential Problem" section, that you would not want to split the storage apart anymore? You mention that you don't see "evidence to justify a change", so should I pivot from solving this similarly to the permission handling? |
I still think we should store the user data separately, but I'm not as convinced what the drawbacks are with the existing implementation. If we wanted to explore the implementation I've highlighted how I think we could investigate it. If we had a pull request in-hand reusing the existing storage concepts for a separate user data storage I wouldn't request changes around these design elements without more more data. |
Hi @peternied, thank you for the clarification. I made some notes over on the linked document about the current limitations with the way that user data is stored from a permissions perspective. In short we have a lot of reloads of the entire cluster, and are inefficient in how they add data. For a specific permissions-related example, there are already 5 sub-evaluators being passed to the PrivilegesEvaluator as new types of actions get added. Each of these corresponds to another plugin which needs to have its permissions stored. This already seems to be adding a lot of overhead and code debt to the process of adding plugins. If we roll out extensions and encourage people to contribute their own then we are only going to run into having more and more mini-evaluators. I know that this thread is about more than permissions but those are currently the configurations I am most familiar with. I will start looking into some of the restrictions for other configurations and update this issue as I uncover details. |
@scrawfor99 Looks like your comment was truncated |
Hi @peternied, oops. I don't have another example yet but will update when I do haha. |
Is your feature request related to a problem?
Initially reported in #1842
The implication of this is that if any information about a user is changed, such as a password being modified or a permission added the plugin is reloaded on all nodes.
What solution would you like?
The internal identity provider should have its configuration data stored in a separate configuration index from the configuration values that cause the whole plugin to reload itself.
The text was updated successfully, but these errors were encountered: