-
Notifications
You must be signed in to change notification settings - Fork 358
[RFC] Healthy amount of configuration options #5624
Description
Introduction
This RFC is about the software engineering practices applied for the security plugin. Its goal is to establish practises that ensure that we have a product that is both adequatly maintainable, testable and operable.
It is about the question: When shall we introduce config options for features? And when shall we possibly remove config options for features?
Example
In my recent work, I had often work with the class SystemIndexAccessEvaluator: https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/privileges/SystemIndexAccessEvaluator.java
This class provides quite a few config options that can be specified in opensearch.yml:
- plugins.security.config_index_name
- plugins.security.filter_securityindex_from_all_requests
- plugins.security.system_indices.indices
- plugins.security.system_indices.enabled
- plugins.security.unsupported.restore.securityindex.enabled
- plugins.security.system_indices.permission.enabled
Additional configurable state is indirectly sourced from the core SystemIndexRegistry and the ActionPrivileges.
Especially the options system_indices.enabled, system_indices.permission.enabled and filter_securityindex_from_all_requests interact quite heavily in the implementation.
This leads to the situation that we get many possible cases. Just for testing the method evaluateSystemIndicesAccess() thoroughly, one would need to check any combinations of the following cases:
- plugins.security.system_indices.enabled is enabled vs disabled
- plugins.security.system_indices.permission.enabled is enabled vs disabled
- plugins.security.filter_securityindex_from_all_requests is enabled vs disabled
- Request contains system index vs request contains no system index
- Request contains security index vs no security index
- User is normal user vs user is plugin user
- Request is on all indices vs request is on specific indices
- Request is write operation vs request is read operation
- Regular user has system index permission vs has not
This leads to 2^9 = 512 different cases we'd need to understand and test (ok, some combinations of cases are impossible, but we still get a very high number of cases).
I would argue that these are too many cases. Maintaining tests which cover so many cases is difficult. And mentally understanding the code paths is even more difficult.
Considerations
We need to keep a few considerations in mind:
- Config options are useful when new code paths are introduced which might cause severe operational issues. Having the possibilty to quickly switch back to the old implementation is a good thing.
- Config options are useful to keep backwards compatibility
- Config options are useful to give users a choice
However, considerations 1 and 2 lose their validity after a while. If code has proven to be reliable, having such config options causes more friction than it provides benefit. Also, backwards compatiblity can be given up at certain points, like major releases.
The consideration of "giving users a choice" also needs to be critically considered. There are cases when giving users a choice does not make sense. For example:
- I would argue that there is no production system which changed the default name of the security config index.
- It is not clear to me what's the benefit of disabling system index permissions. Just toggling this switch won't have any effect, as additional permission configuration in the roles is neccessary to actually grant access to system indices.
Questions
- Do we want some to define some guidelines on config options?
- Do we want to have a process to retire config options?
- Are there other considerations?